by Rob Herring (Arm)

[permalink] [raw]

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

On Tue, 07 Dec 2021 22:08:19 +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 <[email protected]>
>
>
> ---
> v2:
> - Move GPIO into subnodes
> - Improve use of quotes
> - Remove unnecessary minItems/maxItems lines
> - Remove "phandle: true"
> - Use separate prefixes for pinmux and pincfg nodes
> - Add nuvoton,interrupt-map property
> - Make it possible to set pinmux to GPIO explicitly
>
> v1:
> - https://lore.kernel.org/lkml/[email protected]/
> ---
> .../pinctrl/nuvoton,wpcm450-pinctrl.yaml | 190 ++++++++++++++++++
> 1 file changed, 190 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
>

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.example.dt.yaml: pinctrl@b8003000: gpio@0:interrupts: [[2, 4, 3, 4, 4, 4]] is too short
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.example.dt.yaml: pinctrl@b8003000: mux_uid: Additional properties are not allowed ('phandle' was unexpected)
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.example.dt.yaml: pinctrl@b8003000: cfg_uid: Additional properties are not allowed ('phandle' was unexpected)
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.example.dt.yaml: pinctrl@b8003000: '#address-cells', '#size-cells' do not match any of the regexes: '^cfg_.*$', '^gpio@.*$', '^mux_.*$', 'pinctrl-[0-9]+'
From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/pinctrl/nuvoton,wpcm450-pinctrl.yaml

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/1564902

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

2021-12-08 13:58:43

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 <[email protected]>
> >
> >
> > ---
[...]
> > +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

Attachments:

(No filename) (4.50 kB)
signature.asc (833.00 B)
Download all attachments