On 28-12-2011 01:50, Holger Nelson wrote:
> On Mon, 26 Dec 2011, Mauro Carvalho Chehab wrote:
>
>> I'm currently without time right now to work on a patch, but I think that
>> several hacks
>> inside the em28xx probe should be removed, including the one that detects
>> the endpoint
>> based on the packet size.
>>
>> As it is easier to code than to explain in words, the code below could be
>> a start (ok, it doesn't compile, doesn't remove all hacks, doesn't free
>> memory, etc...)
>> Feel free to use it as a start for a real patch, if you wish.
>
> I think, I filled the missing parts and removed most of the hacks in the
> probe code. The code works with my Cinergy HTC USB XS.
The code looks sane on my eyes. Didn't test it through.
On the final version, please include a proper description for the patch and your
Signed-off-by:, according with:
http://linuxtv.org/wiki/index.php/Development:_How_to_submit_patches
>
> Holger
>
> diff --git a/drivers/media/video/em28xx/em28xx-audio.c
> b/drivers/media/video/em28xx/em28xx-audio.c
> index cff0768..e2a7b77 100644
> --- a/drivers/media/video/em28xx/em28xx-audio.c
> +++ b/drivers/media/video/em28xx/em28xx-audio.c
> @@ -193,7 +193,7 @@ static int em28xx_init_audio_isoc(struct em28xx *dev)
>
> urb->dev = dev->udev;
> urb->context = dev;
> - urb->pipe = usb_rcvisocpipe(dev->udev, 0x83);
> + urb->pipe = usb_rcvisocpipe(dev->udev, EM28XX_EP_AUDIO);
> urb->transfer_flags = URB_ISO_ASAP;
> urb->transfer_buffer = dev->adev.transfer_buffer[i];
> urb->interval = 1;
> diff --git a/drivers/media/video/em28xx/em28xx-cards.c
> b/drivers/media/video/em28xx/em28xx-cards.c
> index a7cfded..8082914 100644
> --- a/drivers/media/video/em28xx/em28xx-cards.c
> +++ b/drivers/media/video/em28xx/em28xx-cards.c
> @@ -3087,12 +3087,11 @@ unregister_dev:
> static int em28xx_usb_probe(struct usb_interface *interface,
> const struct usb_device_id *id)
> {
> - const struct usb_endpoint_descriptor *endpoint;
> struct usb_device *udev;
> struct em28xx *dev = NULL;
> int retval;
> - bool is_audio_only = false, has_audio = false;
> - int i, nr, isoc_pipe;
> + bool has_audio = false, has_video = false, has_dvb = false;
> + int i, nr, sizedescr, size;
> const int ifnum = interface->altsetting[0].desc.bInterfaceNumber;
> char *speed;
> char descr[255] = "";
> @@ -3124,56 +3123,69 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
> goto err;
> }
>
> - /* Get endpoints */
> - for (i = 0; i < interface->num_altsetting; i++) {
> - int ep;
> + /* allocate memory for our device state and initialize it */
> + dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> + if (dev == NULL) {
> + em28xx_err(DRIVER_NAME ": out of memory!\n");
> + retval = -ENOMEM;
> + goto err;
> + }
>
> - for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++)
> {
> - struct usb_host_endpoint *e;
> - e = &interface->altsetting[i].endpoint[ep];
> + /* compute alternate max packet sizes */
> + dev->alt_video_max_pkt_size =
> kmalloc(sizeof(dev->alt_video_max_pkt_size[0]) * interface->num_altsetting,
> GFP_KERNEL);
> + if (dev->alt_video_max_pkt_size == NULL) {
> + em28xx_errdev("out of memory!\n");
> + kfree(dev);
> + retval = -ENOMEM;
> + goto err;
> + }
>
> - if (e->desc.bEndpointAddress == 0x83)
> - has_audio = true;
> - }
> + dev->alt_dvb_max_pkt_size = kmalloc(sizeof(dev->alt_dvb_max_pkt_size[0])
> * interface->num_altsetting, GFP_KERNEL);
> + if (dev->alt_dvb_max_pkt_size == NULL) {
> + em28xx_errdev("out of memory!\n");
> + kfree(dev->alt_video_max_pkt_size);
> + kfree(dev);
> + retval = -ENOMEM;
> + goto err;
> }
>
> - endpoint = &interface->cur_altsetting->endpoint[0].desc;
> + /* Get endpoints */
> + for (i = 0; i < interface->num_altsetting; i++) {
> + int ep;
>
> - /* check if the device has the iso in endpoint at the correct place */
> - if (usb_endpoint_xfer_isoc(endpoint)
> - &&
> - (interface->altsetting[1].endpoint[0].desc.wMaxPacketSize == 940)) {
> - /* It's a newer em2874/em2875 device */
> - isoc_pipe = 0;
> - } else {
> - int check_interface = 1;
> - isoc_pipe = 1;
> - endpoint = &interface->cur_altsetting->endpoint[1].desc;
> - if (!usb_endpoint_xfer_isoc(endpoint))
> - check_interface = 0;
> -
> - if (usb_endpoint_dir_out(endpoint))
> - check_interface = 0;
> -
> - if (!check_interface) {
> - if (has_audio) {
> - is_audio_only = true;
> - } else {
> - em28xx_err(DRIVER_NAME " video device (%04x:%04x): "
> - "interface %i, class %i found.\n",
> - le16_to_cpu(udev->descriptor.idVendor),
> - le16_to_cpu(udev->descriptor.idProduct),
> - ifnum,
> - interface->altsetting[0].desc.bInterfaceClass);
> - em28xx_err(DRIVER_NAME " This is an anciliary "
> - "interface not used by the driver\n");
> -
> - retval = -ENODEV;
> - goto err;
> + for (ep = 0; ep < interface->altsetting[i].desc.bNumEndpoints; ep++)
> {
> + const struct usb_endpoint_descriptor *e;
> + + e = &interface->altsetting[i].endpoint[ep].desc;
There is an extra "+" here.
> +
> + sizedescr = le16_to_cpu(e->wMaxPacketSize);
> + size = sizedescr & 0x7fff;
> + if (udev->speed == USB_SPEED_HIGH)
> + size = size * hb_mult(sizedescr);
> +
> + if (usb_endpoint_xfer_isoc(e) && usb_endpoint_dir_in(e)) {
> + switch (e->bEndpointAddress) {
> + case EM28XX_EP_AUDIO:
> + has_audio = true;
> + break;
> + case EM28XX_EP_ANALOG:
> + has_video = true;
> + dev->alt_video_max_pkt_size[i] = size;
> + break;
> + case EM28XX_EP_DIGITAL:
> + has_dvb = true;
> + dev->alt_dvb_max_pkt_size[i] = size;
> + break;
> + }
> }
> }
> }
> -
> + + if (!(has_audio||has_video||has_dvb)) {
There is an extra "+" here. Also Linux CodingStyle requires spaces before
and after operators:
if (!(has_audio || has_video || has_dvb)) {
> + retval=-ENODEV;
> + goto err_free_all;
> + }
> +
> switch (udev->speed) {
> case USB_SPEED_LOW:
> speed = "1.5";
> @@ -3197,6 +3209,7 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
> strlcat(descr, " ", sizeof(descr));
> strlcat(descr, udev->product, sizeof(descr));
> }
> +
> if (*descr)
> strlcat(descr, " ", sizeof(descr));
>
> @@ -3213,6 +3226,14 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
> printk(KERN_INFO DRIVER_NAME
> ": Audio Vendor Class interface %i found\n",
> ifnum);
> + if (has_video)
> + printk(KERN_INFO DRIVER_NAME
> + ": Video interface %i found\n",
> + ifnum);
> + if (has_dvb)
> + printk(KERN_INFO DRIVER_NAME
> + ": DVB interface %i found\n",
> + ifnum);
>
> /*
> * Make sure we have 480 Mbps of bandwidth, otherwise things like
> @@ -3224,22 +3245,14 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
> printk(DRIVER_NAME ": Device must be connected to a high-speed"
> " USB 2.0 port.\n");
> retval = -ENODEV;
> - goto err;
> - }
> -
> - /* allocate memory for our device state and initialize it */
> - dev = kzalloc(sizeof(*dev), GFP_KERNEL);
> - if (dev == NULL) {
> - em28xx_err(DRIVER_NAME ": out of memory!\n");
> - retval = -ENOMEM;
> - goto err;
> + goto err_free_all;
> }
>
> snprintf(dev->name, sizeof(dev->name), "em28xx #%d", nr);
> dev->devno = nr;
> dev->model = id->driver_info;
> dev->alt = -1;
> - dev->is_audio_only = is_audio_only;
> + dev->is_audio_only = has_audio&&!(has_video||has_dvb);
CodingStyle: it should be, instead:
dev->is_audio_only = has_audio && !(has_video || has_dvb);
> dev->has_alsa_audio = has_audio;
> dev->audio_ifnum = ifnum;
>
> @@ -3252,26 +3265,7 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
> }
> }
>
> - /* compute alternate max packet sizes */
> dev->num_alt = interface->num_altsetting;
> - dev->alt_max_pkt_size = kmalloc(32 * dev->num_alt, GFP_KERNEL);
> -
> - if (dev->alt_max_pkt_size == NULL) {
> - em28xx_errdev("out of memory!\n");
> - kfree(dev);
> - retval = -ENOMEM;
> - goto err;
> - }
> -
> - for (i = 0; i < dev->num_alt ; i++) {
> - u16 tmp =
> le16_to_cpu(interface->altsetting[i].endpoint[isoc_pipe].desc.wMaxPacketSize);
> - unsigned int size = tmp & 0x7ff;
> -
> - if (udev->speed == USB_SPEED_HIGH)
> - size = size * hb_mult(tmp);
> -
> - dev->alt_max_pkt_size[i] = size;
> - }
>
> if ((card[nr] >= 0) && (card[nr] < em28xx_bcount))
> dev->model = card[nr];
> @@ -3285,9 +3279,7 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
> retval = em28xx_init_dev(&dev, udev, interface, nr);
> if (retval) {
> mutex_unlock(&dev->lock);
> - kfree(dev->alt_max_pkt_size);
> - kfree(dev);
> - goto err;
> + goto err_free_all;
Maybe you could also move the mutex_unlock to the error path, like:
goto unlock_and_free;
> }
>
> request_modules(dev);
> @@ -3306,6 +3298,11 @@ static int em28xx_usb_probe(struct usb_interface
> *interface,
>
> return 0;
>
> +err_free_all:
> + kfree(dev->alt_dvb_max_pkt_size);
> + kfree(dev->alt_video_max_pkt_size);
> + kfree(dev);
> +
> err:
> clear_bit(nr, &em28xx_devused);
>
> @@ -3369,7 +3366,8 @@ static void em28xx_usb_disconnect(struct usb_interface
> *interface)
> em28xx_close_extension(dev);
>
> if (!dev->users) {
> - kfree(dev->alt_max_pkt_size);
> + kfree(dev->alt_dvb_max_pkt_size);
> + kfree(dev->alt_video_max_pkt_size);
> kfree(dev);
> }
> }
> diff --git a/drivers/media/video/em28xx/em28xx-core.c
> b/drivers/media/video/em28xx/em28xx-core.c
> index 804a4ab..74608f9 100644
> --- a/drivers/media/video/em28xx/em28xx-core.c
> +++ b/drivers/media/video/em28xx/em28xx-core.c
> @@ -829,14 +829,14 @@ int em28xx_set_alternate(struct em28xx *dev)
>
> for (i = 0; i < dev->num_alt; i++) {
> /* stop when the selected alt setting offers enough bandwidth */
> - if (dev->alt_max_pkt_size[i] >= min_pkt_size) {
> + if (dev->alt_video_max_pkt_size[i] >= min_pkt_size) {
> dev->alt = i;
> break;
> /* otherwise make sure that we end up with the maximum bandwidth
> because the min_pkt_size equation might be wrong...
> */
> - } else if (dev->alt_max_pkt_size[i] >
> - dev->alt_max_pkt_size[dev->alt])
> + } else if (dev->alt_video_max_pkt_size[i] >
> + dev->alt_video_max_pkt_size[dev->alt])
> dev->alt = i;
> }
>
> @@ -844,7 +844,7 @@ set_alt:
> if (dev->alt != prev_alt) {
> em28xx_coredbg("minimum isoc packet size: %u (alt=%d)\n",
> min_pkt_size, dev->alt);
> - dev->max_pkt_size = dev->alt_max_pkt_size[dev->alt];
> + dev->max_pkt_size = dev->alt_video_max_pkt_size[dev->alt];
> em28xx_coredbg("setting alternate %d with wMaxPacketSize=%u\n",
> dev->alt, dev->max_pkt_size);
> errCode = usb_set_interface(dev->udev, 0, dev->alt);
> @@ -1070,7 +1070,7 @@ int em28xx_init_isoc(struct em28xx *dev, int
> max_packets,
> should also be using 'desc.bInterval'
> */
> pipe = usb_rcvisocpipe(dev->udev,
> - dev->mode == EM28XX_ANALOG_MODE ? 0x82 : 0x84);
> + dev->mode == EM28XX_ANALOG_MODE ? EM28XX_EP_ANALOG :
> EM28XX_EP_DIGITAL);
>
> usb_fill_int_urb(urb, dev->udev, pipe,
> dev->isoc_ctl.transfer_buffer[i], sb_size,
> @@ -1144,20 +1144,10 @@ int em28xx_isoc_dvb_max_packetsize(struct em28xx *dev)
> }
> break;
> case CHIP_ID_EM2874:
> - /*
> - * FIXME: for now assumes 564 like it was before, but the
> - * em2874 code should be added to return the proper value
> - */
> - packet_size = 564;
> - break;
> case CHIP_ID_EM2884:
> case CHIP_ID_EM28174:
> default:
> - /*
> - * FIXME: same as em2874. 564 was enough for 22 Mbit DVB-T
> - * but not enough for 44 Mbit DVB-C.
> - */
> - packet_size = 752;
> + packet_size = dev->alt_dvb_max_pkt_size[1];;
There are two ";;" above.
It could make sense to do a loop here and get the biggest max_pkt_size.
Of course, this would mean that, at em28xx_start_streaming, the code
should be changed from:
usb_set_interface(dev->udev, 0, 1);
to:
usb_set_interface(dev->udev, 0, dev->dvb_alternate);
and that the loop would fill dvb_alternate with the alternate associated
with the selected packet size.
Now that the packet_size is coming from the endpoint, I think that the
chipset-specific
code there could be removed completely.
> }
>
> return packet_size;
> diff --git a/drivers/media/video/em28xx/em28xx-reg.h
> b/drivers/media/video/em28xx/em28xx-reg.h
> index 66f7923..2f62685 100644
> --- a/drivers/media/video/em28xx/em28xx-reg.h
> +++ b/drivers/media/video/em28xx/em28xx-reg.h
> @@ -12,6 +12,11 @@
> #define EM_GPO_2 (1 << 2)
> #define EM_GPO_3 (1 << 3)
>
> +/* em28xx endpoints */
> +#define EM28XX_EP_ANALOG 0x82
> +#define EM28XX_EP_AUDIO 0x83
> +#define EM28XX_EP_DIGITAL 0x84
> +
> /* em2800 registers */
> #define EM2800_R08_AUDIOSRC 0x08
>
> diff --git a/drivers/media/video/em28xx/em28xx-video.c
> b/drivers/media/video/em28xx/em28xx-video.c
> index 9b4557a..af9f0b7 100644
> --- a/drivers/media/video/em28xx/em28xx-video.c
> +++ b/drivers/media/video/em28xx/em28xx-video.c
> @@ -2254,7 +2254,8 @@ static int em28xx_v4l2_close(struct file *filp)
> free the remaining resources */
> if (dev->state & DEV_DISCONNECTED) {
> em28xx_release_resources(dev);
> - kfree(dev->alt_max_pkt_size);
> + kfree(dev->alt_dvb_max_pkt_size);
> + kfree(dev->alt_video_max_pkt_size);
> kfree(dev);
> return 0;
> }
> diff --git a/drivers/media/video/em28xx/em28xx.h
> b/drivers/media/video/em28xx/em28xx.h
> index b1199ef..f73f028 100644
> --- a/drivers/media/video/em28xx/em28xx.h
> +++ b/drivers/media/video/em28xx/em28xx.h
> @@ -596,7 +596,8 @@ struct em28xx {
> int alt; /* alternate */
> int max_pkt_size; /* max packet size of isoc transaction */
> int num_alt; /* Number of alternative settings */
> - unsigned int *alt_max_pkt_size; /* array of wMaxPacketSize */
> + unsigned int *alt_video_max_pkt_size; /* array of wMaxPacketSize */
> + unsigned int *alt_dvb_max_pkt_size; /* array of wMaxPacketSize */
> struct urb *urb[EM28XX_NUM_BUFS]; /* urb for isoc transfers */
> char *transfer_buffer[EM28XX_NUM_BUFS]; /* transfer buffers for isoc
> transfer */
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
