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] [day] [month] [year] [list]

Message-ID: <2d9036bb-8a90-421e-b612-ed5a112c63d9@amd.com>
Date: Wed, 16 Jul 2025 14:00:36 -0400
From: "Nirujogi, Pratap" <pnirujog@....com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
 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

Hi Laurent, Hi Bryan,

On 7/15/2025 2:13 PM, Laurent Pinchart wrote:
> Caution: This message originated from an External Source. Use proper caution when opening attachments, clicking links, or responding.
> 
> 
> 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..
> 
Shall I add chip_id as the device property variable in the x86/platform 
driver to address this comment? If this is okay, I will add a chip_id 
check reading the property variable in sensor probe(). Please share your 
thoughts.

Thanks,
Pratap


>> 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