Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences

[Thierry Reding]

On Tue, Jul 31, 2012 at 06:51:03PM +0900, Alex Courbot wrote:
> On 07/30/2012 08:33 PM, Thierry Reding wrote:
> >>+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.
> >
> >I don't quite understand why the struct power_seq_resources is needed.
> >Can this not be stored within power_seq?
> 
> power_seq_resources serves two purposes:
> 1) When parsing sequences, it keeps track of the resources we have
> already allocated to avoid getting the same resource twice
> 2) On cleanup, it cleans the resources that needs to be freed (i.e.
> those that are not devm-handled).
> 
> 2) can certainly be removed either by enforcing use of devm, or by
> doing reference counting. 1) seems more difficult to avoid - we need
> to keep track of the resources we already own between calls to
> power_seq_build(). I'd certainly be glad to remove that structure
> from public view and simplify the code if that is possible though.

I still don't see the problem. Managing the resources should be part of
the power_seq core and shouldn't be visible to users. Maybe what you are
worried about is that you may need the same resource both for a power-up
and a power-down sequence? I can see how that would require a global
list of resources.

However I still think it would be easier to encapsulate that completely.
Maybe another level of abstraction is required. You could for example
add another type to encapsulate several power sequences and that could
keep a list of used resources. I can't think of a good name, but maybe
the following DT snippet clarifies what I mean:

	power-sequences {
		#address-cells = <1>;
		#size-cells = <0>;

		sequence@0 {
			name = "up";

			#address-cells = <1>;
			#size-cells = <0>;

			step@0 {
				...
			};

			...
		};

		sequence@1 {
			name = "down";

			#address-cells = <1>;
			#size-cells = <0>;

			step@0 {
				...
			};

			...
		};
	};

If you add a name property like this, you could extend the API to
support running a named sequence:

	power_seq_run(seq, "up");
	...
	power_seq_run(seq, "down);

> >Also, is there some way we can make the id property for GPIOs not
> >require the -gpio suffix? If the resource type is already GPIO, then it
> >seems redundant to add -gpio to the ID.
> 
> There is unfortunately an inconsistency between the way regulators
> and GPIOs are gotten by name. regulator_get(id) will expect to find
> a property named "id-supply", while gpio_request_one(id) expects a
> property named exactly "id". To workaround this we could sprintf the
> correct property name from a non-suffixed property name within the
> driver, but I think this actually speaks more in favor of having
> phandles directly into the sequences.

Yes, if it can be made to work by specifying the phandle directly that
is certainly better.

> >>+static int power_seq_step_run(struct power_seq_step *step)
> >>+{
> >>+     int err = 0;
> >>+
> >>+     if (step->params.pre_delay)
> >>+             mdelay(step->params.pre_delay);
> >>+
> >>+     switch (step->resource->type) {
> >>+#ifdef CONFIG_REGULATOR
> >>+     case POWER_SEQ_REGULATOR:
> >>+             if (step->params.enable)
> >>+                     err = regulator_enable(step->resource->regulator);
> >>+             else
> >>+                     err = regulator_disable(step->resource->regulator);
> >>+             break;
> >>+#endif
> >>+#ifdef CONFIG_PWM
> >>+     case POWER_SEQ_PWM:
> >>+             if (step->params.enable)
> >>+                     err = pwm_enable(step->resource->pwm);
> >>+             else
> >>+                     pwm_disable(step->resource->pwm);
> >>+             break;
> >>+#endif
> >>+#ifdef CONFIG_GPIOLIB
> >>+     case POWER_SEQ_GPIO:
> >>+             gpio_set_value_cansleep(step->resource->gpio,
> >>+                                     step->params.enable);
> >>+             break;
> >>+#endif
> >
> >This kind of #ifdef'ery is quite ugly. I don't know if adding separate
> >*_run() functions for each type of resource would be any better, though.
> >Alternatively, maybe POWER_SEQ should depend on the REGULATOR, PWM and
> >GPIOLIB symbols to side-step the issue completely?
> 
> If it is not realistic to consider a kernel built without regulator,
> pwm or gpiolib support, then we might as well do that. But isn't
> that a possibility?

I'd say that anything complex enough to make use of power-sequencing
probably has all of these enabled anyway. But maybe I'm not very
qualified to judge.

> 
> >>+     if (!seq) return 0;
> >
> >I don't think this is acceptable according to the coding style. Also,
> >perhaps returning -EINVAL would be more meaningful?
> 
> I neglected running checkpatch before submitting, apologies for
> that. The return value seems correct to me, a NULL sequence has no
> effect.

But seq == NULL should never happen anyway, right?

> 
> >>+
> >>+     while (seq->resource) {
> >
> >Perhaps this should check for POWER_SEQ_STOP instead?
> 
> There is no resource for POWER_SEQ_STOP - therefore, a NULL resource
> is used instead.

Still, you use POWER_SEQ_STOP as an explicit sentinel to mark the end of
a sequence, so intuitively I'd be looking for that as a stop condition.

> >>+typedef struct platform_power_seq_step platform_power_seq;
> >
> >Why are the parameters kept in a separate structure? What are the
> >disadvantages of keeping the in the sequence step structure directly?
> 
> This ensures the same parameters are used for the platform data and
> resolved sequences, and also ensures they are all copied correctly
> using memcpy. But maybe I am just making something complex out of
> something that ought to be simpler.
> 
> >>+struct power_seq_step {
> >>+     struct power_seq_resource *resource;
> >>+     struct power_step_params params;
> >>+};
> >>+typedef struct power_seq_step power_seq;
> >
> >Would it make sense to make the struct power_seq opaque? I don't see why
> >anyone but the power_seq code should access the internals.
> 
> I would like to do that actually. The issue is that it did not work
> go well with the legacy pwm_backlight behavior: a power sequence
> needs to be constructed out of a PWM obtained through
> pwm_request(int pwm_id, char *label) and this behavior cannot be
> emulated using the new platform data interface (which only works
> with pwm_get()). But if I remove this old behavior, then I could
> make power_seq opaque. I don't think many drivers are using it. What
> do you think?

I don't see how that is relevant here, since this power-sequencing code
is supposed to be generic and not tied to any specific implementation.
Can you explain further?

In any case you shouldn't be using pwm_request() in new code.

> >For resource
> >managing it might also be easier to separate struct power_seq_step and
> >struct power_seq, making the power_seq basically something like:
> >
> >         struct power_seq {
> >                 struct power_seq_step *steps;
> >                 unsigned int num_steps;
> >         };
> >
> >Perhaps a name field can be included for diagnostic purposes.
> 
> Yes, looks like we are going in that direction. If this can be made
> private then the number of public data structures will not be too
> confusing (platform data only, basically).

Yes, that sounds like a much cleaner approach.

> 
> >>+power_seq *power_seq_build(struct device *dev, power_seq_resources *ress,
> >>+                        platform_power_seq *pseq);
> >
> >I already mentioned this above: I fail to see why the ress parameter is
> >needed here. It is an internal implementation detail of the power
> >sequence code. Maybe a better place would be to include it within the
> >struct power_seq.
> 
> Problem is that I need to track which resources are already
> allocated between calls to power_seq_build(). Even if I attach the
> resources into struct power_seq, they won't be attainable by the
> next call. So I'm afraid we are bound to pass a tracking structure
> at least to power_seq_build.

I think this could be solved nicely by what I proposed earlier.

> >>+/**
> >>+ * Free all the resources previously allocated by power_seq_allocate_resources.
> >>+ */
> >>+void power_seq_free_resources(power_seq_resources *ress);
> >>+
> >>+/**
> >>+ * Run the given power sequence. Returns 0 on success, error code in case of
> >>+ * failure.
> >>+ */
> >>+int power_seq_run(struct device *dev, power_seq *seq);
> >
> >I think the API is too fine grained here. From a user's point of view,
> >I'd expect a sequence like this:
> >
> >         seq = power_seq_build(dev, sequence);
> >         ...
> >         power_seq_run(seq);
> >         ...
> >         power_seq_free(seq);
> >
> >Perhaps with managed variants where the power_seq_free() is executed
> >automatically:
> >
> >         seq = devm_power_seq_build(dev, sequence);
> >         ...
> >         power_seq_run(seq);
> 
> I agree. On top of that, of_parse_power_seq() should directly return
> a resolved power sequence, not the platform data.

I'm not sure. The idiom seems to be to use DT as an alternative source
for platform data, which I guess is due to most drivers already using
platform data. But it has the advantage that after you have the platform
data, be it from DT or directly specified, the subsequent code remains
the same.

Of course you could provide a separate of_build_power_seq() that wraps
both steps for convenience.

Thierry

Attachment: pgp00000.pgp
Description: PGP signature