Re: [PATCH v5 1/4] Runtime Interpreted Power Sequences

On 08/31/2012 05:34 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.

+Runtime Interpreted Power Sequences
+=================> +
+Power sequences are sequential descriptions of actions to be performed on
+power-related resources. Having these descriptions in a precise data format
+allows us to take much of the board-specific power control code out of the
+kernel and place it into the device tree instead, making kernels less
+board-dependant.
+

+In the device tree, power sequences are grouped into a set. The set is always
+declared as the "power-sequences" sub-node of the device node. Power sequences
+may reference resources declared by that device.

I had to read that a few times to realize this was talking about a
device with multiple power sequences, and not talking about the steps in
a sequence. I think if you add the following sentence at the start of
that paragraph, it will be clearer:

A device may support multiple power sequences, for different events such
as powering on and off.

Also, perhaps add "these" at the first comma, so the above would read:

In the device tree, these power sequences are...

+Power Sequences Structure
+-------------------------
+Every device that makes use of power sequences must have a "power-sequences"
+sub-node. Power sequences are sub-nodes of this set node, and their node name
+indicates the id of the sequence.
+
+Every power sequence in turn contains its steps as sub-nodes of itself. Step

The last word on that line should be "Steps".

+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.
+
+Power Sequences Steps
+---------------------
+Step of a sequence describes an action to be performed on a resource. They

s/Step/Steps/, s/describes/describe/.

+always include a "type" property which indicates what kind of resource this
+step works on. Depending on the resource type, additional properties are defined
+to control the action to be performed.
+
+"delay" type required properties:
+  - delay_us: delay to wait in microseconds

DT property name should use "-" not "_" to separate words, so "delay-us".

+"regulator" type required properties:
+  - id: name of the regulator to use. Regulator is obtained by
+        regulator_get(dev, id)
+  - enable / disable: one of these two empty properties must be present to
+                      enable or disable the resource
+
+"pwm" type required properties:
+  - id: name of the PWM to use. PWM is obtained by pwm_get(dev, id)
+  - enable / disable: one of these two empty properties must be present to
+                      enable or disable the resource

For those two, would "name" be a better property name than "id"?

I wonder if we should have "regulator-enable" and "regulator-disable"
step types, rather than a "regulator" step type, with an "enable" or
"disable" property within it. I don't feel strongly though; this is
probably fine.

+"gpio" type required properties:
+  - number: phandle of the GPIO to use.

Naming the property "gpio" would seem more consistent with our GPIO
properties.

+  - enable / disable: one of these two empty properties must be present to
+                      enable or disable the resource

You can't really enable or disable a GPIO (well, perhaps you can, but
I'd consider that to affect tri-state rather than value); it's more that
you're setting the output value to 0 or 1. I think a "value" or
"set-value" property with value <0> or <1> would be better.

...

+After the resources declaration, two sequences follow for powering the backlight
+on and off. Their names are specified by the pwm-backlight driver.

Not the driver, but the binding for the device.

Overall, the general structure of the bindings looks reasonable to me.

...

+Platform Data Format
+--------------------
+All relevant data structures for declaring power sequences are located in
+include/linux/power_seq.h.
+
+The platform data for a given device is an instance of platform_power_seq_set
+which points to instances of platform_power_seq. Every platform_power_seq is a
+single power sequence, and is itself composed of a variable length array of
+steps.

I don't think you can mandate that the entire platform data structure
for a device is a platform_power_seq_set. Instead, I think you should
say that "The platform data for a device may contain an instance of
platform_power_seq_set...".

...

+A power sequence can be executed by power_seq_run:
+
+  int power_seq_run(struct power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code if a
+problem occured.

s/occured/occurred/

+Sometimes, you may want to browse the list of resources allocated by a sequence,
+to for instance ensure that a resource of a given type is present. The
+power_seq_set_resources() function returns a list head that can be used with
+the power_seq_for_each_resource() macro to browse all the resources of a set:
+
+  struct list_head *power_seq_set_resources(struct power_seq_set *seqs);
+  power_seq_for_each_resource(pos, seqs)
+
+Here "pos" will be of type struct power_seq_resource. This structure contains a
+"pdata" pointer that can be used to explore the platform data of this resource,
+as well as the resolved resource, if applicable.

I'm not sure what "pdata" is supposed to point at; platform data applies
to the original "struct device", not one of the resources used by the
power sequences.

+Finally, users of the device tree can build the platform data corresponding to
+the tree node using this function:
+
+  struct platform_power_seq_set *devm_of_parse_power_seq_set(struct device *dev);

Hmmm. It'd be nice not to need separate functions for the non-DT and DT
cases. That would require that devm_power_seq_set_build() be able to
find the power sequence definitions somewhere other than platform data
in the non-DT case - that's exactly why the regulator and pinctrl
subsystems represent the device<->data mapping table separately from the
device's platform data.

+config POWER_SEQ
+	bool

Some kind of help text might be useful?

I didn't review any of the .c files.

+/**
+ * struct platform_power_seq_step - platform data for power sequences steps
+ * @type:	The type of this step. This decides which member of the union is
+ *		valid for this step.
+ * @delay:	Used if type = POWER_SEQ_DELAY
+ * @regulator:	Used if type = POWER_SEQ_REGULATOR
+ * @pwm:	Used if type = POWER_SEQ_PWN
+ * @gpio:	Used if type = POWER_SEQ_GPIO

In those last 4 line, I think s/type/@type/ since you're referencing
another parameter?

+struct power_seq_resource {
+	/* relevant for resolving the resource and knowing its type */
+	struct platform_power_seq_step *pdata;

Aha. So this isn't really platform data for the resource, but actually a
step definition that referenced it. I think it'd be better to rename
this field "step", and amend the documentation above not to refer to
"pdata" but explicitly talk about a step definition; the step may have
been defined in pdata, but isn't in the DT case.

Alternatively, why not just copy the step type enum here, rather than
referencing the step definition?