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

[Sebastian Reichel <sre-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>]

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

Hi,

On Tue, Nov 21, 2017 at 10:46:29PM +0800, Phil Reid wrote:
> G'day Sebastian,
> 
> On 21/11/2017 21:34, Sebastian Reichel wrote:
> > Hi,
> > 
> > On Tue, Nov 21, 2017 at 04:21:30PM +0800, Phil Reid wrote:
> > > The irq polarity is already encoded in the irq config. Use that to
> > > determine the polarity for the mcp32s08 irq output instead of the
> > > custom microchip,irq-active-high property.
> > > 
> > > Signed-off-by: Phil Reid <preid-qgqNFa1JUf/o2iN0hyhwsIdd74u8MsAO@public.gmane.org>
> > > ---
> > 
> > I don't like, that we use the flags for configuring the host
> > interrupt input and the mcp23xxx interrupt output. Usually
> > when the interrupt line has an inverter on it, board DTS files
> > just toggle the interrupts polarity. This will not work with
> > this patch applied. We would need to explicitly add an inverter
> > in the interrupt line, which is completely different to how its
> > implemented everywhere else (I know at least some Tegra devices
> > have implicit inverters on interrupt lines).
> > 
> > In case this is really wanted, this patch and the first patch
> > should be merged to avoid temporarily exposing the splitted
> > logic.
> > 
> Thanks for looking at the series.
> 
> Yes I understand where your coming from. And that's exactly what I
> was trying to do in v2. I have 2 of these devices with open drain output
> that is feed to an inverter. So active low output from devices and irq
> consumer is active high input.
> 
> However Linux wasn't a fan of the property and wanted it gone.

I guess s/Linux/Linus Walleij/?

> He suggested we need a "inverter" device to allow for that in the
> device tree. I haven't got my head around how to do that thou.

Just to be on the same term, we are talking about these two variants:

--------------------------------------------
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.

-- Sebastian

> > >   drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++---
> > >   1 file changed, 8 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/pinctrl/pinctrl-mcp23s08.c b/drivers/pinctrl/pinctrl-mcp23s08.c
> > > index 8ff9b77..6b3f810 100644
> > > --- a/drivers/pinctrl/pinctrl-mcp23s08.c
> > > +++ b/drivers/pinctrl/pinctrl-mcp23s08.c
> > > @@ -773,6 +773,7 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> > >   	bool mirror = false;
> > >   	bool irq_active_high = false;
> > >   	bool open_drain = false;
> > > +	u32 irq_trig;
> > >   	mutex_init(&mcp->lock);
> > > @@ -863,9 +864,13 @@ static int mcp23s08_probe_one(struct mcp23s08 *mcp, struct device *dev,
> > >   	mcp->irq_controller =
> > >   		device_property_read_bool(dev, "interrupt-controller");
> > >   	if (mcp->irq && mcp->irq_controller) {
> > > -		irq_active_high =
> > > -			device_property_read_bool(dev,
> > > -					      "microchip,irq-active-high");
> > > +		if (device_property_present(dev, "microchip,irq-active-high"))
> > > +			dev_warn(dev,
> > > +				 "microchip,irq-active-high is deprecated\n");
> > > +
> > > +		irq_trig = irqd_get_trigger_type(irq_get_irq_data(mcp->irq));
> > > +		if (irq_trig == IRQF_TRIGGER_HIGH)
> > > +			irq_active_high = true;
> > >   		mirror = device_property_read_bool(dev, "microchip,irq-mirror");
> > >   		open_drain = device_property_read_bool(dev, "drive-open-drain");
> > > -- 
> > > 1.8.3.1

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