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

[Doug Anderson]

Thierry,

On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
<thierry.reding@xxxxxxxxx> 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.  :)


-Doug