Re: [PATCH] em28xx: fix usb alternate setting for analog and digital video endpoints > 0

["Frank Schäfer" <fschaefer.oss@googlemail.com>]

Am 05.02.2013 21:57, schrieb Mauro Carvalho Chehab:
> Em Fri, 18 Jan 2013 18:25:48 +0100
> Frank Schäfer <fschaefer.oss@googlemail.com> escreveu:
>
>> While the current code handles sound interfaces with a number > 0 correctly, it
>> assumes that the interface number for analog + digital video is always 0 when
>> changing the alternate setting.
>>
>> (NOTE: the "SpeedLink VAD Laplace webcam" (EM2765) uses interface number 3 for video)
>>
>> Signed-off-by: Frank Schäfer <fschaefer.oss@googlemail.com>
>> ---
>>  drivers/media/usb/em28xx/em28xx-audio.c |   10 +++++-----
>>  drivers/media/usb/em28xx/em28xx-cards.c |    2 +-
>>  drivers/media/usb/em28xx/em28xx-core.c  |    2 +-
>>  drivers/media/usb/em28xx/em28xx-dvb.c   |    2 +-
>>  drivers/media/usb/em28xx/em28xx.h       |    3 +--
>>  5 Dateien geändert, 9 Zeilen hinzugefügt(+), 10 Zeilen entfernt(-)
>>
>> diff --git a/drivers/media/usb/em28xx/em28xx-audio.c b/drivers/media/usb/em28xx/em28xx-audio.c
>> index 2fdb66e..cdbfe0a 100644
>> --- a/drivers/media/usb/em28xx/em28xx-audio.c
>> +++ b/drivers/media/usb/em28xx/em28xx-audio.c
>> @@ -283,15 +283,15 @@ static int snd_em28xx_capture_open(struct snd_pcm_substream *substream)
>>  	}
>>  
>>  	runtime->hw = snd_em28xx_hw_capture;
>> -	if ((dev->alt == 0 || dev->audio_ifnum) && dev->adev.users == 0) {
>> -		if (dev->audio_ifnum)
>> +	if ((dev->alt == 0 || dev->ifnum) && dev->adev.users == 0) {
>> +		if (dev->ifnum)
> Please don't merge a non-fix change (variable rename) with a fix.

Ok, sorry, it seems to be trivial...

> Btw, audio_ifnum is a better name, as it avoids it to be miss-interpreted.

Did you read the complete patch ? ;)
Or do you really want the video interface number to be called audio_ifnum ?

>>  			dev->alt = 1;
>>  		else
>>  			dev->alt = 7;
>>  
>>  		dprintk("changing alternate number on interface %d to %d\n",
>> -			dev->audio_ifnum, dev->alt);
>> -		usb_set_interface(dev->udev, dev->audio_ifnum, dev->alt);
>> +			dev->ifnum, dev->alt);
>> +		usb_set_interface(dev->udev, dev->ifnum, dev->alt);
>>  
>>  		/* Sets volume, mute, etc */
>>  		dev->mute = 0;
>> @@ -642,7 +642,7 @@ static int em28xx_audio_init(struct em28xx *dev)
>>  	static int          devnr;
>>  	int                 err;
>>  
>> -	if (!dev->has_alsa_audio || dev->audio_ifnum < 0) {
>> +	if (!dev->has_alsa_audio) {
>>  		/* This device does not support the extension (in this case
>>  		   the device is expecting the snd-usb-audio module or
>>  		   doesn't have analog audio support at all) */
>> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
>> index 0a5aa62..553db17 100644
>> --- a/drivers/media/usb/em28xx/em28xx-cards.c
>> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
>> @@ -3376,7 +3376,7 @@ static int em28xx_usb_probe(struct usb_interface *interface,
>>  	dev->alt   = -1;
>>  	dev->is_audio_only = has_audio && !(has_video || has_dvb);
>>  	dev->has_alsa_audio = has_audio;
>> -	dev->audio_ifnum = ifnum;
>> +	dev->ifnum = ifnum;
>>  
>>  	/* Checks if audio is provided by some interface */
>>  	for (i = 0; i < udev->config->desc.bNumInterfaces; i++) {
>> diff --git a/drivers/media/usb/em28xx/em28xx-core.c b/drivers/media/usb/em28xx/em28xx-core.c
>> index ce4f252..210859a 100644
>> --- a/drivers/media/usb/em28xx/em28xx-core.c
>> +++ b/drivers/media/usb/em28xx/em28xx-core.c
>> @@ -862,7 +862,7 @@ set_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);
>> +	errCode = usb_set_interface(dev->udev, dev->ifnum, dev->alt);
>>  	if (errCode < 0) {
>>  		em28xx_errdev("cannot change alternate number to %d (error=%i)\n",
>>  				dev->alt, errCode);
> This hunk doesn't apply upstream:
>
> patching file drivers/media/usb/em28xx/em28xx-core.c
> Hunk #1 FAILED at 862.
> 1 out of 1 hunk FAILED -- rejects in file drivers/media/usb/em28xx/em28xx-core.c

It applies after

http://patchwork.linuxtv.org/patch/16197/

has been applied.

Regards,
Frank

>
>> diff --git a/drivers/media/usb/em28xx/em28xx-dvb.c b/drivers/media/usb/em28xx/em28xx-dvb.c
>> index a81ec2e..dbeed6c 100644
>> --- a/drivers/media/usb/em28xx/em28xx-dvb.c
>> +++ b/drivers/media/usb/em28xx/em28xx-dvb.c
>> @@ -196,7 +196,7 @@ static int em28xx_start_streaming(struct em28xx_dvb *dvb)
>>  		dvb_alt = dev->dvb_alt_isoc;
>>  	}
>>  
>> -	usb_set_interface(dev->udev, 0, dvb_alt);
>> +	usb_set_interface(dev->udev, dev->ifnum, dvb_alt);
>>  	rc = em28xx_set_mode(dev, EM28XX_DIGITAL_MODE);
>>  	if (rc < 0)
>>  		return rc;
>> diff --git a/drivers/media/usb/em28xx/em28xx.h b/drivers/media/usb/em28xx/em28xx.h
>> index 5f0b2c5..0dc5b73 100644
>> --- a/drivers/media/usb/em28xx/em28xx.h
>> +++ b/drivers/media/usb/em28xx/em28xx.h
>> @@ -487,8 +487,6 @@ struct em28xx {
>>  
>>  	unsigned char disconnected:1;	/* device has been diconnected */
>>  
>> -	int audio_ifnum;
>> -
>>  	struct v4l2_device v4l2_dev;
>>  	struct v4l2_ctrl_handler ctrl_handler;
>>  	/* provides ac97 mute and volume overrides */
>> @@ -597,6 +595,7 @@ struct em28xx {
>>  
>>  	/* usb transfer */
>>  	struct usb_device *udev;	/* the usb device */
>> +	int ifnum;			/* usb interface number */
>>  	u8 analog_ep_isoc;	/* address of isoc endpoint for analog */
>>  	u8 analog_ep_bulk;	/* address of bulk endpoint for analog */
>>  	u8 dvb_ep_isoc;		/* address of isoc endpoint for DVB */
>