Re: [PATCH v1 3/3] usb: dwc3-generic: Add Starfive JHB100 USB 2.0 device controller and adapter

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] usb: improper handling of optional adapter resource masks legitimate errors
- [Low] usb: missing static keyword on jhb100_glue_ops causes namespace pollution
--

commit e26d59375d5e4555d239557e241788e437655348
Author: Minda Chen <minda.chen@starfivetech.com>

usb: dwc3-generic: Add Starfive JHB100 USB 2.0 device controller and adapter

This patch introduces the Starfive JHB100 USB 2.0 device controller and
its associated PHY adapter settings to the generic DWC3 driver. It adds
io memory mappings for the adapter and implements pre-run/stop operations
to appropriately toggle the adapter port enable bit during gadget enumeration.

diff --git a/drivers/usb/dwc3/dwc3-generic-plat.c b/drivers/usb/dwc3/dwc3-generic-plat.c
index ca69ac0eb07ce..e3394205a5197 100644
--- a/drivers/usb/dwc3/dwc3-generic-plat.c
+++ b/drivers/usb/dwc3/dwc3-generic-plat.c

[ ... ]

> +static void dwc3_jhb100_pre_run_stop(struct dwc3 *dwc3, bool start)
> +{
> +	struct dwc3_generic *data = to_dwc3_generic(dwc3);
> +	u32	val;
> +
> +	if (start) {
> +		val = readl(data->phy_regs);
> +		val |= JHB100_ADAPTER_PORT_EN;
> +		writel(val, data->phy_regs);
> +	} else {
> +		val = readl(data->phy_regs);
> +		val &= ~JHB100_ADAPTER_PORT_EN;
> +		writel(val, data->phy_regs);
> +	}
> +}
> +
> +struct dwc3_glue_ops jhb100_glue_ops = {

[Severity: Low]
Could this struct be marked static and const?

Since jhb100_glue_ops is only assigned to dwc3g->dwc.glue_ops locally within
drivers/usb/dwc3/dwc3-generic-plat.c, leaving it globally exposed might lead
to link-time symbol collisions or sparse warnings.

> +	.pre_run_stop = dwc3_jhb100_pre_run_stop,
> +};
> +
> +static int dwc3_jhb100_init(struct dwc3_generic *dwc3g)
> +{
> +	struct platform_device *pdev = to_platform_device(dwc3g->dev);
> +
> +	dwc3g->phy_regs = devm_platform_ioremap_resource_byname(pdev, "adapter");
> +	if (IS_ERR(dwc3g->phy_regs)) {

[Severity: Medium]
Will this error handling mask legitimate failures?

The adapter resource appears to be optional here since dwc3_jhb100_init() falls
back to generic behavior when it is missing. However, using
devm_platform_ioremap_resource_byname() on an absent resource will
unconditionally log an invalid resource error via dev_err_probe(), potentially
spamming the kernel log on platforms where it is intentionally omitted.

Additionally, checking IS_ERR() here will catch all other errors like -ENOMEM
or -EBUSY and silently downgrade capabilities by returning 0, instead of
propagating the real failure to the caller.

> +		dwc3g->phy_regs = NULL;
> +		/* Generic bmc USB with PHY*/
> +		return 0;
> +	}
> +
> +	/* usb device controller with adapter */
> +	dwc3g->dwc.glue_ops = &jhb100_glue_ops;
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260918071103.24482-1-minda.chen@starfivetech.com?part=3