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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4077 bytes --]

On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:
> On 08/03/2023 12:45, Prathamesh Shete wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >> Sent: Wednesday, February 8, 2023 5:28 PM
> >> To: Thierry Reding <thierry.reding@gmail.com>
> >> Cc: Prathamesh Shete <pshete@nvidia.com>; Jonathan Hunter
> >> <jonathanh@nvidia.com>; linus.walleij@linaro.org; robh+dt@kernel.org;
> >> krzysztof.kozlowski+dt@linaro.org; devicetree@vger.kernel.org; linux-
> >> tegra@vger.kernel.org; linux-gpio@vger.kernel.org; Suresh Mangipudi
> >> <smangipudi@nvidia.com>
> >> 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,
> 
> So the code might be totally unreadable, but it is inline with existing
> code, thus it should stay unreadable. Great.

I'd say this is very subjective. I personally don't find the current
version hard to read, but that's maybe because I wrote it... =)

> > 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.
> 
> Cleanup should happen before adding new bindings.

I don't recall the exact problems that I ran into last time, but I do
remember that pulling out the common bindings to the very top-level was
the main issue.

If I understand correctly what you're saying, the main problem that
makes this hard to read is the if and else constructs for AON/MAIN
variants on Tegra194/Tegra234. These should be quite easy to pull out
into separate bindings. I'll do that first and then see if there's
anything that could be done to further improve things.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]