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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
