Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

[Stephen Warren <swarren@wwwdotorg.org>]

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.