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

From: Shravan.Chippa

Date: Mon Aug 17 2026 - 05:31:55 EST

Hi Sakari,

I am sending a gentle ping on this patch. Please let me know if you
have any feedback or if any changes are required.

Regards,
Shravan

> -----Original Message-----
> From: shravan kumar <shravan.chippa@xxxxxxxxxxxxx>
> Sent: Monday, August 3, 2026 11:21 AM
> To: sakari.ailus@xxxxxxxxxxxxxxx; mchehab@xxxxxxxxxx
> Cc: linux-media@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Conor Dooley -
> M52691 <Conor.Dooley@xxxxxxxxxxxxx>; Valentina Fernandez Alanis -
> M63239 <Valentina.FernandezAlanis@xxxxxxxxxxxxx>; Praveen Kumar -
> I30718 <Praveen.Kumar@xxxxxxxxxxxxx>; shravan Chippa - I35088
> <Shravan.Chippa@xxxxxxxxxxxxx>
> Subject: [PATCH V3] media: i2c: imx334: add new link frequency configuration
>
> From: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
>
> Add 222 MHz link frequency support and refactor clock registers into per-
> frequency register lists. Introduce imx334_clk_params to map each link
> frequency to its register set and supported resolution range.
> Make the link frequency control writable so userspace can select the desired
> operating point.
>
> Signed-off-by: Shravan Chippa <shravan.chippa@xxxxxxxxxxxxx>
>
> ---
> Changes from V2 -> V3
>
> - Refactor: Introduce struct imx334_clk_params
> - Maps each link frequency to its clock register set and supported
> resolution range (width_max, height_max, width_min, height_min).
> - Includes a default_mode pointer for fallback when resolution is
> out of range for the selected link frequency.
>
> - Enhancement: Make link_freq control writable
> - Removed V4L2_CTRL_FLAG_READ_ONLY from link_freq_ctrl so userspace
> can select the desired link frequency operating point.
>
> - Enhancement: Add resolution bounds checking in set_pad_format
> - imx334_set_pad_format() now validates the selected mode against
> the clk_params width/height constraints and falls back to the
> default mode if out of range.
>
> - Cleanup: Remove __v4l2_ctrl_s_ctrl for link_freq in update_controls
> - Link frequency is no longer tied to mode, so the explicit control
> update in imx334_update_controls() was removed.
>
>
> Changes from V1 -> V2
>
> - Fix: Default mode selection bug
> - Problem: Used __ffs(link_freq_bitmap) as index into the compacted
> active_modes array. This index corresponds to a bit position in the
> bitmap, not an array index in the filtered modes list.
> - Fix: Use index 0, which is always the first valid mode in the
> filtered array.
>
> - Fix: Rename fields for clarity
> - Renamed struct fields:
> * new_supported_modes -> active_modes
> * new_modes_size -> num_active_modes
> - Updated kernel-doc header for struct imx334 to document new fields.
>
> - Fix: INCKSEL2 register write - add error handling and switch-case
> - Location: imx334_enable_streams()
> - Problem: Original code only handled 222 MHz case with no error
> checking (passed NULL to cci_write).
> - Fix: Replaced with switch-case covering all three link frequencies:
> * 891 MHz -> INCKSEL2 = 0x02
> * 445 MHz -> INCKSEL2 = 0x06
> * 222 MHz -> INCKSEL2 = 0x0a
> - Added proper error handling using &ret accumulator pattern with
> dev_err and goto err_rpm_put on failure.
>
> - Fix: Use BIT() macro
> - Location: imx334_update_supported_mode_array()
> - Replaced (1 << i) with BIT(i) for kernel coding style compliance.
>
> - Fix: Use devm_kmalloc_array() for overflow-safe allocation
> - Location: imx334_update_supported_mode_array()
> - Replaced: devm_kmalloc(dev, n * sizeof(struct imx334_mode),
> GFP_KERNEL)
> - With: devm_kmalloc_array(dev, n, sizeof(*temp_ptr), GFP_KERNEL)
> - Provides overflow-safe multiplication and uses sizeof(*ptr) idiom.
>
> - Cleanup: Condensed copy loop
> - Location: imx334_update_supported_mode_array(), second loop
> - Simplified the struct copy with post-increment:
> temp_ptr[size++] = supported_modes[j];
>
> - Fix: Updated function kernel-doc comment
> - Location: imx334_update_supported_mode_array()
> - Reworded: "Search for the supported modes add them in the new list"
> - To: "Build filtered modes array based on DTS link frequencies"
>
> - IMX334_LINK_FREQ_222M changed from 222500000 to 222750000
>
> ---
>
> drivers/media/i2c/imx334.c | 132 +++++++++++++++++++++++++++++++--
> ----
> 1 file changed, 112 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c index
> 553a16b84f4d..fb036873859c 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 222750000
> #define IMX334_NUM_DATA_LANES 4
>
> #define IMX334_REG_MIN 0x00
> @@ -154,7 +155,6 @@ struct imx334_reg_list {
> * @vblank_min: Minimal vertical blanking in lines
> * @vblank_max: Maximum vertical blanking in lines
> * @pclk: Sensor pixel clock
> - * @link_freq_idx: Link frequency index
> * @reg_list: Register list for sensor mode
> */
> struct imx334_mode {
> @@ -165,7 +165,28 @@ struct imx334_mode {
> u32 vblank_min;
> u32 vblank_max;
> u64 pclk;
> - u32 link_freq_idx;
> + struct imx334_reg_list reg_list;
> +};
> +
> +/**
> + * struct imx334_clk_params - imx334 sensor clock parameters
> + * @data_rate_per_lane: Data rate per lane in bits per second
> + * @link_freq: Link frequency in Hz
> + * @width_max: Maximum image width in pixels
> + * @height_max: Maximum image height in pixels
> + * @width_min: Minimum image width in pixels
> + * @height_min: Minimum image height in pixels
> + * @default_mode: Pointer to the default sensor mode
> + * @reg_list: Register list for clock configuration */ struct
> +imx334_clk_params {
> + u32 data_rate_per_lane;
> + u32 link_freq;
> + u32 width_max;
> + u32 height_max;
> + u32 width_min;
> + u32 height_min;
> + const struct imx334_mode *default_mode;
> struct imx334_reg_list reg_list;
> };
>
> @@ -216,6 +237,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 */ @@ -233,13 +255,6 @@ static
> const struct cci_reg_sequence common_mode_regs[] = {
> { IMX334_REG_UNREAD_PARAM6, 0x0008 },
> { IMX334_REG_XVS_XHS_OUTSEL, 0x20 },
> { IMX334_REG_XVS_XHS_DRV, 0x0f },
> - { IMX334_REG_BCWAIT_TIME, 0x3b },
> - { IMX334_REG_CPWAIT_TIME, 0x2a },
> - { IMX334_REG_INCKSEL1, 0x0129 },
> - { IMX334_REG_INCKSEL2, 0x06 },
> - { IMX334_REG_INCKSEL3, 0xa0 },
> - { IMX334_REG_INCKSEL4, 0x7e },
> - { IMX334_REG_SYS_MODE, 0x02 },
> { IMX334_REG_HADD_VADD, 0x00 },
> { IMX334_REG_VALID_EXPAND, 0x03 },
> { IMX334_REG_TCYCLE, 0x00 },
> @@ -397,6 +412,39 @@ static const struct cci_reg_sequence
> mode_3840x2160_regs[] = {
> { IMX334_REG_TPLX, 0x005f },
> };
>
> +/* Data rate 1782Mbps per lane and 891Mhz link frequency */ static
> +const struct cci_reg_sequence link_freq_891m_regs[] = {
> + { IMX334_REG_BCWAIT_TIME, 0x3b },
> + { IMX334_REG_CPWAIT_TIME, 0x2a },
> + { IMX334_REG_INCKSEL1, 0x0129 },
> + { IMX334_REG_INCKSEL2, 0x02 },
> + { IMX334_REG_INCKSEL3, 0xa0 },
> + { IMX334_REG_INCKSEL4, 0x7e },
> + { IMX334_REG_SYS_MODE, 0x00 },
> +};
> +
> +/* Data rate 891Mbps per lane and 445Mhz link frequency */ static const
> +struct cci_reg_sequence link_freq_445m_regs[] = {
> + { IMX334_REG_BCWAIT_TIME, 0x3b },
> + { IMX334_REG_CPWAIT_TIME, 0x2a },
> + { IMX334_REG_INCKSEL1, 0x0129 },
> + { IMX334_REG_INCKSEL2, 0x06 },
> + { IMX334_REG_INCKSEL3, 0xa0 },
> + { IMX334_REG_INCKSEL4, 0x7e },
> + { IMX334_REG_SYS_MODE, 0x02 },
> +};
> +
> +/* Data rate 445Mbps per lane and 222Mhz link frequency */ static const
> +struct cci_reg_sequence link_freq_222m_regs[] = {
> + { IMX334_REG_BCWAIT_TIME, 0x3b },
> + { IMX334_REG_CPWAIT_TIME, 0x2a },
> + { IMX334_REG_INCKSEL1, 0x0129 },
> + { IMX334_REG_INCKSEL2, 0x0a },
> + { IMX334_REG_INCKSEL3, 0xa0 },
> + { IMX334_REG_INCKSEL4, 0x7e },
> + { IMX334_REG_SYS_MODE, 0x02 },
> +};
> +
> static const char * const imx334_test_pattern_menu[] = {
> "Disabled",
> "Vertical Color Bars",
> @@ -442,7 +490,6 @@ static const struct imx334_mode supported_modes[]
> = {
> .vblank_min = 90,
> .vblank_max = 132840,
> .pclk = 594000000,
> - .link_freq_idx = 0,
> .reg_list = {
> .num_of_regs =
> ARRAY_SIZE(mode_3840x2160_regs),
> .regs = mode_3840x2160_regs,
> @@ -455,7 +502,6 @@ static const struct imx334_mode supported_modes[]
> = {
> .vblank_min = 45,
> .vblank_max = 132840,
> .pclk = 297000000,
> - .link_freq_idx = 1,
> .reg_list = {
> .num_of_regs =
> ARRAY_SIZE(mode_1920x1080_regs),
> .regs = mode_1920x1080_regs,
> @@ -468,7 +514,6 @@ static const struct imx334_mode supported_modes[]
> = {
> .vblank_min = 45,
> .vblank_max = 132840,
> .pclk = 297000000,
> - .link_freq_idx = 1,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> .regs = mode_1280x720_regs,
> @@ -481,7 +526,6 @@ static const struct imx334_mode supported_modes[]
> = {
> .vblank_min = 45,
> .vblank_max = 132840,
> .pclk = 297000000,
> - .link_freq_idx = 1,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_640x480_regs),
> .regs = mode_640x480_regs,
> @@ -489,6 +533,46 @@ static const struct imx334_mode
> supported_modes[] = {
> },
> };
>
> +static const struct imx334_clk_params imx334_clk_params[] = {
> + {
> + .data_rate_per_lane = 1782000000,
> + .link_freq = IMX334_LINK_FREQ_891M,
> + .width_max = 3840,
> + .height_max = 2160,
> + .width_min = 3840,
> + .height_min = 2160,
> + .default_mode = &supported_modes[0], /* 3840x2160 */
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(link_freq_891m_regs),
> + .regs = link_freq_891m_regs,
> + },
> + }, {
> + .data_rate_per_lane = 891000000,
> + .link_freq = IMX334_LINK_FREQ_445M,
> + .width_max = 1920,
> + .height_max = 1080,
> + .width_min = 640,
> + .height_min = 480,
> + .default_mode = &supported_modes[1], /* 1920x1080 */
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(link_freq_445m_regs),
> + .regs = link_freq_445m_regs,
> + },
> + }, {
> + .data_rate_per_lane = 445500000,
> + .link_freq = IMX334_LINK_FREQ_222M,
> + .width_max = 1920,
> + .height_max = 1080,
> + .width_min = 640,
> + .height_min = 480,
> + .default_mode = &supported_modes[1], /* 1920x1080 */
> + .reg_list = {
> + .num_of_regs = ARRAY_SIZE(link_freq_222m_regs),
> + .regs = link_freq_222m_regs,
> + },
> + }
> +};
> +
> /**
> * to_imx334() - imv334 V4L2 sub-device to imx334 device.
> * @subdev: pointer to imx334 V4L2 sub-device @@ -512,10 +596,6 @@
> static int imx334_update_controls(struct imx334 *imx334, {
> int ret;
>
> - ret = __v4l2_ctrl_s_ctrl(imx334->link_freq_ctrl, mode->link_freq_idx);
> - if (ret)
> - return ret;
> -
> ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> mode->pclk, 1, mode->pclk);
> if (ret)
> @@ -746,6 +826,7 @@ static int imx334_set_pad_format(struct v4l2_subdev
> *sd, {
> struct imx334 *imx334 = to_imx334(sd);
> const struct imx334_mode *mode;
> + const struct imx334_clk_params *clk_params;
> int ret = 0;
>
> mode = v4l2_find_nearest_size(supported_modes,
> @@ -753,6 +834,11 @@ static int imx334_set_pad_format(struct
> v4l2_subdev *sd,
> width, height,
> fmt->format.width, fmt->format.height);
>
> + clk_params = &imx334_clk_params[imx334->link_freq_ctrl->val];
> + if (mode->width > clk_params->width_max || mode->height >
> clk_params->height_max ||
> + mode->width < clk_params->width_min || mode->height <
> clk_params->height_min)
> + mode = clk_params->default_mode;
> +
> imx334_fill_pad_format(imx334, mode, fmt);
> fmt->format.code = imx334_get_format_code(imx334, fmt-
> >format.code);
>
> @@ -824,6 +910,15 @@ static int imx334_enable_streams(struct
> v4l2_subdev *sd,
> goto err_rpm_put;
> }
>
> + /* Write sensor link freq registers */
> + reg_list = &imx334_clk_params[imx334->link_freq_ctrl->val].reg_list;
> + ret = cci_multi_reg_write(imx334->cci, reg_list->regs,
> + reg_list->num_of_regs, NULL);
> + if (ret) {
> + dev_err(imx334->dev, "fail to write initial registers\n");
> + goto err_rpm_put;
> + }
> +
> /* Write sensor mode registers */
> reg_list = &imx334->cur_mode->reg_list;
> ret = cci_multi_reg_write(imx334->cci, reg_list->regs, @@ -1096,9
> +1191,6 @@ static int imx334_init_controls(struct imx334 *imx334)
> __ffs(imx334-
> >link_freq_bitmap),
> link_freq);
>
> - if (imx334->link_freq_ctrl)
> - imx334->link_freq_ctrl->flags |=
> V4L2_CTRL_FLAG_READ_ONLY;
> -
> imx334->hblank_ctrl = v4l2_ctrl_new_std(ctrl_hdlr,
> &imx334_ctrl_ops,
> V4L2_CID_HBLANK,
> --
> 2.34.1