Hi, On Tue, Sep 3, 2019 at 2:07 PM Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > On Tue, Sep 03, 2019 at 01:50:27PM -0700, Doug Anderson wrote: > > Hi, > > > > On Tue, Sep 3, 2019 at 1:15 PM Uwe Kleine-König > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > > > On Tue, Sep 03, 2019 at 12:35:25PM -0700, Doug Anderson wrote: > > > > Hi, > > > > > > > > On Tue, Sep 3, 2019 at 11:48 AM Uwe Kleine-König > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > > > > > > > Hello, > > > > > > > > > > On Tue, Sep 03, 2019 at 09:54:37AM -0700, Doug Anderson wrote: > > > > > > On Mon, Sep 2, 2019 at 7:27 AM Uwe Kleine-König > > > > > > <u.kleine-koenig@xxxxxxxxxxxxxx> wrote: > > > > > > > On Fri, Aug 30, 2019 at 10:39:16AM -0700, Doug Anderson wrote: > > > > > > > > On Sat, Aug 24, 2019 at 8:37 AM Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx> wrote: > > > > > > > > > > > > > > > > > > The pwm-fsl-ftm driver is one of only three 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. > > > > > > > > > > > > > > > > > > Signed-off-by: Uwe Kleine-König <uwe@xxxxxxxxxxxxxxxxx> > > > > > > > > > --- > > > > > > > > > drivers/pwm/pwm-fsl-ftm.c | 4 ---- > > > > > > > > > 1 file changed, 4 deletions(-) > > > > > > > > > > > > > > > > Presumably this patch could break something since the pwm-fsl-ftm > > > > > > > > driver doesn't appear to implement the get_state() function. ...or > > > > > > > > did I miss it? > > > > > > > > > > > > > > I don't expect breakage. We have more than 50 pwm drivers and only three > > > > > > > of them made use of adapting the passed state. So unless you do > > > > > > > something special with the PWM (i.e. more than backlight, LED or fan > > > > > > > control) I don't think a consumer might care. But it might well be that > > > > > > > I miss something so feel free to prove me wrong. > > > > > > > > > > > > I don't have this hardware so I can't prove you wrong. ...but > > > > > > presumably someone added the code to return the state on purpose? > > > > > > > > > > > > Maybe you could implement get_state() for this driver in your series? > > > > > > > > > > Sure, I could. But I don't have hardware either and so I'm not in a > > > > > better position than anybody else on this list. > > > > > > > > > > I suggest to apply as is during the merge window, and let affected > > > > > user report problems (or patches) if there really is an issue. > > > > > Guessing what people might suffer from and trying to cure this with > > > > > untested patches won't help I think. > > > > > > > > I suppose it's not up to me, but I would rather have a patch that > > > > attempts to keep things working like they did before rather than one > > > > that is known to change behavior. Even worse is that your patch > > > > description doesn't mention this functionality change at all. > > > > > > I suggest to add > > > > > > As the driver doesn't provide a .get_state() callback it is > > > expected that this changes behaviour slightly as pwm_get_state() > > > will yield the last set instead of the last implemented setting. > > > > > > to the commit log to fix this. > > > > > > > I will also note that not everyone does a deep test of all > > > > functionality during every kernel merge window. ...so your change in > > > > functionality certain has a pretty high chance of remaining broken for > > > > a while. > > > > > > I don't expect any real breakage. The changed behaviour only affects > > > users of pwm_get_state() that is called after pwm_apply_state(). > > > > > > > In addition if a PWM is used for something like a PWM > > > > regulator then subtle changes can cause totally non-obvious breakages, > > > > maybe adjusting regulators by a very small percentage. > > > > > > So for drivers/regulator/pwm-regulator.c this affects the .get_voltage() > > > call only. Note that .set_voltage() does call pwm_get_state() but > > > doesn't use the result. I don't see how my change would affect the > > > configuration written to the PWM registers when the PWM regulator driver > > > is its user. So if you want to convince me that the PWM regulator is one > > > of the potentially affected consumers, you have to work a bit harder. > > > :-) > > > > Prior to your patch, pwm_apply_state() would call the ->apply() > > function, right? That would modify the state. Then pwm_apply_state() > > would store the state (after it had been modified) into pwm->state. > > All future calls to pwm_get_state() would return the modified state. > > > > ...this means that the call to pwm_get_state() in > > pwm_regulator_get_voltage() would return the actual hardware state. > > > > After your patch series pwm_get_state() will not return the actual > > hardware state for "pwm-fsl-ftm.c", it will return the state that was > > programmed. > > > > While pwm_set_voltage() will not necessarily be affected, future calls > > to pwm_regulator_get_voltage() could be affected. Unless you are > > asserting that 100% of the calls to pwm_get_voltage() cosmetic. > > > > > > Please correct anything I got wrong there. > > I think this is all true. The key question here is then: Who calls the > .get_voltage() callback and cares about the result? Yes, it changes a > few files in sysfs but apart from that? There are lots of drivers that call get_voltage() for things other than sysfs, but without auditing each one I can't say if any of them would change behavior in a way that would matter. -Doug _______________________________________________ Linux-rockchip mailing list Linux-rockchip@xxxxxxxxxxxxxxxxxxx http://lists.infradead.org/mailman/listinfo/linux-rockchip
