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

Thierry Reding
2019-04-29T11:03:54Z

[-- Attachment #1.1: Type: text/plain, Size: 4899 bytes --]

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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org> 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 <u.kleine-koenig@pengutronix.de>
> > >
> > > 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 <heiko-4mtYJXux2i+zQB+pC5nmwQ@public.gmane.org>
> > >
> > > 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/ |

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 200 bytes --]

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org