| Message ID | 20200901210333.8462-1-sakari.ailus@linux.intel.com |
|---|---|
| Headers | show |
| Series | Support running driver's probe for a device powered off | expand |
On Tue, Sep 1, 2020 at 11:03 PM Sakari Ailus <sakari.ailus@linux.intel.com> wrote: > > Document the probe-low-power _DSD property and how it is used with I²C > drivers. I would reorder the series to make this go right after the [1/6] or maybe even fold it into that patch. The point is that the changes in [1/6] clearly depend on the property defined here. > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > --- > .../acpi/dsd/allow-low-power-probe.rst | 28 +++++++++++++++++++ > Documentation/firmware-guide/acpi/index.rst | 1 + > 2 files changed, 29 insertions(+) > create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > new file mode 100644 > index 0000000000000..6fcc89162b898 > --- /dev/null > +++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > @@ -0,0 +1,28 @@ > +.. SPDX-License-Identifier: GPL-2.0 > + > +====================================== > +Probing I²C devices in low power state > +====================================== > + > +Introduction > +============ > + > +In some cases it may be preferred to leave certain devices powered off for the > +entire system bootup if powering on these devices has adverse side effects, > +beyond just powering on the said device. Linux recognizes the _DSD property > +"allow-low-power-probe" that can be used for this purpose. It would be good to refer to the document defining the generic _DSD properties mechanism and the GUID used for that from here. The meaning of "_DSD property" may not be entirely clear to the reader as it stands. And maybe call the property "i2c-allow-low-power-probe" or similar, to indicate that it is specific to i2c. > + > +How it works > +============ > + > +The property "allow-low-power-probe" boolean property may be used to tell Linux "boolean device property" ? > +that the I²C framework should instruct the kernel ACPI framework to leave the > +device in the low power state. If the driver indicates its support for this by > +setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags > +field and the "allow-low-power-probe" property is present, the device will not > +be powered on for probe. > + > +The downside is that as the device is not powered on, even if there's a problem > +with the device, the driver likely probes just fine but the first user will > +find out the device doesn't work, instead of a failure at probe time. This > +feature should thus be used sparingly. It would be good to give an ASL example with this property provided. > diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst > index ad3b5afdae77e..0070fcde9e669 100644 > --- a/Documentation/firmware-guide/acpi/index.rst > +++ b/Documentation/firmware-guide/acpi/index.rst > @@ -11,6 +11,7 @@ ACPI Support > dsd/graph > dsd/data-node-references > dsd/leds > + dsd/allow-low-power-probe > enumeration > osi > method-customizing > --
Hi Rafael, Thank you for the review. On Wed, Sep 02, 2020 at 05:57:01PM +0200, Rafael J. Wysocki wrote: > On Tue, Sep 1, 2020 at 11:03 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Document the probe-low-power _DSD property and how it is used with I²C > > drivers. > > I would reorder the series to make this go right after the [1/6] or > maybe even fold it into that patch. > > The point is that the changes in [1/6] clearly depend on the property > defined here. Ack. > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > --- > > .../acpi/dsd/allow-low-power-probe.rst | 28 +++++++++++++++++++ > > Documentation/firmware-guide/acpi/index.rst | 1 + > > 2 files changed, 29 insertions(+) > > create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > > > diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > new file mode 100644 > > index 0000000000000..6fcc89162b898 > > --- /dev/null > > +++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > @@ -0,0 +1,28 @@ > > +.. SPDX-License-Identifier: GPL-2.0 > > + > > +====================================== > > +Probing I²C devices in low power state > > +====================================== > > + > > +Introduction > > +============ > > + > > +In some cases it may be preferred to leave certain devices powered off for the > > +entire system bootup if powering on these devices has adverse side effects, > > +beyond just powering on the said device. Linux recognizes the _DSD property > > +"allow-low-power-probe" that can be used for this purpose. > > It would be good to refer to the document defining the generic _DSD > properties mechanism and the GUID used for that from here. I'll add a reference to the device properties UUID for _DSD spec. > > The meaning of "_DSD property" may not be entirely clear to the > reader as it stands. > > And maybe call the property "i2c-allow-low-power-probe" or similar, to > indicate that it is specific to i2c. The bus determines that already, and it's only defined for I²C here. Should we need this in the future for e.g. I3C, there would be no need to think of renaming it, just changing the documentation and implementation. I don't have a strong opinion on that though. > > > + > > +How it works > > +============ > > + > > +The property "allow-low-power-probe" boolean property may be used to tell Linux > > "boolean device property" ? Oops. The other "property" was supposed to be removed by now. But yes, I agree with the suggestion. > > > +that the I²C framework should instruct the kernel ACPI framework to leave the > > +device in the low power state. If the driver indicates its support for this by > > +setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags > > +field and the "allow-low-power-probe" property is present, the device will not > > +be powered on for probe. > > + > > +The downside is that as the device is not powered on, even if there's a problem > > +with the device, the driver likely probes just fine but the first user will > > +find out the device doesn't work, instead of a failure at probe time. This > > +feature should thus be used sparingly. > > It would be good to give an ASL example with this property provided. I'll add that for v8. > > > diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst > > index ad3b5afdae77e..0070fcde9e669 100644 > > --- a/Documentation/firmware-guide/acpi/index.rst > > +++ b/Documentation/firmware-guide/acpi/index.rst > > @@ -11,6 +11,7 @@ ACPI Support > > dsd/graph > > dsd/data-node-references > > dsd/leds > > + dsd/allow-low-power-probe > > enumeration > > osi > > method-customizing > > --
On Wed, Sep 02, 2020 at 06:34:30PM +0200, Rafael J. Wysocki wrote: > On Wed, Sep 2, 2020 at 6:31 PM Sakari Ailus > <sakari.ailus@linux.intel.com> wrote: > > > > Hi Rafael, > > > > Thank you for the review. > > > > On Wed, Sep 02, 2020 at 05:57:01PM +0200, Rafael J. Wysocki wrote: > > > On Tue, Sep 1, 2020 at 11:03 PM Sakari Ailus > > > <sakari.ailus@linux.intel.com> wrote: > > > > > > > > Document the probe-low-power _DSD property and how it is used with I²C > > > > drivers. > > > > > > I would reorder the series to make this go right after the [1/6] or > > > maybe even fold it into that patch. > > > > > > The point is that the changes in [1/6] clearly depend on the property > > > defined here. > > > > Ack. > > > > > > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> > > > > --- > > > > .../acpi/dsd/allow-low-power-probe.rst | 28 +++++++++++++++++++ > > > > Documentation/firmware-guide/acpi/index.rst | 1 + > > > > 2 files changed, 29 insertions(+) > > > > create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > > > > > > > diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > > > new file mode 100644 > > > > index 0000000000000..6fcc89162b898 > > > > --- /dev/null > > > > +++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst > > > > @@ -0,0 +1,28 @@ > > > > +.. SPDX-License-Identifier: GPL-2.0 > > > > + > > > > +====================================== > > > > +Probing I²C devices in low power state > > > > +====================================== > > > > + > > > > +Introduction > > > > +============ > > > > + > > > > +In some cases it may be preferred to leave certain devices powered off for the > > > > +entire system bootup if powering on these devices has adverse side effects, > > > > +beyond just powering on the said device. Linux recognizes the _DSD property > > > > +"allow-low-power-probe" that can be used for this purpose. > > > > > > It would be good to refer to the document defining the generic _DSD > > > properties mechanism and the GUID used for that from here. > > > > I'll add a reference to the device properties UUID for _DSD spec. > > > > > > > > The meaning of "_DSD property" may not be entirely clear to the > > > reader as it stands. > > > > > > And maybe call the property "i2c-allow-low-power-probe" or similar, to > > > indicate that it is specific to i2c. > > > > The bus determines that already, and it's only defined for I²C here. Should > > we need this in the future for e.g. I3C, there would be no need to think of > > renaming it, just changing the documentation and implementation. > > > > I don't have a strong opinion on that though. > > The point is that the property is not going to be recognized beyond i2c. Not right now, that is true. Sure, I can change it for v8.
Hi all, These patches enable calling (and finishing) a driver's probe function without powering on the respective device on busses where the practice is to power on the device for probe. While it generally is a driver's job to check the that the device is there, there are cases where it might be undesirable. (In this case it stems from a combination of hardware design and user expectations; see below.) The downside with this change is that if there is something wrong with the device, it will only be found at the time the device is used. In this case (the camera sensors + EEPROM in a sensor) I don't see any tangible harm from that though. An indication both from the driver and the firmware is required to allow the device's power state to remain off during probe (see the first patch). The use case is such that there is a privacy LED next to an integrated user-facing laptop camera, and this LED is there to signal the user that the camera is recording a video or capturing images. That LED also happens to be wired to one of the power supplies of the camera, so whenever you power on the camera, the LED will be lit, whether images are captured from the camera --- or not. There's no way to implement this differently without additional software control (allowing of which is itself a hardware design decision) on most CSI-2-connected camera sensors as they simply have no pin to signal the camera streaming state. This is also what happens during driver probe: the camera will be powered on by the I²C subsystem calling dev_pm_domain_attach() and the device is already powered on when the driver's own probe function is called. To the user this visible during the boot process as a blink of the privacy LED, suggesting that the camera is recording without the user having used an application to do that. From the end user's point of view the behaviour is not expected and for someone unfamiliar with internal workings of a computer surely seems quite suspicious --- even if images are not being actually captured. I've tested these on linux-next master. They also apply to Wolfram's i2c/for-next branch, there's a patch that affects the I²C core changes here (see below). The patches apart from that apply to Bartosz's at24/for-next as well as Mauro's linux-media master branch. since v6 <URL:https://lore.kernel.org/linux-acpi/20200826115432.6103-1-sakari.ailus@linux.intel.com/>: - Use u32 for the flags field in struct i2c_driver. - Use acpi_dev_get_property to read the allow-low-power-probe property. since v5 <URL:https://lore.kernel.org/linux-acpi/20200810142747.12400-1-sakari.ailus@linux.intel.com/>: - Identify sensors when they're first powered on. In previous versions, if this wasn't in probe, it was not done at all. - Return allow_low_power_probe() only for ACPI devices, i.e. OF systems are not affected by these changes. - Document that I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag only applies to ACPI drivers. - Fix extra regulator_disable in at24 driver's remove function when the device was already in low power state. since v4 <URL:https://lore.kernel.org/linux-acpi/20200121134157.20396-1-sakari.ailus@linux.intel.com/>: - Rename "probe-low-power" property as "allow-low-power-probe". This is taken into account in function and file naming, too. - Turn probe_low_power field in struct i2c_driver into flags field. - Rebase on Wolfram's i2c/for-next branch that contains the removal of the support for disabling I²C core IRQ mappings (commit 0c2a34937f7e4c4776bb261114c475392da2355c). - Change wording for "allow-low-power-probe" property in ACPI documentation. since v3 <URL:https://lore.kernel.org/linux-acpi/20200109154529.19484-1-sakari.ailus@linux.intel.com/T/#t>: - Rework the 2nd patch based on Rafael's comments - Rework description of the ACPI low power state helper function, according to Rafael's text. - Rename and rework the same function as acpi_dev_state_low_power(). - Reflect the changes in commit message as well. - Added a patch to document the probe-low-power _DSD property. since v2 <URL:https://patchwork.kernel.org/cover/11114255/>: - Remove extra CONFIG_PM ifdefs; these are not needed. - Move the checks for power state hints from drivers/base/dd.c to drivers/i2c/i2c-base-core.c; these are I²C devices anyway. - Move the probe_low_power field from struct device_driver to struct i2c_driver. since v1: - Rename probe_powered_off struct device field as probe_low_power and reflect the similar naming to the patches overall. - Work with CONFIG_PM disabled, too. Rajmohan Mani (1): media: i2c: imx319: Support probe while the device is off Sakari Ailus (5): i2c: Allow an ACPI driver to manage the device's power state during probe ACPI: Add a convenience function to tell a device is in low power state ov5670: Support probe whilst the device is in a low power state at24: Support probing while off Documentation: ACPI: Document allow-low-power-probe _DSD property .../acpi/dsd/allow-low-power-probe.rst | 28 +++++++ Documentation/firmware-guide/acpi/index.rst | 1 + drivers/acpi/device_pm.c | 31 ++++++++ drivers/i2c/i2c-core-base.c | 19 ++++- drivers/media/i2c/imx319.c | 74 +++++++++++------- drivers/media/i2c/ov5670.c | 76 +++++++++++-------- drivers/misc/eeprom/at24.c | 43 ++++++----- include/linux/acpi.h | 5 ++ include/linux/i2c.h | 14 ++++ 9 files changed, 212 insertions(+), 79 deletions(-) create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst