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

[Thorsten Scherer <thorsten.scherer@xxxxxxxxxxxx>]

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