Re: [PATCH v2 7/9] dt-bindings: arm: stm32: Switch st,stm32mp23/25-syscfg into simple-mfd

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Moving `st,stm32mp23-syscfg` and `st,stm32mp25-syscfg` exclusively to the 3-item array makes the `simple-mfd` compatible string mandatory, breaking validation for existing device trees.
- [High] The st,stm32-usb2phy driver expects a 'reg' property and a syscon parent node, but the binding mandates 'st,syscfg' and forbids 'reg'.
- [Medium] The schema uses `additionalProperties: false` but fails to define the USB PHY subnodes, meaning any added subnodes will be rejected by `dtbs_check`.
- [Medium] Incomplete single-node DWC3 binding prevents the use of standard USB properties.
- [Medium] Mismatch between '#clock-cells' binding requirement and driver implementation.
--

--- Patch [7]: [PATCH v2 7/9] dt-bindings: arm: stm32: Switch st,stm32mp23/25-syscfg into simple-mfd ---
commit 442df2b694ee72167d8a15b0750e1f6687add72a
Author: Marek Vasut <[email protected]>

dt-bindings: arm: stm32: Switch st,stm32mp23/25-syscfg into simple-mfd

The st,stm32mp23-syscfg and st,stm32mp25-syscfg can have subnodes, which
represent the USB 2.0 FEMTO-PHY. Turn the syscfg into "simple-mfd" so the
PHYs would get populated by the OS.

> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml b/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> index 95d2319afe235..ef640443dd239 100644
> --- a/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> +++ b/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> @@ -22,11 +22,12 @@ properties:
>                - st,stm32mp151-pwr-mcu
>                - st,stm32mp157-syscfg
>                - st,stm32mp21-syscfg
> -              - st,stm32mp23-syscfg
> -              - st,stm32mp25-syscfg
>            - const: syscon
>        - items:
> -          - const: st,stm32-tamp
> +          - enum:
> +              - st,stm32-tamp
> +              - st,stm32mp23-syscfg
> +              - st,stm32mp25-syscfg
>            - const: syscon
>            - const: simple-mfd

[Severity: High]
Does moving st,stm32mp23-syscfg and st,stm32mp25-syscfg exclusively to the
3-item array make the simple-mfd compatible string mandatory? It looks like
this breaks backward compatibility, meaning existing valid device trees that
only specify 'syscon' will suddenly fail schema validation.

[Severity: Medium]
Will this schema reject the new USB PHY subnodes mentioned in the commit
message? 

Looking at Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml,
it uses additionalProperties: false without defining patternProperties for
child nodes. Any added subnodes will likely be rejected by dtbs_check.

[Severity: High]
Can the USB2 PHY driver probe successfully with a device tree that conforms
to the st,stm32-usb2phy binding?

The driver unconditionally requires a 'reg' property to extract the offset:

drivers/phy/st/phy-stm32-usb2phy.c:stm32_usb2phy_probe() {
    ...
    offset = of_get_address(dev->of_node, 0, NULL, NULL);
    if (!offset)
        return dev_err_probe(dev, -EINVAL, "Failed to get regmap offset\n");
    ...
}

However, the binding completely omits and implicitly forbids 'reg',
requiring 'st,syscfg' instead:

Documentation/devicetree/bindings/phy/st,stm32-usb2phy.yaml:
  st,syscfg:
    $ref: /schemas/types.yaml#/definitions/phandle-array
    description: Phandle to system configuration controller.

[Severity: Medium]
Is the single-node DWC3 binding complete enough to allow standard USB
properties?

The schema is sealed with unevaluatedProperties: false without including a
$ref to snps,dwc3.yaml#:

Documentation/devicetree/bindings/usb/st,stm32mp25-dwc3.yaml:
unevaluatedProperties: false

Would this unconditionally reject standard USB core properties?

[Severity: Medium]
Is there a mismatch between the '#clock-cells' binding requirement and the
driver implementation?

The driver registers the clock provider with of_clk_hw_simple_get, which
ignores arguments and acts as a 0-cell provider:

drivers/phy/st/phy-stm32-usb2phy.c:stm32_usb2phy_probe() {
    ...
    ret = devm_of_clk_add_hw_provider(phy_dev->dev, of_clk_hw_simple_get,
                                      &phy_dev->clk48_hw);
    ...
}

But the binding mandates #clock-cells = 1:

Documentation/devicetree/bindings/phy/st,stm32-usb2phy.yaml:
  "#clock-cells":
    const: 1

Does this force a dummy argument in the device tree that the driver ignores?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy