Re: [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node

From: Sergio Paracuellos <sergio.paracuellos@gmail.com>
To: NeilBrown <neil@brown.name>
Cc: gregkh@linuxfoundation.org, driverdev-devel@linuxdriverproject.org
Subject: Re: [PATCH 1/6] staging: mt7621-dts: fix property #interrupt-cells for the gpio node
Date: Wed, 30 May 2018 06:54:29 +0200	[thread overview]
Message-ID: <20180530045429.GA18404@foobar> (raw)
In-Reply-To: <87po1eujcg.fsf@notabene.neil.brown.name>

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 <sergio.paracuellos@gmail.com>
> > ---
> >  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 = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
> >  			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 <neil@brown.name>
> 
> 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