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, Bartosz's at24/for-next, Wolfram's
i2c/for-next as well as Linux media master today; the patches apply to all
without trouble.
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 probe while the device is off
Sakari Ailus (5):
i2c: Allow 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 probe-low-power _DSD property
.../acpi/dsd/probe-low-power.rst | 28 +++++++++++++++++
Documentation/firmware-guide/acpi/index.rst | 1 +
drivers/acpi/device_pm.c | 31 +++++++++++++++++++
drivers/i2c/i2c-core-base.c | 15 +++++++--
drivers/media/i2c/imx319.c | 23 ++++++++------
drivers/media/i2c/ov5670.c | 23 ++++++++------
drivers/misc/eeprom/at24.c | 31 +++++++++++++------
include/linux/acpi.h | 5 +++
include/linux/i2c.h | 3 ++
9 files changed, 129 insertions(+), 31 deletions(-)
create mode 100644 Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
--
2.20.1
Enable drivers to tell ACPI that there's no need to power on a device for
probe. Drivers should still perform this by themselves if there's a need
to. In some cases powering on the device during probe is undesirable, and
this change enables a driver to choose what fits best for it.
Signed-off-by: Sakari Ailus <[email protected]>
---
drivers/i2c/i2c-core-base.c | 15 ++++++++++++---
include/linux/i2c.h | 3 +++
2 files changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9f8dcd3f83850..7bf1699c9044d 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -303,6 +303,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
return irq > 0 ? irq : -ENXIO;
}
+static bool probe_low_power(struct device *dev)
+{
+ struct i2c_driver *driver = to_i2c_driver(dev->driver);
+
+ return driver->probe_low_power &&
+ device_property_present(dev, "probe-low-power");
+}
+
static int i2c_device_probe(struct device *dev)
{
struct i2c_client *client = i2c_verify_client(dev);
@@ -375,7 +383,8 @@ static int i2c_device_probe(struct device *dev)
if (status < 0)
goto err_clear_wakeup_irq;
- status = dev_pm_domain_attach(&client->dev, true);
+ status = dev_pm_domain_attach(&client->dev,
+ !probe_low_power(&client->dev));
if (status)
goto err_clear_wakeup_irq;
@@ -397,7 +406,7 @@ static int i2c_device_probe(struct device *dev)
return 0;
err_detach_pm_domain:
- dev_pm_domain_detach(&client->dev, true);
+ dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
err_clear_wakeup_irq:
dev_pm_clear_wake_irq(&client->dev);
device_init_wakeup(&client->dev, false);
@@ -419,7 +428,7 @@ static int i2c_device_remove(struct device *dev)
status = driver->remove(client);
}
- dev_pm_domain_detach(&client->dev, true);
+ dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
dev_pm_clear_wake_irq(&client->dev);
device_init_wakeup(&client->dev, false);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 582ef05ec07ed..6d0d6af393c56 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -229,6 +229,8 @@ enum i2c_alert_protocol {
* @address_list: The I2C addresses to probe (for detect)
* @clients: List of detected clients we created (for i2c-core use only)
* @disable_i2c_core_irq_mapping: Tell the i2c-core to not do irq-mapping
+ * @probe_low_power: Let the driver manage the device's power state
+ * during probe and remove.
*
* The driver.owner field should be set to the module owner of this driver.
* The driver.name field should be set to the name of this driver.
@@ -289,6 +291,7 @@ struct i2c_driver {
struct list_head clients;
bool disable_i2c_core_irq_mapping;
+ bool probe_low_power;
};
#define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
--
2.20.1
wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
>
> Enable drivers to tell ACPI that there's no need to power on a device for
> probe. Drivers should still perform this by themselves if there's a need
> to. In some cases powering on the device during probe is undesirable, and
> this change enables a driver to choose what fits best for it.
>
> Signed-off-by: Sakari Ailus <[email protected]>
> ---
> drivers/i2c/i2c-core-base.c | 15 ++++++++++++---
> include/linux/i2c.h | 3 +++
> 2 files changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9f8dcd3f83850..7bf1699c9044d 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -303,6 +303,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> return irq > 0 ? irq : -ENXIO;
> }
>
> +static bool probe_low_power(struct device *dev)
> +{
> + struct i2c_driver *driver = to_i2c_driver(dev->driver);
> +
> + return driver->probe_low_power &&
> + device_property_present(dev, "probe-low-power");
> +}
> +
> static int i2c_device_probe(struct device *dev)
> {
> struct i2c_client *client = i2c_verify_client(dev);
> @@ -375,7 +383,8 @@ static int i2c_device_probe(struct device *dev)
> if (status < 0)
> goto err_clear_wakeup_irq;
>
> - status = dev_pm_domain_attach(&client->dev, true);
> + status = dev_pm_domain_attach(&client->dev,
> + !probe_low_power(&client->dev));
> if (status)
> goto err_clear_wakeup_irq;
>
> @@ -397,7 +406,7 @@ static int i2c_device_probe(struct device *dev)
> return 0;
>
> err_detach_pm_domain:
> - dev_pm_domain_detach(&client->dev, true);
> + dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
> err_clear_wakeup_irq:
> dev_pm_clear_wake_irq(&client->dev);
> device_init_wakeup(&client->dev, false);
> @@ -419,7 +428,7 @@ static int i2c_device_remove(struct device *dev)
> status = driver->remove(client);
> }
>
> - dev_pm_domain_detach(&client->dev, true);
> + dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
>
> dev_pm_clear_wake_irq(&client->dev);
> device_init_wakeup(&client->dev, false);
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 582ef05ec07ed..6d0d6af393c56 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -229,6 +229,8 @@ enum i2c_alert_protocol {
> * @address_list: The I2C addresses to probe (for detect)
> * @clients: List of detected clients we created (for i2c-core use only)
> * @disable_i2c_core_irq_mapping: Tell the i2c-core to not do irq-mapping
> + * @probe_low_power: Let the driver manage the device's power state
> + * during probe and remove.
> *
> * The driver.owner field should be set to the module owner of this driver.
> * The driver.name field should be set to the name of this driver.
> @@ -289,6 +291,7 @@ struct i2c_driver {
> struct list_head clients;
>
> bool disable_i2c_core_irq_mapping;
> + bool probe_low_power;
I don't see any users of disable_i2c_core_irq_mapping in current
mainline. Maybe instead of adding another 1-byte boolean for every
such property, let's just use the fact that this struct will have at
least an alignment of 32-bits anyway and merge the two into an int
field called 'flags' so that we can extend it in the future if needed?
The name 'probe_low_power' is misleading to me too. It makes me think
it's the default state for some reason. It should be something like
'allow_low_power_probe'.
Bartosz
> };
> #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
>
> --
> 2.20.1
>
Hi Bartosz,
On Wed, Jan 29, 2020 at 02:54:20PM +0100, Bartosz Golaszewski wrote:
> wt., 21 sty 2020 o 14:41 Sakari Ailus <[email protected]> napisał(a):
> >
> > Enable drivers to tell ACPI that there's no need to power on a device for
> > probe. Drivers should still perform this by themselves if there's a need
> > to. In some cases powering on the device during probe is undesirable, and
> > this change enables a driver to choose what fits best for it.
> >
> > Signed-off-by: Sakari Ailus <[email protected]>
> > ---
> > drivers/i2c/i2c-core-base.c | 15 ++++++++++++---
> > include/linux/i2c.h | 3 +++
> > 2 files changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 9f8dcd3f83850..7bf1699c9044d 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -303,6 +303,14 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> > return irq > 0 ? irq : -ENXIO;
> > }
> >
> > +static bool probe_low_power(struct device *dev)
> > +{
> > + struct i2c_driver *driver = to_i2c_driver(dev->driver);
> > +
> > + return driver->probe_low_power &&
> > + device_property_present(dev, "probe-low-power");
> > +}
> > +
> > static int i2c_device_probe(struct device *dev)
> > {
> > struct i2c_client *client = i2c_verify_client(dev);
> > @@ -375,7 +383,8 @@ static int i2c_device_probe(struct device *dev)
> > if (status < 0)
> > goto err_clear_wakeup_irq;
> >
> > - status = dev_pm_domain_attach(&client->dev, true);
> > + status = dev_pm_domain_attach(&client->dev,
> > + !probe_low_power(&client->dev));
> > if (status)
> > goto err_clear_wakeup_irq;
> >
> > @@ -397,7 +406,7 @@ static int i2c_device_probe(struct device *dev)
> > return 0;
> >
> > err_detach_pm_domain:
> > - dev_pm_domain_detach(&client->dev, true);
> > + dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
> > err_clear_wakeup_irq:
> > dev_pm_clear_wake_irq(&client->dev);
> > device_init_wakeup(&client->dev, false);
> > @@ -419,7 +428,7 @@ static int i2c_device_remove(struct device *dev)
> > status = driver->remove(client);
> > }
> >
> > - dev_pm_domain_detach(&client->dev, true);
> > + dev_pm_domain_detach(&client->dev, !probe_low_power(&client->dev));
> >
> > dev_pm_clear_wake_irq(&client->dev);
> > device_init_wakeup(&client->dev, false);
> > diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> > index 582ef05ec07ed..6d0d6af393c56 100644
> > --- a/include/linux/i2c.h
> > +++ b/include/linux/i2c.h
> > @@ -229,6 +229,8 @@ enum i2c_alert_protocol {
> > * @address_list: The I2C addresses to probe (for detect)
> > * @clients: List of detected clients we created (for i2c-core use only)
> > * @disable_i2c_core_irq_mapping: Tell the i2c-core to not do irq-mapping
> > + * @probe_low_power: Let the driver manage the device's power state
> > + * during probe and remove.
> > *
> > * The driver.owner field should be set to the module owner of this driver.
> > * The driver.name field should be set to the name of this driver.
> > @@ -289,6 +291,7 @@ struct i2c_driver {
> > struct list_head clients;
> >
> > bool disable_i2c_core_irq_mapping;
> > + bool probe_low_power;
>
> I don't see any users of disable_i2c_core_irq_mapping in current
> mainline. Maybe instead of adding another 1-byte boolean for every
> such property, let's just use the fact that this struct will have at
> least an alignment of 32-bits anyway and merge the two into an int
> field called 'flags' so that we can extend it in the future if needed?
>
> The name 'probe_low_power' is misleading to me too. It makes me think
> it's the default state for some reason. It should be something like
> 'allow_low_power_probe'.
Ah, got around reading this one after sending the previous mail.
Sounds reasonable. I'll address both in v5.
--
Kind regards,
Sakari Ailus