Re: [PATCH] dt-bindings: net: can: fsl,mpc5xxx-mscan: Convert to DT schema

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] The schema omits the `fsl,mpc5200b-mscan` fallback compatible string used in existing device trees, causing schema validation failures.
- [Medium] The schema is missing conditional `if-then` blocks, globally allowing properties that are invalid for MPC5200, and failing to mandate required properties for MPC5121.
--

commit dee9e6a0d172cc57e05eeda41cfef5b093d00d22
Author: Quchaosheng [off-list ref]

dt-bindings: net: can: fsl,mpc5xxx-mscan: Convert to DT schema

Converts the Freescale MPC5xxx MSCAN controller binding from free-form text
to DT schema. It specifies compatible strings, clocks, clock names, and
mscan clock properties.

[ ... ]

+properties:
+  compatible:
+    enum:
+      - fsl,mpc5200-mscan
+      - fsl,mpc5121-mscan

[Severity: Medium]
Will this strict enum cause validation failures for existing device trees?

Existing device trees, such as arch/powerpc/boot/dts/mpc5200b.dtsi, use a
fallback array pattern:

    compatible = "fsl,mpc5200b-mscan", "fsl,mpc5200-mscan";

Should the schema allow fsl,mpc5200b-mscan as well, perhaps using an items
list to support the fallback?

[ ... ]

+  fsl,mscan-clock-source:
+    description:
+      Clock source used for the controller. Valid values are "ip" for the IP
+      bus clock, "ref" for the reference clock (XTAL) and "sys" for the
+      system clock. "sys" is only available on MPC5121. When the property is
+      absent, an optimal clock source based on the system clock is selected,
+      falling back to the reference clock.
+    enum: [ip, ref, sys]

[Severity: Medium]
Should the sys value be restricted to the MPC5121 variant?

The schema currently allows sys globally, even though it is invalid for
the MPC5200. An if-then block could be used to restrict this.

+
+  fsl,mscan-clock-divider:
+    description:
+      Additional clock divider for the reference and system clock sources.
+      Defaults to 1 when not specified.
+    $ref: /schemas/types.yaml#/definitions/uint32
+    default: 1

[Severity: Medium]
Does this allow the fsl,mscan-clock-divider property on the MPC5200 variant
where it does not exist?

An if-then conditional block could also restrict this property to the MPC5121
variant to prevent it from silently passing validation on MPC5200 nodes.

+
+required:
+  - compatible
+  - reg
+  - interrupts

[Severity: Medium]
Should clocks and clock-names be required for the MPC5121 variant?

The driver probe requires these clocks for MPC5121. Without an if-then block
requiring them, incomplete nodes will silently pass schema validation but fail
at runtime.

+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+
+    can@1300 {
+        compatible = "fsl,mpc5121-mscan";
+        reg = <0x1300 0x80>;
+        interrupts = <12 IRQ_TYPE_LEVEL_LOW>;
+        interrupt-parent = <&ipic>;
+    };

[Severity: Medium]
Since the clocks and clock-names properties are strictly required by the
driver for MPC5121, does this example represent a functionally broken
configuration?

Because the schema does not strictly require the clock properties, this
example passes validation, which demonstrates that the schema accepts
invalid configurations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914030215.437602-4-quchaosheng000406@163.com?part=1