Received-SPF: pass (google.com: domain of linux-kernel+bounces-78782-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
References: <20231218174042.794012-1-mike.rudenko@gmail.com>
 <20231218174042.794012-17-mike.rudenko@gmail.com>
 <20240223114403.GT31348@pendragon.ideasonboard.com>
User-agent: mu4e 1.10.8; emacs 29.2.50
From: Mikhail Rudenko <mike.rudenko@gmail.com>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: linux-media@vger.kernel.org, linux-kernel@vger.kernel.org, Sakari Ailus
 <sakari.ailus@linux.intel.com>, Jacopo Mondi <jacopo@jmondi.org>, Tommaso
 Merciai <tomm.merciai@gmail.com>, Christophe JAILLET
 <christophe.jaillet@wanadoo.fr>, Dave Stevenson
 <dave.stevenson@raspberrypi.com>, Mauro Carvalho Chehab
 <mchehab@kernel.org>
Subject: Re: [PATCH v2 16/20] media: i2c: ov4689: Set timing registers
 programmatically
Date: Fri, 23 Feb 2024 19:34:06 +0300
In-reply-to: <20240223114403.GT31348@pendragon.ideasonboard.com>
Message-ID: <87v86fcdtq.fsf@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain


Hi Laurent,

and thanks for the review!

On 2024-02-23 at 13:44 +02, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:

> Hi Mikhail,
>
> Thank you for the patch.
>
> On Mon, Dec 18, 2023 at 08:40:37PM +0300, Mikhail Rudenko wrote:
>> Set timing-related and BLC anchor registers via cci calls instead of
>> hardcoding them in the register table. This prepares the driver for
>> implementation of configurable analogue crop and binning. No
>> functional change intended.
>>
>> Signed-off-by: Mikhail Rudenko <mike.rudenko@gmail.com>
>> ---
>>  drivers/media/i2c/ov4689.c | 83 +++++++++++++++++++++++++++++++-------
>>  1 file changed, 68 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/media/i2c/ov4689.c b/drivers/media/i2c/ov4689.c
>> index 475508559e3e..3b73ee282761 100644
>> --- a/drivers/media/i2c/ov4689.c
>> +++ b/drivers/media/i2c/ov4689.c
>> @@ -41,6 +41,13 @@
>>  #define OV4689_DIG_GAIN_STEP		1
>>  #define OV4689_DIG_GAIN_DEFAULT		0x800
>>
>> +#define OV4689_REG_H_CROP_START		CCI_REG16(0x3800)
>> +#define OV4689_REG_V_CROP_START		CCI_REG16(0x3802)
>> +#define OV4689_REG_H_CROP_END		CCI_REG16(0x3804)
>> +#define OV4689_REG_V_CROP_END		CCI_REG16(0x3806)
>> +#define OV4689_REG_H_OUTPUT_SIZE	CCI_REG16(0x3808)
>> +#define OV4689_REG_V_OUTPUT_SIZE	CCI_REG16(0x380a)
>> +
>>  #define OV4689_REG_HTS			CCI_REG16(0x380c)
>>  #define OV4689_HTS_DIVIDER		4
>>  #define OV4689_HTS_MAX			0x7fff
>> @@ -48,6 +55,9 @@
>>  #define OV4689_REG_VTS			CCI_REG16(0x380e)
>>  #define OV4689_VTS_MAX			0x7fff
>>
>> +#define OV4689_REG_H_WIN_OFF		CCI_REG16(0x3810)
>> +#define OV4689_REG_V_WIN_OFF		CCI_REG16(0x3812)
>> +
>>  #define OV4689_REG_TIMING_FORMAT1	CCI_REG8(0x3820)
>>  #define OV4689_REG_TIMING_FORMAT2	CCI_REG8(0x3821)
>>  #define OV4689_TIMING_FLIP_MASK		GENMASK(2, 1)
>> @@ -56,6 +66,17 @@
>>  #define OV4689_TIMING_FLIP_BOTH		(OV4689_TIMING_FLIP_ARRAY |\
>>  					 OV4689_TIMING_FLIP_DIGITAL)
>>
>> +#define OV4689_REG_ANCHOR_LEFT_START	CCI_REG16(0x4020)
>> +#define OV4689_ANCHOR_LEFT_START_DEF	576
>> +#define OV4689_REG_ANCHOR_LEFT_END	CCI_REG16(0x4022)
>> +#define OV4689_ANCHOR_LEFT_END_DEF	831
>> +#define OV4689_REG_ANCHOR_RIGHT_START	CCI_REG16(0x4024)
>> +#define OV4689_ANCHOR_RIGHT_START_DEF	1984
>> +#define OV4689_REG_ANCHOR_RIGHT_END	CCI_REG16(0x4026)
>> +#define OV4689_ANCHOR_RIGHT_END_DEF	2239
>> +
>> +#define OV4689_REG_VFIFO_CTRL_01	CCI_REG8(0x4601)
>> +
>>  #define OV4689_REG_WB_GAIN_RED		CCI_REG16(0x500c)
>>  #define OV4689_REG_WB_GAIN_BLUE		CCI_REG16(0x5010)
>>  #define OV4689_WB_GAIN_MIN		1
>> @@ -199,10 +220,6 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
>>  	{CCI_REG8(0x3798), 0x1b},
>>
>>  	/* Timing control */
>> -	{CCI_REG8(0x3801), 0x08}, /* H_CROP_START_L h_crop_start[7:0] = 0x08 */
>> -	{CCI_REG8(0x3805), 0x97}, /* H_CROP_END_L h_crop_end[7:0] = 0x97 */
>> -	{CCI_REG8(0x3811), 0x08}, /* H_WIN_OFF_L h_win_off[7:0] = 0x08*/
>> -	{CCI_REG8(0x3813), 0x04}, /* V_WIN_OFF_L v_win_off[7:0] = 0x04 */
>>  	{CCI_REG8(0x3819), 0x01}, /* VSYNC_END_L vsync_end_point[7:0] = 0x01 */
>>
>>  	/* OTP control */
>> @@ -218,22 +235,11 @@ static const struct cci_reg_sequence ov4689_2688x1520_regs[] = {
>>  	{CCI_REG8(0x401b), 0x00}, /* DEBUG_MODE */
>>  	{CCI_REG8(0x401d), 0x00}, /* DEBUG_MODE */
>>  	{CCI_REG8(0x401f), 0x00}, /* DEBUG_MODE */
>> -	{CCI_REG8(0x4020), 0x00}, /* ANCHOR_LEFT_START_H anchor_left_start[11:8] = 0 */
>> -	{CCI_REG8(0x4021), 0x10}, /* ANCHOR_LEFT_START_L anchor_left_start[7:0] = 0x10 */
>> -	{CCI_REG8(0x4022), 0x07}, /* ANCHOR_LEFT_END_H anchor_left_end[11:8] = 0x07 */
>> -	{CCI_REG8(0x4023), 0xcf}, /* ANCHOR_LEFT_END_L anchor_left_end[7:0] = 0xcf */
>> -	{CCI_REG8(0x4024), 0x09}, /* ANCHOR_RIGHT_START_H anchor_right_start[11:8] = 0x09 */
>> -	{CCI_REG8(0x4025), 0x60}, /* ANCHOR_RIGHT_START_L anchor_right_start[7:0] = 0x60 */
>> -	{CCI_REG8(0x4026), 0x09}, /* ANCHOR_RIGHT_END_H anchor_right_end[11:8] = 0x09 */
>> -	{CCI_REG8(0x4027), 0x6f}, /* ANCHOR_RIGHT_END_L anchor_right_end[7:0] = 0x6f */
>>
>>  	/* ADC sync control */
>>  	{CCI_REG8(0x4500), 0x6c}, /* ADC_SYNC_CTRL */
>>  	{CCI_REG8(0x4503), 0x01}, /* ADC_SYNC_CTRL */
>>
>> -	/* VFIFO */
>> -	{CCI_REG8(0x4601), 0xa7}, /* VFIFO_CTRL_01 r_vfifo_read_start[7:0] = 0xa7 */
>> -
>>  	/* Temperature monitor */
>>  	{CCI_REG8(0x4d00), 0x04}, /* TPM_CTRL_00 tmp_slope[15:8] = 0x04 */
>>  	{CCI_REG8(0x4d01), 0x42}, /* TPM_CTRL_01 tmp_slope[7:0] = 0x42 */
>> @@ -406,6 +412,41 @@ static int ov4689_get_selection(struct v4l2_subdev *sd,
>>  	return -EINVAL;
>>  }
>>
>> +static int ov4689_setup_timings(struct ov4689 *ov4689)
>> +{
>> +	const struct ov4689_mode *mode = ov4689->cur_mode;
>> +	struct regmap *rm = ov4689->regmap;
>> +	int ret = 0;
>> +
>> +	cci_write(rm, OV4689_REG_H_CROP_START, 8, &ret);
>> +	cci_write(rm, OV4689_REG_V_CROP_START, 8, &ret);
>> +	cci_write(rm, OV4689_REG_H_CROP_END, 2711, &ret);
>> +	cci_write(rm, OV4689_REG_V_CROP_END, 1531, &ret);
>
> This is interesting. The previous patch defines
>
> #define OV4689_PIXEL_ARRAY_WIDTH	2720
> #define OV4689_PIXEL_ARRAY_HEIGHT	1536
> #define OV4689_DUMMY_ROWS		8
> #define OV4689_DUMMY_COLUMNS		16
>
> and the (only) mode has
>
> 	.width = 2688,
> 	.height = 1520,
>
> The above register values should result in an analog crop rectangle size
> of 2704x1524. Then, the digital crop is configured below to
> (8,4)/2688x1520. The combined crop rectangle, relative to the pixel
> array, is thus (16,12)/2688x1520. This centers the crop rectangle
> horizontally but not vertically. I wonder why, and I also wonder why
> there's a need to apply both analog crop and digital crop, instead of
> setting
>
> 	OV4689_REG_H_CROP_START = 16
> 	OV4689_REG_V_CROP_START = 8
> 	OV4689_REG_H_CROP_END = 2703
> 	OV4689_REG_V_CROP_END = 1527
>
> 	OV4689_REG_H_WIN_OFF = 0
> 	OV4689_REG_V_WIN_OFF = 0
> 	OV4689_REG_H_OUTPUT_SIZE = 2688
> 	OV4689_REG_V_OUTPUT_SIZE = 1520

The only reason for this is purely historical: these cropping/windowing
settings were used in the register-table-based driver from Rockchip BSP,
and there was no reason to change them until now. In fact, the next
patch ([PATCH v2 17/20] media: i2c: ov4689: Configurable analogue crop)
does exactly what you suggest. :)

> Anyway, this is not an issue introduced by this patch, so
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
>> +
>> +	cci_write(rm, OV4689_REG_H_OUTPUT_SIZE, mode->width, &ret);
>> +	cci_write(rm, OV4689_REG_V_OUTPUT_SIZE, mode->height, &ret);
>> +
>> +	cci_write(rm, OV4689_REG_H_WIN_OFF, 8, &ret);
>> +	cci_write(rm, OV4689_REG_V_WIN_OFF, 4, &ret);
>> +
>> +	cci_write(rm, OV4689_REG_VFIFO_CTRL_01, 167, &ret);
>> +
>> +	return ret;
>> +}
>> +
>> +static int ov4689_setup_blc_anchors(struct ov4689 *ov4689)
>> +{
>> +	struct regmap *rm = ov4689->regmap;
>> +	int ret = 0;
>> +
>> +	cci_write(rm, OV4689_REG_ANCHOR_LEFT_START, 16, &ret);
>> +	cci_write(rm, OV4689_REG_ANCHOR_LEFT_END, 1999, &ret);
>> +	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_START, 2400, &ret);
>> +	cci_write(rm, OV4689_REG_ANCHOR_RIGHT_END, 2415, &ret);
>> +
>> +	return ret;
>> +}
>> +
>>  static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
>>  {
>>  	struct ov4689 *ov4689 = to_ov4689(sd);
>> @@ -429,6 +470,18 @@ static int ov4689_s_stream(struct v4l2_subdev *sd, int on)
>>  			goto unlock_and_return;
>>  		}
>>
>> +		ret = ov4689_setup_timings(ov4689);
>> +		if (ret) {
>> +			pm_runtime_put(dev);
>> +			goto unlock_and_return;
>> +		}
>> +
>> +		ret = ov4689_setup_blc_anchors(ov4689);
>> +		if (ret) {
>> +			pm_runtime_put(dev);
>> +			goto unlock_and_return;
>> +		}
>> +
>>  		ret = __v4l2_ctrl_handler_setup(&ov4689->ctrl_handler);
>>  		if (ret) {
>>  			pm_runtime_put_sync(dev);


--
Best regards,
Mikhail Rudenko