| Message ID | 20230724222116.162555-1-marex@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | media: ov5640: Fix initial RESETB state and annotate timings | expand |
Cc Jai which has recently changed this part to accomodate a design where RESETB and PWDN are not exposed as separate lines On Tue, Jul 25, 2023 at 12:21:16AM +0200, Marek Vasut wrote: > The initial state of RESETB input signal of OV5640 should be > asserted, i.e. the sensor should be in reset. This is not the > case, make it so. > > Since the subsequent assertion of RESETB signal is no longer > necessary and the timing of the power sequencing could be > slightly adjusted, add annotations to the delays which match > OV5640 datasheet rev. 2.03, both: > figure 2-3 power up timing with internal DVDD > figure 2-4 power up timing with external DVDD source > > The 5..10ms delay between PWDN assertion and RESETB assertion > is not even documented in the power sequencing diagram, and > with this reset fix, it is no longer even necessary. > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Signed-off-by: Marek Vasut <marex@denx.de> > --- > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> > Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > Cc: Steve Longerbeam <slongerbeam@gmail.com> > Cc: linux-media@vger.kernel.org > --- > drivers/media/i2c/ov5640.c | 11 ++++------- > 1 file changed, 4 insertions(+), 7 deletions(-) > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > index 7c065c39082dd..74b58380b5e69 100644 > --- a/drivers/media/i2c/ov5640.c > +++ b/drivers/media/i2c/ov5640.c > @@ -2452,16 +2452,13 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable) > static void ov5640_powerup_sequence(struct ov5640_dev *sensor) > { > if (sensor->pwdn_gpio) { > - gpiod_set_value_cansleep(sensor->reset_gpio, 0); > + gpiod_set_value_cansleep(sensor->reset_gpio, 1); > > /* camera power cycle */ > ov5640_power(sensor, false); This is also probably a NOP for most designs but the one Jai had, as in his case a single "powerdown" line controls both PWDN and RESETB with some internal circuitry that handles signals inversions and delays. So I guess it is fine to have it here. > - usleep_range(5000, 10000); > + usleep_range(5000, 10000); /* t2 */ > ov5640_power(sensor, true); > - usleep_range(5000, 10000); > - > - gpiod_set_value_cansleep(sensor->reset_gpio, 1); > - usleep_range(1000, 2000); > + usleep_range(1000, 2000); /* t3 */ > > gpiod_set_value_cansleep(sensor->reset_gpio, 0); > } else { > @@ -2469,7 +2466,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor) > ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > OV5640_REG_SYS_CTRL0_SW_RST); > } > - usleep_range(20000, 25000); > + usleep_range(20000, 25000); /* t4 */ This all looks good to me Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Jai could you give this a run to make sure this works on your setup as well ? Thanks j > > /* > * software standby: allows registers programming; > -- > 2.40.1 >
Hi Marek, Jacopo, On Jul 25, 2023 at 11:09:20 +0200, Jacopo Mondi wrote: > Cc Jai which has recently changed this part to accomodate a design > where RESETB and PWDN are not exposed as separate lines > > On Tue, Jul 25, 2023 at 12:21:16AM +0200, Marek Vasut wrote: > > The initial state of RESETB input signal of OV5640 should be > > asserted, i.e. the sensor should be in reset. This is not the > > case, make it so. > > > > Since the subsequent assertion of RESETB signal is no longer > > necessary and the timing of the power sequencing could be > > slightly adjusted, add annotations to the delays which match > > OV5640 datasheet rev. 2.03, both: > > figure 2-3 power up timing with internal DVDD > > figure 2-4 power up timing with external DVDD source > > > > The 5..10ms delay between PWDN assertion and RESETB assertion > > is not even documented in the power sequencing diagram, and > > with this reset fix, it is no longer even necessary. > > > > Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") > > Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Signed-off-by: Marek Vasut <marex@denx.de> > > --- > > Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> > > Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Cc: Mauro Carvalho Chehab <mchehab@kernel.org> > > Cc: Sakari Ailus <sakari.ailus@linux.intel.com> > > Cc: Steve Longerbeam <slongerbeam@gmail.com> > > Cc: linux-media@vger.kernel.org > > --- > > drivers/media/i2c/ov5640.c | 11 ++++------- > > 1 file changed, 4 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c > > index 7c065c39082dd..74b58380b5e69 100644 > > --- a/drivers/media/i2c/ov5640.c > > +++ b/drivers/media/i2c/ov5640.c > > @@ -2452,16 +2452,13 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable) > > static void ov5640_powerup_sequence(struct ov5640_dev *sensor) > > { > > if (sensor->pwdn_gpio) { > > - gpiod_set_value_cansleep(sensor->reset_gpio, 0); > > + gpiod_set_value_cansleep(sensor->reset_gpio, 1); > > > > /* camera power cycle */ > > ov5640_power(sensor, false); > > This is also probably a NOP for most designs but the one Jai had, as > in his case a single "powerdown" line controls both PWDN and RESETB > with some internal circuitry that handles signals inversions and > delays. So I guess it is fine to have it here. > Yes the modules in question are - - Digilent PCam 5C - ALINX AN5641 - TEVI-OV5640-*-RPI All of them have a 15-pin FFC (RPi) compatible connector. > > - usleep_range(5000, 10000); > > + usleep_range(5000, 10000); /* t2 */ > > ov5640_power(sensor, true); > > - usleep_range(5000, 10000); I remember noticing this before, but chose to keep it around as the above mentioned modules have a 5.5ms + 1ms delay mentioned in the schematics [1] between toggling the weird "PWUP" gpio to the RESET actually getting toggled. But in my tests your patch is not breaking any of those modules, and this delay does look very redundant from driver p.o.v. so I am happy with removing it here. In future if I face any issues I will try to come up with a better approach that does not add redundant delays for modules that don't perform magical delays on-board like those three special ones :) [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf > > - > > - gpiod_set_value_cansleep(sensor->reset_gpio, 1); > > - usleep_range(1000, 2000); > > + usleep_range(1000, 2000); /* t3 */ > > > > gpiod_set_value_cansleep(sensor->reset_gpio, 0); > > } else { > > @@ -2469,7 +2466,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor) > > ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, > > OV5640_REG_SYS_CTRL0_SW_RST); > > } > > - usleep_range(20000, 25000); > > + usleep_range(20000, 25000); /* t4 */ > > > This all looks good to me > Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > > Jai could you give this a run to make sure this works on your setup as > well ? > Works on BeaglePlay and SK-AM62 (with my CSI v8 series applied on next) Tested-by: Jai Luthra <j-luthra@ti.com> > Thanks > j > > > > > /* > > * software standby: allows registers programming; > > -- > > 2.40.1 > > >
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c index 7c065c39082dd..74b58380b5e69 100644 --- a/drivers/media/i2c/ov5640.c +++ b/drivers/media/i2c/ov5640.c @@ -2452,16 +2452,13 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable) static void ov5640_powerup_sequence(struct ov5640_dev *sensor) { if (sensor->pwdn_gpio) { - gpiod_set_value_cansleep(sensor->reset_gpio, 0); + gpiod_set_value_cansleep(sensor->reset_gpio, 1); /* camera power cycle */ ov5640_power(sensor, false); - usleep_range(5000, 10000); + usleep_range(5000, 10000); /* t2 */ ov5640_power(sensor, true); - usleep_range(5000, 10000); - - gpiod_set_value_cansleep(sensor->reset_gpio, 1); - usleep_range(1000, 2000); + usleep_range(1000, 2000); /* t3 */ gpiod_set_value_cansleep(sensor->reset_gpio, 0); } else { @@ -2469,7 +2466,7 @@ static void ov5640_powerup_sequence(struct ov5640_dev *sensor) ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, OV5640_REG_SYS_CTRL0_SW_RST); } - usleep_range(20000, 25000); + usleep_range(20000, 25000); /* t4 */ /* * software standby: allows registers programming;
The initial state of RESETB input signal of OV5640 should be asserted, i.e. the sensor should be in reset. This is not the case, make it so. Since the subsequent assertion of RESETB signal is no longer necessary and the timing of the power sequencing could be slightly adjusted, add annotations to the delays which match OV5640 datasheet rev. 2.03, both: figure 2-3 power up timing with internal DVDD figure 2-4 power up timing with external DVDD source The 5..10ms delay between PWDN assertion and RESETB assertion is not even documented in the power sequencing diagram, and with this reset fix, it is no longer even necessary. Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver") Reported-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Signed-off-by: Marek Vasut <marex@denx.de> --- Cc: Hans Verkuil <hverkuil-cisco@xs4all.nl> Cc: Jacopo Mondi <jacopo.mondi@ideasonboard.com> Cc: Mauro Carvalho Chehab <mchehab@kernel.org> Cc: Sakari Ailus <sakari.ailus@linux.intel.com> Cc: Steve Longerbeam <slongerbeam@gmail.com> Cc: linux-media@vger.kernel.org --- drivers/media/i2c/ov5640.c | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-)