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

Groups > linux.kernel > #1339760 > 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-22 19:00 +0100
    Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-02-22 20:20 +0100
      Re: [PATCH v3 00/12] pwm: add support for atomic update Mark Brown <broonie@kernel.org> - 2016-02-22 22:30 +0100
        Re: [PATCH v3 00/12] pwm: add support for atomic update Doug Anderson <dianders@google.com> - 2016-02-23 04:10 +0100
      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

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

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

On Wed, Feb 03, 2016 at 11:04:20AM -0800, Doug Anderson wrote:
> Thierry
> 
> On Wed, Feb 3, 2016 at 6:53 AM, Thierry Reding <thierry.reding@gmail.com> wrote:
> >> A) The software state here is the period and flags (AKA "inverted),
> >> right?  It does seem possible that you could apply the period and
> >> flags while keeping the calculated bootup duty cycle percentage
> >> (presuming that the PWM was actually enabled at probe time and there
> >> was a bootup duty cycle at all).  That would basically say that
> >> whenever you set the period of a PWM then the duty cycle of the PWM
> >> should remain the same percentage.  That actually seems quite sane
> >> IMHO.  It seems much saner than trying to keep the duty cycle "ns"
> >> when the period changes or resetting the PWM to some default when the
> >> period changes.
> >
> > That really depends on the use-case. If you're interested in the output
> > power of the PWM then, yes, this is sane. But it might not be the right
> > answer in other cases.
> 
> Ah, I see.  You're envisioning a device where active time in "ns" is
> more important than the percentage of active time.  Perhaps an LED
> where it's more important to have it on for no more than .1 seconds
> (so we don't drive it too long and burn it out?).  If we slowed down
> the duty cycle and adjusted the period to match, we could get into a
> bad state.

Yes. Granted, the vast majority of users is not in this category, but
it's something that has been brought to my attention in the past and it
works fine with the current API.

> >> B) Alternatively, I'd also say that setting a period without a duty
> >> cycle doesn't make a lot of sense.  ...so you could just apply the
> >> period at the same time that you apply the duty cycle the first time.
> >> Presumably you'd want to "lie" to the callers of the PWM subsystem and
> >> tell them that you already changed the period even though the change
> >> won't really take effect until they actually set the duty cycle.  If
> >> anyone cared to find out the true hardware period we could add a new
> >> pwm_get_hw_period().  ...or since the only reason you'd want to know
> >> the hardware period would be if you're trying to read the current duty
> >> cycle percentage, you could instead add "pwm_get_hw_state()" and have
> >> that return both the hardware period ns and duty cycle ns (which is
> >> the most accurate way to return the "percentage" without using fix or
> >> floating point math).
> >
> > But then you get into a situation where behaviour is dependent on the
> > PWM driver, whereas this is really very specific to one specific use-
> > case.
> 
> This is because only some drivers would be able to read the hardware
> state?  I'm not sure how we can get away from that.  In all proposals
> we've talked about (including what you propose below, right?) the PWM
> regulator will need a PWM driver that can read hardware state.  Only
> PWM drivers that have been upgraded to support reading hardware state
> can use the PWM regulator (or at least only those drivers will be able
> to use the PWM regulator glitch-free).

Yes, the key here is glitch-free. There's no reason whatsoever that the
rugaltor-pwm driver should be limited to usage with a hardware readout-
capable PWM driver. If you don't care about glitches, likely because no
critical components depend on the regulator, you can simply force what
state you choose on boot.

As a matter of fact, I think that's how regulators work already. If the
current output voltage doesn't match the specified constraints, then a
valid value will be forced by the regulator core. If the voltage lies
within the constraints the core won't touch the regulator. Is this not
going to "just work" with the PWM regulator?

The problem is somewhat simplified if that's the case. An implementation
could then fail the regulator_get_voltage() if hardware readout is not
supported and return the current voltage when readout is possible.

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

> > In the end the PWM API is as low-level as it is because it needs to be
> > flexible enough to cope with other use-cases. In the general case the
> > simple truth is that it doesn't make sense to set a period without the
> > duty cycle and vice versa. That's why pwm_config() takes both as input
> > parameters. The atomic API is going to take that one step further in
> > that you need to specify the complete state of the PWM when applying.
> 
> I guess this is still showing the strangeness of the device tree
> specifying a period without a duty cycle.  You just said it doesn't
> make sense, but that's exactly what the device tree has.

The device tree specifies the period because there is no way for the
driver (such as pwm-backlight) to "guess" the right one. Furthermore
there is a certain degree of board-specificity to that parameter and
therefore any heuristic trying to come up with a sensible value will
inevitably fail eventually.

> I'd imagine that the only reason that the device tree specifies just
> the period is that it's intended to be there only for clients that
> don't care about the specific duty cycle in terms of seconds but
> _only_ care about the duty cycle in terms of percentage.  Anyone who
> cared about the duty cycle in terms of seconds would presumably also
> care about specifying the period (in terms of seconds) in the same
> place they specify the duty cycle.

Yes, exactly.

> >> > That doesn't really get us closer, though. There is still the issue of
> >> > the user having to deal with two states: the current hardware state and
> >> > the software state as configured in DT or board files.
> >>
> >> I think the only users that need to deal with this are one that need a
> >> seamless transition from bootup settings.  Adding a new API call to
> >> support a new feature like this doesn't seem insane, and anyone who
> >> doesn't want this new feature can just never call the new API.
> >>
> >> The only thing that would "change" from the point of view of old
> >> drivers is that the PWM period wouldn't change at bootup until the
> >> duty cycle was set.  IMHO this is probably a bug fix.  AKA, for a PWM
> >> backlight, imagine:
> >>
> >> 1. Firmware sets period to 20000 ns, duty cycle to 8000 ns (40%)
> >> 2. Linux boots up and sets period to 10000 ns.  Brightness of
> >> backlight instantly goes to 80%.
> >> 3. Eventually something decides to set the backlight duty cycle and it
> >> goes to the proper rate.
> >>
> >> Skipping #2 seems like the right move.  ...or did I misunderstand how
> >> something works?
> >
> > I'm not aware of any code in the PWM subsystem that would do this
> > automatically. If you don't call any of the pwm_*() functions the
> > hardware state should not be modified. The responsibility is with
> > the user drivers.
> 
> Ah, OK.  I must have gotten confused.
> 
> Oh, I see.  So pwm_get_period() is actually "lying" about the hardware
> today!  So what you're saying is that at boot time we grab the period
> out of the device tree but we _don't_ apply it to the hardware, right?
>  I was thinking it would get applied right away...

That's correct. The value retrieved by pwm_get_period() is the one
specified in DT (or a PWM lookup table). It should match the hardware
value after the first call to pwm_config(), though.

> That means that if you call pwm_get_period() right away at boot time
> you're not getting the current period of the hardware but the period
> that was specified in the device tree.

Yes.

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

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.

> > That is, it is up to the regulator or backlight driver to apply any new
> > configuration to a PWM channel on boot, or leave it as is. For
> > backlight it's probably fine to simply apply some default, since having
> > the brightness change on boot isn't going to be terribly irritating.
> >
> > With the atomic API it would be possible to avoid even this and have the
> > backlight driver simply read out the current hardware state and apply an
> > equivalent brightness even if the period changed.
> >
> > For the regulator case you'd need to read out the current state and then
> > recompute the values that will yield the same output power given data
> > specified in DT. I think that much is already implemented in Boris'
> > series, and it's really only the details that are being debated.
> >
> > The problematic issue is still that we might have a disparity between
> > the current hardware state and the state initially specified by DT. In
> > the general case the DT will specify the period and polarity of the PWM
> > signal and leave it up to the user driver to determine what a correct
> > duty cycle would be. With the atomic API we'll essentially have two
> > states: the current (hardware) state and the "initial" state, which is
> > what an OS will see as the state to apply. The problem now is that once
> > you have applied the initial state with a duty cycle you've determined,
> > there is no longer a need to keep it around. But there's also no way to
> > know when this is the case. So the controversial part about all this is
> > when to start using the current state rather that the initial state.
> >
> > The most straightforward way to solve this would be to apply the initial
> > configuration on driver probe. That is, when the pwm-regulator driver
> > gets probed it would retrieve the current and initial states, then
> > adjust the current state such that it matches the initial state but with
> > a duty cycle that yields the same output power as the current state, and
> > finally apply the new state. After that, every regulator_set_voltage()
> > call could simply operate on the current state and adjust the duty cycle
> > exclusively.
> >
> > Does that sound reasonable?
> 
> Sure.  ...but you agree that somehow you need a new API call for this,
> right?  Somehow the PWM regulator needs to be able to say that it
> wants the hardware state, not the initial state as specified in the
> device tree.

The atomic PWM API is this new API. I'm not arguing that we don't need
new API. What's being discussed is what the API needs to look like to
support this new use-case and at the same time be maximally compatible
with existing users.

I think perhaps one question that needs answering to do that is whether
or not the regulator-pwm driver can guess the correct period. The issue
that was initially being discussed is what to do with hardware state vs
initial state (i.e. what's defined in DT). Mark commented in another
subthread that DT should simply leave out data that doesn't make sense
to be specified.

However I don't think there's any particularly reasonable period for the
regulator-pwm use-case, so specifying the period within DT would still
be necessary. What works for one board may not be the correct (or
optimal) value for another. Similarly one board may need to invert the
PWM signal to generate the proper voltage, or require other parameters
that we haven't even defined yet.

There's also the issue of a voltage table that you need to define in the
DT for the regulator-pwm device. Does that consist of only duty-cycle
values, or does it have corresponding period values as well. I'd guess
that it really only needs the duty cycle given any period. So something
like this is what I'd expect:

	regulator {
		compatible = "regulator-pwm";

		pwms = <&pwm0 5000>;

		voltages = <0 0>, <1000000 1000>, ..., <5000000 5000>;
	};

I think the pwm-regulator binding defines the duty cycle in percent. I
guess that would work equally well.

And now we've come full circle because, again, we need to differentiate
between the current hardware state and the initial state. Or, as was
also discussed previously, alternatively, ignore the period specified in
DT and just go with what hardware readout defined. In case hardware
readout isn't supported, the "hardware state" will simply be the
"initial state".

The objection to the latter alternative was that we shouldn't trust the
firmware to have set up the regulator correctly. But if it didn't, how
can we trust that the duty cycle to period ratio is correct? Or the
other way around: if we trust the duty cycle to period ratio to have
been setup correctly, why don't we trust the period?

Thierry

[toc] | [next] | [standalone]

#1339850

Thierry,

On Mon, Feb 22, 2016 at 9:59 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
>> This is because only some drivers would be able to read the hardware
>> state?  I'm not sure how we can get away from that.  In all proposals
>> we've talked about (including what you propose below, right?) the PWM
>> regulator will need a PWM driver that can read hardware state.  Only
>> PWM drivers that have been upgraded to support reading hardware state
>> can use the PWM regulator (or at least only those drivers will be able
>> to use the PWM regulator glitch-free).
>
> Yes, the key here is glitch-free. There's no reason whatsoever that the
> rugaltor-pwm driver should be limited to usage with a hardware readout-
> capable PWM driver. If you don't care about glitches, likely because no
> critical components depend on the regulator, you can simply force what
> state you choose on boot.
>
> As a matter of fact, I think that's how regulators work already. If the
> current output voltage doesn't match the specified constraints, then a
> valid value will be forced by the regulator core. If the voltage lies
> within the constraints the core won't touch the regulator. Is this not
> going to "just work" with the PWM regulator?
>
> The problem is somewhat simplified if that's the case. An implementation
> could then fail the regulator_get_voltage() if hardware readout is not
> supported and return the current voltage when readout is possible.

Based on looking at the current code, I believe it just returns 0V
until you call regulator_set_voltage() today.  I don't think that was
always the case.  Back before it was made continuous I think it
returned the voltage that matched with 0% duty cycle.

I haven't dug into what the regulator framework does in the current
system nor what happens if regulator_get_voltage() returns an error.
Perhaps Boris can dig / comment?


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


>> That means that if you call pwm_get_period() right away at boot time
>> you're not getting the current period of the hardware but the period
>> that was specified in the device tree.
>
> Yes.
>
>> 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 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.


>> Sure.  ...but you agree that somehow you need a new API call for this,
>> right?  Somehow the PWM regulator needs to be able to say that it
>> wants the hardware state, not the initial state as specified in the
>> device tree.
>
> The atomic PWM API is this new API. I'm not arguing that we don't need
> new API. What's being discussed is what the API needs to look like to
> support this new use-case and at the same time be maximally compatible
> with existing users.
>
> I think perhaps one question that needs answering to do that is whether
> or not the regulator-pwm driver can guess the correct period. The issue
> that was initially being discussed is what to do with hardware state vs
> initial state (i.e. what's defined in DT). Mark commented in another
> subthread that DT should simply leave out data that doesn't make sense
> to be specified.
>
> However I don't think there's any particularly reasonable period for the
> regulator-pwm use-case, so specifying the period within DT would still
> be necessary. What works for one board may not be the correct (or
> optimal) value for another. Similarly one board may need to invert the
> PWM signal to generate the proper voltage, or require other parameters
> that we haven't even defined yet.
>
> There's also the issue of a voltage table that you need to define in the
> DT for the regulator-pwm device. Does that consist of only duty-cycle
> values, or does it have corresponding period values as well. I'd guess
> that it really only needs the duty cycle given any period. So something
> like this is what I'd expect:
>
>         regulator {
>                 compatible = "regulator-pwm";
>
>                 pwms = <&pwm0 5000>;
>
>                 voltages = <0 0>, <1000000 1000>, ..., <5000000 5000>;
>         };
>
> I think the pwm-regulator binding defines the duty cycle in percent. I
> guess that would work equally well.
>
> And now we've come full circle because, again, we need to differentiate
> between the current hardware state and the initial state. Or, as was
> also discussed previously, alternatively, ignore the period specified in
> DT and just go with what hardware readout defined. In case hardware
> readout isn't supported, the "hardware state" will simply be the
> "initial state".
>
> The objection to the latter alternative was that we shouldn't trust the
> firmware to have set up the regulator correctly. But if it didn't, how
> can we trust that the duty cycle to period ratio is correct? Or the
> other way around: if we trust the duty cycle to period ratio to have
> been setup correctly, why don't we trust the period?

To answer:

* No, PWM regulator can't guess the correct period.

* PWM regulator API is already fine as is.  Period specified in PWM
specifier and table is specified in percentages.

* Firmware may have voltage setup correctly but may not have the
"ideal" period.  Often firmware configures things in a way that works
but is sub-optimal.  Basically the kernel should be able to tell what
voltage the firmware set things up at (so we know not to go lower on
accident), but we shouldn't assume that the firmware values are
perfect.  Said another way: obviously the firmware made values that
were good enough to boot us to where we are (or we wouldn't be even
executing code), but we might want to configure things to reduce noise
on the lines (make HDMI work better?) or optimize power consumption.

--

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.

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

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.

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


Said another way: the only action item from this point of view is just
that we introduce a new pwm_get_state().


Boris: I think that's everything you need, right?

--

Historically there was also a necessity that we were very careful with
the PWM clock because the clock would end up getting disabled
temporarily at bootup (after the PWM probe time but before the PWM
regulator finished probing).  Presumably that's either been solved
already or can be debated totally separately.


-Doug

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

#1339937

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

On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:

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

You do need to fix the users though, change is fine but you can't cause
people's systems to break.

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

#1340196

Mark,

On Mon, Feb 22, 2016 at 1:24 PM, Mark Brown <broonie@kernel.org> wrote:
> On Mon, Feb 22, 2016 at 11:15:09AM -0800, Doug Anderson wrote:
>
>> 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.
>
> You do need to fix the users though, change is fine but you can't cause
> people's systems to break.

Yes, of course!  :)  Thanks for clarifying.

-Doug

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

#1340713

[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] | [prev] | [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