Hi Jacopo, On 10/12/20 5:48 PM, Jacopo Mondi wrote: > Hi Hugues, > > On Mon, Oct 12, 2020 at 04:51:43PM +0200, Hugues Fruchet 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); >> + if (ret) >> + return ret; > > All good so far > >> + >> + if (on) { > > But don't you have retained > if (!on) > at the beginning of the function ? > > I would reflow this as: > > static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) { > if (!on) { > ... > } > > uint8_t polarities = 0; > if (!bt656) { > if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > polarities |= BIT(1); > if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > polarities |= BIT(0); > } > if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > polarities |= BIT(5); > > ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, > polarities); > if (ret) > return ret; > > if (bt656) { > write_reg(CCIR656); > } > > .... > > To make it more readable. What do you think ? Fully agree. > >> + /* >> + * 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) >> } >> > > The part here below looks good! > >> /* >> - * 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 >> BR, Hugues.