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
pgpP7zeKsFPHT.pgp
Description: PGP signature
