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-doc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html