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 v10 <URL:https://lore.kernel.org/linux-acpi/[email protected]/>:
- 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/[email protected]/>:
- 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/[email protected]/>:
- 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/[email protected]/>:
- 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/[email protected]/>:
- 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/[email protected]/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 (6):
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 _DSE object usage for enum power state
ACPI: Add a convenience function to tell a device is in D0 state
ov5670: Support probe whilst the device is in ACPI D state other than
0
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 | 27 +++++++
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/media/i2c/ov5670.c | 78 +++++++++++--------
drivers/misc/eeprom/at24.c | 43 ++++++----
include/acpi/acpi_bus.h | 1 +
include/linux/acpi.h | 6 ++
include/linux/i2c.h | 18 +++++
12 files changed, 265 insertions(+), 82 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/non-d0-probe.rst
--
2.20.1
From: Rajmohan Mani <[email protected]>
Tell ACPI device PM code that the driver supports the device being in
non-zero ACPI D state when the driver's probe function is entered.
Signed-off-by: Rajmohan Mani <[email protected]>
Signed-off-by: Sakari Ailus <[email protected]>
Reviewed-by: Tomasz Figa <[email protected]>
Reviewed-by: Bingbu Cao <[email protected]>
---
drivers/media/i2c/imx319.c | 74 ++++++++++++++++++++++----------------
1 file changed, 44 insertions(+), 30 deletions(-)
diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 8473c0bbb35d6..3bae32ab533fe 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -140,6 +140,8 @@ struct imx319 {
/* Streaming on/off */
bool streaming;
+ /* True if the device has been identified */
+ bool identified;
};
static const struct imx319_reg imx319_global_regs[] = {
@@ -2084,6 +2086,31 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
return 0;
}
+/* Verify chip ID */
+static int imx319_identify_module(struct imx319 *imx319)
+{
+ struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
+ int ret;
+ u32 val;
+
+ if (imx319->identified)
+ return 0;
+
+ ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
+ if (ret)
+ return ret;
+
+ if (val != IMX319_CHIP_ID) {
+ dev_err(&client->dev, "chip id mismatch: %x!=%x",
+ IMX319_CHIP_ID, val);
+ return -EIO;
+ }
+
+ imx319->identified = true;
+
+ return 0;
+}
+
/* Start streaming */
static int imx319_start_streaming(struct imx319 *imx319)
{
@@ -2091,6 +2118,10 @@ static int imx319_start_streaming(struct imx319 *imx319)
const struct imx319_reg_list *reg_list;
int ret;
+ ret = imx319_identify_module(imx319);
+ if (ret)
+ return ret;
+
/* Global Setting */
reg_list = &imx319_global_setting;
ret = imx319_write_regs(imx319, reg_list->regs, reg_list->num_of_regs);
@@ -2208,26 +2239,6 @@ static int __maybe_unused imx319_resume(struct device *dev)
return ret;
}
-/* Verify chip ID */
-static int imx319_identify_module(struct imx319 *imx319)
-{
- struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
- int ret;
- u32 val;
-
- ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
- if (ret)
- return ret;
-
- if (val != IMX319_CHIP_ID) {
- dev_err(&client->dev, "chip id mismatch: %x!=%x",
- IMX319_CHIP_ID, val);
- return -EIO;
- }
-
- return 0;
-}
-
static const struct v4l2_subdev_core_ops imx319_subdev_core_ops = {
.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
.unsubscribe_event = v4l2_event_subdev_unsubscribe,
@@ -2422,6 +2433,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
static int imx319_probe(struct i2c_client *client)
{
struct imx319 *imx319;
+ bool full_power;
int ret;
u32 i;
@@ -2434,11 +2446,14 @@ static int imx319_probe(struct i2c_client *client)
/* Initialize subdev */
v4l2_i2c_subdev_init(&imx319->sd, client, &imx319_subdev_ops);
- /* Check module identity */
- ret = imx319_identify_module(imx319);
- if (ret) {
- dev_err(&client->dev, "failed to find sensor: %d", ret);
- goto error_probe;
+ full_power = acpi_dev_state_d0(&client->dev);
+ if (full_power) {
+ /* Check module identity */
+ ret = imx319_identify_module(imx319);
+ if (ret) {
+ dev_err(&client->dev, "failed to find sensor: %d", ret);
+ goto error_probe;
+ }
}
imx319->hwcfg = imx319_get_hwcfg(&client->dev);
@@ -2490,11 +2505,9 @@ static int imx319_probe(struct i2c_client *client)
if (ret < 0)
goto error_media_entity;
- /*
- * Device is already turned on by i2c-core with ACPI domain PM.
- * Enable runtime PM and turn off the device.
- */
- pm_runtime_set_active(&client->dev);
+ /* Set the device's state to active if it's in D0 state. */
+ if (full_power)
+ pm_runtime_set_active(&client->dev);
pm_runtime_enable(&client->dev);
pm_runtime_idle(&client->dev);
@@ -2547,6 +2560,7 @@ static struct i2c_driver imx319_i2c_driver = {
},
.probe_new = imx319_probe,
.remove = imx319_remove,
+ .flags = I2C_DRV_ACPI_WAIVE_D0_PROBE,
};
module_i2c_driver(imx319_i2c_driver);
--
2.20.1