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@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.
>
>
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
