Re: [PATCH v3 1/3] dt-bindings: phy: nuvoton,ma35d1-usb2-phy: extend for dual-port and OTG

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The DT binding introduces multi-port support, but the corresponding driver lacks implementation, leading to silent hardware malfunction.
- [Medium] The `#phy-cells` property uses an enum constraint instead of a strict const constraint, violating subsystem guidelines.
--

--- Patch [1]: [PATCH v3 1/3] dt-bindings: phy: nuvoton,ma35d1-usb2-phy: extend for dual-port and OTG ---
commit 7f72a8cc4b64e1dfcf4efba1cd67bf9eafd31d72
Author: Joey Lu <a0987203069@gmail.com>

dt-bindings: phy: nuvoton,ma35d1-usb2-phy: extend for dual-port and OTG

This commit updates the nuvoton,ma35d1-usb2-phy device tree binding to
support dual USB PHY ports (PHY0 and PHY1). It relaxes #phy-cells from const: 0
to enum: [0, 1] to allow port selection while preserving backward
compatibility. It also adds new optional properties for resistor calibration
and over-current polarity.

> diff --git a/Documentation/devicetree/bindings/phy/nuvoton,ma35d1-usb2-phy.yaml b/Documentation/devicetree/bindings/phy/nuvoton,ma35d1-usb2-phy.yaml
> index fff858c909a06..191d1b2272ec5 100644
> --- a/Documentation/devicetree/bindings/phy/nuvoton,ma35d1-usb2-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/nuvoton,ma35d1-usb2-phy.yaml
[ ... ]
> @@ -15,7 +27,12 @@ properties:
>        - nuvoton,ma35d1-usb2-phy
>  
>    "#phy-cells":
> -    const: 0
> +    enum: [0, 1]

[Severity: High]
Does the driver currently support this new #phy-cells = <1> configuration?

Looking at ma35_usb_phy_probe() in drivers/phy/nuvoton/phy-ma35d1-usb2.c, the 
provider registers using of_phy_simple_xlate:

    provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);

Because of_phy_simple_xlate ignores cell arguments, any valid DT request for
PHY1 will silently map to the single initialized PHY.

Furthermore, ma35_usb_phy_power_on() strictly hardcodes registers for PHY0:

    regmap_update_bits(p_phy->sysreg, MA35_SYS_REG_USBPMISCR, 0x7, (PHY0POR | PHY0SUSPEND));

Will this cause a consumer attempting to control PHY1 to inadvertently reset and
power on PHY0 instead?

[Severity: Medium]
Is it acceptable to use an enum constraint for a cell-count property?

According to the subsystem/dt-bindings.md global review guidelines, each
cell-count property must have a strict const constraint matching the hardware.
Since the hardware physically has two ports, shouldn't #phy-cells be defined
as const: 1 to accurately model the hardware's capabilities?

> +    description:
> +      When 0, the node exposes PHY0 only and consumers reference it
> +      with no cell argument. When 1, the single cell selects the port,
> +      with 0 for the OTG port (USB0, shared with DWC2 gadget controller)
> +      and 1 for the host-only port (USB1).
[ ... ]
> @@ -39,7 +78,7 @@ examples:
>  
>      usb_phy: usb-phy {
>          compatible = "nuvoton,ma35d1-usb2-phy";
> -        clocks = <&clk USBD_GATE>;
> +        clocks = <&clk HUSBH0_GATE>;
>          nuvoton,sys = <&sys>;
>          #phy-cells = <0>;

[Severity: Medium]
With the hardware supporting multiple ports, should the example be updated to
demonstrate the correct multi-port configuration (#phy-cells = <1>;) instead
of propagating the legacy configuration?

>      };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708103606.1462960-1-a0987203069@gmail.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy