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@xxxxxxxxxx>
> ---
> Changes since rfc:
That's a v2. RFC was v1. *ALWAYS*.
Try by yourself:
b4 diff 20241104125342.1691516-1-sean@xxxxxxxxxx
Works? No. Should work? Yes.
> - 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?
> + - 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?
> + 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.
> + 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.
> +
> +required:
> + - compatible
> + - reg
> + - interrupts
> + - clocks
> + - bosch,mram-cfg
> +
> +additionalProperties: false
Implement feedback. Nothing changed here.
> +
> +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 ,.
Best regards,
Krzysztof
