[16/21,staging] tm6000: Select interface on first open.

  Instead of selecting the default interface setting when preparing
isochronous transfers, select it on the first call to open() to make
sure it is available earlier.
---
 drivers/staging/tm6000/tm6000-video.c |   17 ++++++++++++-----
 1 files changed, 12 insertions(+), 5 deletions(-)
  

Em 04-08-2011 04:14, Thierry Reding escreveu:
> Instead of selecting the default interface setting when preparing
> isochronous transfers, select it on the first call to open() to make
> sure it is available earlier.

Hmm... I fail to see what this is needed earlier. The ISOC endpont is used
only when the device is streaming.

Did you get any bug related to it? If so, please describe it better.

> ---
>  drivers/staging/tm6000/tm6000-video.c |   17 ++++++++++++-----
>  1 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/staging/tm6000/tm6000-video.c b/drivers/staging/tm6000/tm6000-video.c
> index 70fc19e..b59a0da 100644
> --- a/drivers/staging/tm6000/tm6000-video.c
> +++ b/drivers/staging/tm6000/tm6000-video.c
> @@ -595,11 +595,6 @@ static int tm6000_prepare_isoc(struct tm6000_core *dev)
>  	tm6000_uninit_isoc(dev);
>  	/* Stop interrupt USB pipe */
>  	tm6000_ir_int_stop(dev);
> -
> -	usb_set_interface(dev->udev,
> -			  dev->isoc_in.bInterfaceNumber,
> -			  dev->isoc_in.bAlternateSetting);
> -
>  	/* Start interrupt USB pipe */
>  	tm6000_ir_int_start(dev);
>  
> @@ -1484,6 +1479,18 @@ static int tm6000_open(struct file *file)
>  		break;
>  	}
>  
> +	if (dev->users == 0) {
> +		int err = usb_set_interface(dev->udev,
> +				dev->isoc_in.bInterfaceNumber,
> +				dev->isoc_in.bAlternateSetting);
> +		if (err < 0) {
> +			dev_err(&vdev->dev, "failed to select interface %d, "
> +					"alt. setting %d\n",
> +					dev->isoc_in.bInterfaceNumber,
> +					dev->isoc_in.bAlternateSetting);
> +		}
> +	}
> +
>  	/* If more than one user, mutex should be added */
>  	dev->users++;
>  

--
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
  

* Mauro Carvalho Chehab wrote:
> Em 04-08-2011 04:14, Thierry Reding escreveu:
> > Instead of selecting the default interface setting when preparing
> > isochronous transfers, select it on the first call to open() to make
> > sure it is available earlier.
> 
> Hmm... I fail to see what this is needed earlier. The ISOC endpont is used
> only when the device is streaming.
> 
> Did you get any bug related to it? If so, please describe it better.

I'm not sure whether this really fixes a bug, but it seems a little wrong to
me to selecting the interface so late in the process when in fact the device
is already being configured before (video standard, audio mode, firmware
upload, ...).

Thinking about it, this may actually be part of the fix for the "device hangs
sometimes for inexplicable reasons" bug that this whole patch series seems to
fix.

Thierry
  

Em 01-09-2011 02:19, Thierry Reding escreveu:
> * Mauro Carvalho Chehab wrote:
>> Em 04-08-2011 04:14, Thierry Reding escreveu:
>>> Instead of selecting the default interface setting when preparing
>>> isochronous transfers, select it on the first call to open() to make
>>> sure it is available earlier.
>>
>> Hmm... I fail to see what this is needed earlier. The ISOC endpont is used
>> only when the device is streaming.
>>
>> Did you get any bug related to it? If so, please describe it better.
> 
> I'm not sure whether this really fixes a bug, but it seems a little wrong to
> me to selecting the interface so late in the process when in fact the device
> is already being configured before (video standard, audio mode, firmware
> upload, ...).

Some applications may open the device just to change the controls. All other drivers
only set alternates/interfaces when the streaming is requested, as alternates/interfaces
are needed only there.

> Thinking about it, this may actually be part of the fix for the "device hangs
> sometimes for inexplicable reasons" bug that this whole patch series seems to
> fix.

It is unlikely, except if the firmware inside the chip is broken (unfortunately, 
we have serious reasons to believe that the internal firmware on this chipset has
serious bugs).

I prefer to not apply this patch, except if we have a good reason for that,
as otherwise this driver will behave different than the others.

Regards,
Mauro.

> 
> Thierry

--
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
  

* Mauro Carvalho Chehab wrote:
> Em 01-09-2011 02:19, Thierry Reding escreveu:
> > * Mauro Carvalho Chehab wrote:
> >> Em 04-08-2011 04:14, Thierry Reding escreveu:
> >>> Instead of selecting the default interface setting when preparing
> >>> isochronous transfers, select it on the first call to open() to make
> >>> sure it is available earlier.
> >>
> >> Hmm... I fail to see what this is needed earlier. The ISOC endpont is used
> >> only when the device is streaming.
> >>
> >> Did you get any bug related to it? If so, please describe it better.
> > 
> > I'm not sure whether this really fixes a bug, but it seems a little wrong to
> > me to selecting the interface so late in the process when in fact the device
> > is already being configured before (video standard, audio mode, firmware
> > upload, ...).
> 
> Some applications may open the device just to change the controls. All other drivers
> only set alternates/interfaces when the streaming is requested, as alternates/interfaces
> are needed only there.

Okay, I didn't know that it was only necessary for streaming.

> > Thinking about it, this may actually be part of the fix for the "device hangs
> > sometimes for inexplicable reasons" bug that this whole patch series seems to
> > fix.
> 
> It is unlikely, except if the firmware inside the chip is broken (unfortunately, 
> we have serious reasons to believe that the internal firmware on this chipset has
> serious bugs).

Indeed! =)

> I prefer to not apply this patch, except if we have a good reason for that,
> as otherwise this driver will behave different than the others.

Okay, it's your call. Unfortunately I no longer have the hardware available
to test if this is really related to the bug. I'll have to check again when I
have the hardware.

Thierry