Messages in this thread | | | Date | Tue, 31 Jul 2012 19:32:20 +0900 | From | Alex Courbot <> | Subject | Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences |
| |
On 07/31/2012 07:45 AM, Stephen Warren wrote: >> +- Delay to wait before performing the action, >> +- Delay to wait after performing the action. > > I don't see a need to have a delay both before and after an action; > except at the start of the sequence, step n's post-delay is at the same > point in the sequence as step n+1's pre-delay. Perhaps make a "delay" > step type?
My first version used this actually - and you're right, having a "delay" step type would be more flexible and less redundant.
>> +Both new resources and parameters can be introduced, but the goal is of course >> +to keep things as simple and compact as possible. > >> +The platform data is a simple array of platform_power_seq_step instances, each > > Rather than jumping straight into platform data here, I'd expect an > enumeration of legal resource types, and what actions can be performed > on each, followed by a description of a sequence (very simply, just a > list of actions and their parameters). This could be followed by a > section describing the mapping of the abstract concepts to concrete > platform data representation (and concrete device tree representation).
Keeping that in mind for the next revision.
>> +instance describing a step. The type as well as one of id or gpio members >> +(depending on the type) must be specified. The last step must be of type >> +POWER_SEQ_STOP. > > I'd certainly suggest having a step count rather than a sentinel value > in the list.
As Thierry did - I think I will go that way.
>> Regulator and PWM resources are identified by name. GPIO are >> +identified by number. > > That's a little implementation-specific. I guess it's entirely true for > a platform data representation, but not when mapping this into device tree.
If we can come with a way to properly use phandles within DT sequences (and we should), then this will only apply to platform data.
>> +You will need an instance of power_seq_resources to keep track of the resources >> +that are already allocated. 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. > > If the result is devm-allocated, the function probably should be named > devm_power_seq_build().
Right - more generally this needs to have both devm and non-devm variants.
> I wonder if using the same structure/array as input and output would > simplify the API; the platform data would fill in the fields mentioned > above, and power_seq_build() would parse those, then set other fields in > the same structs to the looked-up handle values?
The thing is that I am not sure what happens to the platform data once probe() is done. Isn't it customary to mark it with __devinit and have it freed after probing is successful?
More generally, I think it is a good practice to have data structures tailored right for what they need to do - code with members that are meaningful only at given points of an instance's life tends to be more confusing.
> You can make a custom devm free routine for the power_seq_resources > itself, so the overall free'ing of the content can be triggered by devm, > but the free'ing function can then call whatever non-devm APIs it wants > for the non-devm-allocated members.
That sounds good.
>> +Device tree >> +----------- >> +All the same, power sequences can be encoded as device tree nodes. The following >> +properties and nodes are equivalent to the platform data defined previously: >> + >> + power-supply = <&mydevice_reg>; >> + enable-gpio = <&gpio 6 0>; >> + >> + power-on-sequence { >> + regulator@0 { > > As Thierry mentioned, the step nodes should be named for the type of > object they are (a "step") not the type or name of resource they act > upon ("regulator" or "gpio").
Will fix that.
> If the nodes have a unit address (i.e. end in "@n"), which they will > have to if all named "step" and there's more than one of them, then they > will need a matching reg property. Equally, the parent node will need > #address-cells and #size-cells too. So, the last couple lines would be: > > power-on-sequence { > #address-cells = <1>; > #size-cells = <0>; > step@0 { > reg = <0>;
That's precisely what I would like to avoid - I don't need the steps to be numbered and I certainly have no use for a reg property. Isn't there a way to make it simpler?
>> + id = "power"; > > "id" is usually a name or identifier. I think you mean "type" or perhaps > "action" here: > > type = "regulator"; > action = "enable"; > > or: > > action = "enable-regulator";
Right, that was a clear misuse.
> Oh I see. That's a little confusing. Why not just reference the relevant > resources directly in each step; something more like: > > gpio@1 { > action = "enable-gpio"; > gpio = <&gpio 1 0>; > }; > > I guess that might make parsing/building a little harder, since you'd > have to detect when you'd already done gpio_request() on a given GPIO > and not repeat it or something like that, but to me this makes the DT a > lot easier to comprehend.
You can see my reply to Thierry for the reason - the only issue with that is caused by PWM phandles. If we overcome this, then I agree we should use phandles. The code should not even get more complex as I have to check whether a resource is already allocated with strings as well.
Thanks, Alex.
| |