| Message ID | 20190625105346.3267-1-linus.walleij@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: siox: Pass irqchip when adding gpiochip | expand |
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;
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
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
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/ |
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;
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(-)