[v3,4/5] pinctrl: mcp23s08: configure irq polarity using irq data
diff mbox series

Message ID 1511252491-79952-5-git-send-email-preid@electromag.com.au
State New
Headers show
Series
  • pinctrl: mcp32s08: add open drain config for irq
Related show

Commit Message

Phil Reid Nov. 21, 2017, 8:21 a.m. UTC
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@electromag.com.au>
---
 drivers/pinctrl/pinctrl-mcp23s08.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Comments

Sebastian Reichel Nov. 21, 2017, 1:34 p.m. UTC | #1
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@electromag.com.au>
> ---

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.

-- 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
>
Phil Reid Nov. 21, 2017, 2:46 p.m. UTC | #2
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@electromag.com.au>
>> ---
> 
> 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.
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.

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


> 
>>   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
>>
Sebastian Reichel Nov. 21, 2017, 3:21 p.m. UTC | #3
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@electromag.com.au>
> > > ---
> > 
> > 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
Phil Reid Nov. 21, 2017, 3:38 p.m. UTC | #4
On 21/11/2017 23:21, Sebastian Reichel wrote:
> 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@electromag.com.au>
>>>> ---
>>>
>>> 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/?

oops, yes.

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

Yes that was my understanding of the options, with option 1 being favoured.
Nice summary of the options.

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

Thanks again, I'll await further feedback on the preferred direction.>
> -- 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
Alexander Stein Nov. 21, 2017, 4:04 p.m. UTC | #5
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.

Just my 2 cents.
Alexander

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sebastian Reichel Nov. 21, 2017, 4:30 p.m. UTC | #6
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
Linus Walleij Nov. 30, 2017, 2:17 p.m. UTC | #7
On Tue, Nov 21, 2017 at 9:21 AM, Phil Reid <preid@electromag.com.au> 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@electromag.com.au>
(...)
>         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;

This makes the setting from the interrupt line override the
custom property.

Should it not be the other way around to preserve compatibility
with elder device trees?

I.e. the custom property overrides the line setting?

Otherwise it looks good.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij Nov. 30, 2017, 2:21 p.m. UTC | #8
On Tue, Nov 21, 2017 at 4:21 PM, Sebastian Reichel <sre@kernel.org> wrote:

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

I feel the same.

I am very much back and forth on the subject.

Simplicity of use vs modelling the system as it actually works.

Back and forth.

I honestly have just a very vague idea about this.

I don't know if Marc Z as irqchip maintainer has some idea
on how to model inverters on irq lines or if he's seen some
solutions to it out there.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Marc Zyngier Nov. 30, 2017, 3:50 p.m. UTC | #9
On 30/11/17 14:21, Linus Walleij wrote:
> On Tue, Nov 21, 2017 at 4:21 PM, Sebastian Reichel <sre@kernel.org> wrote:
> 
>> 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.
> 
> I feel the same.
> 
> I am very much back and forth on the subject.
> 
> Simplicity of use vs modelling the system as it actually works.
> 
> Back and forth.
> 
> I honestly have just a very vague idea about this.
> 
> I don't know if Marc Z as irqchip maintainer has some idea
> on how to model inverters on irq lines or if he's seen some
> solutions to it out there.

So far, I've seen two types of solutions:
- One based on a stacked irqchip driver that implements the inverter on
the irq_set_type method
- One based on per-device vendor-specific properties in DT

While the first one is clearly a big hammer, it has the advantage of not
adding new stuff to the DT spec, and accurately describe the signal path
(see the mediatek stuff for reference).

The second one is just a hack, frankly. It just has the advantage of
being trivial to implement.

I'm clearly inclined to prefer the first solution. But maybe it is time
to invent a "generic inverter" driver that could be reusable, just like
we have a generic irqchip?

Thanks,

	M.
Linus Walleij Dec. 1, 2017, 8:38 a.m. UTC | #10
On Thu, Nov 30, 2017 at 4:50 PM, Marc Zyngier <marc.zyngier@arm.com> wrote:

> I'm clearly inclined to prefer the first solution. But maybe it is time
> to invent a "generic inverter" driver that could be reusable, just like
> we have a generic irqchip?

I'm leaning towards that we should create
compatible = "irqline-inverter"; irqchip and just slap that into
drivers/of/irq.c for everyone. *Maybe* with a Kconfig
for it if people worry about the extra bytes.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox series

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");