From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from hemlock.osuosl.org (smtp2.osuosl.org [140.211.166.133]) by ash.osuosl.org (Postfix) with ESMTP id 6EC861C0263 for ; Wed, 30 May 2018 04:55:04 +0000 (UTC) Received: from localhost (localhost [127.0.0.1]) by hemlock.osuosl.org (Postfix) with ESMTP id 6BC7788E95 for ; Wed, 30 May 2018 04:55:04 +0000 (UTC) Received: from hemlock.osuosl.org ([127.0.0.1]) by localhost (.osuosl.org [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id UIetmzv6111k for ; Wed, 30 May 2018 04:55:03 +0000 (UTC) Received: from mail-wr0-f196.google.com (mail-wr0-f196.google.com [209.85.128.196]) by hemlock.osuosl.org (Postfix) with ESMTPS id A0EA388E91 for ; Wed, 30 May 2018 04:55:03 +0000 (UTC) Received: by mail-wr0-f196.google.com with SMTP id v13-v6so16371949wrp.13 for ; Tue, 29 May 2018 21:55:03 -0700 (PDT) Date: Wed, 30 May 2018 06:54:29 +0200 From: Sergio Paracuellos Subject: Re: [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node Message-ID: <20180530045429.GA18404@foobar> References: <87efhwwv9y.fsf@notabene.neil.brown.name> <1527569889-9510-1-git-send-email-sergio.paracuellos@gmail.com> <1527569889-9510-2-git-send-email-sergio.paracuellos@gmail.com> <87po1eujcg.fsf@notabene.neil.brown.name> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <87po1eujcg.fsf@notabene.neil.brown.name> List-Id: Linux Driver Project Developer List List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: driverdev-devel-bounces@linuxdriverproject.org Sender: "devel" To: NeilBrown Cc: gregkh@linuxfoundation.org, driverdev-devel@linuxdriverproject.org On Wed, May 30, 2018 at 09:34:39AM +1000, NeilBrown wrote: > On Tue, May 29 2018, Sergio Paracuellos wrote: > > > Most gpio chips have two cells for interrupts and this should be also. > > Set this property accordly fixing this up. > > > > Signed-off-by: Sergio Paracuellos > > --- > > drivers/staging/mt7621-dts/mt7621.dtsi | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/staging/mt7621-dts/mt7621.dtsi b/drivers/staging/mt7621-dts/mt7621.dtsi > > index d7e4981..bce6029 100644 > > --- a/drivers/staging/mt7621-dts/mt7621.dtsi > > +++ b/drivers/staging/mt7621-dts/mt7621.dtsi > > @@ -70,7 +70,7 @@ > > interrupt-parent = <&gic>; > > interrupts = ; > > interrupt-controller; > > - #interrupt-cells = <1>; > > + #interrupt-cells = <2>; > > Thanks for this ongoing effort. > > I thought I should test this change. It didn't quite go as I expected. > My board has one GPIO wired to a push-button so it is normally > configured with > > gpio-keys { > compatible = "gpio-keys"; > > reset { > label = "reset"; > gpios = <&gpio0 18 GPIO_ACTIVE_HIGH>; > ... > > (though in upstream it still uses the old gpio-keys-polled). > I removed the gpios line and replaced with > > interrupt-parent = <&gpio>; > interrupts = <18 IRQ_TYPE_EDGE_FALLING>; > > which should produce a key-press event whenever the button is pressed. > It didn't. > > The reason is > > .xlate = irq_domain_xlate_onecell, > > in irq_domain_ops in gpio-mt7621.c. > "onecell" obviously correlated with #interrupt-cells = <1>. > I changed it to > .xlate = irq_domain_xlate_twocell, > > and now it works as expected. So we need to combine that change with > the change to #interrupt-cells. I'm certain that we do really want 2 > cells here, as it is possible to change the trigger type. > > You might have noticed that I added > interrupt-parent = <&gpio>; > > even though there is no 'gpio:' tag in the devicetree. I had to add > one. > > - gpio@600 { > + gpio: gpio@600 { > > so that I could refer to the gpio interrupts. > This feels a bit untidy. The gpios are grouped into banks of 32: > gpio0 gpio1 grio2 > but the interrupts are just a single bank of 96 interrupts. > I don't know that this is a problem and I'm not advocating that we "fix" > it. But it might be something that will be queried when we > submit to linux-gpio - I really don't know. > > So if you could redo this patch to added the gpio: label and change > the xlate function, that would be excellent. > For all the rest: > Reviewed-by: NeilBrown > > Thanks a lot, > NeilBrown Thanks for your review and clear explanation, Neil. That really helps. I have just send v2 version for this patch with the changes you are pointing out here. Hope this helps. Best regards, Sergio Paracuellos > > > > > gpio0: bank@0 { > > reg = <0>; > > -- > > 2.7.4 _______________________________________________ devel mailing list devel@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel