Message-ID: <60901c39-b649-4a20-a06a-7faa7ddc9346@kernel.org>
Date: Tue, 5 Nov 2024 12:40:07 +0100
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Sean Nyekjaer <sean@...nix.com>
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

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.
>>>
>>> Signed-off-by: Sean Nyekjaer <sean@...nix.com>
>>> ---
>>> Changes since rfc:
>>
>> That's a v2. RFC was v1. *ALWAYS*.
>> Try by yourself:
>> b4 diff 20241104125342.1691516-1-sean@...nix.com
>>
>> Works? No. Should work? Yes.
>>
>>
> 
> Ok. Good to know RFC cannot be used...
> Next version would need to be? In order to fix this?
> 
> I have enrolled my patch into b4, next verison will be v2 ;)
> 

ok

>>>   - Tried to re-add ti,tcan4x5x wildcard
>>>   - Removed xceiver and vdd supplies (copy paste error)
>>>   - Corrected max SPI frequency
>>>   - Copy pasted bosch,mram-cfg from bosch,m_can.yaml
>>>   - device-state-gpios and device-wake-gpios only available for tcan4x5x
>>
>> ...
>>
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - items:
>>> +          - enum:
>>> +              - ti,tcan4552
>>> +          - const: ti,tcan4x5x
>>> +      - items:
>>> +          - enum:
>>> +              - ti,tcan4553
>>
>> Odd syntax. Combine these two into one enum.
>>
>>> +          - const: ti,tcan4x5x
>>> +      - items:
>>
>> Drop items.
>>
>>> +          - enum:
>>
>> ... and drop enum. That's just const or do you already plan to add here
>> entries?
> 
> Honestly I'm struggling a bit with the syntax and I feel the feedback is containing
> a lot of implicit terms :)
> 
> Something like:
> properties:
>   compatible:
>     oneOf:
>       - items:
>           - enum:
>               - ti,tcan4552
>               - ti,tcan4x5x
>       - items:
>           - enum:
>               - ti,tcan4553
>               - ti,tcan4x5x

No, this won't work. Just use enum for the first entry. enum stands for
enumerate, kind of like C enum, so one of many.

>       - const: ti,tcan4x5x
> 
> 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.

> 
>>
>>> +              - ti,tcan4x5x
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +    description: The GPIO parent interrupt.
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  reset-gpios:
>>> +    description: Hardwired output GPIO. If not defined then software reset.
>>> +    maxItems: 1
>>> +
>>> +  device-state-gpios:
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
>> Didn't you get this comment alerady?
>>
> 
> No, but I have removed the '|'
> 
>>> +      Input GPIO that indicates if the device is in a sleep state or if the
>>> +      device is active. Not available with tcan4552/4553.
>>> +    maxItems: 1
>>> +
>>> +  device-wake-gpios:
>>> +    description: |
>>> +      Wake up GPIO to wake up the TCAN device.
>>> +      Not available with tcan4552/4553.
>>> +    maxItems: 1
>>> +
>>> +  bosch,mram-cfg:
>>> +    description: |
>>> +      Message RAM configuration data.
>>> +      Multiple M_CAN instances can share the same Message RAM
>>> +      and each element(e.g Rx FIFO or Tx Buffer and etc) number
>>> +      in Message RAM is also configurable, so this property is
>>> +      telling driver how the shared or private Message RAM are
>>> +      used by this M_CAN controller.
>>> +
>>> +      The format should be as follows:
>>> +      <offset sidf_elems xidf_elems rxf0_elems rxf1_elems rxb_elems txe_elems txb_elems>
>>> +      The 'offset' is an address offset of the Message RAM where
>>> +      the following elements start from. This is usually set to
>>> +      0x0 if you're using a private Message RAM. The remain cells
>>> +      are used to specify how many elements are used for each FIFO/Buffer.
>>> +
>>> +      M_CAN includes the following elements according to user manual:
>>> +      11-bit Filter	0-128 elements / 0-128 words
>>> +      29-bit Filter	0-64 elements / 0-128 words
>>> +      Rx FIFO 0		0-64 elements / 0-1152 words
>>> +      Rx FIFO 1		0-64 elements / 0-1152 words
>>> +      Rx Buffers	0-64 elements / 0-1152 words
>>> +      Tx Event FIFO	0-32 elements / 0-64 words
>>> +      Tx Buffers	0-32 elements / 0-576 words
>>> +
>>> +      Please refer to 2.4.1 Message RAM Configuration in Bosch
>>> +      M_CAN user manual for details.
>>> +    $ref: /schemas/types.yaml#/definitions/int32-array
>>> +    items:
>>> +      - description: The 'offset' is an address offset of the Message RAM where
>>> +          the following elements start from. This is usually set to 0x0 if
>>> +          you're using a private Message RAM.
>>> +        default: 0
>>> +      - description: 11-bit Filter 0-128 elements / 0-128 words
>>> +        minimum: 0
>>> +        maximum: 128
>>> +      - description: 29-bit Filter 0-64 elements / 0-128 words
>>> +        minimum: 0
>>> +        maximum: 64
>>> +      - description: Rx FIFO 0 0-64 elements / 0-1152 words
>>> +        minimum: 0
>>> +        maximum: 64
>>> +      - description: Rx FIFO 1 0-64 elements / 0-1152 words
>>> +        minimum: 0
>>> +        maximum: 64
>>> +      - description: Rx Buffers 0-64 elements / 0-1152 words
>>> +        minimum: 0
>>> +        maximum: 64
>>> +      - description: Tx Event FIFO 0-32 elements / 0-64 words
>>> +        minimum: 0
>>> +        maximum: 32
>>> +      - description: Tx Buffers 0-32 elements / 0-576 words
>>> +        minimum: 0
>>> +        maximum: 32
>>> +    minItems: 1
>>> +
>>> +  spi-max-frequency:
>>> +    description:
>>> +      Must be half or less of "clocks" frequency.
>>> +    maximum: 18000000
>>> +
>>> +  wakeup-source:
>>> +    $ref: /schemas/types.yaml#/definitions/flag
>>> +    description: |
>>
>> Do not need '|' unless you need to preserve formatting.
>>
> 
> 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.


> 
>>
>>> +
>>> +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.


Best regards,
Krzysztof

Powered by blists - more mailing lists