Re: [PATCH v2] usb: dwc3: gadget: move cmd_endtransfer to extra function

[Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx>]

Hi,

Michael Grzeschik wrote:
> This patch adds the extra function __dwc3_stop_active_transfer to
> consolidate the same codepath.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> ---

Add a comment here on what's changed since previous version.

>  drivers/usb/dwc3/gadget.c | 68 +++++++++++++++++++++------------------
>  1 file changed, 36 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 37dbf6132731b7..baddf6196ceee2 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1673,6 +1673,39 @@ static int __dwc3_gadget_get_frame(struct dwc3 *dwc)
>  	return DWC3_DSTS_SOFFN(reg);
>  }
>  
> +/**
> + * __dwc3_stop_active_transfer - stop the current active transfer
> + * @dep: isoc endpoint
> + * @force: set forcerm bit in the command
> + * @interrupt: command complete interrupt after End Transfer command
> + *
> + * When setting force, the ForceRM bit will be set. In that case
> + * the controller won't update the TRB progress on command
> + * completion. It also won't clear the HWO bit in the TRB.
> + * The command will also not complete immediatly in that case.

"immediately"

> + */

Remove blank line

> +
> +static void __dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force, bool interrupt)

Can we make this return "int"?

> +{
> +	struct dwc3_gadget_ep_cmd_params params;
> +	u32 cmd;
> +	int ret;
> +
> +	cmd = DWC3_DEPCMD_ENDTRANSFER;
> +	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
> +	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> +	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> +	memset(&params, 0, sizeof(params));
> +	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> +	WARN_ON_ONCE(ret);
> +	dep->resource_index = 0;
> +
> +	if (!interrupt)
> +		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> +	else if (!ret)
> +		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;

So we can return ret here.

> +}
> +
>  /**
>   * dwc3_gadget_start_isoc_quirk - workaround invalid frame number
>   * @dep: isoc endpoint
> @@ -1842,21 +1875,8 @@ static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  	 * status, issue END_TRANSFER command and retry on the next XferNotReady
>  	 * event.
>  	 */
> -	if (ret == -EAGAIN) {
> -		struct dwc3_gadget_ep_cmd_params params;
> -		u32 cmd;
> -
> -		cmd = DWC3_DEPCMD_ENDTRANSFER |
> -			DWC3_DEPCMD_CMDIOC |
> -			DWC3_DEPCMD_PARAM(dep->resource_index);
> -
> -		dep->resource_index = 0;
> -		memset(&params, 0, sizeof(params));
> -
> -		ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -		if (!ret)
> -			dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> -	}
> +	if (ret == -EAGAIN)
> +		__dwc3_stop_active_transfer(dep, false, true);

if (ret == -EAGAIN)
	ret = __dwc3_stop_active_transfer(dep, false, true);

>  
>  	return ret;
>  }
> @@ -3597,10 +3617,6 @@ static void dwc3_reset_gadget(struct dwc3 *dwc)
>  static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	bool interrupt)
>  {
> -	struct dwc3_gadget_ep_cmd_params params;
> -	u32 cmd;
> -	int ret;
> -
>  	if (!(dep->flags & DWC3_EP_TRANSFER_STARTED) ||
>  	    (dep->flags & DWC3_EP_END_TRANSFER_PENDING))
>  		return;
> @@ -3632,19 +3648,7 @@ static void dwc3_stop_active_transfer(struct dwc3_ep *dep, bool force,
>  	 * This mode is NOT available on the DWC_usb31 IP.
>  	 */
>  
> -	cmd = DWC3_DEPCMD_ENDTRANSFER;
> -	cmd |= force ? DWC3_DEPCMD_HIPRI_FORCERM : 0;
> -	cmd |= interrupt ? DWC3_DEPCMD_CMDIOC : 0;
> -	cmd |= DWC3_DEPCMD_PARAM(dep->resource_index);
> -	memset(&params, 0, sizeof(params));
> -	ret = dwc3_send_gadget_ep_cmd(dep, cmd, &params);
> -	WARN_ON_ONCE(ret);
> -	dep->resource_index = 0;
> -
> -	if (!interrupt)
> -		dep->flags &= ~DWC3_EP_TRANSFER_STARTED;
> -	else
> -		dep->flags |= DWC3_EP_END_TRANSFER_PENDING;
> +	__dwc3_stop_active_transfer(dep, force, interrupt);
>  }
>  
>  static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)

Thanks,
Thinh