[PATCH 1/5] gpio: siox: Do not call gpiochip_remove() on errorpath

[PATCH 1/5] gpio: siox: Do not call gpiochip_remove() on errorpath

[Linus Walleij]

gpiochip_remove() was called on the errorpath if
gpiochip_add() failed: this is wrong, if the chip failed
to add it is not there so it should not be removed.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-siox.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index fb4e318ab028..0b4450118865 100644
--- a/drivers/gpio/gpio-siox.c
+++ b/drivers/gpio/gpio-siox.c
@@ -243,17 +243,14 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 	if (ret) {
 		dev_err(&sdevice->dev,
 			"Failed to register gpio chip (%d)\n", ret);
-		goto err_gpiochip;
+		return ret;
 	}
 
 	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,
 				   0, handle_level_irq, IRQ_TYPE_EDGE_RISING);
-	if (ret) {
+	if (ret)
 		dev_err(&sdevice->dev,
 			"Failed to register irq chip (%d)\n", ret);
-err_gpiochip:
-		gpiochip_remove(&ddata->gchip);
-	}
 
 	return ret;
 }
-- 
2.20.1

[PATCH 2/5] gpio: siox: Switch to IRQ_TYPE_NONE

[Linus Walleij]

The siox driver is hardcoding a default type of
IRQ_TYPE_EDGE_RISING to the irq helper, but this should only
be applicable to old boardfiles and odd device tree irqchips
with just onecell irq (no flags). I doubt this is the case
with the siox, I think all consumers specify the flags they
use in the device tree.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-siox.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index 0b4450118865..40067e1535d3 100644
--- a/drivers/gpio/gpio-siox.c
+++ b/drivers/gpio/gpio-siox.c
@@ -247,7 +247,7 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 	}
 
 	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,
-				   0, handle_level_irq, IRQ_TYPE_EDGE_RISING);
+				   0, handle_level_irq, IRQ_TYPE_NONE);
 	if (ret)
 		dev_err(&sdevice->dev,
 			"Failed to register irq chip (%d)\n", ret);
-- 
2.20.1

Re: [PATCH 2/5] gpio: siox: Switch to IRQ_TYPE_NONE

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 10:44:04AM +0200, Linus Walleij wrote:
> The siox driver is hardcoding a default type of
> IRQ_TYPE_EDGE_RISING to the irq helper, but this should only
> be applicable to old boardfiles and odd device tree irqchips
> with just onecell irq (no flags). I doubt this is the case
> with the siox, I think all consumers specify the flags they
> use in the device tree.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 3/5] gpio: siox: Pass irqchip when adding gpiochip

[Linus Walleij]

We need to convert all old gpio irqchips to pass the irqchip
setup along when adding the gpio_chip.

For chained irqchips this is a pretty straight-forward
conversion.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
ChangeLog v1->v2:
- Split out bugfixes to separate patches.
---
 drivers/gpio/gpio-siox.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index 40067e1535d3..31749c058e33 100644
--- a/drivers/gpio/gpio-siox.c
+++ b/drivers/gpio/gpio-siox.c
@@ -211,6 +211,7 @@ static int gpio_siox_get_direction(struct gpio_chip *chip, unsigned int offset)
 static int gpio_siox_probe(struct siox_device *sdevice)
 {
 	struct gpio_siox_ddata *ddata;
+	struct gpio_irq_chip *girq;
 	int ret;
 
 	ddata = devm_kzalloc(&sdevice->dev, sizeof(*ddata), GFP_KERNEL);
@@ -239,6 +240,11 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 	ddata->ichip.irq_unmask = gpio_siox_irq_unmask;
 	ddata->ichip.irq_set_type = gpio_siox_irq_set_type;
 
+	girq = &ddata->gchip.irq;
+	girq->chip = &ddata->ichip;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->handler = handle_level_irq;
+
 	ret = gpiochip_add(&ddata->gchip);
 	if (ret) {
 		dev_err(&sdevice->dev,
@@ -246,13 +252,7 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 		return ret;
 	}
 
-	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,
-				   0, handle_level_irq, IRQ_TYPE_NONE);
-	if (ret)
-		dev_err(&sdevice->dev,
-			"Failed to register irq chip (%d)\n", ret);
-
-	return ret;
+	return 0;
 }
 
 static int gpio_siox_remove(struct siox_device *sdevice)
-- 
2.20.1

Re: [PATCH 3/5] gpio: siox: Pass irqchip when adding gpiochip

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 10:44:05AM +0200, Linus Walleij wrote:
> We need to convert all old gpio irqchips to pass the irqchip
> setup along when adding the gpio_chip.
> 
> For chained irqchips this is a pretty straight-forward
> conversion.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: Thierry Reding <treding@nvidia.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> ChangeLog v1->v2:
> - Split out bugfixes to separate patches.
> ---
>  drivers/gpio/gpio-siox.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
> index 40067e1535d3..31749c058e33 100644
> --- a/drivers/gpio/gpio-siox.c
> +++ b/drivers/gpio/gpio-siox.c
> @@ -211,6 +211,7 @@ static int gpio_siox_get_direction(struct gpio_chip *chip, unsigned int offset)
>  static int gpio_siox_probe(struct siox_device *sdevice)
>  {
>  	struct gpio_siox_ddata *ddata;
> +	struct gpio_irq_chip *girq;
>  	int ret;
>  
>  	ddata = devm_kzalloc(&sdevice->dev, sizeof(*ddata), GFP_KERNEL);
> @@ -239,6 +240,11 @@ static int gpio_siox_probe(struct siox_device *sdevice)
>  	ddata->ichip.irq_unmask = gpio_siox_irq_unmask;
>  	ddata->ichip.irq_set_type = gpio_siox_irq_set_type;
>  
> +	girq = &ddata->gchip.irq;
> +	girq->chip = &ddata->ichip;
> +	girq->default_type = IRQ_TYPE_NONE;
> +	girq->handler = handle_level_irq;
> +
>  	ret = gpiochip_add(&ddata->gchip);
>  	if (ret) {
>  		dev_err(&sdevice->dev,
> @@ -246,13 +252,7 @@ static int gpio_siox_probe(struct siox_device *sdevice)
>  		return ret;
>  	}
>  
> -	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,
> -				   0, handle_level_irq, IRQ_TYPE_NONE);
> -	if (ret)
> -		dev_err(&sdevice->dev,
> -			"Failed to register irq chip (%d)\n", ret);
> -
> -	return ret;
> +	return 0;
>  }

You can simplify this a bit further from:

	if (ret) {
		...
		return ret;
	}
	return 0;

to

	if (ret) {
		...
	}
	return ret;

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 4/5] gpio: siox: Add struct device *dev helper variable

[Linus Walleij]

This makes the code easier to read.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-siox.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index 31749c058e33..f85f1fab781f 100644
--- a/drivers/gpio/gpio-siox.c
+++ b/drivers/gpio/gpio-siox.c
@@ -212,20 +212,21 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 {
 	struct gpio_siox_ddata *ddata;
 	struct gpio_irq_chip *girq;
+	struct device *dev = &sdevice->dev;
 	int ret;
 
-	ddata = devm_kzalloc(&sdevice->dev, sizeof(*ddata), GFP_KERNEL);
+	ddata = devm_kzalloc(dev, sizeof(*ddata), GFP_KERNEL);
 	if (!ddata)
 		return -ENOMEM;
 
-	dev_set_drvdata(&sdevice->dev, ddata);
+	dev_set_drvdata(dev, ddata);
 
 	mutex_init(&ddata->lock);
 	spin_lock_init(&ddata->irqlock);
 
 	ddata->gchip.base = -1;
 	ddata->gchip.can_sleep = 1;
-	ddata->gchip.parent = &sdevice->dev;
+	ddata->gchip.parent = dev;
 	ddata->gchip.owner = THIS_MODULE;
 	ddata->gchip.get = gpio_siox_get;
 	ddata->gchip.set = gpio_siox_set;
@@ -247,8 +248,7 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 
 	ret = gpiochip_add(&ddata->gchip);
 	if (ret) {
-		dev_err(&sdevice->dev,
-			"Failed to register gpio chip (%d)\n", ret);
+		dev_err(dev, "Failed to register gpio chip (%d)\n", ret);
 		return ret;
 	}
 
-- 
2.20.1

Re: [PATCH 4/5] gpio: siox: Add struct device *dev helper variable

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 10:44:06AM +0200, Linus Walleij wrote:
> This makes the code easier to read.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

fine,

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH 5/5] gpio: siox: Use devm_ managed gpiochip

[Linus Walleij]

By using devm_gpiochip_add_data() we can get rid of the
remove() callback. As this driver doesn't use the
gpiochip data pointer we simply pass in NULL.

Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-siox.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index f85f1fab781f..5e3861c1ad99 100644
--- a/drivers/gpio/gpio-siox.c
+++ b/drivers/gpio/gpio-siox.c
@@ -246,7 +246,7 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 	girq->default_type = IRQ_TYPE_NONE;
 	girq->handler = handle_level_irq;
 
-	ret = gpiochip_add(&ddata->gchip);
+	ret = devm_gpiochip_add_data(dev, &ddata->gchip, NULL);
 	if (ret) {
 		dev_err(dev, "Failed to register gpio chip (%d)\n", ret);
 		return ret;
@@ -255,17 +255,8 @@ static int gpio_siox_probe(struct siox_device *sdevice)
 	return 0;
 }
 
-static int gpio_siox_remove(struct siox_device *sdevice)
-{
-	struct gpio_siox_ddata *ddata = dev_get_drvdata(&sdevice->dev);
-
-	gpiochip_remove(&ddata->gchip);
-	return 0;
-}
-
 static struct siox_driver gpio_siox_driver = {
 	.probe = gpio_siox_probe,
-	.remove = gpio_siox_remove,
 	.set_data = gpio_siox_set_data,
 	.get_data = gpio_siox_get_data,
 	.driver = {
-- 
2.20.1

Re: [PATCH 5/5] gpio: siox: Use devm_ managed gpiochip

[Uwe Kleine-König]

Hello,

On Wed, Jun 26, 2019 at 10:44:07AM +0200, Linus Walleij wrote:
> By using devm_gpiochip_add_data() we can get rid of the
> remove() callback. As this driver doesn't use the
> gpiochip data pointer we simply pass in NULL.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Thanks,
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/5] gpio: siox: Do not call gpiochip_remove() on errorpath

[Uwe Kleine-König]

On Wed, Jun 26, 2019 at 10:44:03AM +0200, Linus Walleij wrote:
> gpiochip_remove() was called on the errorpath if
> gpiochip_add() failed: this is wrong, if the chip failed
> to add it is not there so it should not be removed.
> 
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Does this warrant a

	Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")

?

Thanks
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH 1/5] gpio: siox: Do not call gpiochip_remove() on errorpath

[Linus Walleij]

On Wed, Jun 26, 2019 at 11:12 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Wed, Jun 26, 2019 at 10:44:03AM +0200, Linus Walleij wrote:
> > gpiochip_remove() was called on the errorpath if
> > gpiochip_add() failed: this is wrong, if the chip failed
> > to add it is not there so it should not be removed.
> >
> > Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
>
> Does this warrant a
>
>         Fixes: be8c8facc707 ("gpio: new driver to work with a 8x12 siox")

Thanks yeah I put the fixes in as a lowprio fix that goes
in with the rest of the v5.3 patches.

Yours,
Linus Walleij