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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mismatch between hardware registers and V4L2 Selection API bounds for new 6MP/8MP modes.
- [Low] Brittle array size math for mode iteration depending on hardcoded array layout.
--

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

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

This patch adds 6MP and 8MP modes for the hi846 sensor using 4-lane MIPI
CSI-2. It extracts register sequences from a downstream driver to maximize
sensor utilization.

> diff --git a/drivers/media/i2c/hi846.c b/drivers/media/i2c/hi846.c
> index 9decfda747876..68302d0b7ca5f 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},

[ ... ]

> @@ -1128,9 +1228,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,
> +		},

Does this crop configuration exceed the V4L2 Selection API bounds?

In hi846_get_selection(), V4L2_SEL_TGT_CROP_BOUNDS and
V4L2_SEL_TGT_NATIVE_SIZE are hardcoded to width 3264 and height 2448 with
left and top set to 0. Here, crop.left is 0x46 and crop.width is 3264,
resulting in a right bound of 3334, which exceeds the native size.

Also, there appears to be a contradiction in the hardware registers. The
crop.left is set to 0x46 here, but HI846_REG_X_START_H is explicitly set to
0x0000 in the mode_3264x1836_config array.

Should crop.left be 0x00, or do the V4L2 hardware bounds and X_START_H need
to be updated to match the physical pixel array?

> +	},
> +	{
> +		.width = 3264,
> +		.height = 2448,

[ ... ]

> +		.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)

Is it safe to rely on this array index arithmetic for mode support?

This implicitly assumes the supported_modes array is strictly ordered so
that the last two modes lack 2-lane support. If modes are added, removed,
or reordered in the future, these macros could silently produce incorrect
array boundaries.

Would it be safer to dynamically filter modes based on
reg_list_2lane.num_of_regs instead?

>  #define NUM_MODES_4LANE		(ARRAY_SIZE(supported_modes) - 1)

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