Re: [PATCHv6 6/8] usb: dwc2: gadget: Do not fail probe if there isn't a clock node

[Felipe Balbi <balbi@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 4637 bytes --]

Hi,

On Tue, Oct 28, 2014 at 06:25:47PM -0500, dinguyen@opensource.altera.com wrote:
> From: Dinh Nguyen <dinguyen@opensource.altera.com>
> 
> Since the dwc2 hcd driver is currently not looking for a clock node during
> init, we should not completely fail if there isn't a clock provided.
> For dual-role mode, we will only fail init for a non-clock node error. We
> then update the HCD to only call gadget funtions if there is a proper clock
> node.
> 
> Signed-off-by: Dinh Nguyen <dinguyen@opensource.altera.com>
> ---
> v5: reworked to not access gadget functions from the hcd.
> ---
>  drivers/usb/dwc2/core.h      |  3 +--
>  drivers/usb/dwc2/core_intr.c |  9 ++++++---
>  drivers/usb/dwc2/hcd.c       |  3 ++-
>  drivers/usb/dwc2/platform.c  | 19 +++++++++++++++----
>  4 files changed, 24 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
> index ec70862..48120c8 100644
> --- a/drivers/usb/dwc2/core.h
> +++ b/drivers/usb/dwc2/core.h
> @@ -660,6 +660,7 @@ struct dwc2_hsotg {
>  #endif
>  #endif /* CONFIG_USB_DWC2_HOST || CONFIG_USB_DWC2_DUAL_ROLE */
>  
> +	struct clk *clk;
>  #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || IS_ENABLED(CONFIG_USB_DWC2_DUAL_ROLE)
>  	/* Gadget structures */
>  	struct usb_gadget_driver *driver;
> @@ -667,8 +668,6 @@ struct dwc2_hsotg {
>  	struct usb_phy *uphy;
>  	struct s3c_hsotg_plat *plat;
>  
> -	struct clk *clk;
> -
>  	struct regulator_bulk_data supplies[ARRAY_SIZE(s3c_hsotg_supply_names)];
>  
>  	u32 phyif;
> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
> index 1240875..1608037 100644
> --- a/drivers/usb/dwc2/core_intr.c
> +++ b/drivers/usb/dwc2/core_intr.c
> @@ -339,7 +339,8 @@ static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>  		}
>  		/* Change to L0 state */
>  		hsotg->lx_state = DWC2_L0;
> -		call_gadget(hsotg, resume);
> +		if (!IS_ERR(hsotg->clk))
> +			call_gadget(hsotg, resume);

instead of exposing the clock detail to the entire driver, add IS_ERR()
checks to resume and suspend instead. In fact, NULL is a valid clock, so
you might as well:

	clk = clk_get(foo, bar);
	if (IS_ERR(clk))
		dwc->clk = NULL;
	else
		dwc->clk = clk;

Then you don't need any IS_ERR() checks sprinkled around the driver.

> @@ -400,7 +401,8 @@ static void dwc2_handle_usb_suspend_intr(struct dwc2_hsotg *hsotg)
>  			"DSTS.Suspend Status=%d HWCFG4.Power Optimize=%d\n",
>  			!!(dsts & DSTS_SUSPSTS),
>  			hsotg->hw_params.power_optimized);
> -		call_gadget(hsotg, suspend);
> +		if (!IS_ERR(hsotg->clk))
> +			call_gadget(hsotg, suspend);
>  	} else {
>  		if (hsotg->op_state == OTG_STATE_A_PERIPHERAL) {
>  			dev_dbg(hsotg->dev, "a_peripheral->a_host\n");
> @@ -477,7 +479,8 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev)
>  	spin_lock(&hsotg->lock);
>  
>  	if (dwc2_is_device_mode(hsotg))
> -		retval = s3c_hsotg_irq(irq, dev);
> +		if (!IS_ERR(hsotg->clk))
> +			retval = s3c_hsotg_irq(irq, dev);

wait a minute, if there is no clock we don't call the gadget interrupt
handler ? Why ? Who will disable the IRQ line ?

> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 44c609f..fa49c72 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -1371,7 +1371,8 @@ static void dwc2_conn_id_status_change(struct work_struct *work)
>  		hsotg->op_state = OTG_STATE_B_PERIPHERAL;
>  		dwc2_core_init(hsotg, false, -1);
>  		dwc2_enable_global_interrupts(hsotg);
> -		s3c_hsotg_core_init(hsotg);
> +		if (!IS_ERR(hsotg->clk))
> +			s3c_hsotg_core_init(hsotg);
>  	} else {
>  		/* A-Device connector (Host Mode) */
>  		dev_dbg(hsotg->dev, "connId A\n");
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 72f32f7..77c8417 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -217,8 +217,17 @@ static int dwc2_driver_probe(struct platform_device *dev)
>  
>  	spin_lock_init(&hsotg->lock);
>  	retval = dwc2_gadget_init(hsotg, irq);
> -	if (retval)
> -		return retval;
> +	if (retval) {
> +		/*
> +		 * We will not fail the driver initialization for dual-role
> +		 * if no clock node is supplied. However, all gadget
> +		 * functionality will be disabled if a clock node is not
> +		 * provided. Host functionality will continue.
> +		 * TO-DO: make clock node a requirement for the HCD.
> +		 */
> +		if (!IS_ERR(hsotg->clk))
> +			return retval;
> +	}

no here... this should have been taken care by dwc2_gadget_init()
itself.

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]