On 08/02/2023 12:24, Thierry Reding wrote:

On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:

quoted

quoted

+          type: object
+          additionalProperties:
+            properties:
+              nvidia,pins:
+                description: An array of strings. Each string contains the name
+                  of a pin or group. Valid values for these names are listed
+                  below.

Define properties in top level, which points to the complexity of your
if-else, thus probably this should be split into two bindings. Dunno,
your other bindings repeat this pattern :(

The property itself is already defined in the common schema found in
nvidia,tegra-pinmux-common.yaml and we're overriding this here for each
instance since each has its own set of pins.

This was a compromise to avoid too many bindings. Originally I attempted
to roll all Tegra pinctrl bindings into a single dt-schema, but that
turned out truly horrible =) Splitting this into per-SoC bindings is
already causing a lot of duplication in these files, 

What would be duplicated? Almost eveerything should be coming from
shared binding, so you will have only compatible,
patternProperties(pinmux) and nvidia,pins. And an example. Maybe I miss
something but I would say this would create many but very easy to read
bindings, referencing common pieces.

though splitting
off the common bits into nvidi,tegra-pinmux-common.yaml helps a bit with
that already. Splitting this into per-instance bindings would
effectively duplicate everything but the pin array here, so we kind of
settled on this compromise for Tegra194.

OK, but are you sure it is now readable? You have if:then: with
patternProperties: with additionalProperties: with properties: with
nvidia,pins.

We're taking a bit of a shortcut here already, since technically not all
pins support all the functions listed above. On the other hand, fully
accurately describing per-pin functions would make this a total mess, so
again, we use this simplified representation as a compromise.

That's okay, many platforms do the same way.

(...)

quoted

quoted

+
+        pex_rst_c5_out_state: pinmux-pex-rst-c5-out {
+            pex_rst {

Underscores are not valid in node names.

We have supported underscore in older bindings for historical reasons.
But I suppose for these newer bindings we could disallow them.

Some of the older DTs have a large number of underscores, so I'm not
sure it makes sense to go back and fix those. Maybe something to keep in
mind for when we're done with all the conversions...

I understand, up to you. I think that if such older platform is still
supported/maintained/used, then such cleanups are positive. Underscores
are reported by dtc at W=2, so it is not that critical. But many other
nits like generic node names are being enforced by dtschema, thus if you
want to achieve 0-warning state, at some point you will need to address
these. Of course different question is on what tasks you want to spend
your time. :)

Best regards,
Krzysztof