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

From: Wolfram Sang <wsa@kernel.org>
To: Biju Das <biju.das.jz@bp.renesas.com>
Cc: Sakari Ailus <sakari.ailus@linux.intel.com>,
	Mauro Carvalho Chehab <mchehab@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 <biju.das.au@gmail.com>,
	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: Tue, 13 Feb 2024 23:32:40 +0100	[thread overview]
Message-ID: <ZcvuCItn1vO6e2CZ@shikoro> (raw)
In-Reply-To: <20240213140240.159057-3-biju.das.jz@bp.renesas.com>

[-- 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 --]