| Message ID | 1602080692-6383-1-git-send-email-hugues.fruchet@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: fix support of BT656 bus mode | expand |
Hi Hugues, On Wed, Oct 07, 2020 at 04:24:52PM +0200, Hugues Fruchet wrote: > fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode") As per the comment from Sakari in another patch, 'Fixes' (capital 'F') usually comes before Signed-off/Reviewed-by tags > > Fix PCLK polarity not being taken into account. > Fix ov5640_write_reg()return value unchecked at power off. Am I wrong or you broke out this part to a separate patch ? As commented there, I don't think it's a good idea. Are you planning to send a v2 of this patch ? Thanks j > Reformat code to keep register access below the register description. > Remove useless ov5640_set_stream_bt656() function. > > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > --- > drivers/media/i2c/ov5640.c | 95 +++++++++++++++++++++++++--------------------- > 1 file changed, 51 insertions(+), 44 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 8d0254d..1b20db7 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,20 +1980,12 @@ 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; > int ret; > > - if (!on) { > - /* Reset settings to their default values. */ > - 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); > - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00); > - return 0; > - } > - > /* > * Note about parallel port configuration. > * > @@ -2024,27 +2002,57 @@ 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 > + * > + * 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 > * > - * 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...) > + * 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, > + on && bt656 ? 0x01 : 0x00); > + 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, > - (pclk_pol << 5) | (hsync_pol << 1) | > + ret = ov5640_write_reg(sensor, > + OV5640_REG_POLARITY_CTRL00, > + (pclk_pol << 5) | > + (hsync_pol << 1) | > vsync_pol); > > if (ret) > @@ -2052,12 +2060,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 +2082,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); > + on ? (bt656 ? 0x1f : 0x7f) : 0x00); > if (ret) > return ret; > > @@ -2085,7 +2092,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) > * PAD OUTPUT ENABLE 02 > * - [7:2]: D[5:0] output enable > */ > - return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc); > + return ov5640_write_reg(sensor, > + OV5640_REG_PAD_OUTPUT_ENABLE02, > + on ? 0xfc : 0); > } > > static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > @@ -2925,8 +2934,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, Thanks for reviewing, comments below On 10/12/20 3:35 PM, Jacopo Mondi wrote: > Hi Hugues, > > On Wed, Oct 07, 2020 at 04:24:52PM +0200, Hugues Fruchet wrote: >> fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode") > > As per the comment from Sakari in another patch, 'Fixes' (capital 'F') > usually comes before Signed-off/Reviewed-by tags I understand the "F" upper case, but I don't understand the "comes before Signed-off", my signed-off is inserted after this line. > >> >> Fix PCLK polarity not being taken into account. This one fixes a real bug. >> Fix ov5640_write_reg()return value unchecked at power off. > > Am I wrong or you broke out this part to a separate patch ? > As commented there, I don't think it's a good idea. > > Are you planning to send a v2 of this patch ? The patches was sent at same time, so it's normal that they are designed in same way, I'll send a v2 with this "reset to default at power off without check" strategy. > > Thanks > j > >> Reformat code to keep register access below the register description. >> Remove useless ov5640_set_stream_bt656() function. >> >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >> --- >> drivers/media/i2c/ov5640.c | 95 +++++++++++++++++++++++++--------------------- >> 1 file changed, 51 insertions(+), 44 deletions(-) >> >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >> index 8d0254d..1b20db7 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,20 +1980,12 @@ 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; >> int ret; >> >> - if (!on) { >> - /* Reset settings to their default values. */ >> - 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); >> - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00); >> - return 0; >> - } >> - >> /* >> * Note about parallel port configuration. >> * >> @@ -2024,27 +2002,57 @@ 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 >> + * >> + * 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 >> * >> - * 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...) >> + * 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, >> + on && bt656 ? 0x01 : 0x00); >> + 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, >> - (pclk_pol << 5) | (hsync_pol << 1) | >> + ret = ov5640_write_reg(sensor, >> + OV5640_REG_POLARITY_CTRL00, >> + (pclk_pol << 5) | >> + (hsync_pol << 1) | >> vsync_pol); >> >> if (ret) >> @@ -2052,12 +2060,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 +2082,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); >> + on ? (bt656 ? 0x1f : 0x7f) : 0x00); >> if (ret) >> return ret; >> >> @@ -2085,7 +2092,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) >> * PAD OUTPUT ENABLE 02 >> * - [7:2]: D[5:0] output enable >> */ >> - return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc); >> + return ov5640_write_reg(sensor, >> + OV5640_REG_PAD_OUTPUT_ENABLE02, >> + on ? 0xfc : 0); >> } >> >> static int ov5640_set_power(struct ov5640_dev *sensor, bool on) >> @@ -2925,8 +2934,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 Hugues On Mon, Oct 12, 2020 at 02:12:09PM +0000, Hugues FRUCHET wrote: > Hi Jacopo, > > Thanks for reviewing, comments below > > On 10/12/20 3:35 PM, Jacopo Mondi wrote: > > Hi Hugues, > > > > On Wed, Oct 07, 2020 at 04:24:52PM +0200, Hugues Fruchet wrote: > > > > As per the comment from Sakari in another patch, 'Fixes' (capital 'F') > > usually comes before Signed-off/Reviewed-by tags > I understand the "F" upper case, but I don't understand the "comes > before Signed-off", my signed-off is inserted after this line. > So, this is not stated explicitly in submitting-patches.rst, but as per all the other tags attached to a patch, they're usually placed after the commit message in this case: Fix PCLK polarity not being taken into account. Fix ov5640_write_reg()return value unchecked at power off. Reformat code to keep register access below the register description. Remove useless ov5640_set_stream_bt656() function. Fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode") Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > > > >> > >> Fix PCLK polarity not being taken into account. > > This one fixes a real bug. > I don't think I've contested this > >> Fix ov5640_write_reg()return value unchecked at power off. > > > > Am I wrong or you broke out this part to a separate patch ? > > As commented there, I don't think it's a good idea. > > > > Are you planning to send a v2 of this patch ? > The patches was sent at same time, so it's normal that they are designed > in same way, I'll send a v2 with this "reset to default at power off > without check" strategy. > I was wondering if you were planning to send a v2 with the part that changed the power-off path removed as I would have reviewed that one instead of this, that's all. > > > > Thanks > > j > > > >> Reformat code to keep register access below the register description. > >> Remove useless ov5640_set_stream_bt656() function. > >> > >> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > >> --- > >> drivers/media/i2c/ov5640.c | 95 +++++++++++++++++++++++++--------------------- > >> 1 file changed, 51 insertions(+), 44 deletions(-) > >> > >> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > >> index 8d0254d..1b20db7 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,20 +1980,12 @@ 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; > >> int ret; > >> > >> - if (!on) { > >> - /* Reset settings to their default values. */ > >> - 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); > >> - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00); > >> - return 0; > >> - } > >> - > >> /* > >> * Note about parallel port configuration. > >> * > >> @@ -2024,27 +2002,57 @@ 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 > >> + * > >> + * 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 > >> * > >> - * 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...) > >> + * 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, > >> + on && bt656 ? 0x01 : 0x00); > >> + 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, > >> - (pclk_pol << 5) | (hsync_pol << 1) | > >> + ret = ov5640_write_reg(sensor, > >> + OV5640_REG_POLARITY_CTRL00, > >> + (pclk_pol << 5) | > >> + (hsync_pol << 1) | > >> vsync_pol); > >> > >> if (ret) > >> @@ -2052,12 +2060,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 +2082,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); > >> + on ? (bt656 ? 0x1f : 0x7f) : 0x00); > >> if (ret) > >> return ret; > >> > >> @@ -2085,7 +2092,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) > >> * PAD OUTPUT ENABLE 02 > >> * - [7:2]: D[5:0] output enable > >> */ > >> - return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc); > >> + return ov5640_write_reg(sensor, > >> + OV5640_REG_PAD_OUTPUT_ENABLE02, > >> + on ? 0xfc : 0); > >> } > >> > >> static int ov5640_set_power(struct ov5640_dev *sensor, bool on) > >> @@ -2925,8 +2934,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, Many thanks for the explanations on commit message, I'm currently writing the v2, but if you already have comments on this v1 -appart from the power off section- feel free to post. BR, Hugues. On 10/12/20 4:27 PM, Jacopo Mondi wrote: > Hi Hugues > > On Mon, Oct 12, 2020 at 02:12:09PM +0000, Hugues FRUCHET wrote: >> Hi Jacopo, >> >> Thanks for reviewing, comments below >> >> On 10/12/20 3:35 PM, Jacopo Mondi wrote: >>> Hi Hugues, >>> >>> On Wed, Oct 07, 2020 at 04:24:52PM +0200, Hugues Fruchet wrote: > >>> >>> As per the comment from Sakari in another patch, 'Fixes' (capital 'F') >>> usually comes before Signed-off/Reviewed-by tags >> I understand the "F" upper case, but I don't understand the "comes >> before Signed-off", my signed-off is inserted after this line. >> > > So, this is not stated explicitly in submitting-patches.rst, but as > per all the other tags attached to a patch, they're usually placed > after the commit message > > in this case: > > Fix PCLK polarity not being taken into account. > Fix ov5640_write_reg()return value unchecked at power off. > Reformat code to keep register access below the register description. > Remove useless ov5640_set_stream_bt656() function. > > Fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode") > Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> > >>> >>>> >>>> Fix PCLK polarity not being taken into account. >> >> This one fixes a real bug. >> > > I don't think I've contested this > >>>> Fix ov5640_write_reg()return value unchecked at power off. >>> >>> Am I wrong or you broke out this part to a separate patch ? >>> As commented there, I don't think it's a good idea. >>> >>> Are you planning to send a v2 of this patch ? >> The patches was sent at same time, so it's normal that they are designed >> in same way, I'll send a v2 with this "reset to default at power off >> without check" strategy. >> > > I was wondering if you were planning to send a v2 with the part that > changed the power-off path removed as I would have reviewed that one > instead of this, that's all. > > >>> >>> Thanks >>> j >>> >>>> Reformat code to keep register access below the register description. >>>> Remove useless ov5640_set_stream_bt656() function. >>>> >>>> Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> >>>> --- >>>> drivers/media/i2c/ov5640.c | 95 +++++++++++++++++++++++++--------------------- >>>> 1 file changed, 51 insertions(+), 44 deletions(-) >>>> >>>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c >>>> index 8d0254d..1b20db7 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,20 +1980,12 @@ 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; >>>> int ret; >>>> >>>> - if (!on) { >>>> - /* Reset settings to their default values. */ >>>> - 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); >>>> - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00); >>>> - return 0; >>>> - } >>>> - >>>> /* >>>> * Note about parallel port configuration. >>>> * >>>> @@ -2024,27 +2002,57 @@ 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 >>>> + * >>>> + * 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 >>>> * >>>> - * 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...) >>>> + * 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, >>>> + on && bt656 ? 0x01 : 0x00); >>>> + 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, >>>> - (pclk_pol << 5) | (hsync_pol << 1) | >>>> + ret = ov5640_write_reg(sensor, >>>> + OV5640_REG_POLARITY_CTRL00, >>>> + (pclk_pol << 5) | >>>> + (hsync_pol << 1) | >>>> vsync_pol); >>>> >>>> if (ret) >>>> @@ -2052,12 +2060,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 +2082,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); >>>> + on ? (bt656 ? 0x1f : 0x7f) : 0x00); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -2085,7 +2092,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) >>>> * PAD OUTPUT ENABLE 02 >>>> * - [7:2]: D[5:0] output enable >>>> */ >>>> - return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc); >>>> + return ov5640_write_reg(sensor, >>>> + OV5640_REG_PAD_OUTPUT_ENABLE02, >>>> + on ? 0xfc : 0); >>>> } >>>> >>>> static int ov5640_set_power(struct ov5640_dev *sensor, bool on) >>>> @@ -2925,8 +2934,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..1b20db7 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,20 +1980,12 @@ 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; int ret; - if (!on) { - /* Reset settings to their default values. */ - 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); - ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0x00); - return 0; - } - /* * Note about parallel port configuration. * @@ -2024,27 +2002,57 @@ 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 + * + * 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 * - * 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...) + * 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, + on && bt656 ? 0x01 : 0x00); + 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, - (pclk_pol << 5) | (hsync_pol << 1) | + ret = ov5640_write_reg(sensor, + OV5640_REG_POLARITY_CTRL00, + (pclk_pol << 5) | + (hsync_pol << 1) | vsync_pol); if (ret) @@ -2052,12 +2060,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 +2082,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); + on ? (bt656 ? 0x1f : 0x7f) : 0x00); if (ret) return ret; @@ -2085,7 +2092,9 @@ static int ov5640_set_power_dvp(struct ov5640_dev *sensor, bool on) * PAD OUTPUT ENABLE 02 * - [7:2]: D[5:0] output enable */ - return ov5640_write_reg(sensor, OV5640_REG_PAD_OUTPUT_ENABLE02, 0xfc); + return ov5640_write_reg(sensor, + OV5640_REG_PAD_OUTPUT_ENABLE02, + on ? 0xfc : 0); } static int ov5640_set_power(struct ov5640_dev *sensor, bool on) @@ -2925,8 +2934,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);
fixes: 4039b03720f7 ("media: i2c: ov5640: Add support for BT656 mode") Fix PCLK polarity not being taken into account. Fix ov5640_write_reg()return value unchecked at power off. Reformat code to keep register access below the register description. Remove useless ov5640_set_stream_bt656() function. Signed-off-by: Hugues Fruchet <hugues.fruchet@st.com> --- drivers/media/i2c/ov5640.c | 95 +++++++++++++++++++++++++--------------------- 1 file changed, 51 insertions(+), 44 deletions(-)