diff mbox series

[v7,3/3] media: i2c: ov772x: Add test pattern control

Message ID 20201002165656.16744-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive)
State Changes Requested, archived
Delegated to: Sakari Ailus
Headers
Series media: i2c: ov772x: Enable BT.656 mode and test pattern support |

Commit Message

Prabhakar Mahadev Lad Oct. 2, 2020, 4:56 p.m. UTC 
  Add support for test pattern control supported by the sensor.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov772x.c | 17 ++++++++++++++++-
 1 file changed, 16 insertions(+), 1 deletion(-)

Comments

Sakari Ailus Oct. 2, 2020, 9:13 p.m. UTC | #1 
On Fri, Oct 02, 2020 at 05:56:56PM +0100, Lad Prabhakar wrote:
> Add support for test pattern control supported by the sensor.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov772x.c | 17 ++++++++++++++++-
>  1 file changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 6b46ad493bf7..b7e10c34ef59 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -227,7 +227,7 @@
>  
>  /* COM3 */
>  #define SWAP_MASK       (SWAP_RGB | SWAP_YUV | SWAP_ML)
> -#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG)
> +#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST)
>  
>  #define VFLIP_IMG       0x80	/* Vertical flip image ON/OFF selection */
>  #define HFLIP_IMG       0x40	/* Horizontal mirror image ON/OFF selection */
> @@ -425,6 +425,7 @@ struct ov772x_priv {
>  	const struct ov772x_win_size     *win;
>  	struct v4l2_ctrl		 *vflip_ctrl;
>  	struct v4l2_ctrl		 *hflip_ctrl;
> +	unsigned int			  test_pattern;

Alignment.

You can get away with one or possibly two but three is too many in such a
small number of lines. :-)

>  	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
>  	struct v4l2_ctrl		 *band_filter_ctrl;
>  	unsigned int			  fps;
> @@ -540,6 +541,11 @@ static const struct ov772x_win_size ov772x_win_sizes[] = {
>  	},
>  };
>  
> +static const char * const ov772x_test_pattern_menu[] = {
> +	"Disabled",
> +	"Vertical Color Bar Type 1",
> +};
> +
>  /*
>   * frame rate settings lists
>   */
> @@ -809,6 +815,9 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
>  		}
>  
>  		return ret;
> +	case V4L2_CID_TEST_PATTERN:
> +		priv->test_pattern = ctrl->val;
> +		return 0;
>  	}
>  
>  	return -EINVAL;
> @@ -1107,6 +1116,8 @@ static int ov772x_set_params(struct ov772x_priv *priv,
>  		val ^= VFLIP_IMG;
>  	if (priv->hflip_ctrl->val)
>  		val ^= HFLIP_IMG;
> +	if (priv->test_pattern)
> +		val |= SCOLOR_TEST;
>  
>  	ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
>  	if (ret < 0)
> @@ -1442,6 +1453,10 @@ static int ov772x_probe(struct i2c_client *client)
>  	priv->band_filter_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
>  						   V4L2_CID_BAND_STOP_FILTER,
>  						   0, 256, 1, 0);
> +	v4l2_ctrl_new_std_menu_items(&priv->hdl, &ov772x_ctrl_ops,
> +				     V4L2_CID_TEST_PATTERN,
> +				     ARRAY_SIZE(ov772x_test_pattern_menu) - 1,
> +				     0, 0, ov772x_test_pattern_menu);
>  	priv->subdev.ctrl_handler = &priv->hdl;
>  	if (priv->hdl.error) {
>  		ret = priv->hdl.error;
Lad, Prabhakar Oct. 2, 2020, 9:32 p.m. UTC | #2 
Hi Sakari,

Thank you for the review.

On Fri, Oct 2, 2020 at 10:13 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Fri, Oct 02, 2020 at 05:56:56PM +0100, Lad Prabhakar wrote:
> > Add support for test pattern control supported by the sensor.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov772x.c | 17 ++++++++++++++++-
> >  1 file changed, 16 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 6b46ad493bf7..b7e10c34ef59 100644
> > --- a/drivers/media/i2c/ov772x.c
> > +++ b/drivers/media/i2c/ov772x.c
> > @@ -227,7 +227,7 @@
> >
> >  /* COM3 */
> >  #define SWAP_MASK       (SWAP_RGB | SWAP_YUV | SWAP_ML)
> > -#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG)
> > +#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST)
> >
> >  #define VFLIP_IMG       0x80 /* Vertical flip image ON/OFF selection */
> >  #define HFLIP_IMG       0x40 /* Horizontal mirror image ON/OFF selection */
> > @@ -425,6 +425,7 @@ struct ov772x_priv {
> >       const struct ov772x_win_size     *win;
> >       struct v4l2_ctrl                 *vflip_ctrl;
> >       struct v4l2_ctrl                 *hflip_ctrl;
> > +     unsigned int                      test_pattern;
>
> Alignment.
>
> You can get away with one or possibly two but three is too many in such a
> small number of lines. :-)
>
It's aligned as per structure members (non pointers)

Cheers
Prabhakar

> >       /* band_filter = COM8[5] ? 256 - BDBASE : 0 */
> >       struct v4l2_ctrl                 *band_filter_ctrl;
> >       unsigned int                      fps;
> > @@ -540,6 +541,11 @@ static const struct ov772x_win_size ov772x_win_sizes[] = {
> >       },
> >  };
> >
> > +static const char * const ov772x_test_pattern_menu[] = {
> > +     "Disabled",
> > +     "Vertical Color Bar Type 1",
> > +};
> > +
> >  /*
> >   * frame rate settings lists
> >   */
> > @@ -809,6 +815,9 @@ static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
> >               }
> >
> >               return ret;
> > +     case V4L2_CID_TEST_PATTERN:
> > +             priv->test_pattern = ctrl->val;
> > +             return 0;
> >       }
> >
> >       return -EINVAL;
> > @@ -1107,6 +1116,8 @@ static int ov772x_set_params(struct ov772x_priv *priv,
> >               val ^= VFLIP_IMG;
> >       if (priv->hflip_ctrl->val)
> >               val ^= HFLIP_IMG;
> > +     if (priv->test_pattern)
> > +             val |= SCOLOR_TEST;
> >
> >       ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
> >       if (ret < 0)
> > @@ -1442,6 +1453,10 @@ static int ov772x_probe(struct i2c_client *client)
> >       priv->band_filter_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
> >                                                  V4L2_CID_BAND_STOP_FILTER,
> >                                                  0, 256, 1, 0);
> > +     v4l2_ctrl_new_std_menu_items(&priv->hdl, &ov772x_ctrl_ops,
> > +                                  V4L2_CID_TEST_PATTERN,
> > +                                  ARRAY_SIZE(ov772x_test_pattern_menu) - 1,
> > +                                  0, 0, ov772x_test_pattern_menu);
> >       priv->subdev.ctrl_handler = &priv->hdl;
> >       if (priv->hdl.error) {
> >               ret = priv->hdl.error;
>
> --
> Sakari Ailus
Sakari Ailus Oct. 2, 2020, 9:58 p.m. UTC | #3 
On Fri, Oct 02, 2020 at 10:32:05PM +0100, Lad, Prabhakar wrote:
> Hi Sakari,
> 
> Thank you for the review.
> 
> On Fri, Oct 2, 2020 at 10:13 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Fri, Oct 02, 2020 at 05:56:56PM +0100, Lad Prabhakar wrote:
> > > Add support for test pattern control supported by the sensor.
> > >
> > > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > > Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  drivers/media/i2c/ov772x.c | 17 ++++++++++++++++-
> > >  1 file changed, 16 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > > index 6b46ad493bf7..b7e10c34ef59 100644
> > > --- a/drivers/media/i2c/ov772x.c
> > > +++ b/drivers/media/i2c/ov772x.c
> > > @@ -227,7 +227,7 @@
> > >
> > >  /* COM3 */
> > >  #define SWAP_MASK       (SWAP_RGB | SWAP_YUV | SWAP_ML)
> > > -#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG)
> > > +#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST)
> > >
> > >  #define VFLIP_IMG       0x80 /* Vertical flip image ON/OFF selection */
> > >  #define HFLIP_IMG       0x40 /* Horizontal mirror image ON/OFF selection */
> > > @@ -425,6 +425,7 @@ struct ov772x_priv {
> > >       const struct ov772x_win_size     *win;
> > >       struct v4l2_ctrl                 *vflip_ctrl;
> > >       struct v4l2_ctrl                 *hflip_ctrl;
> > > +     unsigned int                      test_pattern;
> >
> > Alignment.
> >
> > You can get away with one or possibly two but three is too many in such a
> > small number of lines. :-)
> >
> It's aligned as per structure members (non pointers)

What a weird practice. Oh well. Keep as-is then.

checkpatch.pl no longer seems to complain about lines over 80. That keeps
the number of warnings lower but may lead to unintentional long lines when
you don't need them.
diff mbox series

Patch

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 6b46ad493bf7..b7e10c34ef59 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -227,7 +227,7 @@ 
 
 /* COM3 */
 #define SWAP_MASK       (SWAP_RGB | SWAP_YUV | SWAP_ML)
-#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG)
+#define IMG_MASK        (VFLIP_IMG | HFLIP_IMG | SCOLOR_TEST)
 
 #define VFLIP_IMG       0x80	/* Vertical flip image ON/OFF selection */
 #define HFLIP_IMG       0x40	/* Horizontal mirror image ON/OFF selection */
@@ -425,6 +425,7 @@  struct ov772x_priv {
 	const struct ov772x_win_size     *win;
 	struct v4l2_ctrl		 *vflip_ctrl;
 	struct v4l2_ctrl		 *hflip_ctrl;
+	unsigned int			  test_pattern;
 	/* band_filter = COM8[5] ? 256 - BDBASE : 0 */
 	struct v4l2_ctrl		 *band_filter_ctrl;
 	unsigned int			  fps;
@@ -540,6 +541,11 @@  static const struct ov772x_win_size ov772x_win_sizes[] = {
 	},
 };
 
+static const char * const ov772x_test_pattern_menu[] = {
+	"Disabled",
+	"Vertical Color Bar Type 1",
+};
+
 /*
  * frame rate settings lists
  */
@@ -809,6 +815,9 @@  static int ov772x_s_ctrl(struct v4l2_ctrl *ctrl)
 		}
 
 		return ret;
+	case V4L2_CID_TEST_PATTERN:
+		priv->test_pattern = ctrl->val;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -1107,6 +1116,8 @@  static int ov772x_set_params(struct ov772x_priv *priv,
 		val ^= VFLIP_IMG;
 	if (priv->hflip_ctrl->val)
 		val ^= HFLIP_IMG;
+	if (priv->test_pattern)
+		val |= SCOLOR_TEST;
 
 	ret = regmap_update_bits(priv->regmap, COM3, SWAP_MASK | IMG_MASK, val);
 	if (ret < 0)
@@ -1442,6 +1453,10 @@  static int ov772x_probe(struct i2c_client *client)
 	priv->band_filter_ctrl = v4l2_ctrl_new_std(&priv->hdl, &ov772x_ctrl_ops,
 						   V4L2_CID_BAND_STOP_FILTER,
 						   0, 256, 1, 0);
+	v4l2_ctrl_new_std_menu_items(&priv->hdl, &ov772x_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(ov772x_test_pattern_menu) - 1,
+				     0, 0, ov772x_test_pattern_menu);
 	priv->subdev.ctrl_handler = &priv->hdl;
 	if (priv->hdl.error) {
 		ret = priv->hdl.error;