Re: [PATCH v7 3/3] media: i2c: ov772x: Add test pattern control
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 > 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 > > > Reviewed-by: Biju Das > > > Reviewed-by: Jacopo Mondi > > > --- > > > 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. -- Sakari Ailus
Re: [PATCH v7 3/3] media: i2c: ov772x: Add test pattern control
Hi Sakari, Thank you for the review. On Fri, Oct 2, 2020 at 10:13 PM Sakari Ailus 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 > > Reviewed-by: Biju Das > > Reviewed-by: Jacopo Mondi > > --- > > 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(>hdl, > > _ctrl_ops, > > V4L2_CID_BAND_STOP_FILTER, > > 0, 256, 1, 0); > > + v4l2_ctrl_new_std_menu_items(>hdl, _ctrl_ops, > > + V4L2_CID_TEST_PATTERN, > > + ARRAY_SIZE(ov772x_test_pattern_menu) - 1, > > + 0, 0, ov772x_test_pattern_menu); > > priv->subdev.ctrl_handler = >hdl; > > if (priv->hdl.error) { > > ret = priv->hdl.error; > > -- > Sakari Ailus
Re: [PATCH v7 3/3] media: i2c: ov772x: Add test pattern control
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 > Reviewed-by: Biju Das > Reviewed-by: Jacopo Mondi > --- > 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(>hdl, _ctrl_ops, > V4L2_CID_BAND_STOP_FILTER, > 0, 256, 1, 0); > + v4l2_ctrl_new_std_menu_items(>hdl, _ctrl_ops, > + V4L2_CID_TEST_PATTERN, > + ARRAY_SIZE(ov772x_test_pattern_menu) - 1, > + 0, 0, ov772x_test_pattern_menu); > priv->subdev.ctrl_handler = >hdl; > if (priv->hdl.error) { > ret = priv->hdl.error; -- Sakari Ailus