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

From: Laurent Pinchart
Date: Fri Feb 23 2024 - 06:44:11 EST

Next message: Thomas Zimmermann: "Re: [PATCH v2 6/9] drm/vkms: Add YUV support"
Previous message: Kirill A. Shutemov: "Re: [RFC][PATCH 12/34] x86/cpu/intel: Actually use "address configuration" infrastructure for MKTME"
Next in thread: Mikhail Rudenko: "Re: [PATCH v2 16/20] media: i2c: ov4689: Set timing registers programmatically"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

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

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);

--
Regards,

Laurent Pinchart

Next message: Thomas Zimmermann: "Re: [PATCH v2 6/9] drm/vkms: Add YUV support"
Previous message: Kirill A. Shutemov: "Re: [RFC][PATCH 12/34] x86/cpu/intel: Actually use "address configuration" infrastructure for MKTME"
Next in thread: Mikhail Rudenko: "Re: [PATCH v2 16/20] media: i2c: ov4689: Set timing registers programmatically"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]