Re: [PATCH v1 05/10] gpio: pca953x: Simplify code with cleanup helpers

[Bartosz Golaszewski]

On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@xxxxxxxxxxxxxx> wrote:
>
>         Hi Andy,
>
> On Fri, 1 Sep 2023, Andy Shevchenko wrote:
> > Use macros defined in linux/cleanup.h to automate resource lifetime
> > control in gpio-pca953x.
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
>
> Thanks for your patch, which is now commit 8e471b784a720f6f
> ("gpio: pca953x: Simplify code with cleanup helpers") in
> gpio/gpio/for-next.
>
> > --- a/drivers/gpio/gpio-pca953x.c
> > +++ b/drivers/gpio/gpio-pca953x.c
> > @@ -557,9 +554,8 @@ static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off)
> >       u32 reg_val;
> >       int ret;
> >
> > -     mutex_lock(&chip->i2c_lock);
> > -     ret = regmap_read(chip->regmap, inreg, &reg_val);
> > -     mutex_unlock(&chip->i2c_lock);
> > +     scoped_guard(mutex, &chip->i2c_lock)
> > +             ret = regmap_read(chip->regmap, inreg, &reg_val);
>
> I can't say I'm thrilled about the lack of curly braces.  I was also
> surprised to discover that checkpatch nor gcc W=1 complain about the
> indentation change.
> I know we don't use curly braces in single-statement for_each_*() loops,
> but at least these have the familiar "for"-prefix.  And having the scope
> is very important here, so using braces, this would stand out more.
>
> Hence can we please get curly braces, like
>
>      scoped_guard(mutex, &chip->i2c_lock) {
>             ret = regmap_read(chip->regmap, inreg, &reg_val);
>      }
>
> ?
>
> Thanks! ;-)

I strongly disagree. The scope here is very clear - just like it is in
a for loop, in a while loop or in an if block:

if (foo)
    bar()

if (foo) {
    bar();
    baz();
}

Only compound statements need curly braces in the kernel and it has
been like this forever. I don't really see a need to make it an
exception.

That being said - I don't think the coding style for guard has ever
been addressed yet, so maybe bring it up with Peter Zijlstra?

Bart

>
> Gr{oetje,eeting}s,
>
>                                                 Geert
>
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@xxxxxxxxxxxxxx
>
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                                             -- Linus Torvalds