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