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

From: Sakari Ailus

Date: Mon Jul 20 2026 - 10:25:30 EST

Hi Shravan,

On Tue, May 26, 2026 at 10:10:51AM +0530, shravan kumar wrote:
> 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.

E.g. the CCS driver calculates the PLL configuration based on the input
parameters, one of which is the link frequency. There are other drivers
that do the same. Improving the driver to calculate the PLL frequency
dynamically could be problematic from UAPI point of view. Therefore I think
it'd be safer to require the user to pick the link frequency right from the
start.

I probably wouldn't allocate a separate array for active modes but just
loop over the existing array as needed.

>
> 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 int?

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

Writing the register should be removed from the register lists with this.
No need for the comment either.

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

unsigned int for all, please.

> + struct imx334_mode *temp_ptr;
> +
> + for (i = 0; i < ARRAY_SIZE(link_freq); i++) {
> + if (imx334->link_freq_bitmap & BIT(i)) {

Perhaps

if (!(...))
continue;

?

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

I think you could call temp_ptr active_modes.

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

--
Kind regards,

Sakari Ailus