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

[Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>]

Hi Rob,

Thanks for your time in reviewing this.

On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring <robh@xxxxxxxxxx> wrote:
> On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:
>> Add a devicetree binding documentation for the mt7621 driver.
>
> Bindings are for h/w, not a driver.

You are totally right, my english is not my best as you can see :-).
I'll fix this
for v2.

>
>> Signed-off-by: Sergio Paracuellos <sergio.paracuellos@xxxxxxxxx>
>> Reviewed-by: NeilBrown <neil@xxxxxxxxxx>
>
> Space              ^
>

Actually, this is deliberate, so it will not change.

>> ---
>>  .../bindings/gpio/mediatek,mt7621-gpio.txt         | 68 ++++++++++++++++++++++
>>  1 file changed, 68 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> new file mode 100644
>> index 0000000..30d8a02
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/mediatek,mt7621-gpio.txt
>> @@ -0,0 +1,68 @@
>> +Mediatek SoC GPIO controller bindings
>> +
>> +The IP core used inside these SoCs has 3 banks of 32 GPIOs each.
>> +The registers of all the banks are interwoven inside one single IO range.
>> +We load one GPIO controller instance per bank. To make this possible
>> +we support 2 types of nodes. The parent node defines the memory I/O range and
>> +has 3 children each describing a single bank. Also the GPIO controller can receive
>> +interrupts on any of the GPIOs, either edge or level. It then interrupts the CPU
>> +using GIC INT12.
>> +
>> +Required properties for the top level node:
>> +- compatible:
>> +  - "mediatek,mt7621-gpio" for Mediatek controllers
>> +- reg : Physical base address and length of the controller's registers
>> +- interrupt-parent : phandle of the parent interrupt controller.
>> +- interrupts : Interrupt specifier for the controllers interrupt.
>> +- interrupt-controller : Mark the device node as an interrupt controller.
>> +- #interrupt-cells : Should be 2. The first cell defines the interrupt number.
>> +   The second cell bits[3:0] is used to specify trigger type as follows:
>> +     - 1 = low-to-high edge triggered.
>> +     - 2 = high-to-low edge triggered.
>> +     - 4 = active high level-sensitive.
>> +     - 8 = active low level-sensitive.
>
> Just refer to the common definition.

ok, thanks. I will.

>
>> +
>> +
>> +Required properties for the GPIO bank node:
>> +- compatible:
>> +  - "mediatek,mt7621-gpio-bank" for Mediatek banks
>> +- #gpio-cells : Should be two. The first cell is the GPIO pin number and the
>
> So interrupt numbers and gpio numbers are different? 0-95 and 3x 0-31
>
> That doesn't seem ideal.

Yes, that's true. Actually this is one of the things that has been
changed for v2.

>
>> +   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. ..."

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?

Thanks in advance.

Best regards,
    Sergio Paracuellos
>> +
>> +Example:
>> +     gpio@600 {
>> +             #address-cells = <1>;
>> +             #size-cells = <0>;
>> +
>> +             compatible = "mediatek,mt7621-gpio";
>> +             reg = <0x600 0x100>;
>> +
>> +             interrupt-parent = <&gic>;
>> +             interrupts = <GIC_SHARED 12 IRQ_TYPE_LEVEL_HIGH>;
>> +             interrupt-controller;
>> +             #interrupt-cells = <2>;
>> +
>> +             gpio0: bank@0 {
>> +                     reg = <0>;
>> +                     compatible = "mediatek,mt7621-gpio-bank";
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +             };
>> +
>> +             gpio1: bank@1 {
>> +                     reg = <1>;
>> +                     compatible = "mediatek,mt7621-gpio-bank";
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +             };
>> +
>> +             gpio2: bank@2 {
>> +                     reg = <2>;
>> +                     compatible = "mediatek,mt7621-gpio-bank";
>> +                     gpio-controller;
>> +                     #gpio-cells = <2>;
>> +             };
>> +     };
>> --
>> 2.7.4
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html