Re: [RFC PATCH] Add core tuner_standby op, use where needed
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
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
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);