linux-kernel - Re: [PATCH v7 2/2] media: i2c: add Himax HM1246 image sensor driver

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <f2e77bb5-957e-4751-8304-d9fb94927417@emfend.at>
Date: Tue, 13 Jan 2026 10:06:36 +0100
From: Matthias Fend <matthias.fend@...end.at>
To: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
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>, 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>,
 bsp-development.geo@...ca-geosystems.com
Subject: Re: [PATCH v7 2/2] media: i2c: add Himax HM1246 image sensor driver

Hi Andy,

thanks a lot for feedback.

Am 12.01.2026 um 20:01 schrieb Andy Shevchenko:
> On Mon, Jan 12, 2026 at 03:49:33PM +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.
> 
> ...
> 
>> +struct hm1246_mode {
>> +	u32 codes[4];
>> +	u32 clocks_per_pixel;
> 
>> +	u32 top;
>> +	u32 left;
>> +	u32 width;
>> +	u32 height;
> 
> Why not use struct v4l2_rect?

Valid question. I would save something in six places, but add something 
in about 27 others. Because of this ratio, I opted for the current way.

> 
>> +	u32 hts;
>> +	u32 vts_min;
>> +	const struct hm1246_reg_list reg_list;
>> +};
> 
> ...
> 
>> +static int hm1246_set_format(struct v4l2_subdev *sd,
>> +			     struct v4l2_subdev_state *state,
>> +			     struct v4l2_subdev_format *fmt)
>> +{
>> +	struct hm1246 *hm1246 = to_hm1246(sd);
>> +	struct v4l2_mbus_framefmt *mbus_fmt;
>> +	struct v4l2_rect *crop;
>> +	const struct hm1246_mode *mode;
>> +
>> +	mode = hm1246_find_mode_by_mbus_code(hm1246, fmt->format.code);
>> +	if (IS_ERR(mode))
>> +		mode = &hm1246_modes[0];
>> +
>> +	crop = v4l2_subdev_state_get_crop(state, 0);
> 
>> +	crop->top = mode->top;
>> +	crop->left = mode->left;
>> +	crop->width = mode->width;
>> +	crop->height = mode->height;
> 
> With the above done this becomes a one-liner:
> 
> 	*crop = mode.<rect>; // <rect> is whatever name for the embedded field
> 
>> +	hm1246_update_pad_format(hm1246, mode, &fmt->format);
>> +	mbus_fmt = v4l2_subdev_state_get_format(state, 0);
>> +	*mbus_fmt = fmt->format;
>> +
>> +	return 0;
>> +}
> 
> ...
> 
>> +static int hm1246_get_selection(struct v4l2_subdev *sd,
>> +				struct v4l2_subdev_state *state,
>> +				struct v4l2_subdev_selection *sel)
>> +{
>> +	const struct v4l2_mbus_framefmt *format;
>> +	const struct hm1246_mode *mode;
>> +
>> +	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;
> 
> Seems in the same way here.
> 
>> +		return 0;
>> +	}
> 
>> +	return -EINVAL;
> 
> Why not making it a default case?

I prefer it when the return statement is at the end of the function. Do 
you see a problem here?

> 
>> +}
> 
> ...
> 
>> +	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, PTR_ERR(hm1246->reset_gpio),
>> +				     "failed to get reset GPIO\n");
> 
> Can it be GPIO reset driver used instead? (Note, it's made agnostic now.)

That would probably be possible, but I currently don't see any advantage 
for I2C image sensors. If I understand correctly, you would first have 
to define a reset controller that could then be used in the sensor – 
instead of simply specifying the GPIO directly.
The advantage of being able to share the reset line with other 
components probably doesn't make sense for these sensors in most cases. 
That's perhaps also the reason why it hasn't been used before.

Maybe the media maintainers have an opinion on this?

Thanks
  ~Matthias

>