linux-kernel - Re: [PATCH] gpio: add TS-5500 DIO headers support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1354765784.12428.3.camel@trivette.home.lan>
Date:	Wed, 05 Dec 2012 22:49:44 -0500
From:	Vivien Didelot <vivien.didelot@...oirfairelinux.com>
To:	Linus Walleij <linus.walleij@...aro.org>
Cc:	linux-kernel@...r.kernel.org,
	Grant Likely <grant.likely@...retlab.ca>,
	Jerome Oufella <jerome.oufella@...oirfairelinux.com>
Subject: Re: [PATCH] gpio: add TS-5500 DIO headers support

Hi Linus,

I rewrote some parts according to your comments, but I still have some
concerns.

On Fri, 2012-10-12 at 22:53 +0200, Linus Walleij wrote:
> >> (...)
> >> > +static int ts5500_gpio_to_irq(struct gpio_chip *chip, unsigned
> offset)
> >> > +{
> >> > +       const struct ts5500_dio line = ts5500_dios[offset];
> >> > +
> >> > +       /* Only a few lines are IRQ-capable */
> >> > +       if (line.irq != NO_IRQ)
> >> > +               return line.irq;
> >> > +
> >> > +       /* This allows to bridge a line with the IRQ line of the
> same header */
> >> > +       if (dio1_irq && offset < 13)
> >> > +               return ts5500_dios[13].irq;
> >> > +       if (dio2_irq && offset > 13 && offset < 26)
> >> > +               return ts5500_dios[26].irq;
> >> > +       if (lcd_irq && offset > 26 && offset < 37)
> >> > +               return ts5500_dios[37].irq;
> >>
> >> Don't do this. Please use irqdomain for converting physical
> >> IRQ numbers to Linux IRQ numbers. (Consult other GPIO
> >> drivers for examples.)
> >>
> >> These magic constants (13, 26, 37) are scary too.
> >>
> >> You should not try to handle each block as a single
> >> IRQ, instead instatiate a struct irq_chip in the driver
> >> and let that use irqdomain do demux the IRQ and
> >> register a range of Linux IRQ numbers, on per pin,
> >> so the GPIO driver will handle the physical IRQ line,
> >> then dispatch to a fan-out handler, so drivers that need
> >> IRQs from the GPIO chip just register IRQ handlers like
> >> anyone else.
> >
> > Do you mean that I should not return the same IRQ line for the same
> > header, but virtual ones? I'll try to find a good example for that.
> 
> Basically Linux IRQs (also sometimes called virtual IRQs) are
> separate from the physical IRQ numbers of the system.
> 
> i.e. what you see in /proc/interrupts has no relation to the physical
> interrupt lines.
> 
> Keep in mind that we're trying to disallow IRQ 0 altogether and some
> platforms use that physical line for stuff.
> 
> So we need to use irqdomain to just grab an IRQ number to reference
> the physical line. And we often do that for the IRQ controller.
> 
> The fact that sometimes the physical line number and the Linux
> IRQ number correspond is just misleading...
> 
> In this case, since you have individual IRQs you want to check
> for different lines, register something with e.g.
> irq_domain_add_simple() to handle all these lines as IRQs.
> 
> It's a bit complex but pays off: all of a sudden you get statistics
> in /proc/interrupts for exactly which GPIO IRQs were fired,
> for example, and they get names if you provide that.
> 
> Look at the other GPIO drivers for many good examples of
> how to do this. gpio-em.c is one example. 

I looked at some drivers and if I'm not mistaken, this case is
different. Technologic Systems platforms (such as the TS-5500) have
several pin blocks. Each block has input-only, input-output or
output-only pins. Only one pin per block is connected to an interrupt
line. But sadly, these interrupt-connected lines are input only.
Here are the details about the TS-5500 pin block "DIO1":

http://wiki.embeddedarm.com/wiki/TS-5500#DIO1_Header

Some GPIO devices need a bidirectional data line which can trigger an
IRQ. In this case, we use a bidirectional pin for data, that we strap to
the IRQ-able pin.

Basically, our setup looks like that:

    +---+ in-only+IRQ
    | D |-------------+  data +--------+
    | I | in/out pin  |-------|  GPIO  |
    | O |-------------+   clk | device |
    | 1 |---------------------|(SHT15) |
    +---+ in/out pin          +--------+

That's why I previously used a dio1_irq platform data field, to return
the interrupt connected to the IRQ-able pin for any GPIO on DIO1, in the
gpio_to_irq() implementation.

A Linux IRQ per pin doesn't seem to be possible because the
irq_create_mapping() documentation says that "Only one mapping per
hardware interrupt is permitted." Should I still implement the
irq_chip/irqdomain for a single IRQ per block? For each pin?
What do you think about this implementation?


Yours,
Vivien

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/