Re: [RFC PATCH] Add core tuner_standby op, use where needed

[Hans Verkuil]

On 02/19/18 20:27, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Monday, 19 February 2018 15:12:05 EET Hans Verkuil wrote:
>> The v4l2_subdev core s_power op was used to two different things: power
>> on/off sensors or video decoders/encoders and to put a tuner in standby
>> (and only the tuner). There is no 'tuner wakeup' op, that's done
>> automatically when the tuner is accessed.
>>
>> The danger with calling (s_power, 0) to put a tuner into standby is that it
>> is usually broadcast for all subdevs. So a video receiver subdev that also
>> supports s_power will also be powered off, and since there is no
>> corresponding (s_power, 1) they will never be powered on again.
>>
>> In addition, this is specifically meant for tuners only since they draw the
>> most current.
>>
>> This patch adds a new core op called 'tuner_standby' and replaces all calls
>> to (s_power, 0) by tuner_standby. This prevents confusion between the two
>> uses of s_power. Note that there is no overlap: bridge drivers either just
>> want to put the tuner into standby, or they deal with powering on/off
>> sensors. Never both.
>>
>> This also makes it easier to replace s_power for the remaining bridge
>> drivers with some PM code later.
>>
>> Whether we want something similar for tuners in the future is a separate
>> topic. There is a lot of legacy code surrounding tuners, and I am very
>> hesitant about making changes there.
> 
> While I don't request you to make changes, someone should. I assume the 
> tuners 
> are still maintained, aren't they ? :-)

Theoretically. DVB tuners are certainly maintained, but analog TV? I haven't
seen patches for such tuners in years.

>> Signed-off-by: Hans Verkuil 
>> ---
> 
> [snip]
>  
>>  static const struct v4l2_subdev_tuner_ops tuner_tuner_ops = {
>> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
>> index 980a86c08fce..b214da92a5c0 100644
>> --- a/include/media/v4l2-subdev.h
>> +++ b/include/media/v4l2-subdev.h
>> @@ -184,6 +184,9 @@ struct v4l2_subdev_io_pin_config {
>>   * @s_power: puts subdevice in power saving mode (on == 0) or normal
>> operation *  mode (on == 1).
>>   *
>> + * @tuner_standby: puts the tuner in standby mode. It will be woken up
>> + *  automatically the next time it is used.
>> + *
>>   * @interrupt_service_routine: Called by the bridge chip's interrupt
>> service
>>   *  handler, when an interrupt status has be raised due to this subdev,
>>   *  so that this subdev can handle the details.  It may schedule work to be
>> @@ -212,6 +215,7 @@ struct v4l2_subdev_core_ops {
>>  int (*s_register)(struct v4l2_subdev *sd, const struct v4l2_dbg_register
>> *reg); #endif
>>  int (*s_power)(struct v4l2_subdev *sd, int on);
>> +int (*tuner_standby)(struct v4l2_subdev *sd);
> 
> If it's a tuner operation, how about moving it to v4l2_subdev_tuner_ops ? 
> That 
> would at least make it clear that it shouldn't be used by other drivers (and 
> I 
> think we should also mention in the documentation that this is a legacy 
> operation that shouldn't be used for any new purpose).

That makes a lot of sense. I'll move it.

> 
>>  int (*interrupt_service_routine)(struct v4l2_subdev *sd,
>>  u32 status, bool *handled);
>>  int (*subscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,
> 
> I'd prefer the bridge drivers to be fixed to use s_power in a balanced way, 
> but I understand that it might be difficult to achieve in a timely fashion. 
> I'm thus not against this patch, but I don't think it makes too much sense to 
> merge it without a user, that is a patch series that works on removing 
> s_power.
> 

I actually think it is a nice cleanup/clarification regardless of removing
s_power. The s_power op was really abused for tuners. And let's be honest:
having s_power(0) and never s_power(1) was very confusing!

So I believe that this can go in regardless of the other work.

Regards,

Hans

Re: [RFC PATCH] Add core tuner_standby op, use where needed

[Laurent Pinchart]

Hi Hans,

Thank you for the patch.

On Monday, 19 February 2018 15:12:05 EET Hans Verkuil wrote:
> The v4l2_subdev core s_power op was used to two different things: power
> on/off sensors or video decoders/encoders and to put a tuner in standby
> (and only the tuner). There is no 'tuner wakeup' op, that's done
> automatically when the tuner is accessed.
> 
> The danger with calling (s_power, 0) to put a tuner into standby is that it
> is usually broadcast for all subdevs. So a video receiver subdev that also
> supports s_power will also be powered off, and since there is no
> corresponding (s_power, 1) they will never be powered on again.
> 
> In addition, this is specifically meant for tuners only since they draw the
> most current.
> 
> This patch adds a new core op called 'tuner_standby' and replaces all calls
> to (s_power, 0) by tuner_standby. This prevents confusion between the two
> uses of s_power. Note that there is no overlap: bridge drivers either just
> want to put the tuner into standby, or they deal with powering on/off
> sensors. Never both.
> 
> This also makes it easier to replace s_power for the remaining bridge
> drivers with some PM code later.
> 
> Whether we want something similar for tuners in the future is a separate
> topic. There is a lot of legacy code surrounding tuners, and I am very
> hesitant about making changes there.

While I don't request you to make changes, someone should. I assume the tuners 
are still maintained, aren't they ? :-)

> Signed-off-by: Hans Verkuil 
> ---

[snip]
 
>  static const struct v4l2_subdev_tuner_ops tuner_tuner_ops = {
> diff --git a/include/media/v4l2-subdev.h b/include/media/v4l2-subdev.h
> index 980a86c08fce..b214da92a5c0 100644
> --- a/include/media/v4l2-subdev.h
> +++ b/include/media/v4l2-subdev.h
> @@ -184,6 +184,9 @@ struct v4l2_subdev_io_pin_config {
>   * @s_power: puts subdevice in power saving mode (on == 0) or normal
> operation *   mode (on == 1).
>   *
> + * @tuner_standby: puts the tuner in standby mode. It will be woken up
> + *   automatically the next time it is used.
> + *
>   * @interrupt_service_routine: Called by the bridge chip's interrupt
> service
>   *   handler, when an interrupt status has be raised due to this subdev,
>   *   so that this subdev can handle the details.  It may schedule work to be
> @@ -212,6 +215,7 @@ struct v4l2_subdev_core_ops {
>   int (*s_register)(struct v4l2_subdev *sd, const struct v4l2_dbg_register
> *reg); #endif
>   int (*s_power)(struct v4l2_subdev *sd, int on);
> + int (*tuner_standby)(struct v4l2_subdev *sd);

If it's a tuner operation, how about moving it to v4l2_subdev_tuner_ops ? That 
would at least make it clear that it shouldn't be used by other drivers (and I 
think we should also mention in the documentation that this is a legacy 
operation that shouldn't be used for any new purpose).

>   int (*interrupt_service_routine)(struct v4l2_subdev *sd,
>   u32 status, bool *handled);
>   int (*subscribe_event)(struct v4l2_subdev *sd, struct v4l2_fh *fh,

I'd prefer the bridge drivers to be fixed to use s_power in a balanced way, 
but I understand that it might be difficult to achieve in a timely fashion. 
I'm thus not against this patch, but I don't think it makes too much sense to 
merge it without a user, that is a patch series that works on removing 
s_power.

-- 
Regards,

Laurent Pinchart

[RFC PATCH] Add core tuner_standby op, use where needed

[Hans Verkuil]

The v4l2_subdev core s_power op was used to two different things: power on/off
sensors or video decoders/encoders and to put a tuner in standby (and only the
tuner). There is no 'tuner wakeup' op, that's done automatically when the tuner
is accessed.

The danger with calling (s_power, 0) to put a tuner into standby is that it is
usually broadcast for all subdevs. So a video receiver subdev that also supports
s_power will also be powered off, and since there is no corresponding (s_power, 
1)
they will never be powered on again.

In addition, this is specifically meant for tuners only since they draw the most
current.

This patch adds a new core op called 'tuner_standby' and replaces all calls to
(s_power, 0) by tuner_standby. This prevents confusion between the two uses of
s_power. Note that there is no overlap: bridge drivers either just want to put
the tuner into standby, or they deal with powering on/off sensors. Never both.

This also makes it easier to replace s_power for the remaining bridge drivers
with some PM code later.

Whether we want something similar for tuners in the future is a separate topic.
There is a lot of legacy code surrounding tuners, and I am very hesitant about
making changes there.

Signed-off-by: Hans Verkuil 
---
diff --git a/drivers/media/pci/cx23885/cx23885-core.c 
b/drivers/media/pci/cx23885/cx23885-core.c
index 8f63df1cb418..4e9cd88e573e 100644
--- a/drivers/media/pci/cx23885/cx23885-core.c
+++ b/drivers/media/pci/cx23885/cx23885-core.c
@@ -965,7 +965,7 @@ static int cx23885_dev_setup(struct cx23885_dev *dev)
cx23885_i2c_register(>i2c_bus[1]);
cx23885_i2c_register(>i2c_bus[2]);
cx23885_card_setup(dev);
-   call_all(dev, core, s_power, 0);
+   call_all(dev, core, tuner_standby);
cx23885_ir_init(dev);

if (dev->board == CX23885_BOARD_VIEWCAST_460E) {
diff --git a/drivers/media/pci/cx23885/cx23885-dvb.c 
b/drivers/media/pci/cx23885/cx23885-dvb.c
index 700422b538c0..d0560fd4501c 100644
--- a/drivers/media/pci/cx23885/cx23885-dvb.c
+++ b/drivers/media/pci/cx23885/cx23885-dvb.c
@@ -2514,8 +2514,8 @@ static int dvb_register(struct cx23885_tsport *port)
fe1->dvb.frontend->ops.ts_bus_ctrl = cx23885_dvb_bus_ctrl;
 #endif

-   /* Put the analog decoder in standby to keep it quiet */
-   call_all(dev, core, s_power, 0);
+   /* Put the tuner in standby to keep it quiet */
+   call_all(dev, core, tuner_standby);

if (fe0->dvb.frontend->ops.analog_ops.standby)
fe0->dvb.frontend->ops.analog_ops.standby(fe0->dvb.frontend);
diff --git a/drivers/media/pci/cx88/cx88-cards.c 
b/drivers/media/pci/cx88/cx88-cards.c
index 6df21b29ea17..cdb15f378db4 100644
--- a/drivers/media/pci/cx88/cx88-cards.c
+++ b/drivers/media/pci/cx88/cx88-cards.c
@@ -3592,7 +3592,7 @@ static void cx88_card_setup(struct cx88_core *core)
ctl.fname);
call_all(core, tuner, s_config, _cfg);
}
-   call_all(core, core, s_power, 0);
+   call_all(core, core, tuner_standby);
 }

 /* -- */
diff --git a/drivers/media/pci/cx88/cx88-dvb.c 
b/drivers/media/pci/cx88/cx88-dvb.c
index 49a335f4603e..0aa56d505004 100644
--- a/drivers/media/pci/cx88/cx88-dvb.c
+++ b/drivers/media/pci/cx88/cx88-dvb.c
@@ -1631,8 +1631,8 @@ static int dvb_register(struct cx8802_dev *dev)
if (fe1)
fe1->dvb.frontend->ops.ts_bus_ctrl = cx88_dvb_bus_ctrl;

-   /* Put the analog decoder in standby to keep it quiet */
-   call_all(core, core, s_power, 0);
+   /* Put the tuner in standby to keep it quiet */
+   call_all(core, core, tuner_standby);

/* register everything */
res = vb2_dvb_register_bus(>frontends, THIS_MODULE, dev,
diff --git a/drivers/media/pci/saa7134/saa7134-video.c 
b/drivers/media/pci/saa7134/saa7134-video.c
index 1ca6a32ad10e..bd710836a072 100644
--- a/drivers/media/pci/saa7134/saa7134-video.c
+++ b/drivers/media/pci/saa7134/saa7134-video.c
@@ -1200,7 +1200,7 @@ static int video_release(struct file *file)
saa_andorb(SAA7134_OFMT_DATA_A, 0x1f, 0);
saa_andorb(SAA7134_OFMT_DATA_B, 0x1f, 0);

-   saa_call_all(dev, core, s_power, 0);
+   saa_call_all(dev, core, tuner_standby);
if (vdev->vfl_type == VFL_TYPE_RADIO)
saa_call_all(dev, core, ioctl, SAA6588_CMD_CLOSE, );
mutex_unlock(>lock);
diff --git a/drivers/media/tuners/e4000.c b/drivers/media/tuners/e4000.c
index 564a000f503e..2733b7dfe82f 100644
--- a/drivers/media/tuners/e4000.c
+++ b/drivers/media/tuners/e4000.c
@@ -293,18 +293,13 @@ static inline struct e4000_dev 
*e4000_subdev_to_dev(struct v4l2_subdev *sd)
return container_of(sd, struct e4000_dev, sd);
 }

-static int e4000_s_power(struct v4l2_subdev *sd, int on)
+static int e4000_tuner_standby(struct v4l2_subdev *sd)
 {
struct e4000_dev *dev = e4000_subdev_to_dev(sd);