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 > >> Sent: Wednesday, February 8, 2023 5:28 PM > >> To: Thierry Reding > >> Cc: Prathamesh Shete ; Jonathan Hunter > >> ; 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 > >> > >> 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