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

Thierry,

On Tue, Feb 23, 2016 at 10:14 AM, Thierry Reding
[off-list ref] wrote:

quoted

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.

quoted

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.

quoted

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