Re: [PATCH 2/3] 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_end_transfer to consolidate the
> same codepath.
> 
> Signed-off-by: Michael Grzeschik <m.grzeschik@xxxxxxxxxxxxxx>
> ---
>  drivers/usb/dwc3/gadget.c | 56 +++++++++++++++++----------------------
>  1 file changed, 24 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 0ed837323f6bd3..b89dadaef4db9d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1788,6 +1788,27 @@ static int dwc3_gadget_start_isoc_quirk(struct dwc3_ep *dep)
>  	return __dwc3_gadget_kick_transfer(dep);
>  }
>  
> +static void dwc3_end_transfer(struct dwc3_ep *dep, bool force, bool interrupt)

Maybe we can name this as __dwc3_stop_active_transfer() to be a bit more
consistent in other places of this driver?

Place this function above dwc3_gadget_start_isoc_quirk(). Also, can we
write a brief doc describing this function here as well? I got a feeling
that not many know what setting "force" mean to the controller. If
ForceRM is set, then the controller won't update the TRB progress on
command completion or clearing HWO bit. It doesn't mean that the command
will complete immediately.

Thanks,
Thinh

> +{
> +	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;
> +}
> +
>  static int __dwc3_gadget_start_isoc(struct dwc3_ep *dep)
>  {
>  	const struct usb_endpoint_descriptor *desc = dep->endpoint.desc;
> @@ -1842,21 +1863,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_end_transfer(dep, false, true);
>  
>  	return ret;
>  }
> @@ -3597,10 +3605,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 +3636,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_end_transfer(dep, force, interrupt);
>  }
>  
>  static void dwc3_clear_stall_all_ep(struct dwc3 *dwc)