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

[Tomasz Figa <tfiga@xxxxxxxxxxxx>]

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, &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;

> +       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?

Best regards,
Tomasz