Re: [PATCH] em28xx: balance subdevice power-off calls

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

Hi Guennadi,

thank you for looking at this. A few thoughts:

Am 05.09.2013 15:11, schrieb Guennadi Liakhovetski:
> The em28xx USB driver powers off its subdevices, by calling their .s_power()
> methods to save power, but actually never powers them on. Apparently this
> works with currently used subdevice drivers, but is wrong and might break
> with some other ones. This patch fixes this issue by adding required
> .s_power() calls to turn subdevices on.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> Please, test - only compile tested due to lack of hardware
>
>  drivers/media/usb/em28xx/em28xx-cards.c |    1 +
>  drivers/media/usb/em28xx/em28xx-video.c |   17 ++++++++++-------
>  2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/media/usb/em28xx/em28xx-cards.c b/drivers/media/usb/em28xx/em28xx-cards.c
> index dc65742..d2d3b06 100644
> --- a/drivers/media/usb/em28xx/em28xx-cards.c
> +++ b/drivers/media/usb/em28xx/em28xx-cards.c
> @@ -3066,6 +3066,7 @@ static int em28xx_init_dev(struct em28xx *dev, struct usb_device *udev,
>  	}
>  
>  	/* wake i2c devices */
> +	v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 1);
>  	em28xx_wake_i2c(dev);
I wonder if we should make the (s_power, 1) call part of em28xx_wake_i2c().
This function already does

    v4l2_device_call_all(&dev->v4l2_dev, 0, core,  reset, 0);
    v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_routing,
            INPUT(dev->ctl_input)->vmux, 0, 0);
    v4l2_device_call_all(&dev->v4l2_dev, 0, video, s_stream, 0);

>  
>  	/* init video dma queues */
> diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
> index 9d10334..283fa26 100644
> --- a/drivers/media/usb/em28xx/em28xx-video.c
> +++ b/drivers/media/usb/em28xx/em28xx-video.c
> @@ -1589,15 +1589,18 @@ static int em28xx_v4l2_open(struct file *filp)
>  	fh->type = fh_type;
>  	filp->private_data = fh;
>  
> -	if (fh->type == V4L2_BUF_TYPE_VIDEO_CAPTURE && dev->users == 0) {
> -		em28xx_set_mode(dev, EM28XX_ANALOG_MODE);
> -		em28xx_resolution_set(dev);
> +	if (dev->users == 0) {
> +		v4l2_device_call_all(&dev->v4l2_dev, 0, core, s_power, 1);
>  
> -		/* Needed, since GPIO might have disabled power of
> -		   some i2c device
> -		 */
> -		em28xx_wake_i2c(dev);
> +		if (fh->type == V4L2_BUF_TYPE_VIDEO_CAPTURE) {
> +			em28xx_set_mode(dev, EM28XX_ANALOG_MODE);
em28xx_set_mode() calls em28xx_gpio_set(dev,
INPUT(dev->ctl_input)->gpio) and I'm not sure if this could disable
subdevice power again...

> +			em28xx_resolution_set(dev);
>  
> +			/* Needed, since GPIO might have disabled power of
> +			   some i2c device
> +			*/
> +			em28xx_wake_i2c(dev);
Hmm... your patch didn't change this, but:
Why do we call these functions only in case of V4L2_BUF_TYPE_VIDEO_CAPTURE ?
Isn't it needed for VBI capturing, too ?
em28xx_wake_i2c() is probably also needed for radio mode...

Mauro, what do you think ?

Regards,
Frank

> +		}
>  	}
>  
>  	if (vdev->vfl_type == VFL_TYPE_RADIO) {