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

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

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

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?

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 <u.kleine-koenig-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> >
> > 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...
> 
> 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

[-- 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
http://lists.infradead.org/mailman/listinfo/linux-rockchip