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

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

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
>
  

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