Hi, On Sat, Mar 30, 2019 at 2:17 AM Heiko Stuebner <heiko@xxxxxxxxx> 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@xxxxxxxxxxxxxx> > > 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@xxxxxxxxx> > > 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@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
