Re: [PATCH v2 2/2] phy: add a driver for T-Head TH1520 USB PHY

On Thu, Jul 23, 2026 at 08:55:27AM +0000, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [Low] Missing explicit inclusion of `<linux/delay.h>` for `udelay()`.
[..]
> > +static int th1520_usb_phy_init(struct phy *phy)
> > +{
> > +	struct th1520_usb_phy *th1520_phy = phy_get_drvdata(phy);
> > +	int ret;
> > +
> > +	ret = clk_prepare_enable(th1520_phy->ref_clk);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = reset_control_assert(th1520_phy->phy_reset);
> > +	if (ret)
> > +		goto err_disable_clk;
> > +
> > +	/*
> > +	 * Do some initial PHY setup:
> > +	 * - Set COMMONONN to allow the PHY to automatically power down.
> > +	 * - Set REF_SSP_EN to enable feeding reference clock to SuperSpeed
> > +	 *   PHY clock PLL.
> > +	 */
> > +	regmap_set_bits(th1520_phy->regmap, USB_SYS, USB_SYS_COMMONONN);
> > +	regmap_set_bits(th1520_phy->regmap, USB_SSP_EN, USB_SSP_EN_REF_SSP_EN);
> > +
> > +	ret = reset_control_deassert(th1520_phy->phy_reset);
> > +	if (ret)
> > +		goto err_disable_clk;
> > +
> > +	udelay(10);
> 
> [Severity: Low]
> This isn't a bug, but this code uses udelay() without explicitly including
> <linux/delay.h>.
> 
> While it currently compiles due to indirect inclusions, should the header be
> explicitly included to prevent build failures if the include chain is later
> refactored?

I noticed that checkpatch complains about 'udelay()':

CHECK: usleep_range is preferred over udelay; see function description
of usleep_range() and udelay().

However, it seems that fsleep(10) may be a better choice than
usleep_range(). It is used by phy-snps-eusb2.c, phy-rzg3e-usb3.c and
phy-exynos5-usbdrd.c.

From include/linux/delay.h:

static inline void fsleep(unsigned long usecs)
{
        if (usecs <= 10)
                udelay(usecs);
        else if (usecs < USLEEP_RANGE_UPPER_BOUND)
                usleep_range(usecs, usecs + (usecs >> max_slack_shift));
        else
                msleep(DIV_ROUND_UP(usecs, USEC_PER_MSEC));
}

Thanks,
Drew