diff mbox series

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

Message ID 20240213140240.159057-3-biju.das.jz@bp.renesas.com (mailing list archive)
State Superseded
Delegated to: Sakari Ailus
Headers
Series Fix OV5645 capture issue with 400kHz i2c bus frequency |

Commit Message

Biju Das Feb. 13, 2024, 2:02 p.m. UTC 
  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(+)

Comments

Wolfram Sang Feb. 13, 2024, 10:32 p.m. UTC | #1 
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
> 
>
Biju Das Feb. 14, 2024, 8:30 p.m. UTC | #2 
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
diff mbox series

Patch

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;