On Sun, Mar 26, 2023 at 02:19:45PM +0200, Krzysztof Kozlowski wrote:

On 23/03/2023 15:11, Thierry Reding wrote:

quoted

On Wed, Mar 08, 2023 at 01:24:04PM +0100, Krzysztof Kozlowski wrote:

quoted

On 08/03/2023 12:45, Prathamesh Shete wrote:

quoted

quoted

-----Original Message-----
From: Krzysztof Kozlowski <redacted>
Sent: Wednesday, February 8, 2023 5:28 PM
To: Thierry Reding <redacted>
Cc: Prathamesh Shete <redacted>; Jonathan Hunter
[off-list ref]; 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
[off-list ref]
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:

quoted

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

quoted

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.

quoted

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... =)

quoted

quoted

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.

One problem is allowing characters here which are not allowed. Second
problem is reluctance to change it with argument "existing bindings also
have this problem". It's explanation like "there is already bug like
this, so I am allowed to add similar one".

This is not a bug that we're trying to replicate. We're basing this
binding on a existing bindings that were already reviewed upstream a
long time ago. It uses a shared binding that's in use by these other
bindings, so making any changes to this new binding means either the
other ones need to be changed as well or we can't reuse the existing
shared binding.

Now third is that defining properties in allOf is not the style we want
to have, because it does not work with additionalProperties and is
difficult to read. Again using argument "existing code also does like
this" is a very poor argument.

As far as I can tell, it does work as expected in this case because
we're not actually adding any *new* properties in the allOf/if branches.
If we were, then yes, we would need to use unevaluatedProperties and
that can get complicated. But again, in this case we're merely
overriding existing properties with more specific values, which means
that both the standard binding applies and then things are narrowed down
by the values defined for each compatible.

Thierry