[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <mtuev7pve5ltr6vvknp2bwtwg2m7mzxduzshzbr7y3i7mwbzy6@qjbdjyb56nrv>
Date: Tue, 5 Nov 2024 13:24:26 +0100
From: Sean Nyekjaer <sean@...nix.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Marc Kleine-Budde <mkl@...gutronix.de>,
Vincent Mailhol <mailhol.vincent@...adoo.fr>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
Paolo Abeni <pabeni@...hat.com>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-can@...r.kernel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] dt-bindings: can: convert tcan4x5x.txt to DT schema
Hi Krzysztof,
On Tue, Nov 05, 2024 at 12:40:07PM +0100, Krzysztof Kozlowski wrote:
> On 05/11/2024 11:33, Sean Nyekjaer wrote:
> > On Tue, Nov 05, 2024 at 10:16:30AM +0100, Krzysztof Kozlowski wrote:
> >> On Mon, Nov 04, 2024 at 01:53:40PM +0100, Sean Nyekjaer wrote:
> >>> Convert binding doc tcan4x5x.txt to yaml.
[...]
> >
> > Gives:
> > /linux/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.example.dtb: can@0: compatible: ['ti,tcan4x5x'] is valid under each of {'items': [{'enum': ['ti,tcan4553', 'ti,tcan4x5x']}], 'type': 'array', 'minItems': 1, 'maxItems': 1}, {'items': [{'const': 'ti,tcan4x5x'}], 'type': 'array', 'minItems': 1, 'maxItems': 1}, {'items': [{'enum': ['ti,tcan4552', 'ti,tcan4x5x']}], 'type': 'array', 'minItems': 1, 'maxItems': 1}
> > from schema $id: http://devicetree.org/schemas/net/can/ti,tcan4x5x.yaml#
> > /linux/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.example.dtb: can@0: compatible: 'oneOf' conditional failed, one must be fixed:
> > ['ti,tcan4552', 'ti,tcan4x5x'] is too long
> > 'ti,tcan4552' is not one of ['ti,tcan4553', 'ti,tcan4x5x']
> > 'ti,tcan4x5x' was expected
> > from schema $id: http://devicetree.org/schemas/net/can/ti,tcan4x5x.yaml#
> >
> > I can understand the original binding is broken.
> > I kinda agree with Marc that we cannot break things for users of this.
>
> Hm? Absolutely nothing would get broken for users. I don't understand
> these references or false claims.
>
There are no users for this in-kernel, but out-of-tree there is :)
> >
[...]
> > OK
> >
> >>> + Enable CAN remote wakeup.
> >>> +
> >>> +allOf:
> >>> + - $ref: can-controller.yaml#
> >>> + - $ref: /schemas/spi/spi-peripheral-props.yaml#
> >>> + - if:
> >>> + properties:
> >>> + compatible:
> >>> + contains:
> >>> + enum:
> >>> + - ti,tcan4552
> >>> + - ti,tcan4553
> >>> + then:
> >>> + properties:
> >>> + device-state-gpios: false
> >>> + device-wake-gpios: false
> >>
> >> Heh, this is a weird binding. It should have specific compatibles for
> >> all other variants because above does not make sense. For 4552 one could
> >> skip front compatible and use only fallback, right? And then add these
> >> properties bypassing schema check. I commented on this already that
> >> original binding is flawed and should be fixed, but no one cares then I
> >> also don't care.
> >
> > To me it looks like the example you linked:
> > https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L223
>
> Yes, it looks, that's not the point.
>
> >
> > If you use fallback for a 4552 then it would enable the use of the
> > optional pins device-state-gpios and device-wake-gpios. But the chip
> > doesn't have those so the hw guys would connect them and they won't
> > be in the DT.
> >
> > Honestly I'm confused :/
>
> What stops anyone to use tcan4x5x ALONE for 4552? Nothing. And that's
> the problem here.
>
>
Schema check will fail, but driver wize it will work just fine.
Agree that is kinda broken.
If I have time I can try to fix that later.
Please explain one more time for me. Is this a comment on the if
sentence or the broken behavior of the driver?
> >
> >>
> >>> +
> >>> +required:
> >>> + - compatible
> >>> + - reg
> >>> + - interrupts
> >>> + - clocks
> >>> + - bosch,mram-cfg
> >>> +
> >>> +additionalProperties: false
> >>
> >> Implement feedback. Nothing changed here.
> >>
> >
> > Uh? feedback?
>
> Yeah, CAREFULLY previous review and respond to all comments or implement
> all of them (or any combination). If you leave one comment ignored, it
> will mean reviewer has to do same work twice. That's very discouraging
> and wasteful of my time.
Replaced with:
unevaluatedProperties: false
>
>
> Best regards,
> Krzysztof
>
/Sean
Powered by blists - more mailing lists