.../devicetree/bindings/net/can/tcan4x5x.txt | 48 ----- .../bindings/net/can/ti,tcan4x5x.yaml | 189 ++++++++++++++++++ 2 files changed, 189 insertions(+), 48 deletions(-) delete mode 100644 Documentation/devicetree/bindings/net/can/tcan4x5x.txt create mode 100644 Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
Convert binding doc tcan4x5x.txt to yaml.
Signed-off-by: Sean Nyekjaer <sean@geanix.com>
---
Changes since rfc:
- 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
.../devicetree/bindings/net/can/tcan4x5x.txt | 48 -----
.../bindings/net/can/ti,tcan4x5x.yaml | 189 ++++++++++++++++++
2 files changed, 189 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/tcan4x5x.txt b/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
deleted file mode 100644
index 20c0572c9853..000000000000
--- a/Documentation/devicetree/bindings/net/can/tcan4x5x.txt
+++ /dev/null
@@ -1,48 +0,0 @@
-Texas Instruments TCAN4x5x CAN Controller
-================================================
-
-This file provides device node information for the TCAN4x5x interface contains.
-
-Required properties:
- - compatible:
- "ti,tcan4552", "ti,tcan4x5x"
- "ti,tcan4553", "ti,tcan4x5x" or
- "ti,tcan4x5x"
- - reg: 0
- - #address-cells: 1
- - #size-cells: 0
- - spi-max-frequency: Maximum frequency of the SPI bus the chip can
- operate at should be less than or equal to 18 MHz.
- - interrupt-parent: the phandle to the interrupt controller which provides
- the interrupt.
- - interrupts: interrupt specification for data-ready.
-
-See Documentation/devicetree/bindings/net/can/bosch,m_can.yaml for additional
-required property details.
-
-Optional properties:
- - reset-gpios: Hardwired output GPIO. If not defined then software
- reset.
- - device-state-gpios: Input GPIO that indicates if the device is in
- a sleep state or if the device is active. Not
- available with tcan4552/4553.
- - device-wake-gpios: Wake up GPIO to wake up the TCAN device. Not
- available with tcan4552/4553.
- - wakeup-source: Leave the chip running when suspended, and configure
- the RX interrupt to wake up the device.
-
-Example:
-tcan4x5x: tcan4x5x@0 {
- compatible = "ti,tcan4x5x";
- reg = <0>;
- #address-cells = <1>;
- #size-cells = <1>;
- 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;
-};
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..0351e5c04230
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
@@ -0,0 +1,189 @@
+# 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@pengutronix.de>
+
+properties:
+ compatible:
+ oneOf:
+ - items:
+ - enum:
+ - ti,tcan4552
+ - const: ti,tcan4x5x
+ - items:
+ - enum:
+ - ti,tcan4553
+ - const: ti,tcan4x5x
+ - items:
+ - enum:
+ - 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: |
+ 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: |
+ 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
+
+required:
+ - compatible
+ - reg
+ - interrupts
+ - clocks
+ - bosch,mram-cfg
+
+additionalProperties: false
+
+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";
+ 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>;
+ reset-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
+ wakeup-source;
+ };
+ };
--
2.46.2On 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@geanix.com>
> ---
> Changes since rfc:
That's a v2. RFC was v1. *ALWAYS*.
Try by yourself:
b4 diff 20241104125342.1691516-1-sean@geanix.com
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
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@geanix.com>
> > ---
> > Changes since rfc:
>
> That's a v2. RFC was v1. *ALWAYS*.
> Try by yourself:
> b4 diff 20241104125342.1691516-1-sean@geanix.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
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@geanix.com>
>>> ---
>>> Changes since rfc:
>>
>> That's a v2. RFC was v1. *ALWAYS*.
>> Try by yourself:
>> b4 diff 20241104125342.1691516-1-sean@geanix.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
Hi Krzysztof,
On Tue, Nov 05, 2024 at 12:40:07PM +0100, Krzysztof Kozlowski wrote:
> 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.
[...]
> >
> > 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.
>
There are no users for this in-kernel, but out-of-tree there is :)
> >
[...]
> > 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.
>
>
Schema check will fail, but driver wize it will work just fine.
Agree that is kinda broken.
If I have time I can try to fix that later.
Please explain one more time for me. Is this a comment on the if
sentence or the broken behavior of the driver?
> >
> >>
> >>> +
> >>> +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.
Replaced with:
unevaluatedProperties: false
>
>
> Best regards,
> Krzysztof
>
/Sean
On 05/11/2024 13:24, Sean Nyekjaer wrote: >>> >>> 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. >> >> > > Schema check will fail, but driver wize it will work just fine. Schema will not fail. That's the problem - no errors will be ever reported. The entire point of the schema, in contrast to TXT, is to detect errors and that ridiculous wildcard used as front compatible affects/reduces detection. > Agree that is kinda broken. > If I have time I can try to fix that later. No, the fix is to drop the wildcard alone, as I said in your RFC. > > Please explain one more time for me. Is this a comment on the if > sentence or the broken behavior of the driver? This is just generic comment, nothing to change here because you decided not to fix that wildcard from old binding. Best regards, Krzysztof
On Tue, Nov 05, 2024 at 01:41:26PM +0100, Krzysztof Kozlowski wrote:
[...]
> > Schema check will fail, but driver wize it will work just fine.
>
> Schema will not fail. That's the problem - no errors will be ever
> reported. The entire point of the schema, in contrast to TXT, is to
> detect errors and that ridiculous wildcard used as front compatible
> affects/reduces detection.
NOW I get it :)
diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
index f1d18a5461e0..4fb5e5e80a03 100644
--- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
+++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
@@ -169,7 +169,7 @@ examples:
#size-cells = <0>;
can@0 {
- compatible = "ti,tcan4552", "ti,tcan4x5x";
+ compatible = "ti,tcan4552";
reg = <0>;
clocks = <&can0_osc>;
pinctrl-names = "default";
Would result in a schema check fail, but the driver will never be probed.
>
> > Agree that is kinda broken.
> > If I have time I can try to fix that later.
>
> No, the fix is to drop the wildcard alone, as I said in your RFC.
@Mark, would you be okay with fixing the wildcard in this series?
We have some out-of-tree dtb's that will need fixing, but I get it would be
prefered to get this fixed.
>
> >
> > Please explain one more time for me. Is this a comment on the if
> > sentence or the broken behavior of the driver?
>
> This is just generic comment, nothing to change here because you decided
> not to fix that wildcard from old binding.
Thanks for the clarification!
@Mark, @Krzysztof: What to do from here?
/Sean
On 05/11/2024 13:59, Sean Nyekjaer wrote:
> On Tue, Nov 05, 2024 at 01:41:26PM +0100, Krzysztof Kozlowski wrote:
>
> NOW I get it :)
>
> diff --git a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> index f1d18a5461e0..4fb5e5e80a03 100644
> --- a/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> +++ b/Documentation/devicetree/bindings/net/can/ti,tcan4x5x.yaml
> @@ -169,7 +169,7 @@ examples:
> #size-cells = <0>;
>
> can@0 {
> - compatible = "ti,tcan4552", "ti,tcan4x5x";
> + compatible = "ti,tcan4552";
> reg = <0>;
> clocks = <&can0_osc>;
> pinctrl-names = "default";
>
> Would result in a schema check fail, but the driver will never be probed.
>
>>
Yeah, but we dont' talk about this case.
>>> Agree that is kinda broken.
>>> If I have time I can try to fix that later.
>>
>> No, the fix is to drop the wildcard alone, as I said in your RFC.
>
> @Mark, would you be okay with fixing the wildcard in this series?
> We have some out-of-tree dtb's that will need fixing, but I get it would be
> prefered to get this fixed.
Out of tree DTB will not need any fixes per-se. They will work 100%
fine. They will however report dtbs_check warnings, but before there was
no DT schema validation for them, so you replace one warning into
another warning.
You can of course improve out of tree users by dropping all warnings,
but that's kind of optional. The point is that nothing gets worse.
>
>>
>>>
>>> Please explain one more time for me. Is this a comment on the if
>>> sentence or the broken behavior of the driver?
>>
>> This is just generic comment, nothing to change here because you decided
>> not to fix that wildcard from old binding.
>
> Thanks for the clarification!
>
> @Mark, @Krzysztof: What to do from here?
I will give Rb tag for next version fixing commented issues regardless
whether you drop usage of the wildcard-like 4x5x compatible alone. IOW,
I will be fine with pure conversion and keeping 4x5x, even though I
would prefer to improve the binding based on arguments before: there
will be no changes needed for in-kernel users, no out-of-tree users will
be affected, no breakage and using wildcard compatible alone is
discouraged. Sometimes strongly discouraged, depending on the case.
Best regards,
Krzysztof
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@geanix.com>
> > > ---
> > > Changes since rfc:
> >
> > That's a v2. RFC was v1. *ALWAYS*.
> > Try by yourself:
> > b4 diff 20241104125342.1691516-1-sean@geanix.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
© 2016 - 2024 Red Hat, Inc.