Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc

[Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>]

On 08/02/2023 12:24, Thierry Reding wrote:
> On Tue, Feb 07, 2023 at 04:33:08PM +0100, Krzysztof Kozlowski wrote:


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

(...)

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