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

[Andrew F. Davis]

On 09/17/2015 05:35 PM, Stephen Warren wrote:
> On 09/17/2015 11:21 AM, Andrew F. Davis wrote:
> > On 09/17/2015 12:20 PM, Rob Herring wrote:
> > > On Thu, Sep 17, 2015 at 10:53 AM, Andrew F. Davis <afd@xxxxxx> wrote:
> > > > 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.
> > >
> > > Understood. I was talking generally, not this specific case.
> > >
> > > > > > 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.
> > >
> > > In this example, the correct value is probably IRQ_TYPE_LEVEL_LOW or
> > > IRQ_TYPE_EDGE_FALLING if the original text was correct in its
> > > intentions (but broken in implementation). Since the change you
> > > propose doesn't change the actual dtb at all, if it was wrong before
> > > it will still be wrong.
> >
> > I see, that's kinda what I want, maybe for this example the intentions
> > are obvious but my concern is with a couple others that I don't know
> > what the trigger was meant to be and don't have a board to test the
> > changes with, so I would never be sure if I causing any regressions
> > with the fixes. Most of the affected boards are Tegra based (that's
> > why I cc'd linux-tegra), I was hoping they would be interested in
> > testing and finding the right values.
>
> Presumably/hopefully if you send specific patches, the various
> maintainers/owners of those DT files will validate/ack then; you don't
> need to be able to test all of the changes yourself.

Well that was what I was going to do, but I found in some cases I didn't
know what the right value should be. Submitting patches would be easier,
but instead I had to write that block of text to try to recruit some help
from the original authors and people with the boards to find these correct
values.

--
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/