linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support
@ 2020-09-15 17:42 Lad Prabhakar
  2020-09-15 17:42 ` [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
                   ` (2 more replies)
  0 siblings, 3 replies; 7+ messages in thread
From: Lad Prabhakar @ 2020-09-15 17:42 UTC (permalink / raw)
  To: Jacopo Mondi, Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Lad Prabhakar, 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


^ permalink raw reply	[flat|nested] 7+ messages in thread

* [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-15 17:42 [PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support Lad Prabhakar
@ 2020-09-15 17:42 ` Lad Prabhakar
  2020-09-16  7:47   ` Jacopo Mondi
  2020-09-15 17:42 ` [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode Lad Prabhakar
  2020-09-15 17:42 ` [PATCH v5 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
  2 siblings, 1 reply; 7+ messages in thread
From: Lad Prabhakar @ 2020-09-15 17:42 UTC (permalink / raw)
  To: Jacopo Mondi, Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Lad Prabhakar, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode
  2020-09-15 17:42 [PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support Lad Prabhakar
  2020-09-15 17:42 ` [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
@ 2020-09-15 17:42 ` Lad Prabhakar
  2020-09-16  8:24   ` Jacopo Mondi
  2020-09-15 17:42 ` [PATCH v5 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
  2 siblings, 1 reply; 7+ messages in thread
From: Lad Prabhakar @ 2020-09-15 17:42 UTC (permalink / raw)
  To: Jacopo Mondi, Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Lad Prabhakar, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* [PATCH v5 3/3] media: i2c: ov772x: Add test pattern control
  2020-09-15 17:42 [PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support Lad Prabhakar
  2020-09-15 17:42 ` [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
  2020-09-15 17:42 ` [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode Lad Prabhakar
@ 2020-09-15 17:42 ` Lad Prabhakar
  2 siblings, 0 replies; 7+ messages in thread
From: Lad Prabhakar @ 2020-09-15 17:42 UTC (permalink / raw)
  To: Jacopo Mondi, Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Lad Prabhakar, 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


^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-15 17:42 ` [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
@ 2020-09-16  7:47   ` Jacopo Mondi
  2020-09-16  8:30     ` Jacopo Mondi
  0 siblings, 1 reply; 7+ messages in thread
From: Jacopo Mondi @ 2020-09-16  7:47 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab,
	linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Prabhakar

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode
  2020-09-15 17:42 ` [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode Lad Prabhakar
@ 2020-09-16  8:24   ` Jacopo Mondi
  0 siblings, 0 replies; 7+ messages in thread
From: Jacopo Mondi @ 2020-09-16  8:24 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab,
	linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Prabhakar

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
>

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-16  7:47   ` Jacopo Mondi
@ 2020-09-16  8:30     ` Jacopo Mondi
  0 siblings, 0 replies; 7+ messages in thread
From: Jacopo Mondi @ 2020-09-16  8:30 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Sakari Ailus, Laurent Pinchart, Mauro Carvalho Chehab,
	linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Prabhakar

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
> >

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2020-09-16  8:26 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 17:42 [PATCH v5 0/3] media: i2c: ov772x: Enable BT.656 mode and test pattern support Lad Prabhakar
2020-09-15 17:42 ` [PATCH v5 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
2020-09-16  7:47   ` Jacopo Mondi
2020-09-16  8:30     ` Jacopo Mondi
2020-09-15 17:42 ` [PATCH v5 2/3] media: i2c: ov772x: Add support for BT.656 mode Lad Prabhakar
2020-09-16  8:24   ` Jacopo Mondi
2020-09-15 17:42 ` [PATCH v5 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).