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

[Shravan.Chippa]

Hi Dave,

> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Sent: Thursday, May 21, 2026 5:23 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
> 
> 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.

Yes, it is working with out corruption.
Thanks, Dave, again for taking a deeper look at my patch.
I am using 480p, 720p, and 1080p with a 4‑lane configuration.

Bandwidth Calculation
Data rate per lane = 2 × link frequency
= 2 × 222 MHz
= 444 Mbps per lane

Total raw bandwidth = 444 Mbps × 4
= 1,776 Mbps (1.776 Gbps)

Pixel Rate Calculation
Pixel rate = Total bandwidth / bits per pixel

RAW10 (10 bpp): 177.6 Mpixels/s
RAW12 (12 bpp): 148.0 Mpixels/s

Example: Maximum FPS for 1920 × 1080
FPS = Pixel rate / (width × height)

RAW10:
FPS = 177.6 M / (1920 × 1080) ≈ 85.6 fps

RAW12:
FPS = 148.0 M / (1920 × 1080) ≈ 71.4 fps

This is a theoretical calculation. In practice, horizontal and vertical blanking, along with other overheads, will reduce the achievable frame rate.

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

This value I got from oscilloscope, so kept the same

> 
> > > > > >  #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;
> }

I will try to fix this, in next revision.

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

Hardware support is required to conduct this testing. At a link frequency of 111 MHz, the frame rate is expected to be significantly lower.

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


This I will take care next revision 

> 
> > > > > > +       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]?
>

Yes, this approach will work because the updated supported modes array will include only the modes supported by the device. As a result, the first entry in the list (mode[0]) will be selected.

Thank you, Dave, for reviewing my patch again and for taking the time to look into it in more detail.

Thanks,
Shravan
 
>   Dave
> 
> > > > > >         imx334->cur_code = imx334_mbus_codes[0];
> > > > > >         imx334->vblank = imx334->cur_mode->vblank;
> > > > > >
> > > > > > --
> > > > > > 2.34.1
> > > > > >
> > > > > >