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

[Shravan.Chippa]

> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Sent: Tuesday, May 19, 2026 7:37 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@xxxxxxxxxxxxx>
> Cc: sakari.ailus@xxxxxxxxxxxxxxx; 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] 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, 19 May 2026 at 12:17, shravan kumar
> <shravan.chippa@xxxxxxxxxxxxx> 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.
> 
> Where is the sensor setup to configure this 222MHz link frequency?
> 
> I have a datasheet that lists support for 1782, 1188, and 891Mbit/s, which
> equates to 891, 594, and 445.5MHz link frequencies. There is no mention of
> supporting 444Mbit/s or 222MHz.
> The driver switches from the default 445.5MHz to 891MHz by changing
> SYS_MODE from 0x02 to 0x00 (it's an 8bit register, so I don't know why it's
> trying to write 0x0100).
> 
> As far as I can tell, this patch just changes the advertised link frequency, but
> the sensor will produce exactly the same 445.5MHz output. Can you tell me
> what I've missed?

Hi Dave,

I am attempting to change the value of the register IMX334_REG_INCKSEL2 to 0x0A in this patch, which sets the link frequency to 222 MHz and defines the supported resolutions; however, this behavior is not documented in the datasheet. Additionally, writing 0x0E to IMX334_REG_INCKSEL2 results in a 111 MHz link frequency, while writing 0x06 sets the link frequency to 445 MHz

For the 891 MHz link frequency, the SYS_MODE value is 0x100, and it is written automatically when the 3840×2160 resolution is selected. This behavior does not apply to the 222 MHz mode. For the 222 MHz link frequency, the required SYS_MODE value is 0x02.

Thanks,
Shravan

> Thanks
>   Dave
> 
> > Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
> > ---
> >  drivers/media/i2c/imx334.c | 112
> > +++++++++++++++++++++++++++++++++++--
> >  1 file changed, 106 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 9654f9268056..336de9cd8ff2 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          222500000
> >  #define IMX334_NUM_DATA_LANES          4
> >
> >  #define IMX334_REG_MIN                 0x00
> > @@ -209,6 +210,8 @@ struct imx334 {
> >         };
> >         u32 vblank;
> >         const struct imx334_mode *cur_mode;
> > +       const struct imx334_mode *new_supported_modes;
> > +       int new_modes_size;
> >         unsigned long link_freq_bitmap;
> >         u32 cur_code;
> >  };
> > @@ -216,6 +219,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 +490,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 +756,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->new_modes_size)
> >                 return -EINVAL;
> >
> >         code = imx334_get_format_code(imx334, fsize->code); @@ -721,9
> > +764,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->new_supported_modes[fsize->index].width;
> >         fsize->max_width = fsize->min_width;
> > -       fsize->min_height = supported_modes[fsize->index].height;
> > +       fsize->min_height =
> > + imx334->new_supported_modes[fsize->index].height;
> >         fsize->max_height = fsize->min_height;
> >
> >         return 0;
> > @@ -792,8 +835,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->new_supported_modes,
> > +                                     imx334->new_modes_size,
> >                                       width, height,
> >                                       fmt->format.width,
> > fmt->format.height);
> >
> > @@ -914,6 +957,9 @@ static int imx334_enable_streams(struct
> v4l2_subdev *sd,
> >                 goto err_rpm_put;
> >         }
> >
> > +       if (link_freq[imx334->cur_mode->link_freq_idx] ==
> IMX334_LINK_FREQ_222M)
> > +               cci_write(imx334->cci, IMX334_REG_INCKSEL2, 0x0a,
> > + NULL);
> > +
> >         /* Start streaming */
> >         ret = cci_write(imx334->cci, IMX334_REG_MODE_SELECT,
> >                         IMX334_MODE_STREAMING, NULL); @@ -979,6
> > +1025,55 @@ static int imx334_detect(struct imx334 *imx334)
> >         return 0;
> >  }
> >
> > +/**
> > + * imx334_update_supported_mode_array() - Search for the supported
> > + *                                        modes add them in the new list
> > + * @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 & (1 << i)) {
> > +                       for (j = 0; j < ARRAY_SIZE(supported_modes); j++) {
> > +                               if (supported_modes[j].link_freq_idx == i)
> > +                                       size++;
> > +                       }
> > +               }
> > +       }
> > +
> > +       if (!size)
> > +               return -EINVAL;
> > +
> > +       imx334->new_modes_size = size;
> > +
> > +       size = 0;
> > +
> > +       temp_ptr = devm_kmalloc(imx334->dev, imx334->new_modes_size *
> sizeof(struct imx334_mode),
> > +                               GFP_KERNEL);
> > +       if (!temp_ptr)
> > +               return -ENOMEM;
> > +
> > +       for (i = 0; i < ARRAY_SIZE(link_freq); i++) {
> > +               if (imx334->link_freq_bitmap & (1 << i)) {
> > +                       for (j = 0; j < ARRAY_SIZE(supported_modes); j++) {
> > +                               if (supported_modes[j].link_freq_idx == i) {
> > +                                       temp_ptr[size] = supported_modes[j];
> > +                                       size++;
> > +                               }
> > +                       }
> > +               }
> > +       }
> > +
> > +       imx334->new_supported_modes = temp_ptr;
> > +
> > +       return 0;
> > +}
> > +
> >  /**
> >   * imx334_parse_hw_config() - Parse HW configuration and check if
> supported
> >   * @imx334: pointer to imx334 device
> > @@ -1038,6 +1133,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);
> >
> > @@ -1251,7 +1351,7 @@ static int imx334_probe(struct i2c_client *client)
> >         }
> >
> >         /* Set default mode to max resolution */
> > -       imx334->cur_mode = &supported_modes[__ffs(imx334-
> >link_freq_bitmap)];
> > +       imx334->cur_mode =
> > + &imx334->new_supported_modes[__ffs(imx334->link_freq_bitmap)];
> >         imx334->cur_code = imx334_mbus_codes[0];
> >         imx334->vblank = imx334->cur_mode->vblank;
> >
> > --
> > 2.34.1
> >
> >