linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v4 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support
@ 2020-09-13 18:42 Lad Prabhakar
  2020-09-13 18:42 ` [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Lad Prabhakar @ 2020-09-13 18: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 BT656 mode in the ov772x sensor
and also enables color bar test pattern control.

Cheers,
Prabhakar

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 BT656 mode
  media: i2c: ov772x: Add test pattern control

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

-- 
2.17.1


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

* [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-13 18:42 [PATCH v4 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
@ 2020-09-13 18:42 ` Lad Prabhakar
  2020-09-14  6:54   ` Sakari Ailus
  2020-09-14 14:09   ` Jacopo Mondi
  2020-09-13 18:42 ` [PATCH v4 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
  2020-09-13 18:42 ` [PATCH v4 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
  2 siblings, 2 replies; 8+ messages in thread
From: Lad Prabhakar @ 2020-09-13 18: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.

Also for backward compatibility with the existing DT where bus-type
isnt specified fallback to V4L2_MBUS_PARALLEL.

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

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 2cc6a678069a..551082aa7026 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 = { .bus_type = 0 };
+	struct fwnode_handle	*ep;
 	struct ov772x_priv	*priv;
 	int			ret;
 	static const struct regmap_config ov772x_regmap_config = {
@@ -1415,6 +1419,26 @@ 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;
+	}
+
+	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);
+	if (ret) {
+		/* For backward compatibility with the existing DT where
+		 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
+		 */
+		priv->bus_type = V4L2_MBUS_PARALLEL;
+		dev_notice(&client->dev, "Falling back to V4L2_MBUS_PARALLEL mode\n");
+	}
+
 	ret = ov772x_video_probe(priv);
 	if (ret < 0)
 		goto error_gpio_put;
-- 
2.17.1


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

* [PATCH v4 2/3] media: i2c: ov772x: Add support for BT656 mode
  2020-09-13 18:42 [PATCH v4 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
  2020-09-13 18:42 ` [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
@ 2020-09-13 18:42 ` Lad Prabhakar
  2020-09-14  7:55   ` Sergei Shtylyov
  2020-09-13 18:42 ` [PATCH v4 3/3] media: i2c: ov772x: Add test pattern control Lad Prabhakar
  2 siblings, 1 reply; 8+ messages in thread
From: Lad Prabhakar @ 2020-09-13 18: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 BT656 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 | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
index 551082aa7026..edd7c4c22225 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)
@@ -1427,16 +1434,24 @@ static int ov772x_probe(struct i2c_client *client)
 		goto error_clk_put;
 	}
 
-	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;
+	bus_cfg = (struct v4l2_fwnode_endpoint)
+		  { .bus_type = V4L2_MBUS_BT656 };
 	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
 	priv->bus_type = bus_cfg.bus_type;
 	v4l2_fwnode_endpoint_free(&bus_cfg);
 	if (ret) {
-		/* For backward compatibility with the existing DT where
-		 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
-		 */
-		priv->bus_type = V4L2_MBUS_PARALLEL;
-		dev_notice(&client->dev, "Falling back to V4L2_MBUS_PARALLEL mode\n");
+		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);
+		if (ret) {
+			/* For backward compatibility with the existing DT where
+			 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
+			 */
+			priv->bus_type = V4L2_MBUS_PARALLEL;
+			dev_notice(&client->dev,
+				   "Falling back to V4L2_MBUS_PARALLEL mode\n");
+		}
 	}
 
 	ret = ov772x_video_probe(priv);
-- 
2.17.1


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

* [PATCH v4 3/3] media: i2c: ov772x: Add test pattern control
  2020-09-13 18:42 [PATCH v4 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
  2020-09-13 18:42 ` [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
  2020-09-13 18:42 ` [PATCH v4 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
@ 2020-09-13 18:42 ` Lad Prabhakar
  2 siblings, 0 replies; 8+ messages in thread
From: Lad Prabhakar @ 2020-09-13 18: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 edd7c4c22225..2eeb11eae5f8 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] 8+ messages in thread

* Re: [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-13 18:42 ` [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
@ 2020-09-14  6:54   ` Sakari Ailus
  2020-09-14 18:26     ` Lad, Prabhakar
  2020-09-14 14:09   ` Jacopo Mondi
  1 sibling, 1 reply; 8+ messages in thread
From: Sakari Ailus @ 2020-09-14  6:54 UTC (permalink / raw)
  To: Lad Prabhakar
  Cc: Jacopo Mondi, Laurent Pinchart, Mauro Carvalho Chehab,
	linux-media, linux-kernel, linux-renesas-soc, Biju Das,
	Prabhakar

Hi Prabhakar,

Thanks for the patchset.

On Sun, Sep 13, 2020 at 07:42:45PM +0100, Lad Prabhakar wrote:
> Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> to determine bus-type and store it locally in priv data.
> 
> Also for backward compatibility with the existing DT where bus-type
> isnt specified fallback to V4L2_MBUS_PARALLEL.
> 
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov772x.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..551082aa7026 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 = { .bus_type = 0 };
> +	struct fwnode_handle	*ep;
>  	struct ov772x_priv	*priv;
>  	int			ret;
>  	static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1419,26 @@ 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;
> +	}
> +
> +	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);
> +	if (ret) {
> +		/* For backward compatibility with the existing DT where
> +		 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL

"isn't", "fall back", and a period, please.

> +		 */
> +		priv->bus_type = V4L2_MBUS_PARALLEL;
> +		dev_notice(&client->dev, "Falling back to V4L2_MBUS_PARALLEL mode\n");

I'd just use dev_dbg().

> +	}
> +
>  	ret = ov772x_video_probe(priv);
>  	if (ret < 0)
>  		goto error_gpio_put;

-- 
Regards,

Sakari Ailus

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

* Re: [PATCH v4 2/3] media: i2c: ov772x: Add support for BT656 mode
  2020-09-13 18:42 ` [PATCH v4 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
@ 2020-09-14  7:55   ` Sergei Shtylyov
  0 siblings, 0 replies; 8+ messages in thread
From: Sergei Shtylyov @ 2020-09-14  7:55 UTC (permalink / raw)
  To: Lad Prabhakar, Jacopo Mondi, Sakari Ailus, Laurent Pinchart,
	Mauro Carvalho Chehab
  Cc: linux-media, linux-kernel, linux-renesas-soc, Biju Das, Prabhakar

Hello!

On 13.09.2020 21:42, Lad Prabhakar wrote:

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

    Isn't it called BT.656?

> 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 | 27 +++++++++++++++++++++------
>   1 file changed, 21 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 551082aa7026..edd7c4c22225 100644
> --- a/drivers/media/i2c/ov772x.c
> +++ b/drivers/media/i2c/ov772x.c
[...]
> @@ -1427,16 +1434,24 @@ static int ov772x_probe(struct i2c_client *client)
>   		goto error_clk_put;
>   	}
>   
> -	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;
> +	bus_cfg = (struct v4l2_fwnode_endpoint)
> +		  { .bus_type = V4L2_MBUS_BT656 };
>   	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);
>   	priv->bus_type = bus_cfg.bus_type;
>   	v4l2_fwnode_endpoint_free(&bus_cfg);
>   	if (ret) {
> -		/* For backward compatibility with the existing DT where
> -		 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
> -		 */
> -		priv->bus_type = V4L2_MBUS_PARALLEL;
> -		dev_notice(&client->dev, "Falling back to V4L2_MBUS_PARALLEL mode\n");
> +		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);
> +		if (ret) {
> +			/* For backward compatibility with the existing DT where
> +			 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
                                     ^^^^ isn't
    Could be fixed, while at it?

> +			 */
> +			priv->bus_type = V4L2_MBUS_PARALLEL;
> +			dev_notice(&client->dev,
> +				   "Falling back to V4L2_MBUS_PARALLEL mode\n");
> +		}
>   	}
>   
>   	ret = ov772x_video_probe(priv);

MBR, Sergei

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

* Re: [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-13 18:42 ` [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
  2020-09-14  6:54   ` Sakari Ailus
@ 2020-09-14 14:09   ` Jacopo Mondi
  1 sibling, 0 replies; 8+ messages in thread
From: Jacopo Mondi @ 2020-09-14 14:09 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 Sun, Sep 13, 2020 at 07:42:45PM +0100, Lad Prabhakar wrote:
> Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> to determine bus-type and store it locally in priv data.
>
> Also for backward compatibility with the existing DT where bus-type
> isnt specified fallback to V4L2_MBUS_PARALLEL.
>
> Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>  drivers/media/i2c/ov772x.c | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)
>
> diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> index 2cc6a678069a..551082aa7026 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 = { .bus_type = 0 };
> +	struct fwnode_handle	*ep;
>  	struct ov772x_priv	*priv;
>  	int			ret;
>  	static const struct regmap_config ov772x_regmap_config = {
> @@ -1415,6 +1419,26 @@ 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;
> +	}
> +
> +	bus_cfg.bus_type = V4L2_MBUS_PARALLEL;

Could you set the bus type when you declare bus_cfg ? There's no point
in setting it to UNKNOW and re-writing it here

> +	ret = v4l2_fwnode_endpoint_alloc_parse(ep, &bus_cfg);

Should you here fwnode_handle_put(ep) ?

Also, this driver does not support the 'link_frequency' property, as the
device does not support CSI-2. As commented on the previous version,
the 'alloc' version of v4l2_fwnode_endpoint_parse() reserves space for
that property, and requires a manual free like you have here below. So
using the 'alloc' version is not techincally wrong, but I don't think
it's required here.

> +	priv->bus_type = bus_cfg.bus_type;
> +	v4l2_fwnode_endpoint_free(&bus_cfg);
> +	if (ret) {

By reading the code in v4l2-fwnode it seems to me that, if you set
bus_cfg.bus_type:
1) The "bus_type" property is set in the fwnode and doesn't match the
requested one -> return -ENXIO
2) The "bus_type" property is not set in the fwnode: no error code is
returned and vep->bus_type is not changed.

> +		/* For backward compatibility with the existing DT where
> +		 * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
> +		 */

This mean there's no need to fallback here, as at this point we only
support PARALLEL. If fwnode,bus_type is not set you won't faile, if
it's set and doesn't match you should fail as the DT is not correct
(at this point).

1) In this patch: try to parse as MBUS_PARALLEL, fail if -ENXIO,
assume parallel otherwise and continue.

2) When you introduce BT.656 in the next patch things become nasty.
Newer DTS will have bus_type mandatory once my series gets in. Old
ones do not and assume "parallel".
2.a) If you try to parse with "cfg.bus_type = MBUS_BT656" and there's
no 'bus_type' in the fwnode, the 'parallel' flags are simply cleared
at the end of v4l2_fwnode_endpoint_parse_parallel_bus(), but the
bus_type does not get changed. You won't detect the mismatch and
happily assume the bus type is BT.656. In other word, if you set
cfg.bus_type that's always authoritative unless the fwnode property
contradicts it (and if that's the case, it should be documented in
v4l2-fnode, unless it is and I missed it).
2.b) If the fwnode.bus_type is set, you try to parse one bus type
first, if -ENOXIO try the second. If it fails again, then return an
error.

In the next patch, to avoid the case described in 2.a I would:
- Parse the 'bus_type' property in the driver (mentioning it's for
  retrocompatibility).
-- If not present: Assume parallel and parse the fwnode. Enclose that
with a comment about "retrocompatibility" and proceed.
-- If present: Do what suggested in the 2.b case. Try one, then the
other, then eventually fail.

I hope this makes any sense, this part is such a gray area... and as a
note to "self" in the forthcoming "of_graph.yaml", let's make bus_type
mandatory for all endpoints!

Also, I might be mistaken in following the code and if you have tested
all cases and they work as intended please call me out on this :)

Thanks
  j

> +		priv->bus_type = V4L2_MBUS_PARALLEL;
> +		dev_notice(&client->dev, "Falling back to V4L2_MBUS_PARALLEL mode\n");
> +	}
> +
>  	ret = ov772x_video_probe(priv);
>  	if (ret < 0)
>  		goto error_gpio_put;
> --
> 2.17.1
>

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

* Re: [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties
  2020-09-14  6:54   ` Sakari Ailus
@ 2020-09-14 18:26     ` Lad, Prabhakar
  0 siblings, 0 replies; 8+ messages in thread
From: Lad, Prabhakar @ 2020-09-14 18:26 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Lad Prabhakar, Jacopo Mondi, Laurent Pinchart,
	Mauro Carvalho Chehab, linux-media, LKML, Linux-Renesas,
	Biju Das

Hi Sakari,

THank you for the review.

On Mon, Sep 14, 2020 at 7:54 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> Thanks for the patchset.
>
> On Sun, Sep 13, 2020 at 07:42:45PM +0100, Lad Prabhakar wrote:
> > Parse endpoint properties using v4l2_fwnode_endpoint_alloc_parse()
> > to determine bus-type and store it locally in priv data.
> >
> > Also for backward compatibility with the existing DT where bus-type
> > isnt specified fallback to V4L2_MBUS_PARALLEL.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> > ---
> >  drivers/media/i2c/ov772x.c | 24 ++++++++++++++++++++++++
> >  1 file changed, 24 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov772x.c b/drivers/media/i2c/ov772x.c
> > index 2cc6a678069a..551082aa7026 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 = { .bus_type = 0 };
> > +     struct fwnode_handle    *ep;
> >       struct ov772x_priv      *priv;
> >       int                     ret;
> >       static const struct regmap_config ov772x_regmap_config = {
> > @@ -1415,6 +1419,26 @@ 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;
> > +     }
> > +
> > +     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);
> > +     if (ret) {
> > +             /* For backward compatibility with the existing DT where
> > +              * bus-type isnt specified fallback to V4L2_MBUS_PARALLEL
>
> "isn't", "fall back", and a period, please.
>
Will fix that.

> > +              */
> > +             priv->bus_type = V4L2_MBUS_PARALLEL;
> > +             dev_notice(&client->dev, "Falling back to V4L2_MBUS_PARALLEL mode\n");
>
> I'd just use dev_dbg().
>
OK will switch to dev_dbg()

Cheers,
Prabhakar

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

end of thread, other threads:[~2020-09-14 18:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-13 18:42 [PATCH v4 0/3] media: i2c: ov772x: Enable BT656 mode and test pattern support Lad Prabhakar
2020-09-13 18:42 ` [PATCH v4 1/3] media: i2c: ov772x: Parse endpoint properties Lad Prabhakar
2020-09-14  6:54   ` Sakari Ailus
2020-09-14 18:26     ` Lad, Prabhakar
2020-09-14 14:09   ` Jacopo Mondi
2020-09-13 18:42 ` [PATCH v4 2/3] media: i2c: ov772x: Add support for BT656 mode Lad Prabhakar
2020-09-14  7:55   ` Sergei Shtylyov
2020-09-13 18:42 ` [PATCH v4 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).