[PATCH v3 0/3] media: i2c: imx471: Add line length PCK setting and derive pixel rate from PLL configuration

[PATCH v3 0/3] media: i2c: imx471: Add line length PCK setting and derive pixel rate from PLL configuration

[Kate Hsuan]

This patchset adds the line length PCK setting and calculates the pixel
rate based on the external clock rate and PLL configurations.

Changes in v3:
1. The line length PCK is set with the hblank control.
2. Define symbolic names for the PLL registers.
3. Fix the register size for the PLL registers. (0x0300-0x0310)
4. Derive pixel rate from the PLL configuration.

The discussion of pixel rate can be found at the following URL
https://lore.kernel.org/linux-media/CAEth8oHj1Jon9m_4aFzRGoKcdopUMym+MyV0JxTo6F+AUFOLoQ@mail.gmail.com/

Changes in v2:
The patchset contains three patches:
1. Add line length PCK setting
2. Name the PLL registers in the OP domain
3. Calculate pixel rate based on the external clock rate and the PLL
   configurations in the OP domain.

The line length PCK setting is added to the sensor driver to ensure the
pixel rate is correct. The pixel rate is calculated based on the external
clock rate. Moreover, since the sensor runs in the PLL DUAL mode, the
configurations of the OP domain are considered.

Tested with libcamera 0.7.2 and the patchset mitigated the horizontal
line noise.

Changes in v1:
1. Add line length PCK setting

Kate Hsuan (3):
  media: i2c: imx471: Configure line length PCK via hblank control
  media: i2c: imx471: Define symbolic names for PLL registers
  media: i2c: imx471: Derive pixel rate from PLL configuration

 drivers/media/i2c/imx471.c | 70 ++++++++++++++++++++++++--------------
 1 file changed, 45 insertions(+), 25 deletions(-)

-- 
2.55.0

[PATCH v3 1/3] media: i2c: imx471: Configure line length PCK via hblank control

[Kate Hsuan]

Implement V4L2_CID_HBLANK control to configure the line Length PCK
register setting. This controls the horizontal line duration
required for precise frame rate and exposure time calculation.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/media/i2c/imx471.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index 4053aed84340..bc6bf11ecf31 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -29,6 +29,10 @@
 #define IMX471_REG_FLL				CCI_REG16(0x0340)
 #define IMX471_FLL_MAX				0xffff
 
+/* H-timing internal */
+#define IMX471_REG_LINE_LENGTH_PCK		CCI_REG16(0x0342)
+#define IMX471_LLP_MAX				0xffff
+
 /* Exposure control */
 #define IMX471_REG_EXPOSURE			CCI_REG16(0x0202)
 #define IMX471_EXPOSURE_MIN			1
@@ -282,7 +286,7 @@ static const struct imx471_mode imx471_modes[] = {
 		.height = 1088,
 		.fll_def = 1308,
 		.fll_min = 1308,
-		.llp = 2328,
+		.llp = 2560,
 		.default_mode_regs = mode_1928x1088_regs,
 		.default_mode_regs_length = ARRAY_SIZE(mode_1928x1088_regs),
 	},
@@ -336,6 +340,10 @@ static int imx471_set_ctrl(struct v4l2_ctrl *ctrl)
 		ret = cci_write(sensor->regmap, IMX471_REG_EXPOSURE,
 				ctrl->val, NULL);
 		break;
+	case V4L2_CID_HBLANK:
+		ret = cci_write(sensor->regmap, IMX471_REG_LINE_LENGTH_PCK,
+				ctrl->val + format->width, NULL);
+		break;
 	case V4L2_CID_VBLANK:
 		/* Update FLL that meets expected vertical blanking */
 		ret = cci_write(sensor->regmap, IMX471_REG_FLL,
@@ -446,12 +454,10 @@ static int imx471_set_pad_format(struct v4l2_subdev *sd,
 		return ret;
 
 	h_blank = mode->llp - mode->width;
-	/*
-	 * Currently hblank is not changeable.
-	 * So FPS control is done only by vblank.
-	 */
 	return __v4l2_ctrl_modify_range(sensor->hblank, h_blank,
-					h_blank, 1, h_blank);
+					IMX471_LLP_MAX - mode->width,
+					1,
+					h_blank);
 }
 
 static int imx471_get_selection(struct v4l2_subdev *sd,
@@ -708,7 +714,8 @@ static int imx471_init_controls(struct imx471 *sensor)
 
 	hblank = mode->llp - mode->width;
 	sensor->hblank = v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
-					   V4L2_CID_HBLANK, hblank, hblank,
+					   V4L2_CID_HBLANK, hblank,
+					   IMX471_LLP_MAX - mode->width,
 					   1, hblank);
 
 	/* fll >= exposure time + adjust parameter (default value is 18) */
@@ -745,7 +752,6 @@ static int imx471_init_controls(struct imx471 *sensor)
 	}
 
 	link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	sensor->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 	sensor->hflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 	sensor->vflip->flags |= V4L2_CTRL_FLAG_MODIFY_LAYOUT;
 
-- 
2.55.0

[PATCH v3 2/3] media: i2c: imx471: Define symbolic names for PLL registers

[Kate Hsuan]

Replace raw register addresses with symbolic macros to improve code
readability. Additionally, fix the register sizes for these PLL
configuration registers to 16 bits.

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/media/i2c/imx471.c | 34 +++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index bc6bf11ecf31..b469fef7eef9 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -68,15 +68,24 @@
 #define IMX471_EXT_CLK				19200000
 
 /* PLL */
-#define IMX471_REG_VTPXCK_DIV			CCI_REG8(0x0301)
-#define IMX471_REG_VTSYCK_DIV			CCI_REG8(0x0303)
-#define IMX471_REG_PREPLLCK_VT_DIV		CCI_REG8(0x0305)
+#define IMX471_REG_VTPXCK_DIV			CCI_REG16(0x0300)
+#define IMX471_REG_VTSYCK_DIV			CCI_REG16(0x0302)
+#define IMX471_REG_PREPLLCK_VT_DIV		CCI_REG16(0x0304)
 #define IMX471_REG_PLL_VT_MPY			CCI_REG16(0x0306)
-#define IMX471_REG_OPPXCK_DIV			CCI_REG8(0x0309)
-#define IMX471_REG_OPSYCK_DIV			CCI_REG8(0x030b)
+#define IMX471_REG_OPPXCK_DIV			CCI_REG16(0x0308)
+#define IMX471_REG_OPSYCK_DIV			CCI_REG16(0x030a)
+#define IMX471_REG_OP_PREPLLCK_DIV		CCI_REG16(0x030c)
+#define IMX471_REG_OP_MPY			CCI_REG16(0x030e)
 #define IMX471_REG_PLL_MULT_DRIV		CCI_REG8(0x0310)
 #define IMX471_PLL_SINGLE			0
 #define IMX471_PLL_DUAL				1
+#define IMX471_VTPXCK_DIV			6
+#define IMX471_VTSYCK_DIV			2
+#define IMX471_PREPLLCK_VT_DIV			2
+#define IMX471_PLL_VT_MPY			121
+#define IMX471_OPSYCK_DIV			1
+#define IMX471_PLL_OP_MPY			83
+#define IMX471_PREPLLCK_OP_DIV			2
 
 /* IMX471 native and active pixel array size */
 #define IMX471_NATIVE_WIDTH			4672
@@ -236,14 +245,13 @@ static const struct cci_reg_sequence mode_1928x1088_regs[] = {
 	{ IMX471_REG_DIG_CROP_HEIGHT, 1088 },
 	{ IMX471_REG_X_OUTPUT_SIZE, 1928 },
 	{ IMX471_REG_Y_OUTPUT_SIZE, 1088 },
-	{ IMX471_REG_VTPXCK_DIV, 0x06 },
-	{ IMX471_REG_VTSYCK_DIV, 0x02 },
-	{ IMX471_REG_PREPLLCK_VT_DIV, 0x02 },
-	{ IMX471_REG_PLL_VT_MPY, 0x0079 },
-	{ IMX471_REG_OPSYCK_DIV, 0x01 },
-	{ CCI_REG8(0x030d), 0x02 },
-	{ CCI_REG8(0x030e), 0x00 },
-	{ CCI_REG8(0x030f), 0x53 },
+	{ IMX471_REG_VTPXCK_DIV, IMX471_VTPXCK_DIV },
+	{ IMX471_REG_VTSYCK_DIV, IMX471_VTSYCK_DIV },
+	{ IMX471_REG_PREPLLCK_VT_DIV, IMX471_PREPLLCK_VT_DIV },
+	{ IMX471_REG_PLL_VT_MPY, IMX471_PLL_VT_MPY },
+	{ IMX471_REG_OPSYCK_DIV, IMX471_OPSYCK_DIV },
+	{ IMX471_REG_OP_PREPLLCK_DIV, IMX471_PREPLLCK_OP_DIV },
+	{ IMX471_REG_OP_MPY, IMX471_PLL_OP_MPY },
 	{ IMX471_REG_PLL_MULT_DRIV, IMX471_PLL_DUAL },
 	{ CCI_REG8(0x3f4c), 0x81 },
 	{ CCI_REG8(0x3f4d), 0x81 },
-- 
2.55.0

[PATCH v3 3/3] media: i2c: imx471: Derive pixel rate from PLL configuration

[Kate Hsuan]

Derive the pixel rate from the sensor's external clock frequency and
active PLL settings.

The discussion of pixel rate can be found at the URL
Link: https://lore.kernel.org/linux-media/CAEth8oHj1Jon9m_4aFzRGoKcdopUMym+MyV0JxTo6F+AUFOLoQ@mail.gmail.com/

Signed-off-by: Kate Hsuan <hpa@redhat.com>
---
 drivers/media/i2c/imx471.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx471.c b/drivers/media/i2c/imx471.c
index b469fef7eef9..fc6a12943991 100644
--- a/drivers/media/i2c/imx471.c
+++ b/drivers/media/i2c/imx471.c
@@ -65,7 +65,7 @@
 
 /* default link frequency and external clock */
 #define IMX471_LINK_FREQ_DEFAULT		200000000LL
-#define IMX471_EXT_CLK				19200000
+#define IMX471_EXT_CLK				19200000LL
 
 /* PLL */
 #define IMX471_REG_VTPXCK_DIV			CCI_REG16(0x0300)
@@ -704,9 +704,15 @@ static int imx471_init_controls(struct imx471 *sensor)
 					   ARRAY_SIZE(link_freq_menu_items) - 1,
 					   0,
 					   link_freq_menu_items);
-
-	/* pixel_rate = link_freq * 2 * nr_of_lanes / bits_per_sample */
-	pixel_rate = div_u64(IMX471_LINK_FREQ_DEFAULT * 2 * 4, 10);
+	/*
+	 * The sensor runs in the dual mode so the pixel rate is defined as
+	 * follows:
+	 * (ext_freq * IVT_PLL_MPY * num_of_vt_lanes) /
+	 * (VT_PREPLLCK_DIV * IVT_SYCK_DIV * IVT_PXCK_DIV)
+	 */
+	pixel_rate = (IMX471_EXT_CLK * IMX471_PLL_VT_MPY * 4) /
+		     (IMX471_PREPLLCK_VT_DIV * IMX471_VTSYCK_DIV *
+		      IMX471_VTPXCK_DIV);
 
 	v4l2_ctrl_new_std(ctrl_hdlr, &imx471_ctrl_ops,
 			  V4L2_CID_PIXEL_RATE, pixel_rate,
-- 
2.55.0