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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ff191ee3-bacc-48ed-86a8-2e60ebecc391@kernel.org>
Date: Mon, 4 Nov 2024 12:37:24 +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: [RFC PATCH] dt-bindings: can: convert tcan4x5x.txt to DT schema

On 04/11/2024 11:54, Sean Nyekjaer wrote:
> 
> diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> index 9ff52b8b3063..0fc37b10e899 100644
> --- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> +++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> @@ -50,7 +50,7 @@ properties:
>      maxItems: 1
> 
>    bosch,mram-cfg:
> -    $ref: bosch,m_can.yaml#
> +    $ref: /schemas/net/can/bosch,m_can.yaml#/properties/bosch,mram-cfg
> 
>    spi-max-frequency:
>      description:
> 
> Still results in:
> % make dt_binding_check DT_SCHEMA_FILES=ti,tcan4x5x.yaml
>   SCHEMA  Documentation/devicetree/bindings/processed-schema.json
>   CHKDT   Documentation/devicetree/bindings
> Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml: properties:bosch,mram-cfg: 'anyOf' conditional failed, one must be fixed:
>         'description' is a dependency of '$ref'
>         '/schemas/net/can/bosch,m_can.yaml#/properties/bosch,mram-cfg' does not match 'types.yaml#/definitions/'
>                 hint: A vendor property needs a $ref to types.yaml
>         '/schemas/net/can/bosch,m_can.yaml#/properties/bosch,mram-cfg' does not match '^#/(definitions|\\$defs)/'
>                 hint: A vendor property can have a $ref to a a $defs schema
>         hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
>         from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#

Yeah, probably not much benefits of referencing other schema. Just copy
the property.

> 
>>
>>>
>>> Any hints to share a property?
>>>
>>>  .../devicetree/bindings/net/can/tcan4x5x.txt  | 48 ---------
>>>  .../bindings/net/can/ti,tcan4x5x.yaml         | 97 +++++++++++++++++++
>>>  2 files changed, 97 insertions(+), 48 deletions(-)
>>>  delete mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt
>>>  create mode 100644 Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
>>>
>>
>> ...
>>
>>> diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
>>> new file mode 100644
>>> index 000000000000..62c108fac6b3
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
>>> @@ -0,0 +1,97 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/can/ti,tcan4x5x.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Texas Instruments TCAN4x5x CAN Controller
>>> +
>>> +maintainers:
>>> +  - Marc Kleine-Budde <mkl@...gutronix.de>
>>> +
>>> +allOf:
>>> +  - $ref: can-controller.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    oneOf:
>>> +      - enum:
>>> +          - ti,tcan4552
>>> +          - ti,tcan4553
>>> +          - ti,tcan4x5x
>>
>> That's not really what old binding said.
>>
>> It said for example:
>> "ti,tcan4552", "ti,tcan4x5x"
>>
>> Which is not allowed above. You need list. Considering there are no
>> in-tree users of ti,tcan4x5x alone, I would allow only lists followed by
>> ti,tcan4x5x. IOW: disallow ti,tcan4x5x alone.
>>
>> Mention this change to the binding in the commit message.
>>
>>
> 
> I would prefer to not change anything other that doing the conversion to
> DT schema.

Well, above you changed a lot :/, but fine - wildcard can stay. But
anyway compatible list has to be fixed.

> 
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  interrupts:
>>> +    maxItems: 1
>>> +
>>> +  clocks:
>>> +    maxItems: 1
>>> +
>>> +  vdd-supply:
>>> +    description: Regulator that powers the CAN controller.
>>> +
>>> +  xceiver-supply:
>>> +    description: Regulator that powers the CAN transceiver.
>>
>> You need to mention all changes done to the binding in the commit msg.
>>
> Is this a change? It existed in the old doc aswell...

Where? I pointed out that this is a change. I cannot find it. If you
disagree, please point to the patch. And it's tricky for me to prove my
point - to show that such thing did not exist...


Two more comments below:

> 
>>> +
>>> +  reset-gpios:
>>> +    description: Hardwired output GPIO. If not defined then software reset.
>>> +    maxItems: 1
>>> +
>>> +  device-state-gpios:
>>> +    description: Input GPIO that indicates if the device is in a sleep state or if the device is active.

Please wrap code according to coding style (checkpatch is not a coding
style description, but only a tool).


>>> +      Not available with tcan4552/4553.

Then you need if:then: inside allOf: block disallowing it for this variant.
https://elixir.bootlin.com/linux/v5.19/source/Documentation/devicetree/bindings/example-schema.yaml#L223



Best regards,
Krzysztof