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

[Shravan.Chippa]

> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@xxxxxxxxxxxxxxx>
> Sent: Friday, May 22, 2026 5: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
> 
> On Fri, 22 May 2026 at 05:00, <Shravan.Chippa@xxxxxxxxxxxxx> wrote:
> >
> > 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.
> 
> OK, great. It sounds like Sony has been very conservative for some weird
> reason.
> 
> > >
> > > > > > 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
> 
> An oscilloscope won't measure to that level of accuracy. Divide by 2 would be
> the logical assumption.
> 
Hi Deve

I will make by2 in this case

> <snip>
> 
> > > > > > > >
> > > > > > > >         /* 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.
> 
> Create a device tree configuration with only 222MHz in it.

Sorry missed this test case.

> 
> imx334->link_freq_bitmap will be 0x04.
> __ffs(imx334->link_freq_bitmap) will be 2.
> new_supported_modes will contain
> - [0] 1920x1080
> - [1] 1280x720
> - [2] 640x480.
> 
> cur_mode = &imx334->new_supported_modes[__ffs(imx334-
> >link_freq_bitmap)
> there leaves cur_mode pointing at the 640x480 mode [2], and not the max
> resolution mode that the comment states.

In this case, I think it is better to use
cur_mode = &imx334->new_supported_modes[0];
so that the first available link-frequency mode is selected.

Thanks,
Shravan

> 
>   Dave