Mailing List Archive: [PATCH v1 1/1] leds: sun50i-a100: Use match

[PATCH v1 1/1] leds: sun50i-a100: Use match_string() helper to simplify the code

Apr 26, 2024, 8:25 AM

Post #1 of 5 (2 views)

match_string() returns the array index of a matching string.
Use it instead of the open-coded implementation.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
drivers/leds/leds-sun50i-a100.c | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/leds/leds-sun50i-a100.c b/drivers/leds/leds-sun50i-a100.c
index 62d21c3a3575..119eff9471f0 100644
--- a/drivers/leds/leds-sun50i-a100.c
+++ b/drivers/leds/leds-sun50i-a100.c
@@ -252,18 +252,16 @@ static int sun50i_a100_ledc_parse_format(struct device *dev,
struct sun50i_a100_ledc *priv)
{
const char *format = "grb";
- u32 i;
+ int i;

device_property_read_string(dev, "allwinner,pixel-format", &format);

- for (i = 0; i < ARRAY_SIZE(sun50i_a100_ledc_formats); i++) {
- if (!strcmp(format, sun50i_a100_ledc_formats[i])) {
- priv->format = i;
- return 0;
- }
- }
+ i = match_string(sun50i_a100_ledc_formats, ARRAY_SIZE(sun50i_a100_ledc_formats), format);
+ if (i < 0)
+ return dev_err_probe(dev, i, "Bad pixel format '%s'\n", format);

- return dev_err_probe(dev, -EINVAL, "Bad pixel format '%s'\n", format);
+ priv->format = i;
+ return 0;
}

static void sun50i_a100_ledc_set_format(struct sun50i_a100_ledc *priv)
--
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/1] leds: sun50i-a100: Use match_string() helper to simplify the code [ In reply to ]

jernej.skrabec at gmail

Apr 26, 2024, 8:37 AM

Post #2 of 5 (2 views)

Permalink

Dne petek, 26. april 2024 ob 17:25:15 GMT +2 je Andy Shevchenko napisal(a):
> match_string() returns the array index of a matching string.
> Use it instead of the open-coded implementation.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/leds/leds-sun50i-a100.c | 14 ++++++--------
> 1 file changed, 6 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/leds/leds-sun50i-a100.c b/drivers/leds/leds-sun50i-a100.c
> index 62d21c3a3575..119eff9471f0 100644
> --- a/drivers/leds/leds-sun50i-a100.c
> +++ b/drivers/leds/leds-sun50i-a100.c
> @@ -252,18 +252,16 @@ static int sun50i_a100_ledc_parse_format(struct device *dev,
> struct sun50i_a100_ledc *priv)
> {
> const char *format = "grb";
> - u32 i;
> + int i;
>
> device_property_read_string(dev, "allwinner,pixel-format", &format);
>
> - for (i = 0; i < ARRAY_SIZE(sun50i_a100_ledc_formats); i++) {
> - if (!strcmp(format, sun50i_a100_ledc_formats[i])) {
> - priv->format = i;
> - return 0;
> - }
> - }
> + i = match_string(sun50i_a100_ledc_formats, ARRAY_SIZE(sun50i_a100_ledc_formats), format);
> + if (i < 0)
> + return dev_err_probe(dev, i, "Bad pixel format '%s'\n", format);

I know that old code used dev_err_probe() without reason, but could you change
it to ordinary dev_err()? dev_err_probe() is useful only when return code could
be -EPROBE_DEFER. This is clearly not the case here.

Best regards,
Jernej

>
> - return dev_err_probe(dev, -EINVAL, "Bad pixel format '%s'\n", format);
> + priv->format = i;
> + return 0;
> }
>
> static void sun50i_a100_ledc_set_format(struct sun50i_a100_ledc *priv)
>

Re: [PATCH v1 1/1] leds: sun50i-a100: Use match_string() helper to simplify the code [ In reply to ]

andriy.shevchenko at linux

Apr 26, 2024, 8:45 AM

Post #3 of 5 (2 views)

Permalink

On Fri, Apr 26, 2024 at 05:37:42PM +0200, Jernej Škrabec wrote:
> Dne petek, 26. april 2024 ob 17:25:15 GMT +2 je Andy Shevchenko napisal(a):

..

> > + return dev_err_probe(dev, i, "Bad pixel format '%s'\n", format);
>
> I know that old code used dev_err_probe() without reason, but could you change
> it to ordinary dev_err()?

First of all, it's out of scope of _this_ patch.

> dev_err_probe() is useful only when return code could be -EPROBE_DEFER.

This is simply not true. We are trying to have a uniform output in ->probe()
and even documentation for dev_err_probe() was changed long time ago to
encourage using it for non deferred probe cases.

> This is clearly not the case here.

Is it a problem?

--
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/1] leds: sun50i-a100: Use match_string() helper to simplify the code [ In reply to ]

jernej.skrabec at gmail

Apr 26, 2024, 8:55 AM

Post #4 of 5 (2 views)

Permalink

Dne petek, 26. april 2024 ob 17:45:14 GMT +2 je Andy Shevchenko napisal(a):
> On Fri, Apr 26, 2024 at 05:37:42PM +0200, Jernej Škrabec wrote:
> > Dne petek, 26. april 2024 ob 17:25:15 GMT +2 je Andy Shevchenko napisal(a):
>
> ...
>
> > > + return dev_err_probe(dev, i, "Bad pixel format '%s'\n", format);
> >
> > I know that old code used dev_err_probe() without reason, but could you change
> > it to ordinary dev_err()?
>
> First of all, it's out of scope of _this_ patch.
>
> > dev_err_probe() is useful only when return code could be -EPROBE_DEFER.
>
> This is simply not true. We are trying to have a uniform output in ->probe()
> and even documentation for dev_err_probe() was changed long time ago to
> encourage using it for non deferred probe cases.
>
> > This is clearly not the case here.
>
> Is it a problem?

Sorry, I missed added note for non -EPROBE_DEFER cases.

Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Best regards,
Jernej

Re: [PATCH v1 1/1] leds: sun50i-a100: Use match_string() helper to simplify the code [ In reply to ]

andriy.shevchenko at linux

Apr 26, 2024, 9:33 AM

Post #5 of 5 (2 views)

Permalink

On Fri, Apr 26, 2024 at 05:55:22PM +0200, Jernej Škrabec wrote:
> Dne petek, 26. april 2024 ob 17:45:14 GMT +2 je Andy Shevchenko napisal(a):
> > On Fri, Apr 26, 2024 at 05:37:42PM +0200, Jernej Škrabec wrote:
> > > Dne petek, 26. april 2024 ob 17:25:15 GMT +2 je Andy Shevchenko napisal(a):

..

> > > > + return dev_err_probe(dev, i, "Bad pixel format '%s'\n", format);
> > >
> > > I know that old code used dev_err_probe() without reason, but could you change
> > > it to ordinary dev_err()?
> >
> > First of all, it's out of scope of _this_ patch.
> >
> > > dev_err_probe() is useful only when return code could be -EPROBE_DEFER.
> >
> > This is simply not true. We are trying to have a uniform output in ->probe()
> > and even documentation for dev_err_probe() was changed long time ago to
> > encourage using it for non deferred probe cases.
> >
> > > This is clearly not the case here.
> >
> > Is it a problem?
>
> Sorry, I missed added note for non -EPROBE_DEFER cases.

No problem.

> Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com>

Thank you for the review!

--
With Best Regards,
Andy Shevchenko