| Message ID | 20230901134041.1165562-5-andriy.shevchenko@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | 8e471b784a720f6f34f9fb449ba0744359dcaccb |
| Headers | show |
| Series | [v1,01/10] gpio: pca953x: Drop unused fields in struct pca953x_platform_data | expand |
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@linux.intel.com> 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, ®_val); > - mutex_unlock(&chip->i2c_lock); > + scoped_guard(mutex, &chip->i2c_lock) > + ret = regmap_read(chip->regmap, inreg, ®_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, ®_val); } ? Thanks! ;-) Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org 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
On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> 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@linux.intel.com> > > 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, ®_val); > > - mutex_unlock(&chip->i2c_lock); > > + scoped_guard(mutex, &chip->i2c_lock) > > + ret = regmap_read(chip->regmap, inreg, ®_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, ®_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@linux-m68k.org > > 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
Hi Bartosz, On Wed, Sep 13, 2023 at 5:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > 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@linux.intel.com> > > > > 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, ®_val); > > > - mutex_unlock(&chip->i2c_lock); > > > + scoped_guard(mutex, &chip->i2c_lock) > > > + ret = regmap_read(chip->regmap, inreg, ®_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, ®_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? That's a good idea! I see Peter always used curly braces (but he didn't have any single-statement blocks, except for one with an "if", and we do tend to use curly braces in "for"-statements containing a single "if", too), but he does put a space after the "scoped_guard", as is also shown in the template in include/linux/cleanup.h: scoped_guard (name, args...) { }: Then, "guard" does not get a space (but it is funny syntax anyway, with the double set of parentheses ;-). The template in include/linux/cleanup.h doesn't match actual usage as it lacks the second set of parentheses: guard(name): Peter: care to comment? Or do you have a different bikeshed to paint today? ;-) Thanks! Gr{oetje,eeting}s, Geert
Hey all, A brief disclaimer, I'm a fairly new kernel contributor, but since I was cc'd directly, I figured I might as well drop into the conversation. On Thu, Sep 14, 2023 at 12:47 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > Hi Bartosz, > > On Wed, Sep 13, 2023 at 5:27 PM Bartosz Golaszewski <brgl@bgdev.pl> wrote: > > On Wed, Sep 13, 2023 at 4:35 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > 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@linux.intel.com> > > > > > > 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, ®_val); > > > > - mutex_unlock(&chip->i2c_lock); > > > > + scoped_guard(mutex, &chip->i2c_lock) > > > > + ret = regmap_read(chip->regmap, inreg, ®_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, ®_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. The more I think on this issue, the more I go back and forth. If we only had guard(...), the only way to approximate scoped guard would be to either just do what the macro does (i.e., a dummy for loop that only runs once) or use an anonymous scope, e.g., { guard(...); my_one_statement(); } Since this is how I've previously used std::lock_guard in C++, this pattern feels very familiar to me, and the scoped_guard feels almost like syntax sugar for this. As such, I feel like including the braces is most natural because, as Geert mentioned, it emphasizes the scope that "should" (in my brain, at least) be there. Thanks, Mitchell > > 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? > > That's a good idea! > > I see Peter always used curly braces (but he didn't have any > single-statement blocks, except for one with an "if", and we do tend > to use curly braces in "for"-statements containing a single "if", too), > but he does put a space after the "scoped_guard", as is also > shown in the template in include/linux/cleanup.h: > > scoped_guard (name, args...) { }: > > Then, "guard" does not get a space (but it is funny syntax > anyway, with the double set of parentheses ;-). The template in > include/linux/cleanup.h doesn't match actual usage as it lacks the > second set of parentheses: > > guard(name): > > Peter: care to comment? > Or do you have a different bikeshed to paint today? ;-) > > Thanks! > > Gr{oetje,eeting}s, > > Geert > > -- > Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org > > 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
On Thu, Sep 14, 2023 at 09:47:07AM +0200, Geert Uytterhoeven wrote: > > 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. Kernel coding style is a little different from what C demands. Specifically, kernel style demands { } for anything multi-line. Specifically: for (;;) { /* a comment */ foo(); } or for (;;) { foo(a, very, long, arg, chain); } do both warrant a pile of curlies in kernel style where C does not demand it. > > 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? > > That's a good idea! > > I see Peter always used curly braces (but he didn't have any > single-statement blocks, except for one with an "if", and we do tend > to use curly braces in "for"-statements containing a single "if", too), > but he does put a space after the "scoped_guard", as is also > shown in the template in include/linux/cleanup.h: > > scoped_guard (name, args...) { }: > Right, per scope_guard being a for loop I added the extra space. Our coding style does; if (cond) { } while (cond) { } for (;;) { } etc.. so I too did: scoped_guard (name, args...) { } > Then, "guard" does not get a space (but it is funny syntax > anyway, with the double set of parentheses ;-). The template in > include/linux/cleanup.h doesn't match actual usage as it lacks the > second set of parentheses: > > guard(name): > > Peter: care to comment? > Or do you have a different bikeshed to paint today? ;-) For guard I read the first pair as if it were a C++ template, that is, I pretend, it is actually written like: guard<name>(args..); Both are 'odd' in numerous ways and inconsistent vs where the 'args...' go, but alas, we're trying to wrangle this inside the constraints imposed upon us by C and CPP our dear pre-processor.
On Thu, Sep 14, 2023 at 01:51:01PM -0700, Mitchell Levy wrote: > The more I think on this issue, the more I go back and forth. If we > only had guard(...), the only way to approximate scoped guard would be > to either just do what the macro does (i.e., a dummy for loop that > only runs once) or use an anonymous scope, e.g., > { > guard(...); > my_one_statement(); > } > Since this is how I've previously used std::lock_guard in C++, this > pattern feels very familiar to me, and the scoped_guard feels almost > like syntax sugar for this. As such, I feel like including the braces > is most natural because, as Geert mentioned, it emphasizes the scope > that "should" (in my brain, at least) be there. AFAIC the anonymous scope thing doesn't much happen in kernel coding style -- although I'm sure it's there, the code-base is simply too vast to not have it *somewhere*.
On Thu, Sep 14, 2023 at 09:47:07AM +0200, Geert Uytterhoeven wrote: > > > > + scoped_guard(mutex, &chip->i2c_lock) > > > > + ret = regmap_read(chip->regmap, inreg, ®_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. So in my kernel/events/core.c changes (that I still need to re-post and aren't yet upstream) I have found a single instance where I've done the same lack of curlies: scoped_guard (rcu) pmu = idr_find(&pmu_idr, type); clearly self-consistency mandates I should not object to this style. That said, I see the argument for having them, they do more clearly demarcate the scope. As with everything, small variations in style are bound to happen from one maintainer to another... (or one self to itself over time).
On Fri, Sep 15, 2023 at 12:26:39AM +0200, Peter Zijlstra wrote: > On Thu, Sep 14, 2023 at 01:51:01PM -0700, Mitchell Levy wrote: > > > The more I think on this issue, the more I go back and forth. If we > > only had guard(...), the only way to approximate scoped guard would be > > to either just do what the macro does (i.e., a dummy for loop that > > only runs once) or use an anonymous scope, e.g., > > { > > guard(...); > > my_one_statement(); > > } > > Since this is how I've previously used std::lock_guard in C++, this > > pattern feels very familiar to me, and the scoped_guard feels almost > > like syntax sugar for this. As such, I feel like including the braces > > is most natural because, as Geert mentioned, it emphasizes the scope > > that "should" (in my brain, at least) be there. > > AFAIC the anonymous scope thing doesn't much happen in kernel coding > style -- although I'm sure it's there, the code-base is simply too vast > to not have it *somewhere*. The kernel typical style would be: do { ... } while (0) to create a 'pointless' scope. Apparently this is also what I've done in some conversions where a conditional lock was involved.
diff --git a/drivers/gpio/gpio-pca953x.c b/drivers/gpio/gpio-pca953x.c index 71d87d570a2b..99379ee98749 100644 --- a/drivers/gpio/gpio-pca953x.c +++ b/drivers/gpio/gpio-pca953x.c @@ -10,6 +10,7 @@ #include <linux/acpi.h> #include <linux/bitmap.h> +#include <linux/cleanup.h> #include <linux/gpio/consumer.h> #include <linux/gpio/driver.h> #include <linux/i2c.h> @@ -519,12 +520,10 @@ static int pca953x_gpio_direction_input(struct gpio_chip *gc, unsigned off) struct pca953x_chip *chip = gpiochip_get_data(gc); u8 dirreg = chip->recalc_addr(chip, chip->regs->direction, off); u8 bit = BIT(off % BANK_SZ); - int ret; - mutex_lock(&chip->i2c_lock); - ret = regmap_write_bits(chip->regmap, dirreg, bit, bit); - mutex_unlock(&chip->i2c_lock); - return ret; + guard(mutex)(&chip->i2c_lock); + + return regmap_write_bits(chip->regmap, dirreg, bit, bit); } static int pca953x_gpio_direction_output(struct gpio_chip *gc, @@ -536,17 +535,15 @@ static int pca953x_gpio_direction_output(struct gpio_chip *gc, u8 bit = BIT(off % BANK_SZ); int ret; - mutex_lock(&chip->i2c_lock); + guard(mutex)(&chip->i2c_lock); + /* set output level */ ret = regmap_write_bits(chip->regmap, outreg, bit, val ? bit : 0); if (ret) - goto exit; + return ret; /* then direction */ - ret = regmap_write_bits(chip->regmap, dirreg, bit, 0); -exit: - mutex_unlock(&chip->i2c_lock); - return ret; + return regmap_write_bits(chip->regmap, dirreg, bit, 0); } static int pca953x_gpio_get_value(struct gpio_chip *gc, unsigned off) @@ -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, ®_val); - mutex_unlock(&chip->i2c_lock); + scoped_guard(mutex, &chip->i2c_lock) + ret = regmap_read(chip->regmap, inreg, ®_val); if (ret < 0) return ret; @@ -572,9 +568,9 @@ static void pca953x_gpio_set_value(struct gpio_chip *gc, unsigned off, int val) u8 outreg = chip->recalc_addr(chip, chip->regs->output, off); u8 bit = BIT(off % BANK_SZ); - mutex_lock(&chip->i2c_lock); + guard(mutex)(&chip->i2c_lock); + regmap_write_bits(chip->regmap, outreg, bit, val ? bit : 0); - mutex_unlock(&chip->i2c_lock); } static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off) @@ -585,9 +581,8 @@ static int pca953x_gpio_get_direction(struct gpio_chip *gc, unsigned off) u32 reg_val; int ret; - mutex_lock(&chip->i2c_lock); - ret = regmap_read(chip->regmap, dirreg, ®_val); - mutex_unlock(&chip->i2c_lock); + scoped_guard(mutex, &chip->i2c_lock) + ret = regmap_read(chip->regmap, dirreg, ®_val); if (ret < 0) return ret; @@ -604,9 +599,8 @@ static int pca953x_gpio_get_multiple(struct gpio_chip *gc, DECLARE_BITMAP(reg_val, MAX_LINE); int ret; - mutex_lock(&chip->i2c_lock); - ret = pca953x_read_regs(chip, chip->regs->input, reg_val); - mutex_unlock(&chip->i2c_lock); + scoped_guard(mutex, &chip->i2c_lock) + ret = pca953x_read_regs(chip, chip->regs->input, reg_val); if (ret) return ret; @@ -621,16 +615,15 @@ static void pca953x_gpio_set_multiple(struct gpio_chip *gc, DECLARE_BITMAP(reg_val, MAX_LINE); int ret; - mutex_lock(&chip->i2c_lock); + guard(mutex)(&chip->i2c_lock); + ret = pca953x_read_regs(chip, chip->regs->output, reg_val); if (ret) - goto exit; + return; bitmap_replace(reg_val, reg_val, bits, mask, gc->ngpio); pca953x_write_regs(chip, chip->regs->output, reg_val); -exit: - mutex_unlock(&chip->i2c_lock); } static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip, @@ -638,7 +631,6 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip, unsigned long config) { enum pin_config_param param = pinconf_to_config_param(config); - u8 pull_en_reg = chip->recalc_addr(chip, PCAL953X_PULL_EN, offset); u8 pull_sel_reg = chip->recalc_addr(chip, PCAL953X_PULL_SEL, offset); u8 bit = BIT(offset % BANK_SZ); @@ -651,7 +643,7 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip, if (!(chip->driver_data & PCA_PCAL)) return -ENOTSUPP; - mutex_lock(&chip->i2c_lock); + guard(mutex)(&chip->i2c_lock); /* Configure pull-up/pull-down */ if (param == PIN_CONFIG_BIAS_PULL_UP) @@ -661,17 +653,13 @@ static int pca953x_gpio_set_pull_up_down(struct pca953x_chip *chip, else ret = 0; if (ret) - goto exit; + return ret; /* Disable/Enable pull-up/pull-down */ if (param == PIN_CONFIG_BIAS_DISABLE) - ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, 0); + return regmap_write_bits(chip->regmap, pull_en_reg, bit, 0); else - ret = regmap_write_bits(chip->regmap, pull_en_reg, bit, bit); - -exit: - mutex_unlock(&chip->i2c_lock); - return ret; + return regmap_write_bits(chip->regmap, pull_en_reg, bit, bit); } static int pca953x_gpio_set_config(struct gpio_chip *gc, unsigned int offset, @@ -900,10 +888,8 @@ static irqreturn_t pca953x_irq_handler(int irq, void *devid) bitmap_zero(pending, MAX_LINE); - mutex_lock(&chip->i2c_lock); - ret = pca953x_irq_pending(chip, pending); - mutex_unlock(&chip->i2c_lock); - + scoped_guard(mutex, &chip->i2c_lock) + ret = pca953x_irq_pending(chip, pending); if (ret) { ret = 0; @@ -1235,26 +1221,21 @@ static int pca953x_restore_context(struct pca953x_chip *chip) { int ret; - mutex_lock(&chip->i2c_lock); + guard(mutex)(&chip->i2c_lock); regcache_cache_only(chip->regmap, false); regcache_mark_dirty(chip->regmap); ret = pca953x_regcache_sync(chip); - if (ret) { - mutex_unlock(&chip->i2c_lock); + if (ret) return ret; - } - ret = regcache_sync(chip->regmap); - mutex_unlock(&chip->i2c_lock); - return ret; + return regcache_sync(chip->regmap); } static void pca953x_save_context(struct pca953x_chip *chip) { - mutex_lock(&chip->i2c_lock); + guard(mutex)(&chip->i2c_lock); regcache_cache_only(chip->regmap, true); - mutex_unlock(&chip->i2c_lock); } static int pca953x_suspend(struct device *dev)
Use macros defined in linux/cleanup.h to automate resource lifetime control in gpio-pca953x. Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> --- drivers/gpio/gpio-pca953x.c | 77 ++++++++++++++----------------------- 1 file changed, 29 insertions(+), 48 deletions(-)