| Message ID | 20240213140240.159057-3-biju.das.jz@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | Fix OV5645 capture issue with 400kHz i2c bus frequency | expand |
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 > >
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 --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;
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(+)