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] [thread-next>] [day] [month] [year] [list]
Message-ID: <wdn2rtfahf3iu6rsgxm6ctfgft7bawtp6vzhgn7dffd54i72lu@r4v5lizhae57>
Date: Tue, 5 Nov 2024 11:33:33 +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 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.

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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ