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

[Shravan.Chippa]

Hi Dave,

> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Sent: Wednesday, May 20, 2026 6:06 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 Wed, 20 May 2026 at 06:21, <Shravan.Chippa@xxxxxxxxxxxxx> wrote:
> >
> >
> >
> > > -----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
> 
> Apologies, I'd totally missed that you were writing
> IMX334_REG_INCKSEL2 directly from imx334_enable_streams. So it's the
> magic of the PLL_IF_GC bits, and they aren't documented.
> Most likely the register is only controlling a divider, in which case the link
> frequency would be 222.275MHz.


No need to apologize—I appreciate you taking the time to review it. Your comments are helpful, 
and thank you for taking another look.
Yes, the PLL_IF_GC bits are responsible for changing the link frequency.

> 
> It was fairly ugly with IMX334_REG_INCKSEL2 being written from
> common_mode_regs and then written again from mode_3840x2160_regs,
> but now it may get written from imx334_enable_streams too.
> It'd be nice if that was all factored out into clean handling for link frequency
> (and input clock?), but seeing as this is an orphaned driver there's supposedly
> no one who really cares too much.
> 
> A further question: if the link frequency is lower then doesn't hblank need to
> be extended to allow enough time to output the data? Or is there enough
> slack in the current timings to give enough time, or the pixel rate has changed
> too?
> All modes except 3840x2160 advertise a pixel clock of 297MPix/s with the
> same hblank of 2480 pixels and vblank of 1170 lines. However the actual
> HMAX register isn't changed between modes (0x44c written from
> common_mode_regs), so I suspect they all actually give different refresh rates
> from those advertised.
>

There are two additional registers related to the resolution: IMX334_REG_Y_OUT_SIZE and IMX334_REG_HNUM.
I believe the hblank value is sufficient, and with a 222 MHz link frequency, achieving 30 fps should be possible. 
Increasing the link frequency may allow the frame rate to exceed 30 fps.

> > 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.
> 
> My comment was more that IMX334_REG_SYS_MODE is defined as
> CCI_REG8(0x319e), so only the bottom 8 bits of any value will ever be taken.
> Trying to write 0x100 will therefore be equate to writing 0x00.
> So it's more odd behaviour from the original driver rather than anything in this
> patch.

Correct. This behavior is not introduced by this patch. The register is only 8 bits wide, so writing 0x100 will result in 0x00 being written instead.

> 
> Sorry, I saw this patch and took a look as it's another Starvis sensor, but I seem
> to be seeing various potential issues lurking.

This patch only modifies the link frequency by updating the IMX334_REG_INCKSEL2 register value and configures the sensor to operate at 30 fps.

Thanks,
Shravan

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