Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI)

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Laurent,

On Fri, Aug 20, 2021 at 12:27:23AM +0300, Laurent Pinchart wrote:
> Hi Sakari,
> 
> Thank you for the patch.
> 
> On Thu, Aug 19, 2021 at 11:19:34PM +0300, Sakari Ailus wrote:
> > Return -EPROBE_DEFER if probing the device fails because of the I²C
> > transaction (-EIO only). This generally happens when the power on sequence
> > of the device has not been fully performed yet due to later probing of
> > other drivers.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/imx258.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/media/i2c/imx258.c b/drivers/media/i2c/imx258.c
> > index c249507aa2db..2751c12b6029 100644
> > --- a/drivers/media/i2c/imx258.c
> > +++ b/drivers/media/i2c/imx258.c
> > @@ -1109,6 +1109,14 @@ static int imx258_identify_module(struct imx258 *imx258)
> >  
> >  	ret = imx258_read_reg(imx258, IMX258_REG_CHIP_ID,
> >  			      IMX258_REG_VALUE_16BIT, &val);
> > +	if (ret == -EIO && is_acpi_device_node(dev_fwnode(&client->dev))) {
> > +		/*
> > +		 * If we get -EIO here and it's an ACPI device, there's a fair
> > +		 * likelihood it's because the drivers required to power this
> > +		 * device on have not probed yet. Thus return -EPROBE_DEFER.
> > +		 */
> > +		return -EPROBE_DEFER;
> 
> That's really a hack :-( The driver shouldn't have to deal with this. If
> power management is handled transparently for the driver, which is
> what's meant to happen with ACPI, then it should be fully transparent.
> An -EIO error may mean a real communication issue, turning it into
> infinite probe deferring isn't right. The ACPI subsystem should figure
> this out and not probe the driver until all the required resources that
> are managed transparently for the driver are available.
> 
> If this was a one-off hack I may be able to pretend I haven't noticed,
> but this would need to be copied to every single sensor driver, even
> every single I2C device driver. It should be fixed properly in the ACPI
> subsystem instead.

In practice such communication issues are rare and trying an I²C access
isn't expensive. The patch does solve two practical issues, namely
correctly probing a driver and making it possible to build more things as
modules.

That said, I agree with with you that ideally a driver would know whether a
device has been fully powered up or not. There could also be adverse side
effects as there have been no such checks previously.

-- 
Regards,

Sakari Ailus