Date:   Mon, 13 Dec 2021 00:38:38 +0100
From:   Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>
To:     Rob Herring <robh@kernel.org>
Cc:     Jonathan =?utf-8?Q?Neusch=C3=A4fer?= <j.neuschaefer@gmx.net>,
        linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
        Linus Walleij <linus.walleij@linaro.org>,
        openbmc@lists.ozlabs.org, Tomer Maimon <tmaimon77@gmail.com>,
        Joel Stanley <joel@jms.id.au>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2 4/8] dt-bindings: pinctrl: Add Nuvoton WPCM450
Message-ID: <YbaH/ny1nI5l7hh0@latitude>
References: <20211207210823.1975632-1-j.neuschaefer@gmx.net>
 <20211207210823.1975632-5-j.neuschaefer@gmx.net>
 <YbNvqTUwi1jzff4D@robh.at.kernel.org>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
        protocol="application/pgp-signature"; boundary="dobARXniaDUcaAko"
Content-Disposition: inline
In-Reply-To: <YbNvqTUwi1jzff4D@robh.at.kernel.org>
Precedence: bulk


--dobARXniaDUcaAko
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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=C3=A4fer wrote:
> > This binding is heavily based on the one for NPCM7xx, because the
> > hardware is similar. There are some notable differences, however:
> >=20
> > - 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.
> >=20
> > Certain pins support blink patterns in hardware. This is currently not
> > modelled in the DT binding.
> >=20
> > Signed-off-by: Jonathan Neusch=C3=A4fer <j.neuschaefer@gmx.net>
> >=20
> >=20
> > ---
[...]
> > +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. r=
mii2)
> > +  # 3. a pinconf node configures properties of a single pin
> > +
> > +  "^gpio@.*$":
> > +    if:
>=20
> Not necessary because you can't have a property with '@' in it.

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


> > +        interrupts:
> > +          maxItems: 4
>=20
> 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:
>=20
> Reusing 'interrupt-map' with a different definition bothers me...=20

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


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

Ok

>=20
> > +    if:
>=20
> 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 enabl=
ing
> > +            debouncing
> > +          items:
> > +            enum: [ gpio0, gpio1, gpio2, gpio3, gpio4, gpio5, gpio6, g=
pio7,
> > +                    gpio8, gpio9, gpio10, gpio11, gpio12, gpio13, gpio=
14,
> > +                    gpio15, gpio16, gpio17, gpio18, gpio19, gpio20, gp=
io21,
> > +                    gpio22, gpio23, gpio24, gpio25, gpio26, gpio27, gp=
io28,
> > +                    gpio29, gpio30, gpio31, gpio32, gpio33, gpio34, gp=
io35,
> > +                    gpio36, gpio37, gpio38, gpio39, gpio40, gpio41, gp=
io42,
> > +                    gpio43, gpio44, gpio45, gpio46, gpio47, gpio48, gp=
io49,
> > +                    gpio50, gpio51, gpio52, gpio53, gpio54, gpio55, gp=
io56,
> > +                    gpio57, gpio58, gpio59, gpio60, gpio61, gpio62, gp=
io63,
> > +                    gpio64, gpio65, gpio66, gpio67, gpio68, gpio69, gp=
io70,
> > +                    gpio71, gpio72, gpio73, gpio74, gpio75, gpio76, gp=
io77,
> > +                    gpio78, gpio79, gpio80, gpio81, gpio82, gpio83, gp=
io84,
> > +                    gpio85, gpio86, gpio87, gpio88, gpio89, gpio90, gp=
io91,
> > +                    gpio92, gpio93, gpio94, gpio95, gpio96, gpio97, gp=
io98,
> > +                    gpio99, gpio100, gpio101, gpio102, gpio103, gpio10=
4,
> > +                    gpio105, gpio106, gpio107, gpio108, gpio109, gpio1=
10,
> > +                    gpio111, gpio112, gpio113, gpio114, gpio115, gpio1=
16,
> > +                    gpio117, gpio118, gpio119, gpio120, gpio121, gpio1=
22,
> > +                    gpio123, gpio124, gpio125, gpio126, gpio127 ]
>=20
> 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 =3D <0>;
> > +        gpio-controller;
> > +        #gpio-cells =3D <2>;
> > +        interrupts =3D <2 IRQ_TYPE_LEVEL_HIGH
> > +                      3 IRQ_TYPE_LEVEL_HIGH
> > +                      4 IRQ_TYPE_LEVEL_HIGH>;
> > +        nuvoton,interrupt-map =3D <0 16 0>;
>=20
> 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 =3D <1>;
		gpio-controller;
		#gpio-cells =3D <2>;
		interrupts =3D <5 IRQ_TYPE_LEVEL_HIGH>;
		interrupt-controller;
		nuvoton,interrupt-map =3D <16 2 8>;
	};

=2E.. 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

--dobARXniaDUcaAko
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmG2h9YACgkQCDBEmo7z
X9smnw/9EREnQfOVwcf3sZmXZKAGJ3NOAE3UMGzyikvU9A0cJLjb5TcMUXW6QosO
2pz4Mf/H8zsGrmFFC/DwC8yO7sMGHckNHFof+/uqgUPJF4pO8cAlUSUqO5QWh/Kz
vE8/gVz5mNOna7mKfpMXUs2vznNvAKNWQlhizEsal8zY3Dcir4+09Za5u08oTDbR
52IAbxsuMC68ULft3NJMeib/Zm839MC38GBs93rNyojNu1yBt7DVkNrxyxo8kt6y
pYNkhTrGKx5p8Yetoax7tKchIvs0k03L7BiabHkimQ7QTZ4XPkjG7vRJ+6PcDVOc
wc25k1ERWWdi1ORwBGLlvC8But6bvhM14NRezRGEwRY2WbY5h6aiHSXFfWtk80VA
9ZXaKiuIbCiRGyksdrq2nUu1E258bifUowCYgB/sp68IOdOLYh4zDpFegztDvUtH
LgfFMXGBMb8Gu1VUOc02wKgmA6tjnzNSr1eiR1uCigUyyoJLqOaHjmCJo2ZHnQNQ
jFih2QcjTh7uN9gyveARd1rLSadidfjIO7SEqTnGNMQTBOYeiq8YIIJ+kOx+FeCe
PikOCQyhN0Q/fptQuuCeMe4991//+UBd9sCxzGpDdEnLz4V0GcQcoPATPBnG7bUE
u2YsJaQeNjSURsnVN/S7qZ2cSxkqIRglFV3W4q6aGtMROEnzRLY=
=iNfO
-----END PGP SIGNATURE-----

--dobARXniaDUcaAko--