diff mbox series

[v11,11/15] media: tvp5150: add s_power callback

Message ID 20190930093900.16524-12-m.felsch@pengutronix.de (mailing list archive)
State New, archived
Headers show
Series TVP5150 Features and fixes | expand

Commit Message

Marco Felsch Sept. 30, 2019, 9:38 a.m. UTC 
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(-)

Comments

Sakari Ailus Oct. 24, 2019, 11:59 a.m. UTC | #1 
Hi Marco,

On Mon, Sep 30, 2019 at 11:38:56AM +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 dda9f0a2995f..4afe2093b950 100644
> --- a/drivers/media/i2c/tvp5150.c
> +++ b/drivers/media/i2c/tvp5150.c
> @@ -1356,11 +1356,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 to do this instead?

> +
> +	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;
> @@ -1373,15 +1388,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;
>  }
> @@ -1580,6 +1590,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 = {
> -- 
> 2.20.1
>
Marco Felsch Nov. 8, 2019, 10:25 a.m. UTC | #2 
Hi Sakari,

On 19-10-24 14:59, Sakari Ailus wrote:
> Hi Marco,
> 
> On Mon, Sep 30, 2019 at 11:38:56AM +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 dda9f0a2995f..4afe2093b950 100644
> > --- a/drivers/media/i2c/tvp5150.c
> > +++ b/drivers/media/i2c/tvp5150.c
> > @@ -1356,11 +1356,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 to do this instead?

You mean I should add a simple runtime_resume/suspend() which is called
during v4l2_subdev_internal_ops.open/close()? Of course I can do that
but why?

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;
> > @@ -1373,15 +1388,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;
> >  }
> > @@ -1580,6 +1590,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 = {
> > -- 
> > 2.20.1
> > 
> 
> -- 
> Sakari Ailus
> sakari.ailus@linux.intel.com
>
Sakari Ailus Nov. 8, 2019, 10:38 a.m. UTC | #3 
Hi Marco,

On Fri, Nov 08, 2019 at 11:25:02AM +0100, Marco Felsch wrote:
> Hi Sakari,
> 
> On 19-10-24 14:59, Sakari Ailus wrote:
> > Hi Marco,
> > 
> > On Mon, Sep 30, 2019 at 11:38:56AM +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 dda9f0a2995f..4afe2093b950 100644
> > > --- a/drivers/media/i2c/tvp5150.c
> > > +++ b/drivers/media/i2c/tvp5150.c
> > > @@ -1356,11 +1356,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 to do this instead?
> 
> You mean I should add a simple runtime_resume/suspend() which is called
> during v4l2_subdev_internal_ops.open/close()? Of course I can do that
> but why?

There's no reason to use generic mechanisms that have been around for ten
or so years. Eventually the s_power() op will be dropped.
diff mbox series

Patch

diff --git a/drivers/media/i2c/tvp5150.c b/drivers/media/i2c/tvp5150.c
index dda9f0a2995f..4afe2093b950 100644
--- a/drivers/media/i2c/tvp5150.c
+++ b/drivers/media/i2c/tvp5150.c
@@ -1356,11 +1356,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;
@@ -1373,15 +1388,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;
 }
@@ -1580,6 +1590,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 = {