linux-kernel - Re: [PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all frame sizes

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <effee6cc-680f-3234-2e56-2f6b24d107cd@linaro.org>
Date:   Sun, 7 Jun 2020 19:28:56 +0300
From:   Andrey Konovalov <andrey.konovalov@...aro.org>
To:     Sakari Ailus <sakari.ailus@....fi>
Cc:     mchehab@...nel.org, manivannan.sadhasivam@...aro.org,
        linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        c.barrett@...mos.com, a.brela@...mos.com, peter.griffin@...aro.org
Subject: Re: [PATCH v3 08/10] media: i2c: imx290: Add support to enumerate all
 frame sizes

Hi Sakari,

Thank you for the review!

On 26.05.2020 12:17, Sakari Ailus wrote:
> Hi Andrey,
> 
> On Sun, May 24, 2020 at 10:25:03PM +0300, Andrey Konovalov wrote:
>> From: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
>>
>> Add support to enumerate all frame sizes supported by IMX290. This is
>> required for using with userspace tools such as libcamera.
>>
>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
>> Signed-off-by: Andrey Konovalov <andrey.konovalov@...aro.org>
>> ---
>>   drivers/media/i2c/imx290.c | 20 ++++++++++++++++++++
>>   1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/media/i2c/imx290.c b/drivers/media/i2c/imx290.c
>> index 6e70ff22bc5f..88850f3b1427 100644
>> --- a/drivers/media/i2c/imx290.c
>> +++ b/drivers/media/i2c/imx290.c
>> @@ -471,6 +471,25 @@ static int imx290_enum_mbus_code(struct v4l2_subdev *sd,
>>   	return 0;
>>   }
>>   
>> +static int imx290_enum_frame_size(struct v4l2_subdev *subdev,
>> +				  struct v4l2_subdev_pad_config *cfg,
>> +				  struct v4l2_subdev_frame_size_enum *fse)
>> +{
>> +	if ((fse->code != imx290_formats[0].code) &&
>> +	    (fse->code != imx290_formats[1].code))
>> +		return -EINVAL;
> 
> Please skip the modes that do not have the code specified by the user. They
> should not be enumerated here.

I've double checked this part of the code, and it doesn't seem to need changes.
The reason is that for the both codes the set of the modes and the frame sizes is
exactly the same. And the fse->code check above just guards against the codes not
supported by the driver at all.

Thanks,
Andrey

>> +
>> +	if (fse->index >= ARRAY_SIZE(imx290_modes))
>> +		return -EINVAL;
>> +
>> +	fse->min_width = imx290_modes[fse->index].width;
>> +	fse->max_width = imx290_modes[fse->index].width;
>> +	fse->min_height = imx290_modes[fse->index].height;
>> +	fse->max_height = imx290_modes[fse->index].height;
>> +
>> +	return 0;
>> +}
>> +
>>   static int imx290_get_fmt(struct v4l2_subdev *sd,
>>   			  struct v4l2_subdev_pad_config *cfg,
>>   			  struct v4l2_subdev_format *fmt)
>> @@ -850,6 +869,7 @@ static const struct v4l2_subdev_video_ops imx290_video_ops = {
>>   static const struct v4l2_subdev_pad_ops imx290_pad_ops = {
>>   	.init_cfg = imx290_entity_init_cfg,
>>   	.enum_mbus_code = imx290_enum_mbus_code,
>> +	.enum_frame_size = imx290_enum_frame_size,
>>   	.get_fmt = imx290_get_fmt,
>>   	.set_fmt = imx290_set_fmt,
>>   };
>