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

[Prathamesh Shete <pshete@xxxxxxxxxx>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> Sent: Wednesday, February 8, 2023 5:28 PM
> To: Thierry Reding <thierry.reding@xxxxxxxxx>
> Cc: Prathamesh Shete <pshete@xxxxxxxxxx>; Jonathan Hunter
> <jonathanh@xxxxxxxxxx>; linus.walleij@xxxxxxxxxx; robh+dt@xxxxxxxxxx;
> krzysztof.kozlowski+dt@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx; linux-
> tegra@xxxxxxxxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx; Suresh Mangipudi
> <smangipudi@xxxxxxxxxx>
> Subject: Re: [PATCH 1/3] dt-bindings: pinctrl: tegra234: Add DT binding doc
> 
> External email: Use caution opening links or attachments
> 
> 
> 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.
This is inline with the existing bindings and I think this is the compromise that was reached during review when the bindings were submitted,
offer to rework if a better alternative can be found, but that only makes sense if all the other bindings get changed as well, so I think it'd be good if we can merge in the same format as the existing bindings for now and change all of them later on.
Sent a version v2 addressing all other comments.
Thanks
Prathamesh
> 
> >
> > 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