linux-kernel - Re: [PATCH] pwm: Rename pwm_get_state() to better reflect its semantic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <YHBD7AhOJGyELpVZ@orome.fritz.box>
Date:   Fri, 9 Apr 2021 14:09:16 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
Cc:     Alexandre Belloni <alexandre.belloni@...tlin.com>,
        linux-doc@...r.kernel.org, David Airlie <airlied@...ux.ie>,
        Michael Turquette <mturquette@...libre.com>,
        linux-fbdev@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        Nicolas Ferre <nicolas.ferre@...rochip.com>,
        Lee Jones <lee.jones@...aro.org>,
        linux-stm32@...md-mailman.stormreply.com,
        Daniel Thompson <daniel.thompson@...aro.org>,
        Jonathan Corbet <corbet@....net>,
        Ludovic Desroches <ludovic.desroches@...rochip.com>,
        linux-clk@...r.kernel.org, linux-rockchip@...ts.infradead.org,
        Chen-Yu Tsai <wens@...e.org>,
        NXP Linux Team <linux-imx@....com>,
        linux-input@...r.kernel.org, Sascha Hauer <s.hauer@...gutronix.de>,
        linux-pwm@...r.kernel.org,
        Alexandre Torgue <alexandre.torgue@...com>,
        intel-gfx@...ts.freedesktop.org, Mark Brown <broonie@...nel.org>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Fabrice Gasnier <fabrice.gasnier@...com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        linux-arm-kernel@...ts.infradead.org,
        Maxime Coquelin <mcoquelin.stm32@...il.com>,
        Jernej Skrabec <jernej.skrabec@...l.net>,
        Support Opensource <support.opensource@...semi.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Jingoo Han <jingoohan1@...il.com>,
        Dmitry Torokhov <dmitry.torokhov@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-kernel@...r.kernel.org, Shawn Guo <shawnguo@...nel.org>,
        Claudiu Beznea <claudiu.beznea@...rochip.com>
Subject: 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 <u.kleine-koenig@...gutronix.de>
> > > ---
> > >  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

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)