From mboxrd@z Thu Jan 1 00:00:00 1970 From: Baolin Wang Subject: Re: [PATCH v3] Input: gpio_keys: Add level trigger support for GPIO keys Date: Fri, 2 Mar 2018 11:10:31 +0800 Message-ID: References: <826093167e8fb24723f474b0272f3dcab1b6a97e.1519821626.git.baolin.wang@linaro.org> <20180302004404.GA242226@dtor-ws> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20180302004404.GA242226@dtor-ws> Sender: linux-kernel-owner@vger.kernel.org To: Dmitry Torokhov Cc: Arnd Bergmann , Rob Herring , Mark Rutland , gregkh , stephen lu , Arvind Yadav , Joseph Lo , Kate Stewart , Philippe Ombredanne , Thomas Gleixner , "open list:HID CORE LAYER" , DTML , Linux Kernel Mailing List , Mark Brown , Linus Walleij List-Id: linux-input@vger.kernel.org On 2 March 2018 at 08:44, Dmitry Torokhov wrote: > On Thu, Mar 01, 2018 at 03:35:23PM +0800, Baolin Wang wrote: >> On 28 February 2018 at 22:44, Arnd Bergmann wrote: >> > On Wed, Feb 28, 2018 at 1:44 PM, Baolin Wang wrote: >> >> On some platforms (such as Spreadtrum platform), the GPIO keys can only >> >> be triggered by level type. So this patch introduces one property to >> >> indicate if the GPIO trigger type is level trigger or edge trigger. >> >> >> >> Signed-off-by: Baolin Wang >> >> --- >> >> Changes since v2: >> >> - Use 'interrupt' property to indicate the irq type. >> >> >> >> Changes since v1: >> >> - Diable the GPIO irq until reversing the GPIO level type. >> > >> > I've looked at your patch in more detail now, and given it a bit more thought. >> > >> > I wonder if you could move that logic into your gpiochip/irqchip driver instead. >> > It seems that what you do in the gpio-keys driver is to emulate edge triggered >> > behavior on a level triggered irqchip. >> > >> > If you put the same logic into the gpio driver, you could simply make it >> > pretend to support an edge trigger on both edges and call into the interrupt >> > handler whenever the state changes. >> > >> >> That is really a good suggestion, which can avoid duplicate level >> reverse logic in different drivers. So this patch can be simplified >> just adding one trigger_type to indicate the interrupt type (not >> always edge trigger). Thanks for your suggestion. > > No, there is no need to add trigger type. The gpio-keys driver expects > trigger with both edges, falling and rising. If your GPIO chip does not > support it natively, you need to emulate edge trigger via level > interrupts by reprogramming trigger from active low to active high and > back on the fly. This should be done in the gpiochip/irqchip driver. > There is no need to change gpio-keys driver. Yes, you are right, It can work on my platform after modifying my GPIO driver like you and Arnd's suggestion. Thanks for all your help. -- Baolin.wang Best Regards