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

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] Strictly enforcing `simple-mfd` in the compatible array breaks backward compatibility with existing device trees.
- [High] The `en_refcnt` atomic reference counter is leaked on error paths in `stm32_usb2phy_enable()`, permanently breaking the PHY.
- [Medium] The `allOf:if` condition for `st,stm32mp23/25-syscfg` uses `enum` directly on the `compatible` property array, bypassing the validation block.
- [Low] The `patternProperties` regex in the syscon parent binding rejects the `usb-phy@` node name recommended by the child PHY binding.
--

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

This commit updates the STM32 syscon binding to convert the
st,stm32mp23-syscfg and st,stm32mp25-syscfg compatibles into "simple-mfd".
This allows the OS to automatically populate the subnodes for the USB 2.0
FEMTO-PHY.

> diff --git a/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml b/Documentation/devicetree/bindings/arm/stm32/st,stm32-syscon.yaml
> index 95d2319afe235..4db4198c5dd7a 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: Critical]
Does enforcing simple-mfd in this array break backward compatibility?

Older device trees define these syscfg nodes using only st,stm32mp25-syscfg
and syscon. Moving them into a strict 3-item list requiring simple-mfd
causes validation to fail for existing DTBs.

> @@ -39,6 +40,17 @@ properties:
>    "#clock-cells":
>      const: 0
>  
> +  "#address-cells":
> +    const: 1
> +
> +  "#size-cells":
> +    const: 1
> +
> +patternProperties:
> +  "^phy@[0-9a-f]+$":
> +    type: object
> +    $ref: /schemas/phy/st,stm32-usb2phy.yaml

[Severity: Low]
Will this regex reject the node names recommended by the child PHY binding?

The child binding example uses usb-phy@2400:

    usb-phy@2400 {
        compatible = "st,stm32mp25-usb2phy1";

which fails the stricter ^phy@[0-9a-f]+$ pattern defined here.

[ ... ]

> @@ -57,10 +69,14 @@ allOf:
>    - if:
>        properties:
>          compatible:
> -          const: st,stm32mp25-syscfg
> +          enum:
> +            - st,stm32mp23-syscfg
> +            - st,stm32mp25-syscfg

[Severity: Medium]
Does this conditional block bypass schema validation?

Applying enum directly to an array property checks if the array strictly
equals the scalar string, which always evaluates to false. Should this use
contains: enum: instead?

>      then:
>        required:
>          - "#clock-cells"
> +        - "#address-cells"
> +        - "#size-cells"

[Severity: High]
Is there a reference counter leak in stm32_usb2phy_enable() introduced
earlier in this series?

If an unsupported clock rate is detected, the function returns without
decrementing en_refcnt:

drivers/phy/st/phy-stm32-usb2phy.c:stm32_usb2phy_enable() {
    ...
    if (...)
        ...
    else
        return -EINVAL;
}

Additionally, the common error paths do not decrement en_refcnt after it
was incremented:

drivers/phy/st/phy-stm32-usb2phy.c:stm32_usb2phy_enable() {
    ...
error_regdis:
    regulator_disable(phy_dev->vdd33);

    return ret;
}

Future calls will see en_refcnt > 1 and return success immediately, leaving
the PHY disabled.

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