On Thursday 13 September 2012 06:07:13 Stephen Warren wrote:
> On 09/12/2012 03:57 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.
> >
> > Signed-off-by: Alexandre Courbot <acourbot@xxxxxxxxxx>
> >
> > diff --git a/Documentation/power/power_seq.txt
> > b/Documentation/power/power_seq.txt
> >
> > +Sometimes, you may want to browse the list of resources allocated by a
> > sequence, +for instance to 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);
>
> I don't think you need to include that prototype here?
Why not? I thought it was customary to include the prototypes in the
documentation, and this seems to be the right place for this function.
> > + power_seq_for_each_resource(pos, seqs)
> > +
> > +Here "pos" will be a pointer to a struct power_seq_resource. This
> > structure +contains the type of the resource, the information used for
> > identifying it, and +the resolved resource itself.
> >
> > diff --git a/drivers/power/power_seq/Makefile
> > b/drivers/power/power_seq/Makefile new file mode 100644
> > index 0000000..f77a359
> > --- /dev/null
> > +++ b/drivers/power/power_seq/Makefile
> > @@ -0,0 +1 @@
> > +obj-$(CONFIG_POWER_SEQ) += power_seq.o
>
> Don't you need to compile all the power_seq_*.c too?
>
> Oh, I see the following in power_seq.c:
> > +#include "power_seq_delay.c"
> > +#include "power_seq_regulator.c"
> > +#include "power_seq_pwm.c"
> > +#include "power_seq_gpio.c"
>
> It's probably better just to compile them separately and link them.
>
> > diff --git a/drivers/power/power_seq/power_seq.c
> > b/drivers/power/power_seq/power_seq.c
> >
> > +struct power_seq_step {
> > + /* Copy of the platform data */
> > + struct platform_power_seq_step pdata;
>
> I'd reword the comment to "Copy of the step", and name the field "step".
That would make a step within a step - doesn't pdata make it more explicit
what this member is for (containing the platform data for this step)?
> > +static const struct power_seq_res_ops
> > power_seq_types[POWER_SEQ_NUM_TYPES] = { + [POWER_SEQ_DELAY] =
> > POWER_SEQ_DELAY_TYPE,
> > + [POWER_SEQ_REGULATOR] = POWER_SEQ_REGULATOR_TYPE,
> > + [POWER_SEQ_PWM] = POWER_SEQ_PWM_TYPE,
> > + [POWER_SEQ_GPIO] = POWER_SEQ_GPIO_TYPE,
> > +};
>
> Ah, I see why you're using #include now.
We could also go with something more dynamic and compile these files
separately, but that would require some registration mechanism which I don't
think is needed for such a simple feature.
>
> > +MODULE_LICENSE("GPL");
>
> s/GPL/GPL v2/ given the license header.
>
> > diff --git a/drivers/power/power_seq/power_seq_gpio.c
> > b/drivers/power/power_seq/power_seq_gpio.c
> >
> > +static int power_seq_res_alloc_gpio(struct device *dev,
> > + struct platform_power_seq_step *pstep,
> > + struct power_seq_resource *res)
> > +{
> > + int err;
> > +
> > + err = devm_gpio_request_one(dev, pstep->gpio.gpio,
> > + GPIOF_OUT_INIT_LOW, dev_name(dev));
>
> Hmm. The INIT_LOW part of that might be somewhat presumptive. I would
> suggest simply requesting the GPIO here, and using
> gpio_direction_output() in power_seq_step_run_gpio(), thus deferring the
> decision of what value to set the GPIO to until a real sequence is
> actually run.
>
> > diff --git a/drivers/power/power_seq/power_seq_pwm.c
> > b/drivers/power/power_seq/power_seq_pwm.c
> >
> > diff --git a/drivers/power/power_seq/power_seq_regulator.c
> > b/drivers/power/power_seq/power_seq_regulator.c
> >
> > diff --git a/include/linux/power_seq.h b/include/linux/power_seq.h
> >
> > +#include <net/irda/parameters.h>
>
> That looks out of place.
Totally, thanks. I don't even understand how it landed there in the first
place.
>
> > +/**
> > + * struct power_seq_resource - resource used by a power sequence set
> > + * @pdata: Pointer to the platform data used to resolve this resource
> > + * @regulator: Resolved regulator if of type POWER_SEQ_REGULATOR
> > + * @pwm: Resolved PWM if of type POWER_SEQ_PWM
> > + * @list: Used to link resources together
> > + */
>
> I think that kerneldoc is stale.
>
> > +struct power_seq_resource {
> > + enum power_seq_res_type type;
> > + /* resolved resource and identifier */
> > + union {
> > + struct {
> > + struct regulator *regulator;
> > + const char *id;
> > + } regulator;
> > + struct {
> > + struct pwm_device *pwm;
> > + const char *id;
> > + } pwm;
> > + struct {
> > + int gpio;
> > + } gpio;
> > + };
> > + struct list_head list;
> > +};
>
> Aside from those minor issues, this all looks reasonable to me, so,
> Reviewed-by: Stephen Warren <swarren@xxxxxxxxxxxxx>
Thanks!
Alex.
--
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
