Message ID | 1602579743-10286-1-git-send-email-hugues.fruchet@st.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | [v3] media: ov5640: fix support of BT656 bus mode | expand |
Hi Hugues, On Tue, Oct 13, 2020 at 11:02:23AM +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@st.com> > --- > version 3: > - reformat code as per Jacopo's comments > version 2: > - keep reset to default without error check at power off > > drivers/media/i2c/ov5640.c | 82 +++++++++++++++++++++++++--------------------- > 1 file changed, 45 insertions(+), 37 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 8d0254d..8f0812e 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,13 +1980,13 @@ 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; > - u8 pclk_pol = 0; > - u8 hsync_pol = 0; > - u8 vsync_pol = 0; > + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656; > + u8 polarities = 0; > int ret; > > 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,7 +2010,35 @@ 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 > */ > + > + /* > + * 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 > * > @@ -2035,29 +2049,26 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) > * datasheet and hardware, 0 is active high > * and 1 is active low...) > */ > - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) { > - if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > - pclk_pol = 1; > + if (!bt656) { > if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > - hsync_pol = 1; > + polarities |= BIT(1); > if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > - vsync_pol = 1; Ups, this doesn't match what's reported in the manual version I have (version 2.03, page 134) I read: VSYNC polarity 0: Active low 1: Active high Was this a bug already present in the code ? Anyway, this has not been introduced by this patch, but might be a good occasion to fix it. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > - > - ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, > - (pclk_pol << 5) | (hsync_pol << 1) | > - vsync_pol); > - > - if (ret) > - return ret; > + 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; > > /* > - * 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 +2085,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 +2935,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 >
Hi Jacopo, On 10/13/20 2:01 PM, Jacopo Mondi wrote: > Hi Hugues, > > On Tue, Oct 13, 2020 at 11:02:23AM +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@st.com> >> --- >> version 3: >> - reformat code as per Jacopo's comments >> version 2: >> - keep reset to default without error check at power off >> >> drivers/media/i2c/ov5640.c | 82 +++++++++++++++++++++++++--------------------- >> 1 file changed, 45 insertions(+), 37 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 8d0254d..8f0812e 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,13 +1980,13 @@ 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; >> - u8 pclk_pol = 0; >> - u8 hsync_pol = 0; >> - u8 vsync_pol = 0; >> + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656; >> + u8 polarities = 0; >> int ret; >> >> 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,7 +2010,35 @@ 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 >> */ >> + >> + /* >> + * 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 >> * >> @@ -2035,29 +2049,26 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) >> * datasheet and hardware, 0 is active high >> * and 1 is active low...) >> */ >> - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) { >> - if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING) >> - pclk_pol = 1; >> + if (!bt656) { >> if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) >> - hsync_pol = 1; >> + polarities |= BIT(1); >> if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) >> - vsync_pol = 1; > > Ups, this doesn't match what's reported in the manual version I have > (version 2.03, page 134) I read: > > VSYNC polarity 0: Active low > 1: Active high > > Was this a bug already present in the code ? > > Anyway, this has not been introduced by this patch, but might be a > good occasion to fix it. Code is good, this is a "manual bug" that was documented by me when submitting DVP support few lines above ;): * - [0]: VSYNC polarity (mismatch here between * datasheet and hardware, 0 is active high * and 1 is active low...) > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > >> - >> - ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, >> - (pclk_pol << 5) | (hsync_pol << 1) | >> - vsync_pol); >> - >> - if (ret) >> - return ret; >> + 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; >> >> /* >> - * 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 +2085,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 +2935,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.
Hi Hugues On Tue, Oct 13, 2020 at 12:44:10PM +0000, Hugues FRUCHET wrote: > Hi Jacopo, > > On 10/13/20 2:01 PM, Jacopo Mondi wrote: > > Hi Hugues, > > > > On Tue, Oct 13, 2020 at 11:02:23AM +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@st.com> > >> --- > >> version 3: > >> - reformat code as per Jacopo's comments > >> version 2: > >> - keep reset to default without error check at power off > >> > >> drivers/media/i2c/ov5640.c | 82 +++++++++++++++++++++++++--------------------- > >> 1 file changed, 45 insertions(+), 37 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >> index 8d0254d..8f0812e 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,13 +1980,13 @@ 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; > >> - u8 pclk_pol = 0; > >> - u8 hsync_pol = 0; > >> - u8 vsync_pol = 0; > >> + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656; > >> + u8 polarities = 0; > >> int ret; > >> > >> 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,7 +2010,35 @@ 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 > >> */ > >> + > >> + /* > >> + * 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 > >> * > >> @@ -2035,29 +2049,26 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) > >> * datasheet and hardware, 0 is active high > >> * and 1 is active low...) > >> */ > >> - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) { > >> - if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > >> - pclk_pol = 1; > >> + if (!bt656) { > >> if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > >> - hsync_pol = 1; > >> + polarities |= BIT(1); > >> if (flags & V4L2_MBUS_VSYNC_ACTIVE_LOW) > >> - vsync_pol = 1; > > > > Ups, this doesn't match what's reported in the manual version I have > > (version 2.03, page 134) I read: > > > > VSYNC polarity 0: Active low > > 1: Active high > > > > Was this a bug already present in the code ? > > > Anyway, this has not been introduced by this patch, but might be a > > good occasion to fix it. > > Code is good, this is a "manual bug" that was documented by me when > submitting DVP support few lines above ;): > * - [0]: VSYNC polarity (mismatch here between > * datasheet and hardware, 0 is active high > * and 1 is active low...) Ups, missed that as it was not in the patch :D Thanks for the clarification! > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > Thanks > > j > > > >> - > >> - ret = ov5640_write_reg(sensor, OV5640_REG_POLARITY_CTRL00, > >> - (pclk_pol << 5) | (hsync_pol << 1) | > >> - vsync_pol); > >> - > >> - if (ret) > >> - return ret; > >> + 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; > >> > >> /* > >> - * 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 +2085,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 +2935,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.
Hi Hugues, Thank you for the patch. On Tue, Oct 13, 2020 at 1:01 PM Hugues Fruchet <hugues.fruchet@st.com> 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@st.com> > --- > version 3: > - reformat code as per Jacopo's comments > version 2: > - keep reset to default without error check at power off > > drivers/media/i2c/ov5640.c | 82 +++++++++++++++++++++++++--------------------- > 1 file changed, 45 insertions(+), 37 deletions(-) > Tested-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com> Cheers, Prabhakar > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 8d0254d..8f0812e 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,13 +1980,13 @@ 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; > - u8 pclk_pol = 0; > - u8 hsync_pol = 0; > - u8 vsync_pol = 0; > + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656; > + u8 polarities = 0; > int ret; > > 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,7 +2010,35 @@ 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 > */ > + > + /* > + * 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 > * > @@ -2035,29 +2049,26 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) > * datasheet and hardware, 0 is active high > * and 1 is active low...) > */ > - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) { > - if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING) > - pclk_pol = 1; > + if (!bt656) { > if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) > - hsync_pol = 1; > + polarities |= BIT(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; > + 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; > > /* > - * 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 +2085,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 +2935,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 >
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 8d0254d..8f0812e 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,13 +1980,13 @@ 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; - u8 pclk_pol = 0; - u8 hsync_pol = 0; - u8 vsync_pol = 0; + bool bt656 = sensor->ep.bus_type == V4L2_MBUS_BT656; + u8 polarities = 0; int ret; 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,7 +2010,35 @@ 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 */ + + /* + * 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 * @@ -2035,29 +2049,26 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) * datasheet and hardware, 0 is active high * and 1 is active low...) */ - if (sensor->ep.bus_type == V4L2_MBUS_PARALLEL) { - if (flags & V4L2_MBUS_PCLK_SAMPLE_RISING) - pclk_pol = 1; + if (!bt656) { if (flags & V4L2_MBUS_HSYNC_ACTIVE_HIGH) - hsync_pol = 1; + polarities |= BIT(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; + 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; /* - * 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 +2085,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 +2935,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);
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@st.com> --- version 3: - reformat code as per Jacopo's comments version 2: - keep reset to default without error check at power off drivers/media/i2c/ov5640.c | 82 +++++++++++++++++++++++++--------------------- 1 file changed, 45 insertions(+), 37 deletions(-)