Hi Hugues,
Thank you for catching the polarity bug.
On Mon, Oct 12, 2020 at 3:55 PM Hugues Fruchet <hugues.fruchet@xxxxxx> wrote:
>
> Fix PCLK polarity not being taken into account.
> Add comments about BT656 register control.
> Remove useless ov5640_set_stream_bt656() function.
> Refine comments about MIPI IO register control.
>
> Fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode")
> Signed-off-by: Hugues Fruchet <hugues.fruchet@xxxxxx>
> ---
> drivers/media/i2c/ov5640.c | 77 +++++++++++++++++++++++++++-------------------
> 1 file changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 8d0254d..c0ebf4c 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -1216,20 +1216,6 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
> BIT(1), on ? 0 : BIT(1));
> }
>
> -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 ?
> - OV5640_REG_SYS_CTRL0_SW_PWUP :
> - OV5640_REG_SYS_CTRL0_SW_PWDN);
> -}
> -
> static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
> {
> return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> @@ -1994,6 +1980,7 @@ static int ov5640_set_power_mipi(struct ov5640_dev *sensor, bool on)
> static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
> {
> unsigned int flags = sensor->ep.bus.parallel.flags;
> + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656;
> u8 pclk_pol = 0;
> u8 hsync_pol = 0;
> u8 vsync_pol = 0;
> @@ -2001,6 +1988,7 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
>
> if (!on) {
> /* Reset settings to their default values. */
> + ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00, 0x00);
> ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x58);
> ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, 0x20);
> ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01, 0x00);
> @@ -2024,23 +2012,51 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
> * - VSYNC: active high
> * - HREF: active low
> * - PCLK: active low
> + *
> + * VSYNC & HREF are not configured if BT656 bus mode is selected
> */
> +
> /*
> - * configure parallel port control lines polarity
> + * BT656 embedded synchronization configuration
> *
> - * 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...)
> + * CCIR656 CTRL00
> + * - [7]: SYNC code selection (0: auto generate sync code,
> + * 1: sync code from regs 0x4732-0x4735)
> + * - [6]: f value in CCIR656 SYNC code when fixed f value
> + * - [5]: Fixed f value
> + * - [4:3]: Blank toggle data options (00: data=1'h040/1'h200,
> + * 01: data from regs 0x4736-0x4738, 10: always keep 0)
> + * - [1]: Clip data disable
> + * - [0]: CCIR656 mode enable
> + *
> + * Default CCIR656 SAV/EAV mode with default codes
> + * SAV=0xff000080 & EAV=0xff00009d is enabled here with settings:
> + * - CCIR656 mode enable
> + * - auto generation of sync codes
> + * - blank toggle data 1'h040/1'h200
> + * - clip reserved data (0x00 & 0xff changed to 0x01 & 0xfe)
> */
> - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) {
> + ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00,
> + bt656 ? 0x01 : 0x00);
Did you test bt656 on your platform ? with these changes BT.656 mode
doesn't work anymore on my platform. With the below diff on top of
your patch it works OK. Could you please test the same works on your
platform.
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 49e73ace8685..c5e45bc17bdf 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -1977,6 +1977,40 @@ static int ov5640_set_power_mipi(struct
ov5640_dev *sensor, bool on)
return 0;
}
+static int ov5640_set_stream_bt656(struct ov5640_dev *sensor, bool on)
+{
+ int ret;
+
+ /*
+ * BT656 embedded synchronization configuration
+ *
+ * CCIR656 CTRL00
+ * - [7]: SYNC code selection (0: auto generate sync code,
+ * 1: sync code from regs 0x4732-0x4735)
+ * - [6]: f value in CCIR656 SYNC code when fixed f value
+ * - [5]: Fixed f value
+ * - [4:3]: Blank toggle data options (00: data=1'h040/1'h200,
+ * 01: data from regs 0x4736-0x4738, 10: always keep 0)
+ * - [1]: Clip data disable
+ * - [0]: CCIR656 mode enable
+ *
+ * Default CCIR656 SAV/EAV mode with default codes
+ * SAV=0xff000080 & EAV=0xff00009d is enabled here with settings:
+ * - CCIR656 mode enable
+ * - auto generation of sync codes
+ * - blank toggle data 1'h040/1'h200
+ * - clip reserved data (0x00 & 0xff changed to 0x01 & 0xfe)
+ */
+ 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 ?
+ OV5640_REG_SYS_CTRL0_SW_PWUP :
+ OV5640_REG_SYS_CTRL0_SW_PWDN);
+}
+
static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
{
unsigned int flags = sensor->ep.bus.parallel.flags;
@@ -2014,31 +2048,6 @@ static int ov5640_set_power_dvp(struct
ov5640_dev *sensor, bool on)
* VSYNC & HREF are not configured if BT656 bus mode is selected
*/
- /*
- * BT656 embedded synchronization configuration
- *
- * CCIR656 CTRL00
- * - [7]: SYNC code selection (0: auto generate sync code,
- * 1: sync code from regs 0x4732-0x4735)
- * - [6]: f value in CCIR656 SYNC code when fixed f value
- * - [5]: Fixed f value
- * - [4:3]: Blank toggle data options (00: data=1'h040/1'h200,
- * 01: data from regs 0x4736-0x4738, 10: always keep 0)
- * - [1]: Clip data disable
- * - [0]: CCIR656 mode enable
- *
- * Default CCIR656 SAV/EAV mode with default codes
- * SAV=0xff000080 & EAV=0xff00009d is enabled here with settings:
- * - CCIR656 mode enable
- * - auto generation of sync codes
- * - blank toggle data 1'h040/1'h200
- * - clip reserved data (0x00 & 0xff changed to 0x01 & 0xfe)
- */
- ret = ov5640_write_reg(sensor, OV5640_REG_CCIR656_CTRL00,
- bt656 ? 0x01 : 0x00);
- if (ret)
- return ret;
-
/*
* configure parallel port control lines polarity
*
@@ -2935,6 +2944,8 @@ 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 if (sensor->ep.bus_type == V4L2_MBUS_BT656)
+ ret = ov5640_set_stream_bt656(sensor, enable);
else
ret = ov5640_set_stream_dvp(sensor, enable);
Cheers,
Prabhakar
> + if (ret)
> + return ret;
> +
> + 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)
> + if (!bt656 && flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH)
> hsync_pol = 1;
> - if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> + if (!bt656 && flags & V4L2_MBUS_VSYNC_ACTIVE_LOW)
> vsync_pol = 1;
>
> ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00,
> @@ -2052,12 +2068,12 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
> }
>
> /*
> - * powerdown MIPI TX/RX PHY & disable MIPI
> + * powerdown MIPI TX/RX PHY & enable DVP
> *
> * MIPI CONTROL 00
> - * 4: PWDN PHY TX
> - * 3: PWDN PHY RX
> - * 2: MIPI enable
> + * [4] = 1 : Power down MIPI HS Tx
> + * [3] = 1 : Power down MIPI LS Rx
> + * [2] = 0 : DVP enable (MIPI disable)
> */
> ret = ov5640_write_reg(sensor, OV5640_REG_IO_MIPI_CTRL00, 0x18);
> if (ret)
> @@ -2074,8 +2090,7 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on)
> * - [3:0]: D[9:6] output enable
> */
> ret = ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE01,
> - sensor->ep.bus_type == V4L2_MBUS_PARALLEL ?
> - 0x7f : 0x1f);
> + bt656 ? 0x1f : 0x7f);
> if (ret)
> return ret;
>
> @@ -2925,8 +2940,6 @@ 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 if (sensor->ep.bus_type == V4L2_MBUS_BT656)
> - ret = ov5640_set_stream_bt656(sensor, enable);
> else
> ret = ov5640_set_stream_dvp(sensor, enable);
>
> --
> 2.7.4
>
