RE: [PATCH V2] media: i2c: imx334: add new link frequency configuration

[Shravan.Chippa]

Genting ping ..


> -----Original Message-----
> From: shravan kumar <shravan.chippa@xxxxxxxxxxxxx>
> Sent: Tuesday, May 26, 2026 10:11 AM
> To: sakari.ailus@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Conor Dooley -
> M52691 <Conor.Dooley@xxxxxxxxxxxxx>; Valentina Fernandez Alanis -
> M63239 <Valentina.FernandezAlanis@xxxxxxxxxxxxx>; Praveen Kumar -
> I30718 <Praveen.Kumar@xxxxxxxxxxxxx>; shravan Chippa - I35088
> <Shravan.Chippa@xxxxxxxxxxxxx>
> Subject: [PATCH V2] media: i2c: imx334: add new link frequency configuration
> 
> From: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> 
> Add support for a new 222 MHz link frequency configuration to the
> IMX334 driver and dynamically generate the supported modes array based on
> the link frequencies specified in the DTS. When multiple link frequencies
> support the same resolution, the driver selects the first matching entry;
> therefore, the link frequency must be explicitly defined in the DTS to avoid
> resolution conflicts.
> The link frequency is a read‑only parameter and is automatically set based on
> the selected resolution and DTS configuration.
> 
> Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> 
> ---
> 
> Changes from V1 -> V2
> 
> 1. Fix: Default mode selection bug
>    - Problem: Used __ffs(link_freq_bitmap) as index into the compacted
>      active_modes array. This index corresponds to a bit position in the
>      bitmap, not an array index in the filtered modes list.
>    - Fix: Use index 0, which is always the first valid mode in the
>      filtered array.
> 
> 2. Fix: Rename fields for clarity
>    - Renamed struct fields:
>      * new_supported_modes -> active_modes
>      * new_modes_size -> num_active_modes
>    - Updated kernel-doc header for struct imx334 to document new fields.
> 
> 3. Fix: INCKSEL2 register write - add error handling and switch-case
>    - Location: imx334_enable_streams()
>    - Problem: Original code only handled 222 MHz case with no error
>      checking (passed NULL to cci_write).
>    - Fix: Replaced with switch-case covering all three link frequencies:
>      * 891 MHz -> INCKSEL2 = 0x02
>      * 445 MHz -> INCKSEL2 = 0x06
>      * 222 MHz -> INCKSEL2 = 0x0a
>    - Added proper error handling using &ret accumulator pattern with
>      dev_err and goto err_rpm_put on failure.
> 
> 4. Fix: Use BIT() macro
>    - Location: imx334_update_supported_mode_array()
>    - Replaced (1 << i) with BIT(i) for kernel coding style compliance.
> 
> 5. Fix: Use devm_kmalloc_array() for overflow-safe allocation
>    - Location: imx334_update_supported_mode_array()
>    - Replaced: devm_kmalloc(dev, n * sizeof(struct imx334_mode),
> GFP_KERNEL)
>    - With: devm_kmalloc_array(dev, n, sizeof(*temp_ptr), GFP_KERNEL)
>    - Provides overflow-safe multiplication and uses sizeof(*ptr) idiom.
> 
> 6. Cleanup: Condensed copy loop
>    - Location: imx334_update_supported_mode_array(), second loop
>    - Simplified the struct copy with post-increment:
>      temp_ptr[size++] = supported_modes[j];
> 
> 7. Fix: Updated function kernel-doc comment
>    - Location: imx334_update_supported_mode_array()
>    - Reworded: "Search for the supported modes add them in the new list"
>    - To: "Build filtered modes array based on DTS link frequencies"
> 
> 8. IMX334_LINK_FREQ_222M changed from 222500000 to 222750000
> 
> ---
> 
>  drivers/media/i2c/imx334.c | 128
> +++++++++++++++++++++++++++++++++++--
>  1 file changed, 121 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c index
> 9654f9268056..b6ef314ee717 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -109,6 +109,7 @@
>  /* CSI2 HW configuration */
>  #define IMX334_LINK_FREQ_891M		891000000
>  #define IMX334_LINK_FREQ_445M		445500000
> +#define IMX334_LINK_FREQ_222M		222750000
>  #define IMX334_NUM_DATA_LANES		4
> 
>  #define IMX334_REG_MIN			0x00
> @@ -187,6 +188,8 @@ struct imx334_mode {
>   * @again_ctrl: Pointer to analog gain control
>   * @vblank: Vertical blanking in lines
>   * @cur_mode: Pointer to current selected sensor mode
> + * @active_modes: Pointer to modes filtered by DTS link frequencies
> + * @num_active_modes: Number of entries in active_modes array
>   * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
>   * @cur_code: current selected format code
>   */
> @@ -209,6 +212,8 @@ struct imx334 {
>  	};
>  	u32 vblank;
>  	const struct imx334_mode *cur_mode;
> +	const struct imx334_mode *active_modes;
> +	int num_active_modes;
>  	unsigned long link_freq_bitmap;
>  	u32 cur_code;
>  };
> @@ -216,6 +221,7 @@ struct imx334 {
>  static const s64 link_freq[] = {
>  	IMX334_LINK_FREQ_891M,
>  	IMX334_LINK_FREQ_445M,
> +	IMX334_LINK_FREQ_222M,
>  };
> 
>  /* Sensor common mode registers values */ @@ -486,6 +492,45 @@ static
> const struct imx334_mode supported_modes[] = {
>  			.num_of_regs = ARRAY_SIZE(mode_640x480_regs),
>  			.regs = mode_640x480_regs,
>  		},
> +	}, {
> +		.width = 1920,
> +		.height = 1080,
> +		.hblank = 2480,
> +		.vblank = 1170,
> +		.vblank_min = 45,
> +		.vblank_max = 132840,
> +		.pclk = 297000000,
> +		.link_freq_idx = 2,
> +		.reg_list = {
> +			.num_of_regs =
> ARRAY_SIZE(mode_1920x1080_regs),
> +			.regs = mode_1920x1080_regs,
> +		},
> +	}, {
> +		.width = 1280,
> +		.height = 720,
> +		.hblank = 2480,
> +		.vblank = 1170,
> +		.vblank_min = 45,
> +		.vblank_max = 132840,
> +		.pclk = 297000000,
> +		.link_freq_idx = 2,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> +			.regs = mode_1280x720_regs,
> +		},
> +	}, {
> +		.width = 640,
> +		.height = 480,
> +		.hblank = 2480,
> +		.vblank = 1170,
> +		.vblank_min = 45,
> +		.vblank_max = 132840,
> +		.pclk = 297000000,
> +		.link_freq_idx = 2,
> +		.reg_list = {
> +			.num_of_regs = ARRAY_SIZE(mode_640x480_regs),
> +			.regs = mode_640x480_regs,
> +		},
>  	},
>  };
> 
> @@ -713,7 +758,7 @@ static int imx334_enum_frame_size(struct
> v4l2_subdev *sd,
>  	struct imx334 *imx334 = to_imx334(sd);
>  	u32 code;
> 
> -	if (fsize->index >= ARRAY_SIZE(supported_modes))
> +	if (fsize->index >= imx334->num_active_modes)
>  		return -EINVAL;
> 
>  	code = imx334_get_format_code(imx334, fsize->code); @@ -721,9
> +766,9 @@ static int imx334_enum_frame_size(struct v4l2_subdev *sd,
>  	if (fsize->code != code)
>  		return -EINVAL;
> 
> -	fsize->min_width = supported_modes[fsize->index].width;
> +	fsize->min_width = imx334->active_modes[fsize->index].width;
>  	fsize->max_width = fsize->min_width;
> -	fsize->min_height = supported_modes[fsize->index].height;
> +	fsize->min_height = imx334->active_modes[fsize->index].height;
>  	fsize->max_height = fsize->min_height;
> 
>  	return 0;
> @@ -792,8 +837,8 @@ static int imx334_set_pad_format(struct v4l2_subdev
> *sd,
>  	const struct imx334_mode *mode;
>  	int ret = 0;
> 
> -	mode = v4l2_find_nearest_size(supported_modes,
> -				      ARRAY_SIZE(supported_modes),
> +	mode = v4l2_find_nearest_size(imx334->active_modes,
> +				      imx334->num_active_modes,
>  				      width, height,
>  				      fmt->format.width, fmt->format.height);
> 
> @@ -914,6 +959,23 @@ static int imx334_enable_streams(struct
> v4l2_subdev *sd,
>  		goto err_rpm_put;
>  	}
> 
> +	/* Override INCKSEL2 based on the active link frequency */
> +	switch (link_freq[imx334->cur_mode->link_freq_idx]) {
> +	case IMX334_LINK_FREQ_891M:
> +		cci_write(imx334->cci, IMX334_REG_INCKSEL2, 0x02, &ret);
> +		break;
> +	case IMX334_LINK_FREQ_445M:
> +		cci_write(imx334->cci, IMX334_REG_INCKSEL2, 0x06, &ret);
> +		break;
> +	case IMX334_LINK_FREQ_222M:
> +		cci_write(imx334->cci, IMX334_REG_INCKSEL2, 0x0a, &ret);
> +		break;
> +	}
> +	if (ret) {
> +		dev_err(imx334->dev, "fail to set INCKSEL2\n");
> +		goto err_rpm_put;
> +	}
> +
>  	/* Start streaming */
>  	ret = cci_write(imx334->cci, IMX334_REG_MODE_SELECT,
>  			IMX334_MODE_STREAMING, NULL);
> @@ -979,6 +1041,53 @@ static int imx334_detect(struct imx334 *imx334)
>  	return 0;
>  }
> 
> +/**
> + * imx334_update_supported_mode_array() - Build filtered modes array
> based on
> + *                                        DTS link frequencies
> + * @imx334: pointer to imx334 device
> + *
> + * Return: 0 if successful, error code otherwise.
> + */
> +static int imx334_update_supported_mode_array(struct imx334 *imx334) {
> +	int i, j, size = 0;
> +	struct imx334_mode *temp_ptr;
> +
> +	for (i = 0; i < ARRAY_SIZE(link_freq); i++) {
> +		if (imx334->link_freq_bitmap & BIT(i)) {
> +			for (j = 0; j < ARRAY_SIZE(supported_modes); j++) {
> +				if (supported_modes[j].link_freq_idx == i)
> +					size++;
> +			}
> +		}
> +	}
> +
> +	if (!size)
> +		return -EINVAL;
> +
> +	imx334->num_active_modes = size;
> +
> +	size = 0;
> +
> +	temp_ptr = devm_kmalloc_array(imx334->dev, imx334-
> >num_active_modes,
> +				      sizeof(*temp_ptr), GFP_KERNEL);
> +	if (!temp_ptr)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < ARRAY_SIZE(link_freq); i++) {
> +		if (imx334->link_freq_bitmap & BIT(i)) {
> +			for (j = 0; j < ARRAY_SIZE(supported_modes); j++) {
> +				if (supported_modes[j].link_freq_idx == i)
> +					temp_ptr[size++] =
> supported_modes[j];
> +			}
> +		}
> +	}
> +
> +	imx334->active_modes = temp_ptr;
> +
> +	return 0;
> +}
> +
>  /**
>   * imx334_parse_hw_config() - Parse HW configuration and check if
> supported
>   * @imx334: pointer to imx334 device
> @@ -1038,6 +1147,11 @@ static int imx334_parse_hw_config(struct imx334
> *imx334)
>  				       link_freq, ARRAY_SIZE(link_freq),
>  				       &imx334->link_freq_bitmap);
> 
> +	if (ret)
> +		goto done_endpoint_free;
> +
> +	ret = imx334_update_supported_mode_array(imx334);
> +
>  done_endpoint_free:
>  	v4l2_fwnode_endpoint_free(&bus_cfg);
> 
> @@ -1250,8 +1364,8 @@ static int imx334_probe(struct i2c_client *client)
>  		goto error_power_off;
>  	}
> 
> -	/* Set default mode to max resolution */
> -	imx334->cur_mode = &supported_modes[__ffs(imx334-
> >link_freq_bitmap)];
> +	/* Set default mode to first entry in the filtered modes array */
> +	imx334->cur_mode = &imx334->active_modes[0];
>  	imx334->cur_code = imx334_mbus_codes[0];
>  	imx334->vblank = imx334->cur_mode->vblank;
> 
> --
> 2.34.1