On 05/08/2021 16:55, Sakari Ailus wrote:
> Hi Seongyong,
>
> On Wed, Jun 09, 2021 at 12:24:51AM +0900, Seongyong Park 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@xxxxxxxxx>
>> ---
>> drivers/media/i2c/video-i2c.c | 13 +++++++++----
>> 1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/media/i2c/video-i2c.c b/drivers/media/i2c/video-i2c.c
>> index 64ba96329..2b50a76f3 100644
>> --- a/drivers/media/i2c/video-i2c.c
>> +++ b/drivers/media/i2c/video-i2c.c
>> @@ -74,7 +74,8 @@ 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 */
>
> This device should actually use a multi-plane format as the data isn't
> Y16_BE as the driver declares. That said, the format would be device
> specific and using one would require specific support from applications.
>
> I might just declare it produces fewer lines while an application that
> knows the device could access the extra data close to the end of the
> buffer.
>
> An alternative would be to use that multi-plane format and keep driver
> support for the plain Y16_BE as well. But I think this would be a bit
> heavy-weight solution.
>
> I wonder what Hans thinks.
I think it is a bit overkill as well.
Wouldn't it be better to just add a new pixel format for this device, and
document it properly? I would keep the existing Y16_BE as it was to avoid
breaking userspace (since adding the extra line breaks the ABI IMHO), and
add a new format that properly documents how to parse the contents of the
buffer, which includes the last line with the status register.
Sorry for the late reply, it looks like an attempt was made to CC me, but
the email address was wrong, so I never received it. I only noticed this
when I started to review this patch.
Regards,
Hans
>
>> };
>>
>> static const struct regmap_config amg88xx_regmap_config = {
>> @@ -168,8 +169,12 @@ 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 + (data->chip->buffer_size - 64),
>
> Please wrap before 80 (unless there's a reason to do otherwise).
>
>> + 64);
>> }
>>
>
