Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 03:43:56PM +0200, Uwe Kleine-König wrote: > Hello Thierry, > > On Tue, Apr 06, 2021 at 01:16:31PM +0200, Thierry Reding wrote: > > On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > > > Given that lowlevel drivers usually cannot implement exactly what a > > > consumer requests with pwm_apply_state() there is some rounding involved. > > > > > > pwm_get_state() traditionally returned the setting that was requested most > > > recently by the consumer (opposed to what was actually implemented in > > > hardware in reply to the last request). To make this semantic obvious > > > rename the function. > > > > > > Signed-off-by: Uwe Kleine-König > > > --- > > > Documentation/driver-api/pwm.rst | 6 +++- > > > drivers/clk/clk-pwm.c | 2 +- > > > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > > > drivers/input/misc/da7280.c| 2 +- > > > drivers/input/misc/pwm-beeper.c| 2 +- > > > drivers/input/misc/pwm-vibra.c | 4 +-- > > > drivers/pwm/core.c | 4 +-- > > > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > > > drivers/pwm/pwm-atmel.c| 2 +- > > > drivers/pwm/pwm-imx27.c| 2 +- > > > drivers/pwm/pwm-rockchip.c | 2 +- > > > drivers/pwm/pwm-stm32-lp.c | 4 +-- > > > drivers/pwm/pwm-sun4i.c| 2 +- > > > drivers/pwm/sysfs.c| 18 ++-- > > > drivers/regulator/pwm-regulator.c | 4 +-- > > > drivers/video/backlight/pwm_bl.c | 10 +++ > > > include/linux/pwm.h| 34 ++ > > > 17 files changed, 59 insertions(+), 45 deletions(-) > > > > Honestly, I don't think this is worth the churn. If you think people > > will easily get confused by this then a better solution might be to more > > explicitly document the pwm_get_state() function to say exactly what it > > returns. > > I'm not so optimistic that people become aware of the semantic just > because there is documentation describing it and I strongly believe that > a good name for functions is more important than accurate documentation. > > If you don't agree, what do you think about the updated wording in > Documentation/driver-api/pwm.rst? Yeah, that clarifies this a bit. I can apply that hunk of the patch separately. > > But there's no need to make life difficult for everyone by > > renaming this to something as cumbersome as this. > > I don't expect any merge conflicts (and if still a problem occurs > resolving should be trivial enough). So I obviously don't agree to your > weighing. I wasn't talking about merge conflicts but instead about the extra churn of changing all consumers and having to type all these extra characters for no benefit. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
Hello Thierry, On Tue, Apr 06, 2021 at 01:16:31PM +0200, Thierry Reding wrote: > On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > > Given that lowlevel drivers usually cannot implement exactly what a > > consumer requests with pwm_apply_state() there is some rounding involved. > > > > pwm_get_state() traditionally returned the setting that was requested most > > recently by the consumer (opposed to what was actually implemented in > > hardware in reply to the last request). To make this semantic obvious > > rename the function. > > > > Signed-off-by: Uwe Kleine-König > > --- > > Documentation/driver-api/pwm.rst | 6 +++- > > drivers/clk/clk-pwm.c | 2 +- > > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > > drivers/input/misc/da7280.c| 2 +- > > drivers/input/misc/pwm-beeper.c| 2 +- > > drivers/input/misc/pwm-vibra.c | 4 +-- > > drivers/pwm/core.c | 4 +-- > > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > > drivers/pwm/pwm-atmel.c| 2 +- > > drivers/pwm/pwm-imx27.c| 2 +- > > drivers/pwm/pwm-rockchip.c | 2 +- > > drivers/pwm/pwm-stm32-lp.c | 4 +-- > > drivers/pwm/pwm-sun4i.c| 2 +- > > drivers/pwm/sysfs.c| 18 ++-- > > drivers/regulator/pwm-regulator.c | 4 +-- > > drivers/video/backlight/pwm_bl.c | 10 +++ > > include/linux/pwm.h| 34 ++ > > 17 files changed, 59 insertions(+), 45 deletions(-) > > Honestly, I don't think this is worth the churn. If you think people > will easily get confused by this then a better solution might be to more > explicitly document the pwm_get_state() function to say exactly what it > returns. I'm not so optimistic that people become aware of the semantic just because there is documentation describing it and I strongly believe that a good name for functions is more important than accurate documentation. If you don't agree, what do you think about the updated wording in Documentation/driver-api/pwm.rst? > But there's no need to make life difficult for everyone by > renaming this to something as cumbersome as this. I don't expect any merge conflicts (and if still a problem occurs resolving should be trivial enough). So I obviously don't agree to your weighing. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König| Industrial Linux Solutions | https://www.pengutronix.de/ | signature.asc Description: PGP signature
RE: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, 06 Apr 2021, Uwe Kleine-König wrote: > Given that lowlevel drivers usually cannot implement exactly what a > consumer requests with pwm_apply_state() there is some > rounding involved. > > pwm_get_state() traditionally returned the setting that was requested > most recently by the consumer (opposed to what was > actually implemented in hardware in reply to the last request). > To make this semantic obvious rename the function. > > Signed-off-by: Uwe Kleine-König > --- > drivers/input/misc/da7280.c| 2 +- Acked-by: Roy Im
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > Given that lowlevel drivers usually cannot implement exactly what a > consumer requests with pwm_apply_state() there is some rounding involved. Acked-by: Mark Brown signature.asc Description: PGP signature
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, Apr 06, 2021 at 09:30:36AM +0200, Uwe Kleine-König wrote: > Given that lowlevel drivers usually cannot implement exactly what a > consumer requests with pwm_apply_state() there is some rounding involved. > > pwm_get_state() traditionally returned the setting that was requested most > recently by the consumer (opposed to what was actually implemented in > hardware in reply to the last request). To make this semantic obvious > rename the function. > > Signed-off-by: Uwe Kleine-König > --- > Documentation/driver-api/pwm.rst | 6 +++- > drivers/clk/clk-pwm.c | 2 +- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- > drivers/input/misc/da7280.c| 2 +- > drivers/input/misc/pwm-beeper.c| 2 +- > drivers/input/misc/pwm-vibra.c | 4 +-- > drivers/pwm/core.c | 4 +-- > drivers/pwm/pwm-atmel-hlcdc.c | 2 +- > drivers/pwm/pwm-atmel.c| 2 +- > drivers/pwm/pwm-imx27.c| 2 +- > drivers/pwm/pwm-rockchip.c | 2 +- > drivers/pwm/pwm-stm32-lp.c | 4 +-- > drivers/pwm/pwm-sun4i.c| 2 +- > drivers/pwm/sysfs.c| 18 ++-- > drivers/regulator/pwm-regulator.c | 4 +-- > drivers/video/backlight/pwm_bl.c | 10 +++ > include/linux/pwm.h| 34 ++ > 17 files changed, 59 insertions(+), 45 deletions(-) Honestly, I don't think this is worth the churn. If you think people will easily get confused by this then a better solution might be to more explicitly document the pwm_get_state() function to say exactly what it returns. But there's no need to make life difficult for everyone by renaming this to something as cumbersome as this. Thierry signature.asc Description: PGP signature
Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic
On Tue, 06 Apr 2021, Uwe Kleine-König wrote: > Given that lowlevel drivers usually cannot implement exactly what a > consumer requests with pwm_apply_state() there is some rounding involved. > > pwm_get_state() traditionally returned the setting that was requested most > recently by the consumer (opposed to what was actually implemented in > hardware in reply to the last request). To make this semantic obvious > rename the function. > > Signed-off-by: Uwe Kleine-König > --- > drivers/gpu/drm/i915/display/intel_panel.c | 4 +-- Acked-by: Jani Nikula -- Jani Nikula, Intel Open Source Graphics Center
