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

From: Biju Das <biju.das.jz@bp.renesas.com>
To: Wolfram Sang <wsa@kernel.org>
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>,
	biju.das.au <biju.das.au@gmail.com>,
	"linux-renesas-soc@vger.kernel.org"
	<linux-renesas-soc@vger.kernel.org>
Subject: RE: [PATCH 2/2] media: i2c: ov5645: Add a small delay after writes in ov5645_set_register_array()
Date: Wed, 14 Feb 2024 20:30:31 +0000	[thread overview]
Message-ID: <TYCPR01MB11269BFE7A9D3DC605BA2A2A9864E2@TYCPR01MB11269.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <ZcvuCItn1vO6e2CZ@shikoro>

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