linux-kernel - Re: [PATCH v1 4/4] pwm: sysfs: Utilize an array for polarity strings

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Sun, 7 Aug 2022 13:23:32 +0200
From:   Uwe Kleine-König <u.kleine-koenig@...gutronix.de>
To:     Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Cc:     linux-pwm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>
Subject: Re: [PATCH v1 4/4] pwm: sysfs: Utilize an array for polarity strings

Hello Andy,

On Sun, Aug 07, 2022 at 12:23:31AM +0300, Andy Shevchenko wrote:
> Code is smaller and looks nicer if we combine polarity strings into an array.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
> ---
>  drivers/pwm/sysfs.c | 32 ++++++++++++--------------------
>  1 file changed, 12 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c
> index 767c4b19afb1..1bbc5286b7c6 100644
> --- a/drivers/pwm/sysfs.c
> +++ b/drivers/pwm/sysfs.c
> @@ -151,27 +151,23 @@ static ssize_t enable_store(struct device *child,
>  	return ret ? : size;
>  }
>  
> +static const char * const polarity_strings[] = {

I like having function and variable prefixes, so I'd prefer this to be
called pwm_polarity_strings[]. (Side note: The device show and store
callbacks obviously don't have a prefix either because DEVICE_ATTR_RW et
al force the functions to be called "${name}_show" and "${name}_store".
I considered already a few times to introduce something like

	#define __ATTR_NS_RW_MODE(_name, _ns,_mode) {                           \
		.attr   = { .name = __stringify(_name),                         \
			    .mode = VERIFY_OCTAL_PERMISSIONS(_mode) },          \
		.show   = _ns ## _ ## _name ## _show,                           \
		.store  = _ns ## _ ## _name ## _store,                          \
	}

	#define DEVICE_ATTR_NS_RW(_name, _ns) \
		struct device_attribute _ns ## _dev_attr_ ## _name = __ATTR_NS_RW_MODE(_name, _ns, 0600)

To allow the functions to have a name space. Never came around to do
that though.)

> +	[PWM_POLARITY_NORMAL]	= "normal",
> +	[PWM_POLARITY_INVERSED]	= "inversed",

I slightly prefer to not align the = in such definitions. Using a single
plain space before = is already used in the definiton of pwm_class in
the same file.

Otherwise the patch looks fine.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

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