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

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

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@xxxxxxxxxxxxxx> 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/  |