Re: [PATCH 2/2] media: video-i2c: append register data on MLX90640's frame

[Matt Ranostay <matt.ranostay@konsulko.com>]

On Sun, May 16, 2021 at 4:09 AM Seongyong Park
<euphoriccatface@gmail.com> wrote:
>
> On MLX90640, Each measurement step updates half of the pixels in the frame
> (every other pixel in default "chess mode", and every other row
> in "interleave mode"), while additional coefficient data (25th & 26th row)
> updates every step. The compensational coefficient data only corresponds
> with the pixels updated in the same step.
>
> Only way to know which "subpage" was updated on the last step is to read
> "status register" on address 0x8000. Without this data,
> compensation calculation may be able to detect which sets of pixels have
> been updated, but it will have to make assumptions when frame skip happens,
> and there is no way to do it correctly when the host simply cannot
> keep up with refresh rate.
>
> Signed-off-by: Seongyong Park <euphoriccatface@gmail.com>
> ---
>  drivers/media/i2c/video-i2c.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
> index 2ccb08335..ca3a1c504 100644
> --- a/drivers/media/i2c/video-i2c.c
> +++ b/drivers/media/i2c/video-i2c.c
> @@ -74,7 +74,7 @@ static const struct v4l2_fmtdesc mlx90640_format = {
>
>  static const struct v4l2_frmsize_discrete mlx90640_size = {
>         .width = 32,
> -       .height = 26, /* 24 lines of pixel data + 2 lines of processing data */
> +       .height = 27, /* 24 lines of pixel data + 2 lines of processing data + 1 line of registers */
>  };
>
>  static const struct regmap_config amg88xx_regmap_config = {
> @@ -168,8 +168,11 @@ static int amg88xx_xfer(struct video_i2c_data *data, char *buf)
>
>  static int mlx90640_xfer(struct video_i2c_data *data, char *buf)
>  {
> -       return regmap_bulk_read(data->regmap, 0x400, buf,
> -                               data->chip->buffer_size);
> +       int ret = regmap_bulk_read(data->regmap, 0x400, buf,
> +                                  data->chip->buffer_size - 64);
> +       if (ret) return ret;
> +       return regmap_bulk_read(data->regmap, 0x8000, buf + 1664,
> +                               64);

Rather than "buf + 1664" it would be more portable and clear if you
use "buf + (data->chip->buffer_size - 64)"

Also isn't the 1 line of registers only 32-bytes not 64-bytes?

- Matt

>  }
>
>  static int amg88xx_setup(struct video_i2c_data *data)
> @@ -375,7 +378,7 @@ static const struct video_i2c_chip video_i2c_chip[] = {
>                 .format         = &mlx90640_format,
>                 .frame_intervals        = mlx90640_frame_intervals,
>                 .num_frame_intervals    = ARRAY_SIZE(mlx90640_frame_intervals),
> -               .buffer_size    = 1664,
> +               .buffer_size    = 1728,
>                 .bpp            = 16,
>                 .regmap_config  = &mlx90640_regmap_config,
>                 .nvmem_config   = &mlx90640_nvram_config,
> --
> 2.31.1
>