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

[Dave Stevenson]

On Thu, 21 May 2026 at 05:48, <Shravan.Chippa@xxxxxxxxxxxxx> wrote:
>
> 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.

Is 30fps that max that is achievable with the current 445.5MHz link frequency?
Sony have done their usual in the datasheet of giving individual
examples rather than specifications. It lists the 2x2 binned mode as
needing 891/1188 Mbit/s for 30 or 25fps, and 1188/1782 Mbit/s for 60
or 50fps. If 30fps can actually be achieved on 222MHz/445.5Mbit/s,
then why can't 60fps be achieved on 445.5MHz/891Mbit/s?

It's a tangent, so if the modes all work at the correct frame rates
without corruption, then fine.

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

Yes, sorry. It's the problem of starting to look at a new module and
starting to see the issues and ambiguities. I'll just review the
patch.
I might see if I can find a vendor of a suitable module to play with.
At least with it being a Starvis sensor they tend to stay in
production longer.

> 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

Half of 445500000 would be 222750000.

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

The return value from cci_write is not checked, nor ret passed in.

I'm not maintainer of this driver, but a simple refactor to remove
INCKSEL2 from the register tables and have one switch here
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:
    /* Undocumented, but controlled by IF_PLL_GC */
    cci_write(imx334->cci, IMX334_REG_INCKSEL2, 0xa, &ret);
    break;
}
would make the driver far easier to read. It'd also be trivial to add
the 111MHz link frequency at a later date if desired.

> > > > >         /* 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);

Memory says that using devm_kmalloc_array is now preferred to protect
against overflows (however unlikely in this case).

There is kmalloc_objs as well to remove the requirement for sizeof,
but there doesn't appear to be a devm_ variant of those.

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

Does this work now?
It was relying on mode[0] using link_idx 0, and mode[1] using link_idx
1. If link frequency 0 wasn't enabled, then it switched to a supported
mode.
You're now generating your own version of the table with only the
modes where the corresponding link frequency is enabled, so shouldn't
the default just be imx334->new_supported_modes[0]?

  Dave

> > > > >         imx334->cur_code = imx334_mbus_codes[0];
> > > > >         imx334->vblank = imx334->cur_mode->vblank;
> > > > >
> > > > > --
> > > > > 2.34.1
> > > > >
> > > > >