Hi Geert,

Thank you for the review.

On Tue, Jul 20, 2021 at 11:21 AM Geert Uytterhoeven
[off-list ref] wrote:

Hi Prabhakar,

On Mon, Jul 19, 2021 at 4:39 PM Lad Prabhakar
[off-list ref] wrote:

quoted

Add CANFD binding documentation for Renesas RZ/G2L SoC.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>

Thanks for your patch!

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Just some bikeshedding on the exact naming below ;-)

quoted

--- a/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml
+++ b/Documentation/devicetree/bindings/net/can/renesas,rcar-canfd.yaml

@@ -91,6 +92,59 @@ required:
   - channel0
   - channel1

+if:
+  properties:
+    compatible:
+      contains:
+        enum:
+          - renesas,rzg2l-canfd
+then:
+  properties:
+    interrupts:
+      items:
+        - description: CAN global error interrupt
+        - description: CAN receive FIFO interrupt
+        - description: CAN0 error interrupt
+        - description: CAN0 transmit interrupt
+        - description: CAN0 transmit/receive FIFO receive completion interrupt
+        - description: CAN1 error interrupt
+        - description: CAN1 transmit interrupt
+        - description: CAN1 transmit/receive FIFO receive completion interrupt
+
+    interrupt-names:
+      items:
+        - const: g_error
+        - const: g_rx_fifo
+        - const: can0_error

s/error/err/?

quoted

+        - const: can0_tx
+        - const: can0_tx_rx_fifo_receive_completion
+        - const: can1_error
+        - const: can1_tx
+        - const: can1_tx_rx_fifo_receive_completion

s/receive/rx/?

Some are also a bit long to type.
Perhaps use naming closer to the User's Manual?

INTRCANGERR => g_err
INTRCANGRECC => g_recc
INTRCAN0ERR => ch0_err
INTRCAN0REC => ch0_rec
INTRCAN0TRX => ch0_trx
INTRCAN1ERR => ch1_err
INTRCAN1REC => ch1_rec
INTRCAN1TRX => ch1_trx

These do not have "_int" suffixes...

Agreed thanks for the input.

quoted

+
+    resets:
+      items:
+        - description: CANFD_RSTP_N
+        - description: CANFD_RSTC_N
+
+  required:
+    - interrupt-names
+else:
+  properties:
+    interrupts:
+      items:
+        - description: Channel interrupt
+        - description: Global interrupt
+
+    interrupt-names:
+      items:
+        - const: ch_int
+        - const: g_int

... and these do have "_int" suffixes.

indeed

Cheers,
Prabhakar

quoted

+
+    resets:
+      items:
+        - description: CANFD reset
+
 unevaluatedProperties: false

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds