Re: [PATCH 6/6] usb: dwc3: gadget: Convert epautoconf workaround to match_ep callback

[Mattijs Korpershoek <mkorpershoek@baylibre.com>]

Hi Marek,

Thank you for the patch.

On dim., juin 09, 2024 at 23:32, Marek Vasut <marek.vasut+renesas@mailbox.org> wrote:

> Use the .match_ep() callback instead of workaround in core code.
> Replace descriptor parsing with ch9 macros with the same effect.
> Drop the SPL specific behavior, it is unclear why SPL should even
> be special.

Li, Peng,

Is this good for you as well?

You seem to be the author/committer of:
c93edbf5385e ("usb: gadget: don't change ep name for dwc3 while ep autoconfig")

I'd like to make sure this patch does not break your use-case.
Please let me know within 2 weeks or so, otherwise I'll apply the changes.

>
> Signed-off-by: Marek Vasut <marek.vasut+renesas@mailbox.org>

To me, this looks good.

Reviewed-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
Tested-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> # on vim3

> ---
> Cc: Alexander Sverdlin <alexander.sverdlin@siemens.com>
> Cc: Felipe Balbi <felipe.balbi@linux.intel.com>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Cc: Nishanth Menon <nm@ti.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> Cc: Tom Rini <trini@konsulko.com>
> Cc: u-boot@lists.denx.de
> ---
>  drivers/usb/dwc3/gadget.c       | 33 +++++++++++++++++++++++
>  drivers/usb/gadget/epautoconf.c | 46 +--------------------------------
>  2 files changed, 34 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index fab32575647..3ef2f016a60 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1606,6 +1606,38 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
>  	return 0;
>  }
>  
> +static struct usb_ep *dwc3_find_ep(struct usb_gadget *gadget, const char *name)
> +{
> +	struct usb_ep *ep;
> +
> +	list_for_each_entry(ep, &gadget->ep_list, ep_list)
> +		if (!strcmp(ep->name, name))
> +			return ep;
> +
> +	return NULL;
> +}
> +
> +static struct
> +usb_ep *dwc3_gadget_match_ep(struct usb_gadget *gadget,
> +			     struct usb_endpoint_descriptor *desc,
> +			     struct usb_ss_ep_comp_descriptor *comp_desc)
> +{
> +	/*
> +	 * First try standard, common configuration: ep1in-bulk,
> +	 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
> +	 * confusion in already deployed software (endpoint numbers
> +	 * hardcoded in userspace software/drivers)
> +	 */
> +	if (usb_endpoint_is_bulk_in(desc))
> +		return dwc3_find_ep(gadget, "ep1in");
> +	if (usb_endpoint_is_bulk_out(desc))
> +		return dwc3_find_ep(gadget, "ep2out");
> +	if (usb_endpoint_is_int_in(desc))
> +		return dwc3_find_ep(gadget, "ep3in");
> +
> +	return NULL;
> +}
> +
>  static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.get_frame		= dwc3_gadget_get_frame,
>  	.wakeup			= dwc3_gadget_wakeup,
> @@ -1613,6 +1645,7 @@ static const struct usb_gadget_ops dwc3_gadget_ops = {
>  	.pullup			= dwc3_gadget_pullup,
>  	.udc_start		= dwc3_gadget_start,
>  	.udc_stop		= dwc3_gadget_stop,
> +	.match_ep		= dwc3_gadget_match_ep,
>  };
>  
>  /* -------------------------------------------------------------------------- */
> diff --git a/drivers/usb/gadget/epautoconf.c b/drivers/usb/gadget/epautoconf.c
> index 66599ce8efa..a4da4f72de9 100644
> --- a/drivers/usb/gadget/epautoconf.c
> +++ b/drivers/usb/gadget/epautoconf.c
> @@ -166,18 +166,6 @@ static int ep_matches(
>  	return 1;
>  }
>  
> -static struct usb_ep *
> -find_ep(struct usb_gadget *gadget, const char *name)
> -{
> -	struct usb_ep	*ep;
> -
> -	list_for_each_entry(ep, &gadget->ep_list, ep_list) {
> -		if (0 == strcmp(ep->name, name))
> -			return ep;
> -	}
> -	return NULL;
> -}
> -
>  /**
>   * usb_ep_autoconfig - choose an endpoint matching the descriptor
>   * @gadget: The device to which the endpoint must belong.
> @@ -213,39 +201,7 @@ struct usb_ep *usb_ep_autoconfig(
>  	struct usb_endpoint_descriptor	*desc
>  )
>  {
> -	struct usb_ep	*ep = NULL;
> -	u8		type;
> -
> -	type = desc->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK;
> -
> -	/* First, apply chip-specific "best usage" knowledge.
> -	 * This might make a good usb_gadget_ops hook ...
> -	 */
> -	if (!IS_ENABLED(CONFIG_SPL_BUILD) &&
> -	    IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
> -	    !strcmp("dwc3-gadget", gadget->name)) {
> -		const char *name = NULL;
> -		/*
> -		 * First try standard, common configuration: ep1in-bulk,
> -		 * ep2out-bulk, ep3in-int to match other udc drivers to avoid
> -		 * confusion in already deployed software (endpoint numbers
> -		 * hardcoded in userspace software/drivers)
> -		 */
> -		if ((desc->bEndpointAddress & USB_DIR_IN) &&
> -		    type == USB_ENDPOINT_XFER_BULK)
> -			name = "ep1in";
> -		else if ((desc->bEndpointAddress & USB_DIR_IN) == 0 &&
> -			 type == USB_ENDPOINT_XFER_BULK)
> -			name = "ep2out";
> -		else if ((desc->bEndpointAddress & USB_DIR_IN) &&
> -			 type == USB_ENDPOINT_XFER_INT)
> -			name = "ep3in";
> -
> -		if (name)
> -			ep = find_ep(gadget, name);
> -		if (ep && ep_matches(gadget, ep, desc))
> -			return ep;
> -	}
> +	struct usb_ep *ep;
>  
>  	if (gadget->ops->match_ep) {
>  		ep = gadget->ops->match_ep(gadget, desc, NULL);
> -- 
> 2.43.0