linux-kernel - Re: [PATCH v4] media: i2c: Add OV05C10 camera 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: <20250715181358.GI20231@pendragon.ideasonboard.com>
Date: Tue, 15 Jul 2025 21:13:58 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Bryan O'Donoghue <bod@...nel.org>
Cc: Pratap Nirujogi <pratap.nirujogi@....com>, mchehab@...nel.org,
	sakari.ailus@...ux.intel.com, kieran.bingham@...asonboard.com,
	hao.yao@...el.com, mehdi.djait@...ux.intel.com,
	dongcheng.yan@...ux.intel.com, hverkuil@...all.nl, krzk@...nel.org,
	dave.stevenson@...pberrypi.com, hdegoede@...hat.com,
	jai.luthra@...asonboard.com, tomi.valkeinen@...asonboard.com,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	benjamin.chan@....com, bin.du@....com, grosikop@....com,
	king.li@....com, dantony@....com, vengutta@....com,
	Phil.Jawich@....com
Subject: Re: [PATCH v4] media: i2c: Add OV05C10 camera sensor driver

On Tue, Jul 15, 2025 at 12:54:30PM +0100, Bryan O'Donoghue wrote:
> On 14/07/2025 21:51, Pratap Nirujogi wrote:
> 
> > +	ret = ov05c10_init_controls(ov05c10);
> > +	if (ret) {
> > +		dev_err(ov05c10->dev, "fail to init ov05c10 ctl %d\n", ret);
> > +		goto err_pm;
> > +	}
> 
> I would expect to see an "identify_module()" function here, something 
> similar to ov02c10.
> 
> ret = ov02c10_power_on(&client->dev);
> if (ret) {
>          dev_err_probe(&client->dev, ret, "failed to power on\n");
>          return ret;
> }
> 
> ret = ov02c10_identify_module(ov02c10);
> if (ret) {
>         dev_err(&client->dev, "failed to find sensor: %d", ret);
>          goto probe_error_power_off;
> }
> 
> ret = ov02c10_init_controls(ov02c10);
> if (ret) {
>         dev_err(&client->dev, "failed to init controls: %d", ret);
>          goto probe_error_v4l2_ctrl_handler_free;
> }
> 
> Standard practice is to try to talk to the sensor in probe() and bug out 
> if you can't.

It's actually not that standard, and is a frowned upon behaviour when
the sensor has a privacy LED GPIO connected to the power rail instead of
a hardware streaming signal. It would cause the privacy GPIO to flash at
boot time, which is considered a worrying behaviour for users. That's
why a few sensor drivers make runtime identification optional. We should
try to handle that in a standard way across all drivers, likely based on
a device property..

> With your current logic, the first time you'd realise no sensor was 
> present or is in reset etc is the first time you try to stream I think..
> 
> Definitely a good idea to probe for your sensor in probe failing the 
> probe if you can't find the hardware.

-- 
Regards,

Laurent Pinchart