diff mbox series

gpio: siox: Pass irqchip when adding gpiochip

Message ID 20190625105346.3267-1-linus.walleij@linaro.org
State Superseded
Headers show
Series gpio: siox: Pass irqchip when adding gpiochip | expand

Commit Message

Linus Walleij June 25, 2019, 10:53 a.m. UTC 
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.

The siox GPIO driver passes a IRQ_TYPE_EDGE_RISING as
default IRQ trigger type which seems wrong, as consumers
should explicitly set this up, so set IRQ_TYPE_NONE instead.

Also 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>
Cc: Thierry Reding <treding@nvidia.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

---
 drivers/gpio/gpio-siox.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

-- 
2.20.1

Comments

Uwe Kleine-König June 25, 2019, 7:33 p.m. UTC | #1 
Hello Linus,

On Tue, Jun 25, 2019 at 12:53:46PM +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.

> 

> The siox GPIO driver passes a IRQ_TYPE_EDGE_RISING as

> default IRQ trigger type which seems wrong, as consumers

> should explicitly set this up, so set IRQ_TYPE_NONE instead.

> 

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


So we have a bugfix (gpiochip_remove() in error path), a change of
default behaviour (IRQ_TYPE_EDGE_RISING -> IRQ_TYPE_NONE) and a cleanup
for an API change (I'm guessing here) in a single patch. :-|

@Thorsten: I'm not entirely sure if there is code relying on the default
IRQ_TYPE_EDGE_RISING. Do you know off-hand?

Best regards
Uwe

> diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c

> index fb4e318ab028..e5c85dc932e8 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,20 +240,16 @@ 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,

>  			"Failed to register gpio chip (%d)\n", ret);

> -		goto err_gpiochip;

> -	}

> -

> -	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,

> -				   0, handle_level_irq, IRQ_TYPE_EDGE_RISING);

> -	if (ret) {

> -		dev_err(&sdevice->dev,

> -			"Failed to register irq chip (%d)\n", ret);

> -err_gpiochip:

> -		gpiochip_remove(&ddata->gchip);

> +		return ret;

>  	}

>  

>  	return ret;


-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Linus Walleij June 26, 2019, 8:05 a.m. UTC | #2 
On Tue, Jun 25, 2019 at 9:33 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Jun 25, 2019 at 12:53:46PM +0200, Linus Walleij wrote:


> > The siox GPIO driver passes a IRQ_TYPE_EDGE_RISING as

> > default IRQ trigger type which seems wrong, as consumers

> > should explicitly set this up, so set IRQ_TYPE_NONE instead.

> >

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

>

> So we have a bugfix (gpiochip_remove() in error path), a change of

> default behaviour (IRQ_TYPE_EDGE_RISING -> IRQ_TYPE_NONE) and a cleanup

> for an API change (I'm guessing here) in a single patch. :-|


Yes I tend to do that to save time because I am a bit overwhelmed
by all the stuff that falls upwards to the GPIO maintainer.

More often than not there is zero feedback from the maintainer(s)
of the drivers, and the kernel looks better after than before.

But since you provide some feedback I'll just go and split
the patch.

> @Thorsten: I'm not entirely sure if there is code relying on the default

> IRQ_TYPE_EDGE_RISING. Do you know off-hand?


I saw that the driver has #include <linux/of.h> (hm seems unused) so
if this is used on devicetree systems with normal twocell irqchips then
there shouldn't be a need for any default type. The default type
is only used with board files. The siox seems not even possible
to use with board files (no platform data path).

Yours,
Linus Walleij
Uwe Kleine-König June 26, 2019, 8:10 a.m. UTC | #3 
Hello Linus,

On Wed, Jun 26, 2019 at 10:05:42AM +0200, Linus Walleij wrote:
> On Tue, Jun 25, 2019 at 9:33 PM Uwe Kleine-König

> <u.kleine-koenig@pengutronix.de> wrote:

> > On Tue, Jun 25, 2019 at 12:53:46PM +0200, Linus Walleij wrote:

> 

> > > The siox GPIO driver passes a IRQ_TYPE_EDGE_RISING as

> > > default IRQ trigger type which seems wrong, as consumers

> > > should explicitly set this up, so set IRQ_TYPE_NONE instead.

> > >

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

> >

> > So we have a bugfix (gpiochip_remove() in error path), a change of

> > default behaviour (IRQ_TYPE_EDGE_RISING -> IRQ_TYPE_NONE) and a cleanup

> > for an API change (I'm guessing here) in a single patch. :-|

> 

> Yes I tend to do that to save time because I am a bit overwhelmed

> by all the stuff that falls upwards to the GPIO maintainer.

> 

> More often than not there is zero feedback from the maintainer(s)

> of the drivers, and the kernel looks better after than before.

> 

> But since you provide some feedback I'll just go and split

> the patch.


\o/, thanks.

> > @Thorsten: I'm not entirely sure if there is code relying on the default

> > IRQ_TYPE_EDGE_RISING. Do you know off-hand?

> 

> I saw that the driver has #include <linux/of.h> (hm seems unused) so

> if this is used on devicetree systems with normal twocell irqchips then

> there shouldn't be a need for any default type. The default type

> is only used with board files. The siox seems not even possible

> to use with board files (no platform data path).


I think the gpio irq is used from userspace. If you're convinced it
doesn't matter (and you describe that in the commit log) I'm willing to
believe you :-)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Thorsten Scherer June 26, 2019, 7:36 p.m. UTC | #4 
Hello,

On Tue, Jun 25, 2019 at 09:33:28PM +0200, Uwe Kleine-König wrote:
> Hello Linus,

> 

> On Tue, Jun 25, 2019 at 12:53:46PM +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.

> > 

> > The siox GPIO driver passes a IRQ_TYPE_EDGE_RISING as

> > default IRQ trigger type which seems wrong, as consumers

> > should explicitly set this up, so set IRQ_TYPE_NONE instead.

> > 

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

> 

> So we have a bugfix (gpiochip_remove() in error path), a change of

> default behaviour (IRQ_TYPE_EDGE_RISING -> IRQ_TYPE_NONE) and a cleanup

> for an API change (I'm guessing here) in a single patch. :-|

> 

> @Thorsten: I'm not entirely sure if there is code relying on the default

> IRQ_TYPE_EDGE_RISING. Do you know off-hand?


Didn't know off the top of my head.  So I dug through some application
code.  As far as I can tell, nothing relies on edge rising.  But I would
not bet on it.  And I don't know about code in the other departments.

> 

> Best regards

> Uwe


Best regards
Thorsten

> 

> > diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c

> > index fb4e318ab028..e5c85dc932e8 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,20 +240,16 @@ 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,

> >  			"Failed to register gpio chip (%d)\n", ret);

> > -		goto err_gpiochip;

> > -	}

> > -

> > -	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,

> > -				   0, handle_level_irq, IRQ_TYPE_EDGE_RISING);

> > -	if (ret) {

> > -		dev_err(&sdevice->dev,

> > -			"Failed to register irq chip (%d)\n", ret);

> > -err_gpiochip:

> > -		gpiochip_remove(&ddata->gchip);

> > +		return ret;

> >  	}

> >  

> >  	return ret;

> 

> -- 

> Pengutronix e.K.                           | Uwe Kleine-König            |

> Industrial Linux Solutions                 | http://www.pengutronix.de/  |


-- 
Thorsten Scherer - Eckelmann AG
https://www.eckelmann.de
diff mbox series

Patch

diff --git a/drivers/gpio/gpio-siox.c b/drivers/gpio/gpio-siox.c
index fb4e318ab028..e5c85dc932e8 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,20 +240,16 @@  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,
 			"Failed to register gpio chip (%d)\n", ret);
-		goto err_gpiochip;
-	}
-
-	ret = gpiochip_irqchip_add(&ddata->gchip, &ddata->ichip,
-				   0, handle_level_irq, IRQ_TYPE_EDGE_RISING);
-	if (ret) {
-		dev_err(&sdevice->dev,
-			"Failed to register irq chip (%d)\n", ret);
-err_gpiochip:
-		gpiochip_remove(&ddata->gchip);
+		return ret;
 	}
 
 	return ret;