| Message ID | 20180813092508.1334-8-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | TVP5150 fixes and new features | expand |
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? For an example, the dw9714 driver does this: drivers/media/i2c/dw9714.c . > + > + return 0; > +} > > static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable) > { > struct tvp5150 *decoder = to_tvp5150(sd); > - unsigned int mask, val = 0, int_val = 0; > + unsigned int mask, val = 0; > > mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE | > TVP5150_MISC_CTL_CLOCK_OE; > @@ -1406,15 +1421,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable) > val = decoder->lock ? decoder->oe : 0; > else > val = decoder->oe; > - int_val = TVP5150_INT_A_LOCK; > v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt); > } > > regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val); > - if (decoder->irq) > - /* Enable / Disable lock interrupt */ > - regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A, > - TVP5150_INT_A_LOCK, int_val); > > return 0; > } > @@ -1616,6 +1626,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = { > .g_register = tvp5150_g_register, > .s_register = tvp5150_s_register, > #endif > + .s_power = tvp5150_s_power, > }; > > static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
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. Regards, Marco > > + > > + return 0; > > +} > > > > static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable) > > { > > struct tvp5150 *decoder = to_tvp5150(sd); > > - unsigned int mask, val = 0, int_val = 0; > > + unsigned int mask, val = 0; > > > > mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE | > > TVP5150_MISC_CTL_CLOCK_OE; > > @@ -1406,15 +1421,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable) > > val = decoder->lock ? decoder->oe : 0; > > else > > val = decoder->oe; > > - int_val = TVP5150_INT_A_LOCK; > > v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt); > > } > > > > regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val); > > - if (decoder->irq) > > - /* Enable / Disable lock interrupt */ > > - regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A, > > - TVP5150_INT_A_LOCK, int_val); > > > > return 0; > > } > > @@ -1616,6 +1626,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = { > > .g_register = tvp5150_g_register, > > .s_register = tvp5150_s_register, > > #endif > > + .s_power = tvp5150_s_power, > > }; > > > > static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = { > > -- > Kind regards, > > Sakari Ailus > sakari.ailus@linux.intel.com >
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 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
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
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); + + return 0; +} static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable) { struct tvp5150 *decoder = to_tvp5150(sd); - unsigned int mask, val = 0, int_val = 0; + unsigned int mask, val = 0; mask = TVP5150_MISC_CTL_YCBCR_OE | TVP5150_MISC_CTL_SYNC_OE | TVP5150_MISC_CTL_CLOCK_OE; @@ -1406,15 +1421,10 @@ static int tvp5150_s_stream(struct v4l2_subdev *sd, int enable) val = decoder->lock ? decoder->oe : 0; else val = decoder->oe; - int_val = TVP5150_INT_A_LOCK; v4l2_subdev_notify_event(&decoder->sd, &tvp5150_ev_fmt); } regmap_update_bits(decoder->regmap, TVP5150_MISC_CTL, mask, val); - if (decoder->irq) - /* Enable / Disable lock interrupt */ - regmap_update_bits(decoder->regmap, TVP5150_INT_ENABLE_REG_A, - TVP5150_INT_A_LOCK, int_val); return 0; } @@ -1616,6 +1626,7 @@ static const struct v4l2_subdev_core_ops tvp5150_core_ops = { .g_register = tvp5150_g_register, .s_register = tvp5150_s_register, #endif + .s_power = tvp5150_s_power, }; static const struct v4l2_subdev_tuner_ops tvp5150_tuner_ops = {
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(-)