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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <k5czh4tugbps6fncqyqc4edd4plcrs7uapjo5idaw6lgmktlql@6hgibvhwsfwm>
Date: Tue, 5 Nov 2024 11:57:06 +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

On Tue, Nov 05, 2024 at 11:33:33AM +0100, 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 ;)
> 
> > >   - 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
>       - 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.
> 

Oh, did you mean something like:

properties:
  compatible:
    oneOf:
      - items:
          - enum:
              - ti,tcan4552
              - ti,tcan4553
          - const: ti,tcan4x5x
      - const: ti,tcan4x5x

> > 
> > > +              - 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
> 
> 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 :/
> 
> > 
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +  - interrupts
> > > +  - clocks
> > > +  - bosch,mram-cfg
> > > +
> > > +additionalProperties: false
> > 
> > Implement feedback. Nothing changed here.
> > 
> 
> Uh? feedback?
> 
> > > +
> > > +examples:
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    spi {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        can@0 {
> > > +            compatible = "ti,tcan4x5x";
> > > +            reg = <0>;
> > > +            clocks = <&can0_osc>;
> > > +            pinctrl-names = "default";
> > > +            pinctrl-0 = <&can0_pins>;
> > > +            spi-max-frequency = <10000000>;
> > > +            bosch,mram-cfg = <0x0 0 0 16 0 0 1 1>;
> > > +            interrupt-parent = <&gpio1>;
> > > +            interrupts = <14 IRQ_TYPE_LEVEL_LOW>;
> > > +            device-state-gpios = <&gpio3 21 GPIO_ACTIVE_HIGH>;
> > > +            device-wake-gpios = <&gpio1 15 GPIO_ACTIVE_HIGH>;
> > > +            reset-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
> > > +            wakeup-source;
> > > +        };
> > > +    };
> > > +  - |
> > > +    #include <dt-bindings/gpio/gpio.h>
> > > +    #include <dt-bindings/interrupt-controller/irq.h>
> > > +
> > > +    spi {
> > > +        #address-cells = <1>;
> > > +        #size-cells = <0>;
> > > +
> > > +        can@0 {
> > > +            compatible = "ti,tcan4552","ti,tcan4x5x";
> > 
> > Missing space after ,.
> > 
> 
> Added
> 
> Thanks for the review.
> 
> /Sean
/Sean

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ