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

[Uwe Kleine-König]

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@xxxxxxxxxxxxxxx>
> ---
>  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/ |

Attachment: signature.asc
Description: PGP signature