[PATCH v2 0/3] media: ov5640: Fix power up sequence delays

[PATCH v2 0/3] media: ov5640: Fix power up sequence delays

[Jai Luthra]

This series fixes the power-up sequence delays to support some 15-pin FFC 
compatible OV5640 modules.

Without appropriate delays after both gpio and register-based powerdown and 
reset the sensor SCCB was not very stable, and probe would sometimes fail 
at check_chip_id.

Changes in v2:
- Rename ov5640_reset to ov5640_powerup_sequence to match datasheet
- Remove the check for reset_gpio so that appropriate delays are honored when 
using only the pwdn_gpio
- Remove redundant call to ov5640_power
- Add 5ms delay after SW PWUP used in ov5640_set_stream_dvp()

Jai Luthra (1):
  media: ov5640: Handle delays when no reset_gpio set

Nishanth Menon (2):
  media: ov5640: Honor RESETB to SMBUS time t4 in init_setting
  media: ov5640: Honor power on time in init_setting

 drivers/media/i2c/ov5640.c | 35 ++++++++++++++++++++++++-----------
 1 file changed, 24 insertions(+), 11 deletions(-)

-- 
2.17.1

[PATCH v2 1/3] media: ov5640: Handle delays when no reset_gpio set

[Jai Luthra]

Some module manufacturers [1][2] don't expose the RESETB and PWDN pins
of the sensor directly through the 15-pin FFC connector. Instead wiring
~PWDN gpio to the sensor pins with appropriate delays.

In such cases, reset_gpio will not be available to the driver, but it
will still be toggled when the sensor is powered on, and thus we should
still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3])
before attempting any i/o operations over SCCB.

Also, rename the function to ov5640_powerup_sequence to better match the
datasheet (section 2.7).

[1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf
[2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf
[3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf

Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
Signed-off-by: Jai Luthra <j-luthra@ti.com>
---
 drivers/media/i2c/ov5640.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index a9cd21c49147..41e4a4f1b99d 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -2372,11 +2372,22 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable)
 	gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
 }
 
-static void ov5640_reset(struct ov5640_dev *sensor)
+/*
+ * From section 2.7 power up sequence:
+ * t0 + t1 + t2 >= 5ms	Delay from DOVDD stable to PWDN pull down
+ * t3 >= 1ms		Delay from PWDN pull down to RESETB pull up
+ * t4 >= 20ms		Delay from RESETB pull up to SCCB (i2c) stable
+ *
+ * Some modules don't expose RESETB/PWDN pins directly, instead providing a
+ * "PWUP" GPIO which is wired through appropriate delays and inverters to the
+ * pins.
+ *
+ * In such cases, this gpio should be mapped to pwdn_gpio in the driver, and we
+ * should still toggle the pwdn_gpio below with the appropriate delays, while
+ * the calls to reset_gpio will be ignored.
+ */
+static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
 {
-	if (!sensor->reset_gpio)
-		return;
-
 	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
 
 	/* camera power cycle */
@@ -2412,8 +2423,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
 		goto xclk_off;
 	}
 
-	ov5640_reset(sensor);
-	ov5640_power(sensor, true);
+	ov5640_powerup_sequence(sensor);
 
 	ret = ov5640_init_slave_id(sensor);
 	if (ret)
-- 
2.17.1

[PATCH v2 2/3] media: ov5640: Honor RESETB to SMBUS time t4 in init_setting

[Jai Luthra]

From: Nishanth Menon <nm@ti.com>

OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
that is expected during various initialization steps. Note the t4 >=
20ms, delay from RESETB pull high to SCCB initialization

As indicated in section 2.8, the RESETB assertion can either be via
external pin control OR via the register 0x3008 bit 7. The
initialization sequence asserts bit7, but on deassert, we do not wait
for the reset delay.

[1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf

Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 drivers/media/i2c/ov5640.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 41e4a4f1b99d..7b0ff04a2c93 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -475,7 +475,7 @@ static const struct v4l2_mbus_framefmt ov5640_default_fmt = {
 };
 
 static const struct reg_value ov5640_init_setting[] = {
-	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
+	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 20},
 	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
 	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
 	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
-- 
2.17.1

[PATCH v2 3/3] media: ov5640: Honor power on time in init_setting

[Jai Luthra]

From: Nishanth Menon <nm@ti.com>

OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
that is expected during various initialization steps. Note the power
on time includes t0 + t1 + t2 >= 5ms, delay for poweron.

As indicated in section 2.8, the PWDN assertion can either be via
external pin control OR via the register 0x3008 bit 6 (see table 7-1 in
[1])

[1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf

Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
Signed-off-by: Nishanth Menon <nm@ti.com>
---
 drivers/media/i2c/ov5640.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 7b0ff04a2c93..0ea8691ecded 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -551,7 +551,7 @@ static const struct reg_value ov5640_init_setting[] = {
 	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
 	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
 	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
-	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
+	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
 };
 
 static const struct reg_value ov5640_setting_low_res[] = {
@@ -1735,9 +1735,12 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
 
 static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
 {
-	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
-				OV5640_REG_SYS_CTRL0_SW_PWUP :
-				OV5640_REG_SYS_CTRL0_SW_PWDN);
+	int ret;
+	ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
+			       OV5640_REG_SYS_CTRL0_SW_PWUP :
+			       OV5640_REG_SYS_CTRL0_SW_PWDN);
+	usleep_range(5000, 6000);
+	return ret;
 }
 
 static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
-- 
2.17.1

Re: [PATCH v2 2/3] media: ov5640: Honor RESETB to SMBUS time t4 in init_setting

[Jacopo Mondi]

Hi Jai

On Mon, Dec 19, 2022 at 08:00:55PM +0530, Jai Luthra wrote:
> From: Nishanth Menon <nm@ti.com>
>
> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
> that is expected during various initialization steps. Note the t4 >=
> 20ms, delay from RESETB pull high to SCCB initialization
>
> As indicated in section 2.8, the RESETB assertion can either be via
> external pin control OR via the register 0x3008 bit 7. The
> initialization sequence asserts bit7, but on deassert, we do not wait
> for the reset delay.
>
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/media/i2c/ov5640.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 41e4a4f1b99d..7b0ff04a2c93 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -475,7 +475,7 @@ static const struct v4l2_mbus_framefmt ov5640_default_fmt = {
>  };
>
>  static const struct reg_value ov5640_init_setting[] = {
> -	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> +	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 20},

I'm debated here...

This adds a 20msec delay, even in case the reset has been performed
using the GPIO reset line.

Ideally, these SW resets should be moved to ov5640_reset() and only
used if no HW pin is available.

I wonder if ov5640_reset() could go as:

static void ov5640_reset(struct ov5640_dev *sensor)
{
	if (sensor->pwdn_gpio) {
		gpiod_set_value_cansleep(sensor->reset_gpio, 0);

		/* camera power cycle */
		ov5640_power(sensor, false);
		usleep_range(5000, 10000);
		ov5640_power(sensor, true);
		usleep_range(5000, 10000);

		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
		usleep_range(1000, 2000);

		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
	} else {
		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, 0x82);
		usleep_range(5000, 10000);

		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, 0x42);
	}

	usleep_range(20000, 25000);
}


And remove
         {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
from the init_settings[] table.

FWIW I have a setup with reset and pwdn lines and I have tried
with both lines made available to the driver from DTS, and also by removing
them from DTS. In both cases the sensor works for me and the first
captured pictures are ok. Removing the gpio lines from DTS
is probably not enough to simulate a "software-only" setup as the
lines might be left floating and could interfere with the powerup
operations, but in my case it seems it's ok.

Could you try with your setup to see if it works ?

Am I overthinking it ? A delay of 50+msec (25 hw + 25 sw) at power-up
time is not negligible considering that half of that might not be
necessary.


>  	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
>  	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
>  	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
> --
> 2.17.1
>

Re: [PATCH v2 1/3] media: ov5640: Handle delays when no reset_gpio set

[Jacopo Mondi]

Hi Jai

On Mon, Dec 19, 2022 at 08:00:54PM +0530, Jai Luthra wrote:
> Some module manufacturers [1][2] don't expose the RESETB and PWDN pins
> of the sensor directly through the 15-pin FFC connector. Instead wiring
> ~PWDN gpio to the sensor pins with appropriate delays.
>
> In such cases, reset_gpio will not be available to the driver, but it
> will still be toggled when the sensor is powered on, and thus we should
> still honor the wait time of >= 5ms + 1ms + 20ms (see figure 2-3 in [3])
> before attempting any i/o operations over SCCB.
>
> Also, rename the function to ov5640_powerup_sequence to better match the
> datasheet (section 2.7).
>
> [1] https://digilent.com/reference/_media/reference/add-ons/pcam-5c/pcam_5c_sch.pdf
> [2] https://www.alinx.com/public/upload/file/AN5641_User_Manual.pdf
> [3] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> Signed-off-by: Jai Luthra <j-luthra@ti.com>

This looks fine to me
Reviewed-by: Jacopo Mondi <jacopo.mondi@ideasonboard.com>

Thanks
  j

> ---
>  drivers/media/i2c/ov5640.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index a9cd21c49147..41e4a4f1b99d 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -2372,11 +2372,22 @@ static void ov5640_power(struct ov5640_dev *sensor, bool enable)
>  	gpiod_set_value_cansleep(sensor->pwdn_gpio, enable ? 0 : 1);
>  }
>
> -static void ov5640_reset(struct ov5640_dev *sensor)
> +/*
> + * From section 2.7 power up sequence:
> + * t0 + t1 + t2 >= 5ms	Delay from DOVDD stable to PWDN pull down
> + * t3 >= 1ms		Delay from PWDN pull down to RESETB pull up
> + * t4 >= 20ms		Delay from RESETB pull up to SCCB (i2c) stable
> + *
> + * Some modules don't expose RESETB/PWDN pins directly, instead providing a
> + * "PWUP" GPIO which is wired through appropriate delays and inverters to the
> + * pins.
> + *
> + * In such cases, this gpio should be mapped to pwdn_gpio in the driver, and we
> + * should still toggle the pwdn_gpio below with the appropriate delays, while
> + * the calls to reset_gpio will be ignored.
> + */
> +static void ov5640_powerup_sequence(struct ov5640_dev *sensor)
>  {
> -	if (!sensor->reset_gpio)
> -		return;
> -
>  	gpiod_set_value_cansleep(sensor->reset_gpio, 0);
>
>  	/* camera power cycle */
> @@ -2412,8 +2423,7 @@ static int ov5640_set_power_on(struct ov5640_dev *sensor)
>  		goto xclk_off;
>  	}
>
> -	ov5640_reset(sensor);
> -	ov5640_power(sensor, true);
> +	ov5640_powerup_sequence(sensor);
>
>  	ret = ov5640_init_slave_id(sensor);
>  	if (ret)
> --
> 2.17.1
>

Re: [PATCH v2 3/3] media: ov5640: Honor power on time in init_setting

[Jacopo Mondi]

Hi Jai

On Mon, Dec 19, 2022 at 08:00:56PM +0530, Jai Luthra wrote:
> From: Nishanth Menon <nm@ti.com>
>
> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
> that is expected during various initialization steps. Note the power
> on time includes t0 + t1 + t2 >= 5ms, delay for poweron.
>
> As indicated in section 2.8, the PWDN assertion can either be via
> external pin control OR via the register 0x3008 bit 6 (see table 7-1 in
> [1])
>
> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>
> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
> Signed-off-by: Nishanth Menon <nm@ti.com>
> ---
>  drivers/media/i2c/ov5640.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
> index 7b0ff04a2c93..0ea8691ecded 100644
> --- a/drivers/media/i2c/ov5640.c
> +++ b/drivers/media/i2c/ov5640.c
> @@ -551,7 +551,7 @@ static const struct reg_value ov5640_init_setting[] = {
>  	{0x583b, 0x28, 0, 0}, {0x583c, 0x42, 0, 0}, {0x583d, 0xce, 0, 0},
>  	{0x5025, 0x00, 0, 0}, {0x3a0f, 0x30, 0, 0}, {0x3a10, 0x28, 0, 0},
>  	{0x3a1b, 0x30, 0, 0}, {0x3a1e, 0x26, 0, 0}, {0x3a11, 0x60, 0, 0},
> -	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 0}, {0x3c00, 0x04, 0, 300},
> +	{0x3a1f, 0x14, 0, 0}, {0x3008, 0x02, 0, 5}, {0x3c00, 0x04, 0, 300},
>  };
>
>  static const struct reg_value ov5640_setting_low_res[] = {
> @@ -1735,9 +1735,12 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
>
>  static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
>  {
> -	return ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> -				OV5640_REG_SYS_CTRL0_SW_PWUP :
> -				OV5640_REG_SYS_CTRL0_SW_PWDN);
> +	int ret;
> +	ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, on ?
> +			       OV5640_REG_SYS_CTRL0_SW_PWUP :
> +			       OV5640_REG_SYS_CTRL0_SW_PWDN);
> +	usleep_range(5000, 6000);

Do you need to delay also when !on ?

> +	return ret;
>  }
>
>  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
> --
> 2.17.1
>

Re: [PATCH v2 2/3] media: ov5640: Honor RESETB to SMBUS time t4 in init_setting

[Jai Luthra]

Hi Jacopo,

Thanks for the comments,

On 22/12/22 23:43, Jacopo Mondi wrote:
> Hi Jai
> 
> On Mon, Dec 19, 2022 at 08:00:55PM +0530, Jai Luthra wrote:
>> From: Nishanth Menon <nm@ti.com>
>>
>> OV5640 Datasheet[1] Figures 2-3 and 2-4 indicate the timing sequences
>> that is expected during various initialization steps. Note the t4 >=
>> 20ms, delay from RESETB pull high to SCCB initialization
>>
>> As indicated in section 2.8, the RESETB assertion can either be via
>> external pin control OR via the register 0x3008 bit 7. The
>> initialization sequence asserts bit7, but on deassert, we do not wait
>> for the reset delay.
>>
>> [1] https://cdn.sparkfun.com/datasheets/Sensors/LightImaging/OV5640_datasheet.pdf
>>
>> Fixes: 19a81c1426c1 ("[media] add Omnivision OV5640 sensor driver")
>> Signed-off-by: Nishanth Menon <nm@ti.com>
>> ---
>>   drivers/media/i2c/ov5640.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
>> index 41e4a4f1b99d..7b0ff04a2c93 100644
>> --- a/drivers/media/i2c/ov5640.c
>> +++ b/drivers/media/i2c/ov5640.c
>> @@ -475,7 +475,7 @@ static const struct v4l2_mbus_framefmt ov5640_default_fmt = {
>>   };
>>
>>   static const struct reg_value ov5640_init_setting[] = {
>> -	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
>> +	{0x3103, 0x11, 0, 0}, {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 20},
> 
> I'm debated here...
> 
> This adds a 20msec delay, even in case the reset has been performed
> using the GPIO reset line.
> 
> Ideally, these SW resets should be moved to ov5640_reset() and only
> used if no HW pin is available.
> 
> I wonder if ov5640_reset() could go as:
> 
> static void ov5640_reset(struct ov5640_dev *sensor)
> {
> 	if (sensor->pwdn_gpio) {
> 		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> 
> 		/* camera power cycle */
> 		ov5640_power(sensor, false);
> 		usleep_range(5000, 10000);
> 		ov5640_power(sensor, true);
> 		usleep_range(5000, 10000);
> 
> 		gpiod_set_value_cansleep(sensor->reset_gpio, 1);
> 		usleep_range(1000, 2000);
> 
> 		gpiod_set_value_cansleep(sensor->reset_gpio, 0);
> 	} else {
> 		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, 0x82);
> 		usleep_range(5000, 10000);
> 
> 		ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0, 0x42);
> 	}
> 
> 	usleep_range(20000, 25000);
> }
> 
> 
> And remove
>           {0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},
> from the init_settings[] table.
> 
> FWIW I have a setup with reset and pwdn lines and I have tried
> with both lines made available to the driver from DTS, and also by removing
> them from DTS. In both cases the sensor works for me and the first
> captured pictures are ok. Removing the gpio lines from DTS
> is probably not enough to simulate a "software-only" setup as the
> lines might be left floating and could interfere with the powerup
> operations, but in my case it seems it's ok.
> 
> Could you try with your setup to see if it works ?

Will do.

> 
> Am I overthinking it ? A delay of 50+msec (25 hw + 25 sw) at power-up
> time is not negligible considering that half of that might not be
> necessary.

I agree, 50ms is too much. I will rework [2/3] and [3/3] to move SW 
reset in a common function.

> 
> 
>>   	{0x3103, 0x03, 0, 0}, {0x3630, 0x36, 0, 0},
>>   	{0x3631, 0x0e, 0, 0}, {0x3632, 0xe2, 0, 0}, {0x3633, 0x12, 0, 0},
>>   	{0x3621, 0xe0, 0, 0}, {0x3704, 0xa0, 0, 0}, {0x3703, 0x5a, 0, 0},
>> --
>> 2.17.1
>>

Thanks,
Jai