Re: [PATCH v2 16/20] media: i2c: ov4689: Set timing registers programmatically

From: Mikhail Rudenko
Date: Fri Feb 23 2024 - 12:04:16 EST


Hi Laurent,

and thanks for the review!

On 2024-02-23 at 13:44 +02, Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxxx>
>
>> +
>> + 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