Re: [PATCH v3 4/5] pinctrl: mcp23s08: configure irq polarity using irq data

[Sebastian Reichel <sre@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 3694 bytes --]

Hi,

On Tue, Nov 21, 2017 at 05:04:51PM +0100, Alexander Stein wrote:
> Hi,
> 
> On Tuesday, November 21, 2017, 4:21:42 PM CET Sebastian Reichel wrote:
> >[...]
> > --------------------------------------------
> > gpio: host-gpio {
> >     random-properties;
> > }
> > 
> > inv: line-inverter {
> >     /*
> >      * configure the gpio controller input to be active low
> >      * and the inverter interrupt output to be active low
> >      */
> >     interrupts = <&gpio ACTIVE_LOW>;
> > };
> > 
> > mcp23xxx {
> >     random-properties;
> > 
> >     /*
> >      * configure the chip interrupt output to be active high 
> >      * and the inverter interrupt input to be active high
> >      */
> >     interrupts = <&inv ACTIVE_HIGH>;
> > }
> > --------------------------------------------
> > 
> > versus
> > 
> > --------------------------------------------
> > gpio: host-gpio {
> >     random-properties;
> > }
> > 
> > mcp23xxx {
> >     random-properties;
> > 
> >     /* configure host interrupt input pin to be active low */
> >     interrupts = <&gpio ACTIVE_LOW>;
> > 
> >     /* configure chip interrupt output pin to be active high */
> >     microchip,irq-active-high;
> > }
> > --------------------------------------------
> > 
> > I think this is something, that Rob should comment on. Obviously at
> > least in the mainline kernel nobody implemented the first solution
> > (since there is no fitting interrupt-invert driver), but there are
> > a few instances of the second variant. On the other hand the first
> > solution describes the hardware more detailed.
> >
> > > And if someone is relying on that implicit behaviour are we allowed
> > > to break things? Probably ok with this one as it's currently not possible
> > > due to code patch 1 removes.
> > > 
> > > If we need to model the invert to get the patches accepted I look into that.
> > > I don't actually need it for my system as I can set open-drain with overrides
> > > the active-high control on this device, while have active high irq consumer.
> > > :)
> > 
> > IMHO the explicit line-inverter is a bit over-engineered and
> > implicit line-inverter is enough, but I'm fine with both solutions.
> > I think the DT binding maintainers should comment on this though,
> > since it's pretty much a core decision about interrupt specifiers.
> 
> Once you have a hardware, where one of several IRQ users is not attached to the
> inverter you need this inverter node anyway, caused by the mixed polarities.
> Also some IRQ controllers, like ARM GIC, only support rising edge interrupts
> (also level high).
>
> This might get important if there are dedicated IRQ pads connected to several
> users.

Both binding formats support this use-case as far as I can tell.
Since you seem to understand how it looks like with the inverter
node here is an example without it:

irqctrl: irq-controller {
    random-properties;
}

mcp23xxx0 {
    random-properties;

    /* 
     * configure host interrupt input pin to be active high, since
     * nothing else is supported by the interrupt controller
     */
    interrupts = <&irqctrl ACTIVE_HIGH>;

    /*
     * configure chip interrupt output pin to be active low due to
     * line invert
     */
    microchip,irq-active-high;
}

mcp23xxx1 {
    random-properties;

    /* 
     * shared irq, see description from other chip
     */
    interrupts = <&irqctrl ACTIVE_HIGH>;

    /*
     * configure chip interrupt output pin to be active high, since
     * it's routed through the line invert
     */
    /* microchip,irq-active-high; */
}

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]