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

On 8/16/2012 8:38 AM, Stephen Warren wrote:

On 08/16/2012 12:08 AM, Alexandre Courbot wrote:

quoted

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.

quoted

diff --git a/Documentation/devicetree/bindings/power_seq/power_seq.txt b/Documentation/devicetree/bindings/power_seq/power_seq.txt

quoted

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

While it is true that bindings should try to be OS-agnostic, there is
the practical matter of where to put documentation so that it is widely
accessible.  The Linux source tree is one of the most accessible things
there is, considering how widely it is replicated.

As the original instigator of the policy that the device tree should
describe the hardware "OS-neutrally", I personally don't have a problem
with bindings referring to Linux documentation.  I wouldn't like
references to proprietary and inaccessible documentation.

quoted

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

quoted

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

quoted

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

quoted

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

quoted

	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]

quoted

diff --git a/Documentation/power/power_seq.txt b/Documentation/power/power_seq.txt

quoted

+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);

quoted

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

quoted

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

_______________________________________________
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss