On 09/16/2015 08:26 PM, Rob Herring wrote:
On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis <afd@xxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
