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

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.

+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]

+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.