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 majordomo@xxxxxxxxxxxxxxx > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
