Re: [PATCH net-next v2 1/5] dt-bindings: net: maxlinear: add MxL86211C VDDP supply

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The binding defines `vddp-supply` as "Regulator supplying the
  VDDP pin", which powers the MDIO/MDC/MDINT pads, and makes it…
- [Low] In Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml,
  the `else: properties: vddp-supply: false` branch (lines 35-37) can…

--

dt-bindings: net: maxlinear: add MxL86211C VDDP supply

This commit adds a vddp-supply property to the MaxLinear GPY2xx binding.
The voltage of that supply selects 1.8 V or 3.3 V for the MDIO, MDC and
MDINT pads. A top-level select restricts the schema to the MxL86211C PHY
ID, and an if/then/else makes the supply required for that PHY.

> diff --git a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
> index 8a3713abd1ca9..a0dfb6192d81b 100644
> --- a/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
> +++ b/Documentation/devicetree/bindings/net/maxlinear,gpy2xx.yaml
> @@ -10,10 +10,38 @@ maintainers:
>    - Andrew Lunn <andrew@xxxxxxx>
>    - Michael Walle <michael@xxxxxxxx>
>  
> +select:
> +  properties:
> +    compatible:
> +      contains:
> +        const: ethernet-phy-idc133.5400
> +  required:
> +    - compatible
> +
>  allOf:
>    - $ref: ethernet-phy.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: ethernet-phy-idc133.5400
> +      required:
> +        - compatible
> +
> +    then:
> +      required:
> +        - vddp-supply
> +
> +    else:
> +      properties:
> +        vddp-supply: false

[Severity: Low]
Can this else branch ever be evaluated?

The new top-level select applies the schema only to nodes whose
compatible contains ethernet-phy-idc133.5400. The if above tests the
same predicate, so whenever the schema runs, only the then branch runs.

No other binding references this schema. Other GPY2xx nodes are not
selected at all, so "vddp-supply: false" is never enforced for them.

The later patch in the series, "dt-bindings: net: maxlinear: add
MxL86211C lane mapping support", adds "maxlinear,lane-asp-map: false" to
this same else branch. That constraint would not be enforced either.

>  
>  properties:
> +  vddp-supply:
> +    description: |
> +      Regulator supplying the VDDP pin. Its voltage configures the MDIO,
> +      MDC, and MDINT pad voltage and must be either 1.8 V or 3.3 V.
> +

[Severity: Medium]
This makes vddp-supply mandatory for the MxL86211C and describes it as
the supply for the MDIO, MDC and MDINT pads. Does anything in the series
enable this supply?

The later driver patch, "net: phy: mxl-gpy: add MxL86211C VDDP supply",
does only two things with the regulator: it gets it, and it reads its
voltage.

drivers/net/phy/mxl-gpy.c:mxl86211c_probe() {
    ...
	vddp = devm_regulator_get(dev, "vddp");
    ...
}

drivers/net/phy/mxl-gpy.c:mxl86211c_configure_mdio_voltage() {
    ...
	voltage = regulator_get_voltage(vddp);
    ...
}

Neither function calls regulator_enable() or devm_regulator_get_enable(),
so rdev->use_count stays 0 for this rail. Take a board where VDDP can be
switched and is not marked regulator-always-on. Could
regulator_late_cleanup() turn the rail off there?

drivers/regulator/core.c:regulator_late_cleanup() {
    ...
	if (rdev->use_count)
		goto unlock;
    ...
}

On DT systems have_full_constraints() is true, so the rail would be
disabled and the PHY would lose power to its MDIO pads.
regulator-boot-on does not prevent this, because late cleanup only checks
always_on.

If the rail is off at boot, nothing ever turns it on. The MDIO accesses
in mxl86211c_probe() and gpy_probe() would then fail.

Other PHY drivers handle this by enabling the supply. For example,
qcom/at803x.c calls devm_regulator_get_enable_optional(..., "vddio").
Should the MxL86211C driver do the same?

The binding example uses a regulator-fixed with no GPIO, which cannot be
switched off, so the example does not show this problem.

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016112033.949026-1-robert.marko%40sartura.hr