Hi Jason,
On Mon, Jan 22, 2024 at 8:54 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 | 1199 ++---------------------------------
> 1 file changed, 47 insertions(+), 1152 deletions(-)
>
Thanks for the patch! Please see my comments inline.
> diff --git a/drivers/media/i2c/ov08x40.c b/drivers/media/i2c/ov08x40.c
> index ddcb4b6848b..839c87f1a25 100644
> --- a/drivers/media/i2c/ov08x40.c
> +++ b/drivers/media/i2c/ov08x40.c
[snip]
> +static int ov08x40_burst_write_regs(struct ov08x40 *ov08x, u16 reg, u8 val, size_t num_regs)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&ov08x->sd);
> + struct i2c_msg *msgs;
> + __be16 reg_addr_be = cpu_to_be16(reg);
> + size_t i;
> + int ret;
> +
> + msgs = kmalloc_array(num_regs, sizeof(struct i2c_msg), GFP_KERNEL);
Shouldn't this be num_regs + 1 to also allocate the initial msg for
the register address?
> + if (!msgs)
> + return -ENOMEM;
> +
> + /* Set up the first message for the register address */
> + msgs[0].addr = client->addr;
> + msgs[0].flags = 0;
> + msgs[0].len = 2;
> + msgs[0].buf = (u8 *)®_addr_be;
> +
> + /* Set up the subsequent messages for the data value */
> + for (i = 1; i <= num_regs; ++i) {
> + msgs[i].addr = client->addr;
> + msgs[i].flags = 0;
> + msgs[i].len = 1;
> + msgs[i].buf = &val;
> + }
> +
> + ret = i2c_transfer(client->adapter, msgs, num_regs);
Are we sure this actually works as intended? i2c_transfer() will issue
a repeated start before every subsequent message, but the sensor
datasheet mentions just subsequent bytes without repeated starts for
sequential write (ov08x40 datasheet 2.01 page 2-18 aka 36).
Also, shouldn't the last argument be num_regs + 1?
> +
> + kfree(msgs);
> +
> + if (ret != num_regs) {
> + dev_err(&client->dev, "Only %d out of %zu msgs have transferred successfully\n",
> + ret, num_regs);
> + 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 +1825,12 @@ 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) {
> + ov08x40_burst_write_regs(ov08x, 0x5a80, 0x75, 0x5b9f - 0x5a80 + 1);
> + ov08x40_burst_write_regs(ov08x, 0x5bc0, 0x75, 0x5f1f - 0x5bc0 + 1);
Please define these magic numbers as macros.
Also, would it make sense to make this function prototype something like:
ov0x840_burst_fill_regs(..., first reg, last reg, val)
so we don't have to do the length calculation by hand in every call?
(and the function effectively fills the registers, not writes with
their own values, so the name is more appropriate too)
Also, please add error handling.
Best regards,
Tomasz
