Re: [PATCH v2 07/11] mfd: omap-usb-host: Refactor suspend and resume callbacks

[Lee Jones <lee@kernel.org>]

On Mon, 30 Mar 2026, Thomas Richard wrote:

> The clock handling logic in suspend and resume callbacks is very similar.
> Create a new usbhs_clocks_enable() function to avoid code duplication.
> Also remove ftrace-like debug messages.

They're not similar at all - they use opposing calls.

Bundling them up like this makes them _more_ complicated and hurts readability IMHO.

> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
>  drivers/mfd/omap-usb-host.c | 94 ++++++++++++++++++++++-----------------------
>  1 file changed, 46 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/mfd/omap-usb-host.c b/drivers/mfd/omap-usb-host.c
> index ac974285be34..17a54f0087c3 100644
> --- a/drivers/mfd/omap-usb-host.c
> +++ b/drivers/mfd/omap-usb-host.c
> @@ -270,48 +270,56 @@ static bool is_ohci_port(enum usbhs_omap_port_mode pmode)
>  	}
>  }
>  
> -static int usbhs_runtime_resume(struct device *dev)
> +static int usbhs_clocks_enable(struct device *dev, bool enable)
>  {
> -	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
> -	struct usbhs_omap_platform_data	*pdata = omap->pdata;
> -	int i, r;
> -
> -	dev_dbg(dev, "usbhs_runtime_resume\n");
> +	struct usbhs_hcd_omap *omap = dev_get_drvdata(dev);
> +	struct usbhs_omap_platform_data *pdata = omap->pdata;
> +	int r = 0, i;
>  
> -	omap_tll_enable(pdata);
> -
> -	if (!IS_ERR(omap->ehci_logic_fck))
> -		clk_prepare_enable(omap->ehci_logic_fck);
> +	if (!enable && !IS_ERR(omap->ehci_logic_fck))
> +		clk_disable_unprepare(omap->ehci_logic_fck);
>  
>  	for (i = 0; i < omap->nports; i++) {
>  		switch (pdata->port_mode[i]) {
>  		case OMAP_EHCI_PORT_MODE_HSIC:
>  			if (!IS_ERR(omap->hsic60m_clk[i])) {
> -				r = clk_prepare_enable(omap->hsic60m_clk[i]);
> -				if (r) {
> -					dev_err(dev,
> -					 "Can't enable port %d hsic60m clk:%d\n",
> -					 i, r);
> +				if (enable) {
> +					r = clk_prepare_enable(omap->hsic60m_clk[i]);
> +					if (r) {
> +						dev_err(dev,
> +							"Can't enable port %d hsic60m clk:%d\n",
> +							i, r);
> +					}
> +				} else {
> +					clk_disable_unprepare(omap->hsic60m_clk[i]);
>  				}
>  			}
>  
>  			if (!IS_ERR(omap->hsic480m_clk[i])) {
> -				r = clk_prepare_enable(omap->hsic480m_clk[i]);
> -				if (r) {
> -					dev_err(dev,
> -					 "Can't enable port %d hsic480m clk:%d\n",
> -					 i, r);
> +				if (enable) {
> +					r = clk_prepare_enable(omap->hsic480m_clk[i]);
> +					if (r) {
> +						dev_err(dev,
> +							"Can't enable port %d hsic480m clk:%d\n",
> +							i, r);
> +					}
> +				} else {
> +					clk_disable_unprepare(omap->hsic480m_clk[i]);
>  				}
>  			}
>  			fallthrough;	/* as HSIC mode needs utmi_clk */
>  
>  		case OMAP_EHCI_PORT_MODE_TLL:
>  			if (!IS_ERR(omap->utmi_clk[i])) {
> -				r = clk_prepare_enable(omap->utmi_clk[i]);
> -				if (r) {
> -					dev_err(dev,
> -					 "Can't enable port %d clk : %d\n",
> -					 i, r);
> +				if (enable) {
> +					r = clk_prepare_enable(omap->utmi_clk[i]);
> +					if (r) {
> +						dev_err(dev,
> +							"Can't enable port %d clk : %d\n",
> +							i, r);
> +					}
> +				} else {
> +					clk_disable_unprepare(omap->utmi_clk[i]);
>  				}
>  			}
>  			break;
> @@ -320,38 +328,28 @@ static int usbhs_runtime_resume(struct device *dev)
>  		}
>  	}
>  
> -	return 0;
> +	if (enable && !IS_ERR(omap->ehci_logic_fck))
> +		r = clk_prepare_enable(omap->ehci_logic_fck);
> +
> +	return r;
>  }
>  
> -static int usbhs_runtime_suspend(struct device *dev)
> +static int usbhs_runtime_resume(struct device *dev)
>  {
>  	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
>  	struct usbhs_omap_platform_data	*pdata = omap->pdata;
> -	int i;
>  
> -	dev_dbg(dev, "usbhs_runtime_suspend\n");
> -
> -	for (i = 0; i < omap->nports; i++) {
> -		switch (pdata->port_mode[i]) {
> -		case OMAP_EHCI_PORT_MODE_HSIC:
> -			if (!IS_ERR(omap->hsic60m_clk[i]))
> -				clk_disable_unprepare(omap->hsic60m_clk[i]);
> +	omap_tll_enable(pdata);
>  
> -			if (!IS_ERR(omap->hsic480m_clk[i]))
> -				clk_disable_unprepare(omap->hsic480m_clk[i]);
> -			fallthrough;	/* as utmi_clks were used in HSIC mode */
> +	return usbhs_clocks_enable(dev, true);
> +}
>  
> -		case OMAP_EHCI_PORT_MODE_TLL:
> -			if (!IS_ERR(omap->utmi_clk[i]))
> -				clk_disable_unprepare(omap->utmi_clk[i]);
> -			break;
> -		default:
> -			break;
> -		}
> -	}
> +static int usbhs_runtime_suspend(struct device *dev)
> +{
> +	struct usbhs_hcd_omap		*omap = dev_get_drvdata(dev);
> +	struct usbhs_omap_platform_data	*pdata = omap->pdata;
>  
> -	if (!IS_ERR(omap->ehci_logic_fck))
> -		clk_disable_unprepare(omap->ehci_logic_fck);
> +	usbhs_clocks_enable(dev, false);
>  
>  	omap_tll_disable(pdata);
>  
> 
> -- 
> 2.53.0
> 

-- 
Lee Jones