Re: [PATCH 1/2] gspca: add input support for interrupt endpoints

[Jean-Francois Moine <moinejf@free.fr>]

On Sat, 28 Nov 2009 08:14:57 +0100
Németh Márton <nm127@freemail.hu> wrote:

> what do you think about the latest version of this patchset?

Hello Márton,

I wonder why you did not include the input functions directly in the
file gspca.c instead of adding new files and changing the Makefile.

Below are more remarks.

Regards.

	[snip]
> > diff -r bc16afd1e7a4 linux/drivers/media/video/gspca/gspca.c
> > --- a/linux/drivers/media/video/gspca/gspca.c	Sat Nov 21
> > 12:01:36 2009 +0100 +++
> > b/linux/drivers/media/video/gspca/gspca.c	Sun Nov 22
	[snip]
> > @@ -499,11 +502,13 @@
> >  			i, ep->desc.bEndpointAddress);
> >  	gspca_dev->alt = i;		/* memorize the current
> > alt setting */ if (gspca_dev->nbalt > 1) {
> > +		gspca_input_destroy_urb(gspca_dev);
> >  		ret = usb_set_interface(gspca_dev->dev,
> > gspca_dev->iface, i); if (ret < 0) {
> >  			err("set alt %d err %d", i, ret);
> > -			return NULL;
> > +			ep = NULL;
> >  		}
> > +		gspca_input_create_urb(gspca_dev);
> >  	}
> >  	return ep;
> >  }

If the interface cannot be set, I wonder if creating the input URB is
useful.

> > @@ -707,7 +712,9 @@
> >  		if (gspca_dev->sd_desc->stopN)
> >  			gspca_dev->sd_desc->stopN(gspca_dev);
> >  		destroy_urbs(gspca_dev);
> > +		gspca_input_destroy_urb(gspca_dev);
> >  		gspca_set_alt0(gspca_dev);
> > +		gspca_input_create_urb(gspca_dev);
> >  	}

Instead of destroying and recreating the input URB at each interface
change, it might be simpler to have a global function set_alt() and to
do the input job inside this one.

[snip]
> > diff -r bc16afd1e7a4 linux/drivers/media/video/gspca/input.c
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/drivers/media/video/gspca/input.c	Sun Nov 22
> > 16:40:34 2009 +0100 @@ -0,0 +1,184 @@
	[snip]
> > +EXPORT_SYMBOL(gspca_input_connect);

Don't do that: this function is not used by any other module.

> > +
> > +static int alloc_and_submit_int_urb(struct gspca_dev *gspca_dev,
> > +			  struct usb_endpoint_descriptor *ep)
> > +{
> > +	unsigned int buffer_len;
> > +	int interval;
> > +	struct urb *urb;
> > +	struct usb_device *dev;
> > +	void *buffer = NULL;
> > +	int ret = -EINVAL;
> > +
> > +	buffer_len = ep->wMaxPacketSize;
> > +	interval = ep->bInterval;
> > +	PDEBUG(D_PROBE, "found int in endpoint: 0x%x, "
> > +		"buffer_len=%u, interval=%u",
> > +		ep->bEndpointAddress, buffer_len, interval);
> > +
> > +	dev = gspca_dev->dev;
> > +	gspca_dev->int_urb = NULL;

Not useful: the descriptor is already set to zero.

> > +
> > +	urb = usb_alloc_urb(0, GFP_KERNEL);
> > +	if (!urb) {
> > +		ret = -ENOMEM;
> > +		goto error;
> > +	}
> > +
> > +	buffer = usb_buffer_alloc(dev, ep->wMaxPacketSize,
> > +				GFP_KERNEL, &urb->transfer_dma);
> > +	if (!buffer) {
> > +		ret = -ENOMEM;
> > +		goto error_buffer;
> > +	}
> > +	usb_fill_int_urb(urb, dev,
> > +		usb_rcvintpipe(dev, ep->bEndpointAddress),
> > +		buffer, buffer_len,
> > +		int_irq, (void *)gspca_dev, interval);
> > +	gspca_dev->int_urb = urb;

This instruction should go just before the normal return.

> > +	ret = usb_submit_urb(urb, GFP_KERNEL);
> > +	if (ret < 0) {
> > +		PDEBUG(D_ERR, "submit URB failed with error %i",
> > ret);
> > +		goto error_submit;
> > +	}
> > +	return ret;
> > +
> > +error_submit:
> > +	usb_buffer_free(dev,
> > +			urb->transfer_buffer_length,
> > +			urb->transfer_buffer,
> > +			urb->transfer_dma);
> > +error_buffer:
> > +	usb_free_urb(urb);
> > +error:
> > +	return ret;
> > +}
	[snip]
> > diff -r bc16afd1e7a4 linux/drivers/media/video/gspca/input.h
> > --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> > +++ b/linux/drivers/media/video/gspca/input.h	Sun Nov 22
> > 16:40:34 2009 +0100 @@ -0,0 +1,36 @@
	[snip]
> > +#ifdef CONFIG_INPUT
> > +int gspca_input_connect(struct gspca_dev *gspca_dev);
> > +int gspca_input_create_urb(struct gspca_dev *gspca_dev);
> > +void gspca_input_destroy_urb(struct gspca_dev *gspca_dev);
> > +#else
> > +#define gspca_input_connect(gspca_dev)		0
> > +#define gspca_input_create_urb(gspca_dev)	0
> > +#define gspca_input_destroy_urb(gspca_dev)

I better like empty functions, but this will be natural with the input
functions in the file gspca.c...

> > +#endif
> > +
> > +#endif

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/