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

[Joey Lu <a0987203069@gmail.com>]

On 11/9/2026 3:14 PM, sashiko-bot@kernel.org wrote:
> 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.
MFD_SYSCON: valid in part. Neither ARCH_MA35 nor PHY_MA35_USB selects
MFD_SYSCON, and with CONFIG_REGMAP=n the regmap_read() stub returns -EINVAL
without writing val, so the compiler can warn about val being uninitialized.
In practice the probe would already fail at 
syscon_regmap_lookup_by_phandle()
(-ENOTSUPP) and never reach those paths. The missing dependency is the real
problem, so in v9 I'll add "select MFD_SYSCON" to PHY_MA35_USB (it selects
REGMAP_MMIO as well). I'll keep regmap_read() unchecked as discussed with
Mani, since once syscon is guaranteed the reads are plain MMIO and do not
fail.

Joey