Re: [PATCH 2/2] dt-bindings: document gpio-mt7621 bindings

[Sergio Paracuellos <sergio.paracuellos@gmail.com>]

On Wed, Jun 13, 2018 at 01:28:35PM -0600, Rob Herring wrote:
> On Wed, Jun 13, 2018 at 10:23 AM, Sergio Paracuellos
> <sergio.paracuellos@gmail.com> wrote:
> > Hi Rob,
> >
> > Thanks for your time in reviewing this.
> >
> > On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@kernel.org> wrote:
> >> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
> >>> Add a devicetree binding documentation for the mt7621 driver.
> >>
> 
> >>> +   second cell specifies GPIO flags, as defined in <dt-bindings/gpio/gpio.h>.
> >>> +   Only the GPIO_ACTIVE_HIGH and GPIO_ACTIVE_LOW flags are supported.
> >>> +- gpio-controller : Marks the device node as a GPIO controller.
> >>> +- reg : The id of the bank that the node describes.
> >>
> >> I'd prefer to not have banks defined in DT. Do you have a variable
> >> number or resources that are per bank? If not, then you don't need them.
> >
> > Mmmm, That's what I understood from documentation:
> >
> > "Some system-on-chips (SoCs) use the concept of GPIO banks. ...
> > Usually each such bank is
> > exposed in the device tree as an individual gpio-controller node. ..."
> 
> This should be conditioned on being able to divide up the registers by
> bank which seems like you can't. Or there's the case like the DW GPIO
> block and the number of banks is configurable.

I see. Thanks for clarifing this.

> 
> > If this is not a good approach, could you please me point me out to a
> > device tree example where
> > the correct approach is being used?
> 
> I'm not sure offhand. There are lots of examples of single nodes I'm
> sure. Which ones have banks I haven't a clue. IIRC, there were some
> cases where the bank # was part of the GPIO cells, but I seem to
> recall Linus prefers not having 3 cells.

Ok, so... does the following single node sounds acceptable?

gpio: gpio@600 {
  #gpio-cells = <2>;
  #interrupt-cells = <2>;
  compatible = "mediatek,mt7621-gpio";
  gpio-controller;
  interrupt-controller;
  reg = <0x600 0x60>;
  interrupt-parent = <&gic>;
  interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
  mediatek,gpio-bank-widths = <32 32 32>;
}

Changing definition for "reg" and adding a new one for 
"mediatek,gpio-bank-widths" as follows:

reg: 
  Define the base and range of the address space containing
  the mediatek GPIO controller registers

mediatek,gpio-bank-widths:
  Number of GPIO lines for each bank.  Number of elements must
  correspond to number of banks suggested by the 'reg' property.

Thanks in advance.
> 
> Rob

Best regards,
    Sergio Paracuellos