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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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