Code is smaller and looks nicer if we combine polarity strings
into an array.
Signed-off-by: Andy Shevchenko <[email protected]>
---
v2: added pwm_ prefix to the variable (Uwe), adjusted intendation (Uwe)
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..502167e44a3d 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 pwm_polarity_strings[] = {
+ [PWM_POLARITY_NORMAL] = "normal",
+ [PWM_POLARITY_INVERSED] = "inversed",
+};
+
static ssize_t polarity_show(struct device *child,
struct device_attribute *attr,
char *buf)
{
const struct pwm_device *pwm = child_to_pwm_device(child);
- const char *polarity = "unknown";
struct pwm_state state;
pwm_get_state(pwm, &state);
+ if (state.polarity < 0 || state.polarity >= ARRAY_SIZE(pwm_polarity_strings))
+ return sysfs_emit(buf, "unknown\n");
- switch (state.polarity) {
- case PWM_POLARITY_NORMAL:
- polarity = "normal";
- break;
-
- case PWM_POLARITY_INVERSED:
- polarity = "inversed";
- break;
- }
-
- return sysfs_emit(buf, "%s\n", polarity);
+ return sysfs_emit(buf, "%s\n", pwm_polarity_strings[state.polarity]);
}
static ssize_t polarity_store(struct device *child,
@@ -180,20 +176,16 @@ static ssize_t polarity_store(struct device *child,
{
struct pwm_export *export = child_to_pwm_export(child);
struct pwm_device *pwm = export->pwm;
- enum pwm_polarity polarity;
struct pwm_state state;
int ret;
- if (sysfs_streq(buf, "normal"))
- polarity = PWM_POLARITY_NORMAL;
- else if (sysfs_streq(buf, "inversed"))
- polarity = PWM_POLARITY_INVERSED;
- else
- return -EINVAL;
+ ret = sysfs_match_string(pwm_polarity_strings, buf);
+ if (ret < 0)
+ return ret;
mutex_lock(&export->lock);
pwm_get_state(pwm, &state);
- state.polarity = polarity;
+ state.polarity = ret;
ret = pwm_apply_state(pwm, &state);
mutex_unlock(&export->lock);
--
2.35.1
On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> Code is smaller and looks nicer if we combine polarity strings
> into an array.
It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
are now required to be 0 and 1. As the only 2 values in
an enum they are, but that's not really guaranteed unless
you read the enum definition.
> Signed-off-by: Andy Shevchenko <[email protected]>
> ---
> v2: added pwm_ prefix to the variable (Uwe), adjusted intendation (Uwe)
> 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..502167e44a3d 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 pwm_polarity_strings[] = {
> + [PWM_POLARITY_NORMAL] = "normal",
> + [PWM_POLARITY_INVERSED] = "inversed",
> +};
> +
> static ssize_t polarity_show(struct device *child,
> struct device_attribute *attr,
> char *buf)
> {
> const struct pwm_device *pwm = child_to_pwm_device(child);
> - const char *polarity = "unknown";
> struct pwm_state state;
>
> pwm_get_state(pwm, &state);
> + if (state.polarity < 0 || state.polarity >= ARRAY_SIZE(pwm_polarity_strings))
> + return sysfs_emit(buf, "unknown\n");
>
> - switch (state.polarity) {
> - case PWM_POLARITY_NORMAL:
> - polarity = "normal";
> - break;
> -
> - case PWM_POLARITY_INVERSED:
> - polarity = "inversed";
> - break;
> - }
> -
> - return sysfs_emit(buf, "%s\n", polarity);
> + return sysfs_emit(buf, "%s\n", pwm_polarity_strings[state.polarity]);
> }
>
> static ssize_t polarity_store(struct device *child,
> @@ -180,20 +176,16 @@ static ssize_t polarity_store(struct device *child,
> {
> struct pwm_export *export = child_to_pwm_export(child);
> struct pwm_device *pwm = export->pwm;
> - enum pwm_polarity polarity;
> struct pwm_state state;
> int ret;
>
> - if (sysfs_streq(buf, "normal"))
> - polarity = PWM_POLARITY_NORMAL;
> - else if (sysfs_streq(buf, "inversed"))
> - polarity = PWM_POLARITY_INVERSED;
> - else
> - return -EINVAL;
> + ret = sysfs_match_string(pwm_polarity_strings, buf);
> + if (ret < 0)
> + return ret;
>
> mutex_lock(&export->lock);
> pwm_get_state(pwm, &state);
> - state.polarity = polarity;
> + state.polarity = ret;
> ret = pwm_apply_state(pwm, &state);
> mutex_unlock(&export->lock);
>
On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> On Sunday, August 28, 2022, Joe Perches <[email protected]> wrote:
>
> > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > Code is smaller and looks nicer if we combine polarity strings
> > > into an array.
>
> First of all, please remove unnecessary context when replying.
I am _very_ aware of context.
I specifically left the code in.
> > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > are now required to be 0 and 1. As the only 2 values in
> > an enum they are, but that's not really guaranteed unless
> > you read the enum definition.
>
> So, what do you suggest here and in many other similar places (yes, ABI
> implied) in the kernel?
Leaving the code alone.
On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <[email protected]> wrote:
> On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > On Sunday, August 28, 2022, Joe Perches <[email protected]> wrote:
> > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > Code is smaller and looks nicer if we combine polarity strings
> > > > into an array.
> > First of all, please remove unnecessary context when replying.
>
> I am _very_ aware of context.
> I specifically left the code in.
>
> > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > are now required to be 0 and 1. As the only 2 values in
> > > an enum they are, but that's not really guaranteed unless
> > > you read the enum definition.
> >
> > So, what do you suggest here and in many other similar places (yes, ABI
> > implied) in the kernel?
>
> Leaving the code alone.
It's good that PWM maintainers look at this differently.
--
With Best Regards,
Andy Shevchenko
On Sun, 2022-08-28 at 20:40 +0300, Andy Shevchenko wrote:
> On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <[email protected]> wrote:
> > On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > > On Sunday, August 28, 2022, Joe Perches <[email protected]> wrote:
> > > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > > Code is smaller and looks nicer if we combine polarity strings
> > > > > into an array.
>
> > > First of all, please remove unnecessary context when replying.
> >
> > I am _very_ aware of context.
> > I specifically left the code in.
> >
> > > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > > are now required to be 0 and 1. As the only 2 values in
> > > > an enum they are, but that's not really guaranteed unless
> > > > you read the enum definition.
> > >
> > > So, what do you suggest here and in many other similar places (yes, ABI
> > > implied) in the kernel?
> >
> > Leaving the code alone.
>
> It's good that PWM maintainers look at this differently.
The enum is not userspace so it's not ABI.
The PWM maintainers are free to do what they want but I
prefer obviousness over compactness.
On Sun, Aug 28, 2022 at 9:19 PM Joe Perches <[email protected]> wrote:
> On Sun, 2022-08-28 at 20:40 +0300, Andy Shevchenko wrote:
> > On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <[email protected]> wrote:
> > > On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > > > On Sunday, August 28, 2022, Joe Perches <[email protected]> wrote:
> > > > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > > > Code is smaller and looks nicer if we combine polarity strings
> > > > > > into an array.
> >
> > > > First of all, please remove unnecessary context when replying.
> > >
> > > I am _very_ aware of context.
> > > I specifically left the code in.
> > >
> > > > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > > > are now required to be 0 and 1. As the only 2 values in
> > > > > an enum they are, but that's not really guaranteed unless
> > > > > you read the enum definition.
> > > >
> > > > So, what do you suggest here and in many other similar places (yes, ABI
> > > > implied) in the kernel?
> > >
> > > Leaving the code alone.
> >
> > It's good that PWM maintainers look at this differently.
>
> The enum is not userspace so it's not ABI.
>
> The PWM maintainers are free to do what they want but I
> prefer obviousness over compactness.
Why do you not start "fixing" other similar places in the kernel?
--
With Best Regards,
Andy Shevchenko
On Sun, Aug 28, 2022 at 02:19:22PM -0400, Joe Perches wrote:
> On Sun, 2022-08-28 at 20:40 +0300, Andy Shevchenko wrote:
> > On Sun, Aug 28, 2022 at 4:46 PM Joe Perches <[email protected]> wrote:
> > > On Sun, 2022-08-28 at 09:40 +0300, Andy Shevchenko wrote:
> > > > On Sunday, August 28, 2022, Joe Perches <[email protected]> wrote:
> > > > > On Fri, 2022-08-26 at 20:07 +0300, Andy Shevchenko wrote:
> > > > > > Code is smaller and looks nicer if we combine polarity strings
> > > > > > into an array.
> >
> > > > First of all, please remove unnecessary context when replying.
> > >
> > > I am _very_ aware of context.
> > > I specifically left the code in.
> > >
> > > > > It's less robust though as PWM_POLARITY_NORMAL and _INVERSED
> > > > > are now required to be 0 and 1. As the only 2 values in
> > > > > an enum they are, but that's not really guaranteed unless
> > > > > you read the enum definition.
> > > >
> > > > So, what do you suggest here and in many other similar places (yes, ABI
> > > > implied) in the kernel?
> > >
> > > Leaving the code alone.
> >
> > It's good that PWM maintainers look at this differently.
>
> The enum is not userspace so it's not ABI.
>
> The PWM maintainers are free to do what they want but I
> prefer obviousness over compactness.
I do agree with Joe, I don't see any benefit in this.
Thierry