Re: [PATCH] dt-bindings: can: convert tcan4x5x.txt to DT schema

[Sean Nyekjaer]

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