Re: [PATCH v2 1/3] dt-bindings: can: fsl,flexcan: add S32G2/S32G3 SoC support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/26/2024 9:19 AM, Krzysztof Kozlowski wrote:
On Mon, Nov 25, 2024 at 06:31:00PM +0200, Ciprian Costea wrote:
From: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>

Add S32G2/S32G3 SoCs compatible strings.

A particularity for these SoCs is the presence of separate interrupts for
state change, bus errors, MBs 0-7 and MBs 8-127 respectively.

Increase maxItems of 'interrupts' to 4 for S32G based SoCs and keep the
same restriction for other SoCs.

Also, as part of this commit, move the 'allOf' after the required
properties to make the documentation easier to read.

Signed-off-by: Ciprian Marian Costea <ciprianmarian.costea@xxxxxxxxxxx>
Reviewed-by: Frank Li <Frank.Li@xxxxxxx>


Hello Krzysztof,

Thanks for your time in reviewing this patch.

You made multiple changes afterwards, which invalidated the review. See
submitting-patches which explain what to do in such case.


I will remove the tag in V3 and add this info in the changelog.

---
  .../bindings/net/can/fsl,flexcan.yaml         | 46 +++++++++++++++++--
  1 file changed, 42 insertions(+), 4 deletions(-)

...

      maxItems: 2
@@ -136,6 +143,37 @@ required:
    - reg
    - interrupts
+allOf:
+  - $ref: can-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const: nxp,s32g2-flexcan
+    then:
+      properties:
+        interrupts:
+          items:
+            - description:
+                Message Buffer interrupt for mailboxes 0-7

Keep it in one line.

I will update in V3.


+            - description:
+                Interrupt indicating that the CAN bus went to Buss Off state

s/Interrupt indicating that//
Buss Off state status?

+            - description:
+                Interrupt indicating that errors were detected on the CAN bus

Error detection?

I will rephrase in V2.


+            - description:
+                Message Buffer interrupt for mailboxes 8-127 (ored)
+        interrupt-names:
+          items:
+            - const: mb_0-7

Choose one: either underscores or hyphens. Keep it consistent in your
bindings.

Makes sense. I will update in V3.


+            - const: state
+            - const: berr
+            - const: mb_8-127

Choose one: either underscores or hyphens. Keep it consistent in your
bindings.


I will update in V3.

+      required:
+        - compatible
+        - reg
+        - interrupts
+        - interrupt-names

What happened to "else:"? Why all other devices now have up to 4 interrupts?

Best regards,
Krzysztof


I will add the 'else' branch back in V3.

Best Regards,
Ciprian






[Index of Archives]     [Automotive Discussions]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [CAN Bus]

  Powered by Linux