[v2,1/3] pwm: rockchip: Don't update the state for the caller of pwm_apply_state()

Commit Message

Uwe Kleine-König March 12, 2019, 9:46 p.m. UTC

The pwm-rockchip driver is one of only two PWM drivers which updates the
state for the caller of pwm_apply_state(). This might have surprising
results if the caller reuses the values expecting them to still
represent the same state. Also note that this feedback was incomplete as
the matching struct pwm_device::state wasn't updated and so
pwm_get_state still returned the originally requested state.

Signed-off-by: Uwe Kleine-König <[e-mail]>
---
 drivers/pwm/pwm-rockchip.c | 6 ------
 1 file changed, 6 deletions(-)

Comments

Heiko Stuebner March 30, 2019, 9:17 a.m. UTC | #1

Hi,

[adding two chromeos people, because veyron and gru are quite
heavy users of the rockchip pwm for both backlight and regulators]

Doug, Brian: patchwork patch is here:
https://patchwork.kernel.org/patch/10851001/

Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> The pwm-rockchip driver is one of only two PWM drivers which updates the
> state for the caller of pwm_apply_state(). This might have surprising
> results if the caller reuses the values expecting them to still
> represent the same state. Also note that this feedback was incomplete as
> the matching struct pwm_device::state wasn't updated and so
> pwm_get_state still returned the originally requested state.
> 
> Signed-off-by: Uwe Kleine-König <[e-mail]>

I've tested this on both veyron and gru with backlight and pwm regulator
and at least both still come up, so
Tested-by: Heiko Stuebner <[e-mail]>

But hopefully Doug or Brian could also provide another test-point.

Heiko

> ---
>  drivers/pwm/pwm-rockchip.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
> index 4d99d468df09..16186bcd99e0 100644
> --- a/drivers/pwm/pwm-rockchip.c
> +++ b/drivers/pwm/pwm-rockchip.c
> @@ -215,12 +215,6 @@ static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
>  			goto out;
>  	}
>  
> -	/*
> -	 * Update the state with the real hardware, which can differ a bit
> -	 * because of period/duty_cycle approximation.
> -	 */
> -	rockchip_pwm_get_state(chip, pwm, state);
> -
>  out:
>  	clk_disable(pc->pclk);
>  
>

Doug Anderson April 1, 2019, 10:45 p.m. UTC | #2

Hi,

On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
>
> Hi,
>
> [adding two chromeos people, because veyron and gru are quite
> heavy users of the rockchip pwm for both backlight and regulators]
>
> Doug, Brian: patchwork patch is here:
> https://patchwork.kernel.org/patch/10851001/
>
> Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > state for the caller of pwm_apply_state(). This might have surprising
> > results if the caller reuses the values expecting them to still
> > represent the same state.

It may or may not be surprising, but it is well documented.  Specifically:

 * pwm_apply_state() - atomically apply a new state to a PWM device
 * @pwm: PWM device
 * @state: new state to apply. This can be adjusted by the PWM driver
 *    if the requested config is not achievable, for example,
 *    ->duty_cycle and ->period might be approximated.

I don't think your series updates that documentation, right?


> > Also note that this feedback was incomplete as
> > the matching struct pwm_device::state wasn't updated and so
> > pwm_get_state still returned the originally requested state.

The framework handles that.  Take a look at pwm_apply_state()?  It does:

---

err = pwm->chip->ops->apply(pwm->chip, pwm, state);
if (err)
    return err;

pwm->state = *state;

---

So I think it wasn't incomplete unless I misunderstood?


> > Signed-off-by: Uwe Kleine-König <[e-mail]>
>
> I've tested this on both veyron and gru with backlight and pwm regulator
> and at least both still come up, so
> Tested-by: Heiko Stuebner <[e-mail]>
>
> But hopefully Doug or Brian could also provide another test-point.

I'd definitely be concerned by this change.  Specifically for the PWM
regulator little details about exactly what duty cycle / period you
got could be pretty important.

I guess the problem here is that pwm_get_state() doesn't actually call
into the PWM drivers, it just returns the last state that was applied.
How does one get the state?  I guess you could change get_state() to
actually call into the PWM driver's get_state if it exists?  ...but
your patch set doesn't change that behavior...

For instance, look at pwm_regulator_set_voltage().  The first line
there is  pwm_init_state().  That calls into pwm_get_state() which
just grabs the cached state.  If the last call to pwm_apply_state()
didn't update that then it seems like it'd be bad.


NOTE: it's _possible_ you could still change the pwm_apply_state()
function to be const.  I think most of the need here is that future
calls to pwm_get_state() get the actual state, not the wished-for
state.



-Doug

Uwe Kleine-König April 8, 2019, 2:39 p.m. UTC | #3

On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
> >
> > Hi,
> >
> > [adding two chromeos people, because veyron and gru are quite
> > heavy users of the rockchip pwm for both backlight and regulators]
> >
> > Doug, Brian: patchwork patch is here:
> > https://patchwork.kernel.org/patch/10851001/
> >
> > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > > state for the caller of pwm_apply_state(). This might have surprising
> > > results if the caller reuses the values expecting them to still
> > > represent the same state.
> 
> It may or may not be surprising, but it is well documented.  Specifically:
> 
>  * pwm_apply_state() - atomically apply a new state to a PWM device
>  * @pwm: PWM device
>  * @state: new state to apply. This can be adjusted by the PWM driver
>  *    if the requested config is not achievable, for example,
>  *    ->duty_cycle and ->period might be approximated.
> 
> I don't think your series updates that documentation, right?
> 
> 
> > > Also note that this feedback was incomplete as
> > > the matching struct pwm_device::state wasn't updated and so
> > > pwm_get_state still returned the originally requested state.
> 
> The framework handles that.  Take a look at pwm_apply_state()?  It does:
> 
> ---
> 
> err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> if (err)
>     return err;
> 
> pwm->state = *state;

> 
> ---
> 
> So I think it wasn't incomplete unless I misunderstood?

You're right, I missed that the updated state was saved.

> > > Signed-off-by: Uwe Kleine-König <[e-mail]>
> >
> > I've tested this on both veyron and gru with backlight and pwm regulator
> > and at least both still come up, so
> > Tested-by: Heiko Stuebner <[e-mail]>
> >
> > But hopefully Doug or Brian could also provide another test-point.
> 
> I'd definitely be concerned by this change.  Specifically for the PWM
> regulator little details about exactly what duty cycle / period you
> got could be pretty important.
> 
> I guess the problem here is that pwm_get_state() doesn't actually call
> into the PWM drivers, it just returns the last state that was applied.
> How does one get the state?  I guess you could change get_state() to
> actually call into the PWM driver's get_state if it exists?  ...but
> your patch set doesn't change that behavior...

My intention here is more to make all drivers behave the same way and
because only two drivers updated the pwm_state this was the variant I
removed.

When you say that the caller might actually care about the exact
parameters I fully agree. In this case however the consumer should be
able to know the result before actually applying it. So if you do

	pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})

and this results in .period = 100 and .duty_cycle = 0 then probably the
bad things you want to know about already happend. So my idea is a new
function pwm_round_state() that does the adaptions to pwm_state without
applying it to the hardware. After that pwm_apply_state could do the
following:

	rstate = pwm_round_state(pwm, state)
	pwm.apply(pwm, state)
	gstate = pwm_get_state(pwm)

	if rstate != gstate:
		warn about problems

But before doing that I think it would be sensible to also fix the rules
how the round_state callback is supposed to round.

Apart from that I agree that pwm_get_state should return the actually
implemented configuration.

Best regards
Uwe

Brian Norris April 19, 2019, 12:27 a.m. UTC | #4

Hi,

I'm not sure if I'm misreading you, but I thought I'd add here before
this expires out of my inbox:

On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
<[e-mail]> wrote:
> My intention here is more to make all drivers behave the same way and
> because only two drivers updated the pwm_state this was the variant I
> removed.

To be clear, this patch on its own is probably breaking things. Just
because the other drivers don't implement the documented behavior
doesn't mean you should break this driver. Maybe the others just
aren't used in precise enough scenarios where this matters.

> When you say that the caller might actually care about the exact
> parameters I fully agree. In this case however the consumer should be
> able to know the result before actually applying it. So if you do
>
>         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
>
> and this results in .period = 100 and .duty_cycle = 0 then probably the
> bad things you want to know about already happend. So my idea is a new
> function pwm_round_state() that does the adaptions to pwm_state without
> applying it to the hardware. After that pwm_apply_state could do the
> following:
>
>         rstate = pwm_round_state(pwm, state)
>         pwm.apply(pwm, state)
>         gstate = pwm_get_state(pwm)
>
>         if rstate != gstate:
>                 warn about problems

For our case (we're using this with pwm-regulator), I don't recall [*]
we need to be 100% precise about the period, but we do need to be as
precise as possible with the duty:period ratio -- so once we get the
"feedback" from the underlying PWM driver what the real period will
be, we adjust the duty appropriately.

So I don't see that "warning" would really help for this particular case.

> But before doing that I think it would be sensible to also fix the rules
> how the round_state callback is supposed to round.

I'm not quite sure I grok exactly what you're planning, but I would
much appreciate if you didn't break things on the way toward fixing
them ;)

Thanks,
Brian

[*] Working off a somewhat stale memory and a refresher pass over the code.

> Apart from that I agree that pwm_get_state should return the actually
> implemented configuration.
>
> Best regards
> Uwe

Uwe Kleine-König April 29, 2019, 6:56 a.m. UTC | #5

On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> Hi,
> 
> I'm not sure if I'm misreading you, but I thought I'd add here before
> this expires out of my inbox:
> 
> On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> <[e-mail]> wrote:
> > My intention here is more to make all drivers behave the same way and
> > because only two drivers updated the pwm_state this was the variant I
> > removed.
> 
> To be clear, this patch on its own is probably breaking things. Just
> because the other drivers don't implement the documented behavior
> doesn't mean you should break this driver. Maybe the others just
> aren't used in precise enough scenarios where this matters.
> 
> > When you say that the caller might actually care about the exact
> > parameters I fully agree. In this case however the consumer should be
> > able to know the result before actually applying it. So if you do
> >
> >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> >
> > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > bad things you want to know about already happend. So my idea is a new
> > function pwm_round_state() that does the adaptions to pwm_state without
> > applying it to the hardware. After that pwm_apply_state could do the
> > following:
> >
> >         rstate = pwm_round_state(pwm, state)
> >         pwm.apply(pwm, state)
> >         gstate = pwm_get_state(pwm)
> >
> >         if rstate != gstate:
> >                 warn about problems
> 
> For our case (we're using this with pwm-regulator), I don't recall [*]
> we need to be 100% precise about the period, but we do need to be as
> precise as possible with the duty:period ratio -- so once we get the
> "feedback" from the underlying PWM driver what the real period will
> be, we adjust the duty appropriately.

I admit that I didn't understood the whole situation and (some) things
are worse with my patches applied. I still think that changing the
caller's state variable is bad design, but of course pwm_get_state
should return the currently implemented configuration.

> So I don't see that "warning" would really help for this particular case.
> 
> > But before doing that I think it would be sensible to also fix the rules
> > how the round_state callback is supposed to round.
> 
> I'm not quite sure I grok exactly what you're planning, but I would
> much appreciate if you didn't break things on the way toward fixing
> them ;)

There are currently no rules how the driver should behave for example if
the consumer requests

	.duty_cycle = 10, .period = 50

and the hardware can only implement multiples of 3 for both values. The
obvious candidates are:

 - .duty_cycle = 9, .period = 51 (round nearest for both)
 - .duty_cycle = 12, .period = 51 (round up)
 - .duty_cycle = 9, .period = 48 (round down)
 - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
 - return error (which code?)

And there are some other variants (e.g. round duty_cycle to nearest and
period in the same direction) that might be sensible.

Also it should be possible to know the result before actually
configuring the hardware. Otherwise things might already go wrong
because the driver implements a setting that is too far from the
requested configuration.

Best regards
Uwe

Thierry Reding April 29, 2019, 10:49 a.m. UTC | #6

On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
> >
> > Hi,
> >
> > [adding two chromeos people, because veyron and gru are quite
> > heavy users of the rockchip pwm for both backlight and regulators]
> >
> > Doug, Brian: patchwork patch is here:
> > https://patchwork.kernel.org/patch/10851001/
> >
> > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > > state for the caller of pwm_apply_state(). This might have surprising
> > > results if the caller reuses the values expecting them to still
> > > represent the same state.
> 
> It may or may not be surprising, but it is well documented.  Specifically:
> 
>  * pwm_apply_state() - atomically apply a new state to a PWM device
>  * @pwm: PWM device
>  * @state: new state to apply. This can be adjusted by the PWM driver
>  *    if the requested config is not achievable, for example,
>  *    ->duty_cycle and ->period might be approximated.
> 
> I don't think your series updates that documentation, right?

The documentation is arguably ambiguous here, but I don't think this was
meant as you intepret here. I think the original intent was to give the
drivers some leeway in how they apply a state. So a driver could for
example adjust duty_cycle or period if it doesn't support exactly the
combination requested. However, I don't think it was meant as a
suggestion that it would report that back to the caller.

This obviously implies that ->apply() is deterministic, so given a state
it would program the same register values, irrespective of when, or how
many times that state is applied.

> > > Also note that this feedback was incomplete as
> > > the matching struct pwm_device::state wasn't updated and so
> > > pwm_get_state still returned the originally requested state.
> 
> The framework handles that.  Take a look at pwm_apply_state()?  It does:
> 
> ---
> 
> err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> if (err)
>     return err;
> 
> pwm->state = *state;
> 
> ---
> 
> So I think it wasn't incomplete unless I misunderstood?
> 
> 
> > > Signed-off-by: Uwe Kleine-König <[e-mail]>
> >
> > I've tested this on both veyron and gru with backlight and pwm regulator
> > and at least both still come up, so
> > Tested-by: Heiko Stuebner <[e-mail]>
> >
> > But hopefully Doug or Brian could also provide another test-point.
> 
> I'd definitely be concerned by this change.  Specifically for the PWM
> regulator little details about exactly what duty cycle / period you
> got could be pretty important.
> 
> I guess the problem here is that pwm_get_state() doesn't actually call
> into the PWM drivers, it just returns the last state that was applied.
> How does one get the state?  I guess you could change get_state() to
> actually call into the PWM driver's get_state if it exists?  ...but
> your patch set doesn't change that behavior...
> 
> For instance, look at pwm_regulator_set_voltage().  The first line
> there is  pwm_init_state().  That calls into pwm_get_state() which
> just grabs the cached state.  If the last call to pwm_apply_state()
> didn't update that then it seems like it'd be bad.

The whole point of this atomic API is that the cached state would always
match the hardware state. Given what I said above that doesn't
necessarily mean that the cached state exactly matches the values that
were written to hardware.

But it does mean that the following is idempotent:

	pwm_get_state(pwm, &state);
	pwm_apply_state(pwm, &state);

Thierry

Thierry Reding April 29, 2019, 11:03 a.m. UTC | #7

On Mon, Apr 08, 2019 at 04:39:14PM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote:
> > Hi,
> > 
> > On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
> > >
> > > Hi,
> > >
> > > [adding two chromeos people, because veyron and gru are quite
> > > heavy users of the rockchip pwm for both backlight and regulators]
> > >
> > > Doug, Brian: patchwork patch is here:
> > > https://patchwork.kernel.org/patch/10851001/
> > >
> > > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > > > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > > > state for the caller of pwm_apply_state(). This might have surprising
> > > > results if the caller reuses the values expecting them to still
> > > > represent the same state.
> > 
> > It may or may not be surprising, but it is well documented.  Specifically:
> > 
> >  * pwm_apply_state() - atomically apply a new state to a PWM device
> >  * @pwm: PWM device
> >  * @state: new state to apply. This can be adjusted by the PWM driver
> >  *    if the requested config is not achievable, for example,
> >  *    ->duty_cycle and ->period might be approximated.
> > 
> > I don't think your series updates that documentation, right?
> > 
> > 
> > > > Also note that this feedback was incomplete as
> > > > the matching struct pwm_device::state wasn't updated and so
> > > > pwm_get_state still returned the originally requested state.
> > 
> > The framework handles that.  Take a look at pwm_apply_state()?  It does:
> > 
> > ---
> > 
> > err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > if (err)
> >     return err;
> > 
> > pwm->state = *state;
> 
> > 
> > ---
> > 
> > So I think it wasn't incomplete unless I misunderstood?
> 
> You're right, I missed that the updated state was saved.
> 
> > > > Signed-off-by: Uwe Kleine-König <[e-mail]>
> > >
> > > I've tested this on both veyron and gru with backlight and pwm regulator
> > > and at least both still come up, so
> > > Tested-by: Heiko Stuebner <[e-mail]>
> > >
> > > But hopefully Doug or Brian could also provide another test-point.
> > 
> > I'd definitely be concerned by this change.  Specifically for the PWM
> > regulator little details about exactly what duty cycle / period you
> > got could be pretty important.
> > 
> > I guess the problem here is that pwm_get_state() doesn't actually call
> > into the PWM drivers, it just returns the last state that was applied.
> > How does one get the state?  I guess you could change get_state() to
> > actually call into the PWM driver's get_state if it exists?  ...but
> > your patch set doesn't change that behavior...
> 
> My intention here is more to make all drivers behave the same way and
> because only two drivers updated the pwm_state this was the variant I
> removed.
> 
> When you say that the caller might actually care about the exact
> parameters I fully agree. In this case however the consumer should be
> able to know the result before actually applying it. So if you do
> 
> 	pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> 
> and this results in .period = 100 and .duty_cycle = 0 then probably the
> bad things you want to know about already happend. So my idea is a new
> function pwm_round_state() that does the adaptions to pwm_state without
> applying it to the hardware. After that pwm_apply_state could do the
> following:
> 
> 	rstate = pwm_round_state(pwm, state)
> 	pwm.apply(pwm, state)
> 	gstate = pwm_get_state(pwm)
> 
> 	if rstate != gstate:
> 		warn about problems

I'm not sure this is really useful. I would expect that in most cases
where it is necessary to have an exact match between the requested state
and the actual state is important, you may not even get to warning about
problems because the system may shut down (e.g. the regulator might not
be outputting enough power to keep the system stable).

I think it'd be far more useful to give consumers a way to request that
the state be applied strictly. I'm not sure how realistic that is,
though. Most PWMs have some sort of restrictions, and in most cases this
might still be okay. Perhaps some sort of permissible relative deviation
factor could be added to give more flexibility?

Thierry

> But before doing that I think it would be sensible to also fix the rules
> how the round_state callback is supposed to round.
> 
> Apart from that I agree that pwm_get_state should return the actually
> implemented configuration.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Thierry Reding April 29, 2019, 11:18 a.m. UTC | #8

On Mon, Apr 29, 2019 at 08:56:13AM +0200, Uwe Kleine-König wrote:
> On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> > Hi,
> > 
> > I'm not sure if I'm misreading you, but I thought I'd add here before
> > this expires out of my inbox:
> > 
> > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > <[e-mail]> wrote:
> > > My intention here is more to make all drivers behave the same way and
> > > because only two drivers updated the pwm_state this was the variant I
> > > removed.
> > 
> > To be clear, this patch on its own is probably breaking things. Just
> > because the other drivers don't implement the documented behavior
> > doesn't mean you should break this driver. Maybe the others just
> > aren't used in precise enough scenarios where this matters.
> > 
> > > When you say that the caller might actually care about the exact
> > > parameters I fully agree. In this case however the consumer should be
> > > able to know the result before actually applying it. So if you do
> > >
> > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > >
> > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > bad things you want to know about already happend. So my idea is a new
> > > function pwm_round_state() that does the adaptions to pwm_state without
> > > applying it to the hardware. After that pwm_apply_state could do the
> > > following:
> > >
> > >         rstate = pwm_round_state(pwm, state)
> > >         pwm.apply(pwm, state)
> > >         gstate = pwm_get_state(pwm)
> > >
> > >         if rstate != gstate:
> > >                 warn about problems
> > 
> > For our case (we're using this with pwm-regulator), I don't recall [*]
> > we need to be 100% precise about the period, but we do need to be as
> > precise as possible with the duty:period ratio -- so once we get the
> > "feedback" from the underlying PWM driver what the real period will
> > be, we adjust the duty appropriately.
> 
> I admit that I didn't understood the whole situation and (some) things
> are worse with my patches applied. I still think that changing the
> caller's state variable is bad design, but of course pwm_get_state
> should return the currently implemented configuration.
> 
> > So I don't see that "warning" would really help for this particular case.
> > 
> > > But before doing that I think it would be sensible to also fix the rules
> > > how the round_state callback is supposed to round.
> > 
> > I'm not quite sure I grok exactly what you're planning, but I would
> > much appreciate if you didn't break things on the way toward fixing
> > them ;)
> 
> There are currently no rules how the driver should behave for example if
> the consumer requests
> 
> 	.duty_cycle = 10, .period = 50
> 
> and the hardware can only implement multiples of 3 for both values. The
> obvious candidates are:
> 
>  - .duty_cycle = 9, .period = 51 (round nearest for both)
>  - .duty_cycle = 12, .period = 51 (round up)
>  - .duty_cycle = 9, .period = 48 (round down)
>  - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
>  - return error (which code?)
> 
> And there are some other variants (e.g. round duty_cycle to nearest and
> period in the same direction) that might be sensible.

The problem is that probably all of the above are valid, though maybe
not for all cases. The choice of algorithm probably depends on both the
PWM driver and the consumer, so I don't think fixing things to one such
algorithm is going to improve anything.

> Also it should be possible to know the result before actually
> configuring the hardware. Otherwise things might already go wrong
> because the driver implements a setting that is too far from the
> requested configuration.

I agree.

Perhaps somebody with more experience with pwm-regulator can chime in
here, but it sounds to me like if you really want to accurately output a
voltage, you may want to hand-tune the duty-cycle/period pairs that are
used for pwm-regulator. According to the device tree bindings there's
already support for a voltage table mode where an exact duty-cycle to
output voltage correspondence is defined. This is as opposed to the
continuous voltage mode where the duty cycle is linearly interpolated
based on the requested output voltage.

pwm-regulator in voltage table mode could run in "strict" mode with zero
deviation allowed, on the assumption that duty-cycle values were hand-
picked to give the desired results. For continuous voltage mode it
probably doesn't matter all that much, since very exact results can't be
guaranteed anyway.

Thierry

Uwe Kleine-König April 29, 2019, 12:31 p.m. UTC | #9

On Mon, Apr 29, 2019 at 01:03:54PM +0200, Thierry Reding wrote:
> On Mon, Apr 08, 2019 at 04:39:14PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote:
> > > Hi,
> > > 
> > > On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
> > > >
> > > > Hi,
> > > >
> > > > [adding two chromeos people, because veyron and gru are quite
> > > > heavy users of the rockchip pwm for both backlight and regulators]
> > > >
> > > > Doug, Brian: patchwork patch is here:
> > > > https://patchwork.kernel.org/patch/10851001/
> > > >
> > > > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > > > > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > > > > state for the caller of pwm_apply_state(). This might have surprising
> > > > > results if the caller reuses the values expecting them to still
> > > > > represent the same state.
> > > 
> > > It may or may not be surprising, but it is well documented.  Specifically:
> > > 
> > >  * pwm_apply_state() - atomically apply a new state to a PWM device
> > >  * @pwm: PWM device
> > >  * @state: new state to apply. This can be adjusted by the PWM driver
> > >  *    if the requested config is not achievable, for example,
> > >  *    ->duty_cycle and ->period might be approximated.
> > > 
> > > I don't think your series updates that documentation, right?
> > > 
> > > 
> > > > > Also note that this feedback was incomplete as
> > > > > the matching struct pwm_device::state wasn't updated and so
> > > > > pwm_get_state still returned the originally requested state.
> > > 
> > > The framework handles that.  Take a look at pwm_apply_state()?  It does:
> > > 
> > > ---
> > > 
> > > err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > > if (err)
> > >     return err;
> > > 
> > > pwm->state = *state;
> > 
> > > 
> > > ---
> > > 
> > > So I think it wasn't incomplete unless I misunderstood?
> > 
> > You're right, I missed that the updated state was saved.
> > 
> > > > > Signed-off-by: Uwe Kleine-König <[e-mail]>
> > > >
> > > > I've tested this on both veyron and gru with backlight and pwm regulator
> > > > and at least both still come up, so
> > > > Tested-by: Heiko Stuebner <[e-mail]>
> > > >
> > > > But hopefully Doug or Brian could also provide another test-point.
> > > 
> > > I'd definitely be concerned by this change.  Specifically for the PWM
> > > regulator little details about exactly what duty cycle / period you
> > > got could be pretty important.
> > > 
> > > I guess the problem here is that pwm_get_state() doesn't actually call
> > > into the PWM drivers, it just returns the last state that was applied.
> > > How does one get the state?  I guess you could change get_state() to
> > > actually call into the PWM driver's get_state if it exists?  ...but
> > > your patch set doesn't change that behavior...
> > 
> > My intention here is more to make all drivers behave the same way and
> > because only two drivers updated the pwm_state this was the variant I
> > removed.
> > 
> > When you say that the caller might actually care about the exact
> > parameters I fully agree. In this case however the consumer should be
> > able to know the result before actually applying it. So if you do
> > 
> > 	pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > 
> > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > bad things you want to know about already happend. So my idea is a new
> > function pwm_round_state() that does the adaptions to pwm_state without
> > applying it to the hardware. After that pwm_apply_state could do the
> > following:
> > 
> > 	rstate = pwm_round_state(pwm, state)
> > 	pwm.apply(pwm, state)
> > 	gstate = pwm_get_state(pwm)
> > 
> > 	if rstate != gstate:
> > 		warn about problems
> 
> I'm not sure this is really useful. I would expect that in most cases
> where it is necessary to have an exact match between the requested state
> and the actual state is important, you may not even get to warning about
> problems because the system may shut down (e.g. the regulator might not
> be outputting enough power to keep the system stable).
> 
> I think it'd be far more useful to give consumers a way to request that
> the state be applied strictly. I'm not sure how realistic that is,
> though. Most PWMs have some sort of restrictions, and in most cases this
> might still be okay. Perhaps some sort of permissible relative deviation
> factor could be added to give more flexibility?

I think in practise this isn't going to work. Consider the case that
Brian cares about: "we do need to be as precise as possible with the
duty:period ratio". So if we want 1/5 duty we might request:

	.duty_cycle = 100, .period = 500

an are using pwm_set_state_exact(). Now consider this fails. What is the
next value you should try? It's hard to say without knowing why it
failed. If however you could do

	mystate.duty_cycle = 100
	mystate.period = 500
	pwm_round_state(pwm, &mystate);

and after that we have:

	mystate.duty_cycle = 99;
	mystate.period = 498;

(because pwm_round_state is supposed to round down[1] and the hardware can
implement multiples of 3 only). Then it is easier to determine the next
state to try.

Best regards
Uwe

[1] this isn't fixed yet, but I think it's sensible.

Uwe Kleine-König April 29, 2019, 1:11 p.m. UTC | #10

On Mon, Apr 29, 2019 at 01:18:55PM +0200, Thierry Reding wrote:
> On Mon, Apr 29, 2019 at 08:56:13AM +0200, Uwe Kleine-König wrote:
> > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> > > Hi,
> > > 
> > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > this expires out of my inbox:
> > > 
> > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > <[e-mail]> wrote:
> > > > My intention here is more to make all drivers behave the same way and
> > > > because only two drivers updated the pwm_state this was the variant I
> > > > removed.
> > > 
> > > To be clear, this patch on its own is probably breaking things. Just
> > > because the other drivers don't implement the documented behavior
> > > doesn't mean you should break this driver. Maybe the others just
> > > aren't used in precise enough scenarios where this matters.
> > > 
> > > > When you say that the caller might actually care about the exact
> > > > parameters I fully agree. In this case however the consumer should be
> > > > able to know the result before actually applying it. So if you do
> > > >
> > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > >
> > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > bad things you want to know about already happend. So my idea is a new
> > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > following:
> > > >
> > > >         rstate = pwm_round_state(pwm, state)
> > > >         pwm.apply(pwm, state)
> > > >         gstate = pwm_get_state(pwm)
> > > >
> > > >         if rstate != gstate:
> > > >                 warn about problems
> > > 
> > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > we need to be 100% precise about the period, but we do need to be as
> > > precise as possible with the duty:period ratio -- so once we get the
> > > "feedback" from the underlying PWM driver what the real period will
> > > be, we adjust the duty appropriately.
> > 
> > I admit that I didn't understood the whole situation and (some) things
> > are worse with my patches applied. I still think that changing the
> > caller's state variable is bad design, but of course pwm_get_state
> > should return the currently implemented configuration.
> > 
> > > So I don't see that "warning" would really help for this particular case.
> > > 
> > > > But before doing that I think it would be sensible to also fix the rules
> > > > how the round_state callback is supposed to round.
> > > 
> > > I'm not quite sure I grok exactly what you're planning, but I would
> > > much appreciate if you didn't break things on the way toward fixing
> > > them ;)
> > 
> > There are currently no rules how the driver should behave for example if
> > the consumer requests
> > 
> > 	.duty_cycle = 10, .period = 50
> > 
> > and the hardware can only implement multiples of 3 for both values. The
> > obvious candidates are:
> > 
> >  - .duty_cycle = 9, .period = 51 (round nearest for both)
> >  - .duty_cycle = 12, .period = 51 (round up)
> >  - .duty_cycle = 9, .period = 48 (round down)
> >  - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
> >  - return error (which code?)
> > 
> > And there are some other variants (e.g. round duty_cycle to nearest and
> > period in the same direction) that might be sensible.
> 
> The problem is that probably all of the above are valid, though maybe
> not for all cases. The choice of algorithm probably depends on both the
> PWM driver and the consumer, so I don't think fixing things to one such
> algorithm is going to improve anything.

But if you have pwm_round_state (which implements rounding down for
example) you could easily implement a helper that rounds nearest or up.
If however each driver rounds the way it prefers coming up with a helper
for rounding up is considerably harder.

> > Also it should be possible to know the result before actually
> > configuring the hardware. Otherwise things might already go wrong
> > because the driver implements a setting that is too far from the
> > requested configuration.
> 
> I agree.
> 
> Perhaps somebody with more experience with pwm-regulator can chime in
> here, but it sounds to me like if you really want to accurately output a
> voltage, you may want to hand-tune the duty-cycle/period pairs that are
> used for pwm-regulator.

This might be more ugly than needed because then you have to setup the
table in dependance of the used PWM. Looking at the pwm-regulator code I
think the binding is badly worded. The "Duty-Cycle" parameter is used as
second parameter to pwm_set_relative_duty_cycle (with scale = 100). So
with the regulator defined in the Voltage Table Example of
Documentation/devicetree/bindings/regulator/pwm-regulator.txt you'd have
to configure

	.duty_cycle = 2534, .period = 8448
	
to get 1.056 V.

Note that my considerations are not only about pwm-regulators.

Also in general I prefer a suitable and well reviewed algorithm (if
possible) over a requirement to provide a hand-tuned table of values in
a machine-specific device tree.

> According to the device tree bindings there's
> already support for a voltage table mode where an exact duty-cycle to
> output voltage correspondence is defined. This is as opposed to the
> continuous voltage mode where the duty cycle is linearly interpolated
> based on the requested output voltage.
> 
> pwm-regulator in voltage table mode could run in "strict" mode with zero
> deviation allowed, on the assumption that duty-cycle values were hand-
> picked to give the desired results. For continuous voltage mode it
> probably doesn't matter all that much, since very exact results can't be
> guaranteed anyway.

I don't understand the last sentence? Why is it impossible to get exact
results in continuous voltage mode?

Best regards
Uwe

Thierry Reding April 29, 2019, 4:17 p.m. UTC | #11

On Mon, Apr 29, 2019 at 02:31:02PM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 29, 2019 at 01:03:54PM +0200, Thierry Reding wrote:
> > On Mon, Apr 08, 2019 at 04:39:14PM +0200, Uwe Kleine-König wrote:
> > > On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote:
> > > > Hi,
> > > > 
> > > > On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
> > > > >
> > > > > Hi,
> > > > >
> > > > > [adding two chromeos people, because veyron and gru are quite
> > > > > heavy users of the rockchip pwm for both backlight and regulators]
> > > > >
> > > > > Doug, Brian: patchwork patch is here:
> > > > > https://patchwork.kernel.org/patch/10851001/
> > > > >
> > > > > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > > > > > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > > > > > state for the caller of pwm_apply_state(). This might have surprising
> > > > > > results if the caller reuses the values expecting them to still
> > > > > > represent the same state.
> > > > 
> > > > It may or may not be surprising, but it is well documented.  Specifically:
> > > > 
> > > >  * pwm_apply_state() - atomically apply a new state to a PWM device
> > > >  * @pwm: PWM device
> > > >  * @state: new state to apply. This can be adjusted by the PWM driver
> > > >  *    if the requested config is not achievable, for example,
> > > >  *    ->duty_cycle and ->period might be approximated.
> > > > 
> > > > I don't think your series updates that documentation, right?
> > > > 
> > > > 
> > > > > > Also note that this feedback was incomplete as
> > > > > > the matching struct pwm_device::state wasn't updated and so
> > > > > > pwm_get_state still returned the originally requested state.
> > > > 
> > > > The framework handles that.  Take a look at pwm_apply_state()?  It does:
> > > > 
> > > > ---
> > > > 
> > > > err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > > > if (err)
> > > >     return err;
> > > > 
> > > > pwm->state = *state;
> > > 
> > > > 
> > > > ---
> > > > 
> > > > So I think it wasn't incomplete unless I misunderstood?
> > > 
> > > You're right, I missed that the updated state was saved.
> > > 
> > > > > > Signed-off-by: Uwe Kleine-König <[e-mail]>
> > > > >
> > > > > I've tested this on both veyron and gru with backlight and pwm regulator
> > > > > and at least both still come up, so
> > > > > Tested-by: Heiko Stuebner <[e-mail]>
> > > > >
> > > > > But hopefully Doug or Brian could also provide another test-point.
> > > > 
> > > > I'd definitely be concerned by this change.  Specifically for the PWM
> > > > regulator little details about exactly what duty cycle / period you
> > > > got could be pretty important.
> > > > 
> > > > I guess the problem here is that pwm_get_state() doesn't actually call
> > > > into the PWM drivers, it just returns the last state that was applied.
> > > > How does one get the state?  I guess you could change get_state() to
> > > > actually call into the PWM driver's get_state if it exists?  ...but
> > > > your patch set doesn't change that behavior...
> > > 
> > > My intention here is more to make all drivers behave the same way and
> > > because only two drivers updated the pwm_state this was the variant I
> > > removed.
> > > 
> > > When you say that the caller might actually care about the exact
> > > parameters I fully agree. In this case however the consumer should be
> > > able to know the result before actually applying it. So if you do
> > > 
> > > 	pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > 
> > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > bad things you want to know about already happend. So my idea is a new
> > > function pwm_round_state() that does the adaptions to pwm_state without
> > > applying it to the hardware. After that pwm_apply_state could do the
> > > following:
> > > 
> > > 	rstate = pwm_round_state(pwm, state)
> > > 	pwm.apply(pwm, state)
> > > 	gstate = pwm_get_state(pwm)
> > > 
> > > 	if rstate != gstate:
> > > 		warn about problems
> > 
> > I'm not sure this is really useful. I would expect that in most cases
> > where it is necessary to have an exact match between the requested state
> > and the actual state is important, you may not even get to warning about
> > problems because the system may shut down (e.g. the regulator might not
> > be outputting enough power to keep the system stable).
> > 
> > I think it'd be far more useful to give consumers a way to request that
> > the state be applied strictly. I'm not sure how realistic that is,
> > though. Most PWMs have some sort of restrictions, and in most cases this
> > might still be okay. Perhaps some sort of permissible relative deviation
> > factor could be added to give more flexibility?
> 
> I think in practise this isn't going to work. Consider the case that
> Brian cares about: "we do need to be as precise as possible with the
> duty:period ratio". So if we want 1/5 duty we might request:
> 
> 	.duty_cycle = 100, .period = 500
> 
> an are using pwm_set_state_exact(). Now consider this fails. What is the
> next value you should try?

The idea is that if the driver fails to set the exact state if that's
what was requested, then we just fail. If we really need an exact set
of values, then it doesn't make sense to offer the user alternatives
using rounding helpers.

On the other hand, if we introduce an error margin, we could make the
above work. Let's say the PWM regulator requires accuracy within 1%, we
could do something like this:

	state.duty_cycle = 100;
	state.period = 500;
	state.accuracy = 1; /* there's a slew of ways to encode this */
	pwm_apply_state(pwm, &state);

That way, the PWM driver can determine whether or not the ratio of
possible duty-cycle/period is accurate within that 1% requested by the
user:

	ratio = duty-cycle / period

	requested = 100 / 500
	possible = 99 / 498

	possible / requested ~= 0.993

In other words, possible is > 99% of requested and therefore within the
1% accuracy that pwm-regulator requested. The PWM driver can therefore
go ahead and program the selected set of values.

> It's hard to say without knowing why it failed. If however you could do
> 
> 	mystate.duty_cycle = 100
> 	mystate.period = 500
> 	pwm_round_state(pwm, &mystate);
> 
> and after that we have:
> 
> 	mystate.duty_cycle = 99;
> 	mystate.period = 498;
> 
> (because pwm_round_state is supposed to round down[1] and the hardware can
> implement multiples of 3 only). Then it is easier to determine the next
> state to try.

No, it's really not any easier to determine the next state to try. The
problem is that the consumer doesn't know anything about the
restrictions of the driver, and it shouldn't need to know. Given the
above, how is it supposed to know that the restriction is "multiple of
3". Just because the two values happen to be multiples of 3 doesn't mean
that's the restriction. Also, we really don't want every consumer in the
kernel to implement factorization (and whatnot) to guess what possible
constraints there could be.

Thierry

> 
> Best regards
> Uwe
> 
> [1] this isn't fixed yet, but I think it's sensible.
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Thierry Reding April 29, 2019, 4:28 p.m. UTC | #12

On Mon, Apr 29, 2019 at 03:11:27PM +0200, Uwe Kleine-König wrote:
> On Mon, Apr 29, 2019 at 01:18:55PM +0200, Thierry Reding wrote:
> > On Mon, Apr 29, 2019 at 08:56:13AM +0200, Uwe Kleine-König wrote:
> > > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> > > > Hi,
> > > > 
> > > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > > this expires out of my inbox:
> > > > 
> > > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > > <[e-mail]> wrote:
> > > > > My intention here is more to make all drivers behave the same way and
> > > > > because only two drivers updated the pwm_state this was the variant I
> > > > > removed.
> > > > 
> > > > To be clear, this patch on its own is probably breaking things. Just
> > > > because the other drivers don't implement the documented behavior
> > > > doesn't mean you should break this driver. Maybe the others just
> > > > aren't used in precise enough scenarios where this matters.
> > > > 
> > > > > When you say that the caller might actually care about the exact
> > > > > parameters I fully agree. In this case however the consumer should be
> > > > > able to know the result before actually applying it. So if you do
> > > > >
> > > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > >
> > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > > bad things you want to know about already happend. So my idea is a new
> > > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > > following:
> > > > >
> > > > >         rstate = pwm_round_state(pwm, state)
> > > > >         pwm.apply(pwm, state)
> > > > >         gstate = pwm_get_state(pwm)
> > > > >
> > > > >         if rstate != gstate:
> > > > >                 warn about problems
> > > > 
> > > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > > we need to be 100% precise about the period, but we do need to be as
> > > > precise as possible with the duty:period ratio -- so once we get the
> > > > "feedback" from the underlying PWM driver what the real period will
> > > > be, we adjust the duty appropriately.
> > > 
> > > I admit that I didn't understood the whole situation and (some) things
> > > are worse with my patches applied. I still think that changing the
> > > caller's state variable is bad design, but of course pwm_get_state
> > > should return the currently implemented configuration.
> > > 
> > > > So I don't see that "warning" would really help for this particular case.
> > > > 
> > > > > But before doing that I think it would be sensible to also fix the rules
> > > > > how the round_state callback is supposed to round.
> > > > 
> > > > I'm not quite sure I grok exactly what you're planning, but I would
> > > > much appreciate if you didn't break things on the way toward fixing
> > > > them ;)
> > > 
> > > There are currently no rules how the driver should behave for example if
> > > the consumer requests
> > > 
> > > 	.duty_cycle = 10, .period = 50
> > > 
> > > and the hardware can only implement multiples of 3 for both values. The
> > > obvious candidates are:
> > > 
> > >  - .duty_cycle = 9, .period = 51 (round nearest for both)
> > >  - .duty_cycle = 12, .period = 51 (round up)
> > >  - .duty_cycle = 9, .period = 48 (round down)
> > >  - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
> > >  - return error (which code?)
> > > 
> > > And there are some other variants (e.g. round duty_cycle to nearest and
> > > period in the same direction) that might be sensible.
> > 
> > The problem is that probably all of the above are valid, though maybe
> > not for all cases. The choice of algorithm probably depends on both the
> > PWM driver and the consumer, so I don't think fixing things to one such
> > algorithm is going to improve anything.
> 
> But if you have pwm_round_state (which implements rounding down for
> example) you could easily implement a helper that rounds nearest or up.
> If however each driver rounds the way it prefers coming up with a helper
> for rounding up is considerably harder.

pwm_round_state() is fundamentally racy. What if, for example, you have
two consumers racing to set two PWMs provided by the same controller. If
you have some dependency between the two PWMs (perhaps they need to
share the same divider or something like that), then between the time
where pwm_round_state() returns and pwm_apply_state() is called, the
results of the pwm_round_state() may no longer be valid.

> > > Also it should be possible to know the result before actually
> > > configuring the hardware. Otherwise things might already go wrong
> > > because the driver implements a setting that is too far from the
> > > requested configuration.
> > 
> > I agree.
> > 
> > Perhaps somebody with more experience with pwm-regulator can chime in
> > here, but it sounds to me like if you really want to accurately output a
> > voltage, you may want to hand-tune the duty-cycle/period pairs that are
> > used for pwm-regulator.
> 
> This might be more ugly than needed because then you have to setup the
> table in dependance of the used PWM.

Well, that's what you have to do anyway. I mean, you can't write one
voltage table that works for one device and then expect it to work for
every other device. DT by definition is a board-level definition.

> Looking at the pwm-regulator code I
> think the binding is badly worded. The "Duty-Cycle" parameter is used as
> second parameter to pwm_set_relative_duty_cycle (with scale = 100). So
> with the regulator defined in the Voltage Table Example of
> Documentation/devicetree/bindings/regulator/pwm-regulator.txt you'd have
> to configure
> 
> 	.duty_cycle = 2534, .period = 8448
> 	
> to get 1.056 V.

Hm... indeed. Requiring the duty-cycle to be in percent is not a good
idea. That's going to lead to rounding one way or another.

> 
> Note that my considerations are not only about pwm-regulators.
> 
> Also in general I prefer a suitable and well reviewed algorithm (if
> possible) over a requirement to provide a hand-tuned table of values in
> a machine-specific device tree.

I agree that an algorithm is usually better, but if you can't create an
algorithm that works, it's usually better to have a hand-coded fallback
rather than have no working system at all.

> > According to the device tree bindings there's
> > already support for a voltage table mode where an exact duty-cycle to
> > output voltage correspondence is defined. This is as opposed to the
> > continuous voltage mode where the duty cycle is linearly interpolated
> > based on the requested output voltage.
> > 
> > pwm-regulator in voltage table mode could run in "strict" mode with zero
> > deviation allowed, on the assumption that duty-cycle values were hand-
> > picked to give the desired results. For continuous voltage mode it
> > probably doesn't matter all that much, since very exact results can't be
> > guaranteed anyway.
> 
> I don't understand the last sentence? Why is it impossible to get exact
> results in continuous voltage mode?

I didn't say it was impossible. I said it can't be guaranteed. There may
very well be combinations of drivers and consumers where the results
will be accurate, but there may very well be other combinations where
the results won't be. So if you don't know the exact combination, you
can't be sure that the result will be accurate.

Now it seems like both voltage table and continuous modes seem to rely
on some relative duty cycle setting, in which case there's typically
going to be rounding in both cases (unless you make the duty cycle range
large enough to accomodate the whole range from 0 to period.

Thierry

Doug Anderson April 29, 2019, 6:04 p.m. UTC | #13

Hi,

On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
<[e-mail]> wrote:
>
> On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> > Hi,
> >
> > I'm not sure if I'm misreading you, but I thought I'd add here before
> > this expires out of my inbox:
> >
> > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > <[e-mail]> wrote:
> > > My intention here is more to make all drivers behave the same way and
> > > because only two drivers updated the pwm_state this was the variant I
> > > removed.
> >
> > To be clear, this patch on its own is probably breaking things. Just
> > because the other drivers don't implement the documented behavior
> > doesn't mean you should break this driver. Maybe the others just
> > aren't used in precise enough scenarios where this matters.
> >
> > > When you say that the caller might actually care about the exact
> > > parameters I fully agree. In this case however the consumer should be
> > > able to know the result before actually applying it. So if you do
> > >
> > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > >
> > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > bad things you want to know about already happend. So my idea is a new
> > > function pwm_round_state() that does the adaptions to pwm_state without
> > > applying it to the hardware. After that pwm_apply_state could do the
> > > following:
> > >
> > >         rstate = pwm_round_state(pwm, state)
> > >         pwm.apply(pwm, state)
> > >         gstate = pwm_get_state(pwm)
> > >
> > >         if rstate != gstate:
> > >                 warn about problems
> >
> > For our case (we're using this with pwm-regulator), I don't recall [*]
> > we need to be 100% precise about the period, but we do need to be as
> > precise as possible with the duty:period ratio -- so once we get the
> > "feedback" from the underlying PWM driver what the real period will
> > be, we adjust the duty appropriately.
>
> I admit that I didn't understood the whole situation and (some) things
> are worse with my patches applied. I still think that changing the
> caller's state variable is bad design, but of course pwm_get_state
> should return the currently implemented configuration.

Regardless of the pros and cons of the current situation, hopefully
we're in agreement that we shouldn't break existing users?  In general
I'll probably stay out of the debate as long as we end somewhere that
pwm_regulator is able to somehow know the actual state that it
programmed into the hardware.

+Boris too in case he has any comments.


> > So I don't see that "warning" would really help for this particular case.
> >
> > > But before doing that I think it would be sensible to also fix the rules
> > > how the round_state callback is supposed to round.
> >
> > I'm not quite sure I grok exactly what you're planning, but I would
> > much appreciate if you didn't break things on the way toward fixing
> > them ;)
>
> There are currently no rules how the driver should behave for example if
> the consumer requests
>
>         .duty_cycle = 10, .period = 50
>
> and the hardware can only implement multiples of 3 for both values. The
> obvious candidates are:
>
>  - .duty_cycle = 9, .period = 51 (round nearest for both)
>  - .duty_cycle = 12, .period = 51 (round up)
>  - .duty_cycle = 9, .period = 48 (round down)
>  - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
>  - return error (which code?)
>
> And there are some other variants (e.g. round duty_cycle to nearest and
> period in the same direction) that might be sensible.

I will note that I had to deal with some of this recently when I
wanted to try to replicate the exact voltage levels for "vdd_log" from
downstream in rk3288-veyron devices.  See commit 864c2fee4ee9 ("ARM:
dts: rockchip: Add vdd_logic to rk3288-veyron") in Heiko's tree (or
just look in linux-next)


> Also it should be possible to know the result before actually
> configuring the hardware. Otherwise things might already go wrong
> because the driver implements a setting that is too far from the
> requested configuration.

Later in this thread Thierry didn't like the "round rate" idea due to
races.  One way to solve that could be to indicate to the PWM
framework which direction you'd like it to error in: a higher duty
cycle or a lower one.


-Doug

Uwe Kleine-König April 29, 2019, 9:08 p.m. UTC | #14

Hello Thierry,

On Mon, Apr 29, 2019 at 06:17:49PM +0200, Thierry Reding wrote:
> On Mon, Apr 29, 2019 at 02:31:02PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 29, 2019 at 01:03:54PM +0200, Thierry Reding wrote:
> > > On Mon, Apr 08, 2019 at 04:39:14PM +0200, Uwe Kleine-König wrote:
> > > > On Mon, Apr 01, 2019 at 03:45:47PM -0700, Doug Anderson wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <[e-mail]> wrote:
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > [adding two chromeos people, because veyron and gru are quite
> > > > > > heavy users of the rockchip pwm for both backlight and regulators]
> > > > > >
> > > > > > Doug, Brian: patchwork patch is here:
> > > > > > https://patchwork.kernel.org/patch/10851001/
> > > > > >
> > > > > > Am Dienstag, 12. März 2019, 22:46:03 CET schrieb Uwe Kleine-König:
> > > > > > > The pwm-rockchip driver is one of only two PWM drivers which updates the
> > > > > > > state for the caller of pwm_apply_state(). This might have surprising
> > > > > > > results if the caller reuses the values expecting them to still
> > > > > > > represent the same state.
> > > > > 
> > > > > It may or may not be surprising, but it is well documented.  Specifically:
> > > > > 
> > > > >  * pwm_apply_state() - atomically apply a new state to a PWM device
> > > > >  * @pwm: PWM device
> > > > >  * @state: new state to apply. This can be adjusted by the PWM driver
> > > > >  *    if the requested config is not achievable, for example,
> > > > >  *    ->duty_cycle and ->period might be approximated.
> > > > > 
> > > > > I don't think your series updates that documentation, right?
> > > > > 
> > > > > 
> > > > > > > Also note that this feedback was incomplete as
> > > > > > > the matching struct pwm_device::state wasn't updated and so
> > > > > > > pwm_get_state still returned the originally requested state.
> > > > > 
> > > > > The framework handles that.  Take a look at pwm_apply_state()?  It does:
> > > > > 
> > > > > ---
> > > > > 
> > > > > err = pwm->chip->ops->apply(pwm->chip, pwm, state);
> > > > > if (err)
> > > > >     return err;
> > > > > 
> > > > > pwm->state = *state;
> > > > 
> > > > > 
> > > > > ---
> > > > > 
> > > > > So I think it wasn't incomplete unless I misunderstood?
> > > > 
> > > > You're right, I missed that the updated state was saved.
> > > > 
> > > > > > > Signed-off-by: Uwe Kleine-König <[e-mail]>
> > > > > >
> > > > > > I've tested this on both veyron and gru with backlight and pwm regulator
> > > > > > and at least both still come up, so
> > > > > > Tested-by: Heiko Stuebner <[e-mail]>
> > > > > >
> > > > > > But hopefully Doug or Brian could also provide another test-point.
> > > > > 
> > > > > I'd definitely be concerned by this change.  Specifically for the PWM
> > > > > regulator little details about exactly what duty cycle / period you
> > > > > got could be pretty important.
> > > > > 
> > > > > I guess the problem here is that pwm_get_state() doesn't actually call
> > > > > into the PWM drivers, it just returns the last state that was applied.
> > > > > How does one get the state?  I guess you could change get_state() to
> > > > > actually call into the PWM driver's get_state if it exists?  ...but
> > > > > your patch set doesn't change that behavior...
> > > > 
> > > > My intention here is more to make all drivers behave the same way and
> > > > because only two drivers updated the pwm_state this was the variant I
> > > > removed.
> > > > 
> > > > When you say that the caller might actually care about the exact
> > > > parameters I fully agree. In this case however the consumer should be
> > > > able to know the result before actually applying it. So if you do
> > > > 
> > > > 	pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > 
> > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > bad things you want to know about already happend. So my idea is a new
> > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > following:
> > > > 
> > > > 	rstate = pwm_round_state(pwm, state)
> > > > 	pwm.apply(pwm, state)
> > > > 	gstate = pwm_get_state(pwm)
> > > > 
> > > > 	if rstate != gstate:
> > > > 		warn about problems
> > > 
> > > I'm not sure this is really useful. I would expect that in most cases
> > > where it is necessary to have an exact match between the requested state
> > > and the actual state is important, you may not even get to warning about
> > > problems because the system may shut down (e.g. the regulator might not
> > > be outputting enough power to keep the system stable).
> > > 
> > > I think it'd be far more useful to give consumers a way to request that
> > > the state be applied strictly. I'm not sure how realistic that is,
> > > though. Most PWMs have some sort of restrictions, and in most cases this
> > > might still be okay. Perhaps some sort of permissible relative deviation
> > > factor could be added to give more flexibility?
> > 
> > I think in practise this isn't going to work. Consider the case that
> > Brian cares about: "we do need to be as precise as possible with the
> > duty:period ratio". So if we want 1/5 duty we might request:
> > 
> > 	.duty_cycle = 100, .period = 500
> > 
> > an are using pwm_set_state_exact(). Now consider this fails. What is the
> > next value you should try?
> 
> The idea is that if the driver fails to set the exact state if that's
> what was requested, then we just fail. If we really need an exact set
> of values, then it doesn't make sense to offer the user alternatives
> using rounding helpers.

I think in practise for most cases you don't need a completely exact
duty_cycle/period setting. Some inaccuracy is fine, but how much and in
which directions might differ. Also the consumer might not care if it
gets 30% duty cycle with an inversed polarity or 70% with a normal one.
Some consumer might care about duty-cycle/period ratio, another about
the absolute value of period. Then yes, if you really need

	.duty_cycle = 100, .period = 500

it doesn't help you that the driver suggests 99/498 instead. But it also
doesn't hurt.

> On the other hand, if we introduce an error margin, we could make the
> above work. Let's say the PWM regulator requires accuracy within 1%, we
> could do something like this:
> 
> 	state.duty_cycle = 100;
> 	state.period = 500;
> 	state.accuracy = 1; /* there's a slew of ways to encode this */
> 	pwm_apply_state(pwm, &state);
> 
> That way, the PWM driver can determine whether or not the ratio of
> possible duty-cycle/period is accurate within that 1% requested by the
> user:
> 
> 	ratio = duty-cycle / period
> 
> 	requested = 100 / 500
> 	possible = 99 / 498
> 
> 	possible / requested ~= 0.993
> 
> In other words, possible is > 99% of requested and therefore within the
> 1% accuracy that pwm-regulator requested. The PWM driver can therefore
> go ahead and program the selected set of values.

This is less powerful than my suggestion. With .accuracy implemented in
the lowlevel drivers, how would you request 20 % duty cycle with period
<= 1 ms? With my round_rate suggestion it would be:

	mystate.duty_cycle = 200000;
	mystate.period = 1000000;

   retry:
	pwm_round_rate(pwm, &mystate);

	if mystate.period * 2 == mystate.duty_cycle * 10:
		pwm_apply_state(pwm, &mystate)
		return 0
	elif mystate.period == 0:
		return -ESOMETHING
	elif mystate.period * 2 < mystate.duty_cycle * 10:
		mystate.duty_cycle -= 1
		goto retry
	elif mystate.duty_cycle == 0:
		return -ESOMETHING
	else
		mystate.period -= 1
		goto retry

(if some derivation is ok, this can be implemented in the respective
checks)

Even if a deviation of 1 % is ok for me, with your proposal I would end
with 99 / 498, while I could have 99 / 495 which better matches my needs.
(And if I start with mystate.duty_cycle = 100 and mystate.period = 500
above, I will hit that value-pair with the second call to
pwm_round_rate().)

With your suggestion requesting

	.duty_cycle = 100, .period = 500, .accuracy = 1

the driver is free to set

	.duty_cycle = 100000000, .period = 500000000

as possible / requested is 1. This for sure isn't ok for all consumers.

Also with the above request I might prefer

	.duty_cycle = 98, .period = 500

over

	.duty_cycle = 101, .period = 500

even thought the latter is better according to your metric.

I admit this is not free of complexity to use. But this is because the
problem is not trivial. And it is powerful enough that a helper function
that gets a state and a needed accuracy as you suggested above, can be
implemented.

> > It's hard to say without knowing why it failed. If however you could do
> > 
> > 	mystate.duty_cycle = 100
> > 	mystate.period = 500
> > 	pwm_round_state(pwm, &mystate);
> > 
> > and after that we have:
> > 
> > 	mystate.duty_cycle = 99;
> > 	mystate.period = 498;
> > 
> > (because pwm_round_state is supposed to round down[1] and the hardware can
> > implement multiples of 3 only). Then it is easier to determine the next
> > state to try.
> 
> No, it's really not any easier to determine the next state to try.

See above suggestion. When there is only just pwm_set_state_exact() the
only way to implement "20 % duty cycle with period <= 1 ms" by iterating
over the whole set of allowed values until you find a hit. (Also if
.accuracy is used, you have to go with the first hit unless you iterate
over the good set of parameters several times with giving the driver
more freedom to derivate in each loop.)

> The problem is that the consumer doesn't know anything about the
> restrictions of the driver, and it shouldn't need to know. Given the
> above, how is it supposed to know that the restriction is "multiple of
> 3".

It doesn't. Still there are algorithms that allow to determine the best
set of duty_cycle and period (whatever "best" currently means to you) in
a more effective way than by a testing each good state.

> Just because the two values happen to be multiples of 3 doesn't mean
> that's the restriction. Also, we really don't want every consumer in the
> kernel to implement factorization (and whatnot) to guess what possible
> constraints there could be.

Full ack. A consumer trying to guess what the exact restriction is, is
bound to fail in at least some cases. But as this is not necessary (as
shown above) this argument is irrelevant.

Best regards
Uwe

Uwe Kleine-König April 30, 2019, 9:14 a.m. UTC | #15

Hello,

On Mon, Apr 29, 2019 at 06:28:47PM +0200, Thierry Reding wrote:
> On Mon, Apr 29, 2019 at 03:11:27PM +0200, Uwe Kleine-König wrote:
> > On Mon, Apr 29, 2019 at 01:18:55PM +0200, Thierry Reding wrote:
> > > On Mon, Apr 29, 2019 at 08:56:13AM +0200, Uwe Kleine-König wrote:
> > > > There are currently no rules how the driver should behave for example if
> > > > the consumer requests
> > > > 
> > > > 	.duty_cycle = 10, .period = 50
> > > > 
> > > > and the hardware can only implement multiples of 3 for both values. The
> > > > obvious candidates are:
> > > > 
> > > >  - .duty_cycle = 9, .period = 51 (round nearest for both)
> > > >  - .duty_cycle = 12, .period = 51 (round up)
> > > >  - .duty_cycle = 9, .period = 48 (round down)
> > > >  - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
> > > >  - return error (which code?)
> > > > 
> > > > And there are some other variants (e.g. round duty_cycle to nearest and
> > > > period in the same direction) that might be sensible.
> > > 
> > > The problem is that probably all of the above are valid, though maybe
> > > not for all cases. The choice of algorithm probably depends on both the
> > > PWM driver and the consumer, so I don't think fixing things to one such
> > > algorithm is going to improve anything.
> > 
> > But if you have pwm_round_state (which implements rounding down for
> > example) you could easily implement a helper that rounds nearest or up.
> > If however each driver rounds the way it prefers coming up with a helper
> > for rounding up is considerably harder.
> 
> pwm_round_state() is fundamentally racy. What if, for example, you have
> two consumers racing to set two PWMs provided by the same controller. If
> you have some dependency between the two PWMs (perhaps they need to
> share the same divider or something like that), then between the time
> where pwm_round_state() returns and pwm_apply_state() is called, the
> results of the pwm_round_state() may no longer be valid.

Yes, that's somewhat right. That is a problem that all approaches have
that allow to determine the result of a given request without actually
applying it. (Same for clk_round_rate() for example.) I think that it
won't be possible to map all use cases without such an approach however.

But the effect of the race can be minimised if I calculate a good
PWM state (which might not be optimal because of the race) and then use
pwm_apply_state_exact().

Having said that, we already have a very similar problem today. Each
driver that does clk_get_rate without installing a notifier for
frequency changes might suffer from unexpected results if another clk
consumer changes the given clk.

> > > > Also it should be possible to know the result before actually
> > > > configuring the hardware. Otherwise things might already go wrong
> > > > because the driver implements a setting that is too far from the
> > > > requested configuration.
> > > 
> > > I agree.
> > > 
> > > Perhaps somebody with more experience with pwm-regulator can chime in
> > > here, but it sounds to me like if you really want to accurately output a
> > > voltage, you may want to hand-tune the duty-cycle/period pairs that are
> > > used for pwm-regulator.
> > 
> > This might be more ugly than needed because then you have to setup the
> > table in dependance of the used PWM.
> 
> Well, that's what you have to do anyway. I mean, you can't write one
> voltage table that works for one device and then expect it to work for
> every other device. DT by definition is a board-level definition.

We're talking about different things here. I want that if you assembled
the same type of pwm-regulator to an imx machine and a rockchip machine
the representation of this regulator in the respective device trees
would look identical (apart from the handle to the PWM). Of course this
doesn't work for two different types of regulators.

The datasheet of the regulator for sure is also agnostic to the actual
PWM driving the input and provides a generic table or formula.

> > Looking at the pwm-regulator code I
> > think the binding is badly worded. The "Duty-Cycle" parameter is used as
> > second parameter to pwm_set_relative_duty_cycle (with scale = 100). So
> > with the regulator defined in the Voltage Table Example of
> > Documentation/devicetree/bindings/regulator/pwm-regulator.txt you'd have
> > to configure
> > 
> > 	.duty_cycle = 2534, .period = 8448
> > 	
> > to get 1.056 V.
> 
> Hm... indeed. Requiring the duty-cycle to be in percent is not a good
> idea. That's going to lead to rounding one way or another.
> 
> > 
> > Note that my considerations are not only about pwm-regulators.
> > 
> > Also in general I prefer a suitable and well reviewed algorithm (if
> > possible) over a requirement to provide a hand-tuned table of values in
> > a machine-specific device tree.
> 
> I agree that an algorithm is usually better, but if you can't create an
> algorithm that works, it's usually better to have a hand-coded fallback
> rather than have no working system at all.

We're in agreement here. And as there are cases where a suitable
algorithm exists, we need some support in the PWM API to actually
implement these. pwm_round_state is the best I'm aware of as it is
powerful enough to map all requirements I currently think necessary and
still it is not too hard to implement the needed callback for the device
drivers.

> > > According to the device tree bindings there's
> > > already support for a voltage table mode where an exact duty-cycle to
> > > output voltage correspondence is defined. This is as opposed to the
> > > continuous voltage mode where the duty cycle is linearly interpolated
> > > based on the requested output voltage.
> > > 
> > > pwm-regulator in voltage table mode could run in "strict" mode with zero
> > > deviation allowed, on the assumption that duty-cycle values were hand-
> > > picked to give the desired results. For continuous voltage mode it
> > > probably doesn't matter all that much, since very exact results can't be
> > > guaranteed anyway.
> > 
> > I don't understand the last sentence? Why is it impossible to get exact
> > results in continuous voltage mode?
> 
> I didn't say it was impossible. I said it can't be guaranteed. There may
> very well be combinations of drivers and consumers where the results
> will be accurate, but there may very well be other combinations where
> the results won't be. So if you don't know the exact combination, you
> can't be sure that the result will be accurate.

Either I don't understand what you intend to say, or I fail to see its
relevance here. If the device tree specifies a PWM regulator with
continuous voltage mode the right thing to do with a request for a
certain voltage is to exactly calculate the needed PWM setting with the
parameters provided and rely on the PWM driver to implement what it
promises. If the PWM driver is buggy or the device tree representation
is inaccurate, well there is nothing the PWM framework or the
pwm-regulator driver can do about it.

Best regards
Uwe

Uwe Kleine-König April 30, 2019, 9:28 a.m. UTC | #16

On Mon, Apr 29, 2019 at 11:04:20AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> <[e-mail]> wrote:
> >
> > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:
> > > Hi,
> > >
> > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > this expires out of my inbox:
> > >
> > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > <[e-mail]> wrote:
> > > > My intention here is more to make all drivers behave the same way and
> > > > because only two drivers updated the pwm_state this was the variant I
> > > > removed.
> > >
> > > To be clear, this patch on its own is probably breaking things. Just
> > > because the other drivers don't implement the documented behavior
> > > doesn't mean you should break this driver. Maybe the others just
> > > aren't used in precise enough scenarios where this matters.
> > >
> > > > When you say that the caller might actually care about the exact
> > > > parameters I fully agree. In this case however the consumer should be
> > > > able to know the result before actually applying it. So if you do
> > > >
> > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > >
> > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > bad things you want to know about already happend. So my idea is a new
> > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > following:
> > > >
> > > >         rstate = pwm_round_state(pwm, state)
> > > >         pwm.apply(pwm, state)
> > > >         gstate = pwm_get_state(pwm)
> > > >
> > > >         if rstate != gstate:
> > > >                 warn about problems
> > >
> > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > we need to be 100% precise about the period, but we do need to be as
> > > precise as possible with the duty:period ratio -- so once we get the
> > > "feedback" from the underlying PWM driver what the real period will
> > > be, we adjust the duty appropriately.
> >
> > I admit that I didn't understood the whole situation and (some) things
> > are worse with my patches applied. I still think that changing the
> > caller's state variable is bad design, but of course pwm_get_state
> > should return the currently implemented configuration.
> 
> Regardless of the pros and cons of the current situation, hopefully
> we're in agreement that we shouldn't break existing users?

Sure. And I'm happy you found the issue in my patch before it was
applied.

> In general I'll probably stay out of the debate as long as we end
> somewhere that pwm_regulator is able to somehow know the actual state
> that it programmed into the hardware.
> 
> +Boris too in case he has any comments.
> 
> 
> > > So I don't see that "warning" would really help for this particular case.
> > >
> > > > But before doing that I think it would be sensible to also fix the rules
> > > > how the round_state callback is supposed to round.
> > >
> > > I'm not quite sure I grok exactly what you're planning, but I would
> > > much appreciate if you didn't break things on the way toward fixing
> > > them ;)
> >
> > There are currently no rules how the driver should behave for example if
> > the consumer requests
> >
> >         .duty_cycle = 10, .period = 50
> >
> > and the hardware can only implement multiples of 3 for both values. The
> > obvious candidates are:
> >
> >  - .duty_cycle = 9, .period = 51 (round nearest for both)
> >  - .duty_cycle = 12, .period = 51 (round up)
> >  - .duty_cycle = 9, .period = 48 (round down)
> >  - .duty_cycle = 9, .period = 45 (round duty_cycle and keep proportion)
> >  - return error (which code?)
> >
> > And there are some other variants (e.g. round duty_cycle to nearest and
> > period in the same direction) that might be sensible.
> 
> I will note that I had to deal with some of this recently when I
> wanted to try to replicate the exact voltage levels for "vdd_log" from
> downstream in rk3288-veyron devices.  See commit 864c2fee4ee9 ("ARM:
> dts: rockchip: Add vdd_logic to rk3288-veyron") in Heiko's tree (or
> just look in linux-next)

Actually I think that it would make sense to expand the "pwm-regulator
with table" code to interpolate voltage levels between two table entries
assuming linear behaviour on the interval. This way a continous
regulator becomes just a special case with two entries. (Some care might
have to be applied to prevent changes in behaviour for already existing
machines.)
 
> > Also it should be possible to know the result before actually
> > configuring the hardware. Otherwise things might already go wrong
> > because the driver implements a setting that is too far from the
> > requested configuration.
> 
> Later in this thread Thierry didn't like the "round rate" idea due to
> races.  One way to solve that could be to indicate to the PWM
> framework which direction you'd like it to error in: a higher duty
> cycle or a lower one.

I don't think this would result in settings as optimal as with my
suggestion. If you don't agree and want to convince me: Show how your
suggestion would work with a PWM that can implement only multiples of 3
for duty_cycle and period and you want 20% duty cycle with period <= 1
ms (without making use of the knowledge about the limitation of the
PWM in the algorithm).

Best regards
Uwe

Doug Anderson April 30, 2019, 2:38 p.m. UTC | #17

Hi,

On Tue, Apr 30, 2019 at 2:28 AM Uwe Kleine-König
<[e-mail]> wrote:
>
> > > Also it should be possible to know the result before actually
> > > configuring the hardware. Otherwise things might already go wrong
> > > because the driver implements a setting that is too far from the
> > > requested configuration.
> >
> > Later in this thread Thierry didn't like the "round rate" idea due to
> > races.  One way to solve that could be to indicate to the PWM
> > framework which direction you'd like it to error in: a higher duty
> > cycle or a lower one.
>
> I don't think this would result in settings as optimal as with my
> suggestion. If you don't agree and want to convince me: Show how your
> suggestion would work with a PWM that can implement only multiples of 3
> for duty_cycle and period and you want 20% duty cycle with period <= 1
> ms (without making use of the knowledge about the limitation of the
> PWM in the algorithm).

I guess I was assuming that somehow this would percolate down into an
API call that the PWM driver would implement, so you could make use of
the PWM knowledge in the algorithm.

...but I don't have any real strong feelings about this API so I'll
leave it to you and Thierry to hash out what makes you both happy.


-Doug

Uwe Kleine-König May 2, 2019, 6:48 a.m. UTC | #18

Hello Doug,

On Tue, Apr 30, 2019 at 07:38:09AM -0700, Doug Anderson wrote:
> On Tue, Apr 30, 2019 at 2:28 AM Uwe Kleine-König
> <[e-mail]> wrote:
> >
> > > > Also it should be possible to know the result before actually
> > > > configuring the hardware. Otherwise things might already go wrong
> > > > because the driver implements a setting that is too far from the
> > > > requested configuration.
> > >
> > > Later in this thread Thierry didn't like the "round rate" idea due to
> > > races.  One way to solve that could be to indicate to the PWM
> > > framework which direction you'd like it to error in: a higher duty
> > > cycle or a lower one.
> >
> > I don't think this would result in settings as optimal as with my
> > suggestion. If you don't agree and want to convince me: Show how your
> > suggestion would work with a PWM that can implement only multiples of 3
> > for duty_cycle and period and you want 20% duty cycle with period <= 1
> > ms (without making use of the knowledge about the limitation of the
> > PWM in the algorithm).
> 
> I guess I was assuming that somehow this would percolate down into an
> API call that the PWM driver would implement, so you could make use of
> the PWM knowledge in the algorithm.

As there are so many different possibilities what could be "best" for a
consumer use case, I believe that it would end in a maintenance
mess if each lowlevel driver would need a callback to implement each
rounding strategy.
 
> ...but I don't have any real strong feelings about this API so I'll
> leave it to you and Thierry to hash out what makes you both happy.

I look forward to us two agreeing on a best approach ... :-)

Best regards
Uwe

Boris Brezillon May 2, 2019, 7:16 a.m. UTC | #19

On Mon, 29 Apr 2019 11:04:20 -0700
Doug Anderson <[e-mail]> wrote:

> Hi,
> 
> On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> <[e-mail]> wrote:
> >
> > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:  
> > > Hi,
> > >
> > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > this expires out of my inbox:
> > >
> > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > <[e-mail]> wrote:  
> > > > My intention here is more to make all drivers behave the same way and
> > > > because only two drivers updated the pwm_state this was the variant I
> > > > removed.  
> > >
> > > To be clear, this patch on its own is probably breaking things. Just
> > > because the other drivers don't implement the documented behavior
> > > doesn't mean you should break this driver. Maybe the others just
> > > aren't used in precise enough scenarios where this matters.
> > >  
> > > > When you say that the caller might actually care about the exact
> > > > parameters I fully agree. In this case however the consumer should be
> > > > able to know the result before actually applying it. So if you do
> > > >
> > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > >
> > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > bad things you want to know about already happend. So my idea is a new
> > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > following:
> > > >
> > > >         rstate = pwm_round_state(pwm, state)
> > > >         pwm.apply(pwm, state)
> > > >         gstate = pwm_get_state(pwm)
> > > >
> > > >         if rstate != gstate:
> > > >                 warn about problems  
> > >
> > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > we need to be 100% precise about the period, but we do need to be as
> > > precise as possible with the duty:period ratio -- so once we get the
> > > "feedback" from the underlying PWM driver what the real period will
> > > be, we adjust the duty appropriately.  
> >
> > I admit that I didn't understood the whole situation and (some) things
> > are worse with my patches applied. I still think that changing the
> > caller's state variable is bad design, but of course pwm_get_state
> > should return the currently implemented configuration.  
> 
> Regardless of the pros and cons of the current situation, hopefully
> we're in agreement that we shouldn't break existing users?  In general
> I'll probably stay out of the debate as long as we end somewhere that
> pwm_regulator is able to somehow know the actual state that it
> programmed into the hardware.
> 
> +Boris too in case he has any comments.

Well, the pwm_round_state() approach sounds okay to me, though I don't
really see why it's wrong to adjust the state in pwm_apply_state()
(just like clk_set_rate() will round the rate for you by internally
calling clk_round_rate() before applying the config). Note that
pwm_config() is doing exactly the same: it adjusts the config to HW
caps, excepts in that case you don't know it.

I do understand that some users might want to check how the HW will
adjust the period/duty before applying the new setup, and in that
regard, having pwm_round_rate() is a good thing. But in any case, it's
hard for the user to decide how to adjust things to get what it wants
(should he increase/decrease the period/duty?).

My impression is that most users care about the duty/period ratio with
little interest in accurate period settings (as long as it's close
enough to what they expect of course). For the round-up/down/closest
aspect, I guess that's also something we can pass to the new API. So,
rather than passing it a duty in ns, maybe we could pass it a ratio
(percent is probably not precise enough for some use cases, so we could
make it per-million).

Uwe Kleine-König May 2, 2019, 7:33 a.m. UTC | #20

Hello Boris,

On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> On Mon, 29 Apr 2019 11:04:20 -0700
> Doug Anderson <[e-mail]> wrote:
> 
> > Hi,
> > 
> > On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> > <[e-mail]> wrote:
> > >
> > > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:  
> > > > Hi,
> > > >
> > > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > > this expires out of my inbox:
> > > >
> > > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > > <[e-mail]> wrote:  
> > > > > My intention here is more to make all drivers behave the same way and
> > > > > because only two drivers updated the pwm_state this was the variant I
> > > > > removed.  
> > > >
> > > > To be clear, this patch on its own is probably breaking things. Just
> > > > because the other drivers don't implement the documented behavior
> > > > doesn't mean you should break this driver. Maybe the others just
> > > > aren't used in precise enough scenarios where this matters.
> > > >  
> > > > > When you say that the caller might actually care about the exact
> > > > > parameters I fully agree. In this case however the consumer should be
> > > > > able to know the result before actually applying it. So if you do
> > > > >
> > > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > >
> > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > > bad things you want to know about already happend. So my idea is a new
> > > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > > following:
> > > > >
> > > > >         rstate = pwm_round_state(pwm, state)
> > > > >         pwm.apply(pwm, state)
> > > > >         gstate = pwm_get_state(pwm)
> > > > >
> > > > >         if rstate != gstate:
> > > > >                 warn about problems  
> > > >
> > > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > > we need to be 100% precise about the period, but we do need to be as
> > > > precise as possible with the duty:period ratio -- so once we get the
> > > > "feedback" from the underlying PWM driver what the real period will
> > > > be, we adjust the duty appropriately.  
> > >
> > > I admit that I didn't understood the whole situation and (some) things
> > > are worse with my patches applied. I still think that changing the
> > > caller's state variable is bad design, but of course pwm_get_state
> > > should return the currently implemented configuration.  
> > 
> > Regardless of the pros and cons of the current situation, hopefully
> > we're in agreement that we shouldn't break existing users?  In general
> > I'll probably stay out of the debate as long as we end somewhere that
> > pwm_regulator is able to somehow know the actual state that it
> > programmed into the hardware.
> > 
> > +Boris too in case he has any comments.
> 
> Well, the pwm_round_state() approach sounds okay to me, though I don't
> really see why it's wrong to adjust the state in pwm_apply_state()
> (just like clk_set_rate() will round the rate for you by internally
> calling clk_round_rate() before applying the config).

This are two orthogonal things. The "should pwm_apply_state change the
state argument" isn't really comparable to the clk stuff, as there only
the frequency is provided that is passed by value, not by reference as
the PWM state.

The key argument for me to *not* change it is that it might yield
surprises, still more as today most drivers don't adapt. An -- I admit
constructed, not real-word -- case where adaption would go wrong is:

	pwm_apply_state(pwm1, &mystate);
	pwm_apply_state(pwm2, &mystate);

> Note that pwm_config() is doing exactly the same: it adjusts the
> config to HW caps, excepts in that case you don't know it.

I don't see what you mean here. I don't see any adaption.

> I do understand that some users might want to check how the HW will
> adjust the period/duty before applying the new setup, and in that
> regard, having pwm_round_rate() is a good thing. But in any case, it's
> hard for the user to decide how to adjust things to get what it wants
> (should he increase/decrease the period/duty?).

It depends on the use case. For one of them I suggested an algorithm.

> My impression is that most users care about the duty/period ratio with
> little interest in accurate period settings (as long as it's close
> enough to what they expect of course). For the round-up/down/closest
> aspect, I guess that's also something we can pass to the new API. So,
> rather than passing it a duty in ns, maybe we could pass it a ratio
> (percent is probably not precise enough for some use cases, so we could
> make it per-million).

Yeah, something like that would be good. Still for the device drivers I
would use the callback I suggested because that is easier to implement.
This way the complexity is once in the framework instead of in each
driver.

Best regards
Uwe

Boris Brezillon May 2, 2019, 8:09 a.m. UTC | #21

On Thu, 2 May 2019 09:33:26 +0200
Uwe Kleine-König <[e-mail]> wrote:

> Hello Boris,
> 
> On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> > On Mon, 29 Apr 2019 11:04:20 -0700
> > Doug Anderson <[e-mail]> wrote:
> >   
> > > Hi,
> > > 
> > > On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> > > <[e-mail]> wrote:  
> > > >
> > > > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:    
> > > > > Hi,
> > > > >
> > > > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > > > this expires out of my inbox:
> > > > >
> > > > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > > > <[e-mail]> wrote:    
> > > > > > My intention here is more to make all drivers behave the same way and
> > > > > > because only two drivers updated the pwm_state this was the variant I
> > > > > > removed.    
> > > > >
> > > > > To be clear, this patch on its own is probably breaking things. Just
> > > > > because the other drivers don't implement the documented behavior
> > > > > doesn't mean you should break this driver. Maybe the others just
> > > > > aren't used in precise enough scenarios where this matters.
> > > > >    
> > > > > > When you say that the caller might actually care about the exact
> > > > > > parameters I fully agree. In this case however the consumer should be
> > > > > > able to know the result before actually applying it. So if you do
> > > > > >
> > > > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > > >
> > > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > > > bad things you want to know about already happend. So my idea is a new
> > > > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > > > following:
> > > > > >
> > > > > >         rstate = pwm_round_state(pwm, state)
> > > > > >         pwm.apply(pwm, state)
> > > > > >         gstate = pwm_get_state(pwm)
> > > > > >
> > > > > >         if rstate != gstate:
> > > > > >                 warn about problems    
> > > > >
> > > > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > > > we need to be 100% precise about the period, but we do need to be as
> > > > > precise as possible with the duty:period ratio -- so once we get the
> > > > > "feedback" from the underlying PWM driver what the real period will
> > > > > be, we adjust the duty appropriately.    
> > > >
> > > > I admit that I didn't understood the whole situation and (some) things
> > > > are worse with my patches applied. I still think that changing the
> > > > caller's state variable is bad design, but of course pwm_get_state
> > > > should return the currently implemented configuration.    
> > > 
> > > Regardless of the pros and cons of the current situation, hopefully
> > > we're in agreement that we shouldn't break existing users?  In general
> > > I'll probably stay out of the debate as long as we end somewhere that
> > > pwm_regulator is able to somehow know the actual state that it
> > > programmed into the hardware.
> > > 
> > > +Boris too in case he has any comments.  
> > 
> > Well, the pwm_round_state() approach sounds okay to me, though I don't
> > really see why it's wrong to adjust the state in pwm_apply_state()
> > (just like clk_set_rate() will round the rate for you by internally
> > calling clk_round_rate() before applying the config).  
> 
> This are two orthogonal things. The "should pwm_apply_state change the
> state argument" isn't really comparable to the clk stuff, as there only
> the frequency is provided that is passed by value, not by reference as
> the PWM state.
> 
> The key argument for me to *not* change it is that it might yield
> surprises, still more as today most drivers don't adapt. An -- I admit
> constructed, not real-word -- case where adaption would go wrong is:
> 
> 	pwm_apply_state(pwm1, &mystate);
> 	pwm_apply_state(pwm2, &mystate);

I see, but it's also clearly documented that pwm_apply_state() might
adjust the period/duty params [1], and it's not like we have a lot of
PWM users converted to use pwm_apply_state(), so I'd expect them to be
aware of that and use a reference pwm_state if they need to apply it
to different devices.

> 
> > Note that pwm_config() is doing exactly the same: it adjusts the
> > config to HW caps, excepts in that case you don't know it.  
> 
> I don't see what you mean here. I don't see any adaption.

I mean that the config you end up is not necessarily what you asked
for, and pwm_apply_state() was making that explicit by returning the
actual PWM state instead of letting the user think its config has been
applied with no adjustment.

> 
> > I do understand that some users might want to check how the HW will
> > adjust the period/duty before applying the new setup, and in that
> > regard, having pwm_round_rate() is a good thing. But in any case, it's
> > hard for the user to decide how to adjust things to get what it wants
> > (should he increase/decrease the period/duty?).  
> 
> It depends on the use case. For one of them I suggested an algorithm.

Yes, I was just trying to say that passing a PWM state to
pwm_round_state() is not enough, we need extra info if we want to make
it useful, like the rounding policy, the accepted deviation on period,
duty or the duty/period ratio, ....

> 
> > My impression is that most users care about the duty/period ratio with
> > little interest in accurate period settings (as long as it's close
> > enough to what they expect of course). For the round-up/down/closest
> > aspect, I guess that's also something we can pass to the new API. So,
> > rather than passing it a duty in ns, maybe we could pass it a ratio
> > (percent is probably not precise enough for some use cases, so we could
> > make it per-million).  
> 
> Yeah, something like that would be good. Still for the device drivers I
> would use the callback I suggested because that is easier to implement.

Sorry, I just joined the discussion and couldn't find the email where
you suggested a new driver hook to deal with that. 

> This way the complexity is once in the framework instead of in each
> driver.

I think we want to make it possible for drivers to do complex
adjustments, and that implies passing all info  to the new driver
hook. The core can then provide generic helpers for simple use-cases
(approximation for static period/duty steps, where no reclocking is
involved).

[1]https://elixir.bootlin.com/linux/v5.1-rc7/source/drivers/pwm/core.c#L463

Uwe Kleine-König May 2, 2019, 8:42 a.m. UTC | #22

Hello Boris,

On Thu, May 02, 2019 at 10:09:51AM +0200, Boris Brezillon wrote:
> On Thu, 2 May 2019 09:33:26 +0200
> Uwe Kleine-König <[e-mail]> wrote:
> 
> > Hello Boris,
> > 
> > On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> > > On Mon, 29 Apr 2019 11:04:20 -0700
> > > Doug Anderson <[e-mail]> wrote:
> > >   
> > > > Hi,
> > > > 
> > > > On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> > > > <[e-mail]> wrote:  
> > > > >
> > > > > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:    
> > > > > > Hi,
> > > > > >
> > > > > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > > > > this expires out of my inbox:
> > > > > >
> > > > > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > > > > <[e-mail]> wrote:    
> > > > > > > My intention here is more to make all drivers behave the same way and
> > > > > > > because only two drivers updated the pwm_state this was the variant I
> > > > > > > removed.    
> > > > > >
> > > > > > To be clear, this patch on its own is probably breaking things. Just
> > > > > > because the other drivers don't implement the documented behavior
> > > > > > doesn't mean you should break this driver. Maybe the others just
> > > > > > aren't used in precise enough scenarios where this matters.
> > > > > >    
> > > > > > > When you say that the caller might actually care about the exact
> > > > > > > parameters I fully agree. In this case however the consumer should be
> > > > > > > able to know the result before actually applying it. So if you do
> > > > > > >
> > > > > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > > > >
> > > > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > > > > bad things you want to know about already happend. So my idea is a new
> > > > > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > > > > following:
> > > > > > >
> > > > > > >         rstate = pwm_round_state(pwm, state)
> > > > > > >         pwm.apply(pwm, state)
> > > > > > >         gstate = pwm_get_state(pwm)
> > > > > > >
> > > > > > >         if rstate != gstate:
> > > > > > >                 warn about problems    
> > > > > >
> > > > > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > > > > we need to be 100% precise about the period, but we do need to be as
> > > > > > precise as possible with the duty:period ratio -- so once we get the
> > > > > > "feedback" from the underlying PWM driver what the real period will
> > > > > > be, we adjust the duty appropriately.    
> > > > >
> > > > > I admit that I didn't understood the whole situation and (some) things
> > > > > are worse with my patches applied. I still think that changing the
> > > > > caller's state variable is bad design, but of course pwm_get_state
> > > > > should return the currently implemented configuration.    
> > > > 
> > > > Regardless of the pros and cons of the current situation, hopefully
> > > > we're in agreement that we shouldn't break existing users?  In general
> > > > I'll probably stay out of the debate as long as we end somewhere that
> > > > pwm_regulator is able to somehow know the actual state that it
> > > > programmed into the hardware.
> > > > 
> > > > +Boris too in case he has any comments.  
> > > 
> > > Well, the pwm_round_state() approach sounds okay to me, though I don't
> > > really see why it's wrong to adjust the state in pwm_apply_state()
> > > (just like clk_set_rate() will round the rate for you by internally
> > > calling clk_round_rate() before applying the config).  
> > 
> > This are two orthogonal things. The "should pwm_apply_state change the
> > state argument" isn't really comparable to the clk stuff, as there only
> > the frequency is provided that is passed by value, not by reference as
> > the PWM state.
> > 
> > The key argument for me to *not* change it is that it might yield
> > surprises, still more as today most drivers don't adapt. An -- I admit
> > constructed, not real-word -- case where adaption would go wrong is:
> > 
> > 	pwm_apply_state(pwm1, &mystate);
> > 	pwm_apply_state(pwm2, &mystate);
> 
> I see, but it's also clearly documented that pwm_apply_state() might
> adjust the period/duty params [1], and it's not like we have a lot of
> PWM users converted to use pwm_apply_state(), so I'd expect them to be
> aware of that and use a reference pwm_state if they need to apply it
> to different devices.

If we accept that pwm_apply_state should adapt its state argument that
would be ok for me, too. Then however we should make this consistent and
consider a deviation that is not reported there as a bug.

Note there are also more subtile problems. For example something like:

	def enable(self):
		state = pwm_get_state(self.pwm)
		state.duty_cycle *= 2
		pwm_apply_state(self.pwm, state)

	def disable(self):
		state = pwm_get_state(self.pwm)
		state.duty_cycle /= 2
		pwm_apply_state(self.pwm, state)

doesn't guarantee that the sequence enable(); disable(); is idempotent.
So my favourite would be to not modfies the caller's copy of state for
pwm_apply_state(). (Note, this doesn't necessarily have implications
about the semantik of the lowlevel driver callbacks.) Still
pwm_get_state() should work and yield the corrected settings.

> > > Note that pwm_config() is doing exactly the same: it adjusts the
> > > config to HW caps, excepts in that case you don't know it.  
> > 
> > I don't see what you mean here. I don't see any adaption.
> 
> I mean that the config you end up is not necessarily what you asked
> for, and pwm_apply_state() was making that explicit by returning the
> actual PWM state instead of letting the user think its config has been
> applied with no adjustment.

Ah. Of course the lowlevel driver has to work with the capabilities of
the hardware. That is something we cannot get rid of. It's just a
question how we communicate this to the consumer.

> > > I do understand that some users might want to check how the HW will
> > > adjust the period/duty before applying the new setup, and in that
> > > regard, having pwm_round_rate() is a good thing. But in any case, it's
> > > hard for the user to decide how to adjust things to get what it wants
> > > (should he increase/decrease the period/duty?).  
> > 
> > It depends on the use case. For one of them I suggested an algorithm.
> 
> Yes, I was just trying to say that passing a PWM state to
> pwm_round_state() is not enough, we need extra info if we want to make
> it useful, like the rounding policy, the accepted deviation on period,
> duty or the duty/period ratio, ....

Ack. My suggestion is that round_rate should do:

	if polarity is unsupported:
		polarity = !polarity
		duty_cycle = period - duty_cycle

	period = biggest supportable period <= requested period, 0 if no
		such period exists.

	duty_cycle = biggest supportable duty cycle <= requested
		duty_cycle, 0 if no such value exists

This would allow to let the consumer (or framework helper function)
decide which deviation is ok.

> > > My impression is that most users care about the duty/period ratio with
> > > little interest in accurate period settings (as long as it's close
> > > enough to what they expect of course). For the round-up/down/closest
> > > aspect, I guess that's also something we can pass to the new API. So,
> > > rather than passing it a duty in ns, maybe we could pass it a ratio
> > > (percent is probably not precise enough for some use cases, so we could
> > > make it per-million).  
> > 
> > Yeah, something like that would be good. Still for the device drivers I
> > would use the callback I suggested because that is easier to implement.
> 
> Sorry, I just joined the discussion and couldn't find the email where
> you suggested a new driver hook to deal with that. 

https://www.spinics.net/lists/linux-pwm/msg09627.html

> > This way the complexity is once in the framework instead of in each
> > driver.
> 
> I think we want to make it possible for drivers to do complex
> adjustments, and that implies passing all info  to the new driver
> hook. The core can then provide generic helpers for simple use-cases
> (approximation for static period/duty steps, where no reclocking is
> involved).

The problem is that it is hard to come up with a formalism to map "all
info" because there are so many different ways to prefer one
configuration over another. I believe we won't be able to design a sane
callback prototype that allows to map all use cases.

Best regards
Uwe

Thierry Reding May 3, 2019, 10:59 a.m. UTC | #23

On Thu, May 02, 2019 at 10:42:43AM +0200, Uwe Kleine-König wrote:
> Hello Boris,
> 
> On Thu, May 02, 2019 at 10:09:51AM +0200, Boris Brezillon wrote:
> > On Thu, 2 May 2019 09:33:26 +0200
> > Uwe Kleine-König <[e-mail]> wrote:
> > 
> > > Hello Boris,
> > > 
> > > On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> > > > On Mon, 29 Apr 2019 11:04:20 -0700
> > > > Doug Anderson <[e-mail]> wrote:
> > > >   
> > > > > Hi,
> > > > > 
> > > > > On Sun, Apr 28, 2019 at 11:56 PM Uwe Kleine-König
> > > > > <[e-mail]> wrote:  
> > > > > >
> > > > > > On Thu, Apr 18, 2019 at 05:27:05PM -0700, Brian Norris wrote:    
> > > > > > > Hi,
> > > > > > >
> > > > > > > I'm not sure if I'm misreading you, but I thought I'd add here before
> > > > > > > this expires out of my inbox:
> > > > > > >
> > > > > > > On Mon, Apr 8, 2019 at 7:39 AM Uwe Kleine-König
> > > > > > > <[e-mail]> wrote:    
> > > > > > > > My intention here is more to make all drivers behave the same way and
> > > > > > > > because only two drivers updated the pwm_state this was the variant I
> > > > > > > > removed.    
> > > > > > >
> > > > > > > To be clear, this patch on its own is probably breaking things. Just
> > > > > > > because the other drivers don't implement the documented behavior
> > > > > > > doesn't mean you should break this driver. Maybe the others just
> > > > > > > aren't used in precise enough scenarios where this matters.
> > > > > > >    
> > > > > > > > When you say that the caller might actually care about the exact
> > > > > > > > parameters I fully agree. In this case however the consumer should be
> > > > > > > > able to know the result before actually applying it. So if you do
> > > > > > > >
> > > > > > > >         pwm_apply_state(pwm, { .period = 17, .duty_cycle = 12, ...})
> > > > > > > >
> > > > > > > > and this results in .period = 100 and .duty_cycle = 0 then probably the
> > > > > > > > bad things you want to know about already happend. So my idea is a new
> > > > > > > > function pwm_round_state() that does the adaptions to pwm_state without
> > > > > > > > applying it to the hardware. After that pwm_apply_state could do the
> > > > > > > > following:
> > > > > > > >
> > > > > > > >         rstate = pwm_round_state(pwm, state)
> > > > > > > >         pwm.apply(pwm, state)
> > > > > > > >         gstate = pwm_get_state(pwm)
> > > > > > > >
> > > > > > > >         if rstate != gstate:
> > > > > > > >                 warn about problems    
> > > > > > >
> > > > > > > For our case (we're using this with pwm-regulator), I don't recall [*]
> > > > > > > we need to be 100% precise about the period, but we do need to be as
> > > > > > > precise as possible with the duty:period ratio -- so once we get the
> > > > > > > "feedback" from the underlying PWM driver what the real period will
> > > > > > > be, we adjust the duty appropriately.    
> > > > > >
> > > > > > I admit that I didn't understood the whole situation and (some) things
> > > > > > are worse with my patches applied. I still think that changing the
> > > > > > caller's state variable is bad design, but of course pwm_get_state
> > > > > > should return the currently implemented configuration.    
> > > > > 
> > > > > Regardless of the pros and cons of the current situation, hopefully
> > > > > we're in agreement that we shouldn't break existing users?  In general
> > > > > I'll probably stay out of the debate as long as we end somewhere that
> > > > > pwm_regulator is able to somehow know the actual state that it
> > > > > programmed into the hardware.
> > > > > 
> > > > > +Boris too in case he has any comments.  
> > > > 
> > > > Well, the pwm_round_state() approach sounds okay to me, though I don't
> > > > really see why it's wrong to adjust the state in pwm_apply_state()
> > > > (just like clk_set_rate() will round the rate for you by internally
> > > > calling clk_round_rate() before applying the config).  
> > > 
> > > This are two orthogonal things. The "should pwm_apply_state change the
> > > state argument" isn't really comparable to the clk stuff, as there only
> > > the frequency is provided that is passed by value, not by reference as
> > > the PWM state.
> > > 
> > > The key argument for me to *not* change it is that it might yield
> > > surprises, still more as today most drivers don't adapt. An -- I admit
> > > constructed, not real-word -- case where adaption would go wrong is:
> > > 
> > > 	pwm_apply_state(pwm1, &mystate);
> > > 	pwm_apply_state(pwm2, &mystate);
> > 
> > I see, but it's also clearly documented that pwm_apply_state() might
> > adjust the period/duty params [1], and it's not like we have a lot of
> > PWM users converted to use pwm_apply_state(), so I'd expect them to be
> > aware of that and use a reference pwm_state if they need to apply it
> > to different devices.
> 
> If we accept that pwm_apply_state should adapt its state argument that
> would be ok for me, too. Then however we should make this consistent and
> consider a deviation that is not reported there as a bug.
> 
> Note there are also more subtile problems. For example something like:
> 
> 	def enable(self):
> 		state = pwm_get_state(self.pwm)
> 		state.duty_cycle *= 2
> 		pwm_apply_state(self.pwm, state)
> 
> 	def disable(self):
> 		state = pwm_get_state(self.pwm)
> 		state.duty_cycle /= 2
> 		pwm_apply_state(self.pwm, state)
> 
> doesn't guarantee that the sequence enable(); disable(); is idempotent.
> So my favourite would be to not modfies the caller's copy of state for
> pwm_apply_state(). (Note, this doesn't necessarily have implications
> about the semantik of the lowlevel driver callbacks.) Still
> pwm_get_state() should work and yield the corrected settings.
> 
> > > > Note that pwm_config() is doing exactly the same: it adjusts the
> > > > config to HW caps, excepts in that case you don't know it.  
> > > 
> > > I don't see what you mean here. I don't see any adaption.
> > 
> > I mean that the config you end up is not necessarily what you asked
> > for, and pwm_apply_state() was making that explicit by returning the
> > actual PWM state instead of letting the user think its config has been
> > applied with no adjustment.
> 
> Ah. Of course the lowlevel driver has to work with the capabilities of
> the hardware. That is something we cannot get rid of. It's just a
> question how we communicate this to the consumer.
> 
> > > > I do understand that some users might want to check how the HW will
> > > > adjust the period/duty before applying the new setup, and in that
> > > > regard, having pwm_round_rate() is a good thing. But in any case, it's
> > > > hard for the user to decide how to adjust things to get what it wants
> > > > (should he increase/decrease the period/duty?).  
> > > 
> > > It depends on the use case. For one of them I suggested an algorithm.
> > 
> > Yes, I was just trying to say that passing a PWM state to
> > pwm_round_state() is not enough, we need extra info if we want to make
> > it useful, like the rounding policy, the accepted deviation on period,
> > duty or the duty/period ratio, ....
> 
> Ack. My suggestion is that round_rate should do:
> 
> 	if polarity is unsupported:
> 		polarity = !polarity
> 		duty_cycle = period - duty_cycle

This should really be up to the consumer. The PWM framework or driver
doesn't know whether or not the consumer cares about the polarity or
whether it only cares about the power output.

> 	period = biggest supportable period <= requested period, 0 if no
> 		such period exists.
> 
> 	duty_cycle = biggest supportable duty cycle <= requested
> 		duty_cycle, 0 if no such value exists

This doesn't really work. We need some way to detect "value does not
exist" that is different from value == 0, because value == 0 is a
legitimate use-case.

> This would allow to let the consumer (or framework helper function)
> decide which deviation is ok.

So what's the consumer supposed to do if it gets back these values? How
does it know how to scale them if the deviation is not okay? What in
case the hardware can do achieve a good period that is slightly bigger
than the requested period and which would give a very good result?

Thierry

> > > > My impression is that most users care about the duty/period ratio with
> > > > little interest in accurate period settings (as long as it's close
> > > > enough to what they expect of course). For the round-up/down/closest
> > > > aspect, I guess that's also something we can pass to the new API. So,
> > > > rather than passing it a duty in ns, maybe we could pass it a ratio
> > > > (percent is probably not precise enough for some use cases, so we could
> > > > make it per-million).  
> > > 
> > > Yeah, something like that would be good. Still for the device drivers I
> > > would use the callback I suggested because that is easier to implement.
> > 
> > Sorry, I just joined the discussion and couldn't find the email where
> > you suggested a new driver hook to deal with that. 
> 
> https://www.spinics.net/lists/linux-pwm/msg09627.html
> 
> > > This way the complexity is once in the framework instead of in each
> > > driver.
> > 
> > I think we want to make it possible for drivers to do complex
> > adjustments, and that implies passing all info  to the new driver
> > hook. The core can then provide generic helpers for simple use-cases
> > (approximation for static period/duty steps, where no reclocking is
> > involved).
> 
> The problem is that it is hard to come up with a formalism to map "all
> info" because there are so many different ways to prefer one
> configuration over another. I believe we won't be able to design a sane
> callback prototype that allows to map all use cases.
> 
> Best regards
> Uwe
> 
> -- 
> Pengutronix e.K.                           | Uwe Kleine-König            |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Uwe Kleine-König May 3, 2019, 7:59 p.m. UTC | #24

Hello Thierry,

On Fri, May 03, 2019 at 12:59:15PM +0200, Thierry Reding wrote:
> On Thu, May 02, 2019 at 10:42:43AM +0200, Uwe Kleine-König wrote:
> > On Thu, May 02, 2019 at 10:09:51AM +0200, Boris Brezillon wrote:
> > > On Thu, 2 May 2019 09:33:26 +0200
> > > Uwe Kleine-König <[e-mail]> wrote:
> > > > On Thu, May 02, 2019 at 09:16:38AM +0200, Boris Brezillon wrote:
> > > > > I do understand that some users might want to check how the HW will
> > > > > adjust the period/duty before applying the new setup, and in that
> > > > > regard, having pwm_round_rate() is a good thing. But in any case, it's
> > > > > hard for the user to decide how to adjust things to get what it wants
> > > > > (should he increase/decrease the period/duty?).  

Whenever I wrote pwm_round_rate I actually meant pwm_round_state.

> > > > It depends on the use case. For one of them I suggested an algorithm.
> > > 
> > > Yes, I was just trying to say that passing a PWM state to
> > > pwm_round_state() is not enough, we need extra info if we want to make
> > > it useful, like the rounding policy, the accepted deviation on period,
> > > duty or the duty/period ratio, ....
> > 
> > Ack. My suggestion is that round_rate should do:
> > 
> > 	if polarity is unsupported:
> > 		polarity = !polarity
> > 		duty_cycle = period - duty_cycle
> 
> This should really be up to the consumer. The PWM framework or driver
> doesn't know whether or not the consumer cares about the polarity or
> whether it only cares about the power output.

You don't understand my idea. If the hardware cannot implement the
requested state the round_state function has to return a best
approximation according to some rules. After that the consumer can
decide if they want that or not. As the hardware isn't touched nothing
bad happens.

(Well, apart from that I still cannot imagine a use case where with the
current possibilities of the PWM API a consumer can really care about
the polarity.)

> > 	period = biggest supportable period <= requested period, 0 if no
> > 		such period exists.
> > 
> > 	duty_cycle = biggest supportable duty cycle <= requested
> > 		duty_cycle, 0 if no such value exists
> 
> This doesn't really work. We need some way to detect "value does not
> exist" that is different from value == 0, because value == 0 is a
> legitimate use-case.

Same as above. If I asked for duty_cycle = 5 ns and get back duty_cycle
= 0 ns this means the driver cannot implement a duty_cycle in the
interval (0, 5] ns, nothing more. 

> > This would allow to let the consumer (or framework helper function)
> > decide which deviation is ok.
> 
> So what's the consumer supposed to do if it gets back these values?

If the value is then known to be the best (or good enough if the
consumer doesn't really care), it is used. Otherwise a different state
is rounded or the consumer gives up if it becomes clear that the
hardware cannot satisfy their needs.

> How does it know how to scale them if the deviation is not okay?

It depends on what the consumer considers optimal. I already gave one
example code how I think pwm_round_state should be used. Another is:

If the consumer wants a period as near as possible to a given target
period the algorithm with the round_state function as suggested would
look as follows (duty_cycle ignored here for the ease of discussion):

		mystate.period = target_period
		...
		pwm_round_state(pwm, &mystate)

		if (mystate.period == target_period)
			return mystate.period

		mystate.period = 2 * target_period - mystate.period
		pwm_round_state(pwm, &mystate)

		if (mystate.period < target_period)
			return mystate.period;

		do {
			best_bigger = mystate.period
			mystate.period -= 1
			pwm_round_state(pwm, &mystate)
		} while (mystate.period > target_period)

		return best_bigger

> What in case the hardware can do achieve a good period that is
> slightly bigger than the requested period and which would give a very
> good result?

The above algorithm answers this question.

Theoretically the requirements for pwm_round_state could be changed such
that it is supposed to yield the nearest hit instead of the next smaller
one, but this only shifts the complexity to a different area. (I think
my suggestion is the better one though because the math for the lowlevel
drivers and also the helper functions is easier then. Also the search
terminates earlier if the consumer wants something bigger than the
driver can implement. Moreover "biggest value not bigger than requested"
is (IMHO) easier to understand for the affected coders than "nearest
value" because there are more ugly special cases. (Consider I ask for
(duty_cycle, period) = (10, 20) and the driver can implement (9, 18) and
(12, 21). Which of these is nearer?))

Best regards
Uwe

Patch

diff --git a/drivers/pwm/pwm-rockchip.c b/drivers/pwm/pwm-rockchip.c
index 4d99d468df09..16186bcd99e0 100644
--- a/drivers/pwm/pwm-rockchip.c
+++ b/drivers/pwm/pwm-rockchip.c
@@ -215,12 +215,6 @@  static int rockchip_pwm_apply(struct pwm_chip *chip, struct pwm_device *pwm,
 			goto out;
 	}
 
-	/*
-	 * Update the state with the real hardware, which can differ a bit
-	 * because of period/duty_cycle approximation.
-	 */
-	rockchip_pwm_get_state(chip, pwm, state);
-
 out:
 	clk_disable(pc->pclk);