| Message ID | 20201002165656.16744-4-prabhakar.mahadev-lad.rj@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | media: i2c: ov772x: Enable BT.656 mode and test pattern support | expand |
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;
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
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 --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;