Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

Hi,

On Fri, Jul 27, 2012 at 1:05 PM, Alexandre Courbot [off-list ref] 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.

This all looks very reasonable to me, just a few comments.

For the delay, I think milliseconds is reasonable. I suppose there is
no reasonable need for microseconds?

+
+struct platform_power_seq_step power_on_seq[] = {
+       {
+               .type = POWER_SEQ_REGULATOR,
+               .id = "power",
+               .params = {
+                       .enable = 1,
+                       .post_delay = 10,
+               },
+       },
+       {
+               .type = POWER_SEQ_GPIO,
+               .gpio = 110,
+               .params = {
+                       .enable = 1,
+               },
+       },
+       {
+               .type = POWER_SEQ_STOP,
+       },
+};
+
+Usage by Drivers and Resources Management
+-----------------------------------------
+Power sequences make use of resources that must be properly allocated and
+managed. The power_seq_build() function takes care of resolving the resources as
+they are met in the sequence and to allocate them if needed:
+
+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
+                          platform_power_seq *pseq);
+
+You will need an instance of power_seq_resources to keep track of the resources
+that are already allocated. On success, the function returns a devm allocated
+resolved sequence that is 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(struct device *dev, power_seq *seq);
+
+It returns 0 if the sequence has successfully been run, or an error code if a
+problem occured.
+
+Finally, some resources that cannot be allocated through devm need to be freed
+manually. Therefore, be sure to call power_seq_free_resources() in your device
+remove function:
+
+void power_seq_free_resources(power_seq_resources *ress);
+
+Device tree
+-----------
+All the same, power sequences can be encoded as device tree nodes. The following
+properties and nodes are equivalent to the platform data defined previously:
+
+               power-supply = <&mydevice_reg>;
+               enable-gpio = <&gpio 6 0>;
+
+               power-on-sequence {
+                       regulator@0 {
+                               id = "power";

Is there a reason not to put the phandle here, like:

                                   id = <&mydevice_reg>;

(or maybe 'device' instead of id?)

+                               enable;
+                               post-delay = <10>;
+                       };
+                       gpio@1 {
+                               id = "enable-gpio";
+                               enable;
+                       };
+               };
+
+Note that first, the phandles of the regulator and gpio used in the sequences
+are defined as properties. Then the sequence references them through the id
+property of every step. The name of sub-properties defines the type of the step.
+Valid names are "regulator", "gpio" and "pwm". Steps must be numbered
+sequentially.

For the regulator and gpio types I think you only have an enable. For
the pwm, what is the intended binding? Is that documented elsewhere?

Also it might be worth mentioning how you get a struct power_seq from
an fdt node, and perhaps given an example of a device which has an
attached node, so we can see how it is referenced from the device
(of_parse_power_seq I think). Do put the sequence inside the device
node or reference it with a phandle?

Finally, should you use typedefs?

Regards,
Simon