[PATCH v2 0/4] media: i2c: imx214: Add support for more clock frequencies

[PATCH v2 0/4] media: i2c: imx214: Add support for more clock frequencies

[André Apitzsch via B4 Relay]

The imx214 driver currently supports only a 24 MHz external clock. But
there are devices, like Qualcomm-MSM8916-based phones, which cannot
provide this frequency. To make the sensor usable by those devices, add
support for additional clock frequencies.

This series supersedes
https://lore.kernel.org/linux-media/20250308-imx214_clk_freq-v1-0-467a4c083c35@apitzsch.eu/

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
Changes in v2:
- Add A-b tags
- Switch to v4l2_ctrl_s_ctrl_int64() to acquire the control handler mutex
- Add error handling for v4l2_ctrl_s_ctrl_int64() and
  imx214_pll_update()
- Replace "read clock frequency from dt" patch by "remove hard-coded
  external clock frequency" patch
- Link to v1:
  https://lore.kernel.org/r/20250415-imx214_ccs_pll-v1-0-d3d7748e5fbd@apitzsch.eu

---
André Apitzsch (4):
      media: i2c: imx214: Reorder imx214_parse_fwnode call
      media: i2c: imx214: Prepare for variable clock frequency
      media: i2c: imx214: Make use of CCS PLL calculator
      media: i2c: imx214: Remove hard-coded external clock frequency

 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/imx214.c | 266 ++++++++++++++++++++++++++++++++++-----------
 2 files changed, 204 insertions(+), 63 deletions(-)
---
base-commit: 4d4cc73d2632e07451c98c77fdb04aead3f78b00
change-id: 20250406-imx214_ccs_pll-e4aed0e9e532

Best regards,
-- 
André Apitzsch <git@apitzsch.eu>

[PATCH v2 1/4] media: i2c: imx214: Reorder imx214_parse_fwnode call

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

Reorder imx214_parse_fwnode call to reduce goto paths in upcoming
patches.

No functional change intended.

Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/media/i2c/imx214.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index dd7bc45523d8b5fcb3ec95728a6d32c4fddede72..0199195dcb7d12dc2ff253fe3eb77ddbcd0812a9 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -1261,10 +1261,6 @@ static int imx214_probe(struct i2c_client *client)
 	struct imx214 *imx214;
 	int ret;
 
-	ret = imx214_parse_fwnode(dev);
-	if (ret)
-		return ret;
-
 	imx214 = devm_kzalloc(dev, sizeof(*imx214), GFP_KERNEL);
 	if (!imx214)
 		return -ENOMEM;
@@ -1295,6 +1291,10 @@ static int imx214_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(imx214->regmap),
 				     "failed to initialize CCI\n");
 
+	ret = imx214_parse_fwnode(dev);
+	if (ret)
+		return ret;
+
 	v4l2_i2c_subdev_init(&imx214->sd, client, &imx214_subdev_ops);
 	imx214->sd.internal_ops = &imx214_internal_ops;
 

-- 
2.49.0

[PATCH v2 2/4] media: i2c: imx214: Prepare for variable clock frequency

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

Move clock frequency related parameters out of the constant register
sequences, such that the hard coded external clock frequency can be
replaced by a variable in the upcoming patches.

Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/media/i2c/imx214.c | 54 ++++++++++++++++++++++++++--------------------
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 0199195dcb7d12dc2ff253fe3eb77ddbcd0812a9..3aca6ebb02d649c1b7f0b6a6049c1e3aa3d08951 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -299,16 +299,6 @@ static const struct cci_reg_sequence mode_4096x2304[] = {
 	{ IMX214_REG_DIG_CROP_WIDTH, 4096 },
 	{ IMX214_REG_DIG_CROP_HEIGHT, 2304 },
 
-	{ IMX214_REG_VTPXCK_DIV, 5 },
-	{ IMX214_REG_VTSYCK_DIV, 2 },
-	{ IMX214_REG_PREPLLCK_VT_DIV, 3 },
-	{ IMX214_REG_PLL_VT_MPY, 150 },
-	{ IMX214_REG_OPPXCK_DIV, 10 },
-	{ IMX214_REG_OPSYCK_DIV, 1 },
-	{ IMX214_REG_PLL_MULT_DRIV, IMX214_PLL_SINGLE },
-
-	{ IMX214_REG_REQ_LINK_BIT_RATE, IMX214_LINK_BIT_RATE_MBPS(4800) },
-
 	{ CCI_REG8(0x3A03), 0x09 },
 	{ CCI_REG8(0x3A04), 0x50 },
 	{ CCI_REG8(0x3A05), 0x01 },
@@ -362,16 +352,6 @@ static const struct cci_reg_sequence mode_1920x1080[] = {
 	{ IMX214_REG_DIG_CROP_WIDTH, 1920 },
 	{ IMX214_REG_DIG_CROP_HEIGHT, 1080 },
 
-	{ IMX214_REG_VTPXCK_DIV, 5 },
-	{ IMX214_REG_VTSYCK_DIV, 2 },
-	{ IMX214_REG_PREPLLCK_VT_DIV, 3 },
-	{ IMX214_REG_PLL_VT_MPY, 150 },
-	{ IMX214_REG_OPPXCK_DIV, 10 },
-	{ IMX214_REG_OPSYCK_DIV, 1 },
-	{ IMX214_REG_PLL_MULT_DRIV, IMX214_PLL_SINGLE },
-
-	{ IMX214_REG_REQ_LINK_BIT_RATE, IMX214_LINK_BIT_RATE_MBPS(4800) },
-
 	{ CCI_REG8(0x3A03), 0x04 },
 	{ CCI_REG8(0x3A04), 0xF8 },
 	{ CCI_REG8(0x3A05), 0x02 },
@@ -405,9 +385,6 @@ static const struct cci_reg_sequence mode_table_common[] = {
 	/* ATR setting */
 	{ IMX214_REG_ATR_FAST_MOVE, 2 },
 
-	/* external clock setting */
-	{ IMX214_REG_EXCK_FREQ, IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000) },
-
 	/* global setting */
 	/* basic config */
 	{ IMX214_REG_MASK_CORR_FRAMES, IMX214_CORR_FRAMES_MASK },
@@ -777,6 +754,24 @@ static int imx214_entity_init_state(struct v4l2_subdev *subdev,
 	return 0;
 }
 
+static int imx214_configure_pll(struct imx214 *imx214)
+{
+	int ret = 0;
+
+	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV, 5, &ret);
+	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV, 2, &ret);
+	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV, 3, &ret);
+	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY, 150, &ret);
+	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV, 10, &ret);
+	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV, 1, &ret);
+	cci_write(imx214->regmap, IMX214_REG_PLL_MULT_DRIV,
+		  IMX214_PLL_SINGLE, &ret);
+	cci_write(imx214->regmap, IMX214_REG_EXCK_FREQ,
+		  IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000), &ret);
+
+	return ret;
+}
+
 static int imx214_update_digital_gain(struct imx214 *imx214, u32 val)
 {
 	int ret = 0;
@@ -1020,6 +1015,19 @@ static int imx214_start_streaming(struct imx214 *imx214)
 		return ret;
 	}
 
+	ret = imx214_configure_pll(imx214);
+	if (ret) {
+		dev_err(imx214->dev, "failed to configure PLL %d\n", ret);
+		return ret;
+	}
+
+	ret = cci_write(imx214->regmap, IMX214_REG_REQ_LINK_BIT_RATE,
+			IMX214_LINK_BIT_RATE_MBPS(4800), NULL);
+	if (ret) {
+		dev_err(imx214->dev, "failed to configure link bit rate\n");
+		return ret;
+	}
+
 	ret = cci_write(imx214->regmap, IMX214_REG_CSI_LANE_MODE,
 			IMX214_CSI_4_LANE_MODE, NULL);
 	if (ret) {

-- 
2.49.0

[PATCH v2 3/4] media: i2c: imx214: Make use of CCS PLL calculator

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

Calculate PLL parameters based on clock frequency and link frequency.

Acked-by: Ricardo Ribalda <ribalda@chromium.org>
Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/media/i2c/Kconfig  |   1 +
 drivers/media/i2c/imx214.c | 216 +++++++++++++++++++++++++++++++++++++--------
 2 files changed, 178 insertions(+), 39 deletions(-)

diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
index e576b213084d232e90b7e556a7a855a3bb95544c..c8e24c42e0c4ea169f1b6cdc4f2631234a51c7d9 100644
--- a/drivers/media/i2c/Kconfig
+++ b/drivers/media/i2c/Kconfig
@@ -141,6 +141,7 @@ config VIDEO_IMX214
 	depends on GPIOLIB
 	select REGMAP_I2C
 	select V4L2_CCI_I2C
+	select VIDEO_CCS_PLL
 	help
 	  This is a Video4Linux2 sensor driver for the Sony
 	  IMX214 camera.
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 3aca6ebb02d649c1b7f0b6a6049c1e3aa3d08951..9e9be47394ec768a5b34d44b06b5bbb0988da5a1 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -20,6 +20,8 @@
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
+#include "ccs-pll.h"
+
 /* Chip ID */
 #define IMX214_REG_CHIP_ID		CCI_REG16(0x0016)
 #define IMX214_CHIP_ID			0x0214
@@ -34,7 +36,6 @@
 #define IMX214_DEFAULT_LINK_FREQ	600000000
 /* Keep wrong link frequency for backward compatibility */
 #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
-#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
 #define IMX214_FPS 30
 
 /* V-TIMING internal */
@@ -84,6 +85,7 @@
 #define IMX214_CSI_DATA_FORMAT_RAW10	0x0A0A
 #define IMX214_CSI_DATA_FORMAT_COMP6	0x0A06
 #define IMX214_CSI_DATA_FORMAT_COMP8	0x0A08
+#define IMX214_BITS_PER_PIXEL_MASK	0xFF
 
 #define IMX214_REG_CSI_LANE_MODE	CCI_REG8(0x0114)
 #define IMX214_CSI_2_LANE_MODE		1
@@ -249,6 +251,10 @@ struct imx214 {
 	struct clk *xclk;
 	struct regmap *regmap;
 
+	struct ccs_pll pll;
+
+	struct v4l2_fwnode_endpoint bus_cfg;
+
 	struct v4l2_subdev sd;
 	struct media_pad pad;
 
@@ -758,16 +764,22 @@ static int imx214_configure_pll(struct imx214 *imx214)
 {
 	int ret = 0;
 
-	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV, 5, &ret);
-	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV, 2, &ret);
-	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV, 3, &ret);
-	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY, 150, &ret);
-	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV, 10, &ret);
-	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV, 1, &ret);
+	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV,
+		  imx214->pll.vt_bk.pix_clk_div, &ret);
+	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV,
+		  imx214->pll.vt_bk.sys_clk_div, &ret);
+	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV,
+		  imx214->pll.vt_fr.pre_pll_clk_div, &ret);
+	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY,
+		  imx214->pll.vt_fr.pll_multiplier, &ret);
+	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV,
+		  imx214->pll.op_bk.pix_clk_div, &ret);
+	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV,
+		  imx214->pll.op_bk.sys_clk_div, &ret);
 	cci_write(imx214->regmap, IMX214_REG_PLL_MULT_DRIV,
 		  IMX214_PLL_SINGLE, &ret);
 	cci_write(imx214->regmap, IMX214_REG_EXCK_FREQ,
-		  IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000), &ret);
+		  IMX214_EXCK_FREQ(imx214->pll.ext_clk_freq_hz / 1000000), &ret);
 
 	return ret;
 }
@@ -872,9 +884,6 @@ static const struct v4l2_ctrl_ops imx214_ctrl_ops = {
 
 static int imx214_ctrls_init(struct imx214 *imx214)
 {
-	static const s64 link_freq[] = {
-		IMX214_DEFAULT_LINK_FREQ
-	};
 	static const struct v4l2_area unit_size = {
 		.width = 1120,
 		.height = 1120,
@@ -895,15 +904,14 @@ static int imx214_ctrls_init(struct imx214 *imx214)
 	if (ret)
 		return ret;
 
-	imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
-					       V4L2_CID_PIXEL_RATE, 0,
-					       IMX214_DEFAULT_PIXEL_RATE, 1,
-					       IMX214_DEFAULT_PIXEL_RATE);
+	imx214->pixel_rate =
+		v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_PIXEL_RATE, 1,
+				  INT_MAX, 1, 1);
 
 	imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL,
 						   V4L2_CID_LINK_FREQ,
-						   ARRAY_SIZE(link_freq) - 1,
-						   0, link_freq);
+						   imx214->bus_cfg.nr_of_link_frequencies - 1,
+						   0, imx214->bus_cfg.link_frequencies);
 	if (imx214->link_freq)
 		imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
@@ -1006,6 +1014,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
 	const struct v4l2_mbus_framefmt *fmt;
 	struct v4l2_subdev_state *state;
 	const struct imx214_mode *mode;
+	int bit_rate_mbps;
 	int ret;
 
 	ret = cci_multi_reg_write(imx214->regmap, mode_table_common,
@@ -1021,8 +1030,10 @@ static int imx214_start_streaming(struct imx214 *imx214)
 		return ret;
 	}
 
+	bit_rate_mbps = (imx214->pll.pixel_rate_pixel_array / 1000000)
+			* imx214->pll.bits_per_pixel;
 	ret = cci_write(imx214->regmap, IMX214_REG_REQ_LINK_BIT_RATE,
-			IMX214_LINK_BIT_RATE_MBPS(4800), NULL);
+			IMX214_LINK_BIT_RATE_MBPS(bit_rate_mbps), NULL);
 	if (ret) {
 		dev_err(imx214->dev, "failed to configure link bit rate\n");
 		return ret;
@@ -1105,6 +1116,112 @@ static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
 	return ret;
 }
 
+static int imx214_pll_calculate(struct imx214 *imx214, struct ccs_pll *pll,
+				unsigned int link_freq)
+{
+	struct ccs_pll_limits limits = {
+		.min_ext_clk_freq_hz = 6000000,
+		.max_ext_clk_freq_hz = 27000000,
+
+		.vt_fr = {
+			.min_pre_pll_clk_div = 1,
+			.max_pre_pll_clk_div = 15,
+			/* min_pll_op_clk_freq_hz / max_pll_multiplier */
+			.min_pll_ip_clk_freq_hz = 281667,
+			/* max_pll_op_clk_freq_hz / min_pll_multiplier */
+			.max_pll_ip_clk_freq_hz = 100000000,
+			.min_pll_multiplier = 12,
+			.max_pll_multiplier = 1200,
+			.min_pll_op_clk_freq_hz = 338000000,
+			.max_pll_op_clk_freq_hz = 1200000000,
+		},
+		.vt_bk = {
+			.min_sys_clk_div = 2,
+			.max_sys_clk_div = 4,
+			.min_pix_clk_div = 5,
+			.max_pix_clk_div = 10,
+			.min_pix_clk_freq_hz = 30000000,
+			.max_pix_clk_freq_hz = 120000000,
+		},
+		.op_bk = {
+			.min_sys_clk_div = 1,
+			.max_sys_clk_div = 2,
+			.min_pix_clk_div = 6,
+			.max_pix_clk_div = 10,
+			.min_pix_clk_freq_hz = 30000000,
+			.max_pix_clk_freq_hz = 120000000,
+		},
+
+		.min_line_length_pck_bin = IMX214_PPL_DEFAULT,
+		.min_line_length_pck = IMX214_PPL_DEFAULT,
+	};
+	unsigned int num_lanes = imx214->bus_cfg.bus.mipi_csi2.num_data_lanes;
+	int ret;
+
+	/*
+	 * There are no documented constraints on the sys clock frequency, for
+	 * either branch. Recover them based on the PLL output clock frequency
+	 * and sys_clk_div limits on one hand, and the pix clock frequency and
+	 * the pix_clk_div limits on the other hand.
+	 */
+	limits.vt_bk.min_sys_clk_freq_hz =
+		max(limits.vt_fr.min_pll_op_clk_freq_hz / limits.vt_bk.max_sys_clk_div,
+		    limits.vt_bk.min_pix_clk_freq_hz * limits.vt_bk.min_pix_clk_div);
+	limits.vt_bk.max_sys_clk_freq_hz =
+		min(limits.vt_fr.max_pll_op_clk_freq_hz / limits.vt_bk.min_sys_clk_div,
+		    limits.vt_bk.max_pix_clk_freq_hz * limits.vt_bk.max_pix_clk_div);
+
+	limits.op_bk.min_sys_clk_freq_hz =
+		max(limits.vt_fr.min_pll_op_clk_freq_hz / limits.op_bk.max_sys_clk_div,
+		    limits.op_bk.min_pix_clk_freq_hz * limits.op_bk.min_pix_clk_div);
+	limits.op_bk.max_sys_clk_freq_hz =
+		min(limits.vt_fr.max_pll_op_clk_freq_hz / limits.op_bk.min_sys_clk_div,
+		    limits.op_bk.max_pix_clk_freq_hz * limits.op_bk.max_pix_clk_div);
+
+	memset(pll, 0, sizeof(*pll));
+
+	pll->bus_type = CCS_PLL_BUS_TYPE_CSI2_DPHY;
+	pll->op_lanes = num_lanes;
+	pll->vt_lanes = num_lanes;
+	pll->csi2.lanes = num_lanes;
+
+	pll->binning_horizontal = 1;
+	pll->binning_vertical = 1;
+	pll->scale_m = 1;
+	pll->scale_n = 1;
+	pll->bits_per_pixel =
+		IMX214_CSI_DATA_FORMAT_RAW10 & IMX214_BITS_PER_PIXEL_MASK;
+	pll->flags = CCS_PLL_FLAG_LANE_SPEED_MODEL;
+	pll->link_freq = link_freq;
+	pll->ext_clk_freq_hz = clk_get_rate(imx214->xclk);
+
+	ret = ccs_pll_calculate(imx214->dev, &limits, pll);
+
+	return ret;
+}
+
+static int imx214_pll_update(struct imx214 *imx214)
+{
+	u64 link_freq;
+	int ret;
+
+	link_freq = imx214->bus_cfg.link_frequencies[imx214->link_freq->val];
+	ret = imx214_pll_calculate(imx214, &imx214->pll, link_freq);
+	if (ret) {
+		dev_err(imx214->dev, "PLL calculations failed: %d\n", ret);
+		return ret;
+	}
+
+	ret = v4l2_ctrl_s_ctrl_int64(imx214->pixel_rate,
+				     imx214->pll.pixel_rate_pixel_array);
+	if (ret) {
+		dev_err(imx214->dev, "failed to set pixel rate\n");
+		return ret;
+	}
+
+	return 0;
+}
+
 static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
 				     struct v4l2_subdev_state *sd_state,
 				     struct v4l2_subdev_frame_interval *fival)
@@ -1211,12 +1328,10 @@ static int imx214_identify_module(struct imx214 *imx214)
 	return 0;
 }
 
-static int imx214_parse_fwnode(struct device *dev)
+static int imx214_parse_fwnode(struct device *dev, struct imx214 *imx214)
 {
+	struct v4l2_fwnode_endpoint *bus_cfg = &imx214->bus_cfg;
 	struct fwnode_handle *endpoint;
-	struct v4l2_fwnode_endpoint bus_cfg = {
-		.bus_type = V4L2_MBUS_CSI2_DPHY,
-	};
 	unsigned int i;
 	int ret;
 
@@ -1224,42 +1339,52 @@ static int imx214_parse_fwnode(struct device *dev)
 	if (!endpoint)
 		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
 
-	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
+	bus_cfg->bus_type = V4L2_MBUS_CSI2_DPHY;
+	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, bus_cfg);
+	fwnode_handle_put(endpoint);
 	if (ret) {
 		dev_err_probe(dev, ret, "parsing endpoint node failed\n");
-		goto done;
+		goto error;
 	}
 
 	/* Check the number of MIPI CSI2 data lanes */
-	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
+	if (bus_cfg->bus.mipi_csi2.num_data_lanes != 4) {
 		ret = dev_err_probe(dev, -EINVAL,
 				    "only 4 data lanes are currently supported\n");
-		goto done;
+		goto error;
 	}
 
-	if (bus_cfg.nr_of_link_frequencies != 1)
+	if (bus_cfg->nr_of_link_frequencies != 1)
 		dev_warn(dev, "Only one link-frequency supported, please review your DT. Continuing anyway\n");
 
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
-		if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
+	for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
+		u64 freq = bus_cfg->link_frequencies[i];
+		struct ccs_pll pll;
+
+		if (!imx214_pll_calculate(imx214, &pll, freq))
 			break;
-		if (bus_cfg.link_frequencies[i] ==
-		    IMX214_DEFAULT_LINK_FREQ_LEGACY) {
+		if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
 			dev_warn(dev,
 				 "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
 				 IMX214_DEFAULT_LINK_FREQ);
+			freq = IMX214_DEFAULT_LINK_FREQ;
+			if (imx214_pll_calculate(imx214, &pll, freq))
+				continue;
+			bus_cfg->link_frequencies[i] = freq;
 			break;
 		}
 	}
 
-	if (i == bus_cfg.nr_of_link_frequencies)
+	if (i == bus_cfg->nr_of_link_frequencies)
 		ret = dev_err_probe(dev, -EINVAL,
-				    "link-frequencies %d not supported, please review your DT\n",
-				    IMX214_DEFAULT_LINK_FREQ);
+				    "link-frequencies %lld not supported, please review your DT\n",
+				    bus_cfg->nr_of_link_frequencies ?
+				    bus_cfg->link_frequencies[0] : 0);
 
-done:
-	v4l2_fwnode_endpoint_free(&bus_cfg);
-	fwnode_handle_put(endpoint);
+	return 0;
+
+error:
+	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
 	return ret;
 }
 
@@ -1299,7 +1424,7 @@ static int imx214_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(imx214->regmap),
 				     "failed to initialize CCI\n");
 
-	ret = imx214_parse_fwnode(dev);
+	ret = imx214_parse_fwnode(dev, imx214);
 	if (ret)
 		return ret;
 
@@ -1310,7 +1435,9 @@ static int imx214_probe(struct i2c_client *client)
 	 * Enable power initially, to avoid warnings
 	 * from clk_disable on power_off
 	 */
-	imx214_power_on(imx214->dev);
+	ret = imx214_power_on(imx214->dev);
+	if (ret < 0)
+		goto error_fwnode;
 
 	ret = imx214_identify_module(imx214);
 	if (ret)
@@ -1341,6 +1468,12 @@ static int imx214_probe(struct i2c_client *client)
 	pm_runtime_set_active(imx214->dev);
 	pm_runtime_enable(imx214->dev);
 
+	ret = imx214_pll_update(imx214);
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "failed to update PLL\n");
+		goto error_subdev_cleanup;
+	}
+
 	ret = v4l2_async_register_subdev_sensor(&imx214->sd);
 	if (ret < 0) {
 		dev_err_probe(dev, ret,
@@ -1366,6 +1499,9 @@ static int imx214_probe(struct i2c_client *client)
 error_power_off:
 	imx214_power_off(imx214->dev);
 
+error_fwnode:
+	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
+
 	return ret;
 }
 
@@ -1378,6 +1514,8 @@ static void imx214_remove(struct i2c_client *client)
 	v4l2_subdev_cleanup(sd);
 	media_entity_cleanup(&imx214->sd.entity);
 	v4l2_ctrl_handler_free(&imx214->ctrls);
+	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
+
 	pm_runtime_disable(&client->dev);
 	if (!pm_runtime_status_suspended(&client->dev)) {
 		imx214_power_off(imx214->dev);

-- 
2.49.0

[PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[André Apitzsch via B4 Relay]

From: André Apitzsch <git@apitzsch.eu>

Instead rely on the rate set on the clock (using assigned-clock-rates
etc.)

Signed-off-by: André Apitzsch <git@apitzsch.eu>
---
 drivers/media/i2c/imx214.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -32,7 +32,6 @@
 
 #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
 
-#define IMX214_DEFAULT_CLK_FREQ	24000000
 #define IMX214_DEFAULT_LINK_FREQ	600000000
 /* Keep wrong link frequency for backward compatibility */
 #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
@@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
 		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
 				     "failed to get xclk\n");
 
-	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
-	if (ret)
-		return dev_err_probe(dev, ret,
-				     "failed to set xclk frequency\n");
-
 	ret = imx214_get_regulators(dev, imx214);
 	if (ret < 0)
 		return dev_err_probe(dev, ret, "failed to get regulators\n");

-- 
2.49.0

Re: [PATCH v2 3/4] media: i2c: imx214: Make use of CCS PLL calculator

[Sakari Ailus]

Hi André,

A few more comments below.

On Mon, May 05, 2025 at 11:05:55PM +0200, André Apitzsch via B4 Relay wrote:
> From: André Apitzsch <git@apitzsch.eu>
> 
> Calculate PLL parameters based on clock frequency and link frequency.
> 
> Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/media/i2c/Kconfig  |   1 +
>  drivers/media/i2c/imx214.c | 216 +++++++++++++++++++++++++++++++++++++--------
>  2 files changed, 178 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> index e576b213084d232e90b7e556a7a855a3bb95544c..c8e24c42e0c4ea169f1b6cdc4f2631234a51c7d9 100644
> --- a/drivers/media/i2c/Kconfig
> +++ b/drivers/media/i2c/Kconfig
> @@ -141,6 +141,7 @@ config VIDEO_IMX214
>  	depends on GPIOLIB
>  	select REGMAP_I2C
>  	select V4L2_CCI_I2C
> +	select VIDEO_CCS_PLL
>  	help
>  	  This is a Video4Linux2 sensor driver for the Sony
>  	  IMX214 camera.
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 3aca6ebb02d649c1b7f0b6a6049c1e3aa3d08951..9e9be47394ec768a5b34d44b06b5bbb0988da5a1 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -20,6 +20,8 @@
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> +#include "ccs-pll.h"
> +
>  /* Chip ID */
>  #define IMX214_REG_CHIP_ID		CCI_REG16(0x0016)
>  #define IMX214_CHIP_ID			0x0214
> @@ -34,7 +36,6 @@
>  #define IMX214_DEFAULT_LINK_FREQ	600000000
>  /* Keep wrong link frequency for backward compatibility */
>  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> -#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ * 8LL) / 10)
>  #define IMX214_FPS 30
>  
>  /* V-TIMING internal */
> @@ -84,6 +85,7 @@
>  #define IMX214_CSI_DATA_FORMAT_RAW10	0x0A0A
>  #define IMX214_CSI_DATA_FORMAT_COMP6	0x0A06
>  #define IMX214_CSI_DATA_FORMAT_COMP8	0x0A08
> +#define IMX214_BITS_PER_PIXEL_MASK	0xFF
>  
>  #define IMX214_REG_CSI_LANE_MODE	CCI_REG8(0x0114)
>  #define IMX214_CSI_2_LANE_MODE		1
> @@ -249,6 +251,10 @@ struct imx214 {
>  	struct clk *xclk;
>  	struct regmap *regmap;
>  
> +	struct ccs_pll pll;
> +
> +	struct v4l2_fwnode_endpoint bus_cfg;
> +
>  	struct v4l2_subdev sd;
>  	struct media_pad pad;
>  
> @@ -758,16 +764,22 @@ static int imx214_configure_pll(struct imx214 *imx214)
>  {
>  	int ret = 0;
>  
> -	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV, 5, &ret);
> -	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV, 2, &ret);
> -	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV, 3, &ret);
> -	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY, 150, &ret);
> -	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV, 10, &ret);
> -	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV, 1, &ret);
> +	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV,
> +		  imx214->pll.vt_bk.pix_clk_div, &ret);
> +	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV,
> +		  imx214->pll.vt_bk.sys_clk_div, &ret);
> +	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV,
> +		  imx214->pll.vt_fr.pre_pll_clk_div, &ret);
> +	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY,
> +		  imx214->pll.vt_fr.pll_multiplier, &ret);
> +	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV,
> +		  imx214->pll.op_bk.pix_clk_div, &ret);
> +	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV,
> +		  imx214->pll.op_bk.sys_clk_div, &ret);
>  	cci_write(imx214->regmap, IMX214_REG_PLL_MULT_DRIV,
>  		  IMX214_PLL_SINGLE, &ret);
>  	cci_write(imx214->regmap, IMX214_REG_EXCK_FREQ,
> -		  IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ / 1000000), &ret);
> +		  IMX214_EXCK_FREQ(imx214->pll.ext_clk_freq_hz / 1000000), &ret);
>  
>  	return ret;
>  }
> @@ -872,9 +884,6 @@ static const struct v4l2_ctrl_ops imx214_ctrl_ops = {
>  
>  static int imx214_ctrls_init(struct imx214 *imx214)
>  {
> -	static const s64 link_freq[] = {
> -		IMX214_DEFAULT_LINK_FREQ
> -	};
>  	static const struct v4l2_area unit_size = {
>  		.width = 1120,
>  		.height = 1120,
> @@ -895,15 +904,14 @@ static int imx214_ctrls_init(struct imx214 *imx214)
>  	if (ret)
>  		return ret;
>  
> -	imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> -					       V4L2_CID_PIXEL_RATE, 0,
> -					       IMX214_DEFAULT_PIXEL_RATE, 1,
> -					       IMX214_DEFAULT_PIXEL_RATE);
> +	imx214->pixel_rate =
> +		v4l2_ctrl_new_std(ctrl_hdlr, NULL, V4L2_CID_PIXEL_RATE, 1,
> +				  INT_MAX, 1, 1);
>  
>  	imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr, NULL,
>  						   V4L2_CID_LINK_FREQ,
> -						   ARRAY_SIZE(link_freq) - 1,
> -						   0, link_freq);
> +						   imx214->bus_cfg.nr_of_link_frequencies - 1,
> +						   0, imx214->bus_cfg.link_frequencies);
>  	if (imx214->link_freq)
>  		imx214->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> @@ -1006,6 +1014,7 @@ static int imx214_start_streaming(struct imx214 *imx214)
>  	const struct v4l2_mbus_framefmt *fmt;
>  	struct v4l2_subdev_state *state;
>  	const struct imx214_mode *mode;
> +	int bit_rate_mbps;
>  	int ret;
>  
>  	ret = cci_multi_reg_write(imx214->regmap, mode_table_common,
> @@ -1021,8 +1030,10 @@ static int imx214_start_streaming(struct imx214 *imx214)
>  		return ret;
>  	}
>  
> +	bit_rate_mbps = (imx214->pll.pixel_rate_pixel_array / 1000000)
> +			* imx214->pll.bits_per_pixel;
>  	ret = cci_write(imx214->regmap, IMX214_REG_REQ_LINK_BIT_RATE,
> -			IMX214_LINK_BIT_RATE_MBPS(4800), NULL);
> +			IMX214_LINK_BIT_RATE_MBPS(bit_rate_mbps), NULL);
>  	if (ret) {
>  		dev_err(imx214->dev, "failed to configure link bit rate\n");
>  		return ret;
> @@ -1105,6 +1116,112 @@ static int imx214_s_stream(struct v4l2_subdev *subdev, int enable)
>  	return ret;
>  }
>  
> +static int imx214_pll_calculate(struct imx214 *imx214, struct ccs_pll *pll,
> +				unsigned int link_freq)
> +{
> +	struct ccs_pll_limits limits = {
> +		.min_ext_clk_freq_hz = 6000000,
> +		.max_ext_clk_freq_hz = 27000000,
> +
> +		.vt_fr = {
> +			.min_pre_pll_clk_div = 1,
> +			.max_pre_pll_clk_div = 15,
> +			/* min_pll_op_clk_freq_hz / max_pll_multiplier */
> +			.min_pll_ip_clk_freq_hz = 281667,
> +			/* max_pll_op_clk_freq_hz / min_pll_multiplier */
> +			.max_pll_ip_clk_freq_hz = 100000000,

Regarding these limits -- the pll_ip_clk_freq_hz limits are likely between
around 6 MHz (lower limit) and between 12 MHz and 27 MHz. I'd use 6 for
lower and 12 for higher if they yield correct configuration currently and
loosen them up only if needed.

> +			.min_pll_multiplier = 12,
> +			.max_pll_multiplier = 1200,
> +			.min_pll_op_clk_freq_hz = 338000000,
> +			.max_pll_op_clk_freq_hz = 1200000000,
> +		},
> +		.vt_bk = {
> +			.min_sys_clk_div = 2,
> +			.max_sys_clk_div = 4,
> +			.min_pix_clk_div = 5,
> +			.max_pix_clk_div = 10,
> +			.min_pix_clk_freq_hz = 30000000,
> +			.max_pix_clk_freq_hz = 120000000,
> +		},
> +		.op_bk = {
> +			.min_sys_clk_div = 1,
> +			.max_sys_clk_div = 2,
> +			.min_pix_clk_div = 6,
> +			.max_pix_clk_div = 10,
> +			.min_pix_clk_freq_hz = 30000000,
> +			.max_pix_clk_freq_hz = 120000000,
> +		},
> +
> +		.min_line_length_pck_bin = IMX214_PPL_DEFAULT,
> +		.min_line_length_pck = IMX214_PPL_DEFAULT,
> +	};
> +	unsigned int num_lanes = imx214->bus_cfg.bus.mipi_csi2.num_data_lanes;
> +	int ret;
> +
> +	/*
> +	 * There are no documented constraints on the sys clock frequency, for
> +	 * either branch. Recover them based on the PLL output clock frequency
> +	 * and sys_clk_div limits on one hand, and the pix clock frequency and
> +	 * the pix_clk_div limits on the other hand.
> +	 */
> +	limits.vt_bk.min_sys_clk_freq_hz =
> +		max(limits.vt_fr.min_pll_op_clk_freq_hz / limits.vt_bk.max_sys_clk_div,
> +		    limits.vt_bk.min_pix_clk_freq_hz * limits.vt_bk.min_pix_clk_div);
> +	limits.vt_bk.max_sys_clk_freq_hz =
> +		min(limits.vt_fr.max_pll_op_clk_freq_hz / limits.vt_bk.min_sys_clk_div,
> +		    limits.vt_bk.max_pix_clk_freq_hz * limits.vt_bk.max_pix_clk_div);
> +
> +	limits.op_bk.min_sys_clk_freq_hz =
> +		max(limits.vt_fr.min_pll_op_clk_freq_hz / limits.op_bk.max_sys_clk_div,
> +		    limits.op_bk.min_pix_clk_freq_hz * limits.op_bk.min_pix_clk_div);
> +	limits.op_bk.max_sys_clk_freq_hz =
> +		min(limits.vt_fr.max_pll_op_clk_freq_hz / limits.op_bk.min_sys_clk_div,
> +		    limits.op_bk.max_pix_clk_freq_hz * limits.op_bk.max_pix_clk_div);
> +
> +	memset(pll, 0, sizeof(*pll));
> +
> +	pll->bus_type = CCS_PLL_BUS_TYPE_CSI2_DPHY;
> +	pll->op_lanes = num_lanes;
> +	pll->vt_lanes = num_lanes;
> +	pll->csi2.lanes = num_lanes;
> +
> +	pll->binning_horizontal = 1;
> +	pll->binning_vertical = 1;
> +	pll->scale_m = 1;
> +	pll->scale_n = 1;
> +	pll->bits_per_pixel =
> +		IMX214_CSI_DATA_FORMAT_RAW10 & IMX214_BITS_PER_PIXEL_MASK;
> +	pll->flags = CCS_PLL_FLAG_LANE_SPEED_MODEL;
> +	pll->link_freq = link_freq;
> +	pll->ext_clk_freq_hz = clk_get_rate(imx214->xclk);
> +
> +	ret = ccs_pll_calculate(imx214->dev, &limits, pll);
> +
> +	return ret;

You can drop ret here.

> +}
> +
> +static int imx214_pll_update(struct imx214 *imx214)
> +{
> +	u64 link_freq;
> +	int ret;
> +
> +	link_freq = imx214->bus_cfg.link_frequencies[imx214->link_freq->val];
> +	ret = imx214_pll_calculate(imx214, &imx214->pll, link_freq);
> +	if (ret) {
> +		dev_err(imx214->dev, "PLL calculations failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = v4l2_ctrl_s_ctrl_int64(imx214->pixel_rate,
> +				     imx214->pll.pixel_rate_pixel_array);
> +	if (ret) {
> +		dev_err(imx214->dev, "failed to set pixel rate\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
>  static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
>  				     struct v4l2_subdev_state *sd_state,
>  				     struct v4l2_subdev_frame_interval *fival)
> @@ -1211,12 +1328,10 @@ static int imx214_identify_module(struct imx214 *imx214)
>  	return 0;
>  }
>  
> -static int imx214_parse_fwnode(struct device *dev)
> +static int imx214_parse_fwnode(struct device *dev, struct imx214 *imx214)
>  {
> +	struct v4l2_fwnode_endpoint *bus_cfg = &imx214->bus_cfg;
>  	struct fwnode_handle *endpoint;
> -	struct v4l2_fwnode_endpoint bus_cfg = {
> -		.bus_type = V4L2_MBUS_CSI2_DPHY,
> -	};
>  	unsigned int i;
>  	int ret;
>  
> @@ -1224,42 +1339,52 @@ static int imx214_parse_fwnode(struct device *dev)
>  	if (!endpoint)
>  		return dev_err_probe(dev, -EINVAL, "endpoint node not found\n");
>  
> -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, &bus_cfg);
> +	bus_cfg->bus_type = V4L2_MBUS_CSI2_DPHY;
> +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, bus_cfg);
> +	fwnode_handle_put(endpoint);
>  	if (ret) {
>  		dev_err_probe(dev, ret, "parsing endpoint node failed\n");
> -		goto done;
> +		goto error;
>  	}
>  
>  	/* Check the number of MIPI CSI2 data lanes */
> -	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> +	if (bus_cfg->bus.mipi_csi2.num_data_lanes != 4) {
>  		ret = dev_err_probe(dev, -EINVAL,
>  				    "only 4 data lanes are currently supported\n");
> -		goto done;
> +		goto error;
>  	}
>  
> -	if (bus_cfg.nr_of_link_frequencies != 1)
> +	if (bus_cfg->nr_of_link_frequencies != 1)
>  		dev_warn(dev, "Only one link-frequency supported, please review your DT. Continuing anyway\n");
>  
> -	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> -		if (bus_cfg.link_frequencies[i] == IMX214_DEFAULT_LINK_FREQ)
> +	for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
> +		u64 freq = bus_cfg->link_frequencies[i];
> +		struct ccs_pll pll;
> +
> +		if (!imx214_pll_calculate(imx214, &pll, freq))
>  			break;
> -		if (bus_cfg.link_frequencies[i] ==
> -		    IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> +		if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
>  			dev_warn(dev,
>  				 "link-frequencies %d not supported, please review your DT. Continuing anyway\n",
>  				 IMX214_DEFAULT_LINK_FREQ);
> +			freq = IMX214_DEFAULT_LINK_FREQ;
> +			if (imx214_pll_calculate(imx214, &pll, freq))
> +				continue;
> +			bus_cfg->link_frequencies[i] = freq;
>  			break;
>  		}
>  	}
>  
> -	if (i == bus_cfg.nr_of_link_frequencies)
> +	if (i == bus_cfg->nr_of_link_frequencies)
>  		ret = dev_err_probe(dev, -EINVAL,
> -				    "link-frequencies %d not supported, please review your DT\n",
> -				    IMX214_DEFAULT_LINK_FREQ);
> +				    "link-frequencies %lld not supported, please review your DT\n",
> +				    bus_cfg->nr_of_link_frequencies ?
> +				    bus_cfg->link_frequencies[0] : 0);
>  
> -done:
> -	v4l2_fwnode_endpoint_free(&bus_cfg);
> -	fwnode_handle_put(endpoint);
> +	return 0;
> +
> +error:
> +	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
>  	return ret;
>  }
>  
> @@ -1299,7 +1424,7 @@ static int imx214_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(imx214->regmap),
>  				     "failed to initialize CCI\n");
>  
> -	ret = imx214_parse_fwnode(dev);
> +	ret = imx214_parse_fwnode(dev, imx214);
>  	if (ret)
>  		return ret;
>  
> @@ -1310,7 +1435,9 @@ static int imx214_probe(struct i2c_client *client)
>  	 * Enable power initially, to avoid warnings
>  	 * from clk_disable on power_off
>  	 */
> -	imx214_power_on(imx214->dev);
> +	ret = imx214_power_on(imx214->dev);
> +	if (ret < 0)
> +		goto error_fwnode;
>  
>  	ret = imx214_identify_module(imx214);
>  	if (ret)
> @@ -1341,6 +1468,12 @@ static int imx214_probe(struct i2c_client *client)
>  	pm_runtime_set_active(imx214->dev);
>  	pm_runtime_enable(imx214->dev);
>  
> +	ret = imx214_pll_update(imx214);
> +	if (ret < 0) {
> +		dev_err_probe(dev, ret, "failed to update PLL\n");
> +		goto error_subdev_cleanup;
> +	}
> +
>  	ret = v4l2_async_register_subdev_sensor(&imx214->sd);
>  	if (ret < 0) {
>  		dev_err_probe(dev, ret,
> @@ -1366,6 +1499,9 @@ static int imx214_probe(struct i2c_client *client)
>  error_power_off:
>  	imx214_power_off(imx214->dev);
>  
> +error_fwnode:
> +	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> +
>  	return ret;
>  }
>  
> @@ -1378,6 +1514,8 @@ static void imx214_remove(struct i2c_client *client)
>  	v4l2_subdev_cleanup(sd);
>  	media_entity_cleanup(&imx214->sd.entity);
>  	v4l2_ctrl_handler_free(&imx214->ctrls);
> +	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> +
>  	pm_runtime_disable(&client->dev);
>  	if (!pm_runtime_status_suspended(&client->dev)) {
>  		imx214_power_off(imx214->dev);
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[Sakari Ailus]

Hi André,

On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay wrote:
> From: André Apitzsch <git@apitzsch.eu>
> 
> Instead rely on the rate set on the clock (using assigned-clock-rates
> etc.)
> 
> Signed-off-by: André Apitzsch <git@apitzsch.eu>
> ---
>  drivers/media/i2c/imx214.c | 6 ------
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -32,7 +32,6 @@
>  
>  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
>  
> -#define IMX214_DEFAULT_CLK_FREQ	24000000
>  #define IMX214_DEFAULT_LINK_FREQ	600000000
>  /* Keep wrong link frequency for backward compatibility */
>  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
>  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
>  				     "failed to get xclk\n");
>  
> -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> -	if (ret)
> -		return dev_err_probe(dev, ret,
> -				     "failed to set xclk frequency\n");
> -

Oops. I missed this is what the driver was doing already. Indeed, this is
one of the historic sensor drivers that do set the frequency in DT systems.

The driver never used the clock-frequency property and instead used a fixed
frequency. Changing the behaviour now could be problematic.

There are options here that I think we could do:

1) use your v1 patch (4) which uses "clock-frequency" if it exists and
otherwise uses the default, fixed frequency or

2) set the frequency only if the "clock-frequency" property exists. The DT
currently requires clock-frequency and the YAML conversion was done in 2020
whereas the driver is from 2018. If we do this, the clock-frequency should
be deprecated (or even removed from bingings).

I wonder what others think. Cc'd Laurent in any case.

>  	ret = imx214_get_regulators(dev, imx214);
>  	if (ret < 0)
>  		return dev_err_probe(dev, ret, "failed to get regulators\n");
> 

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 3/4] media: i2c: imx214: Make use of CCS PLL calculator

[André Apitzsch]

Hi Sakari,

thanks for the feedback. One question below.

Am Dienstag, dem 06.05.2025 um 08:05 +0000 schrieb Sakari Ailus:
> Hi André,
> 
> A few more comments below.
> 
> On Mon, May 05, 2025 at 11:05:55PM +0200, André Apitzsch via B4 Relay
> wrote:
> > From: André Apitzsch <git@apitzsch.eu>
> > 
> > Calculate PLL parameters based on clock frequency and link
> > frequency.
> > 
> > Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/media/i2c/Kconfig  |   1 +
> >  drivers/media/i2c/imx214.c | 216
> > +++++++++++++++++++++++++++++++++++++--------
> >  2 files changed, 178 insertions(+), 39 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > index
> > e576b213084d232e90b7e556a7a855a3bb95544c..c8e24c42e0c4ea169f1b6cdc4
> > f2631234a51c7d9 100644
> > --- a/drivers/media/i2c/Kconfig
> > +++ b/drivers/media/i2c/Kconfig
> > @@ -141,6 +141,7 @@ config VIDEO_IMX214
> >  	depends on GPIOLIB
> >  	select REGMAP_I2C
> >  	select V4L2_CCI_I2C
> > +	select VIDEO_CCS_PLL
> >  	help
> >  	  This is a Video4Linux2 sensor driver for the Sony
> >  	  IMX214 camera.
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index
> > 3aca6ebb02d649c1b7f0b6a6049c1e3aa3d08951..9e9be47394ec768a5b34d44b0
> > 6b5bbb0988da5a1 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -20,6 +20,8 @@
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >  
> > +#include "ccs-pll.h"
> > +
> >  /* Chip ID */
> >  #define IMX214_REG_CHIP_ID		CCI_REG16(0x0016)
> >  #define IMX214_CHIP_ID			0x0214
> > @@ -34,7 +36,6 @@
> >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> >  /* Keep wrong link frequency for backward compatibility */
> >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > -#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ *
> > 8LL) / 10)
> >  #define IMX214_FPS 30
> >  
> >  /* V-TIMING internal */
> > @@ -84,6 +85,7 @@
> >  #define IMX214_CSI_DATA_FORMAT_RAW10	0x0A0A
> >  #define IMX214_CSI_DATA_FORMAT_COMP6	0x0A06
> >  #define IMX214_CSI_DATA_FORMAT_COMP8	0x0A08
> > +#define IMX214_BITS_PER_PIXEL_MASK	0xFF
> >  
> >  #define IMX214_REG_CSI_LANE_MODE	CCI_REG8(0x0114)
> >  #define IMX214_CSI_2_LANE_MODE		1
> > @@ -249,6 +251,10 @@ struct imx214 {
> >  	struct clk *xclk;
> >  	struct regmap *regmap;
> >  
> > +	struct ccs_pll pll;
> > +
> > +	struct v4l2_fwnode_endpoint bus_cfg;
> > +
> >  	struct v4l2_subdev sd;
> >  	struct media_pad pad;
> >  
> > @@ -758,16 +764,22 @@ static int imx214_configure_pll(struct imx214
> > *imx214)
> >  {
> >  	int ret = 0;
> >  
> > -	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV, 5, &ret);
> > -	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV, 2, &ret);
> > -	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV, 3,
> > &ret);
> > -	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY, 150,
> > &ret);
> > -	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV, 10,
> > &ret);
> > -	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV, 1, &ret);
> > +	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV,
> > +		  imx214->pll.vt_bk.pix_clk_div, &ret);
> > +	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV,
> > +		  imx214->pll.vt_bk.sys_clk_div, &ret);
> > +	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV,
> > +		  imx214->pll.vt_fr.pre_pll_clk_div, &ret);
> > +	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY,
> > +		  imx214->pll.vt_fr.pll_multiplier, &ret);
> > +	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV,
> > +		  imx214->pll.op_bk.pix_clk_div, &ret);
> > +	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV,
> > +		  imx214->pll.op_bk.sys_clk_div, &ret);
> >  	cci_write(imx214->regmap, IMX214_REG_PLL_MULT_DRIV,
> >  		  IMX214_PLL_SINGLE, &ret);
> >  	cci_write(imx214->regmap, IMX214_REG_EXCK_FREQ,
> > -		  IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ /
> > 1000000), &ret);
> > +		  IMX214_EXCK_FREQ(imx214->pll.ext_clk_freq_hz /
> > 1000000), &ret);
> >  
> >  	return ret;
> >  }
> > @@ -872,9 +884,6 @@ static const struct v4l2_ctrl_ops
> > imx214_ctrl_ops = {
> >  
> >  static int imx214_ctrls_init(struct imx214 *imx214)
> >  {
> > -	static const s64 link_freq[] = {
> > -		IMX214_DEFAULT_LINK_FREQ
> > -	};
> >  	static const struct v4l2_area unit_size = {
> >  		.width = 1120,
> >  		.height = 1120,
> > @@ -895,15 +904,14 @@ static int imx214_ctrls_init(struct imx214
> > *imx214)
> >  	if (ret)
> >  		return ret;
> >  
> > -	imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> > -					      
> > V4L2_CID_PIXEL_RATE, 0,
> > -					      
> > IMX214_DEFAULT_PIXEL_RATE, 1,
> > -					      
> > IMX214_DEFAULT_PIXEL_RATE);
> > +	imx214->pixel_rate =
> > +		v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> > V4L2_CID_PIXEL_RATE, 1,
> > +				  INT_MAX, 1, 1);
> >  
> >  	imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > NULL,
> >  						  
> > V4L2_CID_LINK_FREQ,
> > -						  
> > ARRAY_SIZE(link_freq) - 1,
> > -						   0, link_freq);
> > +						   imx214-
> > >bus_cfg.nr_of_link_frequencies - 1,
> > +						   0, imx214-
> > >bus_cfg.link_frequencies);
> >  	if (imx214->link_freq)
> >  		imx214->link_freq->flags |=
> > V4L2_CTRL_FLAG_READ_ONLY;
> >  
> > @@ -1006,6 +1014,7 @@ static int imx214_start_streaming(struct
> > imx214 *imx214)
> >  	const struct v4l2_mbus_framefmt *fmt;
> >  	struct v4l2_subdev_state *state;
> >  	const struct imx214_mode *mode;
> > +	int bit_rate_mbps;
> >  	int ret;
> >  
> >  	ret = cci_multi_reg_write(imx214->regmap,
> > mode_table_common,
> > @@ -1021,8 +1030,10 @@ static int imx214_start_streaming(struct
> > imx214 *imx214)
> >  		return ret;
> >  	}
> >  
> > +	bit_rate_mbps = (imx214->pll.pixel_rate_pixel_array /
> > 1000000)
> > +			* imx214->pll.bits_per_pixel;
> >  	ret = cci_write(imx214->regmap,
> > IMX214_REG_REQ_LINK_BIT_RATE,
> > -			IMX214_LINK_BIT_RATE_MBPS(4800), NULL);
> > +			IMX214_LINK_BIT_RATE_MBPS(bit_rate_mbps),
> > NULL);
> >  	if (ret) {
> >  		dev_err(imx214->dev, "failed to configure link bit
> > rate\n");
> >  		return ret;
> > @@ -1105,6 +1116,112 @@ static int imx214_s_stream(struct
> > v4l2_subdev *subdev, int enable)
> >  	return ret;
> >  }
> >  
> > +static int imx214_pll_calculate(struct imx214 *imx214, struct
> > ccs_pll *pll,
> > +				unsigned int link_freq)
> > +{
> > +	struct ccs_pll_limits limits = {
> > +		.min_ext_clk_freq_hz = 6000000,
> > +		.max_ext_clk_freq_hz = 27000000,
> > +
> > +		.vt_fr = {
> > +			.min_pre_pll_clk_div = 1,
> > +			.max_pre_pll_clk_div = 15,
> > +			/* min_pll_op_clk_freq_hz /
> > max_pll_multiplier */
> > +			.min_pll_ip_clk_freq_hz = 281667,
> > +			/* max_pll_op_clk_freq_hz /
> > min_pll_multiplier */
> > +			.max_pll_ip_clk_freq_hz = 100000000,
> 
> Regarding these limits -- the pll_ip_clk_freq_hz limits are likely
> between around 6 MHz (lower limit) and between 12 MHz and 27 MHz. I'd
> use 6 for lower and 12 for higher if they yield correct configuration
> currently and loosen them up only if needed.

The range 6-12 MHz seems to work.
With the updated min/max values, the comments are no longer valid,
should I just remove them or what alternative comments could be used?

Best regards,
André

> 
> > +			.min_pll_multiplier = 12,
> > +			.max_pll_multiplier = 1200,
> > +			.min_pll_op_clk_freq_hz = 338000000,
> > +			.max_pll_op_clk_freq_hz = 1200000000,
> > +		},
> > +		.vt_bk = {
> > +			.min_sys_clk_div = 2,
> > +			.max_sys_clk_div = 4,
> > +			.min_pix_clk_div = 5,
> > +			.max_pix_clk_div = 10,
> > +			.min_pix_clk_freq_hz = 30000000,
> > +			.max_pix_clk_freq_hz = 120000000,
> > +		},
> > +		.op_bk = {
> > +			.min_sys_clk_div = 1,
> > +			.max_sys_clk_div = 2,
> > +			.min_pix_clk_div = 6,
> > +			.max_pix_clk_div = 10,
> > +			.min_pix_clk_freq_hz = 30000000,
> > +			.max_pix_clk_freq_hz = 120000000,
> > +		},
> > +
> > +		.min_line_length_pck_bin = IMX214_PPL_DEFAULT,
> > +		.min_line_length_pck = IMX214_PPL_DEFAULT,
> > +	};
> > +	unsigned int num_lanes = imx214-
> > >bus_cfg.bus.mipi_csi2.num_data_lanes;
> > +	int ret;
> > +
> > +	/*
> > +	 * There are no documented constraints on the sys clock
> > frequency, for
> > +	 * either branch. Recover them based on the PLL output
> > clock frequency
> > +	 * and sys_clk_div limits on one hand, and the pix clock
> > frequency and
> > +	 * the pix_clk_div limits on the other hand.
> > +	 */
> > +	limits.vt_bk.min_sys_clk_freq_hz =
> > +		max(limits.vt_fr.min_pll_op_clk_freq_hz /
> > limits.vt_bk.max_sys_clk_div,
> > +		    limits.vt_bk.min_pix_clk_freq_hz *
> > limits.vt_bk.min_pix_clk_div);
> > +	limits.vt_bk.max_sys_clk_freq_hz =
> > +		min(limits.vt_fr.max_pll_op_clk_freq_hz /
> > limits.vt_bk.min_sys_clk_div,
> > +		    limits.vt_bk.max_pix_clk_freq_hz *
> > limits.vt_bk.max_pix_clk_div);
> > +
> > +	limits.op_bk.min_sys_clk_freq_hz =
> > +		max(limits.vt_fr.min_pll_op_clk_freq_hz /
> > limits.op_bk.max_sys_clk_div,
> > +		    limits.op_bk.min_pix_clk_freq_hz *
> > limits.op_bk.min_pix_clk_div);
> > +	limits.op_bk.max_sys_clk_freq_hz =
> > +		min(limits.vt_fr.max_pll_op_clk_freq_hz /
> > limits.op_bk.min_sys_clk_div,
> > +		    limits.op_bk.max_pix_clk_freq_hz *
> > limits.op_bk.max_pix_clk_div);
> > +
> > +	memset(pll, 0, sizeof(*pll));
> > +
> > +	pll->bus_type = CCS_PLL_BUS_TYPE_CSI2_DPHY;
> > +	pll->op_lanes = num_lanes;
> > +	pll->vt_lanes = num_lanes;
> > +	pll->csi2.lanes = num_lanes;
> > +
> > +	pll->binning_horizontal = 1;
> > +	pll->binning_vertical = 1;
> > +	pll->scale_m = 1;
> > +	pll->scale_n = 1;
> > +	pll->bits_per_pixel =
> > +		IMX214_CSI_DATA_FORMAT_RAW10 &
> > IMX214_BITS_PER_PIXEL_MASK;
> > +	pll->flags = CCS_PLL_FLAG_LANE_SPEED_MODEL;
> > +	pll->link_freq = link_freq;
> > +	pll->ext_clk_freq_hz = clk_get_rate(imx214->xclk);
> > +
> > +	ret = ccs_pll_calculate(imx214->dev, &limits, pll);
> > +
> > +	return ret;
> 
> You can drop ret here.
> 
> > +}
> > +
> > +static int imx214_pll_update(struct imx214 *imx214)
> > +{
> > +	u64 link_freq;
> > +	int ret;
> > +
> > +	link_freq = imx214->bus_cfg.link_frequencies[imx214-
> > >link_freq->val];
> > +	ret = imx214_pll_calculate(imx214, &imx214->pll,
> > link_freq);
> > +	if (ret) {
> > +		dev_err(imx214->dev, "PLL calculations failed:
> > %d\n", ret);
> > +		return ret;
> > +	}
> > +
> > +	ret = v4l2_ctrl_s_ctrl_int64(imx214->pixel_rate,
> > +				     imx214-
> > >pll.pixel_rate_pixel_array);
> > +	if (ret) {
> > +		dev_err(imx214->dev, "failed to set pixel
> > rate\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int imx214_get_frame_interval(struct v4l2_subdev *subdev,
> >  				     struct v4l2_subdev_state
> > *sd_state,
> >  				     struct
> > v4l2_subdev_frame_interval *fival)
> > @@ -1211,12 +1328,10 @@ static int imx214_identify_module(struct
> > imx214 *imx214)
> >  	return 0;
> >  }
> >  
> > -static int imx214_parse_fwnode(struct device *dev)
> > +static int imx214_parse_fwnode(struct device *dev, struct imx214
> > *imx214)
> >  {
> > +	struct v4l2_fwnode_endpoint *bus_cfg = &imx214->bus_cfg;
> >  	struct fwnode_handle *endpoint;
> > -	struct v4l2_fwnode_endpoint bus_cfg = {
> > -		.bus_type = V4L2_MBUS_CSI2_DPHY,
> > -	};
> >  	unsigned int i;
> >  	int ret;
> >  
> > @@ -1224,42 +1339,52 @@ static int imx214_parse_fwnode(struct
> > device *dev)
> >  	if (!endpoint)
> >  		return dev_err_probe(dev, -EINVAL, "endpoint node
> > not found\n");
> >  
> > -	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint,
> > &bus_cfg);
> > +	bus_cfg->bus_type = V4L2_MBUS_CSI2_DPHY;
> > +	ret = v4l2_fwnode_endpoint_alloc_parse(endpoint, bus_cfg);
> > +	fwnode_handle_put(endpoint);
> >  	if (ret) {
> >  		dev_err_probe(dev, ret, "parsing endpoint node
> > failed\n");
> > -		goto done;
> > +		goto error;
> >  	}
> >  
> >  	/* Check the number of MIPI CSI2 data lanes */
> > -	if (bus_cfg.bus.mipi_csi2.num_data_lanes != 4) {
> > +	if (bus_cfg->bus.mipi_csi2.num_data_lanes != 4) {
> >  		ret = dev_err_probe(dev, -EINVAL,
> >  				    "only 4 data lanes are
> > currently supported\n");
> > -		goto done;
> > +		goto error;
> >  	}
> >  
> > -	if (bus_cfg.nr_of_link_frequencies != 1)
> > +	if (bus_cfg->nr_of_link_frequencies != 1)
> >  		dev_warn(dev, "Only one link-frequency supported,
> > please review your DT. Continuing anyway\n");
> >  
> > -	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++) {
> > -		if (bus_cfg.link_frequencies[i] ==
> > IMX214_DEFAULT_LINK_FREQ)
> > +	for (i = 0; i < bus_cfg->nr_of_link_frequencies; i++) {
> > +		u64 freq = bus_cfg->link_frequencies[i];
> > +		struct ccs_pll pll;
> > +
> > +		if (!imx214_pll_calculate(imx214, &pll, freq))
> >  			break;
> > -		if (bus_cfg.link_frequencies[i] ==
> > -		    IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> > +		if (freq == IMX214_DEFAULT_LINK_FREQ_LEGACY) {
> >  			dev_warn(dev,
> >  				 "link-frequencies %d not
> > supported, please review your DT. Continuing anyway\n",
> >  				 IMX214_DEFAULT_LINK_FREQ);
> > +			freq = IMX214_DEFAULT_LINK_FREQ;
> > +			if (imx214_pll_calculate(imx214, &pll,
> > freq))
> > +				continue;
> > +			bus_cfg->link_frequencies[i] = freq;
> >  			break;
> >  		}
> >  	}
> >  
> > -	if (i == bus_cfg.nr_of_link_frequencies)
> > +	if (i == bus_cfg->nr_of_link_frequencies)
> >  		ret = dev_err_probe(dev, -EINVAL,
> > -				    "link-frequencies %d not
> > supported, please review your DT\n",
> > -				    IMX214_DEFAULT_LINK_FREQ);
> > +				    "link-frequencies %lld not
> > supported, please review your DT\n",
> > +				    bus_cfg-
> > >nr_of_link_frequencies ?
> > +				    bus_cfg->link_frequencies[0] :
> > 0);
> >  
> > -done:
> > -	v4l2_fwnode_endpoint_free(&bus_cfg);
> > -	fwnode_handle_put(endpoint);
> > +	return 0;
> > +
> > +error:
> > +	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> >  	return ret;
> >  }
> >  
> > @@ -1299,7 +1424,7 @@ static int imx214_probe(struct i2c_client
> > *client)
> >  		return dev_err_probe(dev, PTR_ERR(imx214->regmap),
> >  				     "failed to initialize
> > CCI\n");
> >  
> > -	ret = imx214_parse_fwnode(dev);
> > +	ret = imx214_parse_fwnode(dev, imx214);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -1310,7 +1435,9 @@ static int imx214_probe(struct i2c_client
> > *client)
> >  	 * Enable power initially, to avoid warnings
> >  	 * from clk_disable on power_off
> >  	 */
> > -	imx214_power_on(imx214->dev);
> > +	ret = imx214_power_on(imx214->dev);
> > +	if (ret < 0)
> > +		goto error_fwnode;
> >  
> >  	ret = imx214_identify_module(imx214);
> >  	if (ret)
> > @@ -1341,6 +1468,12 @@ static int imx214_probe(struct i2c_client
> > *client)
> >  	pm_runtime_set_active(imx214->dev);
> >  	pm_runtime_enable(imx214->dev);
> >  
> > +	ret = imx214_pll_update(imx214);
> > +	if (ret < 0) {
> > +		dev_err_probe(dev, ret, "failed to update PLL\n");
> > +		goto error_subdev_cleanup;
> > +	}
> > +
> >  	ret = v4l2_async_register_subdev_sensor(&imx214->sd);
> >  	if (ret < 0) {
> >  		dev_err_probe(dev, ret,
> > @@ -1366,6 +1499,9 @@ static int imx214_probe(struct i2c_client
> > *client)
> >  error_power_off:
> >  	imx214_power_off(imx214->dev);
> >  
> > +error_fwnode:
> > +	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > +
> >  	return ret;
> >  }
> >  
> > @@ -1378,6 +1514,8 @@ static void imx214_remove(struct i2c_client
> > *client)
> >  	v4l2_subdev_cleanup(sd);
> >  	media_entity_cleanup(&imx214->sd.entity);
> >  	v4l2_ctrl_handler_free(&imx214->ctrls);
> > +	v4l2_fwnode_endpoint_free(&imx214->bus_cfg);
> > +
> >  	pm_runtime_disable(&client->dev);
> >  	if (!pm_runtime_status_suspended(&client->dev)) {
> >  		imx214_power_off(imx214->dev);
> > 

Re: [PATCH v2 3/4] media: i2c: imx214: Make use of CCS PLL calculator

[Sakari Ailus]

Hi André,

On Tue, May 06, 2025 at 10:16:23PM +0200, André Apitzsch wrote:
> Hi Sakari,
> 
> thanks for the feedback. One question below.
> 
> Am Dienstag, dem 06.05.2025 um 08:05 +0000 schrieb Sakari Ailus:
> > Hi André,
> > 
> > A few more comments below.
> > 
> > On Mon, May 05, 2025 at 11:05:55PM +0200, André Apitzsch via B4 Relay
> > wrote:
> > > From: André Apitzsch <git@apitzsch.eu>
> > > 
> > > Calculate PLL parameters based on clock frequency and link
> > > frequency.
> > > 
> > > Acked-by: Ricardo Ribalda <ribalda@chromium.org>
> > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > ---
> > >  drivers/media/i2c/Kconfig  |   1 +
> > >  drivers/media/i2c/imx214.c | 216
> > > +++++++++++++++++++++++++++++++++++++--------
> > >  2 files changed, 178 insertions(+), 39 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/Kconfig b/drivers/media/i2c/Kconfig
> > > index
> > > e576b213084d232e90b7e556a7a855a3bb95544c..c8e24c42e0c4ea169f1b6cdc4
> > > f2631234a51c7d9 100644
> > > --- a/drivers/media/i2c/Kconfig
> > > +++ b/drivers/media/i2c/Kconfig
> > > @@ -141,6 +141,7 @@ config VIDEO_IMX214
> > >  	depends on GPIOLIB
> > >  	select REGMAP_I2C
> > >  	select V4L2_CCI_I2C
> > > +	select VIDEO_CCS_PLL
> > >  	help
> > >  	  This is a Video4Linux2 sensor driver for the Sony
> > >  	  IMX214 camera.
> > > diff --git a/drivers/media/i2c/imx214.c
> > > b/drivers/media/i2c/imx214.c
> > > index
> > > 3aca6ebb02d649c1b7f0b6a6049c1e3aa3d08951..9e9be47394ec768a5b34d44b0
> > > 6b5bbb0988da5a1 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -20,6 +20,8 @@
> > >  #include <media/v4l2-fwnode.h>
> > >  #include <media/v4l2-subdev.h>
> > >  
> > > +#include "ccs-pll.h"
> > > +
> > >  /* Chip ID */
> > >  #define IMX214_REG_CHIP_ID		CCI_REG16(0x0016)
> > >  #define IMX214_CHIP_ID			0x0214
> > > @@ -34,7 +36,6 @@
> > >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> > >  /* Keep wrong link frequency for backward compatibility */
> > >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > > -#define IMX214_DEFAULT_PIXEL_RATE ((IMX214_DEFAULT_LINK_FREQ *
> > > 8LL) / 10)
> > >  #define IMX214_FPS 30
> > >  
> > >  /* V-TIMING internal */
> > > @@ -84,6 +85,7 @@
> > >  #define IMX214_CSI_DATA_FORMAT_RAW10	0x0A0A
> > >  #define IMX214_CSI_DATA_FORMAT_COMP6	0x0A06
> > >  #define IMX214_CSI_DATA_FORMAT_COMP8	0x0A08
> > > +#define IMX214_BITS_PER_PIXEL_MASK	0xFF
> > >  
> > >  #define IMX214_REG_CSI_LANE_MODE	CCI_REG8(0x0114)
> > >  #define IMX214_CSI_2_LANE_MODE		1
> > > @@ -249,6 +251,10 @@ struct imx214 {
> > >  	struct clk *xclk;
> > >  	struct regmap *regmap;
> > >  
> > > +	struct ccs_pll pll;
> > > +
> > > +	struct v4l2_fwnode_endpoint bus_cfg;
> > > +
> > >  	struct v4l2_subdev sd;
> > >  	struct media_pad pad;
> > >  
> > > @@ -758,16 +764,22 @@ static int imx214_configure_pll(struct imx214
> > > *imx214)
> > >  {
> > >  	int ret = 0;
> > >  
> > > -	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV, 5, &ret);
> > > -	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV, 2, &ret);
> > > -	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV, 3,
> > > &ret);
> > > -	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY, 150,
> > > &ret);
> > > -	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV, 10,
> > > &ret);
> > > -	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV, 1, &ret);
> > > +	cci_write(imx214->regmap, IMX214_REG_VTPXCK_DIV,
> > > +		  imx214->pll.vt_bk.pix_clk_div, &ret);
> > > +	cci_write(imx214->regmap, IMX214_REG_VTSYCK_DIV,
> > > +		  imx214->pll.vt_bk.sys_clk_div, &ret);
> > > +	cci_write(imx214->regmap, IMX214_REG_PREPLLCK_VT_DIV,
> > > +		  imx214->pll.vt_fr.pre_pll_clk_div, &ret);
> > > +	cci_write(imx214->regmap, IMX214_REG_PLL_VT_MPY,
> > > +		  imx214->pll.vt_fr.pll_multiplier, &ret);
> > > +	cci_write(imx214->regmap, IMX214_REG_OPPXCK_DIV,
> > > +		  imx214->pll.op_bk.pix_clk_div, &ret);
> > > +	cci_write(imx214->regmap, IMX214_REG_OPSYCK_DIV,
> > > +		  imx214->pll.op_bk.sys_clk_div, &ret);
> > >  	cci_write(imx214->regmap, IMX214_REG_PLL_MULT_DRIV,
> > >  		  IMX214_PLL_SINGLE, &ret);
> > >  	cci_write(imx214->regmap, IMX214_REG_EXCK_FREQ,
> > > -		  IMX214_EXCK_FREQ(IMX214_DEFAULT_CLK_FREQ /
> > > 1000000), &ret);
> > > +		  IMX214_EXCK_FREQ(imx214->pll.ext_clk_freq_hz /
> > > 1000000), &ret);
> > >  
> > >  	return ret;
> > >  }
> > > @@ -872,9 +884,6 @@ static const struct v4l2_ctrl_ops
> > > imx214_ctrl_ops = {
> > >  
> > >  static int imx214_ctrls_init(struct imx214 *imx214)
> > >  {
> > > -	static const s64 link_freq[] = {
> > > -		IMX214_DEFAULT_LINK_FREQ
> > > -	};
> > >  	static const struct v4l2_area unit_size = {
> > >  		.width = 1120,
> > >  		.height = 1120,
> > > @@ -895,15 +904,14 @@ static int imx214_ctrls_init(struct imx214
> > > *imx214)
> > >  	if (ret)
> > >  		return ret;
> > >  
> > > -	imx214->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> > > -					      
> > > V4L2_CID_PIXEL_RATE, 0,
> > > -					      
> > > IMX214_DEFAULT_PIXEL_RATE, 1,
> > > -					      
> > > IMX214_DEFAULT_PIXEL_RATE);
> > > +	imx214->pixel_rate =
> > > +		v4l2_ctrl_new_std(ctrl_hdlr, NULL,
> > > V4L2_CID_PIXEL_RATE, 1,
> > > +				  INT_MAX, 1, 1);
> > >  
> > >  	imx214->link_freq = v4l2_ctrl_new_int_menu(ctrl_hdlr,
> > > NULL,
> > >  						  
> > > V4L2_CID_LINK_FREQ,
> > > -						  
> > > ARRAY_SIZE(link_freq) - 1,
> > > -						   0, link_freq);
> > > +						   imx214-
> > > >bus_cfg.nr_of_link_frequencies - 1,
> > > +						   0, imx214-
> > > >bus_cfg.link_frequencies);
> > >  	if (imx214->link_freq)
> > >  		imx214->link_freq->flags |=
> > > V4L2_CTRL_FLAG_READ_ONLY;
> > >  
> > > @@ -1006,6 +1014,7 @@ static int imx214_start_streaming(struct
> > > imx214 *imx214)
> > >  	const struct v4l2_mbus_framefmt *fmt;
> > >  	struct v4l2_subdev_state *state;
> > >  	const struct imx214_mode *mode;
> > > +	int bit_rate_mbps;
> > >  	int ret;
> > >  
> > >  	ret = cci_multi_reg_write(imx214->regmap,
> > > mode_table_common,
> > > @@ -1021,8 +1030,10 @@ static int imx214_start_streaming(struct
> > > imx214 *imx214)
> > >  		return ret;
> > >  	}
> > >  
> > > +	bit_rate_mbps = (imx214->pll.pixel_rate_pixel_array /

You should btw. use pll.pixel_rate_csi here instead: this is on the link,
not the pixel array.

> > > 1000000)
> > > +			* imx214->pll.bits_per_pixel;
> > >  	ret = cci_write(imx214->regmap,
> > > IMX214_REG_REQ_LINK_BIT_RATE,
> > > -			IMX214_LINK_BIT_RATE_MBPS(4800), NULL);
> > > +			IMX214_LINK_BIT_RATE_MBPS(bit_rate_mbps),
> > > NULL);
> > >  	if (ret) {
> > >  		dev_err(imx214->dev, "failed to configure link bit
> > > rate\n");
> > >  		return ret;
> > > @@ -1105,6 +1116,112 @@ static int imx214_s_stream(struct
> > > v4l2_subdev *subdev, int enable)
> > >  	return ret;
> > >  }
> > >  
> > > +static int imx214_pll_calculate(struct imx214 *imx214, struct
> > > ccs_pll *pll,
> > > +				unsigned int link_freq)
> > > +{
> > > +	struct ccs_pll_limits limits = {
> > > +		.min_ext_clk_freq_hz = 6000000,
> > > +		.max_ext_clk_freq_hz = 27000000,
> > > +
> > > +		.vt_fr = {
> > > +			.min_pre_pll_clk_div = 1,
> > > +			.max_pre_pll_clk_div = 15,
> > > +			/* min_pll_op_clk_freq_hz /
> > > max_pll_multiplier */
> > > +			.min_pll_ip_clk_freq_hz = 281667,
> > > +			/* max_pll_op_clk_freq_hz /
> > > min_pll_multiplier */
> > > +			.max_pll_ip_clk_freq_hz = 100000000,
> > 
> > Regarding these limits -- the pll_ip_clk_freq_hz limits are likely
> > between around 6 MHz (lower limit) and between 12 MHz and 27 MHz. I'd
> > use 6 for lower and 12 for higher if they yield correct configuration
> > currently and loosen them up only if needed.
> 
> The range 6-12 MHz seems to work.
> With the updated min/max values, the comments are no longer valid,
> should I just remove them or what alternative comments could be used?

I'd still leave there the fact these are just educated guesses, not
manufacturer-defined values.

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[André Apitzsch]

Hello Sakari,

Am Dienstag, dem 06.05.2025 um 08:24 +0000 schrieb Sakari Ailus:
> Hi André,
> 
> On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay
> wrote:
> > From: André Apitzsch <git@apitzsch.eu>
> > 
> > Instead rely on the rate set on the clock (using assigned-clock-
> > rates
> > etc.)
> > 
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/media/i2c/imx214.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx214.c
> > b/drivers/media/i2c/imx214.c
> > index
> > 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254
> > f1e0d14dc064423 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -32,7 +32,6 @@
> >  
> >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> >  
> > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> >  /* Keep wrong link frequency for backward compatibility */
> >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client
> > *client)
> >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> >  				     "failed to get xclk\n");
> >  
> > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > -	if (ret)
> > -		return dev_err_probe(dev, ret,
> > -				     "failed to set xclk
> > frequency\n");
> > -
> 
> Oops. I missed this is what the driver was doing already. Indeed,
> this is one of the historic sensor drivers that do set the frequency
> in DT systems.
> 
> The driver never used the clock-frequency property and instead used a
> fixed frequency. Changing the behaviour now could be problematic.
> 
> There are options here that I think we could do:
> 
> 1) use your v1 patch (4) which uses "clock-frequency" if it exists
> and otherwise uses the default, fixed frequency or
> 
> 2) set the frequency only if the "clock-frequency" property exists.
> The DT currently requires clock-frequency and the YAML conversion was
> done in 2020 whereas the driver is from 2018. If we do this, the
> clock-frequency should be deprecated (or even removed from bingings).
> 
> I wonder what others think. Cc'd Laurent in any case.

I would go with option 2 and add a patch which updates the bindings,
i.e. which deprecates 'clock-frequency',  to this series.

Best regards,
André
> 
> >  	ret = imx214_get_regulators(dev, imx214);
> >  	if (ret < 0)
> >  		return dev_err_probe(dev, ret, "failed to get
> > regulators\n");
> > 

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[Laurent Pinchart]

Hi Sakari,

On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay wrote:
> > From: André Apitzsch <git@apitzsch.eu>
> > 
> > Instead rely on the rate set on the clock (using assigned-clock-rates
> > etc.)
> > 
> > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > ---
> >  drivers/media/i2c/imx214.c | 6 ------
> >  1 file changed, 6 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> > --- a/drivers/media/i2c/imx214.c
> > +++ b/drivers/media/i2c/imx214.c
> > @@ -32,7 +32,6 @@
> >  
> >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> >  
> > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> >  /* Keep wrong link frequency for backward compatibility */
> >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
> >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> >  				     "failed to get xclk\n");
> >  
> > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > -	if (ret)
> > -		return dev_err_probe(dev, ret,
> > -				     "failed to set xclk frequency\n");
> > -
> 
> Oops. I missed this is what the driver was doing already. Indeed, this is
> one of the historic sensor drivers that do set the frequency in DT systems.
> 
> The driver never used the clock-frequency property and instead used a fixed
> frequency. Changing the behaviour now could be problematic.
> 
> There are options here that I think we could do:
> 
> 1) use your v1 patch (4) which uses "clock-frequency" if it exists and
> otherwise uses the default, fixed frequency or
> 
> 2) set the frequency only if the "clock-frequency" property exists. The DT
> currently requires clock-frequency and the YAML conversion was done in 2020
> whereas the driver is from 2018. If we do this, the clock-frequency should
> be deprecated (or even removed from bingings).
> 
> I wonder what others think. Cc'd Laurent in any case.

Maybe I'm missing something, but I don't really see the issue here. The
clock-frequency DT property is currently ignored, and this patch doesn't
change that situation, does it ?

The change of behaviour here is related to the assigned-clock-rates
property. If that property is specified today, it will set the clock
rate, and the driver will override it to 24MHz right after. With this
patch, the clock rate won't be overridden. I think the risk of
regression is very low here, as I don't expect systems to set
assigned-clock-rates in DT to a value different than 24MHz and expect
the driver to override it.

> >  	ret = imx214_get_regulators(dev, imx214);
> >  	if (ret < 0)
> >  		return dev_err_probe(dev, ret, "failed to get regulators\n");

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[Sakari Ailus]

Hi Laurent,

On Thu, May 15, 2025 at 10:58:46AM +0200, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> > On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay wrote:
> > > From: André Apitzsch <git@apitzsch.eu>
> > > 
> > > Instead rely on the rate set on the clock (using assigned-clock-rates
> > > etc.)
> > > 
> > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > ---
> > >  drivers/media/i2c/imx214.c | 6 ------
> > >  1 file changed, 6 deletions(-)
> > > 
> > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> > > --- a/drivers/media/i2c/imx214.c
> > > +++ b/drivers/media/i2c/imx214.c
> > > @@ -32,7 +32,6 @@
> > >  
> > >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> > >  
> > > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> > >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> > >  /* Keep wrong link frequency for backward compatibility */
> > >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
> > >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> > >  				     "failed to get xclk\n");
> > >  
> > > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > > -	if (ret)
> > > -		return dev_err_probe(dev, ret,
> > > -				     "failed to set xclk frequency\n");
> > > -
> > 
> > Oops. I missed this is what the driver was doing already. Indeed, this is
> > one of the historic sensor drivers that do set the frequency in DT systems.
> > 
> > The driver never used the clock-frequency property and instead used a fixed
> > frequency. Changing the behaviour now could be problematic.
> > 
> > There are options here that I think we could do:
> > 
> > 1) use your v1 patch (4) which uses "clock-frequency" if it exists and
> > otherwise uses the default, fixed frequency or
> > 
> > 2) set the frequency only if the "clock-frequency" property exists. The DT
> > currently requires clock-frequency and the YAML conversion was done in 2020
> > whereas the driver is from 2018. If we do this, the clock-frequency should
> > be deprecated (or even removed from bingings).
> > 
> > I wonder what others think. Cc'd Laurent in any case.
> 
> Maybe I'm missing something, but I don't really see the issue here. The
> clock-frequency DT property is currently ignored, and this patch doesn't
> change that situation, does it ?
> 
> The change of behaviour here is related to the assigned-clock-rates
> property. If that property is specified today, it will set the clock
> rate, and the driver will override it to 24MHz right after. With this
> patch, the clock rate won't be overridden. I think the risk of
> regression is very low here, as I don't expect systems to set
> assigned-clock-rates in DT to a value different than 24MHz and expect
> the driver to override it.

If the DTS had assigned-clock-rates set correctly, then yes. How much can
we trust the older DTS did have that?

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[Laurent Pinchart]

On Thu, May 15, 2025 at 02:54:54PM +0300, Sakari Ailus wrote:
> On Thu, May 15, 2025 at 10:58:46AM +0200, Laurent Pinchart wrote:
> > On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> > > On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay wrote:
> > > > From: André Apitzsch <git@apitzsch.eu>
> > > > 
> > > > Instead rely on the rate set on the clock (using assigned-clock-rates
> > > > etc.)
> > > > 
> > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > ---
> > > >  drivers/media/i2c/imx214.c | 6 ------
> > > >  1 file changed, 6 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> > > > --- a/drivers/media/i2c/imx214.c
> > > > +++ b/drivers/media/i2c/imx214.c
> > > > @@ -32,7 +32,6 @@
> > > >  
> > > >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> > > >  
> > > > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> > > >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> > > >  /* Keep wrong link frequency for backward compatibility */
> > > >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > > > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
> > > >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> > > >  				     "failed to get xclk\n");
> > > >  
> > > > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > > > -	if (ret)
> > > > -		return dev_err_probe(dev, ret,
> > > > -				     "failed to set xclk frequency\n");
> > > > -
> > > 
> > > Oops. I missed this is what the driver was doing already. Indeed, this is
> > > one of the historic sensor drivers that do set the frequency in DT systems.
> > > 
> > > The driver never used the clock-frequency property and instead used a fixed
> > > frequency. Changing the behaviour now could be problematic.
> > > 
> > > There are options here that I think we could do:
> > > 
> > > 1) use your v1 patch (4) which uses "clock-frequency" if it exists and
> > > otherwise uses the default, fixed frequency or
> > > 
> > > 2) set the frequency only if the "clock-frequency" property exists. The DT
> > > currently requires clock-frequency and the YAML conversion was done in 2020
> > > whereas the driver is from 2018. If we do this, the clock-frequency should
> > > be deprecated (or even removed from bingings).
> > > 
> > > I wonder what others think. Cc'd Laurent in any case.
> > 
> > Maybe I'm missing something, but I don't really see the issue here. The
> > clock-frequency DT property is currently ignored, and this patch doesn't
> > change that situation, does it ?
> > 
> > The change of behaviour here is related to the assigned-clock-rates
> > property. If that property is specified today, it will set the clock
> > rate, and the driver will override it to 24MHz right after. With this
> > patch, the clock rate won't be overridden. I think the risk of
> > regression is very low here, as I don't expect systems to set
> > assigned-clock-rates in DT to a value different than 24MHz and expect
> > the driver to override it.
> 
> If the DTS had assigned-clock-rates set correctly, then yes. How much can
> we trust the older DTS did have that?

I am relatively confident that DT-based systems wouldn't have an
assigned-clock-rates property with a frequency that doesn't match
IMX214_DEFAULT_CLK_FREQ. The real question is whether or not I'm
over-confident :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[Sakari Ailus]

Hi Laurent,

On Thu, May 15, 2025 at 03:03:40PM +0200, Laurent Pinchart wrote:
> On Thu, May 15, 2025 at 02:54:54PM +0300, Sakari Ailus wrote:
> > On Thu, May 15, 2025 at 10:58:46AM +0200, Laurent Pinchart wrote:
> > > On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> > > > On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via B4 Relay wrote:
> > > > > From: André Apitzsch <git@apitzsch.eu>
> > > > > 
> > > > > Instead rely on the rate set on the clock (using assigned-clock-rates
> > > > > etc.)
> > > > > 
> > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > > ---
> > > > >  drivers/media/i2c/imx214.c | 6 ------
> > > > >  1 file changed, 6 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > > index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> > > > > --- a/drivers/media/i2c/imx214.c
> > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > @@ -32,7 +32,6 @@
> > > > >  
> > > > >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> > > > >  
> > > > > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> > > > >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> > > > >  /* Keep wrong link frequency for backward compatibility */
> > > > >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > > > > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
> > > > >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> > > > >  				     "failed to get xclk\n");
> > > > >  
> > > > > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > > > > -	if (ret)
> > > > > -		return dev_err_probe(dev, ret,
> > > > > -				     "failed to set xclk frequency\n");
> > > > > -
> > > > 
> > > > Oops. I missed this is what the driver was doing already. Indeed, this is
> > > > one of the historic sensor drivers that do set the frequency in DT systems.
> > > > 
> > > > The driver never used the clock-frequency property and instead used a fixed
> > > > frequency. Changing the behaviour now could be problematic.
> > > > 
> > > > There are options here that I think we could do:
> > > > 
> > > > 1) use your v1 patch (4) which uses "clock-frequency" if it exists and
> > > > otherwise uses the default, fixed frequency or
> > > > 
> > > > 2) set the frequency only if the "clock-frequency" property exists. The DT
> > > > currently requires clock-frequency and the YAML conversion was done in 2020
> > > > whereas the driver is from 2018. If we do this, the clock-frequency should
> > > > be deprecated (or even removed from bingings).
> > > > 
> > > > I wonder what others think. Cc'd Laurent in any case.
> > > 
> > > Maybe I'm missing something, but I don't really see the issue here. The
> > > clock-frequency DT property is currently ignored, and this patch doesn't
> > > change that situation, does it ?
> > > 
> > > The change of behaviour here is related to the assigned-clock-rates
> > > property. If that property is specified today, it will set the clock
> > > rate, and the driver will override it to 24MHz right after. With this
> > > patch, the clock rate won't be overridden. I think the risk of
> > > regression is very low here, as I don't expect systems to set
> > > assigned-clock-rates in DT to a value different than 24MHz and expect
> > > the driver to override it.
> > 
> > If the DTS had assigned-clock-rates set correctly, then yes. How much can
> > we trust the older DTS did have that?
> 
> I am relatively confident that DT-based systems wouldn't have an
> assigned-clock-rates property with a frequency that doesn't match
> IMX214_DEFAULT_CLK_FREQ. The real question is whether or not I'm
> over-confident :-)

The assigned-clock stuff wasn't always there. But nowadays I guess a lot
of things in practice depends on it.

So I guess doing this should be fine then.

-- 
Sakari Ailus

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[André Apitzsch]

Hi Sakari, Hi Laurent,

Am Freitag, dem 16.05.2025 um 00:02 +0300 schrieb Sakari Ailus:
> Hi Laurent,
> 
> On Thu, May 15, 2025 at 03:03:40PM +0200, Laurent Pinchart wrote:
> > On Thu, May 15, 2025 at 02:54:54PM +0300, Sakari Ailus wrote:
> > > On Thu, May 15, 2025 at 10:58:46AM +0200, Laurent Pinchart wrote:
> > > > On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> > > > > On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via
> > > > > B4 Relay wrote:
> > > > > > From: André Apitzsch <git@apitzsch.eu>
> > > > > > 
> > > > > > Instead rely on the rate set on the clock (using assigned-
> > > > > > clock-rates
> > > > > > etc.)
> > > > > > 
> > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > > > ---
> > > > > >  drivers/media/i2c/imx214.c | 6 ------
> > > > > >  1 file changed, 6 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/i2c/imx214.c
> > > > > > b/drivers/media/i2c/imx214.c
> > > > > > index
> > > > > > 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb1
> > > > > > 8c608254f1e0d14dc064423 100644
> > > > > > --- a/drivers/media/i2c/imx214.c
> > > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > > @@ -32,7 +32,6 @@
> > > > > >  
> > > > > >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> > > > > >  
> > > > > > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> > > > > >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> > > > > >  /* Keep wrong link frequency for backward compatibility */
> > > > > >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > > > > > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct
> > > > > > i2c_client *client)
> > > > > >  		return dev_err_probe(dev, PTR_ERR(imx214-
> > > > > > >xclk),
> > > > > >  				     "failed to get
> > > > > > xclk\n");
> > > > > >  
> > > > > > -	ret = clk_set_rate(imx214->xclk,
> > > > > > IMX214_DEFAULT_CLK_FREQ);
> > > > > > -	if (ret)
> > > > > > -		return dev_err_probe(dev, ret,
> > > > > > -				     "failed to set xclk
> > > > > > frequency\n");
> > > > > > -
> > > > > 
> > > > > Oops. I missed this is what the driver was doing already.
> > > > > Indeed, this is one of the historic sensor drivers that do
> > > > > set the frequency in DT systems.
> > > > > 
> > > > > The driver never used the clock-frequency property and
> > > > > instead used a fixed frequency. Changing the behaviour now
> > > > > could be problematic.
> > > > > 
> > > > > There are options here that I think we could do:
> > > > > 
> > > > > 1) use your v1 patch (4) which uses "clock-frequency" if it
> > > > > exists and otherwise uses the default, fixed frequency or
> > > > > 
> > > > > 2) set the frequency only if the "clock-frequency" property
> > > > > exists. The DT currently requires clock-frequency and the
> > > > > YAML conversion was done in 2020 whereas the driver is from
> > > > > 2018. If we do this, the clock-frequency should
> > > > > be deprecated (or even removed from bingings).
> > > > > 
> > > > > I wonder what others think. Cc'd Laurent in any case.
> > > > 
> > > > Maybe I'm missing something, but I don't really see the issue
> > > > here. The clock-frequency DT property is currently ignored, and
> > > > this patch doesn't change that situation, does it ?
> > > > 
> > > > The change of behaviour here is related to the assigned-clock-
> > > > rates property. If that property is specified today, it will
> > > > set the clock rate, and the driver will override it to 24MHz
> > > > right after.
> > > > With this patch, the clock rate won't be overridden. I think
> > > > the risk of regression is very low here, as I don't expect
> > > > systems to set assigned-clock-rates in DT to a value different
> > > > than 24MHz and expect the driver to override it.
> > > 
> > > If the DTS had assigned-clock-rates set correctly, then yes. How
> > > much can we trust the older DTS did have that?
> > 
> > I am relatively confident that DT-based systems wouldn't have an
> > assigned-clock-rates property with a frequency that doesn't match
> > IMX214_DEFAULT_CLK_FREQ. The real question is whether or not I'm
> > over-confident :-)
> 
> The assigned-clock stuff wasn't always there. But nowadays I guess a
> lot of things in practice depends on it.
> 
> So I guess doing this should be fine then.

Just to be clear, this patch is fine and no changes are needed?

Should the bindings be updated in this series or can that be done
later?

Best regards,
André

Re: [PATCH v2 4/4] media: i2c: imx214: Remove hard-coded external clock frequency

[Laurent Pinchart]

Hi André,

On Thu, May 15, 2025 at 11:21:42PM +0200, André Apitzsch wrote:
> Am Freitag, dem 16.05.2025 um 00:02 +0300 schrieb Sakari Ailus:
> > On Thu, May 15, 2025 at 03:03:40PM +0200, Laurent Pinchart wrote:
> > > On Thu, May 15, 2025 at 02:54:54PM +0300, Sakari Ailus wrote:
> > > > On Thu, May 15, 2025 at 10:58:46AM +0200, Laurent Pinchart wrote:
> > > > > On Tue, May 06, 2025 at 08:24:03AM +0000, Sakari Ailus wrote:
> > > > > > On Mon, May 05, 2025 at 11:05:56PM +0200, André Apitzsch via
> > > > > > B4 Relay wrote:
> > > > > > > From: André Apitzsch <git@apitzsch.eu>
> > > > > > > 
> > > > > > > Instead rely on the rate set on the clock (using assigned-clock-rates
> > > > > > > etc.)
> > > > > > > 
> > > > > > > Signed-off-by: André Apitzsch <git@apitzsch.eu>
> > > > > > > ---
> > > > > > >  drivers/media/i2c/imx214.c | 6 ------
> > > > > > >  1 file changed, 6 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > > > > > > index 9e9be47394ec768a5b34d44b06b5bbb0988da5a1..c12996e294dccebb18c608254f1e0d14dc064423 100644
> > > > > > > --- a/drivers/media/i2c/imx214.c
> > > > > > > +++ b/drivers/media/i2c/imx214.c
> > > > > > > @@ -32,7 +32,6 @@
> > > > > > >  
> > > > > > >  #define IMX214_REG_FAST_STANDBY_CTRL	CCI_REG8(0x0106)
> > > > > > >  
> > > > > > > -#define IMX214_DEFAULT_CLK_FREQ	24000000
> > > > > > >  #define IMX214_DEFAULT_LINK_FREQ	600000000
> > > > > > >  /* Keep wrong link frequency for backward compatibility */
> > > > > > >  #define IMX214_DEFAULT_LINK_FREQ_LEGACY	480000000
> > > > > > > @@ -1405,11 +1404,6 @@ static int imx214_probe(struct i2c_client *client)
> > > > > > >  		return dev_err_probe(dev, PTR_ERR(imx214->xclk),
> > > > > > >  				     "failed to get xclk\n");
> > > > > > >  
> > > > > > > -	ret = clk_set_rate(imx214->xclk, IMX214_DEFAULT_CLK_FREQ);
> > > > > > > -	if (ret)
> > > > > > > -		return dev_err_probe(dev, ret,
> > > > > > > -				     "failed to set xclk frequency\n");
> > > > > > > -
> > > > > > 
> > > > > > Oops. I missed this is what the driver was doing already.
> > > > > > Indeed, this is one of the historic sensor drivers that do
> > > > > > set the frequency in DT systems.
> > > > > > 
> > > > > > The driver never used the clock-frequency property and
> > > > > > instead used a fixed frequency. Changing the behaviour now
> > > > > > could be problematic.
> > > > > > 
> > > > > > There are options here that I think we could do:
> > > > > > 
> > > > > > 1) use your v1 patch (4) which uses "clock-frequency" if it
> > > > > > exists and otherwise uses the default, fixed frequency or
> > > > > > 
> > > > > > 2) set the frequency only if the "clock-frequency" property
> > > > > > exists. The DT currently requires clock-frequency and the
> > > > > > YAML conversion was done in 2020 whereas the driver is from
> > > > > > 2018. If we do this, the clock-frequency should
> > > > > > be deprecated (or even removed from bingings).
> > > > > > 
> > > > > > I wonder what others think. Cc'd Laurent in any case.
> > > > > 
> > > > > Maybe I'm missing something, but I don't really see the issue
> > > > > here. The clock-frequency DT property is currently ignored, and
> > > > > this patch doesn't change that situation, does it ?
> > > > > 
> > > > > The change of behaviour here is related to the assigned-clock-
> > > > > rates property. If that property is specified today, it will
> > > > > set the clock rate, and the driver will override it to 24MHz
> > > > > right after.
> > > > > With this patch, the clock rate won't be overridden. I think
> > > > > the risk of regression is very low here, as I don't expect
> > > > > systems to set assigned-clock-rates in DT to a value different
> > > > > than 24MHz and expect the driver to override it.
> > > > 
> > > > If the DTS had assigned-clock-rates set correctly, then yes. How
> > > > much can we trust the older DTS did have that?
> > > 
> > > I am relatively confident that DT-based systems wouldn't have an
> > > assigned-clock-rates property with a frequency that doesn't match
> > > IMX214_DEFAULT_CLK_FREQ. The real question is whether or not I'm
> > > over-confident :-)
> > 
> > The assigned-clock stuff wasn't always there. But nowadays I guess a
> > lot of things in practice depends on it.
> > 
> > So I guess doing this should be fine then.
> 
> Just to be clear, this patch is fine and no changes are needed?

I'm fine with it.

> Should the bindings be updated in this series or can that be done
> later?

If you could do it in the same series it would be very appreciated. It
should be in a separate patch.

-- 
Regards,

Laurent Pinchart