RE: [PATCH 2/4] usb: uhci: Add reset control support

[Ryan Chen <ryan_chen@aspeedtech.com>]

> Subject: Re: [PATCH 2/4] usb: uhci: Add reset control support
> 
> On Mon, Sep 15, 2025 at 03:39:24PM +0800, Ryan Chen wrote:
> > Some SoCs, such as the Aspeed AST2700, require the UHCI controller to
> > be taken out of reset before it can operate. Add optional reset
> > control support to the UHCI platform driver.
> >
> > The driver now acquires an optional reset line from device tree,
> > deasserts it during probe, and asserts it again in the error path and
> > shutdown.
> >
> > Signed-off-by: Ryan Chen <ryan_chen@aspeedtech.com>
> > ---
> >  drivers/usb/host/uhci-hcd.h      |  1 +
> >  drivers/usb/host/uhci-platform.c | 19 +++++++++++++++++--
> >  2 files changed, 18 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/host/uhci-hcd.h b/drivers/usb/host/uhci-hcd.h
> > index 13ee2a6144b2..4326d1f3ca76 100644
> > --- a/drivers/usb/host/uhci-hcd.h
> > +++ b/drivers/usb/host/uhci-hcd.h
> > @@ -445,6 +445,7 @@ struct uhci_hcd {
> >  	short load[MAX_PHASE];			/* Periodic allocations */
> >
> >  	struct clk *clk;			/* (optional) clock source */
> > +	struct reset_control *rsts;		/* (optional) clock reset */
> >
> >  	/* Reset host controller */
> >  	void	(*reset_hc) (struct uhci_hcd *uhci);
> > diff --git a/drivers/usb/host/uhci-platform.c
> > b/drivers/usb/host/uhci-platform.c
> > index 62318291f566..010c458e7d8f 100644
> > --- a/drivers/usb/host/uhci-platform.c
> > +++ b/drivers/usb/host/uhci-platform.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/of.h>
> >  #include <linux/device.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/reset.h>
> >
> >  static int uhci_platform_init(struct usb_hcd *hcd)  { @@ -132,19
> > +133,33 @@ static int uhci_hcd_platform_probe(struct platform_device
> *pdev)
> >  		goto err_rmr;
> >  	}
> >
> > +	uhci->rsts = devm_reset_control_array_get_optional_shared(&pdev->dev);
> > +	if (IS_ERR(uhci->rsts)) {
> > +		ret = PTR_ERR(uhci->rsts);
> > +		goto err_clk;
> > +	}
> > +	ret = reset_control_deassert(uhci->rsts);
> > +	if (ret)
> > +		goto err_clk;
> > +
> >  	ret = platform_get_irq(pdev, 0);
> >  	if (ret < 0)
> > -		goto err_clk;
> > +		goto err_reset;
> >
> >  	ret = usb_add_hcd(hcd, ret, IRQF_SHARED);
> >  	if (ret)
> > -		goto err_clk;
> > +		goto err_reset;
> >
> >  	device_wakeup_enable(hcd->self.controller);
> >  	return 0;
> >
> >  err_clk:
> >  	clk_disable_unprepare(uhci->clk);
> > +
> > +err_reset:
> > +	if (!IS_ERR_OR_NULL(uhci->rsts))
> > +		reset_control_assert(uhci->rsts);
> 
> This error handler was added in the wrong place.  It should come before
> err_clk, so that if an error occurs in platform_get_irq(), the clock will be
> unprepared and disabled.

Thanks, will update in next version.

> 
> Alan Stern
> 
> > +
> >  err_rmr:
> >  	usb_put_hcd(hcd);
> >
> > --
> > 2.34.1