Re: [PATCH v2] gpio: tqmx86: Add GPIO from for this IO controller

[Andrew Lunn <andrew@xxxxxxx>]

On Mon, Jan 14, 2019 at 11:09:07AM +0100, Linus Walleij wrote:
> Hi Andrew,
> 
> (Cc Julia C for the RT spinlock question.)
> 
> thanks for the updated v2 patch! It's almost perfect. But I ran into some
> snags.
> 
> Overall it is pretty much an MMIO driver, just that it uses ioread()/iowrite()
> and we really need to get around to fixing up the gpio-mmio.c to
> support x86 style io with some flag. But it is a bit much to ask for
> a simple driver (I might send or ask for patches later to convert it.)

I consider it, but never made the jump to actually do it. gpio-sch.c
might benefit.  gpio-ts5500.c, gpio-gpio-mm.c, gpio-104-idio-16.c look
like they could be converted to central accessors.

gpio-kempld.c, gpio-sch311x.c gpio-it87.c, gpio-f7188x.c,
gpio-104-idio-16.c use inb() outb(), but are not straight memory
mapped. They have an extra level of indirection. But that indirection
looks similar for them all, so maybe some shared accessors could also
be added.

I would be happy to test patches for this driver, and i also have a
board using the gpio-kempld.d driver.

 
> On Sat, Jan 12, 2019 at 12:49 AM Andrew Lunn <andrew@xxxxxxx> wrote:
> 
> > Some TQ-Systems ComExpress modules contain an IO controller with 8
> > GPIO lines.
> >
> > Signed-off-by: Andrew Lunn <andrew@xxxxxxx>
> > ---
> > v2
> 
> > diff --git a/drivers/gpio/gpio-tqmx86.c b/drivers/gpio/gpio-tqmx86.c
> > new file mode 100644
> > index 000000000000..f626b716f30c
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-tqmx86.c
> 
> Strangely this does not apply in my tree!
> 
> $ git am --signoff lunn1.patch
> Applying: gpio: tqmx86: Add GPIO from for this IO controller
> error: new file drivers/gpio/gpio-tqmx86.c depends on old contents
> Patch failed at 0001 gpio: tqmx86: Add GPIO from for this IO controller

Ah, sorry about that. I have drivers for GPIO, MFD, I2C and platform
in the same branch. In order to make that work, i'm using plain -rc1,
not a maintainer specific tree. That could be the issue.

When i resend, i can cherry-pick it to your tree.

> > +struct tqmx86_gpio_data {
> > +       struct gpio_chip        chip;
> > +       struct irq_chip         irq_chip;
> > +       void __iomem            *io_base;
> > +       int                     irq;
> > +       spinlock_t              spinlock;
> 
> I am not an expert in RT but I think this needs to be a
> raw_spinlock_t (and use raw accessors) to work with realtime.
> 
> But maybe that just apply to chained IRQ handlers?

Not my area of expertise also.

I just read Documentation/driver-api/gpio/driver.rst.

That suggests i should use raw spinlocks. This could be used as a fast
chained IRQ, in that it is hanging off the main x86 interrupt
controller, and accessing the GPIO interrupt registers are also fast,
non-blocking operations.

However, my use case don't require this. I'm only using one pin as an
interrupt. That interrupt is itself just another link in an interrupt
chain, in that i have an Ethernet switch's interrupt pin connected to
the GPIO.  I need to read switch registers to read the switches
interrupt controller register. And that is done over a bitbanging MDIO
bus, implemented by pins on this GPIO controller. So that is all done
in a threaded interrupt handler, and all very slow.

However, this is a generic driver, and used in generic x86 hardware
often used in embedded systems. So other users might have RT use
cases.

Thanks
	Andrew