[RFC 0/3] ACPI, PMICs and probing cameras

[RFC 0/3] ACPI, PMICs and probing cameras

[Sakari Ailus]

Hi all,

The intel_skl_int3472 driver is required for some of the ACPI power state
transitions to function. This driver may be compiled as a module and thus
may probe after a sensor driver that depends on it (e.g. imx258).

Make the sensor driver return -EPROBE_DEFER if the sensor cannot be found.
This way the sensor driver will be probed when both the intel_skl_int3472
and the gpio-tps68470 drivers have successfully probed while also the
gpio-tps68470 driver may be built as a module.

The drawback of the approach is needless probing of the imx258 driver but
everything can be built as modules.

I'm posting this as RFC since the approach would require effectively all
ACPI based sensor drivers to have the same check. This would be a
non-issue to add. I wouldn't mind but...

Is there a way to figure out whether the PMIC opregion callbacks actually
succeed? At least the imx258 driver is happily probed even if the driver
implementing the opregion callback is not there, and thus the device
remains powered off --- and probe fails.

Sakari Ailus (3):
  imx258: Defer probing on ident register read fail (on ACPI)
  gpio-tps68470: Allow building as module
  gpio-tps68470: Add modalias

 drivers/gpio/Kconfig         | 2 +-
 drivers/gpio/gpio-tps68470.c | 5 ++++-
 drivers/media/i2c/imx258.c   | 8 ++++++++
 3 files changed, 13 insertions(+), 2 deletions(-)

-- 
2.30.2

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

[Sakari Ailus]

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;
+	}
 	if (ret) {
 		dev_err(&client->dev, "failed to read chip id %x\n",
 			IMX258_CHIP_ID);
-- 
2.30.2

[RFC 2/3] gpio-tps68470: Allow building as module

[Sakari Ailus]

Allow building gpio-tps68470 driver as a module. The intel_skl_int3472 is
a module anyway, so making this a builtin does not really help setting up
this one before a dependent module probes.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/gpio/Kconfig         | 2 +-
 drivers/gpio/gpio-tps68470.c | 4 +++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index fab571016adf..8911c2df97d1 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1378,7 +1378,7 @@ config GPIO_TPS65912
 	  This driver supports TPS65912 gpio chip
 
 config GPIO_TPS68470
-	bool "TPS68470 GPIO"
+	tristate "TPS68470 GPIO"
 	depends on INTEL_SKL_INT3472
 	help
 	  Select this option to enable GPIO driver for the TPS68470
diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
index 423b7bc30ae8..0ab88ef241de 100644
--- a/drivers/gpio/gpio-tps68470.c
+++ b/drivers/gpio/gpio-tps68470.c
@@ -155,4 +155,6 @@ static struct platform_driver tps68470_gpio_driver = {
 	.probe = tps68470_gpio_probe,
 };
 
-builtin_platform_driver(tps68470_gpio_driver)
+module_platform_driver(tps68470_gpio_driver);
+
+MODULE_LICENSE("GPL");
-- 
2.30.2

[RFC 3/3] gpio-tps68470: Add modalias

[Sakari Ailus]

Add modalias for this driver, so that it is loaded automatically once the
devices pop up.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/gpio/gpio-tps68470.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
index 0ab88ef241de..8afcd31f4ea3 100644
--- a/drivers/gpio/gpio-tps68470.c
+++ b/drivers/gpio/gpio-tps68470.c
@@ -158,3 +158,4 @@ static struct platform_driver tps68470_gpio_driver = {
 module_platform_driver(tps68470_gpio_driver);
 
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:tps68470-gpio");
-- 
2.30.2

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

[Laurent Pinchart]

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

-- 
Regards,

Laurent Pinchart

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

[Andy Shevchenko]

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

Re: [RFC 2/3] gpio-tps68470: Allow building as module

[Andy Shevchenko]

On Thu, Aug 19, 2021 at 11:19:35PM +0300, Sakari Ailus wrote:
> Allow building gpio-tps68470 driver as a module. The intel_skl_int3472 is
> a module anyway, so making this a builtin does not really help setting up
> this one before a dependent module probes.

Fine with me,
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/gpio/Kconfig         | 2 +-
>  drivers/gpio/gpio-tps68470.c | 4 +++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index fab571016adf..8911c2df97d1 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1378,7 +1378,7 @@ config GPIO_TPS65912
>  	  This driver supports TPS65912 gpio chip
>  
>  config GPIO_TPS68470
> -	bool "TPS68470 GPIO"
> +	tristate "TPS68470 GPIO"
>  	depends on INTEL_SKL_INT3472
>  	help
>  	  Select this option to enable GPIO driver for the TPS68470
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index 423b7bc30ae8..0ab88ef241de 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -155,4 +155,6 @@ static struct platform_driver tps68470_gpio_driver = {
>  	.probe = tps68470_gpio_probe,
>  };
>  
> -builtin_platform_driver(tps68470_gpio_driver)
> +module_platform_driver(tps68470_gpio_driver);
> +
> +MODULE_LICENSE("GPL");
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

Re: [RFC 3/3] gpio-tps68470: Add modalias

[Andy Shevchenko]

On Thu, Aug 19, 2021 at 11:19:36PM +0300, Sakari Ailus wrote:
> Add modalias for this driver, so that it is loaded automatically once the
> devices pop up.

Not sure if it should be a separate patch, nevertheless
Acked-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/gpio/gpio-tps68470.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/gpio/gpio-tps68470.c b/drivers/gpio/gpio-tps68470.c
> index 0ab88ef241de..8afcd31f4ea3 100644
> --- a/drivers/gpio/gpio-tps68470.c
> +++ b/drivers/gpio/gpio-tps68470.c
> @@ -158,3 +158,4 @@ static struct platform_driver tps68470_gpio_driver = {
>  module_platform_driver(tps68470_gpio_driver);
>  
>  MODULE_LICENSE("GPL");
> +MODULE_ALIAS("platform:tps68470-gpio");
> -- 
> 2.30.2
> 

-- 
With Best Regards,
Andy Shevchenko

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

[Sakari Ailus]

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

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

[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