| Message ID | 20210819201936.7390-2-sakari.ailus@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | ACPI, PMICs and probing cameras | expand |
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. > + } > if (ret) { > dev_err(&client->dev, "failed to read chip id %x\n", > IMX258_CHIP_ID);
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. ... > + if (ret == -EIO && is_acpi_device_node(dev_fwnode(&client->dev))) { has_acpi_companion() is better to have here, no? > + /* > + * 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; > + } -- With Best Regards, Andy Shevchenko
On Fri, Aug 20, 2021 at 03:20:55PM +0300, Andy Shevchenko wrote: > 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. > > ... > > > + if (ret == -EIO && is_acpi_device_node(dev_fwnode(&client->dev))) { > > has_acpi_companion() is better to have here, no? Agreed. -- Sakari Ailus
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
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; + } if (ret) { dev_err(&client->dev, "failed to read chip id %x\n", IMX258_CHIP_ID);
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(+)