On 08/16/2012 12:08 AM, Alexandre Courbot wrote:
> Some device drivers (panel backlights especially) need to follow precise
> sequences for powering on and off, involving gpios, regulators, PWMs
> with a precise powering order and delays to respect between each steps.
> These sequences are board-specific, and do not belong to a particular
> driver - therefore they have been performed by board-specific hook
> functions to far.
>
> With the advent of the device tree and of ARM kernels that are not
> board-tied, we cannot rely on these board-specific hooks anymore but
> need a way to implement these sequences in a portable manner. This patch
> introduces a simple interpreter that can execute such power sequences
> encoded either as platform data or within the device tree.
> diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt
> b/Documentation/devicetree/bindings/power_seq/power_seq.txt
> +Specifying Power Sequences in the Device Tree
> +=============================================
> +In the device tree, power sequences are specified as sub-nodes of the device
> +node and reference resources declared by that device.
> +
> +For an introduction about runtime interpreted power sequences, see
> +Documentation/power/power_seq.txt and include/linux/power_seq.h.
Device tree bindings shouldn't reference Linux documentation; the
bindings are supposed to be OS-agnostic.
> +Power Sequences Structure
> +-------------------------
> +Power sequences are sub-nodes that are named such as the device driver can
> find
> +them. The driver's documentation should list the sequence names it
> recognizes.
That's a little roundabout. That might be better as simply:
Valid power sequence names are defined by each device's binding. For a
power sequence named "foo", a node named "foo-power-sequence" defines
that sequence.
> +Inside a power sequence node are sub-nodes that describe the different steps
> +of the sequence. Each step must be named sequentially, with the first step
> +named step0, the second step1, etc. Failure to follow this rule will result
> in a
> +parsing error.
Node names shouldn't be interpreted by the code; nodes should all be
named after the type of object the represent. Hence, every step should
be named just "step" for example.
The node's unit address (@0) should be used to distinguish the nodes.
This requires reg properties within each node to match the unit address,
and hence #address-cells and #size-cells properties in the power
sequence itself.
Note that this somewhat conflicts with accessing the top-level power
sequence by name too; perhaps that should be re-thought too. I must
admit this DT rule kinda sucks.
> +Power Sequences Steps
> +---------------------
> +Every step of a sequence describes an action to be performed on a resource.
> It
> +generally includes a property named after the resource type, and which value
> +references the resource to be used. Depending on the resource type,
> additional
> +properties can be defined to control the action to be performed.
I think you need to add a property that indicates what type of resource
each step applies to. Sure, this is implicit in that if a "gpio"
property exists, the step is a GPIO step, but in order to make that
work, you'd have to search each node for each possible resource type's
property name. It'd be far better to read a single type="gpio" property,
then parse the step based on that.
> +Example
> +-------
> +Here are example sequences declared within a backlight device that use all
> the
> +supported resources types:
> +
> + backlight {
> + compatible = "pwm-backlight";
> + ...
> +
> + /* resources used by the sequences */
> + pwms = <&pwm 2 5000000>;
> + pwm-names = "backlight";
> + power-supply = <&backlight_reg>;
> + enable-gpio = <&gpio 28 0>;
> +
> + power-on-sequence {
> + step0 {
> + regulator = "power";
> + enable;
> + };
> + step1 {
> + delay = <10000>;
> + };
> + step2 {
> + pwm = "backlight";
> + enable;
> + };
> + step3 {
> + gpio = "enable";
> + enable;
> + };
> + };
> +
> + power-off-sequence {
> + step0 {
> + gpio = "enable";
> + disable;
> + };
> + step1 {
> + pwm = "backlight";
> + disable;
> + };
> + step2 {
> + delay = <10000>;
> + };
> + step3 {
> + regulator = "power";
> + disable;
> + };
> + };
> + };
I notice that for clocks, pwms, and interrupts, the initial step of the
lookup is via a single property that lists all know resources of that
type. Regulators and GPIOs don't follow this style though. Using the
same mechanism for power-sequences would yield something like the
following, which would avoid the "node names must be significant" issue
I mention above, although it does make everything rather more wordy.
[start my proposal]
> backlight {
> compatible = "pwm-backlight";
>
> /* resources used by the sequences */
> pwms = <&pwm 2 5000000>;
> pwm-names = "backlight";
> power-supply = <&backlight_reg>;
> bl-enable-gpio = <&gpio 28 0>;
> pwr-seqs = <&bl_on &bl_off>;
> pwr-seq-names = "on", "off";
>
> #address-cells = <1>;
> #size-cells = <0>;
>
> bl_on: pwr-seq@0 {
> reg = <0>;
> #address-cells = <1>;
> #size-cells = <0>;
> step@0 {
> reg = <0>;
> type = "regulator";
> regulator = "power";
> enable;
> };
> step@1 {
> reg = <1>;
> type = "delay";
> delay = <10000>;
> };
> step@2 {
> reg = <2>;
> type = "pwm";
> pwm = "backlight";
> enable;
> };
> step@3 {
> reg = <3>;
> type = "gpio";
> gpio = "bl-enable";
> enable;
> };
> };
>
> bl_off: pwr-seq@1 {
> reg = <1>;
> #address-cells = <1>;
> #size-cells = <0>;
> step@0 {
> reg = <0>;
> type = "gpio";
> gpio = "bl-enable";
> disable;
> };
> step@1 {
> reg = <1>;
> type = "pwm";
> pwm = "backlight";
> disable;
> };
> step@2 {
> reg = <2>;
> type = "delay";
> delay = <10000>;
> };
> step@3 {
> reg = <3>;
> type = "regulator";
> regulator = "power";
> disable;
> };
> };
> };
>
[end my proposal]
> diff --git a/Documentation/power/power_seq.txt
> b/Documentation/power/power_seq.txt
> +Usage by Drivers and Resources Management
> +-----------------------------------------
> +Power sequences make use of resources that must be properly allocated and
> +managed. The power_seq_build() function builds a power sequence from the
> +platform data. It also takes care of resolving and allocating the resources
> +referenced by the sequence if needed:
> +
> + struct power_seq *power_seq_build(struct device *dev, struct list_head
> *ress,
> + struct platform_power_seq *pseq);
> +
> +The 'dev' argument is the device in the name of which the resources are to be
> +allocated.
> +
> +The 'ress' argument is a list to which the resolved resources are appended.
> This
> +avoids allocating a resource referenced in several power sequences multiple
> +times.
I see in other parts of the thread, there has been discussion re:
needing the separate ress parameter, and requiring the driver to pass
this in to multiple power_seq_build calls.
The way the pinctrl subsystem solved this was to have a single function
that parsed all pinctrl setting (c.f. all power sequences) at once, and
return a single handle. Later, the driver passes this handle
pinctrl_lookup_state(), along with the requested state (c.f. sequence
name) to search for, and finally passes that handle to
pinctrl_select_state(). Doing something similar here would result in:
struct power_seqs *power_seq_get(struct device *dev);
void power_seq_put(struct power_seqs *seqs);
struct power_seq *power_seq_lookup(struct power_seqs *seqs,
const char *name);
int power_seq_executestruct power_seq *seq);
struct power_seqs *devm_power_seq_get(struct device *dev);
void devm_power_seq_put(struct power_seqs *seqs);
> +On success, the function returns a devm allocated resolved sequence that is
Perhaps the function should be named devm_power_seq_build(), in order to
make this obvious to people who only read the client code, and not this
documentation.
> +ready to be passed to power_seq_run(). In case of failure, and error code is
> +returned.
> +
> +A resolved power sequence returned by power_seq_build can be run by
> +power_run_run():
> +
> + int power_seq_run(power_seq *seq);
> +
> +It returns 0 if the sequence has successfully been run, or an error code if a
> +problem occured.
> +
> +There is no need to explicitly free the resources used by the sequence as
> they
> +are devm-allocated.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
