Re: [PATCH v3] media: ov08x40: Reduce start streaming time

[Sakari Ailus <sakari.ailus@linux.intel.com>]

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@intel.com> wrote:
> >
> > From: Jason Chen <jason.z.chen@intel.com>
> >
> > 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@intel.com>
> > ---
> >  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, &reg_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