Re: [PATCH v8 4/4] phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] phy: nuvoton: missing MFD_SYSCON select and unchecked
  regmap_read()
--

commit ce87e45e509dca5386021bb0dd4e3bb91e3935d8
Author: Joey Lu <a0987203069@gmail.com>

phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

This commit extends the Nuvoton MA35D1 USB 2.0 PHY driver to support both
PHY0 and PHY1, updates clock-stability polling in phy_init(), registers a
read-only USB role switch for PHY0, and adds resistor calibration and
over-current polarity device-tree properties.

> diff --git a/drivers/phy/nuvoton/Kconfig b/drivers/phy/nuvoton/Kconfig
> index d02cae2db315a..1f37e860de507 100644
> --- a/drivers/phy/nuvoton/Kconfig
> +++ b/drivers/phy/nuvoton/Kconfig
> @@ -6,6 +6,7 @@ config PHY_MA35_USB
>  	tristate "Nuvoton MA35 USB2.0 PHY driver"
>  	depends on ARCH_MA35 || COMPILE_TEST
>  	depends on OF
> +	depends on USB_ROLE_SWITCH || !USB_ROLE_SWITCH
>  	select GENERIC_PHY
>  	help
>  	  Enable this to support the USB2.0 PHY on the Nuvoton MA35
> diff --git a/drivers/phy/nuvoton/phy-ma35d1-usb2.c b/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> index 0b9976fb15c17..bc706356c4341 100644
> --- a/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> +++ b/drivers/phy/nuvoton/phy-ma35d1-usb2.c

[ ... ]

> +static int ma35_usb_phy_init(struct phy *phy)
> +{
> +	struct ma35_phy_port *port = phy_get_drvdata(phy);
> +	struct ma35_usb_phy *p = container_of(port - port->idx,
> +					      struct ma35_usb_phy, port[0]);
> +	unsigned int n = port->idx;
>  	unsigned int val;
>  	int ret;
>  
> -	ret = clk_prepare_enable(p_phy->clk);
> -	if (ret < 0) {
> -		dev_err(p_phy->dev, "Failed to enable PHY clock: %d\n", ret);
> -		return ret;
> -	}
> +	ret = clk_prepare_enable(p->clk);
> +	if (ret)
> +		return dev_err_probe(p->dev, ret, "failed to enable clock\n");
>  
> -	regmap_read(p_phy->sysreg, MA35_SYS_REG_USBPMISCR, &val);
> -	if (val & PHY0SUSPEND) {
> -		/*
> -		 * USB PHY0 is in operation mode already
> -		 * make sure USB PHY 60 MHz UTMI Interface Clock ready
> -		 */
> -		ret = regmap_read_poll_timeout(p_phy->sysreg, MA35_SYS_REG_USBPMISCR, val,
> -						val & PHY0DEVCKSTB, 10, 1000);
> -		if (ret == 0)
> -			return 0;
> -	}
> +	regmap_read(p->sysreg, MA35_SYS_USBPMISCR, &val);
>  
> -	/*
> -	 * reset USB PHY0.
> -	 * wait until USB PHY0 60 MHz UTMI Interface Clock ready
> -	 */
> -	regmap_update_bits(p_phy->sysreg, MA35_SYS_REG_USBPMISCR, 0x7, (PHY0POR | PHY0SUSPEND));
> +	if ((val & USBPMISCR_PHY_SUSPEND(n)) &&
> +	    !ma35_usb_phy_wait_ready(p, n, &val))
> +		goto out;

[ ... ]

> +static enum usb_role ma35_role_sw_get(struct usb_role_switch *sw)
> +{
> +	struct ma35_usb_phy *p = usb_role_switch_get_drvdata(sw);
> +	u32 val;
> +
> +	regmap_read(p->sysreg, MA35_SYS_PWRONOTP, &val);
> +
> +	return (val & PWRONOTP_USBP0ID) ? USB_ROLE_HOST : USB_ROLE_DEVICE;
> +}

[Severity: Medium]
Can val be used uninitialized in ma35_usb_phy_init() and
ma35_role_sw_get() when regmap_read() fails or is stubbed out?

Neither PHY_MA35_USB nor ARCH_MA35 selects MFD_SYSCON, so PHY_MA35_USB can
be built with CONFIG_MFD_SYSCON=n and CONFIG_REGMAP=n.  In that
configuration, syscon_regmap_lookup_by_phandle() always fails with
-ENOTSUPP, and the regmap_read() stub returns -EINVAL without writing val,
causing the compiler to flag val as uninitialized in ma35_usb_phy_init()
and ma35_role_sw_get().

Selecting MFD_SYSCON in PHY_MA35_USB and checking the regmap_read() return
value (or initializing val) would avoid both issues.

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