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

Doug Anderson
2019-04-01T22:45:47Z

Hi,

On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <heiko@sntech.de> 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 <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@sntech.de>
>
> 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

_______________________________________________
Linux-rockchip mailing list
Linux-rockchip@lists.infradead.org