[PATCH 0/2] Fix OV5645 capture issue with 400kHz i2c bus frequency

[PATCH 0/2] Fix OV5645 capture issue with 400kHz i2c bus frequency

[Biju Das]

This patch series aims to fix OV5645 capture issue with 400kHz i2c bus
frequency observed on RZ/G2L SMARC EVK platform.

Testing OV5645 with i2c bus frequency @400kHz on RZ/G2L SMARC EVL platform
shows issues like the captured image is either greenish or it is not
capturing the image at all. However, It is working ok when the i2c
frequency is 100kHz. From this, it is clear that we have a timing issue
at high speed. The testing also shows that if we add a delay >= 1 msec
after register write {0x3008, 0x82}, then the captured image is always
good.

Since OV5645 is showing issues @400kHz, it makes sense to add a small
delay after register writes for settling the register values. So introduce
a small delay by adding a read() after write() and also add a debug code
for data mismatch.

Biju Das (2):
  media: i2c: ov5645: Move the register 0x3008 from
    ov5645_global_init_setting
  media: i2c: ov5645: Add a small delay after writes in
    ov5645_set_register_array()

 drivers/media/i2c/ov5645.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Biju Das]

Testing OV5645 with i2c bus frequency @400kHz on RZ/G2L SMARC EVL platform
shows issues like the captured image is either greenish or it is not
capturing the image at all. However, It is working ok when the i2c
frequency is 100kHz. From this, it is clear that we have a timing issue
at high speed. The testing also shows that if we add a delay >= 1 msec
after register write {0x3008, 0x82}, then the captured image is always
good. So, move the register 0x3008 and 0x3103 from ov5645_*_init_setting
to a new table ov5645_global_init_setting.

Drop the unnecessary entry { 0x3008, 0x42 } from ov5645_*init_setting
table at the start.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a26ac11c989d..a5cc959d535e 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -116,10 +116,12 @@ static inline struct ov5645 *to_ov5645(struct v4l2_subdev *sd)
 	return container_of(sd, struct ov5645, sd);
 }
 
-static const struct reg_value ov5645_global_init_setting[] = {
+static const struct reg_value ov5645_global_reset_setting[] = {
 	{ 0x3103, 0x11 },
-	{ 0x3008, 0x82 },
-	{ 0x3008, 0x42 },
+	{ 0x3008, 0x82 }
+};
+
+static const struct reg_value ov5645_global_init_setting[] = {
 	{ 0x3103, 0x03 },
 	{ 0x3503, 0x07 },
 	{ 0x3002, 0x1c },
@@ -671,6 +673,15 @@ static int ov5645_set_power_on(struct device *dev)
 
 	msleep(20);
 
+	ret = ov5645_set_register_array(ov5645, ov5645_global_reset_setting,
+					ARRAY_SIZE(ov5645_global_reset_setting));
+	if (ret < 0) {
+		dev_err(ov5645->dev, "could not reset\n");
+		goto exit;
+	}
+
+	usleep_range(1000, 2000);
+
 	ret = ov5645_set_register_array(ov5645, ov5645_global_init_setting,
 					ARRAY_SIZE(ov5645_global_init_setting));
 	if (ret < 0) {
-- 
2.25.1

Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1300 bytes --]

Hi Biju,

I couldn't find a datasheet for the OV5645 but the one for OV5642 looked
pretty similar when it comes to the issues mentioned here.

> Testing OV5645 with i2c bus frequency @400kHz on RZ/G2L SMARC EVL platform
> shows issues like the captured image is either greenish or it is not
> capturing the image at all. However, It is working ok when the i2c
> frequency is 100kHz. From this, it is clear that we have a timing issue
> at high speed. The testing also shows that if we add a delay >= 1 msec

That could match the "VDD stable to sensor stable" delay in the
datasheet.

> after register write {0x3008, 0x82}, then the captured image is always
> good. So, move the register 0x3008 and 0x3103 from ov5645_*_init_setting
> to a new table ov5645_global_init_setting.
> 
> Drop the unnecessary entry { 0x3008, 0x42 } from ov5645_*init_setting
> table at the start.

It seems this is not needed to fix your issue? Then, this would be a
seperate change with a dedicated reason, I'd think. There is another
pair of activating power-down mode and immediately disabling it again.
Either we simplify it, too, or we leave both in place. Or at least make
sure there wasn't a reason for them. I'd just leave them.

Rest looks good to me.

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

On Tue, Feb 13, 2024 at 10:27 PM Wolfram Sang <wsa@kernel.org> wrote:
>
> Hi Biju,
>
> I couldn't find a datasheet for the OV5645 but the one for OV5642 looked
> pretty similar when it comes to the issues mentioned here.

I don't have the data sheet. I am just debugging with a picoscope.

>
> > Testing OV5645 with i2c bus frequency @400kHz on RZ/G2L SMARC EVL platform
> > shows issues like the captured image is either greenish or it is not
> > capturing the image at all. However, It is working ok when the i2c
> > frequency is 100kHz. From this, it is clear that we have a timing issue
> > at high speed. The testing also shows that if we add a delay >= 1 msec
>
> That could match the "VDD stable to sensor stable" delay in the
> datasheet.

I think it is different here. That 1 msec is delay associated with
applying hardware power see [1]

[1]
https://elixir.bootlin.com/linux/latest/source/drivers/media/i2c/ov5645.c#L667


>
> > after register write {0x3008, 0x82}, then the captured image is always
> > good. So, move the register 0x3008 and 0x3103 from ov5645_*_init_setting
> > to a new table ov5645_global_init_setting.
> >
> > Drop the unnecessary entry { 0x3008, 0x42 } from ov5645_*init_setting
> > table at the start.
>
> It seems this is not needed to fix your issue? Then, this would be a
> seperate change with a dedicated reason, I'd think. There is another
> pair of activating power-down mode and immediately disabling it again.
> Either we simplify it, too, or we leave both in place. Or at least make
> sure there wasn't a reason for them. I'd just leave them.

My bad. I thought as per [2], there are 2 writes for 0x3008, so I
should take out the redundant one.
I will restore it.

[2]
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20240126133116.121981-6-biju.das.jz@bp.renesas.com/#25706558

Cheers,
Biju

Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

Hi Biju,

> I think it is different here. That 1 msec is delay associated with
> applying hardware power see [1]

Okay, ack.

> I will restore it.

Thanks!

I had meanwhile another thought. What if we kind of merge the two
patches, so the outcome is basically this:

In ov5645_set_register_array:

	If (settings->reg == 0x3008 && settings->val == 0x82)
		usleep_range(1000, 2000)

?

Then, we don't need to split the array and we are also future proof if
we ever need to set the reset bit again somewhere else.

Bonus points for replacing 0x82 with a define :)

What do you think?

Happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> -----Original Message-----
> From: Wolfram Sang <wsa@kernel.org>
> Sent: Wednesday, February 14, 2024 8:31 AM
> Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> ov5645_global_init_setting
> 
> Hi Biju,
> 
> > I think it is different here. That 1 msec is delay associated with
> > applying hardware power see [1]
> 
> Okay, ack.
> 
> > I will restore it.
> 
> Thanks!
> 
> I had meanwhile another thought. What if we kind of merge the two patches,
> so the outcome is basically this:
> 
> In ov5645_set_register_array:
> 
> 	If (settings->reg == 0x3008 && settings->val == 0x82)
> 		usleep_range(1000, 2000)
> 
> ?
> 
> Then, we don't need to split the array and we are also future proof if we
> ever need to set the reset bit again somewhere else.
> 
> Bonus points for replacing 0x82 with a define :)
> 
> What do you think?


OK, this will do check for all other registers.

But from your power down clue and checking ov5640.c
Looks like there are 2 registers changes values after writing.

[1] 0x3008, 0x82-->0x80
[2] 0x0601, 0x02-->0x00

I think [1] is soft reset based on ov5640. Since there is a gpio based hardware reset
available, we can safely remove soft reset[1] and like ov5640.c, if there is no
gpio for reset, then do the soft reset[1].


Then add 1msec delay for power down/up(0x3008: 0x42,0x02) and 0x0601 registers.

With this looks like the Camera works ok @400kHz.

The plans is to add a u8 variable for delay and enable delays for the above registers
and add a check like below

static int ov5645_set_register_array(struct ov5645 *ov5645,
				     const struct reg_value *settings,
				     unsigned int num_settings)
{
	unsigned int i;
	int ret;

	for (i = 0; i < num_settings; ++i, ++settings) {
		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
		if (ret < 0)
			return ret;

		if (settings->delay_ms)
			usleep_range(1000 * settings->delay_ms, 2 * 1000 * settings->delay_ms);
	}

	return 0;
}

Please share your thoughts on this approach.

Cheers,
Biju

Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Sakari Ailus]

Hi Biju,

On Wed, Feb 14, 2024 at 08:25:16PM +0000, Biju Das wrote:
> Hi Wolfram,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: Wolfram Sang <wsa@kernel.org>
> > Sent: Wednesday, February 14, 2024 8:31 AM
> > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> > ov5645_global_init_setting
> > 
> > Hi Biju,
> > 
> > > I think it is different here. That 1 msec is delay associated with
> > > applying hardware power see [1]
> > 
> > Okay, ack.
> > 
> > > I will restore it.
> > 
> > Thanks!
> > 
> > I had meanwhile another thought. What if we kind of merge the two patches,
> > so the outcome is basically this:
> > 
> > In ov5645_set_register_array:
> > 
> > 	If (settings->reg == 0x3008 && settings->val == 0x82)
> > 		usleep_range(1000, 2000)
> > 
> > ?
> > 
> > Then, we don't need to split the array and we are also future proof if we
> > ever need to set the reset bit again somewhere else.
> > 
> > Bonus points for replacing 0x82 with a define :)
> > 
> > What do you think?
> 
> 
> OK, this will do check for all other registers.
> 
> But from your power down clue and checking ov5640.c
> Looks like there are 2 registers changes values after writing.
> 
> [1] 0x3008, 0x82-->0x80
> [2] 0x0601, 0x02-->0x00
> 
> I think [1] is soft reset based on ov5640. Since there is a gpio based
> hardware reset available, we can safely remove soft reset[1] and like
> ov5640.c, if there is no gpio for reset, then do the soft reset[1].

I guess that would work. My understanding is that hard reset control is
mandatory for the device, so there really should be no need for soft reset
in the driver.

> 
> 
> Then add 1msec delay for power down/up(0x3008: 0x42,0x02) and 0x0601
> registers.
> 
> With this looks like the Camera works ok @400kHz.
> 
> The plans is to add a u8 variable for delay and enable delays for the above registers
> and add a check like below
> 
> static int ov5645_set_register_array(struct ov5645 *ov5645,
> 				     const struct reg_value *settings,
> 				     unsigned int num_settings)
> {
> 	unsigned int i;
> 	int ret;
> 
> 	for (i = 0; i < num_settings; ++i, ++settings) {
> 		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
> 		if (ret < 0)
> 			return ret;
> 
> 		if (settings->delay_ms)
> 			usleep_range(1000 * settings->delay_ms, 2 * 1000 * settings->delay_ms);

I'd prefer checking the register address in the write function instead of
this if you really need it. But it seems you don't.

> 	}
> 
> 	return 0;
> }
> 
> Please share your thoughts on this approach.
> 
> Cheers,
> Biju

-- 
Regards,

Sakari Ailus

RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Biju Das]

Hi Sakari,

Thanks for the feedback.

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@linux.intel.com>
> Sent: Wednesday, February 14, 2024 8:49 PM
> To: Biju Das <biju.das.jz@bp.renesas.com>
> Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> ov5645_global_init_setting
> 
> Hi Biju,
> 
> On Wed, Feb 14, 2024 at 08:25:16PM +0000, Biju Das wrote:
> > Hi Wolfram,
> >
> > Thanks for the feedback.
> >
> > > -----Original Message-----
> > > From: Wolfram Sang <wsa@kernel.org>
> > > Sent: Wednesday, February 14, 2024 8:31 AM
> > > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register
> > > 0x3008 from ov5645_global_init_setting
> > >
> > > Hi Biju,
> > >
> > > > I think it is different here. That 1 msec is delay associated with
> > > > applying hardware power see [1]
> > >
> > > Okay, ack.
> > >
> > > > I will restore it.
> > >
> > > Thanks!
> > >
> > > I had meanwhile another thought. What if we kind of merge the two
> > > patches, so the outcome is basically this:
> > >
> > > In ov5645_set_register_array:
> > >
> > > 	If (settings->reg == 0x3008 && settings->val == 0x82)
> > > 		usleep_range(1000, 2000)
> > >
> > > ?
> > >
> > > Then, we don't need to split the array and we are also future proof
> > > if we ever need to set the reset bit again somewhere else.
> > >
> > > Bonus points for replacing 0x82 with a define :)
> > >
> > > What do you think?
> >
> >
> > OK, this will do check for all other registers.
> >
> > But from your power down clue and checking ov5640.c Looks like there
> > are 2 registers changes values after writing.
> >
> > [1] 0x3008, 0x82-->0x80
> > [2] 0x0601, 0x02-->0x00
> >
> > I think [1] is soft reset based on ov5640. Since there is a gpio based
> > hardware reset available, we can safely remove soft reset[1] and like
> > ov5640.c, if there is no gpio for reset, then do the soft reset[1].
> 
> I guess that would work. My understanding is that hard reset control is
> mandatory for the device, so there really should be no need for soft reset
> in the driver.

OK.

> 
> >
> >
> > Then add 1msec delay for power down/up(0x3008: 0x42,0x02) and 0x0601
> > registers.
> >
> > With this looks like the Camera works ok @400kHz.
> >
> > The plans is to add a u8 variable for delay and enable delays for the
> > above registers and add a check like below
> >
> > static int ov5645_set_register_array(struct ov5645 *ov5645,
> > 				     const struct reg_value *settings,
> > 				     unsigned int num_settings)
> > {
> > 	unsigned int i;
> > 	int ret;
> >
> > 	for (i = 0; i < num_settings; ++i, ++settings) {
> > 		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
> > 		if (ret < 0)
> > 			return ret;
> >
> > 		if (settings->delay_ms)
> > 			usleep_range(1000 * settings->delay_ms, 2 * 1000 *
> > settings->delay_ms);
> 
> I'd prefer checking the register address in the write function instead of
> this if you really need it. But it seems you don't.

With delays in powerup/down registers (0x3008 : 0x42,0x02) it is not stable.
So, as you and wolfram mentioned will check the register address instead.

I will post the patch, after testing on various platforms(RZ/{G2L,G2LC,V2L,G2UL}
with 400kHz and 100 kHz
 
Cheers,
Biju

> 
> > 	}
> >
> > 	return 0;
> > }
> >
> > Please share your thoughts on this approach.
> >
> > Cheers,
> > Biju
> 
> --
> Regards,
> 
> Sakari Ailus

RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Biju Das]

Hi Sakari,

> -----Original Message-----
> From: Biju Das
> Sent: Wednesday, February 14, 2024 8:55 PM
> Subject: RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> ov5645_global_init_setting
> 
> Hi Sakari,
> 
> Thanks for the feedback.
> 
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Sent: Wednesday, February 14, 2024 8:49 PM
> > To: Biju Das <biju.das.jz@bp.renesas.com>
> > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008
> > from ov5645_global_init_setting
> >
> > Hi Biju,
> >
> > On Wed, Feb 14, 2024 at 08:25:16PM +0000, Biju Das wrote:
> > > Hi Wolfram,
> > >
> > > Thanks for the feedback.
> > >
> > > > -----Original Message-----
> > > > From: Wolfram Sang <wsa@kernel.org>
> > > > Sent: Wednesday, February 14, 2024 8:31 AM
> > > > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register
> > > > 0x3008 from ov5645_global_init_setting
> > > >
> > > > Hi Biju,
> > > >
> > > > > I think it is different here. That 1 msec is delay associated
> > > > > with applying hardware power see [1]
> > > >
> > > > Okay, ack.
> > > >
> > > > > I will restore it.
> > > >
> > > > Thanks!
> > > >
> > > > I had meanwhile another thought. What if we kind of merge the two
> > > > patches, so the outcome is basically this:
> > > >
> > > > In ov5645_set_register_array:
> > > >
> > > > 	If (settings->reg == 0x3008 && settings->val == 0x82)
> > > > 		usleep_range(1000, 2000)
> > > >
> > > > ?
> > > >
> > > > Then, we don't need to split the array and we are also future
> > > > proof if we ever need to set the reset bit again somewhere else.
> > > >
> > > > Bonus points for replacing 0x82 with a define :)
> > > >
> > > > What do you think?
> > >
> > >
> > > OK, this will do check for all other registers.
> > >
> > > But from your power down clue and checking ov5640.c Looks like there
> > > are 2 registers changes values after writing.
> > >
> > > [1] 0x3008, 0x82-->0x80
> > > [2] 0x0601, 0x02-->0x00
> > >
> > > I think [1] is soft reset based on ov5640. Since there is a gpio
> > > based hardware reset available, we can safely remove soft reset[1]
> > > and like ov5640.c, if there is no gpio for reset, then do the soft
> reset[1].
> >
> > I guess that would work. My understanding is that hard reset control
> > is mandatory for the device, so there really should be no need for
> > soft reset in the driver.
> 
> OK.
> 
> >
> > >
> > >
> > > Then add 1msec delay for power down/up(0x3008: 0x42,0x02) and 0x0601
> > > registers.
> > >
> > > With this looks like the Camera works ok @400kHz.
> > >
> > > The plans is to add a u8 variable for delay and enable delays for
> > > the above registers and add a check like below
> > >
> > > static int ov5645_set_register_array(struct ov5645 *ov5645,
> > > 				     const struct reg_value *settings,
> > > 				     unsigned int num_settings)
> > > {
> > > 	unsigned int i;
> > > 	int ret;
> > >
> > > 	for (i = 0; i < num_settings; ++i, ++settings) {
> > > 		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
> > > 		if (ret < 0)
> > > 			return ret;
> > >
> > > 		if (settings->delay_ms)
> > > 			usleep_range(1000 * settings->delay_ms, 2 * 1000 *
> > > settings->delay_ms);
> >
> > I'd prefer checking the register address in the write function instead
> > of this if you really need it. But it seems you don't.
> 
> With delays in powerup/down registers (0x3008 : 0x42,0x02) it is not
> stable.

Typo: without delays in powerup/down registers (0x3008 : 0x42,0x02) it is not
stable.

You can see 0x42 followed by 0x02 which is an indication of power down/up procedure.

Cheers,
Biju

RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting

[Kieran Bingham]

Quoting Biju Das (2024-02-14 20:57:36)
> Hi Sakari,
> 
> > -----Original Message-----
> > From: Biju Das
> > Sent: Wednesday, February 14, 2024 8:55 PM
> > Subject: RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from
> > ov5645_global_init_setting
> > 
> > Hi Sakari,
> > 
> > Thanks for the feedback.
> > 
> > > -----Original Message-----
> > > From: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > Sent: Wednesday, February 14, 2024 8:49 PM
> > > To: Biju Das <biju.das.jz@bp.renesas.com>
> > > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008
> > > from ov5645_global_init_setting
> > >
> > > Hi Biju,
> > >
> > > On Wed, Feb 14, 2024 at 08:25:16PM +0000, Biju Das wrote:
> > > > Hi Wolfram,
> > > >
> > > > Thanks for the feedback.
> > > >
> > > > > -----Original Message-----
> > > > > From: Wolfram Sang <wsa@kernel.org>
> > > > > Sent: Wednesday, February 14, 2024 8:31 AM
> > > > > Subject: Re: [PATCH 1/2] media: i2c: ov5645: Move the register
> > > > > 0x3008 from ov5645_global_init_setting
> > > > >
> > > > > Hi Biju,
> > > > >
> > > > > > I think it is different here. That 1 msec is delay associated
> > > > > > with applying hardware power see [1]
> > > > >
> > > > > Okay, ack.
> > > > >
> > > > > > I will restore it.
> > > > >
> > > > > Thanks!
> > > > >
> > > > > I had meanwhile another thought. What if we kind of merge the two
> > > > > patches, so the outcome is basically this:
> > > > >
> > > > > In ov5645_set_register_array:
> > > > >
> > > > >         If (settings->reg == 0x3008 && settings->val == 0x82)
> > > > >                 usleep_range(1000, 2000)
> > > > >
> > > > > ?
> > > > >
> > > > > Then, we don't need to split the array and we are also future
> > > > > proof if we ever need to set the reset bit again somewhere else.
> > > > >
> > > > > Bonus points for replacing 0x82 with a define :)

Pulling up the datasheet OV5645-A66A v1.1 from the web:

(a little google-fu++ got there, let me know if you need hints if you
want to do a full clean up)

2.8 reset

The OV5645 sensor includes a RESETB pin that forces a complete hardware
reset when it is pulled low (GND). The OV5645 clears all registers and
resets them to their default values when a hardware reset occurs. A
reset can also be initiated through the SCCB interface by setting
register 0x3008[7] to high.  Manually applying a hard reset upon power
up is required even though on-chip reset is included. The hard reset is
active low with an asynchronized design. The reset pulse width should be
greater than or equal to 1 ms.

2.9 hardware and software standby
Two suspend modes are available for the OV5645:

• hardware standby
• SCCB software standby

To initiate hardware standby mode, the PWDNB pin must be tied to low
(while in MIPI mode, set register 0x300E[4:3] to 2’b11 before the PWDNB
pin is set to low). When this occurs, the OV5645 internal device clock
is halted and all internal counters are reset and registers are
maintained.  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.


 Address: 0x3008 
 RegName: SYSTEM_CTROL0
 Default: 0x02
 R/W:	RW
Description: System Control
  Bit[7]: Software reset
  Bit[6]: Software power down

Then later in the datasheet it also describes:
System Control
Bit[7]: Software reset
Bit[6]: Software power down
Bit[5]: Debug mode
Bit[4]: SRB clock sync enable
Bit[3]: Isolation suspend select
Bit[2]: MIPI reset mask
Bit[1]: MIPI suspend mask
Bit[0]: MIPI reset select


> > > > >
> > > > > What do you think?
> > > >
> > > >
> > > > OK, this will do check for all other registers.
> > > >
> > > > But from your power down clue and checking ov5640.c Looks like there
> > > > are 2 registers changes values after writing.
> > > >
> > > > [1] 0x3008, 0x82-->0x80
> > > > [2] 0x0601, 0x02-->0x00
> > > >
> > > > I think [1] is soft reset based on ov5640. Since there is a gpio
> > > > based hardware reset available, we can safely remove soft reset[1]
> > > > and like ov5640.c, if there is no gpio for reset, then do the soft
> > reset[1].
> > >
> > > I guess that would work. My understanding is that hard reset control
> > > is mandatory for the device, so there really should be no need for
> > > soft reset in the driver.
> > 
> > OK.
> > 
> > >
> > > >
> > > >
> > > > Then add 1msec delay for power down/up(0x3008: 0x42,0x02) and 0x0601
> > > > registers.
> > > >
> > > > With this looks like the Camera works ok @400kHz.
> > > >
> > > > The plans is to add a u8 variable for delay and enable delays for
> > > > the above registers and add a check like below
> > > >
> > > > static int ov5645_set_register_array(struct ov5645 *ov5645,
> > > >                                const struct reg_value *settings,
> > > >                                unsigned int num_settings)
> > > > {
> > > >   unsigned int i;
> > > >   int ret;
> > > >
> > > >   for (i = 0; i < num_settings; ++i, ++settings) {
> > > >           ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
> > > >           if (ret < 0)
> > > >                   return ret;
> > > >
> > > >           if (settings->delay_ms)
> > > >                   usleep_range(1000 * settings->delay_ms, 2 * 1000 *
> > > > settings->delay_ms);
> > >
> > > I'd prefer checking the register address in the write function instead
> > > of this if you really need it. But it seems you don't.
> > 
> > With delays in powerup/down registers (0x3008 : 0x42,0x02) it is not
> > stable.
> 
> Typo: without delays in powerup/down registers (0x3008 : 0x42,0x02) it is not
> stable.
> 
> You can see 0x42 followed by 0x02 which is an indication of power down/up procedure.
> 
> Cheers,
> Biju

[PATCH 2/2] media: i2c: ov5645: Add a small delay after writes in ov5645_set_register_array()

[Biju Das]

Since OV5645 is showing issues @400kHz, it makes sense to add a small
delay after register writes for settling the register values. So introduce
a small delay by adding a read() after write() and also add a debug code
for data mismatch.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
---
 drivers/media/i2c/ov5645.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
index a5cc959d535e..ea9b7c610f2d 100644
--- a/drivers/media/i2c/ov5645.c
+++ b/drivers/media/i2c/ov5645.c
@@ -624,11 +624,20 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
 {
 	unsigned int i;
 	int ret;
+	u8 val;
 
 	for (i = 0; i < num_settings; ++i, ++settings) {
 		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
 		if (ret < 0)
 			return ret;
+
+		ret = ov5645_read_reg(ov5645, settings->reg, &val);
+		if (ret < 0)
+			return ret;
+
+		if (val != settings->val)
+			dev_dbg(ov5645->dev, "Data mismatch reg=%x val=%x/%x\n",
+				settings->reg, settings->val, val);
 	}
 
 	return 0;
-- 
2.25.1

Re: [PATCH 2/2] media: i2c: ov5645: Add a small delay after writes in ov5645_set_register_array()

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

Hi Biju,

On Tue, Feb 13, 2024 at 02:02:40PM +0000, Biju Das wrote:
> Since OV5645 is showing issues @400kHz, it makes sense to add a small
> delay after register writes for settling the register values. So introduce
> a small delay by adding a read() after write() and also add a debug code
> for data mismatch.

That looks not right to me. A write reg should succeed. If it doesn't,
then either the bus speed needs to be reduced on that board, or the
frequency should be measured (maybe the clock on the bus is too fast and
bigger than 400kHz?), or maybe the chip has issues so we need delays
here and there. Just printing debug output if there is a mismatch is
nothing for upstream as well, I'd say. It helps to find out after which
register writes we need delays, but this is only for development, no?

All the best,

   Wolfram

> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> ---
>  drivers/media/i2c/ov5645.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5645.c b/drivers/media/i2c/ov5645.c
> index a5cc959d535e..ea9b7c610f2d 100644
> --- a/drivers/media/i2c/ov5645.c
> +++ b/drivers/media/i2c/ov5645.c
> @@ -624,11 +624,20 @@ static int ov5645_set_register_array(struct ov5645 *ov5645,
>  {
>  	unsigned int i;
>  	int ret;
> +	u8 val;
>  
>  	for (i = 0; i < num_settings; ++i, ++settings) {
>  		ret = ov5645_write_reg(ov5645, settings->reg, settings->val);
>  		if (ret < 0)
>  			return ret;
> +
> +		ret = ov5645_read_reg(ov5645, settings->reg, &val);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (val != settings->val)
> +			dev_dbg(ov5645->dev, "Data mismatch reg=%x val=%x/%x\n",
> +				settings->reg, settings->val, val);
>  	}
>  
>  	return 0;
> -- 
> 2.25.1
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 2/2] media: i2c: ov5645: Add a small delay after writes in ov5645_set_register_array()

[Biju Das]

Hi Wolfram,

Thanks for the feedback.

> -----Original Message-----
> From: Wolfram Sang <wsa@kernel.org>
> Sent: Tuesday, February 13, 2024 10:33 PM
> Subject: Re: [PATCH 2/2] media: i2c: ov5645: Add a small delay after
> writes in ov5645_set_register_array()
> 
> Hi Biju,
> 
> On Tue, Feb 13, 2024 at 02:02:40PM +0000, Biju Das wrote:
> > Since OV5645 is showing issues @400kHz, it makes sense to add a small
> > delay after register writes for settling the register values. So
> > introduce a small delay by adding a read() after write() and also add
> > a debug code for data mismatch.
> 
> That looks not right to me. A write reg should succeed. If it doesn't,
> then either the bus speed needs to be reduced on that board, or the
> frequency should be measured (maybe the clock on the bus is too fast and
> bigger than 400kHz?), 

There is no error for writes. I measured the frequency and it turned out to be 450kHz.
So reduced the frequency and it is around 400kHz now, still faced the issue.


or maybe the chip has issues so we need delays here
> and there. Just printing debug output if there is a mismatch is nothing
> for upstream as well, I'd say. It helps to find out after which register
> writes we need delays, but this is only for development, no?

I would like to drop this patch, instead will go with ov5640.c approach
Add a delay variable in LUT and handle it in write as mentioned in patch#1 ewsponse.

Cheers,
Biju