[PATCH v6 1/4] Runtime Interpreted Power Sequences

[Stephen Warren]

On 09/13/2012 12:02 AM, Alex Courbot wrote:
> 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 at nvidia.com>
>>>
>>> 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);
>>> +  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.
>>
>> I don't think you need to include that [power_seq_set_resources] 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.

It's something used internally to the macro; what the user cares about
is which macro to use for the functionality you're describing, not any
prototypes needed by the internal implementation of the macro, which are
always provided by the appropriate header.

>>> 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)?

Well, it's not always platform data; it could come from device tree.
Sorry for bike-shedding slightly, but how about just "data",
"step_data", "config", or "step_config"?

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

Sure. There are already examples in the kernel of basically what you're
doing anyway, and it's not like it'd be hard to change this if we want
to do something different in the future too.