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
