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

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(-)

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
>