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. > +++ b/Documentation/devicetree/bindings/power_seq/power_seq.txt > +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. > +++ b/Documentation/power/power_seq.txt ... > +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. > +++ b/drivers/power/power_seq/Kconfig > +config POWER_SEQ > + bool Some kind of help text might be useful? I didn't review any of the .c files. > +++ b/include/linux/power_seq.h > +/** > + * 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? -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html