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

[Dave Stevenson]

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.

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.

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

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.

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