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

On Tue, Jul 31, 2012 at 07:11:41PM +0900, Alex Courbot wrote:

On 07/31/2012 06:13 PM, Thierry Reding wrote:

quoted

quoted

I don't see any need for microseconds myself - anybody sees use for
finer-grained delays?

Btw, I noticed I was using mdelay instead of msleep - caught and fixed that.

You might want to take a look at Documentation/timers/timers-howto.txt.
msleep() isn't very accurate for periods shorter than 20 ms.

Ok, looks like usleep_range is the way to go here. In that case it
would probably not hurt to specify delays in microseconds in the DT
and platform data as well.

quoted

quoted

quoted

quoted

+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 {
+                               id = "power";

Is there a reason not to put the phandle here, like:

                                   id = <&mydevice_reg>;

(or maybe 'device' instead of id?)

There is one reason, but it might be a bad one. On Tegra, PWM
phandle uses an extra cell to encode the duty-cycle the PWM should
have when we call get_pwm().

This is not only the case on Tegra, but it is the default unless a
driver specifically overrides it. The second cell specifies the index of
the PWM device within the PWM chip.  The third cell doesn't specify the
duty cycle but the period of the PWM.

Then I think there is a mistake in
Documentation/devicetree/bindings/pwm/nvidia,tegra20-pwm.txt:

"the second cell is the duty cycle in nanoseconds."

Yes, that's a mistake. =\

quoted

quoted

This makes it possible to address the
same PWM with different phandles (and different duty cycles),

How so? A phandle will always refer to a PWM chip. Paired with the
second cell, of_pwm_request() will resolve that into the correct PWM
device.

For tegra, we can only address PWMs this way IIRC:

pwm = <&pwm 2 5000000>;

If we had <&pwm 2>, I agree that there would be no problem. But here
the period of the PWM is also given - and in practice, we can
request the same PWM using different phandles. For instance, if the
above property was part of the power-on sequence, and the following

pwm = <&pwm 2 0>;

was part of power-off, how can I know that these two different
phandles refer to the same PWM without calling pwm_get a second time
and getting -EBUSY?

You should specify the same period regardless of the sequence. But you
are right, you still cannot request the device twice.

Of course if the same period is specified for both, I will not have
this issue as the phandles will be identical, but the possibility
remains open that we are given a faulty tree here.

I think the phandle is in fact only the reference to the PWM chip, that
is: &pwm. The second cell, the PWM index, is part of the PWM specifier.

However the issue doesn't go away if you drop the period cell because
you still won't be able to request the PWM device a second time. How is
this solved for regulators and GPIOs? At least for GPIOs I'm pretty sure
that you can't request them more than once either.

More generally speaking, wouldn't it make more sense to have the
period/duty cycle of a PWM encoded into separate properties when
needed and have the phandle just reference the PWM instance? This
also seems to stand from an API point of view, since the period is
not specified when invoking pwm_request or pwm_get, but set by its
own pwm_set_period function?

The problem with specifying the period in a separate property is how to
map them to the correct PWM device. From a hardware description point of
view, making the period part of the specifier makes a lot of sense and
hardware description is what DT is about.

On an unrelated note, I also don't understand why the period is also
a parameter of pwm_config and why pwm_set_period does not do
anything beyond setting a struct member that is returned by
pwm_get_period (but maybe I am missing something here).

pwm_config() is the legacy API that we need to support for compatibility
reasons. Eventually this interface should probably be changed.
pwm_get_period() and pwm_set_period() were merely introduced to support
DT, but I could imagine them becoming the canonical way for configuring
PWM devices in the future, perhaps with a complementary
pwm_set_duty_cycle().

But first we need to convert drivers and users.

Thierry