Re: [PATCH v2 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450

["Jonathan Neuschäfer" <j.neuschaefer@gmx.net>]

[-- Attachment #1: Type: text/plain, Size: 4743 bytes --]

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:
> > 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>
> > 
> > 
> > ---
[...]
> > +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".


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


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


> > +  "^mux_.*$":
> 
> Use '-' rather than '_' and the '.*' is not necessary.
> 
> "^mux-"

Ok

> 
> > +    if:
> 
> Don't need this.

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


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


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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]