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.hris...@microchip.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
