Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1340713 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 00/12] pwm: add support for atomic update Thierry Reding <thierry.reding@gmail.com> - 2016-02-23 15:40 +0100
    Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-02-23 18:40 +0100
      Re: [PATCH v3 00/12] pwm: add support for atomic update Thierry Reding <thierry.reding@gmail.com> - 2016-02-23 19:20 +0100
        Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-02-23 19:50 +0100
          Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-02-26 00:20 +0100

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

[Multipart message — attachments visible in raw view] — view raw

On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:
> Thierry,
> 
> On Mon, Feb 22, 2016 at 9:59 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> >> When we add a new feature then it's expected that only updated drivers
> >> will support that feature.
> >>
> >> We need to make sure that we don't regress / negatively change the
> >> behavior for anyone running non-updated drivers.  ...and we should
> >> strive to require as few changes to drivers as possible.  ...but if
> >> the best we can do requires changes to the PWM driver API then we will
> >> certainly have differences depending on the PWM driver.
> >
> > How so? Drivers should behave consistently, irrespective of the API. Of
> > course if you need to change behaviour of the user driver depending on
> > the availability of a certain feature, that's perfectly fine.
> >
> > Furthermore it's out of the question that changes to the API will be
> > required. That's precisely the reason why the atomic PWM proposal came
> > about. It's an attempt to solve the shortcomings of the current API for
> > cases such as Rockchip.
> 
> I _think_ we're on the same page here.  If there are shortcomings with
> the current API that make it impossible to implement a feature, we've
> got to change and/or add to the existing API.  ...but we don't want to
> break existing users / drivers.
> 
> Note that historically I remember that Linus Torvalds has stated that
> there is no stable API within the Linux kernel and that forcing the
> in-kernel API to never change was bad for software development.  I
> tracked down my memory and found
> <http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
> breaking userspace, but in general there's no strong requirement to
> never change the driver API inside the kernel.  That being said,
> changing the driver API causes a lot of churn, so presumably changing
> it in a backward compatible way (like adding to the API instead of
> changing it) will make things happier.

I didn't say anything about stable API. All I said is that new API
should be well-thought-out. Those are two very different things.

> >> So all we need is a new API call that lets you read the hardware
> >> values and make sure that the PWM regulator calls that before anyone
> >> calls pwm_config().  That's roughly B) above.
> >
> > Yes. I'm thinking that we should have a pwm_get_state() which retrieves
> > the current state of the PWM. For drivers that support hardware readout
> > this state should match the hardware state. For other drivers it should
> > reflect whatever was specified in DT; essentially what pwm_get_period()
> > and friends return today.
> 
> Excellent, so pwm_get_period() gets the period as specified in the
> device tree (or other board config) and pwm_get_state() returns the
> hardware state.  SGTM.

That's not quite what I was thinking. If hardware readout is supported
then whatever we report back should be the current hardware state unless
we're explicitly asked for something else. If we start mixing the state
and legacy APIs this way, we'll get into a situation where drivers that
support hardware readout behave differently than drivers that don't.

For example: A PWM device that's controlled by a driver that supports
hardware readout has a current period of 50000 ns and the firmware set
the period to 25000 ns. pwm_get_period() for this PWM device will return
50000 ns. If you reconfigure the PWM to generate a PWM signal with a
period of 30000 ns, pwm_get_period() would still return 50000 ns.

A driver that doesn't support hardware readout, on the contrary, would
return 50000 ns from pwm_get_period() on the first call, but after you
have reconfigured it using pwm_config() it will return the new period.

> > That way if you want to get the current voltage in the regulator-pwm
> > driver you'd simply do a pwm_get_state() and compute the voltage from
> > the period and duty cycle. If the PWM driver that you happen to use
> > doesn't support hardware readout, you'll get an initial output voltage
> > of 0, which is as good as any, really.
> 
> Sounds fine to me.  PWM regulator is in charge of calling
> pwm_get_state(), which can return 0 (or an error?) if driver (or
> underlying hardware) doesn't support hardware readout.  PWM regulator
> is in charge of using the resulting period / duty cycle to calculate a
> percentage.

I'm not sure if pwm_get_state() should ever return an error. For drivers
that support hardware readout, the resulting state should match what's
programmed to the hardware.

But for drivers without hardware readout support pwm_get_state() still
makes sense because after a pwm_apply_state() the internal logical state
would again match hardware.

The simplest way to get rid of this would be to change the core to apply
an initial configuration on probe. However that's probably going to
break your use-case again (it would set a 0 duty cycle because it isn't
configured in DT).

To allow your use-case to work we'd need to deal with two states: the
current hardware state and the "initial" state as defined by DT.
Unfortunately the PWM specifier in DT is not a full definition, it is
more like a partial initial configuration. The problem with that, and
I think that's what Mark was originally objecting to, is that it isn't
clear when to use the "initial" state and when to use the read hardware
state. After the first pwm_apply_state() you wouldn't ever have to use
the "initial" state again, because it's the same as the current state
(modulo the duty cycle).

Also for drivers such as clk-pwm the usefulness of the "initial" state
is reduced even more, because it doesn't even need the period specified
in DT. It uses only the flags (if at all).

Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
could be introduced to hold configuration arguments given in the PWM
specifier (in DT) or the PWM lookup table (in board files).

It would then be the responsibility of the users to deal with that
information in a sensible way. In (almost?) all cases I would expect
that to be to program the PWM device in the user's ->probe(). In the
case of regulator-pwm I'd expect that ->probe() would do something
along these lines (error handling excluded):

	struct pwm_state state;
	struct pwm_args args;
	unsigned int ratio;

	pwm = pwm_get(...);

	pwm_get_state(pwm, &state);
	pwm_get_args(pwm, &args);

	ratio = (state.duty_cycle * 100) / state.period;

	state.duty_cycle = (ratio * args.period) / 100;
	state.period = args.period;
	state.flags = args.flags;

	pwm_apply_state(pwm, &state);

The ->set_voltage() implementation would then never need to care about
the PWM args, but rather do something like this:

	struct pwm_state state;
	unsigned int ratio;

	pwm_get_state(pwm, &state);

	state.duty_cycle = (ratio * state.period) / 100;

	pwm_apply_state(pwm, &state);

Does that sound about right?

> I _think_ the end result of all this is just:
> 
> 1. Introduce pwm_get_state() that gets hardware state.  Up for debate
> if this returns 0 or ERROR if a driver doesn't implement this.

Like I said above, I don't think pwm_get_state() should ever fail. It
should simply return the current state of the PWM, which might coincide
with the hardware state (for drivers that support hardware readout) or
will be the logical state (for drivers that don't).

Note that in the above example the logical state of the PWM, in cases
where the driver doesn't support hardware readout, the duty cycle will
be assumed to be 0, so the regulator-pwm driver would at ->probe() time
disable the regulator, at which point hardware state and logical state
will coincide again.

> 2. PWM regulator calls pwm_get_state at probe time to get hardware
> state, calculates a percentage (and voltage) with this.

I don't think that's enough. If we do this, we'll keep carrying around
the mismatch between hardware state and logical state indefinitely.

> 3. PWM regulator does nothing else until it is asked to set the
> voltage, but uses the voltage calculated from #2 to satisfy any "get
> voltage" calls.

This should work out of the box in the above. The initial state would
yield a voltage of 0 if hardware readout is not supported, whereas for
drivers that support hardware readout, the proper value can be derived
from duty cycle, period and the lookup table.

> 4. When asked to set the voltage, PWM regulator uses pwm_get_period()
> and calculates a duty cycle based on that, just like it does today.
> This uses pwm_config() which includes a duty cycle and period and is
> thus "atomic".

pwm_config() isn't atomic. pwm_apply_state() would be. The difference is
that pwm_config() can't at the same time enable/disable the PWM or set
the polarity.

Thierry

[toc] | [next] | [standalone]

#1340897

Thierry,

On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> > Furthermore it's out of the question that changes to the API will be
>> > required. That's precisely the reason why the atomic PWM proposal came
>> > about. It's an attempt to solve the shortcomings of the current API for
>> > cases such as Rockchip.
>>
>> I _think_ we're on the same page here.  If there are shortcomings with
>> the current API that make it impossible to implement a feature, we've
>> got to change and/or add to the existing API.  ...but we don't want to
>> break existing users / drivers.
>>
>> Note that historically I remember that Linus Torvalds has stated that
>> there is no stable API within the Linux kernel and that forcing the
>> in-kernel API to never change was bad for software development.  I
>> tracked down my memory and found
>> <http://lwn.net/1999/0211/a/lt-binary.html>.  Linus is rabid about not
>> breaking userspace, but in general there's no strong requirement to
>> never change the driver API inside the kernel.  That being said,
>> changing the driver API causes a lot of churn, so presumably changing
>> it in a backward compatible way (like adding to the API instead of
>> changing it) will make things happier.
>
> I didn't say anything about stable API. All I said is that new API
> should be well-thought-out. Those are two very different things.

I guess I just misunderstood "it's out of the question that changes to
the API will be required".  In any case, I think everyone's on the
same page here, so no need to debate further.  :)


>> >> So all we need is a new API call that lets you read the hardware
>> >> values and make sure that the PWM regulator calls that before anyone
>> >> calls pwm_config().  That's roughly B) above.
>> >
>> > Yes. I'm thinking that we should have a pwm_get_state() which retrieves
>> > the current state of the PWM. For drivers that support hardware readout
>> > this state should match the hardware state. For other drivers it should
>> > reflect whatever was specified in DT; essentially what pwm_get_period()
>> > and friends return today.
>>
>> Excellent, so pwm_get_period() gets the period as specified in the
>> device tree (or other board config) and pwm_get_state() returns the
>> hardware state.  SGTM.
>
> That's not quite what I was thinking. If hardware readout is supported
> then whatever we report back should be the current hardware state unless
> we're explicitly asked for something else. If we start mixing the state
> and legacy APIs this way, we'll get into a situation where drivers that
> support hardware readout behave differently than drivers that don't.
>
> For example: A PWM device that's controlled by a driver that supports
> hardware readout has a current period of 50000 ns and the firmware set
> the period to 25000 ns. pwm_get_period() for this PWM device will return
> 50000 ns. If you reconfigure the PWM to generate a PWM signal with a
> period of 30000 ns, pwm_get_period() would still return 50000 ns.
>
> A driver that doesn't support hardware readout, on the contrary, would
> return 50000 ns from pwm_get_period() on the first call, but after you
> have reconfigured it using pwm_config() it will return the new period.

Ah, right!  I forgot that the existing API will be updated if you've
reconfigured the period via pwm_config().  Ugh, you're right that's a
little ugly then.

So do we define it as:

pwm_get_state(): always get the hardware state w/ no caching (maybe
even pwm_get_raw_state() or pwm_get_hw_state())

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


Is that OK?  That is well defined and doesn't change the existing
behavior of pwm_get_period().


>> > That way if you want to get the current voltage in the regulator-pwm
>> > driver you'd simply do a pwm_get_state() and compute the voltage from
>> > the period and duty cycle. If the PWM driver that you happen to use
>> > doesn't support hardware readout, you'll get an initial output voltage
>> > of 0, which is as good as any, really.
>>
>> Sounds fine to me.  PWM regulator is in charge of calling
>> pwm_get_state(), which can return 0 (or an error?) if driver (or
>> underlying hardware) doesn't support hardware readout.  PWM regulator
>> is in charge of using the resulting period / duty cycle to calculate a
>> percentage.
>
> I'm not sure if pwm_get_state() should ever return an error. For drivers
> that support hardware readout, the resulting state should match what's
> programmed to the hardware.
>
> But for drivers without hardware readout support pwm_get_state() still
> makes sense because after a pwm_apply_state() the internal logical state
> would again match hardware.

I guess it depends on how you define things.  With my above
definitions it seems clearest if pwm_get_state() returns an error if
hardware readout is not supported.  If we call it pwm_get_hw_state()
it's even clearer that it should return an error.


> The simplest way to get rid of this would be to change the core to apply
> an initial configuration on probe. However that's probably going to
> break your use-case again (it would set a 0 duty cycle because it isn't
> configured in DT).

Right, so we can't do that.


> To allow your use-case to work we'd need to deal with two states: the
> current hardware state and the "initial" state as defined by DT.
> Unfortunately the PWM specifier in DT is not a full definition, it is
> more like a partial initial configuration. The problem with that, and
> I think that's what Mark was originally objecting to, is that it isn't
> clear when to use the "initial" state and when to use the read hardware
> state. After the first pwm_apply_state() you wouldn't ever have to use
> the "initial" state again, because it's the same as the current state
> (modulo the duty cycle).
>
> Also for drivers such as clk-pwm the usefulness of the "initial" state
> is reduced even more, because it doesn't even need the period specified
> in DT. It uses only the flags (if at all).
>
> Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
> could be introduced to hold configuration arguments given in the PWM
> specifier (in DT) or the PWM lookup table (in board files).
>
> It would then be the responsibility of the users to deal with that
> information in a sensible way. In (almost?) all cases I would expect
> that to be to program the PWM device in the user's ->probe(). In the
> case of regulator-pwm I'd expect that ->probe() would do something
> along these lines (error handling excluded):
>
>         struct pwm_state state;
>         struct pwm_args args;
>         unsigned int ratio;
>
>         pwm = pwm_get(...);
>
>         pwm_get_state(pwm, &state);
>         pwm_get_args(pwm, &args);
>
>         ratio = (state.duty_cycle * 100) / state.period;
>
>         state.duty_cycle = (ratio * args.period) / 100;
>         state.period = args.period;
>         state.flags = args.flags;
>
>         pwm_apply_state(pwm, &state);
>
> The ->set_voltage() implementation would then never need to care about
> the PWM args, but rather do something like this:
>
>         struct pwm_state state;
>         unsigned int ratio;
>
>         pwm_get_state(pwm, &state);
>
>         state.duty_cycle = (ratio * state.period) / 100;
>
>         pwm_apply_state(pwm, &state);
>
> Does that sound about right?

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

I'm curious, though.  In your proposal, how does pwm_get_period()
behave?  To be backward compatible, I'd imagine that even in your
proposal we'd have the same definition as I had above:

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


If you have a different definition of pwm_get_period() in your
proposal, please let me know!  If my definition matches your thoughts
then I think we can actually just not touch the "set_voltage" call.
It can always use pwm_get_period() and always use pwm_config() just
like today.

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


Thus, if HW readout:

* In probe, we read HW state (pwm_get_hw_state) and atomically adjust
(pwm_apply_state) based on arguments (pwm_get_args).

* In set_voltage we use pwm_get_period which will return the period we
applied in pwm_apply_state() and use pwm_config() to change the duty
cycle.


If no HW readout, no behavior change at all from today:

* In probe we don't do anything to change the PWM

* Upon the first set_voltage we use pwm_get_period() to get the period
as specified in DT and use pwm_config() to change the duty cycle.


That seems pretty sane to me.  What do you think?

-Doug

[toc] | [prev] | [next] | [standalone]

#1340922

[Multipart message — attachments visible in raw view] — view raw

On Tue, Feb 23, 2016 at 09:35:48AM -0800, Doug Anderson wrote:
> On Tue, Feb 23, 2016 at 6:38 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
[...]
> > That's not quite what I was thinking. If hardware readout is supported
> > then whatever we report back should be the current hardware state unless
> > we're explicitly asked for something else. If we start mixing the state
> > and legacy APIs this way, we'll get into a situation where drivers that
> > support hardware readout behave differently than drivers that don't.
> >
> > For example: A PWM device that's controlled by a driver that supports
> > hardware readout has a current period of 50000 ns and the firmware set
> > the period to 25000 ns. pwm_get_period() for this PWM device will return
> > 50000 ns. If you reconfigure the PWM to generate a PWM signal with a
> > period of 30000 ns, pwm_get_period() would still return 50000 ns.
> >
> > A driver that doesn't support hardware readout, on the contrary, would
> > return 50000 ns from pwm_get_period() on the first call, but after you
> > have reconfigured it using pwm_config() it will return the new period.
> 
> Ah, right!  I forgot that the existing API will be updated if you've
> reconfigured the period via pwm_config().  Ugh, you're right that's a
> little ugly then.
> 
> So do we define it as:
> 
> pwm_get_state(): always get the hardware state w/ no caching (maybe
> even pwm_get_raw_state() or pwm_get_hw_state())

Caching vs. no caching should be irrelevant here. Unless PWM hardware is
autonomous the current state will always match the hardware state after
the initial hardware readout.

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

> Is that OK?  That is well defined and doesn't change the existing
> behavior of pwm_get_period().

pwm_get_period() is legacy API and in order to transition to the atomic
API it should be implemented in terms of atomic API. So the goal is to
get everything internally converted to deal with states only, then wrap
the existing API around that concept. pwm_get_period() would become:

	unsigned int pwm_get_period(struct pwm_device *pwm)
	{
		struct pwm_state state;

		pwm_get_state(pwm, &state);

		return state.period;
	}

If we don't do that, we'll never be able to get rid of the legacy API.

> >> > That way if you want to get the current voltage in the regulator-pwm
> >> > driver you'd simply do a pwm_get_state() and compute the voltage from
> >> > the period and duty cycle. If the PWM driver that you happen to use
> >> > doesn't support hardware readout, you'll get an initial output voltage
> >> > of 0, which is as good as any, really.
> >>
> >> Sounds fine to me.  PWM regulator is in charge of calling
> >> pwm_get_state(), which can return 0 (or an error?) if driver (or
> >> underlying hardware) doesn't support hardware readout.  PWM regulator
> >> is in charge of using the resulting period / duty cycle to calculate a
> >> percentage.
> >
> > I'm not sure if pwm_get_state() should ever return an error. For drivers
> > that support hardware readout, the resulting state should match what's
> > programmed to the hardware.
> >
> > But for drivers without hardware readout support pwm_get_state() still
> > makes sense because after a pwm_apply_state() the internal logical state
> > would again match hardware.
> 
> I guess it depends on how you define things.  With my above
> definitions it seems clearest if pwm_get_state() returns an error if
> hardware readout is not supported.  If we call it pwm_get_hw_state()
> it's even clearer that it should return an error.

Again, if we do this, we'll have to keep the legacy API around forever
and keep special-casing atomic vs. legacy API in every user. The goal is
to converge on the atomic API as the standard API in users so that the
legacy API can be removed when all users have been converted.

> > To allow your use-case to work we'd need to deal with two states: the
> > current hardware state and the "initial" state as defined by DT.
> > Unfortunately the PWM specifier in DT is not a full definition, it is
> > more like a partial initial configuration. The problem with that, and
> > I think that's what Mark was originally objecting to, is that it isn't
> > clear when to use the "initial" state and when to use the read hardware
> > state. After the first pwm_apply_state() you wouldn't ever have to use
> > the "initial" state again, because it's the same as the current state
> > (modulo the duty cycle).
> >
> > Also for drivers such as clk-pwm the usefulness of the "initial" state
> > is reduced even more, because it doesn't even need the period specified
> > in DT. It uses only the flags (if at all).
> >
> > Perhaps to avoid this confusion a new type of object, e.g. pwm_args,
> > could be introduced to hold configuration arguments given in the PWM
> > specifier (in DT) or the PWM lookup table (in board files).
> >
> > It would then be the responsibility of the users to deal with that
> > information in a sensible way. In (almost?) all cases I would expect
> > that to be to program the PWM device in the user's ->probe(). In the
> > case of regulator-pwm I'd expect that ->probe() would do something
> > along these lines (error handling excluded):
> >
> >         struct pwm_state state;
> >         struct pwm_args args;
> >         unsigned int ratio;
> >
> >         pwm = pwm_get(...);
> >
> >         pwm_get_state(pwm, &state);
> >         pwm_get_args(pwm, &args);
> >
> >         ratio = (state.duty_cycle * 100) / state.period;
> >
> >         state.duty_cycle = (ratio * args.period) / 100;
> >         state.period = args.period;
> >         state.flags = args.flags;
> >
> >         pwm_apply_state(pwm, &state);
> >
> > The ->set_voltage() implementation would then never need to care about
> > the PWM args, but rather do something like this:
> >
> >         struct pwm_state state;
> >         unsigned int ratio;
> >
> >         pwm_get_state(pwm, &state);
> >
> >         state.duty_cycle = (ratio * state.period) / 100;
> >
> >         pwm_apply_state(pwm, &state);
> >
> > Does that sound about right?
> 
> 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.

> I'm curious, though.  In your proposal, how does pwm_get_period()
> behave?  To be backward compatible, I'd imagine that even in your
> proposal we'd have the same definition as I had above:
> 
> 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).

It would simply return the logical period of the PWM. For drivers that
support hardware readout it would always match the hardware period, but
for drivers that don't it might be wrong until state is first applied.

> If you have a different definition of pwm_get_period() in your
> proposal, please let me know!  If my definition matches your thoughts
> then I think we can actually just not touch the "set_voltage" call.
> It can always use pwm_get_period() and always use pwm_config() just
> like today.
> 
> ...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.

> Thus, if HW readout:
> 
> * In probe, we read HW state (pwm_get_hw_state) and atomically adjust
> (pwm_apply_state) based on arguments (pwm_get_args).
> 
> * In set_voltage we use pwm_get_period which will return the period we
> applied in pwm_apply_state() and use pwm_config() to change the duty
> cycle.
> 
> 
> If no HW readout, no behavior change at all from today:
> 
> * In probe we don't do anything to change the PWM
> 
> * Upon the first set_voltage we use pwm_get_period() to get the period
> as specified in DT and use pwm_config() to change the duty cycle.
> 
> 
> That seems pretty sane to me.  What do you think?

This has the big disadvantage of having to special case hardware readout
vs. non-hardware readout providers. I think that makes the API really
difficult to use. It requires too many details to be aware of.

I guess this boils down to whether applying the "initial" state on probe
really is problematic.

Thierry

[toc] | [prev] | [next] | [standalone]

#1340946

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


-Doug

[toc] | [prev] | [next] | [standalone]

#1343571

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web