From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Andrew F. Davis" Subject: Re: [RFC] Potential issue with GPIO/IRQ flags Date: Thu, 17 Sep 2015 10:53:31 -0500 Message-ID: <55FAE1FB.4070300@ti.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Rob Herring Cc: Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , Stephen Warren , Andreas Fenkart , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: linux-tegra@vger.kernel.org On 09/16/2015 08:26 PM, Rob Herring wrote: > On Wed, Sep 16, 2015 at 4:07 PM, Andrew F. Davis 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 devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html