[PATCH v2 0/5] imx335: Support additional link-freq and TPG

[PATCH v2 0/5] imx335: Support additional link-freq and TPG

[Umang Jain]

This series adds support for additional link frequency and
test pattern generator on IMX335.

Patch 1/5 is a drive-by patch which drops setting of a reserved
register. This register has no effect in operation/streaming of the
sensor.

Patch 2/5 adds usage of v4l2_link_freq_to_bitmap V4L2 helper.

Patch 3/4 supports for additional link-frequency supported by IMX335

Patch 4/5 is also a prep-up patch for TPG introduction(in 5/5), as the test
pattern needs sensor to be powered up to apply the test pattern.

Patch 5/5 adds the TPG.

Changes in v2:
- add new patch 2/5 to use v4l2_link_freq_to_bitmap
- fixup a return; in 4/5

Matthias Fend (1):
  media: i2c: imx335: Add support for test pattern generator

Umang Jain (4):
  media: i2c: imx335: Drop setting of 0x3a00 register
  media: imx335: Use v4l2_link_freq_to_bitmap helper
  media: i2c: imx335: Support multiple link frequency
  media: i2c: imx335: Refactor power sequence to set controls

 drivers/media/i2c/imx335.c | 250 ++++++++++++++++++++++++++++++-------
 1 file changed, 205 insertions(+), 45 deletions(-)


base-commit: d7cb6098f1d4866ae864cccdbb3fdcea1176a7f6
-- 
2.41.0

[PATCH v2 1/5] media: i2c: imx335: Drop setting of 0x3a00 register

[Umang Jain]

Register 0x3a00 is a reserved field as per the IMX335 datasheet,
hence shouldn't be set by the driver.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 7a37eb327ff4..927b4806a5d7 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -249,7 +249,6 @@ static const struct imx335_reg mode_2592x1940_regs[] = {
 	{0x3794, 0x7a},
 	{0x3796, 0xa1},
 	{0x37b0, 0x36},
-	{0x3a00, 0x01},
 };
 
 static const struct imx335_reg raw10_framefmt_regs[] = {
-- 
2.41.0

[PATCH v2 2/5] media: imx335: Use v4l2_link_freq_to_bitmap helper

[Umang Jain]

Use the v4l2_link_freq_to_bitmap() helper to figure out which
driver-supported link frequencies can be used on a given system.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 927b4806a5d7..73691069556f 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -49,7 +49,7 @@
 #define IMX335_INCLK_RATE	24000000
 
 /* CSI2 HW configuration */
-#define IMX335_LINK_FREQ	594000000
+#define IMX335_LINK_FREQ	594000000LL
 #define IMX335_NUM_DATA_LANES	4
 
 #define IMX335_REG_MIN		0x00
@@ -134,6 +134,7 @@ struct imx335_mode {
  * @vblank: Vertical blanking in lines
  * @cur_mode: Pointer to current selected sensor mode
  * @mutex: Mutex for serializing sensor controls
+ * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
  * @cur_mbus_code: Currently selected media bus format code
  */
 struct imx335 {
@@ -157,6 +158,7 @@ struct imx335 {
 	u32 vblank;
 	const struct imx335_mode *cur_mode;
 	struct mutex mutex;
+	unsigned long link_freq_bitmap;
 	u32 cur_mbus_code;
 };
 
@@ -404,7 +406,8 @@ static int imx335_update_controls(struct imx335 *imx335,
 {
 	int ret;
 
-	ret = __v4l2_ctrl_s_ctrl(imx335->link_freq_ctrl, mode->link_freq_idx);
+	ret = __v4l2_ctrl_s_ctrl(imx335->link_freq_ctrl,
+				 __ffs(imx335->link_freq_bitmap));
 	if (ret)
 		return ret;
 
@@ -690,6 +693,13 @@ static int imx335_init_state(struct v4l2_subdev *sd,
 	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
 	imx335_fill_pad_format(imx335, &supported_mode, &fmt);
 
+	mutex_lock(&imx335->mutex);
+	__v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
+				 __fls(imx335->link_freq_bitmap),
+				 ~(imx335->link_freq_bitmap),
+				 __ffs(imx335->link_freq_bitmap));
+	mutex_unlock(&imx335->mutex);
+
 	return imx335_set_pad_format(sd, sd_state, &fmt);
 }
 
@@ -938,19 +948,10 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
 		goto done_endpoint_free;
 	}
 
-	if (!bus_cfg.nr_of_link_frequencies) {
-		dev_err(imx335->dev, "no link frequencies defined\n");
-		ret = -EINVAL;
-		goto done_endpoint_free;
-	}
-
-	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
-		if (bus_cfg.link_frequencies[i] == IMX335_LINK_FREQ)
-			goto done_endpoint_free;
-
-	dev_err(imx335->dev, "no compatible link frequencies found\n");
-
-	ret = -EINVAL;
+	ret = v4l2_link_freq_to_bitmap(imx335->dev, bus_cfg.link_frequencies,
+				       bus_cfg.nr_of_link_frequencies,
+				       link_freq, ARRAY_SIZE(link_freq),
+				       &imx335->link_freq_bitmap);
 
 done_endpoint_free:
 	v4l2_fwnode_endpoint_free(&bus_cfg);
@@ -1098,9 +1099,8 @@ static int imx335_init_controls(struct imx335 *imx335)
 	imx335->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
 							&imx335_ctrl_ops,
 							V4L2_CID_LINK_FREQ,
-							ARRAY_SIZE(link_freq) -
-							1,
-							mode->link_freq_idx,
+							__fls(imx335->link_freq_bitmap),
+							__ffs(imx335->link_freq_bitmap),
 							link_freq);
 	if (imx335->link_freq_ctrl)
 		imx335->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-- 
2.41.0

[PATCH v2 3/5] media: i2c: imx335: Support multiple link frequency

[Umang Jain]

Support link frequency of 445MHz in addition to 594MHz.
Break out the register set specific to each data lane rate and also add
the general timing register set corresponding to the each data
lane rate.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 84 +++++++++++++++++++++++++++++++-------
 1 file changed, 70 insertions(+), 14 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 73691069556f..ca3eab50f714 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -49,7 +49,9 @@
 #define IMX335_INCLK_RATE	24000000
 
 /* CSI2 HW configuration */
-#define IMX335_LINK_FREQ	594000000LL
+#define IMX335_LINK_FREQ_594MHz		594000000LL
+#define IMX335_LINK_FREQ_445MHz		445500000LL
+
 #define IMX335_NUM_DATA_LANES	4
 
 #define IMX335_REG_MIN		0x00
@@ -99,7 +101,6 @@ static const char * const imx335_supply_name[] = {
  * @vblank_min: Minimum vertical blanking in lines
  * @vblank_max: Maximum vertical blanking in lines
  * @pclk: Sensor pixel clock
- * @link_freq_idx: Link frequency index
  * @reg_list: Register list for sensor mode
  */
 struct imx335_mode {
@@ -111,7 +112,6 @@ struct imx335_mode {
 	u32 vblank_min;
 	u32 vblank_max;
 	u64 pclk;
-	u32 link_freq_idx;
 	struct imx335_reg_list reg_list;
 };
 
@@ -162,16 +162,10 @@ struct imx335 {
 	u32 cur_mbus_code;
 };
 
-static const s64 link_freq[] = {
-	IMX335_LINK_FREQ,
-};
-
 /* Sensor mode registers */
 static const struct imx335_reg mode_2592x1940_regs[] = {
 	{0x3000, 0x01},
 	{0x3002, 0x00},
-	{0x300c, 0x3b},
-	{0x300d, 0x2a},
 	{0x3018, 0x04},
 	{0x302c, 0x3c},
 	{0x302e, 0x20},
@@ -179,10 +173,6 @@ static const struct imx335_reg mode_2592x1940_regs[] = {
 	{0x3074, 0xc8},
 	{0x3076, 0x28},
 	{0x304c, 0x00},
-	{0x314c, 0xc6},
-	{0x315a, 0x02},
-	{0x3168, 0xa0},
-	{0x316a, 0x7e},
 	{0x31a1, 0x00},
 	{0x3288, 0x21},
 	{0x328a, 0x02},
@@ -267,6 +257,65 @@ static const struct imx335_reg raw12_framefmt_regs[] = {
 	{0x341d, 0x00},
 };
 
+static const struct imx335_reg mipi_data_rate_1188Mbps[] = {
+	{0x300c, 0x3b},
+	{0x300d, 0x2a},
+	{0x314c, 0xc6},
+	{0x314d, 0x00},
+	{0x315a, 0x02},
+	{0x3168, 0xa0},
+	{0x316a, 0x7e},
+	{0x319e, 0x01},
+	{0x3a18, 0x8f},
+	{0x3a1a, 0x4f},
+	{0x3a1c, 0x47},
+	{0x3a1e, 0x37},
+	{0x3a1f, 0x01},
+	{0x3a20, 0x4f},
+	{0x3a22, 0x87},
+	{0x3a24, 0x4f},
+	{0x3a26, 0x7f},
+	{0x3a28, 0x3f},
+};
+
+static const struct imx335_reg mipi_data_rate_891Mbps[] = {
+	{0x300c, 0x3b},
+	{0x300d, 0x2a},
+	{0x314c, 0x29},
+	{0x314d, 0x01},
+	{0x315a, 0x06},
+	{0x3168, 0xa0},
+	{0x316a, 0x7e},
+	{0x319e, 0x02},
+	{0x3a18, 0x7f},
+	{0x3a1a, 0x37},
+	{0x3a1c, 0x37},
+	{0x3a1e, 0xf7},
+	{0x3a20, 0x3f},
+	{0x3a22, 0x6f},
+	{0x3a24, 0x3f},
+	{0x3a26, 0x5f},
+	{0x3a28, 0x2f},
+};
+
+static const s64 link_freq[] = {
+	/* Corresponds to 1188Mbps data lane rate */
+	IMX335_LINK_FREQ_594MHz,
+	/* Corresponds to 891Mbps data lane rate */
+	IMX335_LINK_FREQ_445MHz,
+};
+
+static const struct imx335_reg_list link_freq_reglist[] = {
+	{
+		.num_of_regs = ARRAY_SIZE(mipi_data_rate_1188Mbps),
+		.regs = mipi_data_rate_1188Mbps,
+	},
+	{
+		.num_of_regs = ARRAY_SIZE(mipi_data_rate_891Mbps),
+		.regs = mipi_data_rate_891Mbps,
+	},
+};
+
 static const u32 imx335_mbus_codes[] = {
 	MEDIA_BUS_FMT_SRGGB12_1X12,
 	MEDIA_BUS_FMT_SRGGB10_1X10,
@@ -281,7 +330,6 @@ static const struct imx335_mode supported_mode = {
 	.vblank_min = 2560,
 	.vblank_max = 133060,
 	.pclk = 396000000,
-	.link_freq_idx = 0,
 	.reg_list = {
 		.num_of_regs = ARRAY_SIZE(mode_2592x1940_regs),
 		.regs = mode_2592x1940_regs,
@@ -764,6 +812,14 @@ static int imx335_start_streaming(struct imx335 *imx335)
 	const struct imx335_reg_list *reg_list;
 	int ret;
 
+	/* Setup PLL */
+	reg_list = &link_freq_reglist[__ffs(imx335->link_freq_bitmap)];
+	ret = imx335_write_regs(imx335, reg_list->regs, reg_list->num_of_regs);
+	if (ret) {
+		dev_err(imx335->dev, "%s failed to set plls\n", __func__);
+		return ret;
+	}
+
 	/* Write sensor mode registers */
 	reg_list = &imx335->cur_mode->reg_list;
 	ret = imx335_write_regs(imx335, reg_list->regs,
-- 
2.41.0

[PATCH v2 4/5] media: i2c: imx335: Refactor power sequence to set controls

[Umang Jain]

Additional controls might require the sensor to be powered up
to set the control value. Currently, only the exposure control
powers up the sensor.

Move the power up sequence out of the switch-case block.
In a subsequent patch, test pattern control will be added that
needs the sensor to be powered up. Hence, refactor the power
sequence to be present outside the switch-case block.

The VBLANK control is also moved out of the switch-case in order
to be handled early on, to propagate the changes to other controls.

Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 35 ++++++++++++++++++++---------------
 1 file changed, 20 insertions(+), 15 deletions(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index ca3eab50f714..250b7136048d 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -526,26 +526,31 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
 	u32 exposure;
 	int ret;
 
-	switch (ctrl->id) {
-	case V4L2_CID_VBLANK:
+	/* Propagate change of current control to all related controls */
+	if (ctrl->id == V4L2_CID_VBLANK) {
 		imx335->vblank = imx335->vblank_ctrl->val;
 
 		dev_dbg(imx335->dev, "Received vblank %u, new lpfr %u\n",
 			imx335->vblank,
 			imx335->vblank + imx335->cur_mode->height);
 
-		ret = __v4l2_ctrl_modify_range(imx335->exp_ctrl,
-					       IMX335_EXPOSURE_MIN,
-					       imx335->vblank +
-					       imx335->cur_mode->height -
-					       IMX335_EXPOSURE_OFFSET,
-					       1, IMX335_EXPOSURE_DEFAULT);
-		break;
-	case V4L2_CID_EXPOSURE:
-		/* Set controls only if sensor is in power on state */
-		if (!pm_runtime_get_if_in_use(imx335->dev))
-			return 0;
+		return __v4l2_ctrl_modify_range(imx335->exp_ctrl,
+						IMX335_EXPOSURE_MIN,
+						imx335->vblank +
+						imx335->cur_mode->height -
+						IMX335_EXPOSURE_OFFSET,
+						1, IMX335_EXPOSURE_DEFAULT);
+	}
+
+	/*
+	 * Applying V4L2 control value only happens
+	 * when power is up for streaming.
+	 */
+	if (pm_runtime_get_if_in_use(imx335->dev) == 0)
+		return 0;
 
+	switch (ctrl->id) {
+	case V4L2_CID_EXPOSURE:
 		exposure = ctrl->val;
 		analog_gain = imx335->again_ctrl->val;
 
@@ -554,14 +559,14 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		ret = imx335_update_exp_gain(imx335, exposure, analog_gain);
 
-		pm_runtime_put(imx335->dev);
-
 		break;
 	default:
 		dev_err(imx335->dev, "Invalid control %d\n", ctrl->id);
 		ret = -EINVAL;
 	}
 
+	pm_runtime_put(imx335->dev);
+
 	return ret;
 }
 
-- 
2.41.0

[PATCH v2 5/5] media: i2c: imx335: Add support for test pattern generator

[Umang Jain]

From: Matthias Fend <matthias.fend@emfend.at>

Add support for the sensor's test pattern generator.

Signed-off-by: Matthias Fend <matthias.fend@emfend.at>
Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
---
 drivers/media/i2c/imx335.c | 102 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 101 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
index 250b7136048d..a003da8a81b1 100644
--- a/drivers/media/i2c/imx335.c
+++ b/drivers/media/i2c/imx335.c
@@ -45,6 +45,21 @@
 /* Group hold register */
 #define IMX335_REG_HOLD		0x3001
 
+/* Test pattern generator */
+#define IMX335_REG_TPG		0x329e
+#define IMX335_TPG_ALL_000	0
+#define IMX335_TPG_ALL_FFF	1
+#define IMX335_TPG_ALL_555	2
+#define IMX335_TPG_ALL_AAA	3
+#define IMX335_TPG_TOG_555_AAA	4
+#define IMX335_TPG_TOG_AAA_555	5
+#define IMX335_TPG_TOG_000_555	6
+#define IMX335_TPG_TOG_555_000	7
+#define IMX335_TPG_TOG_000_FFF	8
+#define IMX335_TPG_TOG_FFF_000	9
+#define IMX335_TPG_H_COLOR_BARS 10
+#define IMX335_TPG_V_COLOR_BARS 11
+
 /* Input clock rate */
 #define IMX335_INCLK_RATE	24000000
 
@@ -162,6 +177,38 @@ struct imx335 {
 	u32 cur_mbus_code;
 };
 
+static const char * const imx335_tpg_menu[] = {
+	"Disabled",
+	"All 000h",
+	"All FFFh",
+	"All 555h",
+	"All AAAh",
+	"Toggle 555/AAAh",
+	"Toggle AAA/555h",
+	"Toggle 000/555h",
+	"Toggle 555/000h",
+	"Toggle 000/FFFh",
+	"Toggle FFF/000h",
+	"Horizontal color bars",
+	"Vertical color bars",
+};
+
+static const int imx335_tpg_val[] = {
+	IMX335_TPG_ALL_000,
+	IMX335_TPG_ALL_000,
+	IMX335_TPG_ALL_FFF,
+	IMX335_TPG_ALL_555,
+	IMX335_TPG_ALL_AAA,
+	IMX335_TPG_TOG_555_AAA,
+	IMX335_TPG_TOG_AAA_555,
+	IMX335_TPG_TOG_000_555,
+	IMX335_TPG_TOG_555_000,
+	IMX335_TPG_TOG_000_FFF,
+	IMX335_TPG_TOG_FFF_000,
+	IMX335_TPG_H_COLOR_BARS,
+	IMX335_TPG_V_COLOR_BARS,
+};
+
 /* Sensor mode registers */
 static const struct imx335_reg mode_2592x1940_regs[] = {
 	{0x3000, 0x01},
@@ -506,6 +553,49 @@ static int imx335_update_exp_gain(struct imx335 *imx335, u32 exposure, u32 gain)
 	return ret;
 }
 
+static int imx335_update_test_pattern(struct imx335 *imx335, u32 pattern_index)
+{
+	int ret;
+
+	if (pattern_index >= ARRAY_SIZE(imx335_tpg_val))
+		return -EINVAL;
+
+	if (pattern_index) {
+		const struct imx335_reg tpg_enable_regs[] = {
+			{ 0x3148, 0x10 },
+			{ 0x3280, 0x00 },
+			{ 0x329c, 0x01 },
+			{ 0x32a0, 0x11 },
+			{ 0x3302, 0x00 },
+			{ 0x3303, 0x00 },
+			{ 0x336c, 0x00 },
+		};
+
+		ret = imx335_write_reg(imx335, IMX335_REG_TPG, 1,
+				       imx335_tpg_val[pattern_index]);
+		if (ret)
+			return ret;
+
+		ret = imx335_write_regs(imx335, tpg_enable_regs,
+					ARRAY_SIZE(tpg_enable_regs));
+	} else {
+		const struct imx335_reg tpg_disable_regs[] = {
+			{ 0x3148, 0x00 },
+			{ 0x3280, 0x01 },
+			{ 0x329c, 0x00 },
+			{ 0x32a0, 0x10 },
+			{ 0x3302, 0x32 },
+			{ 0x3303, 0x00 },
+			{ 0x336c, 0x01 },
+		};
+
+		ret = imx335_write_regs(imx335, tpg_disable_regs,
+					ARRAY_SIZE(tpg_disable_regs));
+	}
+
+	return ret;
+}
+
 /**
  * imx335_set_ctrl() - Set subdevice control
  * @ctrl: pointer to v4l2_ctrl structure
@@ -559,6 +649,10 @@ static int imx335_set_ctrl(struct v4l2_ctrl *ctrl)
 
 		ret = imx335_update_exp_gain(imx335, exposure, analog_gain);
 
+		break;
+	case V4L2_CID_TEST_PATTERN:
+		ret = imx335_update_test_pattern(imx335, ctrl->val);
+
 		break;
 	default:
 		dev_err(imx335->dev, "Invalid control %d\n", ctrl->id);
@@ -1116,7 +1210,7 @@ static int imx335_init_controls(struct imx335 *imx335)
 	u32 lpfr;
 	int ret;
 
-	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 6);
+	ret = v4l2_ctrl_handler_init(ctrl_hdlr, 7);
 	if (ret)
 		return ret;
 
@@ -1150,6 +1244,12 @@ static int imx335_init_controls(struct imx335 *imx335)
 						mode->vblank_max,
 						1, mode->vblank);
 
+	v4l2_ctrl_new_std_menu_items(ctrl_hdlr,
+				     &imx335_ctrl_ops,
+				     V4L2_CID_TEST_PATTERN,
+				     ARRAY_SIZE(imx335_tpg_menu) - 1,
+				     0, 0, imx335_tpg_menu);
+
 	/* Read only controls */
 	imx335->pclk_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
 					      &imx335_ctrl_ops,
-- 
2.41.0

Re: [PATCH v2 1/5] media: i2c: imx335: Drop setting of 0x3a00 register

[Kieran Bingham]

Hi Umang,

+ Cc: Matthias

Quoting Umang Jain (2024-01-31 05:52:04)
> Register 0x3a00 is a reserved field as per the IMX335 datasheet,
> hence shouldn't be set by the driver.

We still need to explain more about why we're dropping this register
write, and what effects it causes.

Matthias - I believe this stemmed from the work you did, and I think I
recall that you stated this register write broke the CSI2 configuration?

Can you clarify anything here for us please?

--
Kieran


> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 7a37eb327ff4..927b4806a5d7 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -249,7 +249,6 @@ static const struct imx335_reg mode_2592x1940_regs[] = {
>         {0x3794, 0x7a},
>         {0x3796, 0xa1},
>         {0x37b0, 0x36},
> -       {0x3a00, 0x01},
>  };
>  
>  static const struct imx335_reg raw10_framefmt_regs[] = {
> -- 
> 2.41.0
>

Re: [PATCH v2 1/5] media: i2c: imx335: Drop setting of 0x3a00 register

[Matthias Fend]

Hi Kieran,

Am 31.01.2024 um 10:52 schrieb Kieran Bingham:
> Hi Umang,
> 
> + Cc: Matthias
> 
> Quoting Umang Jain (2024-01-31 05:52:04)
>> Register 0x3a00 is a reserved field as per the IMX335 datasheet,
>> hence shouldn't be set by the driver.
> 
> We still need to explain more about why we're dropping this register
> write, and what effects it causes.
> 
> Matthias - I believe this stemmed from the work you did, and I think I
> recall that you stated this register write broke the CSI2 configuration?
> 
> Can you clarify anything here for us please?

yes, that's correct.

Since this driver originally did not work in my setup, I came across 
this register while searching for differences to my working reference 
configuration.
With the default value of this register (0x00), the driver works 
perfectly. With the value previously written to it by the driver (0x01), 
I cannot receive any frames.
The problem may depend on the link frequency used.
I can only use and test a frequency of 445.5MHz on my hardware. Since 
only link frequencies of 594MHz were supported so far, this may not have 
been a problem.

Unfortunately I do not have a description of this register, so I can 
only speculate about the exact cause.

~Matthias

> 
> --
> Kieran
> 
> 
>>
>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>> ---
>>   drivers/media/i2c/imx335.c | 1 -
>>   1 file changed, 1 deletion(-)
>>
>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>> index 7a37eb327ff4..927b4806a5d7 100644
>> --- a/drivers/media/i2c/imx335.c
>> +++ b/drivers/media/i2c/imx335.c
>> @@ -249,7 +249,6 @@ static const struct imx335_reg mode_2592x1940_regs[] = {
>>          {0x3794, 0x7a},
>>          {0x3796, 0xa1},
>>          {0x37b0, 0x36},
>> -       {0x3a00, 0x01},
>>   };
>>   
>>   static const struct imx335_reg raw10_framefmt_regs[] = {
>> -- 
>> 2.41.0
>>

Re: [PATCH v2 1/5] media: i2c: imx335: Drop setting of 0x3a00 register

[Umang Jain]

Hi,

On 1/31/24 4:02 PM, Matthias Fend wrote:
> Hi Kieran,
>
> Am 31.01.2024 um 10:52 schrieb Kieran Bingham:
>> Hi Umang,
>>
>> + Cc: Matthias
>>
>> Quoting Umang Jain (2024-01-31 05:52:04)
>>> Register 0x3a00 is a reserved field as per the IMX335 datasheet,
>>> hence shouldn't be set by the driver.
>>
>> We still need to explain more about why we're dropping this register
>> write, and what effects it causes.
>>
>> Matthias - I believe this stemmed from the work you did, and I think I
>> recall that you stated this register write broke the CSI2 configuration?
>>
>> Can you clarify anything here for us please?
>
> yes, that's correct.
>
> Since this driver originally did not work in my setup, I came across 
> this register while searching for differences to my working reference 
> configuration.
> With the default value of this register (0x00), the driver works 
> perfectly. With the value previously written to it by the driver 
> (0x01), I cannot receive any frames.
> The problem may depend on the link frequency used.
> I can only use and test a frequency of 445.5MHz on my hardware. Since 
> only link frequencies of 594MHz were supported so far, this may not 
> have been a problem.
>
> Unfortunately I do not have a description of this register, so I can 
> only speculate about the exact cause.

Is it worth to frame the commit message around this speculation ?

My setup has no effect with this register being set or not.
>
> ~Matthias
>
>>
>> -- 
>> Kieran
>>
>>
>>>
>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
>>> ---
>>>   drivers/media/i2c/imx335.c | 1 -
>>>   1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
>>> index 7a37eb327ff4..927b4806a5d7 100644
>>> --- a/drivers/media/i2c/imx335.c
>>> +++ b/drivers/media/i2c/imx335.c
>>> @@ -249,7 +249,6 @@ static const struct imx335_reg 
>>> mode_2592x1940_regs[] = {
>>>          {0x3794, 0x7a},
>>>          {0x3796, 0xa1},
>>>          {0x37b0, 0x36},
>>> -       {0x3a00, 0x01},
>>>   };
>>>     static const struct imx335_reg raw10_framefmt_regs[] = {
>>> -- 
>>> 2.41.0
>>>

Re: [PATCH v2 1/5] media: i2c: imx335: Drop setting of 0x3a00 register

[Sakari Ailus]

Hi folks,

On Wed, Feb 07, 2024 at 09:51:32AM +0530, Umang Jain wrote:
> Hi,
> 
> On 1/31/24 4:02 PM, Matthias Fend wrote:
> > Hi Kieran,
> > 
> > Am 31.01.2024 um 10:52 schrieb Kieran Bingham:
> > > Hi Umang,
> > > 
> > > + Cc: Matthias
> > > 
> > > Quoting Umang Jain (2024-01-31 05:52:04)
> > > > Register 0x3a00 is a reserved field as per the IMX335 datasheet,
> > > > hence shouldn't be set by the driver.
> > > 
> > > We still need to explain more about why we're dropping this register
> > > write, and what effects it causes.
> > > 
> > > Matthias - I believe this stemmed from the work you did, and I think I
> > > recall that you stated this register write broke the CSI2 configuration?
> > > 
> > > Can you clarify anything here for us please?
> > 
> > yes, that's correct.
> > 
> > Since this driver originally did not work in my setup, I came across
> > this register while searching for differences to my working reference
> > configuration.
> > With the default value of this register (0x00), the driver works
> > perfectly. With the value previously written to it by the driver (0x01),
> > I cannot receive any frames.
> > The problem may depend on the link frequency used.
> > I can only use and test a frequency of 445.5MHz on my hardware. Since
> > only link frequencies of 594MHz were supported so far, this may not have
> > been a problem.
> > 
> > Unfortunately I do not have a description of this register, so I can
> > only speculate about the exact cause.
> 
> Is it worth to frame the commit message around this speculation ?
> 
> My setup has no effect with this register being set or not.

Very interesting case indeed.

Please elaborate this a little in the commit message, but the message
shouldn't be that because the register is marked rewerved, it shouldn't be
written (it's a commonplace when it comes to sensors :().

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 2/5] media: imx335: Use v4l2_link_freq_to_bitmap helper

[Sakari Ailus]

Hi Umang,

On Wed, Jan 31, 2024 at 11:22:05AM +0530, Umang Jain wrote:
> Use the v4l2_link_freq_to_bitmap() helper to figure out which
> driver-supported link frequencies can be used on a given system.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>
> ---
>  drivers/media/i2c/imx335.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx335.c b/drivers/media/i2c/imx335.c
> index 927b4806a5d7..73691069556f 100644
> --- a/drivers/media/i2c/imx335.c
> +++ b/drivers/media/i2c/imx335.c
> @@ -49,7 +49,7 @@
>  #define IMX335_INCLK_RATE	24000000
>  
>  /* CSI2 HW configuration */
> -#define IMX335_LINK_FREQ	594000000
> +#define IMX335_LINK_FREQ	594000000LL

If you change this, please make it ULL---it's unsigned.

>  #define IMX335_NUM_DATA_LANES	4
>  
>  #define IMX335_REG_MIN		0x00
> @@ -134,6 +134,7 @@ struct imx335_mode {
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
>   * @mutex: Mutex for serializing sensor controls
> + * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
>   * @cur_mbus_code: Currently selected media bus format code
>   */
>  struct imx335 {
> @@ -157,6 +158,7 @@ struct imx335 {
>  	u32 vblank;
>  	const struct imx335_mode *cur_mode;
>  	struct mutex mutex;
> +	unsigned long link_freq_bitmap;
>  	u32 cur_mbus_code;
>  };
>  
> @@ -404,7 +406,8 @@ static int imx335_update_controls(struct imx335 *imx335,
>  {
>  	int ret;
>  
> -	ret = __v4l2_ctrl_s_ctrl(imx335->link_freq_ctrl, mode->link_freq_idx);
> +	ret = __v4l2_ctrl_s_ctrl(imx335->link_freq_ctrl,
> +				 __ffs(imx335->link_freq_bitmap));
>  	if (ret)
>  		return ret;
>  
> @@ -690,6 +693,13 @@ static int imx335_init_state(struct v4l2_subdev *sd,
>  	fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
>  	imx335_fill_pad_format(imx335, &supported_mode, &fmt);
>  
> +	mutex_lock(&imx335->mutex);
> +	__v4l2_ctrl_modify_range(imx335->link_freq_ctrl, 0,
> +				 __fls(imx335->link_freq_bitmap),
> +				 ~(imx335->link_freq_bitmap),
> +				 __ffs(imx335->link_freq_bitmap));
> +	mutex_unlock(&imx335->mutex);
> +
>  	return imx335_set_pad_format(sd, sd_state, &fmt);
>  }
>  
> @@ -938,19 +948,10 @@ static int imx335_parse_hw_config(struct imx335 *imx335)
>  		goto done_endpoint_free;
>  	}
>  
> -	if (!bus_cfg.nr_of_link_frequencies) {
> -		dev_err(imx335->dev, "no link frequencies defined\n");
> -		ret = -EINVAL;
> -		goto done_endpoint_free;
> -	}
> -
> -	for (i = 0; i < bus_cfg.nr_of_link_frequencies; i++)
> -		if (bus_cfg.link_frequencies[i] == IMX335_LINK_FREQ)
> -			goto done_endpoint_free;
> -
> -	dev_err(imx335->dev, "no compatible link frequencies found\n");
> -
> -	ret = -EINVAL;
> +	ret = v4l2_link_freq_to_bitmap(imx335->dev, bus_cfg.link_frequencies,
> +				       bus_cfg.nr_of_link_frequencies,
> +				       link_freq, ARRAY_SIZE(link_freq),
> +				       &imx335->link_freq_bitmap);

Thanks! :-)

>  
>  done_endpoint_free:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
> @@ -1098,9 +1099,8 @@ static int imx335_init_controls(struct imx335 *imx335)
>  	imx335->link_freq_ctrl = v4l2_ctrl_new_int_menu(ctrl_hdlr,
>  							&imx335_ctrl_ops,
>  							V4L2_CID_LINK_FREQ,
> -							ARRAY_SIZE(link_freq) -
> -							1,
> -							mode->link_freq_idx,
> +							__fls(imx335->link_freq_bitmap),
> +							__ffs(imx335->link_freq_bitmap),
>  							link_freq);
>  	if (imx335->link_freq_ctrl)
>  		imx335->link_freq_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 3/5] media: i2c: imx335: Support multiple link frequency

[Sakari Ailus]

Hi Umang,

On Wed, Jan 31, 2024 at 11:22:06AM +0530, Umang Jain wrote:
> Support link frequency of 445MHz in addition to 594MHz.
> Break out the register set specific to each data lane rate and also add
> the general timing register set corresponding to the each data
> lane rate.
> 
> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com>

Please use the same subject line prefix (i.e. without "i2c: ").

-- 
Regards,

Sakari Ailus

Re: [PATCH v2 2/5] media: imx335: Use v4l2_link_freq_to_bitmap helper

[Sakari Ailus]

On Thu, Feb 15, 2024 at 12:51:19PM +0000, Sakari Ailus wrote:
> >  /* CSI2 HW configuration */
> > -#define IMX335_LINK_FREQ	594000000
> > +#define IMX335_LINK_FREQ	594000000LL
> 
> If you change this, please make it ULL---it's unsigned.

Ok, the control will be s64 so please ignore this.

-- 
Sakari Ailus