| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID:
<PN3P287MB18290018941BE6A227431C818B0BA@PN3P287MB1829.INDP287.PROD.OUTLOOK.COM>
Date: Sat, 13 Sep 2025 09:11:52 +0000
From: Tarang Raval <tarang.raval@...iconsignals.io>
To: Matthias Fend <matthias.fend@...end.at>, Mauro Carvalho Chehab
<mchehab@...nel.org>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
<krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Hans Verkuil
<hverkuil@...nel.org>, Sakari Ailus <sakari.ailus@...ux.intel.com>, Hans de
Goede <hansg@...nel.org>, Ricardo Ribalda <ribalda@...omium.org>,
André Apitzsch <git@...tzsch.eu>, Andy Shevchenko
<andriy.shevchenko@...ux.intel.com>, Benjamin Mugnier
<benjamin.mugnier@...s.st.com>, Sylvain Petinot
<sylvain.petinot@...s.st.com>, Dongcheng Yan <dongcheng.yan@...el.com>, Bryan
O'Donoghue <bryan.odonoghue@...aro.org>, Alan Stern
<stern@...land.harvard.edu>, Jingjing Xiong <jingjing.xiong@...el.com>,
Heimir Thor Sverrisson <heimir.sverrisson@...il.com>
CC: "linux-media@...r.kernel.org" <linux-media@...r.kernel.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>, Hao Yao
<hao.yao@...el.com>, "bsp-development.geo@...ca-geosystems.com"
<bsp-development.geo@...ca-geosystems.com>
Subject: Re: [PATCH v3 2/2] media: i2c: add Himax HM1246 image sensor driver
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@...end.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 <matthias.fend@...end.at>");
> +MODULE_LICENSE("GPL");
>
> --
> 2.34.1
Best Regards,
Tarang
Powered by blists - more mailing lists