| Message ID | 20201019193353.13066-1-trix@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: bd70528: remove unneeded break | expand |
Thanks Tom, On Mon, 2020-10-19 at 12:33 -0700, trix@redhat.com wrote: > From: Tom Rix <trix@redhat.com> > > A break is not needed if it is preceded by a return > > Signed-off-by: Tom Rix <trix@redhat.com> > --- > drivers/gpio/gpio-bd70528.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio- > bd70528.c > index 45b3da8da336..931e5765fe92 100644 > --- a/drivers/gpio/gpio-bd70528.c > +++ b/drivers/gpio/gpio-bd70528.c > @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct > gpio_chip *chip, unsigned int offset, > GPIO_OUT_REG(offset), > BD70528_GPIO_DRIVE_MASK, > BD70528_GPIO_OPEN_DRAIN); > - break; My personal taste is also to omit these breaks but I am pretty sure I saw some tooling issuing a warning about falling through the switch- case back when I wrote this. Most probably checkpatch didn't like that back then. Anyways - if you have no warnings from any of the tools - this indeed looks better (to me) without the break :) > case PIN_CONFIG_DRIVE_PUSH_PULL: > return regmap_update_bits(bdgpio->chip.regmap, > GPIO_OUT_REG(offset), > BD70528_GPIO_DRIVE_MASK, > BD70528_GPIO_PUSH_PULL); > - break; > case PIN_CONFIG_INPUT_DEBOUNCE: > return bd70528_set_debounce(bdgpio, offset, > pinconf_to_config_argument( > config)); > - break; > default: Actually - my personal taste would be to also get rid of the empty default here - but I guess it was also added to make some tool happy... > break; > } Acked-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
On Tue, Oct 20, 2020 at 2:26 PM Vaittinen, Matti <Matti.Vaittinen@fi.rohmeurope.com> wrote: > On Mon, 2020-10-19 at 12:33 -0700, trix@redhat.com wrote: > > - break; > My personal taste is also to omit these breaks but I am pretty sure I > saw some tooling issuing a warning about falling through the switch- > case back when I wrote this. Most probably checkpatch didn't like that > back then. Anyways - if you have no warnings from any of the tools - > this indeed looks better (to me) without the break :) JFYI: it's a clang which actually *is* complaining for an extra break.
On Tue, 2020-10-20 at 13:07 +0300, Matti Vaittinen wrote: > Thanks Tom, > > On Mon, 2020-10-19 at 12:33 -0700, trix@redhat.com wrote: > > From: Tom Rix <trix@redhat.com> > > > > A break is not needed if it is preceded by a return > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > --- > > drivers/gpio/gpio-bd70528.c | 3 --- > > 1 file changed, 3 deletions(-) > > > > diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio- > > bd70528.c > > index 45b3da8da336..931e5765fe92 100644 > > --- a/drivers/gpio/gpio-bd70528.c > > +++ b/drivers/gpio/gpio-bd70528.c > > @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct > > gpio_chip *chip, unsigned int offset, > > GPIO_OUT_REG(offset), > > BD70528_GPIO_DRIVE_MASK, > > BD70528_GPIO_OPEN_DRAIN); > > - break; > My personal taste is also to omit these breaks but I am pretty sure I > saw some tooling issuing a warning about falling through the switch- > case back when I wrote this. Most probably checkpatch didn't like > that > back then. I did a test and removed the breaks. Then I copied the modified file to drivers/gpio/dummy.c Next I committed this dummy.c in git, ran git-format-patch -s and finally ran the checkpatch on this... Following was produced: [mvaittin@localhost linux]$ scripts/checkpatch.pl 0001-gpio-add- dummy.patch Traceback (most recent call last): File "scripts/spdxcheck.py", line 6, in <module> from ply import lex, yacc ImportError: No module named ply WARNING: added, moved or deleted file(s), does MAINTAINERS need updating? #15: new file mode 100644 WARNING: Possible switch case/default not preceded by break or fallthrough comment #91: FILE: drivers/gpio/dummy.c:72: + case PIN_CONFIG_DRIVE_PUSH_PULL: WARNING: Possible switch case/default not preceded by break or fallthrough comment #96: FILE: drivers/gpio/dummy.c:77: + case PIN_CONFIG_INPUT_DEBOUNCE: total: 0 errors, 3 warnings, 229 lines checked NOTE: For some of the reported defects, checkpatch may be able to mechanically convert to the typical style using --fix or --fix- inplace. 0001-gpio-add-dummy.patch has style problems, please review. NOTE: If any of the errors are false positives, please report them to the maintainer, see CHECKPATCH in MAINTAINERS. I guess that explains the odd "fallthrough" comments you mentioned in another email. I guess the checkpatch should be fixed before you put too much effort in clean-up... And for peeps who have not been following - following function triggers the checkpatch error above: static int bd70528_gpio_set_config(struct gpio_chip *chip, unsigned int offset, unsigned long config) { struct bd70528_gpio *bdgpio = gpiochip_get_data(chip); switch (pinconf_to_config_param(config)) { case PIN_CONFIG_DRIVE_OPEN_DRAIN: return regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset), BD70528_GPIO_DRIVE_MASK, BD70528_GPIO_OPEN_DRAIN); case PIN_CONFIG_DRIVE_PUSH_PULL: return regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset), BD70528_GPIO_DRIVE_MASK, BD70528_GPIO_PUSH_PULL); case PIN_CONFIG_INPUT_DEBOUNCE: return bd70528_set_debounce(bdgpio, offset, pinconf_to_config_argument( config)); default: break; } return -ENOTSUPP; } Best Regards Matti Vaittinen
On Tue, 2020-10-20 at 14:46 +0300, Andy Shevchenko wrote: > On Tue, Oct 20, 2020 at 2:26 PM Vaittinen, Matti > <Matti.Vaittinen@fi.rohmeurope.com> wrote: > > On Mon, 2020-10-19 at 12:33 -0700, trix@redhat.com wrote: > > > - break; > > My personal taste is also to omit these breaks but I am pretty sure > > I > > saw some tooling issuing a warning about falling through the > > switch- > > case back when I wrote this. Most probably checkpatch didn't like > > that > > back then. Anyways - if you have no warnings from any of the tools > > - > > this indeed looks better (to me) without the break :) > > JFYI: it's a clang which actually *is* complaining for an extra > break. > Oh. I just replied before seeing this. So actually, checkpatch complains about missing break and clang about existing break. I'm getting much more nagging at work than at home! Best Regards Matti
On Tue, 2020-10-20 at 11:48 +0000, Vaittinen, Matti wrote: > On Tue, 2020-10-20 at 13:07 +0300, Matti Vaittinen wrote: > > Thanks Tom, > > > > On Mon, 2020-10-19 at 12:33 -0700, trix@redhat.com wrote: > > > From: Tom Rix <trix@redhat.com> > > > > > > A break is not needed if it is preceded by a return > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > --- > > > drivers/gpio/gpio-bd70528.c | 3 --- > > > 1 file changed, 3 deletions(-) > > > > > > diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio- > > > bd70528.c > > > index 45b3da8da336..931e5765fe92 100644 > > > --- a/drivers/gpio/gpio-bd70528.c > > > +++ b/drivers/gpio/gpio-bd70528.c > > > @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct > > > gpio_chip *chip, unsigned int offset, > > > GPIO_OUT_REG(offset), > > > BD70528_GPIO_DRIVE_MASK, > > > BD70528_GPIO_OPEN_DRAIN); > > > - break; > > My personal taste is also to omit these breaks but I am pretty sure I > > saw some tooling issuing a warning about falling through the switch- > > case back when I wrote this. Most probably checkpatch didn't like > > that > > back then. > > I did a test and removed the breaks. Then I copied the modified file to > drivers/gpio/dummy.c > Next I committed this dummy.c in git, ran git-format-patch -s and > finally ran the checkpatch on this... Following was produced: > > > [mvaittin@localhost linux]$ scripts/checkpatch.pl 0001-gpio-add- > dummy.patch > Traceback (most recent call last): > File "scripts/spdxcheck.py", line 6, in <module> > from ply import lex, yacc > ImportError: No module named ply > WARNING: added, moved or deleted file(s), does MAINTAINERS need > updating? > #15: > new file mode 100644 > > WARNING: Possible switch case/default not preceded by break or > fallthrough comment > #91: FILE: drivers/gpio/dummy.c:72: > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > WARNING: Possible switch case/default not preceded by break or > fallthrough comment > #96: FILE: drivers/gpio/dummy.c:77: > + case PIN_CONFIG_INPUT_DEBOUNCE: > > total: 0 errors, 3 warnings, 229 lines checked > > NOTE: For some of the reported defects, checkpatch may be able to > mechanically convert to the typical style using --fix or --fix- > inplace. > > 0001-gpio-add-dummy.patch has style problems, please review. > > NOTE: If any of the errors are false positives, please report > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > I guess that explains the odd "fallthrough" comments you mentioned in > another email. I guess the checkpatch should be fixed before you put > too much effort in clean-up... > > > And for peeps who have not been following - following function triggers > the checkpatch error above: Huh? what version of checkpatch are you using? Send it to me please. > static int bd70528_gpio_set_config(struct gpio_chip *chip, unsigned int > offset, > unsigned long config) > { > struct bd70528_gpio *bdgpio = gpiochip_get_data(chip); > > switch (pinconf_to_config_param(config)) { > case PIN_CONFIG_DRIVE_OPEN_DRAIN: > return regmap_update_bits(bdgpio->chip.regmap, > GPIO_OUT_REG(offset), > BD70528_GPIO_DRIVE_MASK, > BD70528_GPIO_OPEN_DRAIN); > case PIN_CONFIG_DRIVE_PUSH_PULL: > return regmap_update_bits(bdgpio->chip.regmap, > GPIO_OUT_REG(offset), > BD70528_GPIO_DRIVE_MASK, > BD70528_GPIO_PUSH_PULL); > case PIN_CONFIG_INPUT_DEBOUNCE: > return bd70528_set_debounce(bdgpio, offset, > pinconf_to_config_argument( > config)); > default: > break; > } > return -ENOTSUPP; > } > > > Best Regards > Matti Vaittinen >
Hello Joe & All, On Tue, 2020-10-20 at 11:36 -0700, Joe Perches wrote: > On Tue, 2020-10-20 at 11:48 +0000, Vaittinen, Matti wrote: > > On Tue, 2020-10-20 at 13:07 +0300, Matti Vaittinen wrote: > > > Thanks Tom, > > > > > > On Mon, 2020-10-19 at 12:33 -0700, trix@redhat.com wrote: > > > > From: Tom Rix <trix@redhat.com> > > > > > > > > A break is not needed if it is preceded by a return > > > > > > > > Signed-off-by: Tom Rix <trix@redhat.com> > > > > --- > > > > drivers/gpio/gpio-bd70528.c | 3 --- > > > > 1 file changed, 3 deletions(-) > > > > > > > > diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio- > > > > bd70528.c > > > > index 45b3da8da336..931e5765fe92 100644 > > > > --- a/drivers/gpio/gpio-bd70528.c > > > > +++ b/drivers/gpio/gpio-bd70528.c > > > > @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct > > > > gpio_chip *chip, unsigned int offset, > > > > GPIO_OUT_REG(offset), > > > > BD70528_GPIO_DRIVE_MA > > > > SK, > > > > BD70528_GPIO_OPEN_DRA > > > > IN); > > > > - break; > > > My personal taste is also to omit these breaks but I am pretty > > > sure I > > > saw some tooling issuing a warning about falling through the > > > switch- > > > case back when I wrote this. Most probably checkpatch didn't like > > > that > > > back then. > > > > I did a test and removed the breaks. Then I copied the modified > > file to > > drivers/gpio/dummy.c > > Next I committed this dummy.c in git, ran git-format-patch -s and > > finally ran the checkpatch on this... Following was produced: > > > > > > [mvaittin@localhost linux]$ scripts/checkpatch.pl 0001-gpio-add- > > dummy.patch > > Traceback (most recent call last): > > File "scripts/spdxcheck.py", line 6, in <module> > > from ply import lex, yacc > > ImportError: No module named ply > > WARNING: added, moved or deleted file(s), does MAINTAINERS need > > updating? > > #15: > > new file mode 100644 > > > > WARNING: Possible switch case/default not preceded by break or > > fallthrough comment > > #91: FILE: drivers/gpio/dummy.c:72: > > + case PIN_CONFIG_DRIVE_PUSH_PULL: > > > > WARNING: Possible switch case/default not preceded by break or > > fallthrough comment > > #96: FILE: drivers/gpio/dummy.c:77: > > + case PIN_CONFIG_INPUT_DEBOUNCE: > > > > total: 0 errors, 3 warnings, 229 lines checked > > > > NOTE: For some of the reported defects, checkpatch may be able to > > mechanically convert to the typical style using --fix or -- > > fix- > > inplace. > > > > 0001-gpio-add-dummy.patch has style problems, please review. > > > > NOTE: If any of the errors are false positives, please report > > them to the maintainer, see CHECKPATCH in MAINTAINERS. > > > > I guess that explains the odd "fallthrough" comments you mentioned > > in > > another email. I guess the checkpatch should be fixed before you > > put > > too much effort in clean-up... > > > > > > And for peeps who have not been following - following function > > triggers > > the checkpatch error above: > > Huh? what version of checkpatch are you using? > Send it to me please. I was actually accidentally running the checkpatch from stable release 5.4 yesterday. I did today run it from my development repo which currently was based on regulator-v5.8. I didn't test the latest versions. Please find my version of checkpatch and the patch to trigger the warning attached. > > > static int bd70528_gpio_set_config(struct gpio_chip *chip, unsigned > > int > > offset, > > unsigned long config) > > { > > struct bd70528_gpio *bdgpio = gpiochip_get_data(chip); > > > > switch (pinconf_to_config_param(config)) { > > case PIN_CONFIG_DRIVE_OPEN_DRAIN: > > return regmap_update_bits(bdgpio->chip.regmap, > > GPIO_OUT_REG(offset), > > BD70528_GPIO_DRIVE_MASK, > > BD70528_GPIO_OPEN_DRAIN); > > case PIN_CONFIG_DRIVE_PUSH_PULL: > > return regmap_update_bits(bdgpio->chip.regmap, > > GPIO_OUT_REG(offset), > > BD70528_GPIO_DRIVE_MASK, > > BD70528_GPIO_PUSH_PULL); > > case PIN_CONFIG_INPUT_DEBOUNCE: > > return bd70528_set_debounce(bdgpio, offset, > > pinconf_to_config_argument( > > config)); > > default: > > break; > > } > > return -ENOTSUPP; > > } > > > > > > Best Regards > > Matti Vaittinen > >
On Wed, 2020-10-21 at 07:25 +0000, Vaittinen, Matti wrote: > Hello Joe & All, > On Tue, 2020-10-20 at 11:36 -0700, Joe Perches wrote: > > On Tue, 2020-10-20 at 11:48 +0000, Vaittinen, Matti wrote: [] > > > And for peeps who have not been following - following function > > > triggers the checkpatch error above: > > > > Huh? what version of checkpatch are you using? > > Send it to me please. [] > Please find my version of checkpatch and the patch to trigger the > warning attached. Thanks. This test wasn't particularly useful (and had some false positives) and was removed by commit ef3c005c0eb07a60949191bc6ee407d5f43cc502 Author: Joe Perches <joe@perches.com> Date: Tue Aug 11 18:35:19 2020 -0700 checkpatch: remove missing switch/case break test This test doesn't work well and newer compilers are much better at emitting this warning. Signed-off-by: Joe Perches <joe@perches.com> Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Cc: Cambda Zhu <cambda@linux.alibaba.com> Link: http://lkml.kernel.org/r/7e25090c79f6a69d502ab8219863300790192fe2.camel@perches.com Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
Hello, On Wed, 2020-10-21 at 07:39 -0700, Joe Perches wrote: > On Wed, 2020-10-21 at 07:25 +0000, Vaittinen, Matti wrote: > > Hello Joe & All, > > On Tue, 2020-10-20 at 11:36 -0700, Joe Perches wrote: > > > On Tue, 2020-10-20 at 11:48 +0000, Vaittinen, Matti wrote: > [] > > > > And for peeps who have not been following - following function > > > > triggers the checkpatch error above: > > > > > > Huh? what version of checkpatch are you using? > > > Send it to me please. > [] > > Please find my version of checkpatch and the patch to trigger the > > warning attached. > > Thanks. This test wasn't particularly useful > (and had some false positives) and was removed by > > commit ef3c005c0eb07a60949191bc6ee407d5f43cc502 > Author: Joe Perches <joe@perches.com> > Date: Tue Aug 11 18:35:19 2020 -0700 > > checkpatch: remove missing switch/case break test > > This test doesn't work well and newer compilers are much better > at emitting this warning. > > Signed-off-by: Joe Perches <joe@perches.com> > Signed-off-by: Andrew Morton <akpm@linux-foundation.org> > Cc: Cambda Zhu <cambda@linux.alibaba.com> > Link: > http://lkml.kernel.org/r/7e25090c79f6a69d502ab8219863300790192fe2.camel@perches.com > Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org> > Thanks for checking this Joe! And note to self - always check with newest kernel... ;) (Sorry for bothering) Br, Matti Vaittinen
diff --git a/drivers/gpio/gpio-bd70528.c b/drivers/gpio/gpio-bd70528.c index 45b3da8da336..931e5765fe92 100644 --- a/drivers/gpio/gpio-bd70528.c +++ b/drivers/gpio/gpio-bd70528.c @@ -71,17 +71,14 @@ static int bd70528_gpio_set_config(struct gpio_chip *chip, unsigned int offset, GPIO_OUT_REG(offset), BD70528_GPIO_DRIVE_MASK, BD70528_GPIO_OPEN_DRAIN); - break; case PIN_CONFIG_DRIVE_PUSH_PULL: return regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset), BD70528_GPIO_DRIVE_MASK, BD70528_GPIO_PUSH_PULL); - break; case PIN_CONFIG_INPUT_DEBOUNCE: return bd70528_set_debounce(bdgpio, offset, pinconf_to_config_argument(config)); - break; default: break; }