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@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? You're right, I missed that the updated state was saved. > > > 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... 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 -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | _______________________________________________ Linux-rockchip mailing list Linux-rockchip@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
