On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:

On 08/16/2012 04:42 PM, Thierry Reding wrote:

quoted

* PGP Signed by an unknown key

On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:

[...]

quoted

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

I had some comments about this particular interface for creating
sequences in the last series. My point was that explicitly requiring
drivers to manage a list of already allocated resources may be too much
added complexity. Power sequences should be easy to use, and I find the
requirement for a separately managed list of resources cumbersome.

What I proposed last time was to collect all power sequences under a
common parent object, which in turn would take care of managing the
resources.

Yes, I remember that. While I see why you don't like this list,
having a common parent object to all sequences will not reduce the
number of arguments to pass to power_seq_build() (which is the only
function that has to handle this list now). Also having the list of
resources at hand is needed for some drivers: for instance,
pwm-backlight needs to check that exactly one PWM has been
allocated, and takes a reference to it from this list in order to
control the brightness.

I'm not complaining about the additional argument to power_seq_build()
but about the missing encapsulation. I just think that keeping a list
external to the power sequencing code is error-prone. Drivers could do
just about anything with it between calls to power_seq_build(). If you
do all of this internally, then you don't depend on the driver at all
and power sequencing code can just do the right thing.

Obtaining a reference to the PWM, or any other resource for that matter,
from the power sequence could be done via an explicit API.

Ideally we could embed the list into the device structure, but I
don't see how we can do that without modifying it (and we don't want
to modify it). Another solution would be to keep a static mapping
table that associates a device to its power_seq related resources
within power_seq.c. If we protect it for concurrent access this
should make it possible to make resources management transparent.
How does this sound? Only drawback I see is that we would need to
explicitly clean it up through a dedicated function when the driver
exits.

I don't think that's much better. Since the power sequences will be very
tightly coupled to a specific device, tying the sequences and their
resources to the device makes a lot of sense. Keeping a global list of
resources doesn't in my opinion.

quoted

quoted

+static int power_seq_step_run(struct power_seq_step *step)
+{
+     struct platform_power_seq_step *pdata = &step->pdata;
+     int err = 0;
+
+     switch (pdata->type) {
+     case POWER_SEQ_DELAY:
+             usleep_range(pdata->delay.delay_us,
+                          pdata->delay.delay_us + 1000);
+             break;
+#ifdef CONFIG_REGULATOR
+     case POWER_SEQ_REGULATOR:
+             if (pdata->regulator.enable)
+                     err = regulator_enable(step->resource->regulator);
+             else
+                     err = regulator_disable(step->resource->regulator);
+             break;
+#endif
+#ifdef CONFIG_PWM
+     case POWER_SEQ_PWM:
+             if (pdata->gpio.enable)
+                     err = pwm_enable(step->resource->pwm);
+             else
+                     pwm_disable(step->resource->pwm);
+             break;
+#endif
+#ifdef CONFIG_GPIOLIB
+     case POWER_SEQ_GPIO:
+             gpio_set_value_cansleep(pdata->gpio.gpio, pdata->gpio.enable);
+             break;
+#endif
+     /*
+      * should never happen unless the sequence includes a step which
+      * type does not have support compiled in

I think this should be "whose type"? I also remember commenting on the
whole #ifdef'ery here. I really don't think it is necessary. At least
for regulators I know that the functions can be used even if the
subsystem itself isn't supported. The same seems to hold for GPIO and we
can probably add something similar for PWM.

Actually I kept them because I don't really like the empty function
definitions in the regulator framework. They all return 0 as if the
function completed successfully - here we should at least warn the
user that proper support for that resource is missing.

quoted

It might also be a good idea to just skip unsupported resource types
when the sequence is built, accompanied by runtime warnings that the
type is not supported.

Agreed.

If you do this, then I think the above #ifdef'ery becomes obsolete
because any errors that could potentially be hidden have already been
caught when the list was built.

Thierry