Hi Tomasz,
On Tue, Jan 23, 2024 at 07:03:49PM +0900, Tomasz Figa wrote:
> On Tue, Jan 23, 2024 at 6:37 PM Chen, Jason Z <jason.z.chen@xxxxxxxxx> wrote:
> >
> > From: Jason Chen <jason.z.chen@xxxxxxxxx>
> >
> > Because video duration involves calculating the streaming time, and i2c
> > communication incurs too many XTALK register settings every 4 bytes with
> > i2c START and STOP.
> >
> > So we have opted switch to the i2c burst method.
> > This method involves writing the XTALK registers in the order of
> > the register block.
> >
> > The start streaming time can be reduced from around 400ms to 150ms
> >
> > Signed-off-by: Jason Chen <jason.z.chen@xxxxxxxxx>
> > ---
> > drivers/media/i2c/ov08x40.c | 1207 ++---------------------------------
> > 1 file changed, 55 insertions(+), 1152 deletions(-)
> >
> > diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
> > index ddcb4b6848b..7b09f405fc5 100644
> > --- a/drivers/media/i2c/ov08x40.c
> > +++ b/drivers/media/i2c/ov08x40.c
> [snip]
> > +static int ov08x40_burst_fill_regs(struct ov08x40 *ov08x, u16 first_reg,
> > + u16 last_reg, u8 val)
> > +{
> > + struct i2c_client *client = v4l2_get_subdevdata(&ov08x->sd);
> > + struct i2c_msg msgs;
> > + __be16 reg_addr_be = cpu_to_be16(first_reg);
> > + size_t i, num_regs;
> > + int ret;
> > +
> > + num_regs = last_reg - first_reg + 1;
> > + msgs.addr = client->addr;
> > + msgs.flags = 0;
> > + msgs.len = 2 + num_regs;
> > + msgs.buf = kmalloc(msgs.len, GFP_KERNEL);
> > + if (!msgs.buf)
> > + return -ENOMEM;
> > +
> > + /* Set the register address to the first two bytes of the buffer */
> > + memcpy(msgs.buf, ®_addr_be, 2);
>
> nit: Wouldn't the code be much simpler to follow if rewritten as below?
>
> msgs.buf[0] = first_reg >> 8;
> msgs.buf[1] = first_reg & 0xff;
Or... just put_unaligned_be16()?
>
> > + for (i = 0; i < num_regs; ++i)
> > + msgs.buf[2 + i] = val;
> > +
> > + ret = i2c_transfer(client->adapter, &msgs, 1);
> > +
> > + kfree(msgs.buf);
> > +
> > + if (ret != 1) {
> > + dev_err(&client->dev, "Failed %d regs transferred: %d\n", ret);
> > + return -EIO;
> > + }
> > +
> > + dev_dbg(&client->dev, "I2C burst transfer succeeded\n");
> > +
> > + return 0;
> > +}
> > +
> > /* Write registers up to 4 at a time */
> > static int ov08x40_write_reg(struct ov08x40 *ov08x,
> > u16 reg, u32 len, u32 __val)
> > @@ -2936,6 +1826,19 @@ static int ov08x40_start_streaming(struct ov08x40 *ov08x)
> > return ret;
> > }
> >
> > + /* Use i2c burst to write register on full size registers */
> > + if (ov08x->cur_mode->exposure_shift == 1) {
> > + ret = ov08x40_burst_fill_regs(ov08x, OV08X40_REG_XTALK_FIRST_A,
> > + OV08X40_REG_XTALK_LAST_A, 0x75);
> > + ret = ov08x40_burst_fill_regs(ov08x, OV08X40_REG_XTALK_FIRST_B,
> > + OV08X40_REG_XTALK_LAST_B, 0x75);
> > + }
>
> Hmm, if we only need to set these if exposure_shift is 1, don't we
> need to somehow "unset" them if the mode exposure_shift != 1?
I recall the driver powers the sensor off every time streaming is stopped
(without pm_runtime_put_sync(), so it's not reliable)... that would be nice
to address as well, but outside the scope of this patch.
--
Regards,
Sakari Ailus
