Hello,

On Fri, Dec 10, 2021 at 09:18:01AM -0600, Rob Herring wrote:

On Tue, Dec 07, 2021 at 10:08:19PM +0100, Jonathan Neuschäfer wrote:

quoted

This binding is heavily based on the one for NPCM7xx, because the
hardware is similar. There are some notable differences, however:

- The addresses of GPIO banks are not physical addresses but simple
  indices (0 to 7), because the GPIO registers are not laid out in
  convenient blocks.
- Pinmux settings can explicitly specify that the GPIO mode is used.

Certain pins support blink patterns in hardware. This is currently not
modelled in the DT binding.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>


---

[...]

quoted

+patternProperties:
+  # There are three kinds of subnodes:
+  # 1. a GPIO controller node for each GPIO bank
+  # 2. a pinmux node configures pin muxing for a group of pins (e.g. rmii2)
+  # 3. a pinconf node configures properties of a single pin
+
+  "^gpio@.*$":
+    if:

Not necessary because you can't have a property with '@' in it.

Ok, I'll change it to "^gpio".

quoted

+        interrupts:
+          maxItems: 4

Need to define what each interrupt is.

I think in this case one description for all interrupts would be more
useful, e.g.:

  interrupts:
    maxItems: 4
    description: The interrupts associated with this GPIO bank.

quoted

+        nuvoton,interrupt-map:

Reusing 'interrupt-map' with a different definition bothers me... 

I'm open to tweaking the name, perhaps to something like
nuvoton,gpio-interrupt-map. (Or dropping it entirely — see below.)

quoted

+  "^mux_.*$":

Use '-' rather than '_' and the '.*' is not necessary.

"^mux-"

Ok

quoted

+    if:

Don't need this.

Ok, I'll remove the if/type/then lines throughout the file.

quoted

+        pins:
+          description:
+            A list of pins to configure in certain ways, such as enabling
+            debouncing
+          items:
+            enum: [ gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, gpio7,
+                    gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, gpio14,
+                    gpio15, gpio16, gpio17, gpio18, gpio19, gpio20, gpio21,
+                    gpio22, gpio23, gpio24, gpio25, gpio26, gpio27, gpio28,
+                    gpio29, gpio30, gpio31, gpio32, gpio33, gpio34, gpio35,
+                    gpio36, gpio37, gpio38, gpio39, gpio40, gpio41, gpio42,
+                    gpio43, gpio44, gpio45, gpio46, gpio47, gpio48, gpio49,
+                    gpio50, gpio51, gpio52, gpio53, gpio54, gpio55, gpio56,
+                    gpio57, gpio58, gpio59, gpio60, gpio61, gpio62, gpio63,
+                    gpio64, gpio65, gpio66, gpio67, gpio68, gpio69, gpio70,
+                    gpio71, gpio72, gpio73, gpio74, gpio75, gpio76, gpio77,
+                    gpio78, gpio79, gpio80, gpio81, gpio82, gpio83, gpio84,
+                    gpio85, gpio86, gpio87, gpio88, gpio89, gpio90, gpio91,
+                    gpio92, gpio93, gpio94, gpio95, gpio96, gpio97, gpio98,
+                    gpio99, gpio100, gpio101, gpio102, gpio103, gpio104,
+                    gpio105, gpio106, gpio107, gpio108, gpio109, gpio110,
+                    gpio111, gpio112, gpio113, gpio114, gpio115, gpio116,
+                    gpio117, gpio118, gpio119, gpio120, gpio121, gpio122,
+                    gpio123, gpio124, gpio125, gpio126, gpio127 ]

pattern: '^gpio1?[0-9]{1,2}$'

Indeed, that looks better.

Feel free to tweak it more to limit to 127 if you want.

Ok.

quoted

+      gpio0: gpio@0 {
+        reg = <0>;
+        gpio-controller;
+        #gpio-cells = <2>;
+        interrupts = <2 IRQ_TYPE_LEVEL_HIGH
+                      3 IRQ_TYPE_LEVEL_HIGH
+                      4 IRQ_TYPE_LEVEL_HIGH>;
+        nuvoton,interrupt-map = <0 16 0>;

Based on the example, you don't need this as it is 1:1.

Ah, it's a bad example. The real chip also has this node:


	gpio1: gpio@1 {
		reg = <1>;
		gpio-controller;
		#gpio-cells = <2>;
		interrupts = <5 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-controller;
		nuvoton,interrupt-map = <16 2 8>;
	};

... meaning that bits 16 and 17 in the GPIO controller's interrupt
status register correspond to pins 8 and 9 of GPIO bank 1.

I'm not completely sure this is a good property to have in the
devicetree, I could also hide it in the driver (just like the register
offsets are not part of this binding).



Thanks,
Jonathan