diff mbox series

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

Message ID 20210819201936.7390-2-sakari.ailus@linux.intel.com (mailing list archive)
State New
Headers show
Series ACPI, PMICs and probing cameras | expand

Commit Message

Sakari Ailus Aug. 19, 2021, 8:19 p.m. UTC
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(+)

Comments

Laurent Pinchart Aug. 19, 2021, 9:27 p.m. UTC | #1
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);
Andy Shevchenko Aug. 20, 2021, 12:20 p.m. UTC | #2
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;
> +	}
Sakari Ailus Aug. 20, 2021, 12:28 p.m. UTC | #3
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 Aug. 24, 2021, 3:51 p.m. UTC | #4
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.
diff mbox series

Patch

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);