Linux-ACPI Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4 0/6] Support running driver's probe for a device powered off
@ 2020-01-21 13:41 Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe Sakari Ailus
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

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/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 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe
  2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
@ 2020-01-21 13:41 ` Sakari Ailus
  2020-01-29 13:54   ` Bartosz Golaszewski
  2020-01-21 13:41 ` [PATCH v4 2/6] ACPI: Add a convenience function to tell a device is in low power state Sakari Ailus
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

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 <sakari.ailus@linux.intel.com>
---
 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 2/6] ACPI: Add a convenience function to tell a device is in low power state
  2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe Sakari Ailus
@ 2020-01-21 13:41 ` Sakari Ailus
  2020-01-21 16:07   ` Rafael J. Wysocki
  2020-01-21 13:41 ` [PATCH v4 3/6] ov5670: Support probe whilst the device is in a " Sakari Ailus
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

Add a convenience function to tell whether a device is in low power state,
primarily for use in drivers' probe or remove functions on busses where
the custom is to power on the device for the duration of both.

Returns false on non-ACPI systems.

Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/device_pm.c | 31 +++++++++++++++++++++++++++++++
 include/linux/acpi.h     |  5 +++++
 2 files changed, 36 insertions(+)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 5e4a8860a9c0c..d3174c6edf915 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1348,4 +1348,35 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 	return 1;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+/**
+ * acpi_dev_state_low_power - Check the current ACPI power state of a device.
+ * @dev: Physical device the ACPI power state of which to check
+ *
+ * On a system without ACPI, return false. On a system with ACPI, return true if
+ * the current ACPI power state of the device is not D0, or false otherwise.
+ *
+ * Note that the power state of a device is not well-defined after it has been
+ * passed to acpi_device_set_power() and before that function returns, so it is
+ * not valid to ask for the ACPI power state of the device in that time frame.
+ */
+bool acpi_dev_state_low_power(struct device *dev)
+{
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+	int power_state;
+	int ret;
+
+	if (!adev)
+		return false;
+
+	ret = acpi_device_get_power(adev, &power_state);
+	if (ret) {
+		dev_dbg(dev, "Cannot obtain power state (%d)\n", ret);
+		return false;
+	}
+
+	return power_state != ACPI_STATE_D0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_state_low_power);
+
 #endif /* CONFIG_PM */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index 0f37a7d5fa774..aa666da311444 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -926,6 +926,7 @@ int acpi_dev_resume(struct device *dev);
 int acpi_subsys_runtime_suspend(struct device *dev);
 int acpi_subsys_runtime_resume(struct device *dev);
 int acpi_dev_pm_attach(struct device *dev, bool power_on);
+bool acpi_dev_state_low_power(struct device *dev);
 #else
 static inline int acpi_dev_runtime_suspend(struct device *dev) { return 0; }
 static inline int acpi_dev_runtime_resume(struct device *dev) { return 0; }
@@ -935,6 +936,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 {
 	return 0;
 }
+static inline bool acpi_dev_state_low_power(struct device *dev)
+{
+	return false;
+}
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 3/6] ov5670: Support probe whilst the device is in a low power state
  2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 2/6] ACPI: Add a convenience function to tell a device is in low power state Sakari Ailus
@ 2020-01-21 13:41 ` " Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 4/6] media: i2c: imx319: Support probe while the device is off Sakari Ailus
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

Tell ACPI device PM code that the driver supports the device being in a
low power state when the driver's probe function is entered.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov5670.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebdf..47c7241de15c2 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	bool low_power;
 	int ret;
 
 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
@@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-	/* Check module identity */
-	ret = ov5670_identify_module(ov5670);
-	if (ret) {
-		err_msg = "ov5670_identify_module() error";
-		goto error_print;
+	low_power = acpi_dev_state_low_power(&client->dev);
+	if (!low_power) {
+		/* Check module identity */
+		ret = ov5670_identify_module(ov5670);
+		if (ret) {
+			err_msg = "ov5670_identify_module() error";
+			goto error_print;
+		}
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2501,10 +2505,10 @@ static int ov5670_probe(struct i2c_client *client)
 	ov5670->streaming = false;
 
 	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
+	 * Don't set the device's state to active if it's in a low power state.
 	 */
-	pm_runtime_set_active(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 
@@ -2546,7 +2550,7 @@ static const struct dev_pm_ops ov5670_pm_ops = {
 
 #ifdef CONFIG_ACPI
 static const struct acpi_device_id ov5670_acpi_ids[] = {
-	{"INT3479"},
+	{ "INT3479" },
 	{ /* sentinel */ }
 };
 
@@ -2561,6 +2565,7 @@ static struct i2c_driver ov5670_i2c_driver = {
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
+	.probe_low_power = true,
 };
 
 module_i2c_driver(ov5670_i2c_driver);
-- 
2.20.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 4/6] media: i2c: imx319: Support probe while the device is off
  2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
                   ` (2 preceding siblings ...)
  2020-01-21 13:41 ` [PATCH v4 3/6] ov5670: Support probe whilst the device is in a " Sakari Ailus
@ 2020-01-21 13:41 ` Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 5/6] at24: Support probing while off Sakari Ailus
  2020-01-21 13:41 ` [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property Sakari Ailus
  5 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

From: Rajmohan Mani <rajmohan.mani@intel.com>

Tell ACPI device PM code that the driver supports the device being powered
off when the driver's probe function is entered.

Signed-off-by: Rajmohan Mani <rajmohan.mani@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/imx319.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221e..bb094a4d303ca 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2424,6 +2424,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
 static int imx319_probe(struct i2c_client *client)
 {
 	struct imx319 *imx319;
+	bool low_power;
 	int ret;
 	u32 i;
 
@@ -2436,11 +2437,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;
+	low_power = acpi_dev_state_low_power(&client->dev);
+	if (!low_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);
@@ -2493,10 +2497,10 @@ static int imx319_probe(struct i2c_client *client)
 		goto error_media_entity;
 
 	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
+	 * Don't set the device's state to active if it's in a low power state.
 	 */
-	pm_runtime_set_active(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 
@@ -2536,7 +2540,7 @@ static const struct dev_pm_ops imx319_pm_ops = {
 };
 
 static const struct acpi_device_id imx319_acpi_ids[] = {
-	{ "SONY319A" },
+	{ "SONY319A", },
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(acpi, imx319_acpi_ids);
@@ -2549,6 +2553,7 @@ static struct i2c_driver imx319_i2c_driver = {
 	},
 	.probe_new = imx319_probe,
 	.remove = imx319_remove,
+	.probe_low_power = true,
 };
 module_i2c_driver(imx319_i2c_driver);
 
-- 
2.20.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 5/6] at24: Support probing while off
  2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
                   ` (3 preceding siblings ...)
  2020-01-21 13:41 ` [PATCH v4 4/6] media: i2c: imx319: Support probe while the device is off Sakari Ailus
@ 2020-01-21 13:41 ` Sakari Ailus
  2020-01-29 13:36   ` Bartosz Golaszewski
  2020-01-21 13:41 ` [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property Sakari Ailus
  5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

In certain use cases (where the chip is part of a camera module, and the
camera module is wired together with a camera privacy LED), powering on
the device during probe is undesirable. Add support for the at24 to
execute probe while being powered off. For this to happen, a hint in form
of a device property is required from the firmware.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
 1 file changed, 21 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538a..5fc1162b67618 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -564,6 +564,7 @@ static int at24_probe(struct i2c_client *client)
 	bool i2c_fn_i2c, i2c_fn_block;
 	unsigned int i, num_addresses;
 	struct at24_data *at24;
+	bool low_power;
 	struct regmap *regmap;
 	bool writable;
 	u8 test_byte;
@@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
-	/* enable runtime pm */
-	pm_runtime_set_active(dev);
+	low_power = acpi_dev_state_low_power(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(dev);
+
 	pm_runtime_enable(dev);
 
 	/*
-	 * Perform a one-byte test read to verify that the
-	 * chip is functional.
+	 * Perform a one-byte test read to verify that the chip is functional,
+	 * unless powering on the device is to be avoided during probe (i.e.
+	 * it's powered off right now).
 	 */
-	err = at24_read(at24, 0, &test_byte, 1);
-	pm_runtime_idle(dev);
-	if (err) {
-		pm_runtime_disable(dev);
-		return -ENODEV;
+	if (!low_power) {
+		err = at24_read(at24, 0, &test_byte, 1);
+		pm_runtime_idle(dev);
+		if (err) {
+			pm_runtime_disable(dev);
+			return -ENODEV;
+		}
 	}
 
 	if (writable)
@@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
 
 static int at24_remove(struct i2c_client *client)
 {
+	bool low_power;
+
 	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
+	low_power = acpi_dev_state_low_power(&client->dev);
+	if (!low_power)
+		pm_runtime_set_suspended(&client->dev);
 
 	return 0;
 }
@@ -743,6 +753,7 @@ static struct i2c_driver at24_driver = {
 	.probe_new = at24_probe,
 	.remove = at24_remove,
 	.id_table = at24_ids,
+	.probe_low_power = true,
 };
 
 static int __init at24_init(void)
-- 
2.20.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property
  2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
                   ` (4 preceding siblings ...)
  2020-01-21 13:41 ` [PATCH v4 5/6] at24: Support probing while off Sakari Ailus
@ 2020-01-21 13:41 ` Sakari Ailus
  2020-01-21 16:09   ` Rafael J. Wysocki
  5 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 13:41 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa

Document the probe-low-power _DSD property and how it is used with I²C
drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 .../acpi/dsd/probe-low-power.rst              | 28 +++++++++++++++++++
 Documentation/firmware-guide/acpi/index.rst   |  1 +
 2 files changed, 29 insertions(+)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/probe-low-power.rst

diff --git a/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst b/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
new file mode 100644
index 0000000000000..e0343ffefe071
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/probe-low-power.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. The _DSD property
+"probe-low-power" has been defined for this purpose.
+
+How it works
+============
+
+The property "probe-low-power" boolean property may be used to tell Linux
+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 in its struct i2c_driver.probe_low_power field and the
+"probe-low-power" 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.
diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst
index ad3b5afdae77e..4e4ac675c0ff8 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/probe-low-power
    enumeration
    osi
    method-customizing
-- 
2.20.1


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 2/6] ACPI: Add a convenience function to tell a device is in low power state
  2020-01-21 13:41 ` [PATCH v4 2/6] ACPI: Add a convenience function to tell a device is in low power state Sakari Ailus
@ 2020-01-21 16:07   ` Rafael J. Wysocki
  0 siblings, 0 replies; 19+ messages in thread
From: Rafael J. Wysocki @ 2020-01-21 16:07 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki,
	ACPI Devel Maling List, Bingbu Cao, linux-media,
	Chiranjeevi Rapolu, Hyungwoo Yang, Bartosz Golaszewski,
	Arnd Bergmann, Linux Kernel Mailing List, Greg Kroah-Hartman,
	Mani, Rajmohan, Tomasz Figa

On Tue, Jan 21, 2020 at 2:41 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Add a convenience function to tell whether a device is in low power state,
> primarily for use in drivers' probe or remove functions on busses where
> the custom is to power on the device for the duration of both.
>
> Returns false on non-ACPI systems.
>
> Suggested-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

> ---
>  drivers/acpi/device_pm.c | 31 +++++++++++++++++++++++++++++++
>  include/linux/acpi.h     |  5 +++++
>  2 files changed, 36 insertions(+)
>
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index 5e4a8860a9c0c..d3174c6edf915 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1348,4 +1348,35 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>         return 1;
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
> +
> +/**
> + * acpi_dev_state_low_power - Check the current ACPI power state of a device.
> + * @dev: Physical device the ACPI power state of which to check
> + *
> + * On a system without ACPI, return false. On a system with ACPI, return true if
> + * the current ACPI power state of the device is not D0, or false otherwise.
> + *
> + * Note that the power state of a device is not well-defined after it has been
> + * passed to acpi_device_set_power() and before that function returns, so it is
> + * not valid to ask for the ACPI power state of the device in that time frame.
> + */
> +bool acpi_dev_state_low_power(struct device *dev)
> +{
> +       struct acpi_device *adev = ACPI_COMPANION(dev);
> +       int power_state;
> +       int ret;
> +
> +       if (!adev)
> +               return false;
> +
> +       ret = acpi_device_get_power(adev, &power_state);
> +       if (ret) {
> +               dev_dbg(dev, "Cannot obtain power state (%d)\n", ret);
> +               return false;
> +       }
> +
> +       return power_state != ACPI_STATE_D0;
> +}
> +EXPORT_SYMBOL_GPL(acpi_dev_state_low_power);
> +
>  #endif /* CONFIG_PM */
> diff --git a/include/linux/acpi.h b/include/linux/acpi.h
> index 0f37a7d5fa774..aa666da311444 100644
> --- a/include/linux/acpi.h
> +++ b/include/linux/acpi.h
> @@ -926,6 +926,7 @@ int acpi_dev_resume(struct device *dev);
>  int acpi_subsys_runtime_suspend(struct device *dev);
>  int acpi_subsys_runtime_resume(struct device *dev);
>  int acpi_dev_pm_attach(struct device *dev, bool power_on);
> +bool acpi_dev_state_low_power(struct device *dev);
>  #else
>  static inline int acpi_dev_runtime_suspend(struct device *dev) { return 0; }
>  static inline int acpi_dev_runtime_resume(struct device *dev) { return 0; }
> @@ -935,6 +936,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
>  {
>         return 0;
>  }
> +static inline bool acpi_dev_state_low_power(struct device *dev)
> +{
> +       return false;
> +}
>  #endif
>
>  #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property
  2020-01-21 13:41 ` [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property Sakari Ailus
@ 2020-01-21 16:09   ` Rafael J. Wysocki
  2020-01-21 16:18     ` Sakari Ailus
  0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2020-01-21 16:09 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki,
	ACPI Devel Maling List, Bingbu Cao, linux-media,
	Chiranjeevi Rapolu, Hyungwoo Yang, Bartosz Golaszewski,
	Arnd Bergmann, Linux Kernel Mailing List, Greg Kroah-Hartman,
	Mani, Rajmohan, Tomasz Figa

On Tue, Jan 21, 2020 at 2:41 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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  .../acpi/dsd/probe-low-power.rst              | 28 +++++++++++++++++++
>  Documentation/firmware-guide/acpi/index.rst   |  1 +
>  2 files changed, 29 insertions(+)
>  create mode 100644 Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
>
> diff --git a/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst b/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> new file mode 100644
> index 0000000000000..e0343ffefe071
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/probe-low-power.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. The _DSD property
> +"probe-low-power" has been defined for this purpose.

Well, if you say "has been defined", you need to provide a pointer to
the definition document (a specification of some sort or similar).

> +
> +How it works
> +============
> +
> +The property "probe-low-power" boolean property may be used to tell Linux
> +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 in its struct i2c_driver.probe_low_power field and the
> +"probe-low-power" 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.
> diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst
> index ad3b5afdae77e..4e4ac675c0ff8 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/probe-low-power
>     enumeration
>     osi
>     method-customizing
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property
  2020-01-21 16:09   ` Rafael J. Wysocki
@ 2020-01-21 16:18     ` Sakari Ailus
  2020-01-21 16:56       ` Rafael J. Wysocki
  0 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 16:18 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-i2c, Wolfram Sang, ACPI Devel Maling List, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, Linux Kernel Mailing List,
	Greg Kroah-Hartman, Mani, Rajmohan, Tomasz Figa

On Tue, Jan 21, 2020 at 05:09:44PM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 21, 2020 at 2:41 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.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  .../acpi/dsd/probe-low-power.rst              | 28 +++++++++++++++++++
> >  Documentation/firmware-guide/acpi/index.rst   |  1 +
> >  2 files changed, 29 insertions(+)
> >  create mode 100644 Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> >
> > diff --git a/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst b/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> > new file mode 100644
> > index 0000000000000..e0343ffefe071
> > --- /dev/null
> > +++ b/Documentation/firmware-guide/acpi/dsd/probe-low-power.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. The _DSD property
> > +"probe-low-power" has been defined for this purpose.
> 
> Well, if you say "has been defined", you need to provide a pointer to
> the definition document (a specification of some sort or similar).

How about:

s/has been defined/is used/

> 
> > +
> > +How it works
> > +============
> > +
> > +The property "probe-low-power" boolean property may be used to tell Linux
> > +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 in its struct i2c_driver.probe_low_power field and the
> > +"probe-low-power" 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.
> > diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst
> > index ad3b5afdae77e..4e4ac675c0ff8 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/probe-low-power
> >     enumeration
> >     osi
> >     method-customizing
> > --
> > 2.20.1
> >

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property
  2020-01-21 16:18     ` Sakari Ailus
@ 2020-01-21 16:56       ` Rafael J. Wysocki
  2020-01-21 16:58         ` Sakari Ailus
  0 siblings, 1 reply; 19+ messages in thread
From: Rafael J. Wysocki @ 2020-01-21 16:56 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Rafael J. Wysocki, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, Bingbu Cao, linux-media,
	Chiranjeevi Rapolu, Hyungwoo Yang, Bartosz Golaszewski,
	Arnd Bergmann, Linux Kernel Mailing List, Greg Kroah-Hartman,
	Mani, Rajmohan, Tomasz Figa

On Tue, Jan 21, 2020 at 5:19 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Tue, Jan 21, 2020 at 05:09:44PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Jan 21, 2020 at 2:41 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.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  .../acpi/dsd/probe-low-power.rst              | 28 +++++++++++++++++++
> > >  Documentation/firmware-guide/acpi/index.rst   |  1 +
> > >  2 files changed, 29 insertions(+)
> > >  create mode 100644 Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> > >
> > > diff --git a/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst b/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> > > new file mode 100644
> > > index 0000000000000..e0343ffefe071
> > > --- /dev/null
> > > +++ b/Documentation/firmware-guide/acpi/dsd/probe-low-power.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. The _DSD property
> > > +"probe-low-power" has been defined for this purpose.
> >
> > Well, if you say "has been defined", you need to provide a pointer to
> > the definition document (a specification of some sort or similar).
>
> How about:
>
> s/has been defined/is used/

I would say "Linux recognizes the ... that can be used for this purpose."

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property
  2020-01-21 16:56       ` Rafael J. Wysocki
@ 2020-01-21 16:58         ` Sakari Ailus
  0 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-01-21 16:58 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: linux-i2c, Wolfram Sang, ACPI Devel Maling List, Bingbu Cao,
	linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Bartosz Golaszewski, Arnd Bergmann, Linux Kernel Mailing List,
	Greg Kroah-Hartman, Mani, Rajmohan, Tomasz Figa

On Tue, Jan 21, 2020 at 05:56:39PM +0100, Rafael J. Wysocki wrote:
> On Tue, Jan 21, 2020 at 5:19 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Tue, Jan 21, 2020 at 05:09:44PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Jan 21, 2020 at 2:41 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.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  .../acpi/dsd/probe-low-power.rst              | 28 +++++++++++++++++++
> > > >  Documentation/firmware-guide/acpi/index.rst   |  1 +
> > > >  2 files changed, 29 insertions(+)
> > > >  create mode 100644 Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> > > >
> > > > diff --git a/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst b/Documentation/firmware-guide/acpi/dsd/probe-low-power.rst
> > > > new file mode 100644
> > > > index 0000000000000..e0343ffefe071
> > > > --- /dev/null
> > > > +++ b/Documentation/firmware-guide/acpi/dsd/probe-low-power.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. The _DSD property
> > > > +"probe-low-power" has been defined for this purpose.
> > >
> > > Well, if you say "has been defined", you need to provide a pointer to
> > > the definition document (a specification of some sort or similar).
> >
> > How about:
> >
> > s/has been defined/is used/
> 
> I would say "Linux recognizes the ... that can be used for this purpose."

Ack; I'll use this in v5.

-- 
Sakari Ailus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 5/6] at24: Support probing while off
  2020-01-21 13:41 ` [PATCH v4 5/6] at24: Support probing while off Sakari Ailus
@ 2020-01-29 13:36   ` Bartosz Golaszewski
  2020-03-11  8:55     ` Sakari Ailus
  0 siblings, 1 reply; 19+ messages in thread
From: Bartosz Golaszewski @ 2020-01-29 13:36 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	Bingbu Cao, linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Arnd Bergmann, LKML, Greg Kroah-Hartman, rajmohan.mani,
	Tomasz Figa

wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
>
> In certain use cases (where the chip is part of a camera module, and the
> camera module is wired together with a camera privacy LED), powering on
> the device during probe is undesirable. Add support for the at24 to
> execute probe while being powered off. For this to happen, a hint in form
> of a device property is required from the firmware.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
>  1 file changed, 21 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 0681d5fdd538a..5fc1162b67618 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -564,6 +564,7 @@ static int at24_probe(struct i2c_client *client)
>         bool i2c_fn_i2c, i2c_fn_block;
>         unsigned int i, num_addresses;
>         struct at24_data *at24;
> +       bool low_power;
>         struct regmap *regmap;
>         bool writable;
>         u8 test_byte;
> @@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
>
>         i2c_set_clientdata(client, at24);
>
> -       /* enable runtime pm */
> -       pm_runtime_set_active(dev);
> +       low_power = acpi_dev_state_low_power(&client->dev);
> +       if (!low_power)
> +               pm_runtime_set_active(dev);
> +
>         pm_runtime_enable(dev);
>
>         /*
> -        * Perform a one-byte test read to verify that the
> -        * chip is functional.
> +        * Perform a one-byte test read to verify that the chip is functional,
> +        * unless powering on the device is to be avoided during probe (i.e.
> +        * it's powered off right now).
>          */
> -       err = at24_read(at24, 0, &test_byte, 1);
> -       pm_runtime_idle(dev);
> -       if (err) {
> -               pm_runtime_disable(dev);
> -               return -ENODEV;
> +       if (!low_power) {
> +               err = at24_read(at24, 0, &test_byte, 1);
> +               pm_runtime_idle(dev);
> +               if (err) {
> +                       pm_runtime_disable(dev);
> +                       return -ENODEV;
> +               }
>         }
>
>         if (writable)
> @@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
>
>  static int at24_remove(struct i2c_client *client)
>  {
> +       bool low_power;
> +
>         pm_runtime_disable(&client->dev);
> -       pm_runtime_set_suspended(&client->dev);
> +       low_power = acpi_dev_state_low_power(&client->dev);

This is inconsistent. You define the low_power field in the context
structure (BTW the name low_power is a bit vague here - without
looking at its assignment it would make me think it's about something
battery-related, how about 'off_at_probe'?) and instead of reusing
this field here, you call acpi_dev_state_low_power() again. Either
don't store the context for the life-time of the device if not
necessary or don't call acpi_dev_state_low_power() at remove, although
the commit message doesn't describe whether the latter is done on
purpose.

Bartosz

> +       if (!low_power)
> +               pm_runtime_set_suspended(&client->dev);
>
>         return 0;
>  }
> @@ -743,6 +753,7 @@ static struct i2c_driver at24_driver = {
>         .probe_new = at24_probe,
>         .remove = at24_remove,
>         .id_table = at24_ids,
> +       .probe_low_power = true,
>  };
>
>  static int __init at24_init(void)
> --
> 2.20.1
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe
  2020-01-21 13:41 ` [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe Sakari Ailus
@ 2020-01-29 13:54   ` Bartosz Golaszewski
  2020-03-23 21:36     ` Sakari Ailus
  0 siblings, 1 reply; 19+ messages in thread
From: Bartosz Golaszewski @ 2020-01-29 13:54 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	Bingbu Cao, linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Arnd Bergmann, LKML, Greg Kroah-Hartman, rajmohan.mani,
	Tomasz Figa

wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> 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 <sakari.ailus@linux.intel.com>
> ---
>  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
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 5/6] at24: Support probing while off
  2020-01-29 13:36   ` Bartosz Golaszewski
@ 2020-03-11  8:55     ` Sakari Ailus
  2020-03-12 13:10       ` Bartosz Golaszewski
  0 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-03-11  8:55 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	Bingbu Cao, linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Arnd Bergmann, LKML, Greg Kroah-Hartman, rajmohan.mani,
	Tomasz Figa

Hi Bartosz,

Thanks for the reply.

On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> >
> > In certain use cases (where the chip is part of a camera module, and the
> > camera module is wired together with a camera privacy LED), powering on
> > the device during probe is undesirable. Add support for the at24 to
> > execute probe while being powered off. For this to happen, a hint in form
> > of a device property is required from the firmware.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> >  1 file changed, 21 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 0681d5fdd538a..5fc1162b67618 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -564,6 +564,7 @@ static int at24_probe(struct i2c_client *client)
> >         bool i2c_fn_i2c, i2c_fn_block;
> >         unsigned int i, num_addresses;
> >         struct at24_data *at24;
> > +       bool low_power;
> >         struct regmap *regmap;
> >         bool writable;
> >         u8 test_byte;
> > @@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
> >
> >         i2c_set_clientdata(client, at24);
> >
> > -       /* enable runtime pm */
> > -       pm_runtime_set_active(dev);
> > +       low_power = acpi_dev_state_low_power(&client->dev);
> > +       if (!low_power)
> > +               pm_runtime_set_active(dev);
> > +
> >         pm_runtime_enable(dev);
> >
> >         /*
> > -        * Perform a one-byte test read to verify that the
> > -        * chip is functional.
> > +        * Perform a one-byte test read to verify that the chip is functional,
> > +        * unless powering on the device is to be avoided during probe (i.e.
> > +        * it's powered off right now).
> >          */
> > -       err = at24_read(at24, 0, &test_byte, 1);
> > -       pm_runtime_idle(dev);
> > -       if (err) {
> > -               pm_runtime_disable(dev);
> > -               return -ENODEV;
> > +       if (!low_power) {
> > +               err = at24_read(at24, 0, &test_byte, 1);
> > +               pm_runtime_idle(dev);
> > +               if (err) {
> > +                       pm_runtime_disable(dev);
> > +                       return -ENODEV;
> > +               }
> >         }
> >
> >         if (writable)
> > @@ -728,8 +734,12 @@ static int at24_probe(struct i2c_client *client)
> >
> >  static int at24_remove(struct i2c_client *client)
> >  {
> > +       bool low_power;
> > +
> >         pm_runtime_disable(&client->dev);
> > -       pm_runtime_set_suspended(&client->dev);
> > +       low_power = acpi_dev_state_low_power(&client->dev);
> 
> This is inconsistent. You define the low_power field in the context
> structure (BTW the name low_power is a bit vague here - without
> looking at its assignment it would make me think it's about something
> battery-related, how about 'off_at_probe'?) and instead of reusing

The field was called probe_powered_off in v1, but I changed it to
probe_low_power (and renamed related functions etc.) based on review
comments --- for the device may not be powered off actually.

> this field here, you call acpi_dev_state_low_power() again. Either
> don't store the context for the life-time of the device if not
> necessary or don't call acpi_dev_state_low_power() at remove, although
> the commit message doesn't describe whether the latter is done on
> purpose.

Right. probe-low-power property has the same effect on remove for
consistency, i.e. the device can remain in low power state during remove.
This is documented in probe_low_power field documentation in the first
patch.

-- 
Regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 5/6] at24: Support probing while off
  2020-03-11  8:55     ` Sakari Ailus
@ 2020-03-12 13:10       ` Bartosz Golaszewski
  2020-03-23 21:31         ` Sakari Ailus
  0 siblings, 1 reply; 19+ messages in thread
From: Bartosz Golaszewski @ 2020-03-12 13:10 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	Bingbu Cao, linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Arnd Bergmann, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa

śr., 11 mar 2020 o 09:56 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
>
> Hi Bartosz,
>
> Thanks for the reply.
>
> On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> > >
> > > In certain use cases (where the chip is part of a camera module, and the
> > > camera module is wired together with a camera privacy LED), powering on
> > > the device during probe is undesirable. Add support for the at24 to
> > > execute probe while being powered off. For this to happen, a hint in form
> > > of a device property is required from the firmware.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------

[snip!]

> > >
> > >  static int at24_remove(struct i2c_client *client)
> > >  {
> > > +       bool low_power;
> > > +
> > >         pm_runtime_disable(&client->dev);
> > > -       pm_runtime_set_suspended(&client->dev);
> > > +       low_power = acpi_dev_state_low_power(&client->dev);
> >
> > This is inconsistent. You define the low_power field in the context
> > structure (BTW the name low_power is a bit vague here - without
> > looking at its assignment it would make me think it's about something
> > battery-related, how about 'off_at_probe'?) and instead of reusing
>
> The field was called probe_powered_off in v1, but I changed it to
> probe_low_power (and renamed related functions etc.) based on review
> comments --- for the device may not be powered off actually.
>

But is it actually ever low-power? What are the possible logical
states of the device? If I understood correctly: it's either off or on
at probe - not actually low-power. Am I missing something? In your
cover letter you're writing: "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." To me there's no mention of a low-power state,
which makes the name 'probe_low_power' seem completely unrelated.

> > this field here, you call acpi_dev_state_low_power() again. Either
> > don't store the context for the life-time of the device if not
> > necessary or don't call acpi_dev_state_low_power() at remove, although
> > the commit message doesn't describe whether the latter is done on
> > purpose.
>
> Right. probe-low-power property has the same effect on remove for
> consistency, i.e. the device can remain in low power state during remove.
> This is documented in probe_low_power field documentation in the first
> patch.
>

Just please don't store any state if you're not using it outside of
the probe() function.

Bartosz

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 5/6] at24: Support probing while off
  2020-03-12 13:10       ` Bartosz Golaszewski
@ 2020-03-23 21:31         ` Sakari Ailus
  2020-03-25 13:48           ` Bartosz Golaszewski
  0 siblings, 1 reply; 19+ messages in thread
From: Sakari Ailus @ 2020-03-23 21:31 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	Bingbu Cao, linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Arnd Bergmann, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa

Bartosz,

On Thu, Mar 12, 2020 at 02:10:32PM +0100, Bartosz Golaszewski wrote:
> śr., 11 mar 2020 o 09:56 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> >
> > Hi Bartosz,
> >
> > Thanks for the reply.
> >
> > On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > > wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> > > >
> > > > In certain use cases (where the chip is part of a camera module, and the
> > > > camera module is wired together with a camera privacy LED), powering on
> > > > the device during probe is undesirable. Add support for the at24 to
> > > > execute probe while being powered off. For this to happen, a hint in form
> > > > of a device property is required from the firmware.
> > > >
> > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> 
> [snip!]
> 
> > > >
> > > >  static int at24_remove(struct i2c_client *client)
> > > >  {
> > > > +       bool low_power;
> > > > +
> > > >         pm_runtime_disable(&client->dev);
> > > > -       pm_runtime_set_suspended(&client->dev);
> > > > +       low_power = acpi_dev_state_low_power(&client->dev);
> > >
> > > This is inconsistent. You define the low_power field in the context
> > > structure (BTW the name low_power is a bit vague here - without
> > > looking at its assignment it would make me think it's about something
> > > battery-related, how about 'off_at_probe'?) and instead of reusing
> >
> > The field was called probe_powered_off in v1, but I changed it to
> > probe_low_power (and renamed related functions etc.) based on review
> > comments --- for the device may not be powered off actually.
> >
> 
> But is it actually ever low-power? What are the possible logical
> states of the device? If I understood correctly: it's either off or on
> at probe - not actually low-power. Am I missing something? In your
> cover letter you're writing: "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." To me there's no mention of a low-power state,
> which makes the name 'probe_low_power' seem completely unrelated.

See <URL:https://patchwork.kernel.org/patch/10938483/>

I've updated the patches according to the comments but did not update the
cover page accordingly.

Generally drivers are interested whether a device is powered on so it can
be accessed, but the actual power state of the device isn't known to the
driver when it is, well, not in an operational state. A device may be
powered from a regulator that is always enabled, for instance.

> 
> > > this field here, you call acpi_dev_state_low_power() again. Either
> > > don't store the context for the life-time of the device if not
> > > necessary or don't call acpi_dev_state_low_power() at remove, although
> > > the commit message doesn't describe whether the latter is done on
> > > purpose.
> >
> > Right. probe-low-power property has the same effect on remove for
> > consistency, i.e. the device can remain in low power state during remove.
> > This is documented in probe_low_power field documentation in the first
> > patch.
> >
> 
> Just please don't store any state if you're not using it outside of
> the probe() function.

What exactly are you referring to? The patch adds a local variable to the
driver's probe and remove functions.

-- 
Kind regards,

Sakari Ailus

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe
  2020-01-29 13:54   ` Bartosz Golaszewski
@ 2020-03-23 21:36     ` Sakari Ailus
  0 siblings, 0 replies; 19+ messages in thread
From: Sakari Ailus @ 2020-03-23 21:36 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	Bingbu Cao, linux-media, Chiranjeevi Rapolu, Hyungwoo Yang,
	Arnd Bergmann, LKML, Greg Kroah-Hartman, rajmohan.mani,
	Tomasz Figa

Hi Bartosz,

On Wed, Jan 29, 2020 at 02:54:20PM +0100, Bartosz Golaszewski wrote:
> wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> 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 <sakari.ailus@linux.intel.com>
> > ---
> >  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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH v4 5/6] at24: Support probing while off
  2020-03-23 21:31         ` Sakari Ailus
@ 2020-03-25 13:48           ` Bartosz Golaszewski
  0 siblings, 0 replies; 19+ messages in thread
From: Bartosz Golaszewski @ 2020-03-25 13:48 UTC (permalink / raw)
  To: Sakari Ailus, Rafael J. Wysocki
  Cc: linux-i2c, Wolfram Sang, linux-acpi, Bingbu Cao, linux-media,
	Chiranjeevi Rapolu, Hyungwoo Yang, Arnd Bergmann, LKML,
	Greg Kroah-Hartman, Rajmohan Mani, Tomasz Figa

pon., 23 mar 2020 o 22:31 Sakari Ailus <sakari.ailus@linux.intel.com>
napisał(a):
>
> Bartosz,
>
> On Thu, Mar 12, 2020 at 02:10:32PM +0100, Bartosz Golaszewski wrote:
> > śr., 11 mar 2020 o 09:56 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> > >
> > > Hi Bartosz,
> > >
> > > Thanks for the reply.
> > >
> > > On Wed, Jan 29, 2020 at 02:36:17PM +0100, Bartosz Golaszewski wrote:
> > > > wt., 21 sty 2020 o 14:41 Sakari Ailus <sakari.ailus@linux.intel.com> napisał(a):
> > > > >
> > > > > In certain use cases (where the chip is part of a camera module, and the
> > > > > camera module is wired together with a camera privacy LED), powering on
> > > > > the device during probe is undesirable. Add support for the at24 to
> > > > > execute probe while being powered off. For this to happen, a hint in form
> > > > > of a device property is required from the firmware.
> > > > >
> > > > > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/misc/eeprom/at24.c | 31 +++++++++++++++++++++----------
> >
> > [snip!]
> >
> > > > >
> > > > >  static int at24_remove(struct i2c_client *client)
> > > > >  {
> > > > > +       bool low_power;
> > > > > +
> > > > >         pm_runtime_disable(&client->dev);
> > > > > -       pm_runtime_set_suspended(&client->dev);
> > > > > +       low_power = acpi_dev_state_low_power(&client->dev);
> > > >
> > > > This is inconsistent. You define the low_power field in the context
> > > > structure (BTW the name low_power is a bit vague here - without
> > > > looking at its assignment it would make me think it's about something
> > > > battery-related, how about 'off_at_probe'?) and instead of reusing
> > >
> > > The field was called probe_powered_off in v1, but I changed it to
> > > probe_low_power (and renamed related functions etc.) based on review
> > > comments --- for the device may not be powered off actually.
> > >
> >
> > But is it actually ever low-power? What are the possible logical
> > states of the device? If I understood correctly: it's either off or on
> > at probe - not actually low-power. Am I missing something? In your
> > cover letter you're writing: "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." To me there's no mention of a low-power state,
> > which makes the name 'probe_low_power' seem completely unrelated.
>
> See <URL:https://patchwork.kernel.org/patch/10938483/>
>
> I've updated the patches according to the comments but did not update the
> cover page accordingly.
>

I see.

Rafael: I think that there are two issues with patch 1/5:
1. It adds a very specific boolean flag to a structure that's meant to
be very general. As I pointed out in the i2c patch: at the very least
this could be made into an int storing flag values, instead of a
boolean field. But rather than that - it looks to me more like a
device (or bus) feature than a driver feature. Is there any ACPI flag
we could use to pass this information to the driver model without
changing the driver structure?
2. The name is still misleading: probe_low_power doesn't correspond
with what it actually does at all (neither did power_off). I'd go with
something like probe_allow_low_power.

> Generally drivers are interested whether a device is powered on so it can
> be accessed, but the actual power state of the device isn't known to the
> driver when it is, well, not in an operational state. A device may be
> powered from a regulator that is always enabled, for instance.
>
> >
> > > > this field here, you call acpi_dev_state_low_power() again. Either
> > > > don't store the context for the life-time of the device if not
> > > > necessary or don't call acpi_dev_state_low_power() at remove, although
> > > > the commit message doesn't describe whether the latter is done on
> > > > purpose.
> > >
> > > Right. probe-low-power property has the same effect on remove for
> > > consistency, i.e. the device can remain in low power state during remove.
> > > This is documented in probe_low_power field documentation in the first
> > > patch.
> > >
> >
> > Just please don't store any state if you're not using it outside of
> > the probe() function.
>
> What exactly are you referring to? The patch adds a local variable to the
> driver's probe and remove functions.
>

Yes, sorry, I looked at the patch and somehow thought it adds a new
field to the data structure and then doesn't reuse it. My bad. Maybe
it was a previous version IDK.


Bartosz

> --
> Kind regards,
>
> Sakari Ailus

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, back to index

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-21 13:41 [PATCH v4 0/6] Support running driver's probe for a device powered off Sakari Ailus
2020-01-21 13:41 ` [PATCH v4 1/6] i2c: Allow driver to manage the device's power state during probe Sakari Ailus
2020-01-29 13:54   ` Bartosz Golaszewski
2020-03-23 21:36     ` Sakari Ailus
2020-01-21 13:41 ` [PATCH v4 2/6] ACPI: Add a convenience function to tell a device is in low power state Sakari Ailus
2020-01-21 16:07   ` Rafael J. Wysocki
2020-01-21 13:41 ` [PATCH v4 3/6] ov5670: Support probe whilst the device is in a " Sakari Ailus
2020-01-21 13:41 ` [PATCH v4 4/6] media: i2c: imx319: Support probe while the device is off Sakari Ailus
2020-01-21 13:41 ` [PATCH v4 5/6] at24: Support probing while off Sakari Ailus
2020-01-29 13:36   ` Bartosz Golaszewski
2020-03-11  8:55     ` Sakari Ailus
2020-03-12 13:10       ` Bartosz Golaszewski
2020-03-23 21:31         ` Sakari Ailus
2020-03-25 13:48           ` Bartosz Golaszewski
2020-01-21 13:41 ` [PATCH v4 6/6] Documentation: ACPI: Document probe-low-power _DSD property Sakari Ailus
2020-01-21 16:09   ` Rafael J. Wysocki
2020-01-21 16:18     ` Sakari Ailus
2020-01-21 16:56       ` Rafael J. Wysocki
2020-01-21 16:58         ` Sakari Ailus

Linux-ACPI Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-acpi/0 linux-acpi/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-acpi linux-acpi/ https://lore.kernel.org/linux-acpi \
		linux-acpi@vger.kernel.org
	public-inbox-index linux-acpi

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-acpi


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git