Re: [PATCH v3 3/3] 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 4 potential issue(s) to consider:
- [High] Kconfig Dependency / Build and Probe Regression
- [High] Missing Wait for Clock Stability on Already-Operating PHY (Suspend/Resume Regression)
- [Medium] Uninitialized variable usage due to ignored regmap_read return value
- [Low] Undocumented Functional Changes in Hardware Configuration
--

--- Patch [3]: [PATCH v3 3/3] phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support ---
Note: The format of this report is altered due to recitation restrictions. I am providing a free-form summary of the findings without quoting the original patch code.

commit 0f7c9811782335bab86ecc4c08f7f6a17839e8c3
Author: Joey Lu <a0987203069@gmail.com>
Subject: phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

Summary: This commit extends the existing MA35D1 USB 2.0 PHY driver to manage
both PHY0 and PHY1. It updates the initialization logic to support host-mode
clock stability polling and registers a read-only USB role switch for PHY0.

[Severity: High]
In ma35_role_switch_init(), the driver fails the probe if the role switch
registration returns an error. Does this break the driver when the role
switch framework is disabled?

If CONFIG_USB_ROLE_SWITCH is not enabled, the register function returns
-ENODEV, causing ma35_role_switch_init() to treat it as a fatal error and
unconditionally fail the PHY probe. 

Furthermore, shouldn't the driver declare a Kconfig dependency on
USB_ROLE_SWITCH or handle the -ENODEV return case gracefully?

[Severity: High]
In ma35_usb_phy_init(), the driver returns early if the suspend bit is
already set. Does this skip waiting for clock stability during system resume?

On system suspend, the suspend bit remains set. During resume, the PHY
crystal might have been stopped and requires time to restabilize. Returning
early skips the host/device clock stability polling. 

The original code explicitly handled this by checking for clock stability
even when the suspend bit was already set. Could this lead to controller
hangs when the controller proceeds before the PHY clocks are stable?

[Severity: Medium]
In ma35_role_sw_get(), the return value of regmap_read() is ignored before
evaluating the val variable. Can this expose uninitialized kernel stack
memory?

If the syscon read fails, val will contain uninitialized memory, which is
then evaluated and returned to userspace via the role sysfs attribute.

This is a pre-existing issue, but the same pattern of ignoring the
regmap_read() return value also appears in ma35_usb_phy_init(). Should the
return values be checked in both locations?

[Severity: Low]
In ma35_usb_phy_probe(), new logic is introduced to parse nuvoton,rcalcode
and nuvoton,oc-active-high from the device tree and apply them to the
resistor trim and over-current polarity registers.

Could the commit message be updated to document these functional hardware
configuration changes? They are currently completely omitted from the
commit history.

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