Re: [PATCH v7 4/6] phy: fsl-imx8mq-usb: add control register regmap

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Incorrect error handling of pm_runtime_get_sync() treats a successful active state as a failure.
- [Low] Missing Kconfig dependency for REGMAP_MMIO.
--

--- Patch [4]: [PATCH v7 4/6] phy: fsl-imx8mq-usb: add control register regmap ---
commit 3be8b97176fdd5e159a3f1a8541ee32a8755658b
Author: Xu Yang <[email protected]>

phy: fsl-imx8mq-usb: add control register regmap

This commit adds a 32-bit MMIO control register regmap for on-chip access
to the control registers inside the USB 3.0 femtoPHY. It exports these
registers via debugfs to help with PHY diagnostic.

> diff --git a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> index 74e665525ec48..83d5a44c40848 100644
> --- a/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
> +++ b/drivers/phy/freescale/phy-fsl-imx8mq-usb.c
[ ... ]
> @@ -696,6 +708,13 @@ static int imx8mq_usb_phy_probe(struct platform_device *pdev)
>  	if (IS_ERR(imx_phy->base))
>  		return PTR_ERR(imx_phy->base);
>  
> +	imx_phy->cr_regmap = devm_regmap_init_mmio(dev, imx_phy->base + PHY_CRCTL,
> +						   &imx_cr_regmap_config);

[Severity: Low]
Does this new regmap initialization require a Kconfig update?

Since the driver now uses devm_regmap_init_mmio(), it depends on the MMIO
regmap subsystem. However, PHY_FSL_IMX8MQ_USB in drivers/phy/freescale/Kconfig
does not select REGMAP_MMIO.

Could this lead to build failures (such as missing module or undefined
reference to __devm_regmap_init_mmio_clk) in configurations where no other
component selects REGMAP_MMIO?

> +	if (IS_ERR(imx_phy->cr_regmap)) {
> +		dev_warn(dev, "Fail to init debug register regmap\n");
> +		imx_phy->cr_regmap = NULL;
> +	}
> +
[ ... ]

[Severity: Medium]
Looking at the driver's unbind path in imx8mq_usb_phy_remove():

static void imx8mq_usb_phy_remove(struct platform_device *pdev)
{
	...
	ret = pm_runtime_get_sync(dev);
	if (ret)
		dev_warn(dev, "failed to resume on remove: %d\n", ret);

According to the PM subsystem rules, pm_runtime_get_sync() returns 1 when the
device is already active. Does checking if (ret) instead of if (ret < 0)
treat this success condition as a failure, potentially causing a spurious
kernel warning during driver unbind?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4