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

[Dave Stevenson]

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.

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

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.

  Dave