Hi Rob,

Thanks for your time in reviewing this.

On Tue, Jun 12, 2018 at 10:56 PM, Rob Herring [off-list ref] wrote:

On Sat, Jun 02, 2018 at 09:30:10AM +0200, Sergio Paracuellos wrote:

quoted

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.

quoted

Signed-off-by: Sergio Paracuellos <sergio.paracuellos@gmail.com>
Reviewed-by: NeilBrown <neil@brown.name>

Space              ^

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

quoted

---
 .../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.

quoted

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

quoted

+   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

quoted

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