* [PATCH] media: i2c: imx334: add new link frequency configuration
@ 2026-05-19 10:41 shravan kumar
2026-05-19 14:06 ` Dave Stevenson
2026-05-19 18:08 ` kernel test robot
0 siblings, 2 replies; 10+ messages in thread
From: shravan kumar @ 2026-05-19 10:41 UTC (permalink / raw)
To: sakari.ailus, mchehab
Cc: linux-media, linux-kernel, conor.dooley,
valentina.fernandezalanis, praveen.kumar, shravan.chippa
From: Shravan Chippa <shravan.chippa@microchip.com>
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.
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-19 10:41 [PATCH] media: i2c: imx334: add new link frequency configuration shravan kumar
@ 2026-05-19 14:06 ` Dave Stevenson
2026-05-20 5:20 ` Shravan.Chippa
2026-05-19 18:08 ` kernel test robot
1 sibling, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2026-05-19 14:06 UTC (permalink / raw)
To: shravan kumar
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, conor.dooley,
valentina.fernandezalanis, praveen.kumar
Hi Shravan
On Tue, 19 May 2026 at 12:17, shravan kumar
<shravan.chippa@microchip.com> wrote:
>
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> 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?
Thanks
Dave
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
> 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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-19 10:41 [PATCH] media: i2c: imx334: add new link frequency configuration shravan kumar
2026-05-19 14:06 ` Dave Stevenson
@ 2026-05-19 18:08 ` kernel test robot
1 sibling, 0 replies; 10+ messages in thread
From: kernel test robot @ 2026-05-19 18:08 UTC (permalink / raw)
To: shravan kumar, sakari.ailus, mchehab
Cc: oe-kbuild-all, linux-media, linux-kernel, conor.dooley,
valentina.fernandezalanis, praveen.kumar, shravan.chippa
Hi shravan,
kernel test robot noticed the following build warnings:
[auto build test WARNING on sailus-media-tree/master]
[also build test WARNING on linuxtv-media-pending/master media-tree/master linus/master v7.1-rc4 next-20260518]
[cannot apply to sailus-media-tree/streams]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/shravan-kumar/media-i2c-imx334-add-new-link-frequency-configuration/20260519-191302
base: git://linuxtv.org/sailus/media_tree.git master
patch link: https://lore.kernel.org/r/20260519104129.2001018-1-shravan.chippa%40microchip.com
patch subject: [PATCH] media: i2c: imx334: add new link frequency configuration
config: sh-allmodconfig (https://download.01.org/0day-ci/archive/20260520/202605200257.113Ddqne-lkp@intel.com/config)
compiler: sh4-linux-gcc (GCC) 15.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260520/202605200257.113Ddqne-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202605200257.113Ddqne-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> Warning: drivers/media/i2c/imx334.c:216 struct member 'new_supported_modes' not described in 'imx334'
>> Warning: drivers/media/i2c/imx334.c:216 struct member 'new_modes_size' not described in 'imx334'
>> Warning: drivers/media/i2c/imx334.c:216 struct member 'new_supported_modes' not described in 'imx334'
>> Warning: drivers/media/i2c/imx334.c:216 struct member 'new_modes_size' not described in 'imx334'
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-19 14:06 ` Dave Stevenson
@ 2026-05-20 5:20 ` Shravan.Chippa
2026-05-20 12:35 ` Dave Stevenson
0 siblings, 1 reply; 10+ messages in thread
From: Shravan.Chippa @ 2026-05-20 5:20 UTC (permalink / raw)
To: dave.stevenson
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Sent: Tuesday, May 19, 2026 7:37 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> <Praveen.Kumar@microchip.com>
> 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@microchip.com> wrote:
> >
> > From: Shravan Chippa <shravan.chippa@microchip.com>
> >
> > 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
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.
Thanks,
Shravan
> Thanks
> Dave
>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> > 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
> >
> >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-20 5:20 ` Shravan.Chippa
@ 2026-05-20 12:35 ` Dave Stevenson
2026-05-21 4:48 ` Shravan.Chippa
0 siblings, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2026-05-20 12:35 UTC (permalink / raw)
To: Shravan.Chippa
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
Hi Shravan
On Wed, 20 May 2026 at 06:21, <Shravan.Chippa@microchip.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Sent: Tuesday, May 19, 2026 7:37 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> > <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> > <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> > <Praveen.Kumar@microchip.com>
> > 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@microchip.com> wrote:
> > >
> > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > >
> > > 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@microchip.com>
> > > ---
> > > 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
> > >
> > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-20 12:35 ` Dave Stevenson
@ 2026-05-21 4:48 ` Shravan.Chippa
2026-05-21 11:52 ` Dave Stevenson
0 siblings, 1 reply; 10+ messages in thread
From: Shravan.Chippa @ 2026-05-21 4:48 UTC (permalink / raw)
To: dave.stevenson
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
Hi Dave,
> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Sent: Wednesday, May 20, 2026 6:06 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> <Praveen.Kumar@microchip.com>
> 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@microchip.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Sent: Tuesday, May 19, 2026 7:37 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley -
> > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > I30718 <Praveen.Kumar@microchip.com>
> > > 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@microchip.com> wrote:
> > > >
> > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > >
> > > > 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.
> > 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.
Thanks,
Shravan
>
> Dave
>
> > Thanks,
> > Shravan
> >
> > > Thanks
> > > Dave
> > >
> > > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > > > ---
> > > > 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
> > > >
> > > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-21 4:48 ` Shravan.Chippa
@ 2026-05-21 11:52 ` Dave Stevenson
2026-05-22 4:00 ` Shravan.Chippa
0 siblings, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2026-05-21 11:52 UTC (permalink / raw)
To: Shravan.Chippa
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
On Thu, 21 May 2026 at 05:48, <Shravan.Chippa@microchip.com> wrote:
>
> Hi Dave,
>
> > -----Original Message-----
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Sent: Wednesday, May 20, 2026 6:06 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> > <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> > <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> > <Praveen.Kumar@microchip.com>
> > 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@microchip.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > Sent: Tuesday, May 19, 2026 7:37 PM
> > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley -
> > > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > > I30718 <Praveen.Kumar@microchip.com>
> > > > 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@microchip.com> wrote:
> > > > >
> > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > >
> > > > > 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@microchip.com>
> > > > > ---
> > > > > 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
> > > > >
> > > > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-21 11:52 ` Dave Stevenson
@ 2026-05-22 4:00 ` Shravan.Chippa
2026-05-22 11:35 ` Dave Stevenson
0 siblings, 1 reply; 10+ messages in thread
From: Shravan.Chippa @ 2026-05-22 4:00 UTC (permalink / raw)
To: dave.stevenson
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
Hi Dave,
> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Sent: Thursday, May 21, 2026 5:23 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> <Praveen.Kumar@microchip.com>
> 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@microchip.com> wrote:
> >
> > Hi Dave,
> >
> > > -----Original Message-----
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Sent: Wednesday, May 20, 2026 6:06 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley -
> > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > I30718 <Praveen.Kumar@microchip.com>
> > > 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@microchip.com>
> wrote:
> > > >
> > > >
> > > >
> > > > > -----Original Message-----
> > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > Sent: Tuesday, May 19, 2026 7:37 PM
> > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor
> > > > > Dooley -
> > > > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis
> > > > > -
> > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar
> > > > > -
> > > > > I30718 <Praveen.Kumar@microchip.com>
> > > > > 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@microchip.com> wrote:
> > > > > >
> > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > >
> > > > > > 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@microchip.com>
> > > > > > ---
> > > > > > 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
> > > > > >
> > > > > >
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-22 4:00 ` Shravan.Chippa
@ 2026-05-22 11:35 ` Dave Stevenson
2026-05-24 14:23 ` Shravan.Chippa
0 siblings, 1 reply; 10+ messages in thread
From: Dave Stevenson @ 2026-05-22 11:35 UTC (permalink / raw)
To: Shravan.Chippa
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
On Fri, 22 May 2026 at 05:00, <Shravan.Chippa@microchip.com> wrote:
>
> Hi Dave,
>
> > -----Original Message-----
> > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > Sent: Thursday, May 21, 2026 5:23 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> > <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> > <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> > <Praveen.Kumar@microchip.com>
> > 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@microchip.com> wrote:
> > >
> > > Hi Dave,
> > >
> > > > -----Original Message-----
> > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > Sent: Wednesday, May 20, 2026 6:06 PM
> > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley -
> > > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > > I30718 <Praveen.Kumar@microchip.com>
> > > > 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@microchip.com>
> > wrote:
> > > > >
> > > > >
> > > > >
> > > > > > -----Original Message-----
> > > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > > Sent: Tuesday, May 19, 2026 7:37 PM
> > > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > > > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor
> > > > > > Dooley -
> > > > > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis
> > > > > > -
> > > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar
> > > > > > -
> > > > > > I30718 <Praveen.Kumar@microchip.com>
> > > > > > 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@microchip.com> wrote:
> > > > > > >
> > > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > >
> > > > > > > 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@microchip.com>
> > > > > > > ---
> > > > > > > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] media: i2c: imx334: add new link frequency configuration
2026-05-22 11:35 ` Dave Stevenson
@ 2026-05-24 14:23 ` Shravan.Chippa
0 siblings, 0 replies; 10+ messages in thread
From: Shravan.Chippa @ 2026-05-24 14:23 UTC (permalink / raw)
To: dave.stevenson
Cc: sakari.ailus, mchehab, linux-media, linux-kernel, Conor.Dooley,
Valentina.FernandezAlanis, Praveen.Kumar
> -----Original Message-----
> From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> Sent: Friday, May 22, 2026 5:06 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley - M52691
> <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis - M63239
> <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar - I30718
> <Praveen.Kumar@microchip.com>
> 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@microchip.com> wrote:
> >
> > Hi Dave,
> >
> > > -----Original Message-----
> > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > Sent: Thursday, May 21, 2026 5:23 PM
> > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor Dooley -
> > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis -
> > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar -
> > > I30718 <Praveen.Kumar@microchip.com>
> > > 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@microchip.com> wrote:
> > > >
> > > > Hi Dave,
> > > >
> > > > > -----Original Message-----
> > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > Sent: Wednesday, May 20, 2026 6:06 PM
> > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor
> > > > > Dooley -
> > > > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez Alanis
> > > > > -
> > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen Kumar
> > > > > -
> > > > > I30718 <Praveen.Kumar@microchip.com>
> > > > > 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@microchip.com>
> > > wrote:
> > > > > >
> > > > > >
> > > > > >
> > > > > > > -----Original Message-----
> > > > > > > From: Dave Stevenson <dave.stevenson@raspberrypi.com>
> > > > > > > Sent: Tuesday, May 19, 2026 7:37 PM
> > > > > > > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> > > > > > > Cc: sakari.ailus@linux.intel.com; mchehab@kernel.org; linux-
> > > > > > > media@vger.kernel.org; linux-kernel@vger.kernel.org; Conor
> > > > > > > Dooley -
> > > > > > > M52691 <Conor.Dooley@microchip.com>; Valentina Fernandez
> > > > > > > Alanis
> > > > > > > -
> > > > > > > M63239 <Valentina.FernandezAlanis@microchip.com>; Praveen
> > > > > > > Kumar
> > > > > > > -
> > > > > > > I30718 <Praveen.Kumar@microchip.com>
> > > > > > > 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@microchip.com> wrote:
> > > > > > > >
> > > > > > > > From: Shravan Chippa <shravan.chippa@microchip.com>
> > > > > > > >
> > > > > > > > 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@microchip.com>
> > > > > > > > ---
> > > > > > > > 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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2026-05-24 14:23 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-05-19 10:41 [PATCH] media: i2c: imx334: add new link frequency configuration shravan kumar
2026-05-19 14:06 ` Dave Stevenson
2026-05-20 5:20 ` Shravan.Chippa
2026-05-20 12:35 ` Dave Stevenson
2026-05-21 4:48 ` Shravan.Chippa
2026-05-21 11:52 ` Dave Stevenson
2026-05-22 4:00 ` Shravan.Chippa
2026-05-22 11:35 ` Dave Stevenson
2026-05-24 14:23 ` Shravan.Chippa
2026-05-19 18:08 ` kernel test robot
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®