Re: [PATCHv2] media: add tuner standby op, use where needed

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Wednesday, 21 February 2018 09:40:29 EET Hans Verkuil wrote:
> On 02/21/2018 01:02 AM, Laurent Pinchart wrote:
> > On Tuesday, 20 February 2018 11:44:20 EET Hans Verkuil wrote:
> >> The v4l2_subdev core s_power op was used for 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
> >> 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 tuner op called 'standby' and replaces all calls to
> >> (core, 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 cleaner 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 <hans.verkuil@cisco.com>
> >> ---
> >> Changes since v1:
> >> - move the standby op to the tuner_ops, which makes much more sense.
> >> ---
> > 
> > [snip]
> > 
> >> diff --git a/drivers/media/v4l2-core/tuner-core.c
> >> b/drivers/media/v4l2-core/tuner-core.c index 82852f23a3b6..cb126baf8771
> >> 100644
> >> --- a/drivers/media/v4l2-core/tuner-core.c
> >> +++ b/drivers/media/v4l2-core/tuner-core.c
> >> @@ -1099,23 +1099,15 @@ static int tuner_s_radio(struct v4l2_subdev *sd)
> >>   */
> >>  
> >>  /**
> >> - * tuner_s_power - controls the power state of the tuner
> >> + * tuner_standby - controls the power state of the tuner
> > 
> > I'd update the description too.
> > 
> >>   * @sd: pointer to struct v4l2_subdev
> >>   * @on: a zero value puts the tuner to sleep, non-zero wakes it up
> > 
> > And this parameter doesn't exist anymore. You could have caught that by
> > compiling the documentation.
> 
> Oops! I'll make a v3. Thanks for catching this.
> 
> >>   */
> >> -static int tuner_s_power(struct v4l2_subdev *sd, int on)
> >> +static int tuner_standby(struct v4l2_subdev *sd)
> >>  {
> >>  	struct tuner *t = to_tuner(sd);
> >>  	struct analog_demod_ops *analog_ops = &t->fe.ops.analog_ops;
> >> 
> >> -	if (on) {
> >> -		if (t->standby && set_mode(t, t->mode) == 0) {
> >> -			dprintk("Waking up tuner\n");
> >> -			set_freq(t, 0);
> >> -		}
> >> -		return 0;
> >> -	}
> >> -
> > 
> > Interesting how this code was not used. I've had a look at tuner-core
> > driver out of curiosity, it clearly shows its age :/ I2C address probing
> > belongs to another century.
> 
> Not really. It's still needed for USB/PCI devices.

Why is that ?

> I was also surprised that it wasn't used. I expected to see some internal
> calls to tuner_s_power(sd, 1) to turn things on, but that's not what
> happened.
> 
> >>  	dprintk("Putting tuner to sleep\n");
> >>  	t->standby = true;
> >>  	if (analog_ops->standby)

[snip]

-- 
Regards,

Laurent Pinchart