[PATCH v7 6/6] phy: airoha: Add support for Airoha AN7581 USB PHY
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Tue May 19 16:23:46 PDT 2026
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] `dev_err_probe` is called with `IS_ERR()` instead of `PTR_ERR()`, breaking error propagation and probe deferral.
- [High] Incorrect configuration of `FORCE_IDDIG` in `PHY_MODE_USB_DEVICE` due to a missing flag.
- [Medium] `fm_out` may be used uninitialized if `regmap_read_poll_timeout` fails.
- [Medium] Missing `select REGMAP_MMIO` dependency in Kconfig.
- [Low] Dead code in `an7581_usb_phy_power_on` checking the USB PHY's mode instead of the SerDes PHY's mode.
- [Low] Assigning generic `PHY_TYPE_USB2` values to a variable typed as a local internal enum.
--
commit a54da44fe952522b06761e81fb643efff0dad77a
Author: Christian Marangi <ansuelsmth at gmail.com>
phy: airoha: Add support for Airoha AN7581 USB PHY
Add support for Airoha AN7581 USB PHY driver. AN7581 supports up to 2
USB port with USB 2.0 mode always supported and USB 3.0 mode available
only if the Serdes port is correctly configured for USB 3.0.
> diff --git a/drivers/phy/airoha/Kconfig b/drivers/phy/airoha/Kconfig
> index 9a1b625a77012..bb4e3367baa58 100644
> --- a/drivers/phy/airoha/Kconfig
> +++ b/drivers/phy/airoha/Kconfig
[ ... ]
> @@ -11,3 +11,13 @@ config PHY_AIROHA_AN7581_PCIE
> Say Y here to add support for Airoha AN7581 PCIe PHY driver.
> This driver create the basic PHY instance and provides initialize
> callback for PCIe GEN3 port.
> +
> +config PHY_AIROHA_AN7581_USB
> + tristate "Airoha AN7581 USB PHY Driver"
> + depends on ARCH_AIROHA || COMPILE_TEST
> + depends on OF
> + select GENERIC_PHY
[Severity: Medium]
Will this fail to link under COMPILE_TEST on architectures without REGMAP_MMIO?
The driver relies on devm_regmap_init_mmio(), so it might need to explicitly
select REGMAP_MMIO.
> + help
> + Say 'Y' here to add support for Airoha AN7581 USB PHY driver.
> + This driver create the basic PHY instance and provides initialize
> + callback for USB port.
> diff --git a/drivers/phy/airoha/phy-an7581-usb.c b/drivers/phy/airoha/phy-an7581-usb.c
> new file mode 100644
> index 0000000000000..90fd2cbe68d42
> --- /dev/null
> +++ b/drivers/phy/airoha/phy-an7581-usb.c
[ ... ]
> +static void an7581_usb_phy_u2_slew_rate_calibration(struct an7581_usb_phy_priv *priv)
> +{
> + u32 fm_out;
> + u32 srctrl;
[ ... ]
> + /* Timeout can happen and we will apply workaround at the end */
> + regmap_read_poll_timeout(priv->regmap, AIROHA_USB_PHY_FMMONR0, fm_out,
> + fm_out, AIROHA_USB_PHY_FREQDET_SLEEP,
> + AIROHA_USB_PHY_FREQDET_TIMEOUT);
> +
> + /* Disable Frequency meter */
> + regmap_clear_bits(priv->regmap, AIROHA_USB_PHY_FMCR0,
> + AIROHA_USB_PHY_FREQDET_EN);
> +
> + /* Disable Free run clock */
> + regmap_clear_bits(priv->regmap, AIROHA_USB_PHY_FMMONR1,
> + AIROHA_USB_PHY_FRCK_EN);
> +
> + /* Disable HS TX SR calibration */
> + regmap_clear_bits(priv->regmap, AIROHA_USB_PHY_USBPHYACR5,
> + AIROHA_USB_PHY_USB20_HSTX_SRCAL_EN);
> +
> + usleep_range(1000, 1500);
> +
> + /* Frequency was not detected, use default SR calibration value */
> + if (!fm_out) {
[Severity: Medium]
If the underlying regmap_read() fails immediately (e.g., due to an I/O error),
does fm_out remain uninitialized?
Since the return value of regmap_read_poll_timeout() isn't checked, an
uninitialized stack value could bypass this check and cause unpredictable
behavior during the subsequent division.
> + srctrl = AIROHA_USB_PHY_DEFAULT_SR_CALIBRATION;
> + dev_err(priv->dev, "Frequency not detected, using default SR calibration.\n");
> + } else {
> + /* (1024 / FM_OUT) * REF_CK * U2_SR_COEF (round to the nearest digits) */
> + srctrl = AIROHA_USB_PHY_REF_CK * AIROHA_USB_PHY_U2_SR_COEF;
> + srctrl = (srctrl * AIROHA_USB_PHY_U2_FM_DET_CYCLE_CNT) / fm_out;
> + srctrl = DIV_ROUND_CLOSEST(srctrl, AIROHA_USB_PHY_U2_SR_COEF_DIVISOR);
> + dev_dbg(priv->dev, "SR calibration applied: %x\n", srctrl);
> + }
[ ... ]
> +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:
> + if (phy_get_mode(phy) == PHY_MODE_PCIE ||
> + phy_get_mode(phy) == PHY_MODE_ETHERNET)
> + return 0;
[Severity: Low]
Is this check examining the wrong phy object?
It seems to be checking the USB PHY's mode instead of the shared SerDes combo
PHY (priv->serdes_phy). Since the USB PHY's mode will presumably never be PCIe
or Ethernet, will this condition ever evaluate to true?
> +
> + an7581_usb_phy_u3_power_on(priv);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
[ ... ]
> +static int an7581_usb_phy_power_off(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_off(priv);
> + break;
> + case PHY_TYPE_USB3:
> + if (phy_get_mode(phy) == PHY_MODE_PCIE ||
> + phy_get_mode(phy) == PHY_MODE_ETHERNET)
> + return 0;
[Severity: Low]
Similar to the an7581_usb_phy_power_on() check above, does this check the
wrong phy object?
> +
> + an7581_usb_phy_u3_power_off(priv);
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int an7581_usb_phy_u2_set_mode(struct an7581_usb_phy_priv *priv,
> + enum phy_mode mode)
> +{
> + u32 val;
> +
> + /*
> + * For Device and Host mode, enable force IDDIG.
> + * For Device set IDDIG, for Host clear IDDIG.
> + * For OTG disable force and clear IDDIG bit while at it.
> + */
> + switch (mode) {
> + case PHY_MODE_USB_DEVICE:
> + val = AIROHA_USB_PHY_IDDIG;
[Severity: High]
The comment mentions enabling force IDDIG for Device mode, but val is only
assigned AIROHA_USB_PHY_IDDIG here. Should AIROHA_USB_PHY_FORCE_IDDIG be
included as well?
> + break;
> + case PHY_MODE_USB_HOST:
> + val = AIROHA_USB_PHY_FORCE_IDDIG |
> + AIROHA_USB_PHY_FORCE_IDDIG;
[Severity: Low]
This isn't a bug, but the AIROHA_USB_PHY_FORCE_IDDIG flag is duplicated here.
> + break;
> + case PHY_MODE_USB_OTG:
> + val = 0;
> + break;
> + default:
> + return 0;
> + }
> +
> + regmap_update_bits(priv->regmap, AIROHA_USB_PHY_U2PHYDTM1,
> + AIROHA_USB_PHY_FORCE_IDDIG |
> + AIROHA_USB_PHY_IDDIG, val);
> +
> + return 0;
> +}
[ ... ]
> +static int an7581_usb_phy_probe(struct platform_device *pdev)
> +{
> + struct phy_provider *phy_provider;
> + struct an7581_usb_phy_priv *priv;
> + struct device *dev = &pdev->dev;
> + unsigned int index;
> + void __iomem *base;
> + int ret;
[ ... ]
> + for (index = 0; index < AIROHA_PHY_USB_MAX; index++) {
> + enum an7581_usb_phy_instance_type phy_type;
> + struct an7581_usb_phy_instance *instance;
> +
> + switch (index) {
> + case AIROHA_PHY_USB2:
> + phy_type = PHY_TYPE_USB2;
> + break;
> + case AIROHA_PHY_USB3:
> + phy_type = PHY_TYPE_USB3;
> + break;
> + }
[Severity: Low]
Should phy_type be declared as an integer or u32?
The generic PHY_TYPE_USB2 macro evaluates to 3, but phy_type is typed as
enum an7581_usb_phy_instance_type which only defines values 0 to 2.
> +
> + if (phy_type == PHY_TYPE_USB3) {
> + priv->serdes_phy = devm_phy_get(dev, NULL);
> + if (IS_ERR(priv->serdes_phy))
> + return dev_err_probe(dev, IS_ERR(priv->serdes_phy), "missing serdes phy for USB 3.0\n");
[Severity: High]
Does this inadvertently return 1 instead of a proper negative error code?
Since IS_ERR() returns a boolean, returning it breaks error propagation and
probe deferral (e.g. -EPROBE_DEFER). Should this be PTR_ERR() instead?
> + }
> +
> + instance = devm_kzalloc(dev, sizeof(*instance), GFP_KERNEL);
> + if (!instance)
> + return -ENOMEM;
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260519220813.28468-1-ansuelsmth@gmail.com?part=6
More information about the linux-phy
mailing list