[PATCH 0/2] media: ov5640 feature enhancement and fixes

[PATCH 0/2] media: ov5640 feature enhancement and fixes

[Lad Prabhakar]

Hi All,

This patch series fixes DVP support and enables BT656 mode in
the driver.

Cheers,
Prabhakar

Lad Prabhakar (2):
  media: i2c: ov5640: Enable data pins on startup for DVP mode
  media: i2c: ov5640: Add support for BT656 mode

 drivers/media/i2c/ov5640.c | 263 +++++++++++++++++++++++----------------------
 1 file changed, 134 insertions(+), 129 deletions(-)

-- 
2.7.4

[PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Lad Prabhakar]

During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
capture worked only for first run and for subsequent runs it failed.

This patch does the following in DVP mode:
1: Enables data lines on power up
2: Configures HVP lines on power up instead of configuring everytime on
   stream ON/OFF
3: Disables MIPI interface.
4: Puts the sensor in power down mode during stream OFF.

Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
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/ov5640.c | 253 +++++++++++++++++++++------------------------
 1 file changed, 120 insertions(+), 133 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 2fe4a7a..ac305a5 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
 /* YUV422 UYVY VGA@30fps */
 static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
 	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
-	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
+	{0x3103, 0x03, 0, 0},
 	{0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
@@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
 
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
-	int ret;
-	unsigned int flags = sensor->ep.bus.parallel.flags;
-	u8 pclk_pol = 0;
-	u8 hsync_pol = 0;
-	u8 vsync_pol = 0;
-
-	/*
-	 * Note about parallel port configuration.
-	 *
-	 * When configured in parallel mode, the OV5640 will
-	 * output 10 bits data on DVP data lines [9:0].
-	 * If only 8 bits data are wanted, the 8 bits data lines
-	 * of the camera interface must be physically connected
-	 * on the DVP data lines [9:2].
-	 *
-	 * Control lines polarity can be configured through
-	 * devicetree endpoint control lines properties.
-	 * If no endpoint control lines properties are set,
-	 * polarity will be as below:
-	 * - VSYNC:	active high
-	 * - HREF:	active low
-	 * - PCLK:	active low
-	 */
-
-	if (on) {
-		/*
-		 * configure parallel port control lines polarity
-		 *
-		 * POLARITY CTRL0
-		 * - [5]:	PCLK polarity (0: active low, 1: active high)
-		 * - [1]:	HREF polarity (0: active low, 1: active high)
-		 * - [0]:	VSYNC polarity (mismatch here between
-		 *		datasheet and hardware, 0 is active high
-		 *		and 1 is active low...)
-		 */
-		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-			pclk_pol = 1;
-		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-			hsync_pol = 1;
-		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-			vsync_pol = 1;
-
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_POLARITY_CTRL00,
-				       (pclk_pol << 5) |
-				       (hsync_pol << 1) |
-				       vsync_pol);
-
-		if (ret)
-			return ret;
-	}
-
-	/*
-	 * powerdown MIPI TX/RX PHY & disable MIPI
-	 *
-	 * MIPI CONTROL 00
-	 * 4:	 PWDN PHY TX
-	 * 3:	 PWDN PHY RX
-	 * 2:	 MIPI enable
-	 */
-	ret = ov5640_write_reg(sensor,
-			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * enable VSYNC/HREF/PCLK DVP control lines
-	 * & D[9:6] DVP data lines
-	 *
-	 * PAD OUTPUT ENABLE 01
-	 * - 6:		VSYNC output enable
-	 * - 5:		HREF output enable
-	 * - 4:		PCLK output enable
-	 * - [3:0]:	D[9:6] output enable
-	 */
-	ret = ov5640_write_reg(sensor,
-			       OV5640_REG_PAD_OUTPUT_ENABLE01,
-			       on ? 0x7f : 0);
-	if (ret)
-		return ret;
-
-	/*
-	 * enable D[5:0] DVP data lines
-	 *
-	 * PAD OUTPUT ENABLE 02
-	 * - [7:2]:	D[5:0] output enable
-	 */
-	return ov5640_write_reg(sensor,
-				OV5640_REG_PAD_OUTPUT_ENABLE02,
-				on ? 0xfc : 0);
+	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
 }
 
 static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
@@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
 
 static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 {
+	unsigned int flags = sensor->ep.bus.parallel.flags;
+	u8 pclk_pol = 0;
+	u8 hsync_pol = 0;
+	u8 vsync_pol = 0;
 	int ret = 0;
 
 	if (on) {
@@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 		if (ret)
 			goto power_off;
 
-		/* We're done here for DVP bus, while CSI-2 needs setup. */
-		if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
-			return 0;
+		/* CSI-2 setup. */
+		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
+			/*
+			 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
+			 *
+			 * 0x300e = 0x40
+			 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
+			 *		  "ov5640_set_stream_mipi()")
+			 * [4] = 0	: Power up MIPI HS Tx
+			 * [3] = 0	: Power up MIPI LS Rx
+			 * [2] = 0	: MIPI interface disabled
+			 */
+			ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
+			if (ret)
+				goto power_off;
 
-		/*
-		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
-		 *
-		 * 0x300e = 0x40
-		 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
-		 *		  "ov5640_set_stream_mipi()")
-		 * [4] = 0	: Power up MIPI HS Tx
-		 * [3] = 0	: Power up MIPI LS Rx
-		 * [2] = 0	: MIPI interface disabled
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
-		if (ret)
-			goto power_off;
+			/*
+			 * Gate clock and set LP11 in 'no packets mode' (idle)
+			 *
+			 * 0x4800 = 0x24
+			 * [5] = 1	: Gate clock when 'no packets'
+			 * [2] = 1	: MIPI bus in LP11 when 'no packets'
+			 */
+			ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
+			if (ret)
+				goto power_off;
 
-		/*
-		 * Gate clock and set LP11 in 'no packets mode' (idle)
-		 *
-		 * 0x4800 = 0x24
-		 * [5] = 1	: Gate clock when 'no packets'
-		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_MIPI_CTRL00, 0x24);
-		if (ret)
-			goto power_off;
+			/*
+			 * Set data lanes and clock in LP11 when 'sleeping'
+			 *
+			 * 0x3019 = 0x70
+			 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
+			 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
+			 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
+			 */
+			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
+			if (ret)
+				goto power_off;
 
-		/*
-		 * Set data lanes and clock in LP11 when 'sleeping'
-		 *
-		 * 0x3019 = 0x70
-		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
-		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
-		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
-		 */
-		ret = ov5640_write_reg(sensor,
-				       OV5640_REG_PAD_OUTPUT00, 0x70);
-		if (ret)
-			goto power_off;
+			/* Give lanes some time to coax into LP11 state. */
+			usleep_range(500, 1000);
+		} else {
+			/*
+			 * Note about parallel port configuration.
+			 *
+			 * When configured in parallel mode, the OV5640 will
+			 * output 10 bits data on DVP data lines [9:0].
+			 * If only 8 bits data are wanted, the 8 bits data lines
+			 * of the camera interface must be physically connected
+			 * on the DVP data lines [9:2].
+			 *
+			 * Control lines polarity can be configured through
+			 * devicetree endpoint control lines properties.
+			 * If no endpoint control lines properties are set,
+			 * polarity will be as below:
+			 * - VSYNC:	active high
+			 * - HREF:	active low
+			 * - PCLK:	active low
+			 */
+			/*
+			 * configure parallel port control lines polarity
+			 *
+			 * POLARITY CTRL0
+			 * - [5]:	PCLK polarity (0: active low, 1: active high)
+			 * - [1]:	HREF polarity (0: active low, 1: active high)
+			 * - [0]:	VSYNC polarity (mismatch here between
+			 *		datasheet and hardware, 0 is active high
+			 *		and 1 is active low...)
+			 */
+			if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+				pclk_pol = 1;
+			if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+				hsync_pol = 1;
+			if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+				vsync_pol = 1;
+
+			ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
+					       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
+
+			if (ret)
+				goto power_off;
+
+			/*
+			 * powerdown MIPI TX/RX PHY & disable MIPI
+			 *
+			 * MIPI CONTROL 00
+			 * 4:	 PWDN PHY TX
+			 * 3:	 PWDN PHY RX
+			 * 2:	 MIPI enable
+			 */
+			ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
+			if (ret)
+				goto power_off;
+
+			/*
+			 * enable VSYNC/HREF/PCLK DVP control lines
+			 * & D[9:6] DVP data lines
+			 *
+			 * PAD OUTPUT ENABLE 01
+			 * - 6:		VSYNC output enable
+			 * - 5:		HREF output enable
+			 * - 4:		PCLK output enable
+			 * - [3:0]:	D[9:6] output enable
+			 */
+			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+			if (ret)
+				goto power_off;
 
-		/* Give lanes some time to coax into LP11 state. */
-		usleep_range(500, 1000);
+			/*
+			 * enable D[5:0] DVP data lines
+			 *
+			 * PAD OUTPUT ENABLE 02
+			 * - [7:2]:	D[5:0] output enable
+			 */
+			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
+			if (ret)
+				goto power_off;
+		}
 
 	} else {
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
-- 
2.7.4

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Sakari Ailus]

Hi Prabhakar,

Thank you for the patch.

On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> capture worked only for first run and for subsequent runs it failed.
> 
> This patch does the following in DVP mode:
> 1: Enables data lines on power up
> 2: Configures HVP lines on power up instead of configuring everytime on
>    stream ON/OFF
> 3: Disables MIPI interface.
> 4: Puts the sensor in power down mode during stream OFF.

Could you detail a little the underlying problem, the environment where it
can be reproduced and how the patch addresses it, please?

Are you using CSI-2 or the parallel interface, for instance?

> 
> Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> 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/ov5640.c | 253 +++++++++++++++++++++------------------------
>  1 file changed, 120 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2fe4a7a..ac305a5 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>  /* YUV422 UYVY VGA@30fps */
>  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
>  	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> -	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> +	{0x3103, 0x03, 0, 0},
>  	{0x3630, 0x36, 0, 0},
>  	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
>  	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>  
>  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
> -	int ret;
> -	unsigned int flags = sensor->ep.bus.parallel.flags;
> -	u8 pclk_pol = 0;
> -	u8 hsync_pol = 0;
> -	u8 vsync_pol = 0;
> -
> -	/*
> -	 * Note about parallel port configuration.
> -	 *
> -	 * When configured in parallel mode, the OV5640 will
> -	 * output 10 bits data on DVP data lines [9:0].
> -	 * If only 8 bits data are wanted, the 8 bits data lines
> -	 * of the camera interface must be physically connected
> -	 * on the DVP data lines [9:2].
> -	 *
> -	 * Control lines polarity can be configured through
> -	 * devicetree endpoint control lines properties.
> -	 * If no endpoint control lines properties are set,
> -	 * polarity will be as below:
> -	 * - VSYNC:	active high
> -	 * - HREF:	active low
> -	 * - PCLK:	active low
> -	 */
> -
> -	if (on) {
> -		/*
> -		 * configure parallel port control lines polarity
> -		 *
> -		 * POLARITY CTRL0
> -		 * - [5]:	PCLK polarity (0: active low, 1: active high)
> -		 * - [1]:	HREF polarity (0: active low, 1: active high)
> -		 * - [0]:	VSYNC polarity (mismatch here between
> -		 *		datasheet and hardware, 0 is active high
> -		 *		and 1 is active low...)
> -		 */
> -		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -			pclk_pol = 1;
> -		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> -			hsync_pol = 1;
> -		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -			vsync_pol = 1;
> -
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_POLARITY_CTRL00,
> -				       (pclk_pol << 5) |
> -				       (hsync_pol << 1) |
> -				       vsync_pol);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
> -	/*
> -	 * powerdown MIPI TX/RX PHY & disable MIPI
> -	 *
> -	 * MIPI CONTROL 00
> -	 * 4:	 PWDN PHY TX
> -	 * 3:	 PWDN PHY RX
> -	 * 2:	 MIPI enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable VSYNC/HREF/PCLK DVP control lines
> -	 * & D[9:6] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 01
> -	 * - 6:		VSYNC output enable
> -	 * - 5:		HREF output enable
> -	 * - 4:		PCLK output enable
> -	 * - [3:0]:	D[9:6] output enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_PAD_OUTPUT_ENABLE01,
> -			       on ? 0x7f : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable D[5:0] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 02
> -	 * - [7:2]:	D[5:0] output enable
> -	 */
> -	return ov5640_write_reg(sensor,
> -				OV5640_REG_PAD_OUTPUT_ENABLE02,
> -				on ? 0xfc : 0);
> +	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
>  }
>  
>  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
>  
>  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  {
> +	unsigned int flags = sensor->ep.bus.parallel.flags;
> +	u8 pclk_pol = 0;
> +	u8 hsync_pol = 0;
> +	u8 vsync_pol = 0;
>  	int ret = 0;
>  
>  	if (on) {
> @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  		if (ret)
>  			goto power_off;
>  
> -		/* We're done here for DVP bus, while CSI-2 needs setup. */
> -		if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> -			return 0;
> +		/* CSI-2 setup. */
> +		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> +			/*
> +			 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> +			 *
> +			 * 0x300e = 0x40
> +			 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
> +			 *		  "ov5640_set_stream_mipi()")
> +			 * [4] = 0	: Power up MIPI HS Tx
> +			 * [3] = 0	: Power up MIPI LS Rx
> +			 * [2] = 0	: MIPI interface disabled
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);

Please wrap lines over 80 if it can be reasonably one.

> +			if (ret)
> +				goto power_off;
>  
> -		/*
> -		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> -		 *
> -		 * 0x300e = 0x40
> -		 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
> -		 *		  "ov5640_set_stream_mipi()")
> -		 * [4] = 0	: Power up MIPI HS Tx
> -		 * [3] = 0	: Power up MIPI LS Rx
> -		 * [2] = 0	: MIPI interface disabled
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
> -		if (ret)
> -			goto power_off;
> +			/*
> +			 * Gate clock and set LP11 in 'no packets mode' (idle)
> +			 *
> +			 * 0x4800 = 0x24
> +			 * [5] = 1	: Gate clock when 'no packets'
> +			 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> +			if (ret)
> +				goto power_off;
>  
> -		/*
> -		 * Gate clock and set LP11 in 'no packets mode' (idle)
> -		 *
> -		 * 0x4800 = 0x24
> -		 * [5] = 1	: Gate clock when 'no packets'
> -		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_MIPI_CTRL00, 0x24);
> -		if (ret)
> -			goto power_off;
> +			/*
> +			 * Set data lanes and clock in LP11 when 'sleeping'
> +			 *
> +			 * 0x3019 = 0x70
> +			 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
> +			 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> +			 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> +			if (ret)
> +				goto power_off;
>  
> -		/*
> -		 * Set data lanes and clock in LP11 when 'sleeping'
> -		 *
> -		 * 0x3019 = 0x70
> -		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
> -		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> -		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_PAD_OUTPUT00, 0x70);
> -		if (ret)
> -			goto power_off;
> +			/* Give lanes some time to coax into LP11 state. */
> +			usleep_range(500, 1000);
> +		} else {
> +			/*
> +			 * Note about parallel port configuration.
> +			 *
> +			 * When configured in parallel mode, the OV5640 will
> +			 * output 10 bits data on DVP data lines [9:0].
> +			 * If only 8 bits data are wanted, the 8 bits data lines
> +			 * of the camera interface must be physically connected
> +			 * on the DVP data lines [9:2].
> +			 *
> +			 * Control lines polarity can be configured through
> +			 * devicetree endpoint control lines properties.
> +			 * If no endpoint control lines properties are set,
> +			 * polarity will be as below:
> +			 * - VSYNC:	active high
> +			 * - HREF:	active low
> +			 * - PCLK:	active low
> +			 */
> +			/*
> +			 * configure parallel port control lines polarity
> +			 *
> +			 * POLARITY CTRL0
> +			 * - [5]:	PCLK polarity (0: active low, 1: active high)
> +			 * - [1]:	HREF polarity (0: active low, 1: active high)
> +			 * - [0]:	VSYNC polarity (mismatch here between
> +			 *		datasheet and hardware, 0 is active high
> +			 *		and 1 is active low...)
> +			 */
> +			if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +				pclk_pol = 1;
> +			if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +				hsync_pol = 1;
> +			if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +				vsync_pol = 1;
> +
> +			ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> +					       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> +
> +			if (ret)
> +				goto power_off;
> +
> +			/*
> +			 * powerdown MIPI TX/RX PHY & disable MIPI
> +			 *
> +			 * MIPI CONTROL 00
> +			 * 4:	 PWDN PHY TX
> +			 * 3:	 PWDN PHY RX
> +			 * 2:	 MIPI enable
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> +			if (ret)
> +				goto power_off;
> +
> +			/*
> +			 * enable VSYNC/HREF/PCLK DVP control lines
> +			 * & D[9:6] DVP data lines
> +			 *
> +			 * PAD OUTPUT ENABLE 01
> +			 * - 6:		VSYNC output enable
> +			 * - 5:		HREF output enable
> +			 * - 4:		PCLK output enable
> +			 * - [3:0]:	D[9:6] output enable
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> +			if (ret)
> +				goto power_off;
>  
> -		/* Give lanes some time to coax into LP11 state. */
> -		usleep_range(500, 1000);
> +			/*
> +			 * enable D[5:0] DVP data lines
> +			 *
> +			 * PAD OUTPUT ENABLE 02
> +			 * - [7:2]:	D[5:0] output enable
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> +			if (ret)
> +				goto power_off;
> +		}
>  
>  	} else {
>  		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Lad, Prabhakar]

Hi Sakari,

Thank you for the review.

On Fri, Jul 31, 2020 at 2:03 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> > During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> > capture worked only for first run and for subsequent runs it failed.
> >
> > This patch does the following in DVP mode:
> > 1: Enables data lines on power up
> > 2: Configures HVP lines on power up instead of configuring everytime on
> >    stream ON/OFF
> > 3: Disables MIPI interface.
> > 4: Puts the sensor in power down mode during stream OFF.
>
> Could you detail a little the underlying problem, the environment where it
> can be reproduced and how the patch addresses it, please?
>
my bad.

> Are you using CSI-2 or the parallel interface, for instance?
>
while using the sensor in parallel interface mode (DVP) 8-bit mode,
with rcar-vin bridge noticed the capture worked fine for the first run
(with yavta), but for subsequent runs the bridge driver waited for the
frame to be captured. Debugging further noticed the data lines were
enabled/disabled in stream on/off callback. But enabling the data
lines in startup (as done in the patch) fixed this issue.

Without this patch I can confirm the i2c writes were happening in
stream on/off callback and the values were updated in the respective
register to enable/disable datalines. (I didn't find any information
relating to sequence of enabling the data lines in data sheet [1])

[1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf

> >
> > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > 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/ov5640.c | 253 +++++++++++++++++++++------------------------
> >  1 file changed, 120 insertions(+), 133 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > index 2fe4a7a..ac305a5 100644
> > --- a/drivers/media/i2c/ov5640.c
> > +++ b/drivers/media/i2c/ov5640.c
> > @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> >  /* YUV422 UYVY VGA@30fps */
> >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > +     {0x3103, 0x03, 0, 0},
> >       {0x3630, 0x36, 0, 0},
> >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> >
> >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> >  {
> > -     int ret;
> > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > -     u8 pclk_pol = 0;
> > -     u8 hsync_pol = 0;
> > -     u8 vsync_pol = 0;
> > -
> > -     /*
> > -      * Note about parallel port configuration.
> > -      *
> > -      * When configured in parallel mode, the OV5640 will
> > -      * output 10 bits data on DVP data lines [9:0].
> > -      * If only 8 bits data are wanted, the 8 bits data lines
> > -      * of the camera interface must be physically connected
> > -      * on the DVP data lines [9:2].
> > -      *
> > -      * Control lines polarity can be configured through
> > -      * devicetree endpoint control lines properties.
> > -      * If no endpoint control lines properties are set,
> > -      * polarity will be as below:
> > -      * - VSYNC:     active high
> > -      * - HREF:      active low
> > -      * - PCLK:      active low
> > -      */
> > -
> > -     if (on) {
> > -             /*
> > -              * configure parallel port control lines polarity
> > -              *
> > -              * POLARITY CTRL0
> > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > -              * - [0]:       VSYNC polarity (mismatch here between
> > -              *              datasheet and hardware, 0 is active high
> > -              *              and 1 is active low...)
> > -              */
> > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > -                     pclk_pol = 1;
> > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > -                     hsync_pol = 1;
> > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > -                     vsync_pol = 1;
> > -
> > -             ret = ov5640_write_reg(sensor,
> > -                                    OV5640_REG_POLARITY_CTRL00,
> > -                                    (pclk_pol << 5) |
> > -                                    (hsync_pol << 1) |
> > -                                    vsync_pol);
> > -
> > -             if (ret)
> > -                     return ret;
> > -     }
> > -
> > -     /*
> > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > -      *
> > -      * MIPI CONTROL 00
> > -      * 4:    PWDN PHY TX
> > -      * 3:    PWDN PHY RX
> > -      * 2:    MIPI enable
> > -      */
> > -     ret = ov5640_write_reg(sensor,
> > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > -     if (ret)
> > -             return ret;
> > -
> > -     /*
> > -      * enable VSYNC/HREF/PCLK DVP control lines
> > -      * & D[9:6] DVP data lines
> > -      *
> > -      * PAD OUTPUT ENABLE 01
> > -      * - 6:         VSYNC output enable
> > -      * - 5:         HREF output enable
> > -      * - 4:         PCLK output enable
> > -      * - [3:0]:     D[9:6] output enable
> > -      */
> > -     ret = ov5640_write_reg(sensor,
> > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > -                            on ? 0x7f : 0);
> > -     if (ret)
> > -             return ret;
> > -
> > -     /*
> > -      * enable D[5:0] DVP data lines
> > -      *
> > -      * PAD OUTPUT ENABLE 02
> > -      * - [7:2]:     D[5:0] output enable
> > -      */
> > -     return ov5640_write_reg(sensor,
> > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > -                             on ? 0xfc : 0);
> > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
> >  }
> >
> >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> >
> >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> >  {
> > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > +     u8 pclk_pol = 0;
> > +     u8 hsync_pol = 0;
> > +     u8 vsync_pol = 0;
> >       int ret = 0;
> >
> >       if (on) {
> > @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> >               if (ret)
> >                       goto power_off;
> >
> > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > -                     return 0;
> > +             /* CSI-2 setup. */
> > +             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > +                     /*
> > +                      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > +                      *
> > +                      * 0x300e = 0x40
> > +                      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > +                      *                "ov5640_set_stream_mipi()")
> > +                      * [4] = 0      : Power up MIPI HS Tx
> > +                      * [3] = 0      : Power up MIPI LS Rx
> > +                      * [2] = 0      : MIPI interface disabled
> > +                      */
> > +                     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
>
> Please wrap lines over 80 if it can be reasonably one.
>
checkpatch doesn't complain about it  (size is increased to 100)?

Cheers,
Prabhakar

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Sakari Ailus]

Hi Prabhakar,

On Fri, Jul 31, 2020 at 02:18:12PM +0100, Lad, Prabhakar wrote:
> Hi Sakari,
> 
> Thank you for the review.
> 
> On Fri, Jul 31, 2020 at 2:03 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Prabhakar,
> >
> > Thank you for the patch.
> >
> > On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> > > During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> > > capture worked only for first run and for subsequent runs it failed.
> > >
> > > This patch does the following in DVP mode:
> > > 1: Enables data lines on power up
> > > 2: Configures HVP lines on power up instead of configuring everytime on
> > >    stream ON/OFF
> > > 3: Disables MIPI interface.
> > > 4: Puts the sensor in power down mode during stream OFF.

Does this last one only apply to DVP (parallel) mode?

> >
> > Could you detail a little the underlying problem, the environment where it
> > can be reproduced and how the patch addresses it, please?
> >
> my bad.
> 
> > Are you using CSI-2 or the parallel interface, for instance?
> >
> while using the sensor in parallel interface mode (DVP) 8-bit mode,
> with rcar-vin bridge noticed the capture worked fine for the first run
> (with yavta), but for subsequent runs the bridge driver waited for the
> frame to be captured. Debugging further noticed the data lines were
> enabled/disabled in stream on/off callback. But enabling the data
> lines in startup (as done in the patch) fixed this issue.

Could you add a note on this bug to the commit message, please?

> 
> Without this patch I can confirm the i2c writes were happening in
> stream on/off callback and the values were updated in the respective
> register to enable/disable datalines. (I didn't find any information
> relating to sequence of enabling the data lines in data sheet [1])
> 
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> 
> > >
> > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > 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/ov5640.c | 253 +++++++++++++++++++++------------------------
> > >  1 file changed, 120 insertions(+), 133 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 2fe4a7a..ac305a5 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > >  /* YUV422 UYVY VGA@30fps */
> > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > +     {0x3103, 0x03, 0, 0},
> > >       {0x3630, 0x36, 0, 0},
> > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > >
> > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > >  {
> > > -     int ret;
> > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > -     u8 pclk_pol = 0;
> > > -     u8 hsync_pol = 0;
> > > -     u8 vsync_pol = 0;
> > > -
> > > -     /*
> > > -      * Note about parallel port configuration.
> > > -      *
> > > -      * When configured in parallel mode, the OV5640 will
> > > -      * output 10 bits data on DVP data lines [9:0].
> > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > -      * of the camera interface must be physically connected
> > > -      * on the DVP data lines [9:2].
> > > -      *
> > > -      * Control lines polarity can be configured through
> > > -      * devicetree endpoint control lines properties.
> > > -      * If no endpoint control lines properties are set,
> > > -      * polarity will be as below:
> > > -      * - VSYNC:     active high
> > > -      * - HREF:      active low
> > > -      * - PCLK:      active low
> > > -      */
> > > -
> > > -     if (on) {
> > > -             /*
> > > -              * configure parallel port control lines polarity
> > > -              *
> > > -              * POLARITY CTRL0
> > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > -              *              datasheet and hardware, 0 is active high
> > > -              *              and 1 is active low...)
> > > -              */
> > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > -                     pclk_pol = 1;
> > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > -                     hsync_pol = 1;
> > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > -                     vsync_pol = 1;
> > > -
> > > -             ret = ov5640_write_reg(sensor,
> > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > -                                    (pclk_pol << 5) |
> > > -                                    (hsync_pol << 1) |
> > > -                                    vsync_pol);
> > > -
> > > -             if (ret)
> > > -                     return ret;
> > > -     }
> > > -
> > > -     /*
> > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > -      *
> > > -      * MIPI CONTROL 00
> > > -      * 4:    PWDN PHY TX
> > > -      * 3:    PWDN PHY RX
> > > -      * 2:    MIPI enable
> > > -      */
> > > -     ret = ov5640_write_reg(sensor,
> > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     /*
> > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > -      * & D[9:6] DVP data lines
> > > -      *
> > > -      * PAD OUTPUT ENABLE 01
> > > -      * - 6:         VSYNC output enable
> > > -      * - 5:         HREF output enable
> > > -      * - 4:         PCLK output enable
> > > -      * - [3:0]:     D[9:6] output enable
> > > -      */
> > > -     ret = ov5640_write_reg(sensor,
> > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > -                            on ? 0x7f : 0);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     /*
> > > -      * enable D[5:0] DVP data lines
> > > -      *
> > > -      * PAD OUTPUT ENABLE 02
> > > -      * - [7:2]:     D[5:0] output enable
> > > -      */
> > > -     return ov5640_write_reg(sensor,
> > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > -                             on ? 0xfc : 0);
> > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
> > >  }
> > >
> > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > >
> > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > >  {
> > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > +     u8 pclk_pol = 0;
> > > +     u8 hsync_pol = 0;
> > > +     u8 vsync_pol = 0;
> > >       int ret = 0;
> > >
> > >       if (on) {
> > > @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > >               if (ret)
> > >                       goto power_off;
> > >
> > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > -                     return 0;
> > > +             /* CSI-2 setup. */
> > > +             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > +                     /*
> > > +                      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > +                      *
> > > +                      * 0x300e = 0x40
> > > +                      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > +                      *                "ov5640_set_stream_mipi()")
> > > +                      * [4] = 0      : Power up MIPI HS Tx
> > > +                      * [3] = 0      : Power up MIPI LS Rx
> > > +                      * [2] = 0      : MIPI interface disabled
> > > +                      */
> > > +                     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> >
> > Please wrap lines over 80 if it can be reasonably one.
> >
> checkpatch doesn't complain about it  (size is increased to 100)?

That's not a reason leave it as-is.

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Lad, Prabhakar]

Hi Sakari,

On Fri, Jul 31, 2020 at 4:19 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jul 31, 2020 at 02:18:12PM +0100, Lad, Prabhakar wrote:
> > Hi Sakari,
> >
> > Thank you for the review.
> >
> > On Fri, Jul 31, 2020 at 2:03 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Prabhakar,
> > >
> > > Thank you for the patch.
> > >
> > > On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> > > > During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> > > > capture worked only for first run and for subsequent runs it failed.
> > > >
> > > > This patch does the following in DVP mode:
> > > > 1: Enables data lines on power up
> > > > 2: Configures HVP lines on power up instead of configuring everytime on
> > > >    stream ON/OFF
> > > > 3: Disables MIPI interface.
> > > > 4: Puts the sensor in power down mode during stream OFF.
>
> Does this last one only apply to DVP (parallel) mode?
>
Yes only in DVP mode.

> > >
> > > Could you detail a little the underlying problem, the environment where it
> > > can be reproduced and how the patch addresses it, please?
> > >
> > my bad.
> >
> > > Are you using CSI-2 or the parallel interface, for instance?
> > >
> > while using the sensor in parallel interface mode (DVP) 8-bit mode,
> > with rcar-vin bridge noticed the capture worked fine for the first run
> > (with yavta), but for subsequent runs the bridge driver waited for the
> > frame to be captured. Debugging further noticed the data lines were
> > enabled/disabled in stream on/off callback. But enabling the data
> > lines in startup (as done in the patch) fixed this issue.
>
> Could you add a note on this bug to the commit message, please?
>
Sure will do.

> >
> > Without this patch I can confirm the i2c writes were happening in
> > stream on/off callback and the values were updated in the respective
> > register to enable/disable datalines. (I didn't find any information
> > relating to sequence of enabling the data lines in data sheet [1])
> >
> > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> >
> > > >
> > > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > > 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/ov5640.c | 253 +++++++++++++++++++++------------------------
> > > >  1 file changed, 120 insertions(+), 133 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > > index 2fe4a7a..ac305a5 100644
> > > > --- a/drivers/media/i2c/ov5640.c
> > > > +++ b/drivers/media/i2c/ov5640.c
> > > > @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > > >  /* YUV422 UYVY VGA@30fps */
> > > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > > +     {0x3103, 0x03, 0, 0},
> > > >       {0x3630, 0x36, 0, 0},
> > > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > > @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > > >
> > > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > > >  {
> > > > -     int ret;
> > > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > -     u8 pclk_pol = 0;
> > > > -     u8 hsync_pol = 0;
> > > > -     u8 vsync_pol = 0;
> > > > -
> > > > -     /*
> > > > -      * Note about parallel port configuration.
> > > > -      *
> > > > -      * When configured in parallel mode, the OV5640 will
> > > > -      * output 10 bits data on DVP data lines [9:0].
> > > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > > -      * of the camera interface must be physically connected
> > > > -      * on the DVP data lines [9:2].
> > > > -      *
> > > > -      * Control lines polarity can be configured through
> > > > -      * devicetree endpoint control lines properties.
> > > > -      * If no endpoint control lines properties are set,
> > > > -      * polarity will be as below:
> > > > -      * - VSYNC:     active high
> > > > -      * - HREF:      active low
> > > > -      * - PCLK:      active low
> > > > -      */
> > > > -
> > > > -     if (on) {
> > > > -             /*
> > > > -              * configure parallel port control lines polarity
> > > > -              *
> > > > -              * POLARITY CTRL0
> > > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > > -              *              datasheet and hardware, 0 is active high
> > > > -              *              and 1 is active low...)
> > > > -              */
> > > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > -                     pclk_pol = 1;
> > > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > -                     hsync_pol = 1;
> > > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > -                     vsync_pol = 1;
> > > > -
> > > > -             ret = ov5640_write_reg(sensor,
> > > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > > -                                    (pclk_pol << 5) |
> > > > -                                    (hsync_pol << 1) |
> > > > -                                    vsync_pol);
> > > > -
> > > > -             if (ret)
> > > > -                     return ret;
> > > > -     }
> > > > -
> > > > -     /*
> > > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > -      *
> > > > -      * MIPI CONTROL 00
> > > > -      * 4:    PWDN PHY TX
> > > > -      * 3:    PWDN PHY RX
> > > > -      * 2:    MIPI enable
> > > > -      */
> > > > -     ret = ov5640_write_reg(sensor,
> > > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > > -     if (ret)
> > > > -             return ret;
> > > > -
> > > > -     /*
> > > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > > -      * & D[9:6] DVP data lines
> > > > -      *
> > > > -      * PAD OUTPUT ENABLE 01
> > > > -      * - 6:         VSYNC output enable
> > > > -      * - 5:         HREF output enable
> > > > -      * - 4:         PCLK output enable
> > > > -      * - [3:0]:     D[9:6] output enable
> > > > -      */
> > > > -     ret = ov5640_write_reg(sensor,
> > > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > > -                            on ? 0x7f : 0);
> > > > -     if (ret)
> > > > -             return ret;
> > > > -
> > > > -     /*
> > > > -      * enable D[5:0] DVP data lines
> > > > -      *
> > > > -      * PAD OUTPUT ENABLE 02
> > > > -      * - [7:2]:     D[5:0] output enable
> > > > -      */
> > > > -     return ov5640_write_reg(sensor,
> > > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > > -                             on ? 0xfc : 0);
> > > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
> > > >  }
> > > >
> > > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > > @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > > >
> > > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > >  {
> > > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > +     u8 pclk_pol = 0;
> > > > +     u8 hsync_pol = 0;
> > > > +     u8 vsync_pol = 0;
> > > >       int ret = 0;
> > > >
> > > >       if (on) {
> > > > @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > >               if (ret)
> > > >                       goto power_off;
> > > >
> > > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > -                     return 0;
> > > > +             /* CSI-2 setup. */
> > > > +             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > > +                     /*
> > > > +                      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > +                      *
> > > > +                      * 0x300e = 0x40
> > > > +                      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > +                      *                "ov5640_set_stream_mipi()")
> > > > +                      * [4] = 0      : Power up MIPI HS Tx
> > > > +                      * [3] = 0      : Power up MIPI LS Rx
> > > > +                      * [2] = 0      : MIPI interface disabled
> > > > +                      */
> > > > +                     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > >
> > > Please wrap lines over 80 if it can be reasonably one.
> > >
> > checkpatch doesn't complain about it  (size is increased to 100)?
>
> That's not a reason leave it as-is.
>
OK.

Cheers,
Prabhakar

> --
> Regards,
>
> Sakari Ailus

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Laurent Pinchart]

Hi Prabhakar,

On Fri, Jul 31, 2020 at 02:18:12PM +0100, Lad, Prabhakar wrote:
> On Fri, Jul 31, 2020 at 2:03 PM Sakari Ailus wrote:
> > On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> > > During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> > > capture worked only for first run and for subsequent runs it failed.
> > >
> > > This patch does the following in DVP mode:
> > > 1: Enables data lines on power up
> > > 2: Configures HVP lines on power up instead of configuring everytime on
> > >    stream ON/OFF
> > > 3: Disables MIPI interface.
> > > 4: Puts the sensor in power down mode during stream OFF.
> >
> > Could you detail a little the underlying problem, the environment where it
> > can be reproduced and how the patch addresses it, please?
>
> my bad.
> 
> > Are you using CSI-2 or the parallel interface, for instance?
>
> while using the sensor in parallel interface mode (DVP) 8-bit mode,
> with rcar-vin bridge noticed the capture worked fine for the first run
> (with yavta), but for subsequent runs the bridge driver waited for the
> frame to be captured. Debugging further noticed the data lines were
> enabled/disabled in stream on/off callback. But enabling the data
> lines in startup (as done in the patch) fixed this issue.

This looks like a hack though. We should disable the outputs somewhere,
if not at stream off time, at least in ov5640_set_power(0). Leaving
everything powered on will increase power consumption.

This issue needs to be debugged further to understand what's going on,
and a proper fix needs to then be submitted.

> Without this patch I can confirm the i2c writes were happening in
> stream on/off callback and the values were updated in the respective
> register to enable/disable datalines. (I didn't find any information
> relating to sequence of enabling the data lines in data sheet [1])
> 
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> 
> > >
> > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > 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/ov5640.c | 253 +++++++++++++++++++++------------------------
> > >  1 file changed, 120 insertions(+), 133 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > index 2fe4a7a..ac305a5 100644
> > > --- a/drivers/media/i2c/ov5640.c
> > > +++ b/drivers/media/i2c/ov5640.c
> > > @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > >  /* YUV422 UYVY VGA@30fps */
> > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > +     {0x3103, 0x03, 0, 0},
> > >       {0x3630, 0x36, 0, 0},
> > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > >
> > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > >  {
> > > -     int ret;
> > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > -     u8 pclk_pol = 0;
> > > -     u8 hsync_pol = 0;
> > > -     u8 vsync_pol = 0;
> > > -
> > > -     /*
> > > -      * Note about parallel port configuration.
> > > -      *
> > > -      * When configured in parallel mode, the OV5640 will
> > > -      * output 10 bits data on DVP data lines [9:0].
> > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > -      * of the camera interface must be physically connected
> > > -      * on the DVP data lines [9:2].
> > > -      *
> > > -      * Control lines polarity can be configured through
> > > -      * devicetree endpoint control lines properties.
> > > -      * If no endpoint control lines properties are set,
> > > -      * polarity will be as below:
> > > -      * - VSYNC:     active high
> > > -      * - HREF:      active low
> > > -      * - PCLK:      active low
> > > -      */
> > > -
> > > -     if (on) {
> > > -             /*
> > > -              * configure parallel port control lines polarity
> > > -              *
> > > -              * POLARITY CTRL0
> > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > -              *              datasheet and hardware, 0 is active high
> > > -              *              and 1 is active low...)
> > > -              */
> > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > -                     pclk_pol = 1;
> > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > -                     hsync_pol = 1;
> > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > -                     vsync_pol = 1;
> > > -
> > > -             ret = ov5640_write_reg(sensor,
> > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > -                                    (pclk_pol << 5) |
> > > -                                    (hsync_pol << 1) |
> > > -                                    vsync_pol);
> > > -
> > > -             if (ret)
> > > -                     return ret;
> > > -     }
> > > -
> > > -     /*
> > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > -      *
> > > -      * MIPI CONTROL 00
> > > -      * 4:    PWDN PHY TX
> > > -      * 3:    PWDN PHY RX
> > > -      * 2:    MIPI enable
> > > -      */
> > > -     ret = ov5640_write_reg(sensor,
> > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     /*
> > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > -      * & D[9:6] DVP data lines
> > > -      *
> > > -      * PAD OUTPUT ENABLE 01
> > > -      * - 6:         VSYNC output enable
> > > -      * - 5:         HREF output enable
> > > -      * - 4:         PCLK output enable
> > > -      * - [3:0]:     D[9:6] output enable
> > > -      */
> > > -     ret = ov5640_write_reg(sensor,
> > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > -                            on ? 0x7f : 0);
> > > -     if (ret)
> > > -             return ret;
> > > -
> > > -     /*
> > > -      * enable D[5:0] DVP data lines
> > > -      *
> > > -      * PAD OUTPUT ENABLE 02
> > > -      * - [7:2]:     D[5:0] output enable
> > > -      */
> > > -     return ov5640_write_reg(sensor,
> > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > -                             on ? 0xfc : 0);
> > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
> > >  }
> > >
> > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > >
> > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > >  {
> > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > +     u8 pclk_pol = 0;
> > > +     u8 hsync_pol = 0;
> > > +     u8 vsync_pol = 0;
> > >       int ret = 0;
> > >
> > >       if (on) {
> > > @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > >               if (ret)
> > >                       goto power_off;
> > >
> > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > -                     return 0;
> > > +             /* CSI-2 setup. */
> > > +             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > +                     /*
> > > +                      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > +                      *
> > > +                      * 0x300e = 0x40
> > > +                      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > +                      *                "ov5640_set_stream_mipi()")
> > > +                      * [4] = 0      : Power up MIPI HS Tx
> > > +                      * [3] = 0      : Power up MIPI LS Rx
> > > +                      * [2] = 0      : MIPI interface disabled
> > > +                      */
> > > +                     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> >
> > Please wrap lines over 80 if it can be reasonably one.
>
> checkpatch doesn't complain about it  (size is increased to 100)?

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Lad, Prabhakar]

Hi Laurent

On Fri, Jul 31, 2020 at 4:32 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jul 31, 2020 at 02:18:12PM +0100, Lad, Prabhakar wrote:
> > On Fri, Jul 31, 2020 at 2:03 PM Sakari Ailus wrote:
> > > On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> > > > During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> > > > capture worked only for first run and for subsequent runs it failed.
> > > >
> > > > This patch does the following in DVP mode:
> > > > 1: Enables data lines on power up
> > > > 2: Configures HVP lines on power up instead of configuring everytime on
> > > >    stream ON/OFF
> > > > 3: Disables MIPI interface.
> > > > 4: Puts the sensor in power down mode during stream OFF.
> > >
> > > Could you detail a little the underlying problem, the environment where it
> > > can be reproduced and how the patch addresses it, please?
> >
> > my bad.
> >
> > > Are you using CSI-2 or the parallel interface, for instance?
> >
> > while using the sensor in parallel interface mode (DVP) 8-bit mode,
> > with rcar-vin bridge noticed the capture worked fine for the first run
> > (with yavta), but for subsequent runs the bridge driver waited for the
> > frame to be captured. Debugging further noticed the data lines were
> > enabled/disabled in stream on/off callback. But enabling the data
> > lines in startup (as done in the patch) fixed this issue.
>
> This looks like a hack though. We should disable the outputs somewhere,
> if not at stream off time, at least in ov5640_set_power(0). Leaving
> everything powered on will increase power consumption.
>
In fact this patch enables  the pin on poweron(1) and during stream
OFF the sensor is on power down mode (just for DVP).

> This issue needs to be debugged further to understand what's going on,
> and a proper fix needs to then be submitted.
>
Unfortunately I couldn't find more details on the freely available
data sheet let me know if you have any other thoughts I can give it a
quick try.

Cheers,
Prabhakar

> > Without this patch I can confirm the i2c writes were happening in
> > stream on/off callback and the values were updated in the respective
> > register to enable/disable datalines. (I didn't find any information
> > relating to sequence of enabling the data lines in data sheet [1])
> >
> > [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
> >
> > > >
> > > > Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> > > > 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/ov5640.c | 253 +++++++++++++++++++++------------------------
> > > >  1 file changed, 120 insertions(+), 133 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> > > > index 2fe4a7a..ac305a5 100644
> > > > --- a/drivers/media/i2c/ov5640.c
> > > > +++ b/drivers/media/i2c/ov5640.c
> > > > @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
> > > >  /* YUV422 UYVY VGA@30fps */
> > > >  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
> > > >       {0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> > > > -     {0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> > > > +     {0x3103, 0x03, 0, 0},
> > > >       {0x3630, 0x36, 0, 0},
> > > >       {0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
> > > >       {0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> > > > @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> > > >
> > > >  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> > > >  {
> > > > -     int ret;
> > > > -     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > -     u8 pclk_pol = 0;
> > > > -     u8 hsync_pol = 0;
> > > > -     u8 vsync_pol = 0;
> > > > -
> > > > -     /*
> > > > -      * Note about parallel port configuration.
> > > > -      *
> > > > -      * When configured in parallel mode, the OV5640 will
> > > > -      * output 10 bits data on DVP data lines [9:0].
> > > > -      * If only 8 bits data are wanted, the 8 bits data lines
> > > > -      * of the camera interface must be physically connected
> > > > -      * on the DVP data lines [9:2].
> > > > -      *
> > > > -      * Control lines polarity can be configured through
> > > > -      * devicetree endpoint control lines properties.
> > > > -      * If no endpoint control lines properties are set,
> > > > -      * polarity will be as below:
> > > > -      * - VSYNC:     active high
> > > > -      * - HREF:      active low
> > > > -      * - PCLK:      active low
> > > > -      */
> > > > -
> > > > -     if (on) {
> > > > -             /*
> > > > -              * configure parallel port control lines polarity
> > > > -              *
> > > > -              * POLARITY CTRL0
> > > > -              * - [5]:       PCLK polarity (0: active low, 1: active high)
> > > > -              * - [1]:       HREF polarity (0: active low, 1: active high)
> > > > -              * - [0]:       VSYNC polarity (mismatch here between
> > > > -              *              datasheet and hardware, 0 is active high
> > > > -              *              and 1 is active low...)
> > > > -              */
> > > > -             if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> > > > -                     pclk_pol = 1;
> > > > -             if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> > > > -                     hsync_pol = 1;
> > > > -             if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> > > > -                     vsync_pol = 1;
> > > > -
> > > > -             ret = ov5640_write_reg(sensor,
> > > > -                                    OV5640_REG_POLARITY_CTRL00,
> > > > -                                    (pclk_pol << 5) |
> > > > -                                    (hsync_pol << 1) |
> > > > -                                    vsync_pol);
> > > > -
> > > > -             if (ret)
> > > > -                     return ret;
> > > > -     }
> > > > -
> > > > -     /*
> > > > -      * powerdown MIPI TX/RX PHY & disable MIPI
> > > > -      *
> > > > -      * MIPI CONTROL 00
> > > > -      * 4:    PWDN PHY TX
> > > > -      * 3:    PWDN PHY RX
> > > > -      * 2:    MIPI enable
> > > > -      */
> > > > -     ret = ov5640_write_reg(sensor,
> > > > -                            OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> > > > -     if (ret)
> > > > -             return ret;
> > > > -
> > > > -     /*
> > > > -      * enable VSYNC/HREF/PCLK DVP control lines
> > > > -      * & D[9:6] DVP data lines
> > > > -      *
> > > > -      * PAD OUTPUT ENABLE 01
> > > > -      * - 6:         VSYNC output enable
> > > > -      * - 5:         HREF output enable
> > > > -      * - 4:         PCLK output enable
> > > > -      * - [3:0]:     D[9:6] output enable
> > > > -      */
> > > > -     ret = ov5640_write_reg(sensor,
> > > > -                            OV5640_REG_PAD_OUTPUT_ENABLE01,
> > > > -                            on ? 0x7f : 0);
> > > > -     if (ret)
> > > > -             return ret;
> > > > -
> > > > -     /*
> > > > -      * enable D[5:0] DVP data lines
> > > > -      *
> > > > -      * PAD OUTPUT ENABLE 02
> > > > -      * - [7:2]:     D[5:0] output enable
> > > > -      */
> > > > -     return ov5640_write_reg(sensor,
> > > > -                             OV5640_REG_PAD_OUTPUT_ENABLE02,
> > > > -                             on ? 0xfc : 0);
> > > > +     return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
> > > >  }
> > > >
> > > >  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> > > > @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
> > > >
> > > >  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > >  {
> > > > +     unsigned int flags = sensor->ep.bus.parallel.flags;
> > > > +     u8 pclk_pol = 0;
> > > > +     u8 hsync_pol = 0;
> > > > +     u8 vsync_pol = 0;
> > > >       int ret = 0;
> > > >
> > > >       if (on) {
> > > > @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
> > > >               if (ret)
> > > >                       goto power_off;
> > > >
> > > > -             /* We're done here for DVP bus, while CSI-2 needs setup. */
> > > > -             if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> > > > -                     return 0;
> > > > +             /* CSI-2 setup. */
> > > > +             if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> > > > +                     /*
> > > > +                      * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> > > > +                      *
> > > > +                      * 0x300e = 0x40
> > > > +                      * [7:5] = 010  : 2 data lanes mode (see FIXME note in
> > > > +                      *                "ov5640_set_stream_mipi()")
> > > > +                      * [4] = 0      : Power up MIPI HS Tx
> > > > +                      * [3] = 0      : Power up MIPI LS Rx
> > > > +                      * [2] = 0      : MIPI interface disabled
> > > > +                      */
> > > > +                     ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> > >
> > > Please wrap lines over 80 if it can be reasonably one.
> >
> > checkpatch doesn't complain about it  (size is increased to 100)?
>
> --
> Regards,
>
> Laurent Pinchart

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Laurent Pinchart]

Hi Prabhakar,

Thank you for the patch.

On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
> During testing this sensor on iW-RainboW-G21D-Qseven platform noticed the
> capture worked only for first run and for subsequent runs it failed.
> 
> This patch does the following in DVP mode:
> 1: Enables data lines on power up
> 2: Configures HVP lines on power up instead of configuring everytime on
>    stream ON/OFF
> 3: Disables MIPI interface.
> 4: Puts the sensor in power down mode during stream OFF.
> 
> Fixes: f22996db44e2d ("media: ov5640: add support of DVP parallel interface")
> 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/ov5640.c | 253 +++++++++++++++++++++------------------------
>  1 file changed, 120 insertions(+), 133 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 2fe4a7a..ac305a5 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -274,7 +274,7 @@ static inline struct v4l2_subdev *ctrl_to_sd(struct v4l2_ctrl *ctrl)
>  /* YUV422 UYVY VGA@30fps */
>  static const struct reg_value ov5640_init_setting_30fps_VGA[] = {
>  	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> -	{0x3103, 0x03, 0, 0}, {0x3017, 0x00, 0, 0}, {0x3018, 0x00, 0, 0},
> +	{0x3103, 0x03, 0, 0},
>  	{0x3630, 0x36, 0, 0},
>  	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
>  	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> @@ -1210,96 +1210,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>  
>  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
> -	int ret;
> -	unsigned int flags = sensor->ep.bus.parallel.flags;
> -	u8 pclk_pol = 0;
> -	u8 hsync_pol = 0;
> -	u8 vsync_pol = 0;
> -
> -	/*
> -	 * Note about parallel port configuration.
> -	 *
> -	 * When configured in parallel mode, the OV5640 will
> -	 * output 10 bits data on DVP data lines [9:0].
> -	 * If only 8 bits data are wanted, the 8 bits data lines
> -	 * of the camera interface must be physically connected
> -	 * on the DVP data lines [9:2].
> -	 *
> -	 * Control lines polarity can be configured through
> -	 * devicetree endpoint control lines properties.
> -	 * If no endpoint control lines properties are set,
> -	 * polarity will be as below:
> -	 * - VSYNC:	active high
> -	 * - HREF:	active low
> -	 * - PCLK:	active low
> -	 */
> -
> -	if (on) {
> -		/*
> -		 * configure parallel port control lines polarity
> -		 *
> -		 * POLARITY CTRL0
> -		 * - [5]:	PCLK polarity (0: active low, 1: active high)
> -		 * - [1]:	HREF polarity (0: active low, 1: active high)
> -		 * - [0]:	VSYNC polarity (mismatch here between
> -		 *		datasheet and hardware, 0 is active high
> -		 *		and 1 is active low...)
> -		 */
> -		if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> -			pclk_pol = 1;
> -		if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> -			hsync_pol = 1;
> -		if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> -			vsync_pol = 1;
> -
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_POLARITY_CTRL00,
> -				       (pclk_pol << 5) |
> -				       (hsync_pol << 1) |
> -				       vsync_pol);
> -
> -		if (ret)
> -			return ret;
> -	}
> -
> -	/*
> -	 * powerdown MIPI TX/RX PHY & disable MIPI
> -	 *
> -	 * MIPI CONTROL 00
> -	 * 4:	 PWDN PHY TX
> -	 * 3:	 PWDN PHY RX
> -	 * 2:	 MIPI enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_IO_MIPI_CTRL00, on ? 0x18 : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable VSYNC/HREF/PCLK DVP control lines
> -	 * & D[9:6] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 01
> -	 * - 6:		VSYNC output enable
> -	 * - 5:		HREF output enable
> -	 * - 4:		PCLK output enable
> -	 * - [3:0]:	D[9:6] output enable
> -	 */
> -	ret = ov5640_write_reg(sensor,
> -			       OV5640_REG_PAD_OUTPUT_ENABLE01,
> -			       on ? 0x7f : 0);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * enable D[5:0] DVP data lines
> -	 *
> -	 * PAD OUTPUT ENABLE 02
> -	 * - [7:2]:	D[5:0] output enable
> -	 */
> -	return ov5640_write_reg(sensor,
> -				OV5640_REG_PAD_OUTPUT_ENABLE02,
> -				on ? 0xfc : 0);
> +	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
>  }
>  
>  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> @@ -2003,6 +1914,10 @@ static void ov5640_set_power_off(struct ov5640_dev *sensor)
>  
>  static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  {
> +	unsigned int flags = sensor->ep.bus.parallel.flags;
> +	u8 pclk_pol = 0;
> +	u8 hsync_pol = 0;
> +	u8 vsync_pol = 0;
>  	int ret = 0;
>  
>  	if (on) {
> @@ -2014,52 +1929,124 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
>  		if (ret)
>  			goto power_off;
>  
> -		/* We're done here for DVP bus, while CSI-2 needs setup. */
> -		if (sensor->ep.bus_type != V4L2_MBUS_CSI2_DPHY)
> -			return 0;
> +		/* CSI-2 setup. */
> +		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
> +			/*
> +			 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> +			 *
> +			 * 0x300e = 0x40
> +			 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
> +			 *		  "ov5640_set_stream_mipi()")
> +			 * [4] = 0	: Power up MIPI HS Tx
> +			 * [3] = 0	: Power up MIPI LS Rx
> +			 * [2] = 0	: MIPI interface disabled
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x40);
> +			if (ret)
> +				goto power_off;
>  
> -		/*
> -		 * Power up MIPI HS Tx and LS Rx; 2 data lanes mode
> -		 *
> -		 * 0x300e = 0x40
> -		 * [7:5] = 010	: 2 data lanes mode (see FIXME note in
> -		 *		  "ov5640_set_stream_mipi()")
> -		 * [4] = 0	: Power up MIPI HS Tx
> -		 * [3] = 0	: Power up MIPI LS Rx
> -		 * [2] = 0	: MIPI interface disabled
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_IO_MIPI_CTRL00, 0x40);
> -		if (ret)
> -			goto power_off;
> +			/*
> +			 * Gate clock and set LP11 in 'no packets mode' (idle)
> +			 *
> +			 * 0x4800 = 0x24
> +			 * [5] = 1	: Gate clock when 'no packets'
> +			 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_MIPI_CTRL00, 0x24);
> +			if (ret)
> +				goto power_off;
>  
> -		/*
> -		 * Gate clock and set LP11 in 'no packets mode' (idle)
> -		 *
> -		 * 0x4800 = 0x24
> -		 * [5] = 1	: Gate clock when 'no packets'
> -		 * [2] = 1	: MIPI bus in LP11 when 'no packets'
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_MIPI_CTRL00, 0x24);
> -		if (ret)
> -			goto power_off;
> +			/*
> +			 * Set data lanes and clock in LP11 when 'sleeping'
> +			 *
> +			 * 0x3019 = 0x70
> +			 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
> +			 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> +			 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT00, 0x70);
> +			if (ret)
> +				goto power_off;
>  
> -		/*
> -		 * Set data lanes and clock in LP11 when 'sleeping'
> -		 *
> -		 * 0x3019 = 0x70
> -		 * [6] = 1	: MIPI data lane 2 in LP11 when 'sleeping'
> -		 * [5] = 1	: MIPI data lane 1 in LP11 when 'sleeping'
> -		 * [4] = 1	: MIPI clock lane in LP11 when 'sleeping'
> -		 */
> -		ret = ov5640_write_reg(sensor,
> -				       OV5640_REG_PAD_OUTPUT00, 0x70);
> -		if (ret)
> -			goto power_off;
> +			/* Give lanes some time to coax into LP11 state. */
> +			usleep_range(500, 1000);
> +		} else {
> +			/*
> +			 * Note about parallel port configuration.
> +			 *
> +			 * When configured in parallel mode, the OV5640 will
> +			 * output 10 bits data on DVP data lines [9:0].
> +			 * If only 8 bits data are wanted, the 8 bits data lines
> +			 * of the camera interface must be physically connected
> +			 * on the DVP data lines [9:2].
> +			 *
> +			 * Control lines polarity can be configured through
> +			 * devicetree endpoint control lines properties.
> +			 * If no endpoint control lines properties are set,
> +			 * polarity will be as below:
> +			 * - VSYNC:	active high
> +			 * - HREF:	active low
> +			 * - PCLK:	active low
> +			 */
> +			/*
> +			 * configure parallel port control lines polarity
> +			 *
> +			 * POLARITY CTRL0
> +			 * - [5]:	PCLK polarity (0: active low, 1: active high)
> +			 * - [1]:	HREF polarity (0: active low, 1: active high)
> +			 * - [0]:	VSYNC polarity (mismatch here between
> +			 *		datasheet and hardware, 0 is active high
> +			 *		and 1 is active low...)
> +			 */
> +			if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
> +				pclk_pol = 1;
> +			if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> +				hsync_pol = 1;
> +			if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> +				vsync_pol = 1;
> +
> +			ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> +					       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
> +
> +			if (ret)
> +				goto power_off;
> +
> +			/*
> +			 * powerdown MIPI TX/RX PHY & disable MIPI
> +			 *
> +			 * MIPI CONTROL 00
> +			 * 4:	 PWDN PHY TX
> +			 * 3:	 PWDN PHY RX
> +			 * 2:	 MIPI enable
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> +			if (ret)
> +				goto power_off;
> +
> +			/*
> +			 * enable VSYNC/HREF/PCLK DVP control lines
> +			 * & D[9:6] DVP data lines
> +			 *
> +			 * PAD OUTPUT ENABLE 01
> +			 * - 6:		VSYNC output enable
> +			 * - 5:		HREF output enable
> +			 * - 4:		PCLK output enable
> +			 * - [3:0]:	D[9:6] output enable
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> +			if (ret)
> +				goto power_off;
>  
> -		/* Give lanes some time to coax into LP11 state. */
> -		usleep_range(500, 1000);
> +			/*
> +			 * enable D[5:0] DVP data lines
> +			 *
> +			 * PAD OUTPUT ENABLE 02
> +			 * - [7:2]:	D[5:0] output enable
> +			 */
> +			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> +			if (ret)
> +				goto power_off;
> +		}

I'd split this to two separate functions, one for CSI-2, one for
parallel, as this is getting difficult to read.

>  
>  	} else {
>  		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/2] media: i2c: ov5640: Enable data pins on startup for DVP mode

[Lad, Prabhakar]

Hi Laurent,

Thank you for the review.

On Fri, Jul 31, 2020 at 4:27 PM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Prabhakar,
>
> Thank you for the patch.
>
> On Fri, Jul 31, 2020 at 10:24:46AM +0100, Lad Prabhakar wrote:
<snip>
> > +                      * - 6:         VSYNC output enable
> > +                      * - 5:         HREF output enable
> > +                      * - 4:         PCLK output enable
> > +                      * - [3:0]:     D[9:6] output enable
> > +                      */
> > +                     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
> > +                     if (ret)
> > +                             goto power_off;
> >
> > -             /* Give lanes some time to coax into LP11 state. */
> > -             usleep_range(500, 1000);
> > +                     /*
> > +                      * enable D[5:0] DVP data lines
> > +                      *
> > +                      * PAD OUTPUT ENABLE 02
> > +                      * - [7:2]:     D[5:0] output enable
> > +                      */
> > +                     ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc);
> > +                     if (ret)
> > +                             goto power_off;
> > +             }
>
> I'd split this to two separate functions, one for CSI-2, one for
> parallel, as this is getting difficult to read.
>
Sure I'll split this up in v2..

Cheers,
Prabhakar

> >
> >       } else {
> >               if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
>
> --
> Regards,
>
> Laurent Pinchart

[PATCH 2/2] media: i2c: ov5640: Add support for BT656 mode

[Lad Prabhakar]

Enable support for BT656 mode.

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/ov5640.c | 48 +++++++++++++++++++++++++++++++---------------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index ac305a5..2b23988 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -82,6 +82,7 @@
 #define OV5640_REG_VFIFO_HSIZE		0x4602
 #define OV5640_REG_VFIFO_VSIZE		0x4604
 #define OV5640_REG_JPG_MODE_SELECT	0x4713
+#define OV5640_REG_CCIR656_CTRL00	0x4730
 #define OV5640_REG_POLARITY_CTRL00	0x4740
 #define OV5640_REG_MIPI_CTRL00		0x4800
 #define OV5640_REG_DEBUG_MODE		0x4814
@@ -1213,6 +1214,17 @@ static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
 }
 
+static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
+{
+	int ret;
+
+	ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00, on ? 0x1 : 0x00);
+	if (ret)
+		return ret;
+
+	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ? 0x2 : 0x42);
+}
+
 static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
 {
 	int ret;
@@ -1998,18 +2010,21 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 			 *		datasheet and hardware, 0 is active high
 			 *		and 1 is active low...)
 			 */
-			if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
-				pclk_pol = 1;
-			if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
-				hsync_pol = 1;
-			if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
-				vsync_pol = 1;
-
-			ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
-					       (pclk_pol << 5) | (hsync_pol << 1) | vsync_pol);
-
-			if (ret)
-				goto power_off;
+			if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
+				if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING)
+					pclk_pol = 1;
+				if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
+					hsync_pol = 1;
+				if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
+					vsync_pol = 1;
+
+				ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
+						       (pclk_pol << 5) | (hsync_pol << 1) |
+						       vsync_pol);
+
+				if (ret)
+					goto power_off;
+			}
 
 			/*
 			 * powerdown MIPI TX/RX PHY & disable MIPI
@@ -2033,7 +2048,9 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 			 * - 4:		PCLK output enable
 			 * - [3:0]:	D[9:6] output enable
 			 */
-			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x7f);
+			ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
+					       sensor->ep.bus_type == V4L2_MBUS_PARALLEL ?
+					       0x7f : 0x1f);
 			if (ret)
 				goto power_off;
 
@@ -2047,7 +2064,6 @@ static int ov5640_set_power(struct ov5640_dev *sensor, bool on)
 			if (ret)
 				goto power_off;
 		}
-
 	} else {
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY) {
 			/* Reset MIPI bus settings to their default values. */
@@ -2875,8 +2891,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
 
 		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
 			ret = ov5640_set_stream_mipi(sensor, enable);
-		else
+		else if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL)
 			ret = ov5640_set_stream_dvp(sensor, enable);
+		else
+			ret = ov5640_set_stream_bt656(sensor, enable);
 
 		if (!ret)
 			sensor->streaming = enable;
-- 
2.7.4

Re: [PATCH 2/2] media: i2c: ov5640: Add support for BT656 mode

[Sakari Ailus]

Hi Prabhakar,

On Fri, Jul 31, 2020 at 10:24:47AM +0100, Lad Prabhakar wrote:
> @@ -2875,8 +2891,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  		if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
>  			ret = ov5640_set_stream_mipi(sensor, enable);
> -		else
> +		else if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL)
>  			ret = ov5640_set_stream_dvp(sensor, enable);
> +		else
> +			ret = ov5640_set_stream_bt656(sensor, enable);

I'd also check for the Bt.656 mode here, rather than assuming it. 

Could you also update the DT bindings, given that the sensor appears to
support Bt.656 as well? I believe this requires the bus-type property, too.

>  
>  		if (!ret)
>  			sensor->streaming = enable;

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/2] media: i2c: ov5640: Add support for BT656 mode

[Lad, Prabhakar]

Hi Sakari,

Thank you for the review.

On Fri, Jul 31, 2020 at 4:23 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Prabhakar,
>
> On Fri, Jul 31, 2020 at 10:24:47AM +0100, Lad Prabhakar wrote:
> > @@ -2875,8 +2891,10 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
> >
> >               if (sensor->ep.bus_type == V4L2_MBUS_CSI2_DPHY)
> >                       ret = ov5640_set_stream_mipi(sensor, enable);
> > -             else
> > +             else if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL)
> >                       ret = ov5640_set_stream_dvp(sensor, enable);
> > +             else
> > +                     ret = ov5640_set_stream_bt656(sensor, enable);
>
> I'd also check for the Bt.656 mode here, rather than assuming it.
>
Sure will do.

> Could you also update the DT bindings, given that the sensor appears to
> support Bt.656 as well? I believe this requires the bus-type property, too.
>
Aha I ignored because this wasnt updated when DVP support was added to
the driver, but will do now.

Cheers,
Prabhakar

> >
> >               if (!ret)
> >                       sensor->streaming = enable;
>
> --
> Kind regards,
>
> Sakari Ailus