linux-kernel - Re: [PATCH] media: imx274: add support for sensor mode6, 1280x540

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] [day] [month] [year] [list]

Message-ID: <de057619-f314-58d8-2000-aad0cd052bba@lucaceresoli.net>
Date:   Mon, 14 Sep 2020 09:48:13 +0200
From:   Luca Ceresoli <luca@...aceresoli.net>
To:     Sakari Ailus <sakari.ailus@....fi>,
        Eugen Hristev <eugen.hristev@...rochip.com>
Cc:     leonl@...pardimaging.com, linux-media@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] media: imx274: add support for sensor mode6, 1280x540

Hi,

On 11/09/20 23:44, Sakari Ailus wrote:
> Hi Eugen,
> 
> Thanks for the patch.
> 
> On Wed, Sep 09, 2020 at 01:53:28PM +0300, Eugen Hristev wrote:
>> Add support for the mode 6 for the sensor, this mode uses
>> 3/8 subsampling and 3 horizontal binning.
>> Aspect ratio is changed.
>> Split the bin_ratio variable into two parts, one for
>> width and one for height, as the ratio is no longer preserved
>> when doing subsampling in this mode.
>>
>> Signed-off-by: Eugen Hristev <eugen.hristev@...rochip.com>
>> ---
>>  drivers/media/i2c/imx274.c | 89 +++++++++++++++++++++++++++++++-------
>>  1 file changed, 73 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/media/i2c/imx274.c b/drivers/media/i2c/imx274.c
>> index e6aa9f32b6a8..5adb11f036e2 100644
>> --- a/drivers/media/i2c/imx274.c
>> +++ b/drivers/media/i2c/imx274.c
>> @@ -147,8 +147,9 @@ static const struct regmap_config imx274_regmap_config = {
>>  
>>  enum imx274_binning {
>>  	IMX274_BINNING_OFF,
>> -	IMX274_BINNING_2_1,
>> -	IMX274_BINNING_3_1,
>> +	IMX274_BINNING_2_2,
>> +	IMX274_BINNING_3_3,
>> +	IMX274_BINNING_4_3,
> 
> This enum is unused (apart from the useless use of the first value). Please
> remove it (may be a new patch).

I agree.

[...]

>> @@ -454,7 +498,8 @@ static const struct imx274_mode imx274_modes[] = {
>>  	},
>>  	{
>>  		/* mode 3, 1080p */
>> -		.bin_ratio = 2,
>> +		.wbin_ratio = 2, /* 1620 */

s/1620/1920/

[...]

>> @@ -906,14 +963,14 @@ static int __imx274_change_compose(struct stimx274 *imx274,
>>  		}
>>  	}
>>  
>> -	*width = cur_crop->width / best_mode->bin_ratio;
>> -	*height = cur_crop->height / best_mode->bin_ratio;
>> +	*width = cur_crop->width / best_mode->wbin_ratio;
>> +	*height = cur_crop->height / best_mode->hbin_ratio;
>>  
>>  	if (which == V4L2_SUBDEV_FORMAT_ACTIVE)
>>  		imx274->mode = best_mode;
>>  
>> -	dev_dbg(dev, "%s: selected %u:1 binning\n",
>> -		__func__, best_mode->bin_ratio);
>> +	dev_dbg(dev, "%s: selected %u:%u binning\n",
> 
> I think the earlier message suggested it was ratio. How about "x"
> instead of a colon?
> 
> Apart from these looks good to me.

LGTM too.

-- 
Luca