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

[Luca Ceresoli]

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@xxxxxxxxxxxxx>
>> ---
>>  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