Re: [PATCH] gpio: bd70528: remove unneeded break

["Vaittinen, Matti" <Matti.Vaittinen@fi.rohmeurope.com>]

[-- Attachment #1: Type: text/plain, Size: 4440 bytes --]

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
> > 


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-gpio-add-dummy.patch --]
[-- Type: text/x-patch; name="0001-gpio-add-dummy.patch", Size: 7230 bytes --]

From 5e70eba5d3b0e8e0505378400c417fb4060463c4 Mon Sep 17 00:00:00 2001
From: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
Date: Tue, 20 Oct 2020 14:35:16 +0300
Subject: [PATCH] gpio: add dummy

this description is likely to not make any sense.

Signed-off-by: Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>
---
 drivers/gpio/dummy.c | 229 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 229 insertions(+)
 create mode 100644 drivers/gpio/dummy.c

diff --git a/drivers/gpio/dummy.c b/drivers/gpio/dummy.c
new file mode 100644
index 000000000000..75c1bae94514
--- /dev/null
+++ b/drivers/gpio/dummy.c
@@ -0,0 +1,229 @@
+// SPDX-License-Identifier: GPL-2.0
+// Copyright (C) 2018 ROHM Semiconductors
+// gpio-bd70528.c ROHM BD70528MWV gpio driver
+
+#include <linux/gpio/driver.h>
+#include <linux/mfd/rohm-bd70528.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+
+#define GPIO_IN_REG(offset) (BD70528_REG_GPIO1_IN + (offset) * 2)
+#define GPIO_OUT_REG(offset) (BD70528_REG_GPIO1_OUT + (offset) * 2)
+
+struct bd70528_gpio {
+	struct rohm_regmap_dev chip;
+	struct gpio_chip gpio;
+};
+
+static int bd70528_set_debounce(struct bd70528_gpio *bdgpio,
+				unsigned int offset, unsigned int debounce)
+{
+	u8 val;
+
+	switch (debounce) {
+	case 0:
+		val = BD70528_DEBOUNCE_DISABLE;
+		break;
+	case 1 ... 15000:
+		val = BD70528_DEBOUNCE_15MS;
+		break;
+	case 15001 ... 30000:
+		val = BD70528_DEBOUNCE_30MS;
+		break;
+	case 30001 ... 50000:
+		val = BD70528_DEBOUNCE_50MS;
+		break;
+	default:
+		dev_err(bdgpio->chip.dev,
+			"Invalid debounce value %u\n", debounce);
+		return -EINVAL;
+	}
+	return regmap_update_bits(bdgpio->chip.regmap, GPIO_IN_REG(offset),
+				 BD70528_DEBOUNCE_MASK, val);
+}
+
+static int bd70528_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct bd70528_gpio *bdgpio = gpiochip_get_data(chip);
+	int val, ret;
+
+	/* Do we need to do something to IRQs here? */
+	ret = regmap_read(bdgpio->chip.regmap, GPIO_OUT_REG(offset), &val);
+	if (ret) {
+		dev_err(bdgpio->chip.dev, "Could not read gpio direction\n");
+		return ret;
+	}
+
+	return !(val & BD70528_GPIO_OUT_EN_MASK);
+}
+
+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;
+}
+
+static int bd70528_direction_input(struct gpio_chip *chip, unsigned int offset)
+{
+	struct bd70528_gpio *bdgpio = gpiochip_get_data(chip);
+
+	/* Do we need to do something to IRQs here? */
+	return regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
+				 BD70528_GPIO_OUT_EN_MASK,
+				 BD70528_GPIO_OUT_DISABLE);
+}
+
+static void bd70528_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			     int value)
+{
+	int ret;
+	struct bd70528_gpio *bdgpio = gpiochip_get_data(chip);
+	u8 val = (value) ? BD70528_GPIO_OUT_HI : BD70528_GPIO_OUT_LO;
+
+	ret = regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
+				 BD70528_GPIO_OUT_MASK, val);
+	if (ret)
+		dev_err(bdgpio->chip.dev, "Could not set gpio to %d\n", value);
+}
+
+static int bd70528_direction_output(struct gpio_chip *chip, unsigned int offset,
+				    int value)
+{
+	struct bd70528_gpio *bdgpio = gpiochip_get_data(chip);
+
+	bd70528_gpio_set(chip, offset, value);
+	return regmap_update_bits(bdgpio->chip.regmap, GPIO_OUT_REG(offset),
+				 BD70528_GPIO_OUT_EN_MASK,
+				 BD70528_GPIO_OUT_ENABLE);
+}
+
+#define GPIO_IN_STATE_MASK(offset) (BD70528_GPIO_IN_STATE_BASE << (offset))
+
+static int bd70528_gpio_get_o(struct bd70528_gpio *bdgpio, unsigned int offset)
+{
+	int ret;
+	unsigned int val;
+
+	ret = regmap_read(bdgpio->chip.regmap, GPIO_OUT_REG(offset), &val);
+	if (!ret)
+		ret = !!(val & BD70528_GPIO_OUT_MASK);
+	else
+		dev_err(bdgpio->chip.dev, "GPIO (out) state read failed\n");
+
+	return ret;
+}
+
+static int bd70528_gpio_get_i(struct bd70528_gpio *bdgpio, unsigned int offset)
+{
+	unsigned int val;
+	int ret;
+
+	ret = regmap_read(bdgpio->chip.regmap, BD70528_REG_GPIO_STATE, &val);
+
+	if (!ret)
+		ret = !(val & GPIO_IN_STATE_MASK(offset));
+	else
+		dev_err(bdgpio->chip.dev, "GPIO (in) state read failed\n");
+
+	return ret;
+}
+
+static int bd70528_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int ret;
+	struct bd70528_gpio *bdgpio = gpiochip_get_data(chip);
+
+	/*
+	 * There is a race condition where someone might be changing the
+	 * GPIO direction after we get it but before we read the value. But
+	 * application design where GPIO direction may be changed just when
+	 * we read GPIO value would be pointless as reader could not know
+	 * whether the returned high/low state is caused by input or output.
+	 * Or then there must be other ways to mitigate the issue. Thus
+	 * locking would make no sense.
+	 */
+	ret = bd70528_get_direction(chip, offset);
+	if (ret == 0)
+		ret = bd70528_gpio_get_o(bdgpio, offset);
+	else if (ret == 1)
+		ret = bd70528_gpio_get_i(bdgpio, offset);
+	else
+		dev_err(bdgpio->chip.dev, "failed to read GPIO direction\n");
+
+	return ret;
+}
+
+static int bd70528_probe(struct platform_device *pdev)
+{
+	struct bd70528_gpio *bdgpio;
+	struct rohm_regmap_dev *bd70528;
+	int ret;
+
+	bd70528 = dev_get_drvdata(pdev->dev.parent);
+	if (!bd70528) {
+		dev_err(&pdev->dev, "No MFD driver data\n");
+		return -EINVAL;
+	}
+
+	bdgpio = devm_kzalloc(&pdev->dev, sizeof(*bdgpio),
+			      GFP_KERNEL);
+	if (!bdgpio)
+		return -ENOMEM;
+	bdgpio->chip.dev = &pdev->dev;
+	bdgpio->gpio.parent = pdev->dev.parent;
+	bdgpio->gpio.label = "bd70528-gpio";
+	bdgpio->gpio.owner = THIS_MODULE;
+	bdgpio->gpio.get_direction = bd70528_get_direction;
+	bdgpio->gpio.direction_input = bd70528_direction_input;
+	bdgpio->gpio.direction_output = bd70528_direction_output;
+	bdgpio->gpio.set_config = bd70528_gpio_set_config;
+	bdgpio->gpio.can_sleep = true;
+	bdgpio->gpio.get = bd70528_gpio_get;
+	bdgpio->gpio.set = bd70528_gpio_set;
+	bdgpio->gpio.ngpio = 4;
+	bdgpio->gpio.base = -1;
+#ifdef CONFIG_OF_GPIO
+	bdgpio->gpio.of_node = pdev->dev.parent->of_node;
+#endif
+	bdgpio->chip.regmap = bd70528->regmap;
+
+	ret = devm_gpiochip_add_data(&pdev->dev, &bdgpio->gpio,
+				     bdgpio);
+	if (ret)
+		dev_err(&pdev->dev, "gpio_init: Failed to add bd70528-gpio\n");
+
+	return ret;
+}
+
+static struct platform_driver bd70528_gpio = {
+	.driver = {
+		.name = "bd70528-gpio"
+	},
+	.probe = bd70528_probe,
+};
+
+module_platform_driver(bd70528_gpio);
+
+MODULE_AUTHOR("Matti Vaittinen <matti.vaittinen@fi.rohmeurope.com>");
+MODULE_DESCRIPTION("BD70528 voltage regulator driver");
+MODULE_LICENSE("GPL");
-- 
2.21.3


[-- Attachment #3: checkpatch.pl --]
[-- Type: application/x-perl, Size: 207571 bytes --]