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

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Wolfram Sang <wsa@kernel.org>, biju.das.au <biju.das.au@gmail.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	Geert Uytterhoeven <geert+renesas@glider.be>,
	Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH 1/2] media: i2c: ov5645: Move the register 0x3008 from ov5645_global_init_setting
Date: Wed, 14 Feb 2024 20:25:16 +0000	[thread overview]
Message-ID: <TYCPR01MB11269CC8B2EAB564154C829A2864E2@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <Zcx6Ty2tu_ZGdURj@ninjato>

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