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

Jai Luthra <j-luthra@xxxxxx> · Mon, 19 Dec 2022 19:47:28 +0530

Hi Jacopo,

On 19/12/22 18:55, Jacopo Mondi wrote:
Hi Jai

On Mon, Dec 19, 2022 at 06:38:54PM +0530, Jai Luthra wrote:
Hi Jacopo,

Thank you for the comments.

On 19/12/22 15:40, Jacopo Mondi wrote:
Hi Jai

On Fri, Dec 16, 2022 at 07:14:09PM +0530, Jai Luthra wrote:
From: Nishanth Menon <nm@xxxxxx>

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@xxxxxx>
---
   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 fa84e60de0db..ff2a2c9358e7 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -608,7 +608,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},

Two observations:

as per the description of register 0x3008

3008 default value = 0x02
3008[7] = Software Reset
3008[6] = Software Power Down

The init_settings[] register table has these entries at the very
beginning

	{0x3008, 0x82, 0, 5}, {0x3008, 0x42, 0, 0},

and ends with the entry you have modified

          {0x3008, 0x02, 0, 5}

As I read from the 2.8 section of the datasheet

A reset can also be initiated through the SCCB interface by setting
register 0x3008[7] to high.

So I presume the first two registers entries:

	{0x3008, 0x82, 0, 5} -> Start a SW reset and wait 5 msec
                                  for the chip to resume
	{0x3008, 0x42, 0, 0} -> SW standby mode

SW standby mode is described as:

          Executing a software standby through the SCCB interface
          suspends internal circuit activity but does not halt the
          device clock. All register content is maintained in standby
          mode.

I agree with everything above.


I presume that the first

	{0x3008, 0x42, 0, 0}

   exists from SW standby mode to program the chip and the last

Here I'm a little confused. Isn't it the opposite according to the
datasheet?

Setting the register to 0x42 (bit 6 = 1) initiates SW standby, which I
assume stops all other functions on the chip but still allows programming
registers.


I haven't found any mention about the BIT(6) polarity. IOW I haven't
found documented if setting it to 1 enters or exists SW standby mode.

Your reasoning is valid, I'm just a bit surprised the SCCB is active
and registers can be programmed while in standby mode. >
However, this

#define OV5640_REG_SYS_CTRL0_SW_PWDN	0x42
#define OV5640_REG_SYS_CTRL0_SW_PWUP	0x02

seems to confirm what you said. >

          {0x3008, 0x02, 0, 5}

puts the chip in sw standby at the end of init_settings[]

Then setting 0x02 (bit 6 = 0) exits from SW standby in case of MIPI.

For DVP though, I see that we skip setting 0x02 to that register in
ov5640_load_regs(), so sensor stays in SW standby mode until
ov5640_set_stream_dvp() is called.


That's what confused me. init_settings[] are programmed for both DVP
and MIPI. If the last write to 0x3008 is 0x02 there shouldn't be any
need to re-program it. However since ov5640_set_stream_dvp() is called
in the s_stream() call path it means SW standby is entered between
every streaming session.

Please note that no 5msec delay is respected when
existing/entering sw standby between streaming session.

Good catch, I wonder if that is what causes failure to stream here
https://lore.kernel.org/all/3b54ab9b-4ffd-ab32-d495-fad6132ea504@xxxxxxxxxxxxx/

Will fix in v2.

>
Software standby is then exited by ov5640_set_stream_dvp() for DVP and
by clearing 0x300e[4:3] in MIPI mode, as the datasheet reports:

          To initiate hardware standby mode, the PWDN pin must be tied to high
          (while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDN
          pin is set to high >
My second observation is that those entries in the init_settings[]
table performs SW reset/standby regardless if there's a GPIO or not
installed to control the reset and pwdn lines.

Would it be worth in your opinion trying to modify ov5640_power()
and ov5640_reset() to use either SW or HW standby/reset conditionally
on the avialability of sensor->reset_gpio and sensor->pwdn_gpio and
remove the initial SW standby/reset from init_setting[] ?

I don't think we can use the SCCB register pwdn & reset for the initial
sensor power up sequence, as sensor SCCB depends on the correct sequence
being followed on hardware pins. From Section 2.8:

I was suggesting to use sw reset/powerup if no gpios are registered.


	Manually applying a hard reset upon power up is required even
	though on-chip reset is included.


Do you interpret "Manually applying a reset upon power up" to imply
"software reset" ? As I read it it doesn't require the reset to be
SW or HW.

It's the second phrase "though on-chip reset is included" that makes me 
believe they require it to be HW only.


But I agree, if some module does not wire the pins at all then we might have
to ignore the datasheet here and try it using the register.


That's what I was suggesting. If you have gpios you shouldn't need
initial SW resets and powerup. If you don't have gpios, use sw reset
so that the reset/powerup routines are centralized and not spread between
register tables and functions ?

Makes sense, I will try this out and spin it as a separate series.


What setup are you testing with ? DVP or MIPI ? With gpios or without
? I can test on a 2-data lanes MIPI setup with HW gpio lines if it
helps.

Thanks, my module is similar with MIPI but only has the PWUP gpio line. 
Might send you the patches separately if it doesn't work on my setup.


Thanks
   j


Thanks,
Jai




   };

   static const struct reg_value ov5640_setting_low_res[] = {
--
2.17.1


Thanks,
Jai






