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

[Shravan.Chippa]

Hi Sakari,

> -----Original Message-----
> From: Sakari Ailus <sakari.ailus@xxxxxxxxxxxxxxx>
> Sent: Monday, July 20, 2026 7:40 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@xxxxxxxxxxxxx>
> Cc: mchehab@xxxxxxxxxx; 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>
> Subject: Re: [PATCH V2] media: i2c: imx334: add new link frequency
> configuration
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> 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.

Thanks for the review,
I will work on improving the driver to calculate the PLL frequency dynamically while setting the resolution.

Thanks,
Shravan

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