| 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: <aQn_lguAdP-ZwCzK@smile.fi.intel.com>
Date: Tue, 4 Nov 2025 15:28:54 +0200
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Matthias Fend <matthias.fend@...end.at>
Cc: 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>,
Tarang Raval <tarang.raval@...iconsignals.io>,
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>,
Mehdi Djait <mehdi.djait@...ux.intel.com>,
Vladimir Zapolskiy <vladimir.zapolskiy@...aro.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Hardevsinh Palaniya <hardevsinh.palaniya@...iconsignals.io>,
linux-media@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Hao Yao <hao.yao@...el.com>,
bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH v5 2/2] media: i2c: add Himax HM1246 image sensor driver
On Tue, Nov 04, 2025 at 11:31:34AM +0100, Matthias Fend wrote:
> 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.
...
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/module.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/units.h>
This block is semi-random.
First of all, no new code must use gpio.h, use the proper one.
Second, many are missing, e.g., bits.h, regmap.h, types.h.
Please, follow IWYU principle (Include What You Use).
...
> +static inline struct hm1246 *to_hm1246(struct v4l2_subdev *sd)
> +{
> + return container_of_const(sd, struct hm1246, sd);
It's unclear and confusing that _const() variant is used here.
Either const qualifier is missed somewhere, or _const is redundant.
> +}
...
> + /*
> + * XSHUTDOWN to crystal clock oscillation: tcrystal typ. 650us
> + * Sample bootstrap pin: tsample max. 2000us
> + * Built in self test: tbist max. 3000us
> + */
> + fsleep(6000);
Also possible to write as 6 * USEC_PER_MSEC
...
> + 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);
> +
> + switch (sel->target) {
> + case V4L2_SEL_TGT_CROP:
> + sel->r = *v4l2_subdev_state_get_crop(state, 0);
> + return 0;
> +
> + case V4L2_SEL_TGT_NATIVE_SIZE:
> + sel->r.top = 0;
> + sel->r.left = 0;
> + sel->r.width = HM1246_NATIVE_WIDTH;
> + sel->r.height = HM1246_NATIVE_HEIGHT;
> + return 0;
> +
> + case V4L2_SEL_TGT_CROP_DEFAULT:
> + case V4L2_SEL_TGT_CROP_BOUNDS:
> + sel->r.top = mode->top;
> + sel->r.left = mode->left;
> + sel->r.width = mode->width;
> + sel->r.height = mode->height;
> + return 0;
> +
> + default:
> + return -EINVAL;
> + }
> + return 0;
Do we need this line?
...
> +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;
> +
> + 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])
> + goto sysclk_pclk_ratio_found;
> + }
> + }
And instead of this goto mess, factor out to a helper.
> + return -EINVAL;
> +sysclk_pclk_ratio_found:
> +
> + /* 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_diff(vco_out, vco);
> +
> + 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_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
> + u8 pll3)
> +{
> + const struct cci_reg_sequence pll_regs[] = {
static ?
> + { HM1246_PLL1CFG_REG, pll1 },
> + { HM1246_PLL2CFG_REG, pll2 },
> + { HM1246_PLL3CFG_REG, pll3 },
> + { HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
> + };
I would even move it outside the function. Note, static const maybe located in
ro memory while w/o static it's just a guarantee that compiler doesn't change
the values. Hence there is no guarantee it will be in ro memory.
> + return cci_multi_reg_write(hm1246->regmap, pll_regs,
> + ARRAY_SIZE(pll_regs), NULL);
> +}
> +
> +static int hm1246_pll_check_locked(struct hm1246 *hm1246)
> +{
> + u64 boot_ref2;
> + int ret;
> +
> + ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2,
> + NULL);
Despite being longer 80 I still would put it on one line. It will increase readability.
ret = cci_read(hm1246->regmap, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
Another option is to define local regmap:
struct regmap *map = hm1246->regmap;
...
ret = cci_read(map, HM1246_SBC_BOOT_REF2_REG, &boot_ref2, NULL);
which will be most readable and satisfy 80 limit.
> + if (ret)
> + return ret;
> +
> + return (boot_ref2 & HM1246_SBC_BOOT_REF2_PLL_LOCK) ? 0 : -EIO;
Think about similar improvements elsewhere in this driver.
> +}
...
> + /* PLL lock time: tpll typ. 100us */
It's not a variable name, use proper English.
> + fsleep(200);
...
> +static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
> + u16 g, u16 b)
Use logical split.
static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode,
u16 r, u16 g, u16 b)
This applies to other similar places in the code.
...
> +static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
> +{
> + const u16 RGBMIN = 0x0, RGBMAX = 0x3ff;
0 is enough (no need 0x).
So, the MAX is 10-bit, Can we use rather (BIT(10) - 1) to show this?
> + const struct tp {
> + int pattern;
> + u16 r, g, b;
> + } tps[] = {
> + /* 0 - Disabled */
Instead of indices in the comment, make the code robust
> + { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
[0] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
It even fit 80 characters.
> + /* 1 - Checkboard pattern */
> + { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* 2 - Ramp */
> + { .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* 3 - Moving ones */
> + { .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* 4 - Blending color bars */
> + { .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* 5 - Color bars */
> + { .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* 6 - Solid white */
> + { .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
> + /* 7 - Solid black */
> + { .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* 8 - Solid red */
> + { .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
> + /* 9 - Solid green */
> + { .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
> + /* 10- Solid blue */
> + { .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMAX },
> + };
> + u8 mode;
> +
> + if (index >= ARRAY_SIZE(tps))
> + return -EINVAL;
> +
> + mode = HM1246_TEST_PATTERN_MODE_MODE(tps[index].pattern);
> + if (index)
> + mode |= HM1246_TEST_PATTERN_MODE_ENABLE;
> +
> + return hm1246_cci_write_test_pattern(hm1246, mode, tps[index].r,
> + tps[index].g, tps[index].b);
> +}
...
> +static int hm1246_set_ctrl(struct v4l2_ctrl *ctrl)
> +{
> + struct hm1246 *hm1246 = container_of_const(ctrl->handler, struct hm1246,
> + ctrls);
Why _const()?
Why not split it between lines like:
struct hm1246 *hm1246 =
container_of_const(ctrl->handler, struct hm1246, ctrls);
> + struct v4l2_subdev_state *state;
> + const struct v4l2_mbus_framefmt *format;
> + u32 val;
> + bool needs_cmu_update = true;
> + int ret = 0;
> +
> + state = v4l2_subdev_get_locked_active_state(&hm1246->sd);
> + format = v4l2_subdev_state_get_format(state, 0);
> +
> + 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;
Use ACQUIRE() and return directly where it makes sense.
> + switch (ctrl->id) {
> + case V4L2_CID_EXPOSURE:
> + cci_write(hm1246->regmap, HM1246_COARSE_INTG_REG, ctrl->val,
> + &ret);
> + break;
> +
> + case V4L2_CID_ANALOGUE_GAIN:
> + cci_write(hm1246->regmap, HM1246_ANALOG_GLOBAL_GAIN_REG,
> + ctrl->val, &ret);
> + break;
> +
> + case V4L2_CID_VBLANK:
> + val = format->height + ctrl->val;
> + cci_write(hm1246->regmap, HM1246_FRAME_LENGTH_LINES_REG, val,
> + &ret);
> + 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);
> + break;
> +
> + case V4L2_CID_TEST_PATTERN:
> + ret = hm1246_test_pattern(hm1246, ctrl->val);
> + needs_cmu_update = false;
Like here, and you won't need needs_cmu_update anymore.
> + break;
> +
> + default:
> + ret = -EINVAL;
> + needs_cmu_update = false;
> + break;
> + }
> +
> + if (needs_cmu_update)
> + cci_write(hm1246->regmap, HM1246_CMU_UPDATE_REG, 0, &ret);
> +
> + pm_runtime_put(hm1246->dev);
> +
> + return ret;
> +}
...
> +static int hm1246_identify_module(struct hm1246 *hm1246)
This is a singleton function, right?
Check what once.h (or even once_lite.h) provides for you for such a case,
and drop unneeded "identified" variable.
> +{
> + u64 model_id;
> + int ret;
> +
> + if (hm1246->identified)
> + return 0;
> +
> + ret = cci_read(hm1246->regmap, HM1246_MODEL_ID_REG, &model_id, NULL);
> + if (ret)
> + return ret;
> +
> + if (model_id != HM1246_MODEL_ID) {
> + dev_err(hm1246->dev, "model id mismatch: 0x%llx!=0x%x\n",
> + model_id, HM1246_MODEL_ID);
> + return -ENXIO;
> + }
> +
> + hm1246->identified = true;
> +
> + return 0;
> +}
...
> +static int __maybe_unused hm1246_g_register(struct v4l2_subdev *sd,
> + struct v4l2_dbg_register *reg)
If v4l2.h doesn't provide a ptr_*() macro for these cases, I recommend to add and drop these __maybe_unused.
> +{
> + 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);
> + reg->val = val;
> +
> + pm_runtime_put(hm1246->dev);
> +
> + return ret;
> +}
...
> + ret = cci_write(hm1246->regmap, CCI_REG8(reg->reg), (u64)reg->val,
> + NULL);
Do you need casting?
...
> + endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(hm1246->dev), 0,
> + 0,
> + FWNODE_GRAPH_ENDPOINT_NEXT);
Better split is
endpoint = fwnode_graph_get_endpoint_by_id(dev_fwnode(hm1246->dev),
0, 0,
FWNODE_GRAPH_ENDPOINT_NEXT);
> + if (!endpoint)
> + return dev_err_probe(hm1246->dev, -EINVAL,
> + "missing endpoint node\n");
...
> +static DEFINE_RUNTIME_DEV_PM_OPS(hm1246_pm_ops, hm1246_power_off,
> + hm1246_power_on, NULL);
Use logical split:
static DEFINE_RUNTIME_DEV_PM_OPS(hm1246_pm_ops,
hm1246_power_off, hm1246_power_on, NULL);
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists