[1/2,RFC] gspca: add input support for interrupt endpoints

  From: Márton Németh <nm127@freemail.hu>

Add support functions for interrupt endpoint based input handling.

Signed-off-by: Márton Németh <nm127@freemail.hu>
---
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hi,

Thanks for your continued work on this. I'm afraid I found
one thing which needs fixing (can be fixed with
a separate patch after merging, but that is up to
Jean-Francois).

See my comments inline.

On 01/17/2010 02:08 PM, Németh Márton wrote:
> From: Márton Németh<nm127@freemail.hu>
>
> Add support functions for interrupt endpoint based input handling.
>
> Signed-off-by: Márton Németh<nm127@freemail.hu>
> ---
> diff -r 875c200a19dc linux/drivers/media/video/gspca/gspca.c
> --- a/linux/drivers/media/video/gspca/gspca.c	Sun Jan 17 07:58:51 2010 +0100
> +++ b/linux/drivers/media/video/gspca/gspca.c	Sun Jan 17 13:47:44 2010 +0100
> @@ -3,6 +3,9 @@
>    *
>    * Copyright (C) 2008-2009 Jean-Francois Moine (http://moinejf.free.fr)
>    *
> + * Camera button input handling by Márton Németh
> + * Copyright (C) 2009-2010 Márton Németh<nm127@freemail.hu>
> + *
>    * This program is free software; you can redistribute it and/or modify it
>    * under the terms of the GNU General Public License as published by the
>    * Free Software Foundation; either version 2 of the License, or (at your
> @@ -41,6 +44,9 @@
>
>   #include "gspca.h"
>
> +#include<linux/input.h>
> +#include<linux/usb/input.h>
> +
>   /* global values */
>   #define DEF_NURBS 3		/* default number of URBs */
>   #if DEF_NURBS>  MAX_NURBS
> @@ -112,6 +118,167 @@
>   	.close		= gspca_vm_close,
>   };
>
> +/*
> + * Input and interrupt endpoint handling functions
> + */
> +#ifdef CONFIG_INPUT
> +#if LINUX_VERSION_CODE<  KERNEL_VERSION(2, 6, 19)
> +static void int_irq(struct urb *urb, struct pt_regs *regs)
> +#else
> +static void int_irq(struct urb *urb)
> +#endif
> +{
> +	struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
> +	int ret;
> +
> +	if (urb->status == 0) {
> +		if (gspca_dev->sd_desc->int_pkt_scan(gspca_dev,
> +		    urb->transfer_buffer, urb->actual_length)<  0) {
> +			PDEBUG(D_ERR, "Unknown packet received");
> +		}
> +
> +		ret = usb_submit_urb(urb, GFP_ATOMIC);
> +		if (ret<  0)
> +			PDEBUG(D_ERR, "Resubmit URB failed with error %i", ret);
> +	}
> +}
> +

If the status is not 0 you should print an error message, and
reset the status and still resubmit the urb, if you don't resubmit
on error, after one single usb glitch, the button will stop working.

> +static int gspca_input_connect(struct gspca_dev *dev)
> +{
> +	struct input_dev *input_dev;
> +	int err = 0;
> +
> +	dev->input_dev = NULL;
> +	if (dev->sd_desc->int_pkt_scan)  {
> +		input_dev = input_allocate_device();
> +		if (!input_dev)
> +			return -ENOMEM;
> +
> +		usb_make_path(dev->dev, dev->phys, sizeof(dev->phys));
> +		strlcat(dev->phys, "/input0", sizeof(dev->phys));
> +
> +		input_dev->name = dev->sd_desc->name;
> +		input_dev->phys = dev->phys;
> +
> +		usb_to_input_id(dev->dev,&input_dev->id);
> +
> +		input_dev->evbit[0] = BIT_MASK(EV_KEY);
> +		input_dev->keybit[BIT_WORD(KEY_CAMERA)] = BIT_MASK(KEY_CAMERA);
> +		input_dev->dev.parent =&dev->dev->dev;
> +
> +		err = input_register_device(input_dev);
> +		if (err) {
> +			PDEBUG(D_ERR, "Input device registration failed "
> +				"with error %i", err);
> +			input_dev->dev.parent = NULL;
> +			input_free_device(input_dev);
> +		} else {
> +			dev->input_dev = input_dev;
> +		}
> +	} else
> +		err = -EINVAL;
> +
> +	return err;
> +}
> +
> +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;
> +
> +	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;
> +	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;
> +}
> +
> +static int gspca_input_create_urb(struct gspca_dev *gspca_dev)
> +{
> +	int ret = -EINVAL;
> +	struct usb_interface *intf;
> +	struct usb_host_interface *intf_desc;
> +	struct usb_endpoint_descriptor *ep;
> +	int i;
> +
> +	if (gspca_dev->sd_desc->int_pkt_scan)  {
> +		intf = usb_ifnum_to_if(gspca_dev->dev, gspca_dev->iface);
> +		intf_desc = intf->cur_altsetting;
> +		for (i = 0; i<  intf_desc->desc.bNumEndpoints; i++) {
> +			ep =&intf_desc->endpoint[i].desc;
> +			if (usb_endpoint_dir_in(ep)&&
> +			    usb_endpoint_xfer_int(ep)) {
> +
> +				ret = alloc_and_submit_int_urb(gspca_dev, ep);
> +				break;
> +			}
> +		}
> +	}
> +	return ret;
> +}
> +
> +static void gspca_input_destroy_urb(struct gspca_dev *gspca_dev)
> +{
> +	struct urb *urb;
> +
> +	urb = gspca_dev->int_urb;
> +	if (urb) {
> +		gspca_dev->int_urb = NULL;
> +		usb_kill_urb(urb);
> +		usb_buffer_free(gspca_dev->dev,
> +				urb->transfer_buffer_length,
> +				urb->transfer_buffer,
> +				urb->transfer_dma);
> +		usb_free_urb(urb);
> +	}
> +}
> +#else
> +#define gspca_input_connect(gspca_dev)		0
> +#define gspca_input_create_urb(gspca_dev)	0
> +#define gspca_input_destroy_urb(gspca_dev)
> +#endif
> +
>   /* get the current input frame buffer */
>   struct gspca_frame *gspca_get_i_frame(struct gspca_dev *gspca_dev)
>   {
> @@ -499,11 +666,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;
>   }
> @@ -714,7 +883,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);
>   	}
>
>   	/* always call stop0 to free the subdriver's resources */
> @@ -2137,6 +2308,11 @@
>
>   	usb_set_intfdata(intf, gspca_dev);
>   	PDEBUG(D_PROBE, "%s created", video_device_node_name(&gspca_dev->vdev));
> +
> +	ret = gspca_input_connect(gspca_dev);
> +	if (0<= ret)
> +		ret = gspca_input_create_urb(gspca_dev);
> +

I don't like this reverse psychology if. Why not just write:
if (ret == 0) ?



>   	return 0;
>   out:
>   	kfree(gspca_dev->usb_buf);



Otherwise it looks good.

Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

Hans de Goede wrote:
> Hi,
> 
> Thanks for your continued work on this. I'm afraid I found
> one thing which needs fixing (can be fixed with
> a separate patch after merging, but that is up to
> Jean-Francois).
> 
> See my comments inline.

Thanks for the feedback, I hope the quality of the software increases
during this review phase.

> On 01/17/2010 02:08 PM, Németh Márton wrote:
[snip]
>> +#ifdef CONFIG_INPUT
>> +#if LINUX_VERSION_CODE<  KERNEL_VERSION(2, 6, 19)
>> +static void int_irq(struct urb *urb, struct pt_regs *regs)
>> +#else
>> +static void int_irq(struct urb *urb)
>> +#endif
>> +{
>> +	struct gspca_dev *gspca_dev = (struct gspca_dev *) urb->context;
>> +	int ret;
>> +
>> +	if (urb->status == 0) {
>> +		if (gspca_dev->sd_desc->int_pkt_scan(gspca_dev,
>> +		    urb->transfer_buffer, urb->actual_length)<  0) {
>> +			PDEBUG(D_ERR, "Unknown packet received");
>> +		}
>> +
>> +		ret = usb_submit_urb(urb, GFP_ATOMIC);
>> +		if (ret<  0)
>> +			PDEBUG(D_ERR, "Resubmit URB failed with error %i", ret);
>> +	}
>> +}
>> +
> 
> If the status is not 0 you should print an error message, and
> reset the status and still resubmit the urb, if you don't resubmit
> on error, after one single usb glitch, the button will stop working.

I rewrote this function, see the following patch version.

[snip]
>> @@ -2137,6 +2308,11 @@
>>
>>   	usb_set_intfdata(intf, gspca_dev);
>>   	PDEBUG(D_PROBE, "%s created", video_device_node_name(&gspca_dev->vdev));
>> +
>> +	ret = gspca_input_connect(gspca_dev);
>> +	if (0<= ret)
>> +		ret = gspca_input_create_urb(gspca_dev);
>> +
> 
> I don't like this reverse psychology if. Why not just write:
> if (ret == 0) ?

I changed this line to "if (ret == 0)".

When I use a not-equal relation I try to put the smaller value on the left
hand side and the bigger one to the right hand side. In this case a range
which is expressed in mathematical notation like 10 < a <= 16 can be written
in C like (10 < a) && (a <= 16).

But it is a different question what people like and what not, so I've changed
that line.

> Otherwise it looks good.
Thanks.

I'm looking forward to receive more comments on the new version of this patch.

Regars,

	Márton Németh
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html