Re: [PATCH v3 2/2] media: i2c: add Himax HM1246 image sensor driver
From: Tarang Raval <hidden>
Date: 2025-09-17 11:59:23
Also in:
linux-media, lkml
Hi Matthias,
Am 13.09.2025 um 11:11 schrieb Tarang Raval:quoted
Hi Matthias,quoted
Add a V4L2 sub-device driver for Himax HM1246 image sensor. The Himax HM1246-AWD is a 1/3.7-Inch CMOS image sensor SoC with an active array size of 1296 x 976. It is programmable through an I2C interface and connected via parallel bus. The sensor has an internal ISP with a complete image processing pipeline including control loops. However, this driver uses the sensor in raw mode and the entire ISP is bypassed. Signed-off-by: Matthias Fend <matthias.fend@emfend.at> --- MAINTAINERS | 8 + drivers/media/i2c/Kconfig | 10 + drivers/media/i2c/Makefile | 1 + drivers/media/i2c/hm1246.c | 1427 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 1446 insertions(+)...quoted
+#define FLIP_FORMAT_INDEX(v, h) ((v ? 2 : 0) | (h ? 1 : 0)) + +/* Get the format code of the mode considering current flip setting. */ +static u32 hm1246_get_format_code(struct hm1246 *hm1246, + const struct hm1246_mode *hm1246_mode) +{ + return hm1246_mode->codes[FLIP_FORMAT_INDEX(hm1246->vflip_ctrl->val, + hm1246->hflip_ctrl->val)]; +} + +static const struct hm1246_mode hm1246_modes[] = { + { + .codes = { + [FLIP_FORMAT_INDEX(0, 0)] = MEDIA_BUS_FMT_SBGGR10_1X10, + [FLIP_FORMAT_INDEX(0, 1)] = MEDIA_BUS_FMT_SGBRG10_1X10, + [FLIP_FORMAT_INDEX(1, 0)] = MEDIA_BUS_FMT_SGRBG10_1X10, + [FLIP_FORMAT_INDEX(1, 1)] = MEDIA_BUS_FMT_SRGGB10_1X10,Instead of defining four mbus codes per mode, you can keep a single global Bayer-format table and derive the active code based on the H/V flip controls. This makes the code simpler and easier to maintain. You can refer to the imx319 or imx219 driver for implementation.I copied it from there at first. But since I didn't like the implied assignment (calculated index but required order is only a comment), I decided on this explicit implementation. So that is at least the intention behind the current code. Do you seed that as an issue?
I didn’t find any issues with the current implementation. However, if additional modes are added later, we would need to define four mbus codes for each, which leads to some repetition and slightly longer code. Sakari, what would you consider the preferred approach in this case?
quoted
quoted
+ }, + .link_freq_index = 0, + .clocks_per_pixel = 1, + .top = 0, + .left = 0, + .width = 1296, + .height = 976, + .hts = 1420, + .vts_min = 990, + .reg_list = { + .num_of_regs = ARRAY_SIZE(mode_1296x976_raw), + .regs = mode_1296x976_raw, + }, + }, +};...quoted
+static int hm1246_update_controls(struct hm1246 *hm1246, + const struct hm1246_mode *mode) +{ + s64 pixel_rate, exposure_max, vblank, hblank; + int ret; + + ret = __v4l2_ctrl_s_ctrl(hm1246->link_freq_ctrl, mode->link_freq_index); + if (ret) { + dev_err(hm1246->dev, "link_freq ctrl range update failed\n"); + return ret; + } + + pixel_rate = div_u64(hm1246_link_freqs[mode->link_freq_index], + mode->clocks_per_pixel); + ret = __v4l2_ctrl_modify_range(hm1246->pixel_rate_ctrl, pixel_rate, + pixel_rate, 1, pixel_rate); + if (ret) { + dev_err(hm1246->dev, "pixel_rate ctrl range update failed\n"); + return ret; + } + + vblank = mode->vts_min - mode->height,With the comma, the driver won’t compile.Huh. That actually compiles without errors. But you're right, of course, that it's not intended to be that way.quoted
quoted
+ ret = __v4l2_ctrl_modify_range(hm1246->vblank_ctrl, vblank, + HM1246_VTS_MAX - mode->height, 1, + vblank); + if (ret) { + dev_err(hm1246->dev, "vblank ctrl range update failed\n"); + return ret; + } + + hblank = mode->hts - mode->width; + ret = __v4l2_ctrl_modify_range(hm1246->hblank_ctrl, hblank, hblank, 1, + hblank); + if (ret) { + dev_err(hm1246->dev, "hblank ctrl range update failed\n"); + return ret; + } + + exposure_max = mode->vts_min - HM1246_COARSE_INTG_MARGIN; + ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl, + HM1246_COARSE_INTG_MIN, exposure_max, + HM1246_COARSE_INTG_STEP, exposure_max); + if (ret) { + dev_err(hm1246->dev, "exposure ctrl range update failed\n"); + return ret; + } + + return 0; +}...quoted
+static int hm1246_calc_pll(struct hm1246 *hm1246, u32 xclk, u32 link_freq, + u32 clocks_per_pixel, u8 *pll1, u8 *pll2, u8 *pll3) +{ + const u8 pclk_div_table[] = { 4, 5, 6, 7, 8, 12, 14, 16 }; + const u8 sysclk_div_table[] = { 1, 2, 3, 4 }; + const u8 post_div_table[] = { 1, 2, 4, 8 }; + const int sysclk_pclk_ratio = 3; /* Recommended value */ + u32 pclk, vco_out, best_vco_diff; + int pclk_div_index, sysclk_div_index, post_div_index; + u8 pre_div = 0, multiplier_h = 0, multiplier_l = 0; + bool sysclk_pclk_ratio_found = false; + + if (link_freq < HM1246_PCLK_MIN || link_freq > HM1246_PCLK_MAX) + return -EINVAL; + + /* + * In raw mode (1 pixel per clock) the pixel clock is internally + * divided by two. + */ + pclk = (2 * link_freq) / clocks_per_pixel; + + /* Find suitable PCLK and SYSCLK dividers. */ + for (pclk_div_index = 0; pclk_div_index < ARRAY_SIZE(pclk_div_table); + pclk_div_index++) { + for (sysclk_div_index = 0; + sysclk_div_index < ARRAY_SIZE(sysclk_div_table); + sysclk_div_index++) { + if (sysclk_div_table[sysclk_div_index] * + sysclk_pclk_ratio == + pclk_div_table[pclk_div_index]) { + sysclk_pclk_ratio_found = true; + break; + } + } + if (sysclk_pclk_ratio_found) + break; + } + + if (!sysclk_pclk_ratio_found) + return -EINVAL; + + /* Determine an appropriate post divider. */ + for (post_div_index = 0; post_div_index < ARRAY_SIZE(post_div_table); + post_div_index++) { + vco_out = pclk * (pclk_div_table[pclk_div_index] * + post_div_table[post_div_index]); + + if (vco_out >= HM1246_PLL_VCO_MIN && + vco_out <= HM1246_PLL_VCO_MAX) + break; + } + if (post_div_index >= ARRAY_SIZE(post_div_table)) + return -EINVAL; + + /* Find best pre-divider and multiplier values. */ + best_vco_diff = U32_MAX; + for (u32 div = DIV_ROUND_UP(xclk, HM1246_PLL_INCLK_MAX); + div <= (xclk / HM1246_PLL_INCLK_MIN); div++) { + u32 multi, multi_h, multi_l, vco, diff; + + multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk); + if (multi < HM1246_PLL_MULTI_MIN || + multi > HM1246_PLL_MULTI_MAX) + continue; + + multi_h = multi / (HM1246_PLL_MULTI_H_MIN * + HM1246_PLL_MULTI_L_MAX) + + 2; + multi_l = multi / multi_h; + vco = div_u64((u64)xclk * multi_h * multi_l, div); + + diff = abs(vco_out - vco);Here vco_out is always higher than vco?? because vco_out & vco are u32. abs is for signed int so if vco is higher than vco_out abs() can misbehave on wrap.No, the difference can also be negative. Although this works on my test system at least, it is of course better to replace abs() with abs_diff() here.quoted
quoted
+ if (diff < best_vco_diff) { + best_vco_diff = diff; + pre_div = div; + multiplier_h = multi_h; + multiplier_l = multi_l; + } + + if (!diff) + break; + } + + if (best_vco_diff == U32_MAX) + return -EINVAL; + + *pll1 = HM1246_PLL1CFG_MULTIPLIER(multiplier_l - 1); + *pll2 = HM1246_PLL2CFG_PRE_DIV(pre_div - 1) | + HM1246_PLL2CFG_MULTIPLIER(multiplier_h - 2); + *pll3 = HM1246_PLL3CFG_POST_DIV(post_div_index) | + HM1246_PLL3CFG_SYSCLK_DIV(sysclk_div_index) | + HM1246_PLL3CFG_PCLK_DIV(pclk_div_index); + + return 0; +}...quoted
+static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl) +{ + struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246, + ctrls); + struct v4l2_subdev_state *state; + const struct v4l2_mbus_framefmt *format; + const struct hm1246_mode *mode; + u32 val; + bool needs_cmu_update = false;I think you missed my last response here.Yes, I missed that. I will invert the logic accordingly.quoted
quoted
+ int ret = 0; + + state = v4l2_subdev_get_locked_active_state(&hm1246->sd); + format = v4l2_subdev_state_get_format(state, 0); + mode = v4l2_find_nearest_size(hm1246_modes, ARRAY_SIZE(hm1246_modes), + width, height, format->width, + format->height);No need for mode. In vblank you can directly use format->height.quoted
+ + if (ctrl->id == V4L2_CID_VBLANK) { + s64 exposure_max; + + exposure_max = + format->height + ctrl->val - HM1246_COARSE_INTG_MARGIN; + ret = __v4l2_ctrl_modify_range(hm1246->exposure_ctrl, + hm1246->exposure_ctrl->minimum, + exposure_max, + hm1246->exposure_ctrl->step, + exposure_max); + + if (ret) { + dev_err(hm1246->dev, "exposure ctrl range update failed\n"); + return ret; + } + } + + if (!pm_runtime_get_if_active(hm1246->dev)) + return 0; + + switch (ctrl->id) { + case V4L2_CID_EXPOSURE: + cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val, + &ret); + needs_cmu_update = true; + break; + + case V4L2_CID_ANALOGUE_GAIN: + cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG, + ctrl->val, &ret); + needs_cmu_update = true; + break; + + case V4L2_CID_VBLANK: + val = mode->height + ctrl->val; + cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val, + &ret); + needs_cmu_update = true; + break; + + case V4L2_CID_HFLIP: + case V4L2_CID_VFLIP: + val = 0; + if (hm1246->hflip_ctrl->val) + val |= HM1246_IMAGE_ORIENTATION_HFLIP; + if (hm1246->vflip_ctrl->val) + val |= HM1246_IMAGE_ORIENTATION_VFLIP; + + cci_write(hm1246->regmap, HM1246_IMAGE_ORIENTATION_REG, val, + &ret); + needs_cmu_update = true; + break; + + case V4L2_CID_TEST_PATTERN: + ret = hm1246_test_pattern(hm1246, ctrl->val); + break; + + default: + ret = -EINVAL; + break; + } + + if (needs_cmu_update) + cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret); + + pm_runtime_put(hm1246->dev); + + return ret; +}...quoted
+static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd, + struct v4l2_dbg_register *reg) +{ + struct hm1246 *hm1246 = to_hm1246(sd); + u64 val; + int ret; + + if (!pm_runtime_get_if_in_use(hm1246->dev)) + return 0; + + ret = cci_read(hm1246->regmap, CCI_REG8(reg->reg), &val, NULL);Many of your registers are 16-bit, so is it fine to use CCI_REG8? I think you should use simple regmap_read.Yes, that's fine. Even if values are distributed across multiple 8-bit registers, they can be addressed individually.
Agreed. Even when using regmap_read, we would still be accessing the individual 8-bit registers separately, so the current approach is valid. You're right. Best Regards, Tarang