Re: [RFC] Potential issue with GPIO/IRQ flags

["Andrew F. Davis" <afd@ti.com>]

On 09/16/2015 08:26 PM, Rob Herring wrote:
> On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@ti.com> wrote:
>> Hello all,
>>
>> I've noticed that in a few DT bindings GPIO_ACTIVE_* defines are
>> incorrectly used as interrupt flags. GPIO_ACTIVE_*'s are defined
>> in:
>>
>> include/dt-bindings/gpio/gpio.h
>>
>> and are used to describe GPIO pins. IRQ types are defined in:
>>
>> include/dt-bindings/interrupt-controller/irq.h
>>
>> and are flags for IRQ pins.
>
> It is perfectly valid for the meaning of the field to be defined by
> the interrupt controller, and gpio interrupts could do something
> different. We've tried to standardize this though.
>

Sure, but in this case these are not what the interrupt controller
is expecting.

>> These seem to have been mixed up in a few places, take for example:
>> arch/arm/boot/dts/tegra124-jetson-tk1.dts. On line 1393 we see the
>> correct usage, but just before on line 1384 we see the issue.
>> GPIO_ACTIVE_HIGH is defined as 0, the same as IRQ_TYPE_NONE. If
>> this IRQ was not hard-coded with the correct edge in the driver
>> this would not work. What the author probably wanted was
>> IRQ_TYPE_LEVEL_HIGH.
>>
>> Now lets look at commit c21e678b256b, in this the IRQ flags did not
>> matter as the correct flag was hard-coded (IRQF_TRIGGER_LOW), this
>> patch moves this to the DT, but changed the flag to GPIO_ACTIVE_LOW
>> instead of the desired IRQ_TYPE_LEVEL_LOW. GPIO_ACTIVE_LOW is defined
>> as 1, or IRQ_TYPE_EDGE_RISING in IRQ flags, which is not the
>> equivalent to IRQF_TRIGGER_LOW the author was probably looking for.
>>
>> A quick grep (git grep "interrupt.*GPIO_ACTIVE_") shows several more
>> instances of this. I found this by using one of these files as an
>> example and giving myself a lot of problems, so I would like to fix
>> this before it spreads anymore.
>>
>> I have a couple of ideas of how to go at this, first would be to
>> just replace the incorrect flags with what was intended, but for
>> some of these I don't know what was intended and do not have the
>> board to test.
>>
>> My other solution would be to just change all instances of the GPIO
>> flags to their value corresponding IRQ flags:
>>
>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>;
>>
>> this would not make any functional change as the defines would
>> still evaluate to the same value, but would make it obvious where
>> a problem may be and that they should probably be checked and
>> corrected, maybe we could even put a comment after:
>>
>> - interrupts = <11 GPIO_ACTIVE_LOW>;
>> + interrupts = <11 IRQ_TYPE_EDGE_RISING>; // FIXME: Check IRQ type
>>
>> Well, what do you think?
>
> This seems fine. It is no less wrong.
>

I'm not sure what you mean here.

Regards,
Andrew

> Rob
>