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-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
