[PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support

[PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support

[Lad Prabhakar]

Hi All,

This patch series adds support for BT.656 mode in the ov772x sensor
and also enables color bar test pattern control.

Cheers,
Prabhakar

Changes for v5:
* Put the ep instance back using fwnode_handle_put()
* Renamed BT656 to BT.656
* Correctly handled backward compatibility case falling
  back to parallel mode.

Changes for v4:
* New patch 1/3 to fallback in parallel mode.
* Switched to v4l2_fwnode_endpoint_alloc_parse() for parsing the ep.
* Dropped support for pdat for test pattern control.
* DT documentation patches [1].

Changes for v3:
* Dropped DT binding documentation patch as this is handled by Jacopo.
* Fixed review comments pointed by Jacopo.

[1] https://patchwork.kernel.org/project/
    linux-renesas-soc/list/?series=346809

Lad Prabhakar (3):
  media: i2c: ov772x: Parse endpoint properties
  media: i2c: ov772x: Add support for BT.656 mode
  media: i2c: ov772x: Add test pattern control

 drivers/media/i2c/ov772x.c | 57 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 56 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties

[Lad Prabhakar]

Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
to determine bus-type and store it locally in priv data.

v4l2_fwnode_endpoint_alloc_parse() with bus_type set to
V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling
backward compatibility with existing DT where bus-type isn't specified.

Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2cc6a678069a..4ab4b3c883d0 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -31,6 +31,7 @@
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-event.h>
+#include <media/v4l2-fwnode.h>
 #include <media/v4l2-image-sizes.h>
 #include <media/v4l2-subdev.h>
 
@@ -434,6 +435,7 @@ struct ov772x_priv {
 #ifdef CONFIG_MEDIA_CONTROLLER
 	struct media_pad pad;
 #endif
+	enum v4l2_mbus_type		  bus_type;
 };
 
 /*
@@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
 
 static int ov772x_probe(struct i2c_client *client)
 {
+	struct v4l2_fwnode_endpoint bus_cfg;
+	struct fwnode_handle	*ep;
 	struct ov772x_priv	*priv;
 	int			ret;
 	static const struct regmap_config ov772x_regmap_config = {
@@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client)
 		goto error_clk_put;
 	}
 
+	ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
+					    NULL);
+	if (!ep) {
+		dev_err(&client->dev, "endpoint node not found\n");
+		ret = -EINVAL;
+		goto error_clk_put;
+	}
+
+	/* For backward compatibility with the existing DT where
+	 * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse()
+	 * with bus_type set to V4L2_MBUS_PARALLEL falls back to
+	 * V4L2_MBUS_PARALLEL
+	 */
+	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;
+	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+	priv->bus_type = bus_cfg.bus_type;
+	v4l2_fwnode_endpoint_free(&bus_cfg);
+	fwnode_handle_put(ep);
+	if (ret)
+		goto error_clk_put;
+
 	ret = ov772x_video_probe(priv);
 	if (ret < 0)
 		goto error_gpio_put;
-- 
2.17.1

Re: [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties

[Jacopo Mondi]

Hi Prabhakar,

On Tue, Sep 15, 2020 at 06:42:33PM +0100, Lad Prabhakar wrote:
> Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> to determine bus-type and store it locally in priv data.
>
> v4l2_fwnode_endpoint_alloc_parse() with bus_type set to
> V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling
> backward compatibility with existing DT where bus-type isn't specified.


I don't think this is necessary here. This patch does not need to
handle any retrocompatibility, as only PARALLEL is supported.

The 'right' way to put it to me would be
"Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
to determine the bus type and store it in the driver structure.

Set bus_type to V4L2_MBUS_PARALLEL as it's the only supported one"

See comment in the next patch for retrocompatibility

>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++
>  1 file changed, 25 insertions(+)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..4ab4b3c883d0 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -31,6 +31,7 @@
>  #include <media/v4l2-ctrls.h>
>  #include <media/v4l2-device.h>
>  #include <media/v4l2-event.h>
> +#include <media/v4l2-fwnode.h>
>  #include <media/v4l2-image-sizes.h>
>  #include <media/v4l2-subdev.h>
>
> @@ -434,6 +435,7 @@ struct ov772x_priv {
>  #ifdef CONFIG_MEDIA_CONTROLLER
>  	struct media_pad pad;
>  #endif
> +	enum v4l2_mbus_type		  bus_type;
>  };
>
>  /*
> @@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
>
>  static int ov772x_probe(struct i2c_client *client)
>  {
> +	struct v4l2_fwnode_endpoint bus_cfg;
> +	struct fwnode_handle	*ep;
>  	struct ov772x_priv	*priv;
>  	int			ret;
>  	static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client)
>  		goto error_clk_put;
>  	}
>
> +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> +					    NULL);
> +	if (!ep) {
> +		dev_err(&client->dev, "endpoint node not found\n");
> +		ret = -EINVAL;
> +		goto error_clk_put;
> +	}
> +
> +	/* For backward compatibility with the existing DT where
> +	 * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse()
> +	 * with bus_type set to V4L2_MBUS_PARALLEL falls back to
> +	 * V4L2_MBUS_PARALLEL
> +	 */

You can drop this comment block

> +	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;
> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> +	priv->bus_type = bus_cfg.bus_type;

Set this after if (ret)

> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +	fwnode_handle_put(ep);
> +	if (ret)
> +		goto error_clk_put;
> +
>  	ret = ov772x_video_probe(priv);
>  	if (ret < 0)
>  		goto error_gpio_put;
> --
> 2.17.1
>

Re: [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties

[Jacopo Mondi]

Hi Prabhakar,
  sorry, two more tiny nits

On Wed, Sep 16, 2020 at 09:47:37AM +0200, Jacopo Mondi wrote:
> Hi Prabhakar,
>
> On Tue, Sep 15, 2020 at 06:42:33PM +0100, Lad Prabhakar wrote:
> > Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> > to determine bus-type and store it locally in priv data.
> >
> > v4l2_fwnode_endpoint_alloc_parse() with bus_type set to
> > V4L2_MBUS_PARALLEL falls back to V4L2_MBUS_PARALLEL thus handling
> > backward compatibility with existing DT where bus-type isn't specified.
>
>
> I don't think this is necessary here. This patch does not need to
> handle any retrocompatibility, as only PARALLEL is supported.
>
> The 'right' way to put it to me would be
> "Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> to determine the bus type and store it in the driver structure.
>
> Set bus_type to V4L2_MBUS_PARALLEL as it's the only supported one"
>
> See comment in the next patch for retrocompatibility
>
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov772x.c | 25 +++++++++++++++++++++++++
> >  1 file changed, 25 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 2cc6a678069a..4ab4b3c883d0 100644
> > --- a/drivers/media/i2c/ov772x.c
> > +++ b/drivers/media/i2c/ov772x.c
> > @@ -31,6 +31,7 @@
> >  #include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-device.h>
> >  #include <media/v4l2-event.h>
> > +#include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-image-sizes.h>
> >  #include <media/v4l2-subdev.h>
> >
> > @@ -434,6 +435,7 @@ struct ov772x_priv {
> >  #ifdef CONFIG_MEDIA_CONTROLLER
> >  	struct media_pad pad;
> >  #endif
> > +	enum v4l2_mbus_type		  bus_type;
> >  };
> >
> >  /*
> > @@ -1354,6 +1356,8 @@ static const struct v4l2_subdev_ops ov772x_subdev_ops = {
> >
> >  static int ov772x_probe(struct i2c_client *client)
> >  {
> > +	struct v4l2_fwnode_endpoint bus_cfg;
> > +	struct fwnode_handle	*ep;
> >  	struct ov772x_priv	*priv;
> >  	int			ret;
> >  	static const struct regmap_config ov772x_regmap_config = {
> > @@ -1415,6 +1419,27 @@ static int ov772x_probe(struct i2c_client *client)
> >  		goto error_clk_put;
> >  	}
> >
> > +	ep = fwnode_graph_get_next_endpoint(dev_fwnode(&client->dev),
> > +					    NULL);
> > +	if (!ep) {
> > +		dev_err(&client->dev, "endpoint node not found\n");

Nit: other error messages in the driver start with a capital letter,

> > +		ret = -EINVAL;
> > +		goto error_clk_put;
> > +	}
> > +
> > +	/* For backward compatibility with the existing DT where
> > +	 * bus-type isn't specified v4l2_fwnode_endpoint_alloc_parse()
> > +	 * with bus_type set to V4L2_MBUS_PARALLEL falls back to
> > +	 * V4L2_MBUS_PARALLEL
> > +	 */
>
> You can drop this comment block
>

Or better move it to the next patch. Two nits in the meantime:

Use
        /*
         * This

in place of

        /* This

And I would write it as something like

        /*
         * For backward compatibility with older DTS where the
         * bus-type property was not mandatory, assume
         * V4L2_MBUS_PARALLEL as it was the only supported bus at the
         * time. v4l2_fwnode_endpoint_alloc_parse() will not fail if
         * 'bus-type' is not specified.
         */

Thanks
   j

> > +	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
> > +	priv->bus_type = bus_cfg.bus_type;
>
> Set this after if (ret)
>
> > +	v4l2_fwnode_endpoint_free(&bus_cfg);
> > +	fwnode_handle_put(ep);
> > +	if (ret)
> > +		goto error_clk_put;
> > +
> >  	ret = ov772x_video_probe(priv);
> >  	if (ret < 0)
> >  		goto error_gpio_put;
> > --
> > 2.17.1
> >

[PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode

[Lad Prabhakar]

Add support to read the bus-type for V4L2_MBUS_BT656 and
enable BT.656 mode in the sensor if needed.

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

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 4ab4b3c883d0..869f2d94faec 100644
--- a/drivers/media/i2c/ov772x.c
+++ b/drivers/media/i2c/ov772x.c
@@ -583,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
 	if (priv->streaming == enable)
 		goto done;
 
+	if (priv->bus_type == V4L2_MBUS_BT656) {
+		ret = regmap_update_bits(priv->regmap, COM7, ITU656_ON_OFF,
+					 enable ? ITU656_ON_OFF : ~ITU656_ON_OFF);
+		if (ret)
+			goto done;
+	}
+
 	ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
 				 enable ? 0 : SOFT_SLEEP_MODE);
 	if (ret)
@@ -1436,9 +1443,17 @@ static int ov772x_probe(struct i2c_client *client)
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	priv->bus_type = bus_cfg.bus_type;
 	v4l2_fwnode_endpoint_free(&bus_cfg);
+	if (ret) {
+		bus_cfg = (struct v4l2_fwnode_endpoint)
+			  { .bus_type = V4L2_MBUS_BT656 };
+		ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
+		if (ret) {
+			fwnode_handle_put(ep);
+			goto error_clk_put;
+		}
+		priv->bus_type = bus_cfg.bus_type;
+	}
 	fwnode_handle_put(ep);
-	if (ret)
-		goto error_clk_put;
 
 	ret = ov772x_video_probe(priv);
 	if (ret < 0)
-- 
2.17.1

Re: [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode

[Jacopo Mondi]

Hi Prabhakar,

On Tue, Sep 15, 2020 at 06:42:34PM +0100, Lad Prabhakar wrote:
> Add support to read the bus-type for V4L2_MBUS_BT656 and
> enable BT.656 mode in the sensor if needed.

Here we should be concerned about retro-compatibility, as a new bus
type is added. I would move the comment you had in 1/3 to this patch.

Otherwise the code flow looks ok: if no bus-type is specified assume
parallel as it was the only supported bus type at the time.
If someone wants BT.656 it has to be a new DTS and then the bus-type
property is mandatory.

>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Reviewed-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/i2c/ov772x.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 4ab4b3c883d0..869f2d94faec 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
> @@ -583,6 +583,13 @@ static int ov772x_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (priv->streaming == enable)
>  		goto done;
>
> +	if (priv->bus_type == V4L2_MBUS_BT656) {
> +		ret = regmap_update_bits(priv->regmap, COM7, ITU656_ON_OFF,
> +					 enable ? ITU656_ON_OFF : ~ITU656_ON_OFF);
> +		if (ret)
> +			goto done;
> +	}
> +
>  	ret = regmap_update_bits(priv->regmap, COM2, SOFT_SLEEP_MODE,
>  				 enable ? 0 : SOFT_SLEEP_MODE);
>  	if (ret)
> @@ -1436,9 +1443,17 @@ static int ov772x_probe(struct i2c_client *client)
>  	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
>  	priv->bus_type = bus_cfg.bus_type;
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
> +	if (ret) {
> +		bus_cfg = (struct v4l2_fwnode_endpoint)
> +			  { .bus_type = V4L2_MBUS_BT656 };
> +		ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);

If you really want to keep using alloc_parse() you should remember to
endpoint_free() here.

> +		if (ret) {
> +			fwnode_handle_put(ep);
> +			goto error_clk_put;
> +		}
> +		priv->bus_type = bus_cfg.bus_type;
> +	}
>  	fwnode_handle_put(ep);

I would assign priv->bus_type here.

Also, this has grown quite a bit, have you considered making a
ov772x_parse_dt() function ?

With this last changes I think we're good to go. I'll send tags on
the next version!

Thank you for your perseverance

> -	if (ret)
> -		goto error_clk_put;
>
>  	ret = ov772x_video_probe(priv);
>  	if (ret < 0)
> --
> 2.17.1
>

[PATCH v5 3/3] media: i2c: ov772x: Add test pattern control

[Lad Prabhakar]

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 869f2d94faec..3f68801692f4 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)
@@ -1405,6 +1416,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;
-- 
2.17.1