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