[PATCH] em28xx: balance subdevice power-off calls

[PATCH] em28xx: balance subdevice power-off calls

[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);
 
 	/* 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_resolution_set(dev);
 
+			/* Needed, since GPIO might have disabled power of
+			   some i2c device
+			*/
+			em28xx_wake_i2c(dev);
+		}
 	}
 
 	if (vdev->vfl_type == VFL_TYPE_RADIO) {
-- 
1.7.2.5

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

[Frank Schäfer]

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