Re: [PATCH v3 00/12] pwm: add support for atomic update

[Doug Anderson <dianders@google.com>]

Thierry,

On Tue, Feb 23, 2016 at 10:42 AM, Doug Anderson <dianders@google.com> wrote:
> Thierry,
>
> On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
>>> pwm_get_period(): get the period of the PWM; if the PWM has not yet
>>> been configured by software this gets the default period (possibly
>>> specified by the device tree).
>>
>> No. I think we'll need a different construct for the period defined by
>> DT or board files. pwm_get_period() is the legacy API to retrieve the
>> "current" period, even if it was lying a little before the atomic API.
>
> Ah, got it.  I think I missed that you considered pwm_get_period()
> legacy and that you eventually wanted to get rid of it.  OK, then what
> you say makes sense.
>
>
>>> That should work with one minor problem.  If HW readout isn't
>>> supported then pwm_get_state() in probe will presumably return 0 for
>>> the duty cycle.  That means it will change the voltage.  That's in
>>> contrast to how I think things work today where the voltage isn't
>>> changed until the first set_voltage() call.  At least the last time I
>>> tested things get_voltage() would simply report an incorrect value
>>> until the first set_voltage().  I think existing behavior (reporting
>>> the wrong value) is better than new behavior (change the value at
>>> probe).
>>
>> That's exactly the point. Reporting a wrong value isn't really a good
>> option. Changing the voltage on boot is the only way to make the logical
>> state match the hardware state on boot. Chances are that if you don't
>> have hardware readout support you probably don't care what state your
>> regulator will be in.
>>
>> Then again, if we don't support hardware readout, setting up the logical
>> state with data from DT (or board files) and defaulting the duty cycle
>> to 0, we end up with exactly what we had before, even with the atomic
>> API, right? Maybe that's okay, too.
>
> IMHO this is a change in behavior that will break existing users.
> Anyone using a PWM regulator will suddenly find their voltage changing
> at bootup.  Certainly today all users of the PWM regulator don't seem
> to mind (apparently) the the voltage is reported incorrectly at bootup
> but I bet they'd mind if the voltage suddenly started changing for
> them at bootup.
>
> It seems better to preserve existing behavior and print a warning that
> the voltage will be reported incorrectly until HW Readout is
> supported.
>
> Of course, we're only talking about two real users in mainline here:
> Rockchip boards and the "stih407-family".  If we just fix both of
> those to support HW Readout before landing the change then I'm fine
> with doing what you say.
>
>
>>> ...and if set_voltage() remains untouched then we can solve my probe
>>> problem by renaming pwm_get_state() to pwm_get_hw_state() and having
>>> it return an error if HW readout is not supported.  Then we only call
>>> pwm_get_args() / pwm_apply_state() when we support HW readout.
>>
>> The problem is that we make the API clumsy to use. If we don't sync the
>> "initial" state (as defined by DT or board files) to hardware at any
>> point, then we need to add the pwm_args construct and always stick to
>> it. I think it weird to have to use the pwm_args.period instead of the
>> current period.
>>
>> So we're back to square one, really. That's exactly what Mark brought up
>> originally.
>
> I had missed the part where you wanted to deprecate pwm_get_period().
> Thus my points here aren't really valid.
>
> In my mind the old API was perfectly fine (and actually quite clean /
> simple to use) except in the special case of avoiding the PWM
> regulator glitches.  With that mindset I think my previous email make
> sense.  However, this is your subsystem to maintain and if you think
> moving everyone to a new atomic API makes more sense then you're in
> the best position to make that decision.  :)

So just to summarize:

* Add pwm_get_state(), pwm_apply_state(), pwm_get_args().
pwm_get_state() initially returns 0 for duty cycle if driver doesn't
support readout.

* Re-implement pwm_get_period() (and maybe other similar functions)
atop pwm_get_state() as you describe earlier in the thread.

* Document pwm_get_period() (and maybe other similar functions) as deprecated.

* Fix drivers for all current 2 users of PWM regulator to support
hardware readout.

* Update PWM regulator as you described earlier in the thread (Feb 23).

* If PWM regulator is ever used on a new board whose PWM doesn't
support hardware readout, the voltage will change at probe time.


Did I get all that right?  Thanks!

-Doug