[PATCH v5 0/5] fix and improve for Hi846

[PATCH v5 0/5] fix and improve for Hi846

[Pengyu Luo]

This series fixes errors blocking Hi846 driver function, fixes
link frequency and supports 6MP and 8MP modes on Hi846.

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
Changes in v5:
- Use separated fields instead of raw register values for PLL cfg (Sakari)
- Use mul_u64_u32_div() to avoid loss of pricision and u64/u32 issues (Sakari)
- Use v4l2_find_nearest_size_conditional() to filter for different lane cases. (Sakari)
- Drop line break (Sakari)
- Rename post_div to mipi_clk_div
- Correct div1 number from 1 to 2
- Link to v4: https://lore.kernel.org/linux-media/20260511103927.279550-1-mitltlatltl@gmail.com

Changes in v4:
- Correct default lane cfg in commit for 'fix link frequency handling'
- Fix messed header (the Media CI robot)
- Move constant to the right side when comparsion (the Media CI robot)
- Link to v3: https://lore.kernel.org/linux-media/20260511090924.269106-1-mitltlatltl@gmail.com

Changes in v3:
- Fix modes handling for different lane cases (Sebastian)
- Fix header missing (the Media CI robot)
- Link to v2: https://lore.kernel.org/linux-media/20260501095433.1609309-1-mitltlatltl@gmail.com

Changes in v2:
- Remove uncessary `else` (Sakari)
- Fix link frequency (Sakari)
- Correct link frequency for DT
- Link to v1: https://lore.kernel.org/linux-media/20260429070351.1307204-1-mitltlatltl@gmail.com


Pengyu Luo (5):
  media: hi846: fix hi846_write_reg_16 handling
  media: hi846: Fix link frequency handling
  media: hi846: fix modes handling for different lane cases
  media: hi846: Add 6MP and 8MP modes support
  arm64: dts: imx8mq-librem5: Correct link frequency list

 .../boot/dts/freescale/imx8mq-librem5.dtsi    |   2 +-
 drivers/media/i2c/hi846.c                     | 296 +++++++++++++-----
 2 files changed, 227 insertions(+), 71 deletions(-)

-- 
2.54.0

[PATCH v5 2/5] media: hi846: Fix link frequency handling

[Pengyu Luo]

Link frequency is tied to PLL configuration, lane count, and external
clock rate, so use runtime here instead of hardcoding for specific
configuration. To implement this, we do

1. Drop exposed link_freq as a v4l2_ctrl, it is inconvenient to expose
it as an int_menu when freq_links are varies between mclk rates, this
will not break v4l2_get_link_freq() since it can be calculated back
from pixel rate which is previously from the dynamic link frequency.

2. Attach mipi_clk_div_{2,4}lane to current mode, and use the div with
mclk clock, lane count to calculate link frequency.

3. Drop mclk clock rate check.

Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
v5:
- Use separated fields instead of raw register values for PLL cfg (Sakari)
- Use mul_u64_u32_div() to avoid loss of pricision and u64/u32 issues (Sakari)
- Drop line break (Sakari)
---
Hi, Sakari. I did not follow to use v4l2_link_freq_to_bitmap(), which
you mentioned in v4, since we have no fixed int_menu for link_freq.
---
 drivers/media/i2c/hi846.c | 82 ++++++++++++++++++---------------------
 1 file changed, 37 insertions(+), 45 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 7f069aca0fce..6bcd862aee30 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1,7 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2021 Purism SPC
 
-#include <linux/unaligned.h>
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/delay.h>
 #include <linux/gpio/consumer.h>
@@ -11,6 +11,7 @@
 #include <linux/pm.h>
 #include <linux/property.h>
 #include <linux/regulator/consumer.h>
+#include <linux/unaligned.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
@@ -219,8 +220,8 @@ struct hi846_mode {
 	/* Horizontal timing size */
 	u32 llp;
 
-	/* Link frequency needed for this resolution */
-	u8 link_freq_index;
+	u8 mipi_clk_div_2lane;
+	u8 mipi_clk_div_4lane;
 
 	u16 fps;
 
@@ -1040,13 +1041,6 @@ static const char * const hi846_test_pattern_menu[] = {
 	"Resolution Pattern",
 };
 
-#define FREQ_INDEX_640	0
-#define FREQ_INDEX_1280	1
-static const s64 hi846_link_freqs[] = {
-	[FREQ_INDEX_640] = 80000000,
-	[FREQ_INDEX_1280] = 200000000,
-};
-
 static const struct hi846_reg_list hi846_init_regs_list_2lane = {
 	.num_of_regs = ARRAY_SIZE(hi846_init_2lane),
 	.regs = hi846_init_2lane,
@@ -1061,7 +1055,13 @@ static const struct hi846_mode supported_modes[] = {
 	{
 		.width = 640,
 		.height = 480,
-		.link_freq_index = FREQ_INDEX_640,
+		.mipi_clk_div_2lane = 4,
+		/*
+		 * Dummy but necessary if we set this mode default, otherwise
+		 * hi846_calc_pixel_rate() will be broken in
+		 * hi846_init_controls()
+		 */
+		.mipi_clk_div_4lane = 8,
 		.fps = 120,
 		.frame_len = 631,
 		.llp = HI846_LINE_LENGTH,
@@ -1086,7 +1086,8 @@ static const struct hi846_mode supported_modes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.link_freq_index = FREQ_INDEX_1280,
+		.mipi_clk_div_2lane = 2,
+		.mipi_clk_div_4lane = 4,
 		.fps = 90,
 		.frame_len = 842,
 		.llp = HI846_LINE_LENGTH,
@@ -1112,7 +1113,8 @@ static const struct hi846_mode supported_modes[] = {
 	{
 		.width = 1632,
 		.height = 1224,
-		.link_freq_index = FREQ_INDEX_1280,
+		.mipi_clk_div_2lane = 2,
+		.mipi_clk_div_4lane = 4,
 		.fps = 30,
 		.frame_len = 2526,
 		.llp = HI846_LINE_LENGTH,
@@ -1161,7 +1163,6 @@ struct hi846 {
 	struct v4l2_ctrl_handler ctrl_handler;
 	u8 nr_lanes;
 
-	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *vblank;
 	struct v4l2_ctrl *hblank;
@@ -1192,21 +1193,28 @@ static const struct hi846_datafmt *hi846_find_datafmt(u32 code)
 	return NULL;
 }
 
-static inline u8 hi846_get_link_freq_index(struct hi846 *hi846)
+static u64
+hi846_get_link_freq(struct hi846 *hi846, const struct hi846_mode *mode)
 {
-	return hi846->cur_mode->link_freq_index;
-}
+	u64 mclk = clk_get_rate(hi846->clock);
+	u8 mipi_clk_div;
 
-static u64 hi846_get_link_freq(struct hi846 *hi846)
-{
-	u8 index = hi846_get_link_freq_index(hi846);
+	if (hi846->nr_lanes == 2)
+		mipi_clk_div = mode->mipi_clk_div_2lane;
+	else
+		mipi_clk_div = mode->mipi_clk_div_4lane;
 
-	return hi846_link_freqs[index];
+	/*
+	 * HI846_REG_PLL_CFG_MIPI1_H = 0x025a, it is fixed in listed modes
+	 * [11:8]: 0x02 => pre_div = 3
+	 * [7:0]: 0x5a => multiplier = 90
+	 */
+	return mul_u64_u32_div(mclk, 90, 3 * mipi_clk_div);
 }
 
 static u64 hi846_calc_pixel_rate(struct hi846 *hi846)
 {
-	u64 link_freq = hi846_get_link_freq(hi846);
+	u64 link_freq = hi846_get_link_freq(hi846, hi846->cur_mode);
 	u64 pixel_rate = link_freq * 2 * hi846->nr_lanes;
 
 	do_div(pixel_rate, HI846_RGB_DEPTH);
@@ -1426,14 +1434,6 @@ static int hi846_init_controls(struct hi846 *hi846)
 
 	ctrl_hdlr->lock = &hi846->mutex;
 
-	hi846->link_freq =
-		v4l2_ctrl_new_int_menu(ctrl_hdlr, &hi846_ctrl_ops,
-				       V4L2_CID_LINK_FREQ,
-				       ARRAY_SIZE(hi846_link_freqs) - 1,
-				       0, hi846_link_freqs);
-	if (hi846->link_freq)
-		hi846->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
-
 	hi846->pixel_rate =
 		v4l2_ctrl_new_std(ctrl_hdlr, &hi846_ctrl_ops,
 				  V4L2_CID_PIXEL_RATE, 0,
@@ -1503,10 +1503,9 @@ static int hi846_set_video_mode(struct hi846 *hi846, int fps)
 	u64 frame_length;
 	int ret = 0;
 	int dummy_lines;
-	u64 link_freq = hi846_get_link_freq(hi846);
+	u64 link_freq = hi846_get_link_freq(hi846, hi846->cur_mode);
 
-	dev_dbg(&client->dev, "%s: link freq: %llu\n", __func__,
-		hi846_get_link_freq(hi846));
+	dev_dbg(&client->dev, "%s: link freq: %llu\n", __func__, link_freq);
 
 	do_div(link_freq, fps);
 	frame_length = link_freq;
@@ -1749,7 +1748,6 @@ static int hi846_set_format(struct v4l2_subdev *sd,
 	mf->code = HI846_MEDIA_BUS_FORMAT;
 	mf->field = V4L2_FIELD_NONE;
 
-	__v4l2_ctrl_s_ctrl(hi846->link_freq, hi846_get_link_freq_index(hi846));
 	__v4l2_ctrl_s_ctrl_int64(hi846->pixel_rate,
 				 hi846_calc_pixel_rate(hi846));
 
@@ -1950,16 +1948,17 @@ static int hi846_identify_module(struct hi846 *hi846)
 static s64 hi846_check_link_freqs(struct hi846 *hi846,
 				  struct v4l2_fwnode_endpoint *ep)
 {
-	const s64 *freqs = hi846_link_freqs;
-	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
+	int freqs_count = ARRAY_SIZE(supported_modes);
+	u64 link_freq;
 	int i, j;
 
 	for (i = 0; i < freqs_count; i++) {
+		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
 		for (j = 0; j < ep->nr_of_link_frequencies; j++)
-			if (freqs[i] == ep->link_frequencies[j])
+			if (link_freq == ep->link_frequencies[j])
 				break;
 		if (j == ep->nr_of_link_frequencies)
-			return freqs[i];
+			return link_freq;
 	}
 
 	return 0;
@@ -2041,7 +2040,6 @@ static int hi846_probe(struct i2c_client *client)
 	struct hi846 *hi846;
 	int ret;
 	int i;
-	u32 mclk_freq;
 
 	hi846 = devm_kzalloc(&client->dev, sizeof(*hi846), GFP_KERNEL);
 	if (!hi846)
@@ -2060,12 +2058,6 @@ static int hi846_probe(struct i2c_client *client)
 				     "failed to get clock: %pe\n",
 				     hi846->clock);
 
-	mclk_freq = clk_get_rate(hi846->clock);
-	if (mclk_freq != 25000000)
-		dev_warn(&client->dev,
-			 "External clock freq should be 25000000, not %u.\n",
-			 mclk_freq);
-
 	for (i = 0; i < HI846_NUM_SUPPLIES; i++)
 		hi846->supplies[i].supply = hi846_supply_names[i];
 
-- 
2.54.0

[PATCH v5 3/5] media: hi846: Fix modes handling for different lane cases

[Pengyu Luo]

When using 4-lane, v4l2_find_nearest_size may return an unsupported
mode, 640x480 mode, use v4l2_find_nearest_size_conditional() to fix
it.

Reported-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
Closes: https://lore.kernel.org/linux-media/OmTXoHZJTSGePymL9I-1Cw@puri.sm
Fixes: e8c0882685f9 ("media: i2c: add driver for the SK Hynix Hi-846 8M pixel camera")
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
v5:
- Use v4l2_find_nearest_size_conditional() to filter for different lane cases. (Sakari)
---
 drivers/media/i2c/hi846.c | 58 +++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 6bcd862aee30..1223567641c4 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1688,6 +1688,16 @@ static int __maybe_unused hi846_resume(struct device *dev)
 	return hi846_power_on(hi846);
 }
 
+static bool filter_by_mipi_lanes(const void *array, size_t index,
+				 const void *context)
+{
+	const struct hi846_mode *mode = array;
+	const struct hi846 *hi846 = context;
+
+	return (hi846->nr_lanes == 2 && mode->reg_list_2lane.num_of_regs) ||
+	       (hi846->nr_lanes == 4 && mode->reg_list_4lane.num_of_regs);
+}
+
 static int hi846_set_format(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *sd_state,
 			    struct v4l2_subdev_format *format)
@@ -1710,20 +1720,6 @@ static int hi846_set_format(struct v4l2_subdev *sd,
 		return 0;
 	}
 
-	if (hi846->nr_lanes == 2) {
-		if (!hi846->cur_mode->reg_list_2lane.num_of_regs) {
-			dev_err(&client->dev,
-				"this mode is not supported for 2 lanes\n");
-			return -EINVAL;
-		}
-	} else {
-		if (!hi846->cur_mode->reg_list_4lane.num_of_regs) {
-			dev_err(&client->dev,
-				"this mode is not supported for 4 lanes\n");
-			return -EINVAL;
-		}
-	}
-
 	mutex_lock(&hi846->mutex);
 
 	if (hi846->streaming) {
@@ -1734,9 +1730,12 @@ static int hi846_set_format(struct v4l2_subdev *sd,
 	hi846->fmt = fmt;
 
 	hi846->cur_mode =
-		v4l2_find_nearest_size(supported_modes,
-				       ARRAY_SIZE(supported_modes),
-				       width, height, mf->width, mf->height);
+		v4l2_find_nearest_size_conditional(supported_modes,
+						   ARRAY_SIZE(supported_modes),
+						   width, height,
+						   mf->width, mf->height,
+						   filter_by_mipi_lanes, hi846);
+
 	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
 		hi846->cur_mode->width, hi846->cur_mode->height);
 
@@ -1819,6 +1818,8 @@ static int hi846_enum_frame_size(struct v4l2_subdev *sd,
 				 struct v4l2_subdev_frame_size_enum *fse)
 {
 	struct i2c_client *client = v4l2_get_subdevdata(sd);
+	struct hi846 *hi846 = to_hi846(sd);
+	int i, count;
 
 	if (fse->pad || fse->index >= ARRAY_SIZE(supported_modes))
 		return -EINVAL;
@@ -1828,15 +1829,24 @@ static int hi846_enum_frame_size(struct v4l2_subdev *sd,
 		return -EINVAL;
 	}
 
-	fse->min_width = supported_modes[fse->index].width;
-	fse->max_width = supported_modes[fse->index].width;
-	fse->min_height = supported_modes[fse->index].height;
-	fse->max_height = supported_modes[fse->index].height;
+	for (count = i = 0; i < ARRAY_SIZE(supported_modes); i++) {
+		if (!filter_by_mipi_lanes(&supported_modes[i], i, hi846))
+			continue;
+
+		if (count == fse->index) {
+			fse->min_width = supported_modes[i].width;
+			fse->max_width = fse->min_width;
+			fse->min_height = supported_modes[i].height;
+			fse->max_height = fse->min_height;
+			dev_dbg(&client->dev, "%s: max width: %d max height: %d\n", __func__,
+				fse->max_width, fse->max_height);
+			return 0;
+		}
 
-	dev_dbg(&client->dev, "%s: max width: %d max height: %d\n", __func__,
-		fse->max_width, fse->max_height);
+		count++;
+	}
 
-	return 0;
+	return -EINVAL;
 }
 
 static int hi846_get_selection(struct v4l2_subdev *sd,
-- 
2.54.0

[PATCH v5 4/5] media: hi846: Add 6MP and 8MP modes support

[Pengyu Luo]

Hi846 is an 8MP sensor, but the upstream driver has only supported 2MP
mode for years. This patch adds 6MP and 8MP modes to maximize sensor
utilization.

Note that these modes require 4-lane MIPI CSI-2, as the downstream
driver only exposes 2MP, 6MP, and 8MP configurations in 4-lane
operation on the target device. The register sequences are extracted
from the downstream Windows driver.

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
v5:
- Use separated fields instead of raw register values for PLL cfg (Sakari)
---
 drivers/media/i2c/hi846.c | 154 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 153 insertions(+), 1 deletion(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 1223567641c4..cb06fc4188a6 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1028,6 +1028,106 @@ static const struct hi846_reg mode_1632x1224_mipi_4lane[] = {
 	{HI846_REG_TG_ENABLE,			0x0100},
 };
 
+static const struct hi846_reg mode_3264x1836_config[] = {
+	{HI846_REG_MODE_SELECT,			0x0000},
+	{HI846_REG_Y_ODD_INC_FOBP,		0x1111},
+	{HI846_REG_Y_ODD_INC_VACT,		0x1111},
+	{HI846_REG_Y_ADDR_START_VACT_H,		0x0172},
+	{HI846_REG_Y_ADDR_END_VACT_H,		0x089d},
+	{HI846_REG_UNKNOWN_005C,		0x2101},
+	{HI846_REG_FLL,				0x09de},
+	{HI846_REG_LLP,				0x0ed8},
+	{HI846_REG_BINNING_MODE,		0x0022},
+	{HI846_REG_HBIN_MODE,			0x0000},
+	{HI846_REG_UNKNOWN_0A24,		0x0000},
+	{HI846_REG_X_START_H,			0x0000},
+	{HI846_REG_X_OUTPUT_SIZE_H,		0x0cc0},
+	{HI846_REG_Y_OUTPUT_SIZE_H,		0x072c},
+	{HI846_REG_EXPOSURE,			0x09d8},
+
+	/* For OTP */
+	{HI846_REG_UNKNOWN_021C,		0x0001},
+	{HI846_REG_UNKNOWN_021E,		0x0235},
+
+	{HI846_REG_ISP_EN_H,			0x014a},
+	{HI846_REG_UNKNOWN_0418,		0x023e},
+	{HI846_REG_UNKNOWN_0B02,		0xe04d},
+	{HI846_REG_UNKNOWN_0B10,		0x6821},
+	{HI846_REG_UNKNOWN_0B12,		0x0120},
+	{HI846_REG_UNKNOWN_0B14,		0x0001},
+	{HI846_REG_UNKNOWN_2008,		0x38fd},
+	{HI846_REG_UNKNOWN_326E,		0x0000},
+};
+
+static const struct hi846_reg mode_3264x1836_mipi_4lane[] = {
+	{HI846_REG_UNKNOWN_0900,		0x0300},
+	{HI846_REG_MIPI_TX_OP_MODE,		0xc319},
+	{HI846_REG_UNKNOWN_0914,		0xc109},
+	{HI846_REG_TCLK_PREPARE,		0x061a},
+	{HI846_REG_UNKNOWN_0918,		0x0407},
+	{HI846_REG_THS_ZERO,			0x0a0b},
+	{HI846_REG_TCLK_POST,			0x0e08},
+	{HI846_REG_UNKNOWN_091E,		0x0a00},
+	{HI846_REG_UNKNOWN_090C,		0x0427},
+	{HI846_REG_UNKNOWN_090E,		0x0059},
+	{HI846_REG_UNKNOWN_0954,		0x0089},
+	{HI846_REG_UNKNOWN_0956,		0x0000},
+	{HI846_REG_UNKNOWN_0958,		0xca80},
+	{HI846_REG_UNKNOWN_095A,		0x9240},
+	{HI846_REG_PLL_CFG_MIPI2_H,		0x4124},
+	{HI846_REG_TG_ENABLE,			0x0100},
+};
+
+static const struct hi846_reg mode_3264x2448_config[] = {
+	{HI846_REG_MODE_SELECT,			0x0000},
+	{HI846_REG_Y_ODD_INC_FOBP,		0x1111},
+	{HI846_REG_Y_ODD_INC_VACT,		0x1111},
+	{HI846_REG_Y_ADDR_START_VACT_H,		0x0040},
+	{HI846_REG_Y_ADDR_END_VACT_H,		0x09cf},
+	{HI846_REG_UNKNOWN_005C,		0x2101},
+	{HI846_REG_FLL,				0x09de},
+	{HI846_REG_LLP,				0x0ed8},
+	{HI846_REG_BINNING_MODE,		0x0022},
+	{HI846_REG_HBIN_MODE,			0x0000},
+	{HI846_REG_UNKNOWN_0A24,		0x0000},
+	{HI846_REG_X_START_H,			0x0000},
+	{HI846_REG_X_OUTPUT_SIZE_H,		0x0cc0},
+	{HI846_REG_Y_OUTPUT_SIZE_H,		0x0990},
+	{HI846_REG_EXPOSURE,			0x09d8},
+
+	/* For OTP */
+	{HI846_REG_UNKNOWN_021C,		0x0001},
+	{HI846_REG_UNKNOWN_021E,		0x0235},
+
+	{HI846_REG_ISP_EN_H,			0x014a},
+	{HI846_REG_UNKNOWN_0418,		0x0000},
+	{HI846_REG_UNKNOWN_0B02,		0xe04d},
+	{HI846_REG_UNKNOWN_0B10,		0x6821},
+	{HI846_REG_UNKNOWN_0B12,		0x0120},
+	{HI846_REG_UNKNOWN_0B14,		0x0001},
+	{HI846_REG_UNKNOWN_2008,		0x38fd},
+	{HI846_REG_UNKNOWN_326E,		0x0000},
+};
+
+static const struct hi846_reg mode_3264x2448_mipi_4lane[] = {
+	{HI846_REG_UNKNOWN_0900,		0x0300},
+	{HI846_REG_MIPI_TX_OP_MODE,		0xc319},
+	{HI846_REG_UNKNOWN_0914,		0xc109},
+	{HI846_REG_TCLK_PREPARE,		0x061a},
+	{HI846_REG_UNKNOWN_0918,		0x0407},
+	{HI846_REG_THS_ZERO,			0x0a0b},
+	{HI846_REG_TCLK_POST,			0x0e08},
+	{HI846_REG_UNKNOWN_091E,		0x0a00},
+	{HI846_REG_UNKNOWN_090C,		0x0427},
+	{HI846_REG_UNKNOWN_090E,		0x0059},
+	{HI846_REG_UNKNOWN_0954,		0x0089},
+	{HI846_REG_UNKNOWN_0956,		0x0000},
+	{HI846_REG_UNKNOWN_0958,		0xca80},
+	{HI846_REG_UNKNOWN_095A,		0x9240},
+	{HI846_REG_PLL_CFG_MIPI2_H,		0x4124},
+	{HI846_REG_TG_ENABLE,			0x0100},
+};
+
 static const char * const hi846_test_pattern_menu[] = {
 	"Disabled",
 	"Solid Colour",
@@ -1136,7 +1236,59 @@ static const struct hi846_mode supported_modes[] = {
 			.width = 1632 * 2,
 			.height = 1224 * 2,
 		},
-	}
+	},
+	{
+		.width = 3264,
+		.height = 1836,
+		.mipi_clk_div_2lane = 1, /* dummy */
+		.mipi_clk_div_4lane = 2,
+		.fps = 30,
+		.frame_len = 2526,
+		.llp = HI846_LINE_LENGTH,
+		.reg_list_config = {
+			.num_of_regs = ARRAY_SIZE(mode_3264x1836_config),
+			.regs = mode_3264x1836_config,
+		},
+		.reg_list_2lane = {
+			.num_of_regs = 0,
+		},
+		.reg_list_4lane = {
+			.num_of_regs = ARRAY_SIZE(mode_3264x1836_mipi_4lane),
+			.regs = mode_3264x1836_mipi_4lane,
+		},
+		.crop = {
+			.left = 0x46,
+			.top = 0x172,
+			.width = 3264,
+			.height = 1836,
+		},
+	},
+	{
+		.width = 3264,
+		.height = 2448,
+		.mipi_clk_div_2lane = 1, /* dummy */
+		.mipi_clk_div_4lane = 2,
+		.fps = 30,
+		.frame_len = 2526,
+		.llp = HI846_LINE_LENGTH,
+		.reg_list_config = {
+			.num_of_regs = ARRAY_SIZE(mode_3264x2448_config),
+			.regs = mode_3264x2448_config,
+		},
+		.reg_list_2lane = {
+			.num_of_regs = 0,
+		},
+		.reg_list_4lane = {
+			.num_of_regs = ARRAY_SIZE(mode_3264x2448_mipi_4lane),
+			.regs = mode_3264x2448_mipi_4lane,
+		},
+		.crop = {
+			.left = 0x46,
+			.top = 0x40,
+			.width = 3264,
+			.height = 2448,
+		},
+	},
 };
 
 struct hi846_datafmt {
-- 
2.54.0

[PATCH v5 5/5] arm64: dts: imx8mq-librem5: Correct link frequency list

[Pengyu Luo]

According to the hi846 datasheet and the register lists, they are

mclk / prediv * multiplier / mipi_clk_div1 / mipi_clk_div2 =
25Mhz / 3 * 90 / 2 / {1,2} = 187.5Mhz, 375Mhz

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
v5:
- Rename post_div to mipi_clk_div
- Correct div1 number from 1 to 2
---
 arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
index f5d529c5baf3..cc5604172192 100644
--- a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
+++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
@@ -1122,7 +1122,7 @@ port {
 			camera1_ep: endpoint {
 				data-lanes = <1 2>;
 				link-frequencies = /bits/ 64
-					<80000000 200000000 300000000>;
+					<187500000 375000000>;
 				remote-endpoint = <&mipi1_sensor_ep>;
 			};
 		};
-- 
2.54.0

[PATCH v5 1/5] media: hi846: Fix hi846_write_reg_16 handling

[Pengyu Luo]

hi846_write_reg_16() does not clear a positive *err value on success.
pm_runtime_get_if_in_use() returns a positive value when the device
is already in use. When hi846_set_ctrl() passes &ret holding this
positive value) to hi846_write_reg_16(), the function returns with ret
as is, the positive value propagates back as a return code, which
callers interpret as an error.

Fix this by resetting *err to 0 only when it is positive.

Fixes: 04fc06f6dc15 ("media: hi846: fix usage of pm_runtime_get_if_in_use()")
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 drivers/media/i2c/hi846.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index a3f77b8434ca..7f069aca0fce 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1271,6 +1271,8 @@ static void hi846_write_reg_16(struct hi846 *hi846, u16 reg, u16 val, int *err)
 	if (*err < 0)
 		return;
 
+	*err = 0;
+
 	put_unaligned_be16(reg, buf);
 	put_unaligned_be16(val, buf + 2);
 	ret = i2c_master_send(client, buf, sizeof(buf));
-- 
2.54.0

Re: [PATCH v5 5/5] arm64: dts: imx8mq-librem5: Correct link frequency list

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The link frequency validation loop incorrectly tests dummy modes that are unsupported by the active MIPI lane configuration, demanding dummy frequencies in the device tree.
- [Medium] The driver validates link frequencies before obtaining its clock, causing the validation check to evaluate to 0 and silently bypass error handling.
--

--- Patch [5]: [PATCH v5 5/5] arm64: dts: imx8mq-librem5: Correct link frequency list ---
commit 7374317f64e34ad7703aeaa4a94012403d0aca4d
Author: Pengyu Luo <mitltlatltl@gmail.com>

arm64: dts: imx8mq-librem5: Correct link frequency list

This patch updates the link frequencies for the hi846 camera sensor on the
imx8mq-librem5 board to 187.5MHz and 375MHz based on the datasheet.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> index f5d529c5baf3e..cc5604172192c 100644
> --- a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> +++ b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> @@ -1122,7 +1122,7 @@ port {
>  			camera1_ep: endpoint {
>  				data-lanes = <1 2>;
>  				link-frequencies = /bits/ 64
> -					<80000000 200000000 300000000>;
> +					<187500000 375000000>;

[Severity: Medium]
Does the driver actually validate these new device tree link frequencies?

Looking at hi846_probe() in drivers/media/i2c/hi846.c, it calls
hi846_parse_dt() before obtaining hi846->clock. During parsing,
hi846_check_link_freqs() calls clk_get_rate(hi846->clock) on a NULL pointer,
returning 0.

When 0 is not found in the DT, it is returned as an unsupported frequency:

drivers/media/i2c/hi846.c:hi846_parse_dt() {
    ...
    if (fq) {
        dev_err(dev, "Link frequency of %lld is not supported\n", fq);
        ret = -EINVAL;
        goto check_hwcfg_error;
    }
    ...
}

Since fq evaluates to 0, the error handler is skipped and the driver silently
bypasses link frequency validation.

[Severity: High]
If the clock ordering bug in the driver is fixed, will probe fatally fail
for this 2-lane configuration?

The librem5 has a 2-lane setup, but hi846_check_link_freqs() in the driver
iterates over all supported_modes without filtering out modes that are
exclusive to 4-lanes:

drivers/media/i2c/hi846.c:hi846_check_link_freqs() {
    ...
    for (i = 0; i < freqs_count; i++) {
        link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
    ...
}

For 4-lane modes, the 2-lane divider is a dummy value. This causes
hi846_get_link_freq() to calculate an invalid 750MHz frequency:

drivers/media/i2c/hi846.c:hi846_get_link_freq() {
    ...
    return mul_u64_u32_div(mclk, 90, 3 * mipi_clk_div);
    ...
}

Because 750MHz is rightfully absent from these updated link-frequencies,
probe will abort once the NULL clock issue is resolved. Could the driver
be updated to filter modes by active lane count?

>  				remote-endpoint = <&mipi1_sensor_ep>;
>  			};
>  		};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712115012.91600-1-mitltlatltl@gmail.com?part=5

Re: [PATCH v5 3/5] media: hi846: Fix modes handling for different lane cases

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] `hi846_check_link_freqs()` iterates over all modes without filtering them by the active MIPI lanes, demanding bogus link frequencies for unsupported configurations.

Pre-existing issues:
- [High] The sensor's default active mode is unconditionally initialized to an unsupported mode for 4-lane MIPI configurations during probe.
--

--- Patch [3]: [PATCH v5 3/5] media: hi846: Fix modes handling for different lane cases ---
commit 110c167f519b3b39914f2d9ef7ed2f39cf8cf0d1
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Fix modes handling for different lane cases

This patch fixes an issue where v4l2_find_nearest_size() could return
an unsupported 640x480 mode on 4-lane configurations. It introduces
filter_by_mipi_lanes() to filter modes based on MIPI lanes when finding
the nearest size and enumerating frame sizes.

Link: https://lore.kernel.org/linux-media/OmTXoHZJTSGePymL9I-1Cw@puri.sm

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 6bcd862aee301..df7ea3ecf0d82 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1688,6 +1688,16 @@ static int __maybe_unused hi846_resume(struct device *dev)
>  	return hi846_power_on(hi846);
>  }
>  
> +static bool filter_by_mipi_lanes(const void *array, size_t index,
> +				 const void *context)
> +{
> +	const struct hi846_mode *mode = array;
> +	const struct hi846 *hi846 = context;
> +
> +	return (hi846->nr_lanes == 2 && mode->reg_list_2lane.num_of_regs) ||
> +	       (hi846->nr_lanes == 4 && mode->reg_list_4lane.num_of_regs);
> +}
> +

[Severity: High]
Since we are filtering unsupported modes by lanes here, do we also need to
apply this filter in hi846_check_link_freqs()?

Currently, hi846_check_link_freqs() iterates over all modes without filtering
by active MIPI lanes:

drivers/media/i2c/hi846.c:hi846_check_link_freqs() {
    ...
	for (i = 0; i < freqs_count; i++) {
		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
		for (j = 0; j < ep->nr_of_link_frequencies; j++)
    ...
}

This demands link frequencies for unsupported configurations. For a 4-lane
setup, it will calculate a link frequency for the unsupported 640x480 mode.
If this frequency is correctly omitted from the device tree's
link-frequencies property, the probe will fail with -EINVAL, making the
driver unusable on 4-lane hardware.

[Severity: High]
This is a pre-existing issue, but the default mode initialized in
hi846_probe() might be unsupported on 4-lane configurations:

drivers/media/i2c/hi846.c:hi846_probe() {
    ...
	hi846->cur_mode = &supported_modes[0];
    ...
}

The first mode in supported_modes is the 640x480 mode, which has
reg_list_4lane.num_of_regs = 0. When userspace starts streaming without
explicitly changing the format, hi846_start_streaming() will execute zero
MIPI configuration register writes for 4-lane setups:

drivers/media/i2c/hi846.c:hi846_start_streaming() {
    ...
	else
		ret = hi846_write_reg_list(hi846,
					   &hi846->cur_mode->reg_list_4lane);
    ...
}

This leaves the sensor in a non-functional state. Should the probe function
initialize cur_mode to a supported mode using the new filtering logic?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712115012.91600-1-mitltlatltl@gmail.com?part=3

Re: [PATCH v5 1/5] media: hi846: Fix hi846_write_reg_16 handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Improper error handling of `pm_runtime_get_if_in_use()` causes skipped I2C writes when Runtime PM is disabled (e.g., CONFIG_PM=n).
--

--- Patch [1]: [PATCH v5 1/5] media: hi846: Fix hi846_write_reg_16 handling ---
commit a93dfb63bfce92d91d72fc1b7b2f261ebe6609ab
Author: Pengyu Luo <mitltlatltl@gmail.com>

    media: hi846: Fix hi846_write_reg_16 handling
    
    hi846_write_reg_16() does not clear a positive *err value on success.
    pm_runtime_get_if_in_use() returns a positive value when the device
    is already in use. When hi846_set_ctrl() passes &ret holding this
    positive value) to hi846_write_reg_16(), the function returns with ret
    as is, the positive value propagates back as a return code, which
    callers interpret as an error.
    
    Fix this by resetting *err to 0 only when it is positive.
    
    Fixes: 04fc06f6dc15 ("media: hi846: fix usage of pm_runtime_get_if_in_use()")

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index a3f77b8434ca7..7f069aca0fce5 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1271,6 +1271,8 @@ static void hi846_write_reg_16(struct hi846 *hi846, u16 reg, u16 val, int *err)
>  	if (*err < 0)
>  		return;
>  
> +	*err = 0;
> +

[Severity: High]
This is a pre-existing issue, but does hi846_set_ctrl() properly handle
negative errors like -EINVAL from pm_runtime_get_if_in_use() when CONFIG_PM
is disabled?

When CONFIG_PM is disabled, pm_runtime_get_if_in_use() returns -EINVAL.
Because hi846_set_ctrl() checks if (!ret || ret == -EAGAIN), it will fall
through and pass the -EINVAL via &ret to hi846_write_reg_16():

drivers/media/i2c/hi846.c:hi846_set_ctrl() {
    ...
	ret = pm_runtime_get_if_in_use(&client->dev);
	if (!ret || ret == -EAGAIN)
		return 0;

	switch (ctrl->id) {
    ...
	case V4L2_CID_VBLANK:
		/* Update FLL that meets expected vertical blanking */
		hi846_write_reg_16(hi846, HI846_REG_FLL,
				   hi846->cur_mode->height + ctrl->val, &ret);
		break;
    ...
}

Because hi846_write_reg_16() exits early on negative errors, won't this
skip all I2C register writes when CONFIG_PM is disabled?

>  	put_unaligned_be16(reg, buf);
>  	put_unaligned_be16(val, buf + 2);
>  	ret = i2c_master_send(client, buf, sizeof(buf));

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712115012.91600-1-mitltlatltl@gmail.com?part=1

Re: [PATCH v5 2/5] media: hi846: Fix link frequency handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Critical] The device tree link frequency validation is silently bypassed because `hi846->clock` is used before it is initialized, causing the check to always succeed erroneously.
- [High] Removing the V4L2_CID_LINK_FREQ control without implementing the get_mbus_config pad operation breaks CSI-2 receiver drivers that rely on it.
- [High] Link frequency validation in probe fails for all configurations because it mandates frequencies for modes that do not support the active lane count.

Pre-existing issues:
- [High] Format changes silently fail to update the pixel_rate control value due to unmodified control limits.
- [High] hi846_set_format performs an unprotected read of hi846->cur_mode outside a lock and validates the old mode rather than the newly requested one.
- [High] hi846_set_ctrl incorrectly handles the -EINVAL return value from pm_runtime_get_if_in_use(), leading to an unconditional usage_count decrement.
- [High] hi846_set_stream fails to track internal streaming state, allowing unbalanced s_stream(0) calls to underflow the PM runtime usage count.
--

--- Patch [2]: [PATCH v5 2/5] media: hi846: Fix link frequency handling ---
commit 552600ce78beca6ba3cd666623936aecd2e1ea5d
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Fix link frequency handling

This patch updates the link frequency handling to dynamically calculate it
based on PLL configuration, lane count, and external clock rate. It removes
the hardcoded link frequencies and the V4L2_CID_LINK_FREQ control, and drops
the MCLK rate check.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce5..6bcd862aee301 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1426,14 +1434,6 @@ static int hi846_init_controls(struct hi846 *hi846)
>  
>  	ctrl_hdlr->lock = &hi846->mutex;
>  
> -	hi846->link_freq =
> -		v4l2_ctrl_new_int_menu(ctrl_hdlr, &hi846_ctrl_ops,
> -				       V4L2_CID_LINK_FREQ,
> -				       ARRAY_SIZE(hi846_link_freqs) - 1,
> -				       0, hi846_link_freqs);
> -	if (hi846->link_freq)
> -		hi846->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -

[Severity: High]
Does removing V4L2_CID_LINK_FREQ without implementing the get_mbus_config pad
operation break CSI-2 receiver drivers?

Several CSI-2 receivers rely on v4l2_get_link_freq(), which falls back to
V4L2_CID_PIXEL_RATE only if the caller provides valid mul and div arguments.

When downstream receivers pass 0 for these arguments, the framework relies
entirely on .get_mbus_config or V4L2_CID_LINK_FREQ. Without either, the
framework returns -ENOENT, preventing the camera pipeline from streaming.

[ ... ]
> @@ -1749,7 +1748,6 @@ static int hi846_set_format(struct v4l2_subdev *sd,
>  	mf->code = HI846_MEDIA_BUS_FORMAT;
>  	mf->field = V4L2_FIELD_NONE;
>  
> -	__v4l2_ctrl_s_ctrl(hi846->link_freq, hi846_get_link_freq_index(hi846));
>  	__v4l2_ctrl_s_ctrl_int64(hi846->pixel_rate,
>  				 hi846_calc_pixel_rate(hi846));

[Severity: High]
This isn't a bug introduced by this patch, but does this silently fail to
update the pixel_rate control value when changing formats?

The control is instantiated in hi846_init_controls() with its maximum bound
permanently locked to the lowest-resolution mode's pixel rate:

hi846_init_controls()
    hi846->pixel_rate = v4l2_ctrl_new_std(ctrl_hdlr, ...,
                                          hi846_calc_pixel_rate(hi846), 1,
                                          hi846_calc_pixel_rate(hi846));

When setting a new format with a higher pixel rate, the new rate is applied
here but it fails to call __v4l2_ctrl_modify_range() to raise the upper limit
first. The framework will clamp the new value to the old maximum limit,
breaking downstream bandwidth calculations.

[Severity: High]
This is a pre-existing issue, but does hi846_set_format() perform validation
against the old mode rather than the newly requested one?

Earlier in hi846_set_format(), the code checks lane capability using an
unprotected read of hi846->cur_mode before the new mode is resolved:

    if (hi846->nr_lanes == 2) {
        if (!hi846->cur_mode->reg_list_2lane.num_of_regs) {

This allows users to configure a mode that is incompatible with the active
hardware lane count (because it checks the current mode, not the requested
one), causing a silent failure when the stream starts and MIPI registers
are omitted.

[ ... ]
> @@ -1950,16 +1948,17 @@ static int hi846_identify_module(struct hi846 *hi846)
>  static s64 hi846_check_link_freqs(struct hi846 *hi846,
>  				  struct v4l2_fwnode_endpoint *ep)
>  {
> -	const s64 *freqs = hi846_link_freqs;
> -	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
> +	int freqs_count = ARRAY_SIZE(supported_modes);
> +	u64 link_freq;
>  	int i, j;
>  
>  	for (i = 0; i < freqs_count; i++) {
> +		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);

[Severity: Critical]
Will this call to hi846_get_link_freq() result in a zero frequency calculation
because the clock is not yet initialized?

In hi846_probe(), hi846_parse_dt() is called before the clock is initialized:

hi846_probe()
    hi846_parse_dt()
        hi846_check_link_freqs()
            hi846_get_link_freq()
                clk_get_rate(hi846->clock);

    hi846->clock = devm_v4l2_sensor_clk_get(&client->dev, NULL);

Since hi846->clock is NULL during the DT parsing, clk_get_rate() will return 0.
The validation loop then checks if 0 is in the device tree endpoints, fails to
find it, and returns 0. The caller hi846_parse_dt() checks "if (fq)", which
evaluates to false when fq is 0, completely bypassing the error path and
treating the validation as successful.

[Severity: High]
Does iterating over all supported_modes cause probe to unconditionally fail?

For modes that do not support the active lane count, the calculation uses
dummy divider values (like mipi_clk_div_2lane = 1 in the 3264x1836 mode).

This incorrectly calculates a bogus link frequency and demands that it exists
in the device tree link-frequencies array. When it is naturally not found,
the validation rejects the device tree and aborts the probe.

[ ... ]

There are also a couple of pre-existing issues in other parts of the driver
that were noticed during this review:

[Severity: High]
This is a pre-existing issue, but does hi846_set_ctrl() incorrectly handle the
-EINVAL return value from pm_runtime_get_if_in_use()?

In hi846_set_ctrl():
    ret = pm_runtime_get_if_in_use(&client->dev);
    if (!ret || ret == -EAGAIN)
        return 0;
    ...
    pm_runtime_put(&client->dev);

If runtime PM is disabled, pm_runtime_get_if_in_use() returns -EINVAL.
The condition fails to catch -EINVAL, falls through, and unconditionally
executes pm_runtime_put(). Since the PM refcount was not incremented,
this underflows the usage count.

[Severity: High]
This is a pre-existing issue, but does hi846_set_stream() fail to track
internal streaming state?

In hi846_set_stream():
    if (!enable || ret) {
        hi846_stop_streaming(hi846);
        pm_runtime_put(&client->dev);
    }

Subdev s_stream(0) can be called multiple times. The function does not verify
if the sensor is actually currently streaming. When called with enable = 0,
it unconditionally enters the teardown block and executes pm_runtime_put().
If the stream was already stopped, the refcount was already dropped, leading
to an underflow.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712115012.91600-1-mitltlatltl@gmail.com?part=2

Re: [PATCH v5 4/5] media: hi846: Add 6MP and 8MP modes support

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] Unconditional link frequency validation fails probe on 2-lane hardware for modes with dummy frequencies.
- [High] Dynamic link frequency calculation breaks backward compatibility with existing device trees.
- [Medium] The active crop rectangles for the new 6MP and 8MP modes exceed the sensor's native crop bounds.
- [Low] The patch author failed to address reviewer feedback requesting the use of separated fields for PLL configuration.

Pre-existing issues:
- [High] The default active mode lacks a 4-lane register configuration, leading to missing MIPI setup and streaming failures on 4-lane hardware.
--

--- Patch [4]: [PATCH v5 4/5] media: hi846: Add 6MP and 8MP modes support ---
commit 3cb5bb77cc3eceb92db3158280dae51e0e69e655
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Add 6MP and 8MP modes support
    
Hi846 is an 8MP sensor, but the upstream driver has only supported 2MP
mode for years. This patch adds 6MP and 8MP modes to maximize sensor
utilization.

Note that these modes require 4-lane MIPI CSI-2, as the downstream
driver only exposes 2MP, 6MP, and 8MP configurations in 4-lane
operation on the target device. The register sequences are extracted
from the downstream Windows driver.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index df7ea3ecf0d82..0fc27545eec8b 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
> @@ -1028,6 +1028,106 @@ static const struct hi846_reg mode_1632x1224_mipi_4lane[] = {
[ ... ]
> +static const struct hi846_reg mode_3264x1836_mipi_4lane[] = {
[ ... ]
> +	{HI846_REG_UNKNOWN_095A,		0x9240},
> +	{HI846_REG_PLL_CFG_MIPI2_H,		0x4124},

[Severity: Low]
Does this still use raw register values instead of separated fields?

The v5 changelog states: "Use separated fields instead of raw register values
for PLL cfg (Sakari)". 

However, this newly added array defines the PLL configuration using a raw 16-bit
payload (0x4124) rather than using field preparation macros (FIELD_PREP) or
separated fields.

Was this reviewer feedback missed?

> +	{HI846_REG_TG_ENABLE,			0x0100},
> +};
> +
[ ... ]
> +static const struct hi846_reg mode_3264x2448_mipi_4lane[] = {
[ ... ]
> +	{HI846_REG_UNKNOWN_095A,		0x9240},
> +	{HI846_REG_PLL_CFG_MIPI2_H,		0x4124},

[Severity: Low]
Does this still use raw register values instead of separated fields?

As mentioned above, the v5 changelog indicates this should use separated fields
rather than a raw 16-bit payload.

> +	{HI846_REG_TG_ENABLE,			0x0100},
> +};
> +
[ ... ]
> @@ -1136,7 +1236,59 @@ static const struct hi846_mode supported_modes[] = {
>  			.width = 1632 * 2,
>  			.height = 1224 * 2,
>  		},
> -	}

[Severity: High]
This isn't a bug introduced by this patch, but does the default active mode
lack a 4-lane register configuration, leading to missing MIPI setup?

During probe, the driver sets the default format in hi846->cur_mode to
&supported_modes[0] (640x480). This mode has an empty 4-lane MIPI configuration.

If a userspace application running on 4-lane hardware starts streaming without
explicitly setting a supported format via VIDIOC_S_FMT, hi846_start_streaming()
will attempt to apply the empty 4-lane register list. 

Will this silently leave the sensor's MIPI interface unconfigured, resulting in
capture hangs or deadlocks?

> +	},
> +	{
> +		.width = 3264,
> +		.height = 1836,
> +		.mipi_clk_div_2lane = 1, /* dummy */

[Severity: High]
Does this dummy frequency break probe on 2-lane hardware?

During probe, hi846_check_link_freqs() iterates over all elements in
supported_modes, calculating their link frequencies and verifying they exist
in the device tree endpoint.

Since this dummy frequency is not present in the 2-lane device tree's
link-frequencies property, will the check unconditionally fail and abort driver
probe, breaking 2-lane hardware configurations?

> +		.mipi_clk_div_4lane = 2,

[Severity: High]
Does this dynamic link frequency calculation break backward compatibility with
existing device trees?

Prior to this series, the driver hardcoded acceptable link frequencies (80MHz
and 200MHz), and existing device trees list these specific values in their
link-frequencies property. 

Since patch 2 ("media: hi846: Fix link frequency handling") replaced these
hardcoded values with a dynamic calculation based on mclk and lane count,
calculating it here yields 187.5MHz and 375MHz when driven by the previously
standard 25MHz external clock. 

Because hi846_check_link_freqs() requires the exact calculated frequencies to
exist in the device tree, will probe fail on existing platforms using the legacy
mandated values, violating Device Tree backward compatibility rules?

> +		.fps = 30,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x172,
> +			.width = 3264,
> +			.height = 1836,
> +		},

[Severity: Medium]
Do these active crop rectangles exceed the sensor's native crop bounds?

The V4L2 selection API strictly requires that any active crop rectangle must be
fully contained within the native bounds (V4L2_SEL_TGT_CROP_BOUNDS), which are
hardcoded to 3264x2448 in hi846_get_selection().

However, this crop rectangle is defined with a width of 3264 and a left offset
of 0x46 (70), positioning the rightmost edge at X = 3334, which exceeds the
3264 width limit.

Can this API violation cause v4l2-compliance test failures and confuse
userspace applications?

> +	},
> +	{
> +		.width = 3264,
> +		.height = 2448,
> +		.mipi_clk_div_2lane = 1, /* dummy */
> +		.mipi_clk_div_4lane = 2,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x40,
> +			.width = 3264,
> +			.height = 2448,
> +		},

[Severity: Medium]
Similarly, does this crop rectangle exceed the sensor's native bounds?

With a top offset of 0x40 and height of 2448, the bottom edge is at Y = 2512,
which exceeds the 2448 native height limit, in addition to exceeding the width
limit as seen in the previous mode.

> +	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260712115012.91600-1-mitltlatltl@gmail.com?part=4