| 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: <aXqDdN2fHDlcYjhk@smile.fi.intel.com>
Date: Wed, 28 Jan 2026 23:45:24 +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>,
Svyatoslav Ryhel <clamor95@...il.com>,
Philipp Zabel <p.zabel@...gutronix.de>, linux-media@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Hao Yao <hao.yao@...el.com>,
Himanshu Bhavani <himanshu.bhavani@...iconsignals.io>
Subject: Re: [PATCH v9 2/2] media: i2c: add Himax HM1246 image sensor driver
On Wed, Jan 28, 2026 at 05:08:02PM +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.
I found no serious issues here, just a few nit-picks. FWIW,
Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
...
> +/* Clock setup registers */
> +#define HM1246_PLL1CFG_REG CCI_REG8(0x0303)
> +#define HM1246_PLL1CFG_MULTIPLIER(x) (((x) & 0xff) << 0)
> +#define HM1246_PLL2CFG_REG CCI_REG8(0x0305)
> +#define HM1246_PLL2CFG_PRE_DIV(x) (((x) & 0x1f) << 1)
> +#define HM1246_PLL2CFG_MULTIPLIER(x) (((x) & 0x01) << 0)
> +#define HM1246_PLL3CFG_REG CCI_REG8(0x0307)
> +#define HM1246_PLL3CFG_POST_DIV(x) (((x) & 0x3) << 6)
> +#define HM1246_PLL3CFG_SYSCLK_DIV(x) (((x) & 0x3) << 4)
> +#define HM1246_PLL3CFG_PCLK_DIV(x) (((x) & 0x7) << 0)
Is not using BIT*() and GENMASK*() in the above on purpose?
...
> +/* Test pattern registers */
> +#define HM1246_TEST_PATTERN_MODE_REG CCI_REG8(0x0601)
> +#define HM1246_TEST_PATTERN_MODE_MODE(x) (((x) & 0xf) << 4)
> +#define HM1246_TEST_PATTERN_MODE_ENABLE BIT(0)
Ditto.
...
> +#define HM1246_ANALOG_GLOBAL_GAIN_STEP 1
0x01 ? (Since the min and max are represented in hexadecimal form)
...
> +struct hm1246 {
> + struct device *dev;
> + struct v4l2_subdev sd;
> + struct media_pad pad;
Wondering if putting the embedded structs first (either sd or pad) gives a
better (smaller) object size. Sometimes this kind of shuffling makes
container_of() to be a compile-time no-op.
> + struct regulator_bulk_data supplies[ARRAY_SIZE(hm1246_supply_names)];
> + struct clk *xclk;
> + unsigned long xclk_freq;
> + struct reset_control *reset;
> + unsigned int mbus_flags;
> + s64 link_frequency;
> +
> + struct v4l2_ctrl_handler ctrls;
> + struct v4l2_ctrl *exposure_ctrl;
> + struct v4l2_ctrl *hflip_ctrl;
> + struct v4l2_ctrl *vflip_ctrl;
> +
> + struct regmap *regmap;
> +
> + bool identified;
> +};
...
> +static int hm1246_enum_frame_size(struct v4l2_subdev *subdev,
> + struct v4l2_subdev_state *sd_state,
> + struct v4l2_subdev_frame_size_enum *fse)
> +{
> + struct hm1246 *hm1246 = to_hm1246(subdev);
> + const struct hm1246_mode *mode;
> +
> + if (fse->index > 0)
> + return -EINVAL;
> +
> + mode = hm1246_find_mode_by_mbus_code(hm1246, fse->code);
> + if (!mode)
> + return -EINVAL;
Hmm... Is it common practice to return -EINVAL in such cases?
>From the context (not found) I would rather return -ENOENT.
> + fse->min_width = mode->rect.width;
> + fse->max_width = mode->rect.width;
> + fse->min_height = mode->rect.height;
> + fse->max_height = mode->rect.height;
> +
> + return 0;
> +}
...
> +static int hm1246_calc_pll(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 */
Why not marking them static? Note, in some cases compiler might decide to fill
up the values on the stack or put them as a direct constant in the register.
It might lead to suboptimal code.
> + u32 pclk, vco_out;
> + int pclk_div_index, sysclk_div_index, post_div_index;
> + bool sysclk_pclk_ratio_found = false;
We don't need this. See below how.
> + 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;
Drop.
> + break;
> + }
> + }
if (sysclk_div_index < ARRAY_SIZE(sysclk_div_table))
> + if (sysclk_pclk_ratio_found)
> + break;
> + }
if (pclk_div_index < ARRAY_SIZE(pclk_div_table))
> + 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 pre-divider and multiplier values. */
> + 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;
> + multi = DIV_ROUND_CLOSEST_ULL((u64)vco_out * div, xclk);
Can we define vco_out as u64 and drop the casting?
> + 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);
Similar Q for multi_h and multi_l.
> + if (vco != vco_out)
> + continue;
> +
> + if (pll1 && pll2 && pll3) {
> + *pll1 = HM1246_PLL1CFG_MULTIPLIER(multi_l - 1);
> + *pll2 = HM1246_PLL2CFG_PRE_DIV(div - 1) |
> + HM1246_PLL2CFG_MULTIPLIER(multi_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;
> + }
> +
> + return -EINVAL;
> +}
...
> +static int hm1246_cci_write_pll(struct hm1246 *hm1246, u8 pll1, u8 pll2,
> + u8 pll3)
I believe we can leave a prototype to be on a single line.
> +{
> + const struct cci_reg_sequence pll_regs[] = {
> + { HM1246_PLL1CFG_REG, pll1 },
> + { HM1246_PLL2CFG_REG, pll2 },
> + { HM1246_PLL3CFG_REG, pll3 },
> + { HM1246_SBC_CTRL_REG, HM1246_SBC_CTRL_PLL_EN },
> + };
> +
> + return cci_multi_reg_write(hm1246->regmap, pll_regs,
> + ARRAY_SIZE(pll_regs), NULL);
> +}
...
> +static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode, u16 r,
> + u16 g, u16 b)
More logical is to have them as
static int hm1246_cci_write_test_pattern(struct hm1246 *hm1246, u8 mode,
u16 r, u16 g, u16 b)
> +{
> + const struct cci_reg_sequence tpg_enable_regs[] = {
> + { HM1246_TEST_DATA_RED_REG, r },
> + { HM1246_TEST_DATA_GR_REG, g },
> + { HM1246_TEST_DATA_GB_REG, g },
> + { HM1246_TEST_DATA_BLUE_REG, b },
> + { HM1246_TEST_PATTERN_MODE_REG, mode },
> + };
> +
> + return cci_multi_reg_write(hm1246->regmap, tpg_enable_regs,
> + ARRAY_SIZE(tpg_enable_regs), NULL);
> +}
...
> +static int hm1246_test_pattern(struct hm1246 *hm1246, u32 index)
> +{
> + const u16 RGBMIN = 0, RGBMAX = 0x3ff;
> + const struct tp {
> + int pattern;
> + u16 r, g, b;
static const ... ?
> + } tps[] = {
> + /* Disabled */
> + [0] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Checkboard pattern */
> + [1] = { .pattern = 0, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Ramp */
> + [2] = { .pattern = 1, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Moving ones */
> + [3] = { .pattern = 2, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Blending color bars */
> + [4] = { .pattern = 3, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Color bars */
> + [5] = { .pattern = 4, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Solid white */
> + [6] = { .pattern = 15, .r = RGBMAX, .g = RGBMAX, .b = RGBMAX },
> + /* Solid black */
> + [7] = { .pattern = 15, .r = RGBMIN, .g = RGBMIN, .b = RGBMIN },
> + /* Solid red */
> + [8] = { .pattern = 15, .r = RGBMAX, .g = RGBMIN, .b = RGBMIN },
> + /* Solid green */
> + [9] = { .pattern = 15, .r = RGBMIN, .g = RGBMAX, .b = RGBMIN },
> + /* Solid blue */
> + [10] = { .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);
> + struct v4l2_subdev_state *state;
> + const struct v4l2_mbus_framefmt *format;
> + u32 val;
> + bool needs_cmu_update = true;
> + int ret = 0;
The assignments like this is fragile. Better to decouple and put it near to the first user.
> + 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) {
What if this is changed to return something positive? Or what if we will ignore
one of the error code in the future for some reason?
These questions are to justify the idea of decoupling the above assignment.
> + dev_err(hm1246->dev, "exposure ctrl range update failed\n");
> + return ret;
> + }
> + }
> + if (!pm_runtime_get_if_active(hm1246->dev))
> + return 0;
Wondering if we can use one of PM_ACQUIRE() macros here.
ret = 0;
> + 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;
> + 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;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists