Re: [PATCH v2 7/7] [media] tvp5150: add s_power callback

[Marco Felsch <m.felsch@pengutronix.de>]

Hi,

On 18-09-14 15:57, Mauro Carvalho Chehab wrote:
> Em Fri, 14 Sep 2018 20:20:46 +0200
> Marco Felsch <m.felsch@pengutronix.de> escreveu:
> 
> > Hi Sakari,
> > 
> > On 18-09-14 16:23, Sakari Ailus wrote:
> > > Hi Marco,
> > > 
> > > On Mon, Aug 13, 2018 at 11:25:08AM +0200, Marco Felsch wrote:  
> > > > Don't en-/disable the interrupts during s_stream because someone can
> > > > disable the stream but wants to get informed if the stream is locked
> > > > again. So keep the interrupts enabled the whole time the pipeline is
> > > > opened.
> > > > 
> > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > > > ---
> > > >  drivers/media/i2c/tvp5150.c | 23 +++++++++++++++++------
> > > >  1 file changed, 17 insertions(+), 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
> > > > index e736f609fecd..e296f5bfae21 100644
> > > > --- a/drivers/media/i2c/tvp5150.c
> > > > +++ b/drivers/media/i2c/tvp5150.c
> > > > @@ -1389,11 +1389,26 @@ static const struct media_entity_operations tvp5150_sd_media_ops = {
> > > >  /****************************************************************************
> > > >  			I2C Command
> > > >   ****************************************************************************/
> > > > +static int tvp5150_s_power(struct  v4l2_subdev *sd, int on)
> > > > +{
> > > > +	struct tvp5150 *decoder = to_tvp5150(sd);
> > > > +	unsigned int val = 0;
> > > > +
> > > > +	if (on)
> > > > +		val = TVP5150_INT_A_LOCK;
> > > > +
> > > > +	if (decoder->irq)
> > > > +		/* Enable / Disable lock interrupt */
> > > > +		regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A,
> > > > +				   TVP5150_INT_A_LOCK, val);  
> > > 
> > > Could you use runtime PM instead?  
> > 
> > I will test it next monday. What's the different between s_power and
> > runtime PM?
> > 
> > > 
> > > For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c .  
> > 
> > Hopefully I got you right, should I use the
> > v4l2_subdev_internal_ops.open/close and call the pm_runtime_put/get
> > there or did you mean the driver.pm callbacks? I'm not that familiar
> > with the pm ops at the moment, sorry.

I did some tests with the PM runtime ops. I used the suspend() callback
to disbale the IRQ and the resume() callback to enable it. But this
won't work with my IRQ-driven setup because if the signal is not locked
(maybe lost due to no input stream) the tvp went into suspend. In this
mode the IRQ is disabled and I have no chance to enable the chip again.

As Mauro pointed out below, there maybe some other issues I can't test
at the moment. I'm with Mauro to keep the .s_power() callback for this
series. There should be another patchset which covers the PM rework for
all devices using the TVP5150.

> 
> I guess the main issue here is: will this work if the bridge
> driver is em28xx?
> 
> Whatever change we do, tvp5150 should still fully work with em28xx,
> as several devices use this demod there.
> 
> Changing em28xx to cope with runtime PM would be *very* complex,
> as there are lots of other drivers that can work with it, and
> touching those will affect lots of other drivers. At the end, it
> will very likely affect all PCI/PCIe V4L2 drivers, and several
> USB ones.
> 
> If it can be done without affecting PM with em28xx, let's do it.
> Otherwise, let's stick with s_power on this series, and let 
> the mass PM rework on non-platform drivers to happen on some
> separate patchset.
> 
> Thanks,
> Mauro
> 

Kind regards,
Marco