lkml.org 
[lkml]   [2015]   [Sep]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [RFC] Potential issue with GPIO/IRQ flags
    From
    Date
    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.


    \
     
     \ /
      Last update: 2015-09-18 01:01    [W:4.639 / U:0.024 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site