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@xxxxxxx>
> >
> >
> > ---
[...]
> > +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:
signature.asc
Description: PGP signature
