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 <[email protected]>
> Sent: Monday, August 3, 2026 11:21 AM
> To: [email protected]; [email protected]
> Cc: [email protected]; [email protected]; Conor Dooley -
> M52691 <[email protected]>; Valentina Fernandez Alanis -
> M63239 <[email protected]>; Praveen Kumar -
> I30718 <[email protected]>; shravan Chippa - I35088
> <[email protected]>
> Subject: [PATCH V3] media: i2c: imx334: add new link frequency configuration
>
> From: Shravan Chippa <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.