Re: [PATCH v4] media: i2c: Add OV05C10 camera sensor driver

[Nirujogi, Pratap]

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