[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAJZ5v0gABbjXzahAG-pNd1KBjn_pgaWJUQjvV0Exiju0Ho2FBQ@mail.gmail.com>
Date: Wed, 3 Nov 2021 19:10:19 +0100
From: "Rafael J. Wysocki" <rafael@...nel.org>
To: Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc: linux-i2c <linux-i2c@...r.kernel.org>,
Wolfram Sang <wsa@...-dreams.de>,
"Rafael J. Wysocki" <rafael@...nel.org>,
ACPI Devel Maling List <linux-acpi@...r.kernel.org>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Mani, Rajmohan" <rajmohan.mani@...el.com>,
Tomasz Figa <tfiga@...omium.org>,
Bartosz Golaszewski <bgolaszewski@...libre.com>,
Bingbu Cao <bingbu.cao@...el.com>,
Chiranjeevi Rapolu <chiranjeevi.rapolu@...el.com>,
Hyungwoo Yang <hyungwoo.yang@...el.com>,
Linux Media Mailing List <linux-media@...r.kernel.org>
Subject: Re: [PATCH v11 0/6] Support running driver's probe for a device
powered off
On Mon, Oct 18, 2021 at 5:16 PM Sakari Ailus
<sakari.ailus@...ux.intel.com> wrote:
>
> Hello everyone,
>
> 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 second 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.
>
> since v11 <URL:https://lore.kernel.org/lkml/20210210230800.30291-1-sakari.ailus@linux.intel.com/>:
>
> - Rebase on linux-next.
>
> - Call device object that designates the intended power state for probe
> "_DSC" instead of "_DSE".
>
> - Remove the ov5670 driver patch from the set due to conflict (will merge
> through media tree later on).
>
> since v10 <URL:https://lore.kernel.org/linux-acpi/20210205132505.20173-1-sakari.ailus@linux.intel.com/>:
>
> - Instead of "low power state" refer to ACPI D states and "full power", of
> which latter is used in individual drivers.
>
> - Fix remaining references to _DSD properties.
>
> - Rework commit messages to reflect recent changes.
>
> - Rework the documentation to separate _DSE from I²C as it's not really
> specific to that.
>
> - Rename the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag as
> I2C_DRV_ACPI_WAIVE_D0_PROBE.
>
> since v9 <URL:https://lore.kernel.org/linux-acpi/CAJZ5v0jjgy2KXOw5pyshvaEVzLctu4jsgYK1+YDA+8sZfp-6mw@mail.gmail.com/T/#m003f56b33350772364b1f5c832e9025677107933>:
>
> - Use _DSE object designed for the very purpose of designating intended
> device probe power state instead of _PRE.
>
> - Rework documentation to reflect the property to _DSE changes (missed in
> v9).
>
> - Put maximum device enumeration power state in struct acpi_device_power,
> instead of a flag in struct acpi_device_power_flags. The default is
> ACPI_STATE_D0.
>
> - i2c_acpi_allow_low_power_probe() now returns true if the desired power
> state is greater or equal to the current device power state.
>
> - Rename local variable low_power as off_during_probe.
>
> since v8 <URL:https://lore.kernel.org/patchwork/cover/1300068/>:
>
> - Make use of ACPI _PRE object instead of a _DSD property (new patch,
> 1st), align documentation with that.
>
> - Added a blank line.
>
> - Rebased on current linux-next master.
>
> since v7 <URL:https://lore.kernel.org/linux-acpi/20200901210333.8462-1-sakari.ailus@linux.intel.com/>:
>
> - Reorder documentation patch right after the implemenation in the I²C
> framework.
>
> - Rename allow-low-power-probe property as i2c-allow-low-power-probe.
>
> - Remove extra "property" from the description of the
> i2c-allow-low-power-probe property and mention it's a device property.
>
> - Add an example to the documentation and refer to the _DSD property spec.
>
> 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 device probe in non-zero ACPI D state
>
> Sakari Ailus (5):
> ACPI: scan: Obtain device's desired enumeration power state
> i2c: Allow an ACPI driver to manage the device's power state during
> probe
> Documentation: ACPI: Document _DSC object usage for enum power state
> ACPI: Add a convenience function to tell a device is in D0 state
> at24: Support probing while in non-zero ACPI D state
>
> Documentation/firmware-guide/acpi/index.rst | 1 +
> .../firmware-guide/acpi/non-d0-probe.rst | 78 +++++++++++++++++++
> drivers/acpi/device_pm.c | 26 +++++++
> drivers/acpi/scan.c | 4 +
> drivers/i2c/i2c-core-acpi.c | 10 +++
> drivers/i2c/i2c-core-base.c | 7 +-
> drivers/media/i2c/imx319.c | 74 +++++++++++-------
> drivers/misc/eeprom/at24.c | 45 ++++++-----
> include/acpi/acpi_bus.h | 1 +
> include/linux/acpi.h | 5 ++
> include/linux/i2c.h | 18 +++++
> 11 files changed, 218 insertions(+), 51 deletions(-)
> create mode 100644 Documentation/firmware-guide/acpi/non-d0-probe.rst
>
> --
I've queued up this series for 5.16-rc, thanks!
Powered by blists - more mailing lists