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

[sashiko-bot@kernel.org]

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 <quchaosheng000406@163.com>

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.

> diff --git a/Documentation/devicetree/bindings/net/can/fsl,mpc5xxx-mscan.yaml b/Documentation/devicetree/bindings/net/can/fsl,mpc5xxx-mscan.yaml
> new file mode 100644
> index 0000000000000..108f225f91963
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/can/fsl,mpc5xxx-mscan.yaml
> @@ -0,0 +1,87 @@
[ ... ]
> +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