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

Jai Luthra <j-luthra@xxxxxx> · Tue, 3 Jan 2023 14:09:04 +0530

Hi Jacopo,

On 02/01/23 18:29, Jacopo Mondi wrote:
Hello again

On Thu, Dec 29, 2022 at 07:10:36PM +0100, Jacopo Mondi wrote:
Hi again

On Tue, Dec 27, 2022 at 11:06:34PM +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>
Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
---
  drivers/media/i2c/ov5640.c | 16 ++++++++++++----
  1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 5df16fb6f0a0..bd7cc294cfe6 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -609,7 +609,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},

This might also allow to remove the

		/* remain in power down mode for DVP */
		if (regs->reg_addr == OV5640_REG_SYS_CTRL0 &&
		    val == OV5640_REG_SYS_CTRL0_SW_PWUP &&
		    !ov5640_is_csi2(sensor))
			continue;

in ov5640_load_regs()

Prabhakar since you have introduced it, coudl you test if you still
need it with Nishanth's patch applied ?

No, it doesn't, sorry for the confusion.

The following patch would allow to remove the above quirk by removing
         {0x3008, 0x02}
from the init_sequence[].

Unfortunately the first 3 images are then black when running in CSI-2
mode.

-------------------------------------------------------------------------------
diff --git a/drivers/media/i2c/ov5640.c b/drivers/media/i2c/ov5640.c
index 96b986051414..bfb60648c72a 100644
--- a/drivers/media/i2c/ov5640.c
+++ b/drivers/media/i2c/ov5640.c
@@ -609,7 +609,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, 5}, {0x3c00, 0x04, 0, 300},
+       {0x3a1f, 0x14, 0, 0}, {0x3c00, 0x04, 0, 300},
  };

  static const struct reg_value ov5640_setting_low_res[] = {
@@ -1808,7 +1808,7 @@ static int ov5640_set_autogain(struct ov5640_dev *sensor, bool on)
                               BIT(1), on ? 0 : BIT(1));
  }

-static int ov5640_set_stream_dvp(struct ov5640_dev *sensor, bool on)
+static int ov5640_set_stream(struct ov5640_dev *sensor, bool on)
  {
         int ret;

@@ -3725,11 +3725,13 @@ static int ov5640_s_stream(struct v4l2_subdev *sd, int enable)
                         sensor->pending_fmt_change = false;
                 }

-               if (ov5640_is_csi2(sensor))
+               if (ov5640_is_csi2(sensor)) {
                         ret = ov5640_set_stream_mipi(sensor, enable);
-               else
-                       ret = ov5640_set_stream_dvp(sensor, enable);
+                       if (ret)
+                               goto out;
+               }

+               ret = ov5640_set_stream(sensor, enable);
                 if (!ret)
                         sensor->streaming = enable;
         }
-------------------------------------------------------------------------------

I do however now question the patch utility itself. SW PWDN is the
software standby state, while Figure 2.3 and 2.4 you mention in the
commit message report:

t2 = 5 ms: delay from DVDD stable to sensor power up stable

I doubt this apply to SW standby as it the delay is probably required
to have the internal circuitry stabilize after the power rail has been
enabled.

Good catch.


Does this patch make any practical difference in your setup ? I'm
asking as in my case it doesn't, but I'm on a CSI-2 setup.

I tested today with this patch removed, and it does not make any 
difference to the probe success rate. We missed this during development 
as we first tried the SW powerdown and reset timing changes, and later 
the hardware reset timing changes [1/3].

I will drop this patch in the next series. Thanks for pointing this out.



Thanks
   j


  };

  static const struct reg_value ov5640_setting_low_res[] = {
@@ -1810,9 +1810,17 @@ 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;
+
+	if (on) {
+		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
+				       OV5640_REG_SYS_CTRL0_SW_PWUP);
+		usleep_range(5000, 6000);
+	} else {
+		ret = ov5640_write_reg(sensor, OV5640_REG_SYS_CTRL0,
+				       OV5640_REG_SYS_CTRL0_SW_PWDN);
+	}
+	return ret;
  }

  static int ov5640_set_stream_mipi(struct ov5640_dev *sensor, bool on)
--
2.17.1


Thanks,
Jai






