Re: [PATCH v11 3/3] phy: airoha: Add support for Airoha AN7581 USB PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing PHY Initialization for SerDes PHY (API Violation)
- [High] Incorrect Register Update using regmap_set_bits with a multi-bit mask
- [Medium] Semantic Mismatch: Hardcoding SerDes PHY to DEVICE mode ignores HOST controller capability
- [Low] Missing Header Files for Core Kernel APIs
--

--- Patch [3]: [PATCH v11 3/3] phy: airoha: Add support for Airoha AN7581 USB PHY ---
commit 22b1a55f0db8cf42614375aa158b5f25ac5f97d6
Author: Christian Marangi <ansuelsmth@gmail.com>
Subject: phy: airoha: Add support for Airoha AN7581 USB PHY

This commit adds support for the Airoha AN7581 USB PHY driver, which supports
up to 2 USB ports. USB 2.0 is always supported, while USB 3.0 mode is only
available if the Serdes port is correctly configured. The driver includes slew
rate calibration logic using a selectable monitor clock.

> diff --git a/drivers/phy/airoha/phy-an7581-usb.c b/drivers/phy/airoha/phy-an7581-usb.c
> new file mode 100644
> index 0000000000000..92c5e5c2fbf3b
> --- /dev/null
> +++ b/drivers/phy/airoha/phy-an7581-usb.c
> @@ -0,0 +1,559 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Author: Christian Marangi <ansuelsmth@gmail.com>
> + */
> +
> +#include <dt-bindings/phy/phy.h>
> +#include <dt-bindings/soc/airoha,scu-ssr.h>
> +#include <linux/bitfield.h>
> +#include <linux/math.h>
> +#include <linux/module.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/phy/phy.h>
> +#include <linux/platform_device.h>
> +#include <linux/regmap.h>

[Severity: Low]
Does this file need to include linux/delay.h for usleep_range(), linux/of.h
for of_property_read_u32(), and linux/err.h for PTR_ERR_OR_ZERO() to avoid
potential build breaks on other architectures if implicit inclusions change?

[ ... ]
> +static void an7581_usb_phy_u3_init(struct an7581_usb_phy_priv *priv)
> +{
[ ... ]
> +	regmap_update_bits(priv->regmap, AIROHA_USB_PHY_U3_PHYA_REG0,
> +			   AIROHA_USB_PHY_SSUSB_BG_DIV,
> +			   AIROHA_USB_PHY_SSUSB_BG_DIV_4);
> +
> +	regmap_set_bits(priv->regmap, AIROHA_USB_PHY_U3_PHYA_REG1,
> +			FIELD_PREP(AIROHA_USB_PHY_SSUSB_XTAL_TOP_RESERVE, 0x600));

[Severity: High]
Is it intentional to use regmap_set_bits() here with a multi-bit mask?
This performs a logical OR, which sets the desired bits but will not clear any
pre-existing bits in the mask from the bootloader. Should this be
regmap_update_bits() instead to ensure a clean state?

> +
> +	regmap_update_bits(priv->regmap, AIROHA_USB_PHY_U3_PHYA_DA_REG19,
> +			   AIROHA_USB_PHY_SSUSB_PLL_SSC_DELTA1_U3,
> +			   FIELD_PREP(AIROHA_USB_PHY_SSUSB_PLL_SSC_DELTA1_U3, 0x43));
> +}
> +
> +static int an7581_usb_phy_init(struct phy *phy)
> +{
> +	struct an7581_usb_phy_instance *instance = phy_get_drvdata(phy);
> +	struct an7581_usb_phy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +	int ret;
> +
> +	switch (instance->type) {
> +	case PHY_TYPE_USB2:
> +		an7581_usb_phy_u2_init(priv);
> +		break;
> +	case PHY_TYPE_USB3:
> +		ret = phy_set_mode(priv->serdes_phy, PHY_MODE_USB_DEVICE_SS);

[Severity: High]
This calls phy_set_mode() on priv->serdes_phy, but does the underlying
serdes PHY hardware ever get initialized? It appears phy_init() is missing
before this call, which could leave the serdes PHY in an uninitialized state.

[Severity: Medium]
Additionally, is it correct to hardcode the serdes PHY mode to
PHY_MODE_USB_DEVICE_SS here? The commit message mentions xHCI which is a host
controller, so forcing device mode might cause initialization failures if the
serdes PHY driver distinguishes between host and device operational modes.

> +		if (ret)
> +			return ret;
> +
> +		an7581_usb_phy_u3_init(priv);
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int an7581_usb_phy_power_on(struct phy *phy)
> +{
> +	struct an7581_usb_phy_instance *instance = phy_get_drvdata(phy);
> +	struct an7581_usb_phy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +
> +	switch (instance->type) {
> +	case PHY_TYPE_USB2:
> +		an7581_usb_phy_u2_power_on(priv);
> +		break;
> +	case PHY_TYPE_USB3:
> +		an7581_usb_phy_u3_power_on(priv);

[Severity: High]
Does priv->serdes_phy need to be powered on here as well? The wrapper
powers on its USB3 instance, but it looks like phy_power_on(priv->serdes_phy)
is never called.

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
[ ... ]
> +static int an7581_usb_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
> +{
> +	struct an7581_usb_phy_instance *instance = phy_get_drvdata(phy);
> +	struct an7581_usb_phy_priv *priv = dev_get_drvdata(phy->dev.parent);
> +
> +	switch (instance->type) {
> +	case PHY_TYPE_USB2:
> +		return an7581_usb_phy_u2_set_mode(priv, mode);
> +	default:
> +		return 0;

[Severity: Medium]
Does this code silently ignore the consumer's requested mode for
PHY_TYPE_USB3? Since an7581_usb_phy_init() forces PHY_MODE_USB_DEVICE_SS,
should this function handle propagating the requested mode to the serdes PHY
instead of hardcoding it during initialization?

> +	}
> +}
[ ... ]
> +static const struct phy_ops airoha_phy = {
> +	.init		= an7581_usb_phy_init,
> +	.power_on	= an7581_usb_phy_power_on,
> +	.power_off	= an7581_usb_phy_power_off,
> +	.set_mode	= an7581_usb_phy_set_mode,
> +	.owner		= THIS_MODULE,
> +};

[Severity: High]
Is the .exit callback intentionally omitted here? Without an .exit hook,
there doesn't seem to be a way to call phy_exit() on the serdes PHY, which
might lead to resource leaks when the PHY is released.

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

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