Re: [PATCH 3/3] USB: mxu11x0: move firmware download and endpoint testing to probe callback

[Johan Hovold <johan@kernel.org>]

On Sun, Jan 03, 2016 at 03:26:01PM +0100, Mathieu OTHACEHE wrote:
> Move interrupt in endpoint test and firmware download to a new probe
> callback. This avoids unnecessary memory allocations done by core
> before port_probe callback is called.
> 
> If the device has to be reseted (firmware downloaded) or if the
> interface is incorrect (no interrupt in endpoint), the probe fails
> by returning ENODEV.
> 
> Signed-off-by: Mathieu OTHACEHE <m.othacehe@gmail.com>
> ---
>  drivers/usb/serial/mxu11x0.c | 130 ++++++++++++++++++++++++++-----------------
>  1 file changed, 78 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/usb/serial/mxu11x0.c b/drivers/usb/serial/mxu11x0.c
> index 354fcb5..0392531 100644
> --- a/drivers/usb/serial/mxu11x0.c
> +++ b/drivers/usb/serial/mxu11x0.c
> @@ -272,7 +272,8 @@ static int mxu1_send_ctrl_urb(struct usb_serial *serial,
>  }
>  
>  static int mxu1_download_firmware(struct usb_serial *serial,
> -				  const struct firmware *fw_p)
> +				  const struct firmware *fw_p,
> +				  struct usb_endpoint_descriptor *endpoint)
>  {
>  	int status = 0;
>  	int buffer_size;
> @@ -285,7 +286,7 @@ static int mxu1_download_firmware(struct usb_serial *serial,
>  	struct mxu1_firmware_header *header;
>  	unsigned int pipe;
>  
> -	pipe = usb_sndbulkpipe(dev, serial->port[0]->bulk_out_endpointAddress);
> +	pipe = usb_sndbulkpipe(dev, endpoint->bEndpointAddress);
>  
>  	buffer_size = fw_p->size + sizeof(*header);
>  	buffer = kmalloc(buffer_size, GFP_KERNEL);
> @@ -336,16 +337,85 @@ static void mxu1_release(struct usb_serial *serial)
>  	kfree(mxdev);
>  }
>  
> -static int mxu1_port_probe(struct usb_serial_port *port)
> +static int mxu1_probe(struct usb_serial *serial, const struct usb_device_id *id)
>  {
> -	struct mxu1_port *mxport;
> -	struct mxu1_device *mxdev;
> +	struct usb_host_interface *cur_altsetting;
> +	char fw_name[32];
> +	const struct firmware *fw_p = NULL;
> +	struct usb_device *dev = serial->dev;
> +	u16 model;
> +	int err;
> +	struct usb_endpoint_descriptor *endpoint, *interrupt_in, *bulk_out;
> +	int i;
> +
> +	dev_dbg(&serial->interface->dev, "%s - product 0x%04X, num configurations %d, configuration value %d\n",
> +		__func__, le16_to_cpu(dev->descriptor.idProduct),
> +		dev->descriptor.bNumConfigurations,
> +		dev->actconfig->desc.bConfigurationValue);
> +
> +	cur_altsetting = serial->interface->cur_altsetting;
> +	interrupt_in = NULL;
> +	bulk_out = NULL;
> +
> +	for (i = 0; i < cur_altsetting->desc.bNumEndpoints; i++) {
> +		endpoint = &cur_altsetting->endpoint[i].desc;
> +		if (usb_endpoint_is_bulk_out(endpoint)) {
> +			dev_dbg(&serial->interface->dev,
> +				"found bulk out on endpoint %d\n", i);
> +			bulk_out = endpoint;
> +		}
> +
> +		if (usb_endpoint_is_int_in(endpoint)) {
> +			dev_dbg(&serial->interface->dev,
> +				"found interrupt in on endpoint %d\n", i);
> +			interrupt_in = endpoint;
> +		}
> +	}
> +
> +	/* if we have only 1 bulk out endpoint, download firmware */
> +	if (bulk_out && (cur_altsetting->desc.bNumEndpoints == 1)) {

No need for inner parentheses.

> +
> +		model = le16_to_cpu(dev->descriptor.idProduct);
> +		snprintf(fw_name,
> +			 sizeof(fw_name),
> +			 "moxa/moxa-%04x.fw",
> +			 model);
> +
> +		err = request_firmware(&fw_p, fw_name, &serial->interface->dev);
> +		if (err) {
> +			dev_err(&serial->interface->dev, "failed to request firmware: %d\n",
> +				err);
> +			return -ENODEV;
> +		}
> +
> +		err = mxu1_download_firmware(serial, fw_p, bulk_out);
> +		if (err)
> +			goto err_release_firmware;
>  
> -	if (!port->interrupt_in_urb) {
> -		dev_err(&port->dev, "no interrupt urb\n");
> +		/* device is being reset */
> +		err = -ENODEV;
> +		goto err_release_firmware;
> +
> +	} else if (!interrupt_in) {
> +		/* firmware is already loaded but there is
> +		 * no interrupt in endpoint available
> +		 */

Multi-line comments should be on the following format

	/*
	 * ...
	 */

Looks good otherwise.

Johan