* [RFC 0/3] ACPI, PMICs and probing cameras
@ 2021-08-19 20:19 Sakari Ailus
2021-08-19 20:19 ` [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) Sakari Ailus
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Sakari Ailus @ 2021-08-19 20:19 UTC (permalink / raw)
To: linux-acpi, linux-media; +Cc: andriy.shevchenko, rafael
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
^ permalink raw reply [flat|nested] 10+ messages in thread* [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) 2021-08-19 20:19 [RFC 0/3] ACPI, PMICs and probing cameras Sakari Ailus @ 2021-08-19 20:19 ` Sakari Ailus 2021-08-19 21:27 ` Laurent Pinchart 2021-08-20 12:20 ` Andy Shevchenko 2021-08-19 20:19 ` [RFC 2/3] gpio-tps68470: Allow building as module Sakari Ailus 2021-08-19 20:19 ` [RFC 3/3] gpio-tps68470: Add modalias Sakari Ailus 2 siblings, 2 replies; 10+ messages in thread From: Sakari Ailus @ 2021-08-19 20:19 UTC (permalink / raw) To: linux-acpi, linux-media; +Cc: andriy.shevchenko, rafael 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) 2021-08-19 20:19 ` [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) Sakari Ailus @ 2021-08-19 21:27 ` Laurent Pinchart 2021-08-24 15:51 ` Sakari Ailus 2021-08-20 12:20 ` Andy Shevchenko 1 sibling, 1 reply; 10+ messages in thread From: Laurent Pinchart @ 2021-08-19 21:27 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-acpi, linux-media, andriy.shevchenko, rafael 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) 2021-08-19 21:27 ` Laurent Pinchart @ 2021-08-24 15:51 ` Sakari Ailus 0 siblings, 0 replies; 10+ messages in thread From: Sakari Ailus @ 2021-08-24 15:51 UTC (permalink / raw) To: Laurent Pinchart; +Cc: linux-acpi, linux-media, andriy.shevchenko, rafael 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) 2021-08-19 20:19 ` [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) Sakari Ailus 2021-08-19 21:27 ` Laurent Pinchart @ 2021-08-20 12:20 ` Andy Shevchenko 2021-08-20 12:28 ` Sakari Ailus 1 sibling, 1 reply; 10+ messages in thread From: Andy Shevchenko @ 2021-08-20 12:20 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) 2021-08-20 12:20 ` Andy Shevchenko @ 2021-08-20 12:28 ` Sakari Ailus 0 siblings, 0 replies; 10+ messages in thread From: Sakari Ailus @ 2021-08-20 12:28 UTC (permalink / raw) To: Andy Shevchenko; +Cc: linux-acpi, linux-media, rafael 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC 2/3] gpio-tps68470: Allow building as module 2021-08-19 20:19 [RFC 0/3] ACPI, PMICs and probing cameras Sakari Ailus 2021-08-19 20:19 ` [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) Sakari Ailus @ 2021-08-19 20:19 ` Sakari Ailus 2021-08-20 12:22 ` Andy Shevchenko 2021-08-19 20:19 ` [RFC 3/3] gpio-tps68470: Add modalias Sakari Ailus 2 siblings, 1 reply; 10+ messages in thread From: Sakari Ailus @ 2021-08-19 20:19 UTC (permalink / raw) To: linux-acpi, linux-media; +Cc: andriy.shevchenko, rafael 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 2/3] gpio-tps68470: Allow building as module 2021-08-19 20:19 ` [RFC 2/3] gpio-tps68470: Allow building as module Sakari Ailus @ 2021-08-20 12:22 ` Andy Shevchenko 0 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2021-08-20 12:22 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [RFC 3/3] gpio-tps68470: Add modalias 2021-08-19 20:19 [RFC 0/3] ACPI, PMICs and probing cameras Sakari Ailus 2021-08-19 20:19 ` [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) Sakari Ailus 2021-08-19 20:19 ` [RFC 2/3] gpio-tps68470: Allow building as module Sakari Ailus @ 2021-08-19 20:19 ` Sakari Ailus 2021-08-20 12:24 ` Andy Shevchenko 2 siblings, 1 reply; 10+ messages in thread From: Sakari Ailus @ 2021-08-19 20:19 UTC (permalink / raw) To: linux-acpi, linux-media; +Cc: andriy.shevchenko, rafael 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC 3/3] gpio-tps68470: Add modalias 2021-08-19 20:19 ` [RFC 3/3] gpio-tps68470: Add modalias Sakari Ailus @ 2021-08-20 12:24 ` Andy Shevchenko 0 siblings, 0 replies; 10+ messages in thread From: Andy Shevchenko @ 2021-08-20 12:24 UTC (permalink / raw) To: Sakari Ailus; +Cc: linux-acpi, linux-media, rafael 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2021-08-24 15:51 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-19 20:19 [RFC 0/3] ACPI, PMICs and probing cameras Sakari Ailus 2021-08-19 20:19 ` [RFC 1/3] imx258: Defer probing on ident register read fail (on ACPI) Sakari Ailus 2021-08-19 21:27 ` Laurent Pinchart 2021-08-24 15:51 ` Sakari Ailus 2021-08-20 12:20 ` Andy Shevchenko 2021-08-20 12:28 ` Sakari Ailus 2021-08-19 20:19 ` [RFC 2/3] gpio-tps68470: Allow building as module Sakari Ailus 2021-08-20 12:22 ` Andy Shevchenko 2021-08-19 20:19 ` [RFC 3/3] gpio-tps68470: Add modalias Sakari Ailus 2021-08-20 12:24 ` Andy Shevchenko
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.