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

[PATCH v4 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 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                     | 251 +++++++++++++++---
 2 files changed, 210 insertions(+), 43 deletions(-)

-- 
2.54.0

[PATCH v4 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 v4 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:
- [High] Runtime PM usage count underflow and torn hardware state due to unhandled negative return values.
--

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

media: hi846: fix hi846_write_reg_16 handling

This commit attempts to fix error propagation by resetting the error
variable to 0 when it is positive. It clears the positive return value
from pm_runtime_get_if_in_use() before performing I2C writes.

> 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;
> +

While this clears the positive return values from pm_runtime_get_if_in_use(),
does this leave negative return values unhandled in the caller?

If pm_runtime_get_if_in_use() in hi846_set_ctrl() returns a negative error
like -EINVAL when Runtime PM is disabled, the usage count is not incremented.
The check for (!ret || ret == -EAGAIN) fails, so execution falls through with
a negative value.

Because this negative value is passed into hi846_write_reg_16(), the check
just above this change will cause I2C writes to be silently skipped for
controls like V4L2_CID_EXPOSURE. Other controls like V4L2_CID_ANALOGUE_GAIN
might ignore the error and execute unconditionally, leaving the hardware in a
torn state.

Finally, since execution continues, pm_runtime_put() is called unconditionally
at the end of hi846_set_ctrl().

Because the usage count was never incremented for the negative error case,
will this cause a PM usage count underflow?

Would it be better to correctly validate the PM return value directly in
hi846_set_ctrl() with a check like if (ret <= 0) return 0; instead of
clearing the positive error here?

>  	put_unaligned_be16(reg, buf);
>  	put_unaligned_be16(val, buf + 2);

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

[PATCH v4 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, this commit did

1. dropped exposed link freqs as a v4l2_ctrl, since we are runtime
now, it is inconvenient and unnecessary to expose it, and
hi846_set_ctrl has nothing to do with it.

2. attached pll_cfg_4lane to current mode, and use it with clock, lane
count to calculate link frequency, we use 4-lane config as default,
but we can double it easily for 2-lane case.

3. dropped 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>
---
 drivers/media/i2c/hi846.c | 75 ++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 37 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 7f069aca0fce..61297ef66a0e 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;
+	/* PLL configuration for 4-lane link at this resolution */
+	u16 pll_cfg_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,7 @@ static const struct hi846_mode supported_modes[] = {
 	{
 		.width = 640,
 		.height = 480,
-		.link_freq_index = FREQ_INDEX_640,
+		.pll_cfg_4lane = 0x4924, /* HI846_REG_PLL_CFG_MIPI2_H */
 		.fps = 120,
 		.frame_len = 631,
 		.llp = HI846_LINE_LENGTH,
@@ -1086,7 +1080,7 @@ static const struct hi846_mode supported_modes[] = {
 	{
 		.width = 1280,
 		.height = 720,
-		.link_freq_index = FREQ_INDEX_1280,
+		.pll_cfg_4lane = 0x4924, /* HI846_REG_PLL_CFG_MIPI2_H */
 		.fps = 90,
 		.frame_len = 842,
 		.llp = HI846_LINE_LENGTH,
@@ -1112,7 +1106,7 @@ static const struct hi846_mode supported_modes[] = {
 	{
 		.width = 1632,
 		.height = 1224,
-		.link_freq_index = FREQ_INDEX_1280,
+		.pll_cfg_4lane = 0x4924, /* HI846_REG_PLL_CFG_MIPI2_H */
 		.fps = 30,
 		.frame_len = 2526,
 		.llp = HI846_LINE_LENGTH,
@@ -1161,7 +1155,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 +1185,37 @@ 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 *cur_mode)
 {
-	return hi846->cur_mode->link_freq_index;
-}
+	u16 cfg = cur_mode->pll_cfg_4lane;
+	u64 mclk = clk_get_rate(hi846->clock);
 
-static u64 hi846_get_link_freq(struct hi846 *hi846)
-{
-	u8 index = hi846_get_link_freq_index(hi846);
+	/* NOTE: 6, 7 actually map to 8, 10, but this won't be that big */
+	u8 post_div1 = 1 + FIELD_GET(GENMASK(10, 8), cfg);
+	u8 post_div2 = 1 << FIELD_GET(GENMASK(12, 11), cfg);
 
-	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
+	 */
+	u64 link_freq = mclk / 3 * 90 / post_div1 / post_div2;
+	/*
+	 * for shared modes, since lane count is halved for 2-lane, then getting
+	 * link_freq doubled to match the same data rate. Since 720x480 is
+	 * 2-lane only(reg_list_4lane.num_of_regs == 0), no pll cfg for 4-lane,
+	 * the pll cfg is for 2-lane, so use the cfg as is.
+	 */
+	if (hi846->nr_lanes == 2 && cur_mode->reg_list_4lane.num_of_regs)
+		link_freq *= 2;
+
+	return link_freq;
 }
 
 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 +1435,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 +1504,10 @@ 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));
+		link_freq);
 
 	do_div(link_freq, fps);
 	frame_length = link_freq;
@@ -1749,7 +1750,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 +1950,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;
-- 
2.54.0

Re: [PATCH v4 2/5] media: hi846: fix link frequency handling

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Dropping the mandatory V4L2_CID_LINK_FREQ control breaks compatibility with downstream MIPI CSI-2 receivers.
- [High] Uninitialized clock pointer access causes the device tree link frequency validation to be silently bypassed.
- [High] Integer division precision loss in the link frequency calculation causes mismatches with exact device tree specifications.
- [Medium] The commit message claims to drop the mclk clock rate check, but the corresponding code remains unmodified.
--

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

media: hi846: fix link frequency handling

This commit modifies link frequency handling to use runtime calculation
based on PLL configuration, lane count, and external clock rate, removing
hardcoded link frequencies. It drops the exposed link_freq control and
adjusts pixel rate calculations.

>     3. dropped mclk clock rate check.

The commit message says the mclk clock rate check was dropped, but looking at
the driver, the check if (mclk_freq != 25000000) appears to still be present
in hi846_probe(). Was this change accidentally omitted from the patch?

[ ... ]

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce5..61297ef66a0eb 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c

[ ... ]

> @@ -1192,21 +1185,37 @@ 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 *cur_mode)
>  {
> -	return hi846->cur_mode->link_freq_index;
> -}
> +	u16 cfg = cur_mode->pll_cfg_4lane;
> +	u64 mclk = clk_get_rate(hi846->clock);
>  
> -static u64 hi846_get_link_freq(struct hi846 *hi846)
> -{
> -	u8 index = hi846_get_link_freq_index(hi846);
> +	/* NOTE: 6, 7 actually map to 8, 10, but this won't be that big */
> +	u8 post_div1 = 1 + FIELD_GET(GENMASK(10, 8), cfg);
> +	u8 post_div2 = 1 << FIELD_GET(GENMASK(12, 11), cfg);
>  
> -	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
> +	 */
> +	u64 link_freq = mclk / 3 * 90 / post_div1 / post_div2;

Could this integer division cause a precision loss?

If mclk is 25000000, 25000000 / 3 truncates to 8333333. Multiplied by 90 and
divided by 4 (the post dividers), this evaluates to 187499992 Hz instead of
the exact 187500000 Hz.

This truncation could cause hi846_check_link_freqs() to fail the equality
check against the frequencies specified in the device tree, falsely rejecting
valid hardware configurations.

Would it be better to reorder the operations to multiply before dividing
(e.g., mclk * 90 / 3 / ...)?

[ ... ]

> @@ -1426,14 +1435,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;
> -

Will dropping the V4L2_CID_LINK_FREQ control break compatibility with
downstream MIPI CSI-2 receivers?

The V4L2 specification mandates the V4L2_CID_LINK_FREQ control for MIPI CSI-2
transmitters. Downstream receiver hardware queries this control via
v4l2_get_link_freq() to dynamically configure their own PLLs and lane timing
constraints. Removing it might prevent receivers from successfully locking onto
the transmitter's data lanes.

[ ... ]

> @@ -1950,16 +1950,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]);

Can this access hi846->clock before it is initialized?

If hi846_check_link_freqs() is called during probe via hi846_parse_dt() before
the clock is initialized, hi846->clock will be NULL.

Calling clk_get_rate(NULL) safely returns 0, leading to a computed link_freq
of 0. When this 0 frequency is not found in the DT endpoints, this function
returns 0. The caller checks if (fq), which evaluates to false for 0,
incorrectly treating the mismatch as a success and bypassing the DT hardware
capability validation.

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

Re: [PATCH v4 2/5] media: hi846: fix link frequency handling

[Sakari Ailus]

Hi Penguy,

On Mon, May 11, 2026 at 06:39:24PM +0800, Pengyu Luo wrote:
> link frequency is tied to PLL configuration, lane count, and external

A sentence begins with a capital letter, also the subject does.

> clock rate, so use runtime here instead of hardcoding for specific
> configuration. To implement this, this commit did
> 
> 1. dropped exposed link freqs as a v4l2_ctrl, since we are runtime
> now, it is inconvenient and unnecessary to expose it, and
> hi846_set_ctrl has nothing to do with it.

Please describe the changes the patch does in imperative form.

> 
> 2. attached pll_cfg_4lane to current mode, and use it with clock, lane
> count to calculate link frequency, we use 4-lane config as default,
> but we can double it easily for 2-lane case.
> 
> 3. dropped 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>
> ---
>  drivers/media/i2c/hi846.c | 75 ++++++++++++++++++++-------------------
>  1 file changed, 38 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 7f069aca0fce..61297ef66a0e 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;
> +	/* PLL configuration for 4-lane link at this resolution */
> +	u16 pll_cfg_4lane;

Please use separate fields for different PLL configuration parameters, i.e.
not register values.

>  
>  	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,7 @@ static const struct hi846_mode supported_modes[] = {
>  	{
>  		.width = 640,
>  		.height = 480,
> -		.link_freq_index = FREQ_INDEX_640,
> +		.pll_cfg_4lane = 0x4924, /* HI846_REG_PLL_CFG_MIPI2_H */
>  		.fps = 120,
>  		.frame_len = 631,
>  		.llp = HI846_LINE_LENGTH,
> @@ -1086,7 +1080,7 @@ static const struct hi846_mode supported_modes[] = {
>  	{
>  		.width = 1280,
>  		.height = 720,
> -		.link_freq_index = FREQ_INDEX_1280,
> +		.pll_cfg_4lane = 0x4924, /* HI846_REG_PLL_CFG_MIPI2_H */
>  		.fps = 90,
>  		.frame_len = 842,
>  		.llp = HI846_LINE_LENGTH,
> @@ -1112,7 +1106,7 @@ static const struct hi846_mode supported_modes[] = {
>  	{
>  		.width = 1632,
>  		.height = 1224,
> -		.link_freq_index = FREQ_INDEX_1280,
> +		.pll_cfg_4lane = 0x4924, /* HI846_REG_PLL_CFG_MIPI2_H */
>  		.fps = 30,
>  		.frame_len = 2526,
>  		.llp = HI846_LINE_LENGTH,
> @@ -1161,7 +1155,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 +1185,37 @@ 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 *cur_mode)
>  {
> -	return hi846->cur_mode->link_freq_index;
> -}
> +	u16 cfg = cur_mode->pll_cfg_4lane;
> +	u64 mclk = clk_get_rate(hi846->clock);
>  
> -static u64 hi846_get_link_freq(struct hi846 *hi846)
> -{
> -	u8 index = hi846_get_link_freq_index(hi846);
> +	/* NOTE: 6, 7 actually map to 8, 10, but this won't be that big */
> +	u8 post_div1 = 1 + FIELD_GET(GENMASK(10, 8), cfg);
> +	u8 post_div2 = 1 << FIELD_GET(GENMASK(12, 11), cfg);

s/1U/1/

>  
> -	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
> +	 */
> +	u64 link_freq = mclk / 3 * 90 / post_div1 / post_div2;

This needs div_u64().

Don't divide by 3 upfront, you'll lose information.

> +	/*
> +	 * for shared modes, since lane count is halved for 2-lane, then getting
> +	 * link_freq doubled to match the same data rate. Since 720x480 is
> +	 * 2-lane only(reg_list_4lane.num_of_regs == 0), no pll cfg for 4-lane,
> +	 * the pll cfg is for 2-lane, so use the cfg as is.
> +	 */
> +	if (hi846->nr_lanes == 2 && cur_mode->reg_list_4lane.num_of_regs)
> +		link_freq *= 2;
> +
> +	return link_freq;
>  }
>  
>  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 +1435,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 +1504,10 @@ 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));
> +		link_freq);

Fits on the same line.

>  
>  	do_div(link_freq, fps);
>  	frame_length = link_freq;
> @@ -1749,7 +1750,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 +1950,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;

I think v4l2_link_freq_to_bitmap() would be useful here.

>  	}
>  
>  	return 0;

-- 
Regards,

Sakari Ailus

[PATCH v4 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, and it is set to the default mode.

To fix it, specifying the supported modes for different lane cases,
once the lane count is determined.

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>
---
 drivers/media/i2c/hi846.c | 26 ++++++++++++++++++++------
 1 file changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index 61297ef66a0e..c77b34a5e166 100644
--- a/drivers/media/i2c/hi846.c
+++ b/drivers/media/i2c/hi846.c
@@ -1130,6 +1130,8 @@ static const struct hi846_mode supported_modes[] = {
 		},
 	}
 };
+#define NUM_MODES_2LANE		ARRAY_SIZE(supported_modes)
+#define NUM_MODES_4LANE		(ARRAY_SIZE(supported_modes) - 1)
 
 struct hi846_datafmt {
 	u32 code;
@@ -1162,6 +1164,8 @@ struct hi846 {
 
 	struct mutex mutex; /* protect cur_mode, streaming and chip access */
 	const struct hi846_mode *cur_mode;
+	const struct hi846_mode *supported_modes;
+	int num_modes;
 	bool streaming;
 };
 
@@ -1736,8 +1740,8 @@ 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),
+		v4l2_find_nearest_size(hi846->supported_modes,
+				       hi846->num_modes,
 				       width, height, mf->width, mf->height);
 	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
 		hi846->cur_mode->width, hi846->cur_mode->height);
@@ -1821,8 +1825,11 @@ 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);
+	const struct hi846_mode *supported_modes = hi846->supported_modes;
+
 
-	if (fse->pad || fse->index >= ARRAY_SIZE(supported_modes))
+	if (fse->pad || fse->index >= hi846->num_modes)
 		return -EINVAL;
 
 	if (fse->code != HI846_MEDIA_BUS_FORMAT) {
@@ -1950,12 +1957,12 @@ static int hi846_identify_module(struct hi846 *hi846)
 static s64 hi846_check_link_freqs(struct hi846 *hi846,
 				  struct v4l2_fwnode_endpoint *ep)
 {
-	int freqs_count = ARRAY_SIZE(supported_modes);
+	int freqs_count = hi846->num_modes;
 	u64 link_freq;
 	int i, j;
 
 	for (i = 0; i < freqs_count; i++) {
-		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
+		link_freq = hi846_get_link_freq(hi846, &hi846->supported_modes[i]);
 		for (j = 0; j < ep->nr_of_link_frequencies; j++)
 			if (link_freq == ep->link_frequencies[j])
 				break;
@@ -1999,6 +2006,13 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
 
 	hi846->nr_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
 
+	hi846->supported_modes = supported_modes;
+	hi846->num_modes = NUM_MODES_2LANE;
+	if (hi846->nr_lanes == 4) {
+		hi846->supported_modes = supported_modes + 1;
+		hi846->num_modes = NUM_MODES_4LANE;
+	}
+
 	if (!bus_cfg.nr_of_link_frequencies) {
 		dev_err(dev, "link-frequency property not found in DT\n");
 		ret = -EINVAL;
@@ -2088,7 +2102,7 @@ static int hi846_probe(struct i2c_client *client)
 	if (ret)
 		goto err_power_off;
 
-	hi846->cur_mode = &supported_modes[0];
+	hi846->cur_mode = &hi846->supported_modes[0];
 
 	ret = hi846_init_controls(hi846);
 	if (ret) {
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Hardware configuration validation for link frequencies is silently bypassed because of a missing clock initialization.
- [High] V4L2 format constraints are not enforced when evaluating `V4L2_SUBDEV_FORMAT_TRY`, allowing arbitrary dimensions.
- [Medium] The logic to filter modes for 4-lane configurations relies on brittle array pointer math and implicit array ordering.
- [Low] The local variable `supported_modes` shadows a global file-scope variable.
--

commit d690502fca6d668750580bed748cc5e7a41d5785
Author: Pengyu Luo <mitltlatltl@gmail.com>
Date:   Mon May 11 10:39:25 2026 +0000

    media: hi846: fix modes handling for different lane cases
    
    When using 4-lane, v4l2_find_nearest_size may return an unsupported
    mode, 640x480 mode, and it is set to the default mode. To fix it,
    specifying the supported modes for different lane cases, once the lane
    count is determined.
    
    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 61297ef66a0eb..c77b34a5e1662 100644
> --- a/drivers/media/i2c/hi846.c
> +++ b/drivers/media/i2c/hi846.c

[ ... ]

> @@ -1736,8 +1740,8 @@ 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),
> +		v4l2_find_nearest_size(hi846->supported_modes,
> +				       hi846->num_modes,
>  				       width, height, mf->width, mf->height);
>  	dev_dbg(&client->dev, "%s: found mode: %dx%d\n", __func__,
>  		hi846->cur_mode->width, hi846->cur_mode->height);

Is the V4L2_SUBDEV_FORMAT_TRY request handled correctly earlier in this
function?

The API mandates that TRY requests must be validated, clamped, and adjusted
to match the hardware supported sizes identically to active format requests.
By returning early and blindly copying the format like this:

if (format->which == V4L2_SUBDEV_FORMAT_TRY) {
        *v4l2_subdev_state_get_format(sd_state, format->pad) = *mf;
        return 0;
}

Does this allow userspace to inject arbitrary dimensions into the subdev
state?

[ ... ]

> @@ -1821,8 +1825,11 @@ 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);
> +	const struct hi846_mode *supported_modes = hi846->supported_modes;
> +
>  
> -	if (fse->pad || fse->index >= ARRAY_SIZE(supported_modes))
> +	if (fse->pad || fse->index >= hi846->num_modes)
>  		return -EINVAL;

This is not a bug, but does this local variable shadow the global file-scope
supported_modes array? This might trigger compiler warnings and cause
confusion during future refactoring.

[ ... ]

> @@ -1950,12 +1957,12 @@ static int hi846_identify_module(struct hi846 *hi846)
>  static s64 hi846_check_link_freqs(struct hi846 *hi846,
>  				  struct v4l2_fwnode_endpoint *ep)
>  {
> -	int freqs_count = ARRAY_SIZE(supported_modes);
> +	int freqs_count = hi846->num_modes;
>  	u64 link_freq;
>  	int i, j;
>  
>  	for (i = 0; i < freqs_count; i++) {
> -		link_freq = hi846_get_link_freq(hi846, &supported_modes[i]);
> +		link_freq = hi846_get_link_freq(hi846, &hi846->supported_modes[i]);
>  		for (j = 0; j < ep->nr_of_link_frequencies; j++)
>  			if (link_freq == ep->link_frequencies[j])
>  				break;

Does this logic successfully validate the hardware configuration?

Looking at hi846_probe(), it calls hi846_parse_dt() which uses this function
to check frequencies. However, this happens before the hi846 clock is
initialized via devm_v4l2_sensor_clk_get().

Because the clock is zero-initialized, hi846_get_link_freq() will calculate
a frequency of 0. This loop will then fail to find 0 in the device tree
endpoint list and ultimately return 0.

Since the caller checks if (fq), which evaluates to false, does this
silently bypass the hardware configuration validation entirely?

[ ... ]

> @@ -1999,6 +2006,13 @@ static int hi846_parse_dt(struct hi846 *hi846, struct device *dev)
>  
>  	hi846->nr_lanes = bus_cfg.bus.mipi_csi2.num_data_lanes;
>  
> +	hi846->supported_modes = supported_modes;
> +	hi846->num_modes = NUM_MODES_2LANE;
> +	if (hi846->nr_lanes == 4) {
> +		hi846->supported_modes = supported_modes + 1;
> +		hi846->num_modes = NUM_MODES_4LANE;
> +	}
> +

Is this pointer math relying on implicit array ordering? It appears to
assume that there is exactly one unsupported mode for 4-lane setups and
that it is always located at index 0.

If a new mode is added later, or a 2-lane-only mode is appended to the
end of the array, this offset calculation will fail. Could we use
v4l2_find_nearest_size_conditional() here instead and pass a condition
function to dynamically evaluate if reg_list_4lane.num_of_regs is greater
than zero?

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

[PATCH v4 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>
---
 drivers/media/i2c/hi846.c | 154 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 152 insertions(+), 2 deletions(-)

diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
index c77b34a5e166..12df1dc96163 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",
@@ -1128,9 +1228,59 @@ static const struct hi846_mode supported_modes[] = {
 			.width = 1632 * 2,
 			.height = 1224 * 2,
 		},
-	}
+	},
+	{
+		.width = 3264,
+		.height = 1836,
+		.pll_cfg_4lane = 0x4124, /* HI846_REG_PLL_CFG_MIPI2_H */
+		.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,
+		.pll_cfg_4lane = 0x4124, /* HI846_REG_PLL_CFG_MIPI2_H */
+		.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,
+		},
+	},
 };
-#define NUM_MODES_2LANE		ARRAY_SIZE(supported_modes)
+#define NUM_MODES_2LANE		(ARRAY_SIZE(supported_modes) - 2)
 #define NUM_MODES_4LANE		(ARRAY_SIZE(supported_modes) - 1)
 
 struct hi846_datafmt {
-- 
2.54.0

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The V4L2 crop rectangles for the newly added 6MP and 8MP modes exceed the static V4L2 crop bounds returned by `hi846_get_selection()` and contradict the hardware register configurations.
--

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

media: hi846: Add 6MP and 8MP modes support

This patch adds 6MP and 8MP modes to maximize sensor utilization on the
hi846 sensor. These modes require 4-lane MIPI CSI-2 and use register
sequences extracted from the downstream Windows driver.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index c77b34a5e1662..12df1dc96163e 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_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},

Does this register configuration contradict the crop configuration below? 

HI846_REG_X_START_H is configured to 0x0000, meaning the hardware reads
from column 0. However, the crop left offset is set to 0x46 (70). Exposing
a left crop offset to userspace while the hardware is programmed to read
from 0 seems contradictory.

> +	{HI846_REG_X_OUTPUT_SIZE_H,		0x0cc0},
> +	{HI846_REG_Y_OUTPUT_SIZE_H,		0x0990},

[ ... ]

> @@ -1128,9 +1228,59 @@ static const struct hi846_mode supported_modes[] = {
>  			.width = 1632 * 2,
>  			.height = 1224 * 2,
>  		},
> -	}
> +	},

[ ... ]

> +	{
> +		.width = 3264,
> +		.height = 2448,
> +		.pll_cfg_4lane = 0x4124, /* HI846_REG_PLL_CFG_MIPI2_H */
> +		.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,

Could this crop configuration break userspace boundary checks? 

In hi846_get_selection(), V4L2_SEL_TGT_CROP_BOUNDS unconditionally returns a 
hardcoded rectangle:

static int hi846_get_selection(...)
{
...
	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;
		return 0;
...
}

With the newly introduced 3264x2448 mode defining its active crop rectangle
with .left = 70 and .top = 64, the right boundary becomes 3334 and the bottom 
boundary becomes 2512. 

These values strictly exceed the crop bounds. According to the V4L2
specification, any valid crop rectangle must fit completely inside the crop
bounds. Returning a crop bound that is physically smaller than the allowed 
crop geometries breaks boundary checks in applications like libcamera.

> +		},
> +	},
>  };

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

[PATCH v4 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 / post_div1 / post_div2 =
25Mhz / 3 * 90 / 1 / {1,2} = 187.5Mhz, 375Mhz

Signed-off-by: Pengyu Luo <mitltlatltl@gmail.com>
---
 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

Re: [PATCH v4 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 exact link frequencies of 187500000 and 375000000 will fail the exact-match validation in the hi846 driver, causing the sensor probe to fail with -EINVAL.
- [Low] The arithmetic formula provided in the commit message is mathematically incorrect and misrepresents the driver's internal logic.
--

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

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

This commit updates the link frequencies for the hi846 camera sensor on the
imx8mq-librem5 board to match the expected values calculated from the mclk.

>     According to the hi846 datasheet and the register lists, they are
> 
>     mclk / prediv * multiplier / post_div1 / post_div2 =
>     25Mhz / 3 * 90 / 1 / {1,2} = 187.5Mhz, 375Mhz

This isn't a bug, but is the arithmetic in the commit message correct?

Mathematically, 25 / 3 * 90 is exactly 750. Dividing 750 by 1 and then by 1
or 2 results in 750 MHz and 375 MHz. To arrive at 187.5 MHz, the division
should be / 2 / 2, which corresponds to post_div1 = 2 and post_div2 = 2 in
the driver logic.

> diff --git a/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi b/arch/arm64/boot/dts/freescale/imx8mq-librem5.dtsi
> index eee390c272105..959a5c8dcd0d3 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>;

Does this regression cause the sensor probe to fail with -EINVAL?

The hi846 driver dynamically calculates expected link frequencies from mclk
using the C expression: mclk / 3 * 90 / post_div1 / post_div2.

In C, operators with the same precedence are evaluated left-to-right. With
mclk configured to 25,000,000 Hz, the division 25000000 / 3 occurs first
and truncates to 8,333,333.

Subsequent multiplication by 90 and division yields 187,499,992 and
374,999,984 instead of exactly 187,500,000 and 375,000,000.

Because 187500000 != 187499992, the exact equality check in the driver:
    if (link_freq == ep->link_frequencies[j])
will fail.

This rejects these mathematically ideal frequencies and aborts the probe,
breaking camera initialization.

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