Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences

[Thierry Reding <thierry.reding@avionic-design.de>]

[-- Attachment #1: Type: text/plain, Size: 6392 bytes --]

On Thu, Aug 16, 2012 at 06:19:08PM +0900, Alex Courbot wrote:
> On 08/16/2012 04:42 PM, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 16, 2012 at 03:08:55PM +0900, Alexandre Courbot wrote:
[...]
> >>+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.

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

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]