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