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.paracuel...@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 <n...@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
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel

Reply via email to