Messages in this thread | | | Subject | Re: [RFC] Potential issue with GPIO/IRQ flags | From | "Andrew F. Davis" <> | Date | Thu, 17 Sep 2015 18:49:11 -0500 |
| |
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@ti.com> wrote: >>>> 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. >>> >>> 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
| |