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

[PATCH v6 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 v6:
- Add link freq ctrl back (Sakari)
- Use v4l2_link_freq_to_bitmap() to get matched link freqs (Sakari)
- Move clk_get() before than hi846_parse_dt(), since we use clock in hi846_parse_dt()
- Add link freq check in filter for v4l2_find_nearest_size_conditional().
- Link to v5: https://lore.kernel.org/linux-media/20260712115012.91600-1-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                     | 370 ++++++++++++++----
 2 files changed, 289 insertions(+), 83 deletions(-)

-- 
2.55.0

[PATCH v6 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.55.0

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

[Pengyu Luo]

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

1. Drop fixed link freqs, we calculate the driver supported values and
use v4l2_link_freq_to_bitmap() to get the intersection with the DT
supported values.

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>
---
v6:
- Add link freq ctrl back (Sakari)
- Use v4l2_link_freq_to_bitmap() to get matched link freqs (Sakari)
- Move clk_get() before than hi846_parse_dt(), since we use clock in hi846_parse_dt()
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)
---
 drivers/media/i2c/hi846.c | 151 ++++++++++++++++++++++++--------------
 1 file changed, 94 insertions(+), 57 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 7f069aca0fce..2f8624f9bdf3 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,
@@ -1167,6 +1169,9 @@ struct hi846 {
 	struct v4l2_ctrl *hblank;
 	struct v4l2_ctrl *exposure;
 
+	s64 link_freqs[ARRAY_SIZE(supported_modes)];
+	int num_link_freqs;
+
 	struct mutex mutex; /* protect cur_mode, streaming and chip access */
 	const struct hi846_mode *cur_mode;
 	bool streaming;
@@ -1192,21 +1197,41 @@ 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(const 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;
+
+	if (hi846->nr_lanes == 2)
+		mipi_clk_div = mode->mipi_clk_div_2lane;
+	else
+		mipi_clk_div = mode->mipi_clk_div_4lane;
+
+	/*
+	 * 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_get_link_freq(struct hi846 *hi846)
+static int hi846_get_link_freq_index(const struct hi846 *hi846,
+				     const struct hi846_mode *mode)
 {
-	u8 index = hi846_get_link_freq_index(hi846);
+	u64 link_freq = hi846_get_link_freq(hi846, mode);
+	int i;
+
+	for (i = 0; i < hi846->num_link_freqs; i++)
+		if (hi846->link_freqs[i] == link_freq)
+			return i;
 
-	return hi846_link_freqs[index];
+	return -EINVAL;
 }
 
 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);
@@ -1429,8 +1454,8 @@ static int hi846_init_controls(struct hi846 *hi846)
 	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);
+				       hi846->num_link_freqs - 1,
+				       0, hi846->link_freqs);
 	if (hi846->link_freq)
 		hi846->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
 
@@ -1503,10 +1528,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;
@@ -1699,6 +1723,7 @@ static int hi846_set_format(struct v4l2_subdev *sd,
 	const struct hi846_datafmt *fmt = hi846_find_datafmt(mf->code);
 	u32 tgt_fps;
 	s32 vblank_def, h_blank;
+	int idx;
 
 	if (!fmt) {
 		mf->code = hi846_colour_fmts[0].code;
@@ -1749,7 +1774,14 @@ 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));
+	idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
+	if (idx < 0) {
+		dev_err(&client->dev,
+			"failed to get link freq index: %d\n", idx);
+		return -EINVAL;
+	}
+
+	__v4l2_ctrl_s_ctrl(hi846->link_freq, idx);
 	__v4l2_ctrl_s_ctrl_int64(hi846->pixel_rate,
 				 hi846_calc_pixel_rate(hi846));
 
@@ -1947,20 +1979,33 @@ static int hi846_identify_module(struct hi846 *hi846)
 	return 0;
 }
 
-static s64 hi846_check_link_freqs(struct hi846 *hi846,
-				  struct v4l2_fwnode_endpoint *ep)
+static int hi846_add_link_freqs(struct hi846 *hi846, struct device *dev,
+				struct v4l2_fwnode_endpoint *ep)
 {
-	const s64 *freqs = hi846_link_freqs;
-	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
-	int i, j;
-
-	for (i = 0; i < freqs_count; i++) {
-		for (j = 0; j < ep->nr_of_link_frequencies; j++)
-			if (freqs[i] == ep->link_frequencies[j])
-				break;
-		if (j == ep->nr_of_link_frequencies)
-			return freqs[i];
-	}
+	s64 hi846_link_freqs[ARRAY_SIZE(supported_modes)];
+	unsigned long freq_bitmap;
+	int ret, i;
+
+	/*
+	 * Since the MCLK freq varies between platforms, calculating driver
+	 * supported link freqs here.
+	 */
+	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
+		hi846_link_freqs[i] = hi846_get_link_freq(hi846, &supported_modes[i]);
+
+	ret = v4l2_link_freq_to_bitmap(dev, ep->link_frequencies,
+				       ep->nr_of_link_frequencies,
+				       hi846_link_freqs,
+				       ARRAY_SIZE(hi846_link_freqs),
+				       &freq_bitmap);
+	if (ret || !freq_bitmap)
+		return ret;
+
+	for (i = 0; i < ARRAY_SIZE(hi846_link_freqs); i++)
+		if (BIT(i) & freq_bitmap) {
+			hi846->link_freqs[hi846->num_link_freqs++] = hi846_link_freqs[i];
+			dev_dbg(dev, "Add supported link frequency %lld\n", hi846_link_freqs[i]);
+		}
 
 	return 0;
 }
@@ -1973,7 +2018,6 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
 		.bus_type = V4L2_MBUS_CSI2_DPHY
 	};
 	int ret;
-	s64 fq;
 
 	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
 	if (!ep) {
@@ -2004,11 +2048,10 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
 		goto check_hwcfg_error;
 	}
 
-	/* Check that link frequences for all the modes are in device tree */
-	fq = hi846_check_link_freqs(hi846, &bus_cfg);
-	if (fq) {
-		dev_err(dev, "Link frequency of %lld is not supported\n", fq);
-		ret = -EINVAL;
+	/* Add link frequencies which are supported by both DT and the driver */
+	ret = hi846_add_link_freqs(hi846, dev, &bus_cfg);
+	if (ret) {
+		dev_err(dev, "failed to add link frequency %d\n", ret);
 		goto check_hwcfg_error;
 	}
 
@@ -2041,30 +2084,24 @@ 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)
 		return -ENOMEM;
 
-	ret = hi846_parse_dt(hi846, &client->dev);
-	if (ret) {
-		dev_err(&client->dev, "failed to check HW configuration: %d",
-			ret);
-		return ret;
-	}
-
+	/* Get the MCLK first, since we need it to calculate link freqs */
 	hi846->clock = devm_v4l2_sensor_clk_get(&client->dev, NULL);
 	if (IS_ERR(hi846->clock))
 		return dev_err_probe(&client->dev, PTR_ERR(hi846->clock),
 				     "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);
+	ret = hi846_parse_dt(hi846, &client->dev);
+	if (ret) {
+		dev_err(&client->dev, "failed to check HW configuration: %d",
+			ret);
+		return ret;
+	}
 
 	for (i = 0; i < HI846_NUM_SUPPLIES; i++)
 		hi846->supplies[i].supply = hi846_supply_names[i];
-- 
2.55.0

[PATCH v6 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 filter
out 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")
Link: https://lore.kernel.org/linux-media/OmTXoHZJTSGePymL9I-1Cw@puri.sm
Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
v6:
- Add link freq check in filter.
v5:
- Use v4l2_find_nearest_size_conditional() to filter for different lane cases. (Sakari)
---
 drivers/media/i2c/hi846.c | 63 ++++++++++++++++++++++++---------------
 1 file changed, 39 insertions(+), 24 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 2f8624f9bdf3..648192521344 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1713,6 +1713,21 @@ static int __maybe_unused hi846_resume(struct device *dev)
 	return hi846_power_on(hi846);
 }
 
+static bool filter_modes(const void *array, size_t index, const void *context)
+{
+	const struct hi846_mode *mode = array;
+	const struct hi846 *hi846 = context;
+	bool valid_link_freq;
+	int freq_idx;
+
+	freq_idx = hi846_get_link_freq_index(hi846, mode);
+	valid_link_freq = freq_idx < 0 ? false : true;
+
+	return ((hi846->nr_lanes == 2 && mode->reg_list_2lane.num_of_regs) ||
+		(hi846->nr_lanes == 4 && mode->reg_list_4lane.num_of_regs)) &&
+	       valid_link_freq;
+}
+
 static int hi846_set_format(struct v4l2_subdev *sd,
 			    struct v4l2_subdev_state *sd_state,
 			    struct v4l2_subdev_format *format)
@@ -1736,20 +1751,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) {
@@ -1760,9 +1761,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_modes, hi846);
+
 	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
 		hi846->cur_mode->width, hi846->cur_mode->height);
 
@@ -1853,6 +1857,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;
@@ -1862,15 +1868,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_modes(&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.55.0

[PATCH v6 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 648192521344..719598938638 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.55.0

[PATCH v6 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.55.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] DT ABI Breakage: Removing existing link frequencies from the DT breaks backwards compatibility with older kernel drivers.
--

commit e1f0f3ee9bb107c23572fda8db68e68a5ce30d59
Author: Pengyu Luo <mitltlatltl@gmail.com>

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

This commit corrects the link frequency list for the hi846 camera in the
imx8mq-librem5 device tree to match calculations from 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: Critical]
Does removing the existing link frequencies here break backwards compatibility
with older kernels?

When an older kernel boots with this updated device tree, the hi846 driver
invokes hi846_parse_dt() and hi846_check_link_freqs(), which iterates through
its array of expected hardcoded frequencies (80000000 and 200000000) and
ensures each one is present in the DT.

Because this removes those legacy frequencies entirely instead of appending to
them, it appears the loop will fail to find a match and return -EINVAL, causing
the camera initialization to fail on older kernels (DT ABI breakage).

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

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

Re: [PATCH v6 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] Unbalanced pm_runtime_put() underflow and skipped I2C hardware writes result from unhandled negative error codes (e.g., -EINVAL) returned by pm_runtime_get_if_in_use().
--

commit baf92f1dfb7e9ab68d0b8a703aedfddfef46030d
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. Fix this by resetting *err to 0 only when it is positive.

> 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;
> +
>  	put_unaligned_be16(reg, buf);
>  	put_unaligned_be16(val, buf + 2);
>  	ret = i2c_master_send(client, buf, sizeof(buf));

[Severity: High]
This is a pre-existing issue, but does the error handling in hi846_set_ctrl()
correctly handle negative error codes returned by pm_runtime_get_if_in_use()?

If pm_runtime_get_if_in_use() returns a negative error code (like -EINVAL),
it bypasses the early return condition:

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) {
    ...
}

This causes the negative error code to propagate into the switch statement.
If the V4L2_CID_EXPOSURE or V4L2_CID_VBLANK case is hit, the negative value
is passed to hi846_write_reg_16(), which will immediately abort and skip all
I2C register initialization because of the `if (*err < 0)` check.

Furthermore, execution then unconditionally falls through to the end of
hi846_set_ctrl():

    ...
	pm_runtime_put(&client->dev);

	return ret;
}

Since a negative return from pm_runtime_get_if_in_use() means the reference
counter was not incremented, could this unconditional pm_runtime_put()
underflow the power management reference counter?

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

Re: [PATCH v6 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:
- [High] v4l2_find_nearest_size_conditional() can return NULL if all modes are filtered out, leading to a NULL pointer dereference.
- [Medium] Missing mutex_unlock(&hi846->mutex) on the error path in hi846_set_format().
--

commit 76f8fe21d198e6d98886ca4cb671239b0ef7030d
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Fix modes handling for different lane cases

When using 4-lane, v4l2_find_nearest_size may return an unsupported
mode. This commit uses v4l2_find_nearest_size_conditional() to filter
it out.

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 2f8624f9bdf3a..26b4290148b61 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1736,20 +1751,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) {
> @@ -1760,9 +1761,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_modes, hi846);
> +
>  	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
>  		hi846->cur_mode->width, hi846->cur_mode->height);

[Severity: High]
Does v4l2_find_nearest_size_conditional() have the potential to return NULL
here?

If the device tree's link-frequencies property does not match any mode for the
configured number of lanes, filter_modes() could filter out all modes. If this
happens, v4l2_find_nearest_size_conditional() would return NULL, which would
then be dereferenced in the dev_dbg() call immediately after.

Could we add a NULL check for hi846->cur_mode before dereferencing it?

[Severity: Medium]
In the error path slightly below this diff, if hi846_get_link_freq_index()
returns a negative value, it appears we return without unlocking the mutex:

	idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
	if (idx < 0) {
		dev_err(&client->dev,
			"failed to get link freq index: %d\n", idx);
		return -EINVAL;
	}

Since hi846->mutex is acquired earlier in the function, does this return path
need a mutex_unlock(&hi846->mutex)?

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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Incomplete filtering of unsupported link frequencies during probe causes state corruption, leading to a kernel crash (NULL pointer dereference) in the final series.
--

commit bffa4cf45bf88ea9bb0f32956b9b2cda43e64344
Author: Pengyu Luo <mitltlatltl@gmail.com>

media: hi846: Fix link frequency handling

This patch modifies link frequency handling to calculate supported values
dynamically based on PLL configuration, lane count, and MCLK rate. It replaces
hardcoded frequencies and uses v4l2_link_freq_to_bitmap() to find the
intersection with device tree values.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce5..2f8624f9bdf3a 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1749,7 +1774,14 @@ 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));
> +	idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
> +	if (idx < 0) {
> +		dev_err(&client->dev,
> +			"failed to get link freq index: %d\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	__v4l2_ctrl_s_ctrl(hi846->link_freq, idx);

[Severity: High]
If an unsupported mode was selected, could this regression violate the V4L2
API by returning -EINVAL here and leaving hi846->cur_mode corrupted?

In patch 3 of this series, when filter_modes() correctly rejects the
lane-incompatible Mode 0, does this result in 0 valid modes?

If 0 valid modes remain, would v4l2_find_nearest_size_conditional() return
NULL, causing hi846_set_format() to dereference a NULL hi846->cur_mode and
crash the kernel?

Also, in probe, does the default mode initialization blindly assign
hi846->cur_mode = &supported_modes[0], leaving the driver in an inconsistent
state if Mode 0 is unsupported?

[ ... ]
> @@ -1947,20 +1979,33 @@ static int hi846_identify_module(struct hi846 *hi846)
>  	return 0;
>  }
>  
> -static s64 hi846_check_link_freqs(struct hi846 *hi846,
> -				  struct v4l2_fwnode_endpoint *ep)
> +static int hi846_add_link_freqs(struct hi846 *hi846, struct device *dev,
> +				struct v4l2_fwnode_endpoint *ep)
>  {
> -	const s64 *freqs = hi846_link_freqs;
> -	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
> -	int i, j;
> -
> -	for (i = 0; i < freqs_count; i++) {
> -		for (j = 0; j < ep->nr_of_link_frequencies; j++)
> -			if (freqs[i] == ep->link_frequencies[j])
> -				break;
> -		if (j == ep->nr_of_link_frequencies)
> -			return freqs[i];
> -	}
> +	s64 hi846_link_freqs[ARRAY_SIZE(supported_modes)];
> +	unsigned long freq_bitmap;
> +	int ret, i;
> +
> +	/*
> +	 * Since the MCLK freq varies between platforms, calculating driver
> +	 * supported link freqs here.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> +		hi846_link_freqs[i] = hi846_get_link_freq(hi846, &supported_modes[i]);
> +
> +	ret = v4l2_link_freq_to_bitmap(dev, ep->link_frequencies,
> +				       ep->nr_of_link_frequencies,
> +				       hi846_link_freqs,
> +				       ARRAY_SIZE(hi846_link_freqs),
> +				       &freq_bitmap);

[Severity: High]
Does this filtering loop contain a regression by failing to check if the
corresponding modes actually support the configured lane count?

If hi846_add_link_freqs() evaluates hi846_get_link_freq() for all modes
regardless of whether the mode supports the physical lane count
(hi846->nr_lanes), could this allow incompatible configurations to pass probe?

For instance, supported_modes[0] (640x480) lacks 4-lane registers, but this
patch adds a dummy mipi_clk_div_4lane = 8. If hi846->nr_lanes == 4 and the
device tree specifies only this dummy link frequency, would
v4l2_link_freq_to_bitmap() incorrectly match it, causing probe to succeed
instead of failing?

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

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

[sashiko-bot]

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

New issues:
- [Medium] The crop rectangles for the newly added 6MP (3264x1836) and 8MP (3264x2448) modes overflow the native crop bounds of the sensor.

Pre-existing issues:
- [High] Missing mutex_unlock() on the error path in hi846_set_format(), leading to a deadlock.
- [High] Unchecked return value from v4l2_find_nearest_size_conditional() leads to a NULL pointer dereference in hi846_set_format().
--

commit e8c04d7885133a51b470ee7348db670b6ed848dd
Author: Pengyu Luo <mitltlatltl@gmail.com>
Date:   Sun Aug 30 16:00:24 2026 +0000

    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.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 26b4290148b61..7ad691364cdc1 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c
[ ... ]
> @@ -1136,7 +1236,59 @@ static const struct hi846_mode supported_modes[] = {
>  			.width = 1632 * 2,
>  			.height = 1224 * 2,
>  		},
> -	}
> +	},
> +	{
> +		.width = 3264,
> +		.height = 1836,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x172,
> +			.width = 3264,
> +			.height = 1836,
> +		},

[Severity: Medium]
Will this crop configuration violate the V4L2 API contract?

The crop rectangle extends beyond the native crop bounds of the sensor.
The hardcoded absolute crop bounds in hi846_get_selection() are:

	case V4L2_SEL_TGT_CROP_BOUNDS:
	case V4L2_SEL_TGT_NATIVE_SIZE:
		sel->r.top = 0;
		sel->r.left = 0;
		sel->r.width = 3264;
		sel->r.height = 2448;

For this new 3264x1836 mode, an offset of (70, 370) with a width of 3264 means
the right boundary extends to 3334, which strictly exceeds the advertised
3264 maximum limit. Strict userspace frameworks like libcamera or
v4l2-compliance might reject or clip this, breaking functionality.

> +	},
> +	{
> +		.width = 3264,
> +		.height = 2448,
[ ... ]
> +		.crop = {
> +			.left = 0x46,
> +			.top = 0x40,
> +			.width = 3264,
> +			.height = 2448,
> +		},

[Severity: Medium]
Similarly, does this crop configuration overflow the native width and height
bounds?

With an offset of (70, 64) and size 3264x2448, the boundaries are pushed
to 3334x2512, exceeding the 3264x2448 limits from hi846_get_selection().

Should the native bounds be expanded to accommodate these crop offsets, or
should the offsets be adjusted to 0?

While reviewing this patch, I also noticed a couple of pre-existing issues in
the file introduced earlier in this series:

> static int hi846_set_format(struct v4l2_subdev *sd,
> 			    struct v4l2_subdev_state *sd_state,
> 			    struct v4l2_subdev_format *fmt)
> {
[ ... ]
> 	hi846->cur_mode =
> 		v4l2_find_nearest_size_conditional(supported_modes,
> 						   ARRAY_SIZE(supported_modes),
> 						   width, height,
> 						   mf->width, mf->height,
> 						   filter_modes, hi846);
> 
> 	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
> 		hi846->cur_mode->width, hi846->cur_mode->height);

[Severity: High]
This isn't a bug introduced by this patch, but does
v4l2_find_nearest_size_conditional() return NULL if no matching mode is found?

If the device tree configuration provides data-lanes = <4> but the
link-frequencies property only matches modes that have 0 registers for 4-lane
configurations, filter_modes() will evaluate to false for every mode. 

This would cause v4l2_find_nearest_size_conditional() to return NULL, leading
to a kernel crash when hi846->cur_mode->width is unconditionally dereferenced.

[ ... ]
> 	idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
> 	if (idx < 0) {
> 		dev_err(&client->dev,
> 			"failed to get link freq index: %d\n", idx);
> 		return -EINVAL;
> 	}

[Severity: High]
This is a pre-existing issue, but does this error path leave hi846->mutex
locked?

The function returns without calling mutex_unlock(&hi846->mutex), causing a
deadlock on subsequent calls to driver functions that acquire this mutex.

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

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

[Sakari Ailus]

Hi Pengyu,

Thanks for the update.

On Mon, Aug 31, 2026 at 12:00:22AM +0800, Pengyu Luo wrote:
> Link frequency is tied to PLL configuration, lane count, and external
> and configurable clock, so use runtime here instead of hardcoding for
> specific configuration. To implement this, we do
> 
> 1. Drop fixed link freqs, we calculate the driver supported values and
> use v4l2_link_freq_to_bitmap() to get the intersection with the DT
> supported values.
> 
> 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>
> ---
> v6:
> - Add link freq ctrl back (Sakari)
> - Use v4l2_link_freq_to_bitmap() to get matched link freqs (Sakari)
> - Move clk_get() before than hi846_parse_dt(), since we use clock in hi846_parse_dt()
> 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)
> ---
>  drivers/media/i2c/hi846.c | 151 ++++++++++++++++++++++++--------------
>  1 file changed, 94 insertions(+), 57 deletions(-)
> 
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce..2f8624f9bdf3 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,

The divider of the 4-lane case appears to be always two times that of the
2-lane case. Could you calculate the value instead?

I think it'd be better to keep the link frequencies and modes at separate
indices; this is the way it used to be, too.

>  		.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,
> @@ -1167,6 +1169,9 @@ struct hi846 {
>  	struct v4l2_ctrl *hblank;
>  	struct v4l2_ctrl *exposure;
>  
> +	s64 link_freqs[ARRAY_SIZE(supported_modes)];
> +	int num_link_freqs;
> +
>  	struct mutex mutex; /* protect cur_mode, streaming and chip access */
>  	const struct hi846_mode *cur_mode;
>  	bool streaming;
> @@ -1192,21 +1197,41 @@ 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(const 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;
> +
> +	if (hi846->nr_lanes == 2)
> +		mipi_clk_div = mode->mipi_clk_div_2lane;
> +	else
> +		mipi_clk_div = mode->mipi_clk_div_4lane;
> +
> +	/*
> +	 * 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_get_link_freq(struct hi846 *hi846)
> +static int hi846_get_link_freq_index(const struct hi846 *hi846,
> +				     const struct hi846_mode *mode)
>  {
> -	u8 index = hi846_get_link_freq_index(hi846);
> +	u64 link_freq = hi846_get_link_freq(hi846, mode);
> +	int i;
> +
> +	for (i = 0; i < hi846->num_link_freqs; i++)
> +		if (hi846->link_freqs[i] == link_freq)
> +			return i;
>  
> -	return hi846_link_freqs[index];
> +	return -EINVAL;
>  }
>  
>  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);
> @@ -1429,8 +1454,8 @@ static int hi846_init_controls(struct hi846 *hi846)
>  	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);
> +				       hi846->num_link_freqs - 1,
> +				       0, hi846->link_freqs);
>  	if (hi846->link_freq)
>  		hi846->link_freq->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  
> @@ -1503,10 +1528,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;
> @@ -1699,6 +1723,7 @@ static int hi846_set_format(struct v4l2_subdev *sd,
>  	const struct hi846_datafmt *fmt = hi846_find_datafmt(mf->code);
>  	u32 tgt_fps;
>  	s32 vblank_def, h_blank;
> +	int idx;
>  
>  	if (!fmt) {
>  		mf->code = hi846_colour_fmts[0].code;
> @@ -1749,7 +1774,14 @@ 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));
> +	idx = hi846_get_link_freq_index(hi846, hi846->cur_mode);
> +	if (idx < 0) {
> +		dev_err(&client->dev,
> +			"failed to get link freq index: %d\n", idx);
> +		return -EINVAL;
> +	}
> +
> +	__v4l2_ctrl_s_ctrl(hi846->link_freq, idx);
>  	__v4l2_ctrl_s_ctrl_int64(hi846->pixel_rate,
>  				 hi846_calc_pixel_rate(hi846));
>  
> @@ -1947,20 +1979,33 @@ static int hi846_identify_module(struct hi846 *hi846)
>  	return 0;
>  }
>  
> -static s64 hi846_check_link_freqs(struct hi846 *hi846,
> -				  struct v4l2_fwnode_endpoint *ep)
> +static int hi846_add_link_freqs(struct hi846 *hi846, struct device *dev,
> +				struct v4l2_fwnode_endpoint *ep)
>  {
> -	const s64 *freqs = hi846_link_freqs;
> -	int freqs_count = ARRAY_SIZE(hi846_link_freqs);
> -	int i, j;
> -
> -	for (i = 0; i < freqs_count; i++) {
> -		for (j = 0; j < ep->nr_of_link_frequencies; j++)
> -			if (freqs[i] == ep->link_frequencies[j])
> -				break;
> -		if (j == ep->nr_of_link_frequencies)
> -			return freqs[i];
> -	}
> +	s64 hi846_link_freqs[ARRAY_SIZE(supported_modes)];
> +	unsigned long freq_bitmap;
> +	int ret, i;
> +
> +	/*
> +	 * Since the MCLK freq varies between platforms, calculating driver
> +	 * supported link freqs here.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(supported_modes); i++)
> +		hi846_link_freqs[i] = hi846_get_link_freq(hi846, &supported_modes[i]);
> +
> +	ret = v4l2_link_freq_to_bitmap(dev, ep->link_frequencies,
> +				       ep->nr_of_link_frequencies,
> +				       hi846_link_freqs,
> +				       ARRAY_SIZE(hi846_link_freqs),
> +				       &freq_bitmap);
> +	if (ret || !freq_bitmap)
> +		return ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(hi846_link_freqs); i++)

Could you use the hi846_link_freqs array as-is for the control?

The selectable modes are expected to depend on the chose link frequency,
which is not affected by setting the format, for instance. I wonder if it'd
be useful to squash the next patch into this one.

> +		if (BIT(i) & freq_bitmap) {
> +			hi846->link_freqs[hi846->num_link_freqs++] = hi846_link_freqs[i];
> +			dev_dbg(dev, "Add supported link frequency %lld\n", hi846_link_freqs[i]);
> +		}
>  
>  	return 0;
>  }
> @@ -1973,7 +2018,6 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
>  		.bus_type = V4L2_MBUS_CSI2_DPHY
>  	};
>  	int ret;
> -	s64 fq;
>  
>  	ep = fwnode_graph_get_next_endpoint(fwnode, NULL);
>  	if (!ep) {
> @@ -2004,11 +2048,10 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
>  		goto check_hwcfg_error;
>  	}
>  
> -	/* Check that link frequences for all the modes are in device tree */
> -	fq = hi846_check_link_freqs(hi846, &bus_cfg);
> -	if (fq) {
> -		dev_err(dev, "Link frequency of %lld is not supported\n", fq);
> -		ret = -EINVAL;
> +	/* Add link frequencies which are supported by both DT and the driver */
> +	ret = hi846_add_link_freqs(hi846, dev, &bus_cfg);
> +	if (ret) {
> +		dev_err(dev, "failed to add link frequency %d\n", ret);
>  		goto check_hwcfg_error;
>  	}
>  
> @@ -2041,30 +2084,24 @@ 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)
>  		return -ENOMEM;
>  
> -	ret = hi846_parse_dt(hi846, &client->dev);
> -	if (ret) {
> -		dev_err(&client->dev, "failed to check HW configuration: %d",
> -			ret);
> -		return ret;
> -	}
> -
> +	/* Get the MCLK first, since we need it to calculate link freqs */
>  	hi846->clock = devm_v4l2_sensor_clk_get(&client->dev, NULL);
>  	if (IS_ERR(hi846->clock))
>  		return dev_err_probe(&client->dev, PTR_ERR(hi846->clock),
>  				     "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);
> +	ret = hi846_parse_dt(hi846, &client->dev);
> +	if (ret) {
> +		dev_err(&client->dev, "failed to check HW configuration: %d",
> +			ret);
> +		return ret;
> +	}
>  
>  	for (i = 0; i < HI846_NUM_SUPPLIES; i++)
>  		hi846->supplies[i].supply = hi846_supply_names[i];

-- 
Regards,

Sakari Ailus