diff mbox series

media: ov5640: Fix initial RESETB state and annotate timings

Message ID 20230724222116.162555-1-marex@denx.de (mailing list archive)
State Accepted
Delegated to: Sakari Ailus
Headers
Series media: ov5640: Fix initial RESETB state and annotate timings |

Commit Message

Marek Vasut July 24, 2023, 10:21 p.m. UTC 
  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(-)

Comments

Jacopo Mondi July 25, 2023, 9:09 a.m. UTC | #1 
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
>
Jai Luthra Aug. 3, 2023, 8:56 a.m. UTC | #2 
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 mbox series

Patch

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;