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