Re: [PATCH v3 2/2] media: i2c: add Himax HM1246 image sensor driver
From: Tarang Raval <hidden>
Date: 2025-09-13 09:11:56
Also in:
linux-media, lkml
Hi Matthias,
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(+)
...
+#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.
+ },
+ .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,
+ },
+ },
+};...
+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.
+ 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;
+}...
+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.
+ 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;
+}...
+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.
+ 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.
+
+ 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;
+}...
+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.
+ reg->val = val;
+
+ pm_runtime_put(hm1246->dev);
+
+ return ret;
+}
+
+static int __maybe_unused hm1246_s_register(struct v4l2_subdev *sd,
+ const struct v4l2_dbg_register *reg)
+{
+ struct hm1246 *hm1246 = to_hm1246(sd);
+ int ret;
+
+ if (!pm_runtime_get_if_in_use(hm1246->dev))
+ return 0;
+
+ ret = cci_write(hm1246->regmap, CCI_REG8(reg->reg), (u64)reg->val,
+ NULL);same here.
+
+ pm_runtime_put(hm1246->dev);
+
+ return ret;
+}
+
+static const struct v4l2_subdev_core_ops hm1246_core_ops = {
+#ifdef CONFIG_VIDEO_ADV_DEBUG
+ .g_register = hm1246_g_register,
+ .s_register = hm1246_s_register,
+#endif
+};...
+static int hm1246_probe(struct i2c_client *client)
+{
+ struct hm1246 *hm1246;
+ int ret;
+
+ hm1246 = devm_kzalloc(&client->dev, sizeof(*hm1246), GFP_KERNEL);
+ if (!hm1246)
+ return -ENOMEM;
+
+ hm1246->dev = &client->dev;
+
+ ret = hm1246_parse_fwnode(hm1246);
+ if (ret)
+ return ret;
+
+ hm1246->regmap = devm_cci_regmap_init_i2c(client, 16);
+ if (IS_ERR(hm1246->regmap))
+ return dev_err_probe(hm1246->dev, PTR_ERR(hm1246->regmap),
+ "failed to init CCI\n");
+
+ hm1246->xclk = devm_v4l2_sensor_clk_get(hm1246->dev, NULL);
+ if (IS_ERR(hm1246->xclk))
+ return dev_err_probe(hm1246->dev, PTR_ERR(hm1246->xclk),
+ "failed to get xclk\n");
+
+ hm1246->xclk_freq = clk_get_rate(hm1246->xclk);
+ if (hm1246->xclk_freq < HM1246_XCLK_MIN ||
+ hm1246->xclk_freq > HM1246_XCLK_MAX)
+ dev_err_probe(hm1246->dev, -EINVAL,
+ "xclk frequency out of range: %luHz\n",
+ hm1246->xclk_freq);return dev_err_probe(...)
+ + ret = hm1246_get_regulators(hm1246->dev, hm1246); + if (ret) + return dev_err_probe(hm1246->dev, ret, + "failed to get regulators\n"); + + hm1246->reset_gpio = + devm_gpiod_get_optional(hm1246->dev, "reset", GPIOD_OUT_HIGH); + if (IS_ERR(hm1246->reset_gpio)) + return dev_err_probe(hm1246->dev, ret,
error code ret ?? I think it should be PTR_ERR(hm1246->reset_gpio).
+ "failed to get reset GPIO\n");
+
+ v4l2_i2c_subdev_init(&hm1246->sd, client, &hm1246_subdev_ops);
+ hm1246->sd.internal_ops = &hm1246_internal_ops;
+
+ ret = hm1246_init_controls(hm1246);
+ if (ret)
+ return dev_err_probe(hm1246->dev, ret,
+ "failed to init controls\n");
+
+ hm1246->sd.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
+ hm1246->pad.flags = MEDIA_PAD_FL_SOURCE;
+ hm1246->sd.entity.function = MEDIA_ENT_F_CAM_SENSOR;
+
+ ret = media_entity_pads_init(&hm1246->sd.entity, 1, &hm1246->pad);
+ if (ret) {
+ dev_err_probe(hm1246->dev, ret, "failed to init media pads\n");
+ goto err_v4l2_ctrl_handler_free;
+ }
+
+ hm1246->sd.state_lock = hm1246->ctrls.lock;
+ ret = v4l2_subdev_init_finalize(&hm1246->sd);
+ if (ret) {
+ dev_err_probe(hm1246->dev, ret, "failed to init v4l2 subdev\n");
+ goto err_media_entity_cleanup;
+ }
+
+ pm_runtime_enable(hm1246->dev);
+ pm_runtime_set_autosuspend_delay(hm1246->dev, 1000);
+ pm_runtime_use_autosuspend(hm1246->dev);
+ pm_runtime_idle(hm1246->dev);
+
+ ret = v4l2_async_register_subdev_sensor(&hm1246->sd);
+ if (ret) {
+ dev_err_probe(hm1246->dev, ret,
+ "failed to register v4l2 subdev\n");
+ goto err_subdev_cleanup;
+ }
+
+ return 0;
+
+err_subdev_cleanup:
+ v4l2_subdev_cleanup(&hm1246->sd);
+ pm_runtime_disable(hm1246->dev);
+ pm_runtime_set_suspended(hm1246->dev);
+
+err_media_entity_cleanup:
+ media_entity_cleanup(&hm1246->sd.entity);
+
+err_v4l2_ctrl_handler_free:
+ v4l2_ctrl_handler_free(&hm1246->ctrls);
+
+ return ret;
+}...
+static struct i2c_driver hm1246_i2c_driver = {
+ .driver = {
+ .of_match_table = hm1246_of_match,
+ .pm = pm_ptr(&hm1246_pm_ops),
+ .name = "hm1246",
+ },
+ .probe = hm1246_probe,
+ .remove = hm1246_remove,
+};
+module_i2c_driver(hm1246_i2c_driver);
+
+MODULE_DESCRIPTION("Himax HM1246 camera driver");
+MODULE_AUTHOR("Matthias Fend [off-list ref]");
+MODULE_LICENSE("GPL");
--
2.34.1Best Regards, Tarang