All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v10 0/7] Support running driver's probe for a device powered off
@ 2021-02-05 13:24 Sakari Ailus
  2021-02-05 13:24 ` [PATCH v10 1/7] ACPI: scan: Obtain device's desired enumeration power state Sakari Ailus
                   ` (6 more replies)
  0 siblings, 7 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:24 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

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 v9 <URL:https://lore.kernel.org/linux-acpi/CAJZ5v0jjgy2KXOw5pyshvaEVzLctu4jsgYK1+YDA+8sZfp-6mw@mail.gmail.com/T/#m003f56b33350772364b1f5c832e9025677107933>:

- Use _DSE object designed for the very purpose of designating intended
  device probe power state instead of _PRE.

- Rework documentation to reflect the property to _DSE changes (missed in
  v9).

- Put maximum device enumeration power state in struct acpi_device_power,
  instead of a flag in struct acpi_device_power_flags. The default is
  ACPI_STATE_D0.

- i2c_acpi_allow_low_power_probe() now returns true if the desired power
  state is greater or equal to the current device power state.

- Rename local variable low_power as off_during_probe.

since v8 <URL:https://lore.kernel.org/patchwork/cover/1300068/>:

- Make use of ACPI _PRE object instead of a _DSD property (new patch,
  1st), align documentation with that.

- Added a blank line.

- Rebased on current linux-next master.

since v7 <URL:https://lore.kernel.org/linux-acpi/20200901210333.8462-1-sakari.ailus@linux.intel.com/>:

- Reorder documentation patch right after the implemenation in the I²C
  framework.

- Rename allow-low-power-probe property as i2c-allow-low-power-probe.

- Remove extra "property" from the description of the
  i2c-allow-low-power-probe property and mention it's a device property.

- Add an example to the documentation and refer to the _DSD property spec.

since v6 <URL:https://lore.kernel.org/linux-acpi/20200826115432.6103-1-sakari.ailus@linux.intel.com/>:

- Use u32 for the flags field in struct i2c_driver.

- Use acpi_dev_get_property to read the allow-low-power-probe property.

since v5 <URL:https://lore.kernel.org/linux-acpi/20200810142747.12400-1-sakari.ailus@linux.intel.com/>:

- Identify sensors when they're first powered on. In previous versions, if
  this wasn't in probe, it was not done at all.

- Return allow_low_power_probe() only for ACPI devices, i.e. OF systems
  are not affected by these changes.

- Document that I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag only applies to ACPI
  drivers.

- Fix extra regulator_disable in at24 driver's remove function when the
  device was already in low power state.

since v4 <URL:https://lore.kernel.org/linux-acpi/20200121134157.20396-1-sakari.ailus@linux.intel.com/>:

- Rename "probe-low-power" property as "allow-low-power-probe". This is
  taken into account in function and file naming, too.

- Turn probe_low_power field in struct i2c_driver into flags field.

- Rebase on Wolfram's i2c/for-next branch that contains the removal of the
  support for disabling I²C core IRQ mappings (commit
  0c2a34937f7e4c4776bb261114c475392da2355c).

- Change wording for "allow-low-power-probe" property in ACPI
  documentation.

since v3 <URL:https://lore.kernel.org/linux-acpi/20200109154529.19484-1-sakari.ailus@linux.intel.com/T/#t>:

- Rework the 2nd patch based on Rafael's comments

	- Rework description of the ACPI low power state helper function,
	  according to Rafael's text.

	- Rename and rework the same function as
	  acpi_dev_state_low_power().

	- Reflect the changes in commit message as well.

- Added a patch to document the probe-low-power _DSD property.

since v2 <URL:https://patchwork.kernel.org/cover/11114255/>:

- Remove extra CONFIG_PM ifdefs; these are not needed.

- Move the checks for power state hints from drivers/base/dd.c to
  drivers/i2c/i2c-base-core.c; these are I²C devices anyway.

- Move the probe_low_power field from struct device_driver to struct
  i2c_driver.

since v1:

- Rename probe_powered_off struct device field as probe_low_power and
  reflect the similar naming to the patches overall.

- Work with CONFIG_PM disabled, too.

Rajmohan Mani (1):
  media: i2c: imx319: Support probe while the device is off

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 low power
    state
  ov5670: Support probe whilst the device is in a low power state
  at24: Support probing while off

 Documentation/firmware-guide/acpi/index.rst   |  1 +
 .../firmware-guide/acpi/low-power-probe.rst   | 69 +++++++++++++++++
 drivers/acpi/device_pm.c                      | 23 ++++++
 drivers/acpi/scan.c                           |  4 +
 drivers/i2c/i2c-core-acpi.c                   | 10 +++
 drivers/i2c/i2c-core-base.c                   |  9 ++-
 drivers/media/i2c/imx319.c                    | 72 +++++++++++-------
 drivers/media/i2c/ov5670.c                    | 76 +++++++++++--------
 drivers/misc/eeprom/at24.c                    | 43 ++++++-----
 include/acpi/acpi_bus.h                       |  1 +
 include/linux/acpi.h                          |  6 ++
 include/linux/i2c.h                           | 19 +++++
 12 files changed, 255 insertions(+), 78 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/low-power-probe.rst

-- 
2.20.1


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

* [PATCH v10 1/7] ACPI: scan: Obtain device's desired enumeration power state
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
@ 2021-02-05 13:24 ` Sakari Ailus
  2021-02-05 13:25 ` [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe Sakari Ailus
                   ` (5 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:24 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

Store a device's desired enumeration power state in struct
acpi_device_power during acpi_device object's initialisation.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/scan.c     | 4 ++++
 include/acpi/acpi_bus.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
index 1d7a02ee45e05..cdff32f297122 100644
--- a/drivers/acpi/scan.c
+++ b/drivers/acpi/scan.c
@@ -987,6 +987,7 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
 
 static void acpi_bus_get_power_flags(struct acpi_device *device)
 {
+	unsigned long long dse = ACPI_STATE_D0;
 	u32 i;
 
 	/* Presence of _PS0|_PR0 indicates 'power manageable' */
@@ -1008,6 +1009,9 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
 	if (acpi_has_method(device->handle, "_DSW"))
 		device->power.flags.dsw_present = 1;
 
+	acpi_evaluate_integer(device->handle, "_DSE", NULL, &dse);
+	device->power.state_for_enumeration = dse;
+
 	/*
 	 * Enumerate supported power management states
 	 */
diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
index 02a716a0af5d4..becfc9f57002b 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -276,6 +276,7 @@ struct acpi_device_power {
 	int state;		/* Current state */
 	struct acpi_device_power_flags flags;
 	struct acpi_device_power_state states[ACPI_D_STATE_COUNT];	/* Power states (D0-D3Cold) */
+	u8 state_for_enumeration; /* Maximum power state for enumeration */
 };
 
 /* Performance Management */
-- 
2.20.1


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

* [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
  2021-02-05 13:24 ` [PATCH v10 1/7] ACPI: scan: Obtain device's desired enumeration power state Sakari Ailus
@ 2021-02-05 13:25 ` Sakari Ailus
  2021-02-09 21:04   ` Wolfram Sang
  2021-02-05 13:25 ` [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state Sakari Ailus
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:25 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

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.

Add a field called "flags" into struct i2c_driver for driver flags, and a
flag I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to tell a driver supports probe in
low power state.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/i2c/i2c-core-acpi.c | 10 ++++++++++
 drivers/i2c/i2c-core-base.c |  9 ++++++---
 include/linux/i2c.h         | 19 +++++++++++++++++++
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-acpi.c b/drivers/i2c/i2c-core-acpi.c
index 8ceaa88dd78fb..7bab8b9126ee6 100644
--- a/drivers/i2c/i2c-core-acpi.c
+++ b/drivers/i2c/i2c-core-acpi.c
@@ -493,6 +493,16 @@ struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 }
 EXPORT_SYMBOL_GPL(i2c_acpi_new_device);
 
+bool i2c_acpi_allow_low_power_probe(struct device *dev)
+{
+	struct i2c_driver *driver = to_i2c_driver(dev->driver);
+	struct acpi_device *adev = ACPI_COMPANION(dev);
+
+	return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE &&
+		adev && adev->power.state_for_enumeration >= adev->power.state;
+}
+EXPORT_SYMBOL_GPL(i2c_acpi_allow_low_power_probe);
+
 #ifdef CONFIG_ACPI_I2C_OPREGION
 static int acpi_gsb_i2c_read_bytes(struct i2c_client *client,
 		u8 cmd, u8 *data, u8 data_len)
diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 63ebf722a4248..87b84eee01da6 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -514,7 +514,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,
+				      !i2c_acpi_allow_low_power_probe(dev));
 	if (status)
 		goto err_clear_wakeup_irq;
 
@@ -536,7 +537,8 @@ 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,
+			     !i2c_acpi_allow_low_power_probe(dev));
 err_clear_wakeup_irq:
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -563,7 +565,8 @@ static int i2c_device_remove(struct device *dev)
 			dev_warn(dev, "remove failed (%pe), will be ignored\n", ERR_PTR(status));
 	}
 
-	dev_pm_domain_detach(&client->dev, true);
+	dev_pm_domain_detach(&client->dev,
+			     !i2c_acpi_allow_low_power_probe(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 56622658b2158..1a103c5933d2f 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -11,6 +11,7 @@
 #define _LINUX_I2C_H
 
 #include <linux/acpi.h>		/* for acpi_handle */
+#include <linux/bits.h>
 #include <linux/mod_devicetable.h>
 #include <linux/device.h>	/* for struct device */
 #include <linux/sched.h>	/* for completion */
@@ -217,6 +218,16 @@ enum i2c_alert_protocol {
 	I2C_PROTOCOL_SMBUS_HOST_NOTIFY,
 };
 
+/**
+ * enum i2c_driver_flags - Flags for an I2C device driver
+ *
+ * @I2C_DRV_FL_ALLOW_LOW_POWER_PROBE: Let the ACPI driver manage the device's
+ *				      power state during probe and remove
+ */
+enum i2c_driver_flags {
+	I2C_DRV_FL_ALLOW_LOW_POWER_PROBE = BIT(0),
+};
+
 /**
  * struct i2c_driver - represent an I2C device driver
  * @class: What kind of i2c device we instantiate (for detect)
@@ -231,6 +242,7 @@ enum i2c_alert_protocol {
  * @detect: Callback for device detection
  * @address_list: The I2C addresses to probe (for detect)
  * @clients: List of detected clients we created (for i2c-core use only)
+ * @flags: A bitmask of flags defined in &enum i2c_driver_flags
  *
  * 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 +301,8 @@ struct i2c_driver {
 	int (*detect)(struct i2c_client *client, struct i2c_board_info *info);
 	const unsigned short *address_list;
 	struct list_head clients;
+
+	u32 flags;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
@@ -996,6 +1010,7 @@ u32 i2c_acpi_find_bus_speed(struct device *dev);
 struct i2c_client *i2c_acpi_new_device(struct device *dev, int index,
 				       struct i2c_board_info *info);
 struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle handle);
+bool i2c_acpi_allow_low_power_probe(struct device *dev);
 #else
 static inline bool i2c_acpi_get_i2c_resource(struct acpi_resource *ares,
 					     struct acpi_resource_i2c_serialbus **i2c)
@@ -1015,6 +1030,10 @@ static inline struct i2c_adapter *i2c_acpi_find_adapter_by_handle(acpi_handle ha
 {
 	return NULL;
 }
+static inline bool i2c_acpi_allow_low_power_probe(struct device *dev)
+{
+	return false;
+}
 #endif /* CONFIG_ACPI */
 
 #endif /* _LINUX_I2C_H */
-- 
2.20.1


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

* [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
  2021-02-05 13:24 ` [PATCH v10 1/7] ACPI: scan: Obtain device's desired enumeration power state Sakari Ailus
  2021-02-05 13:25 ` [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe Sakari Ailus
@ 2021-02-05 13:25 ` Sakari Ailus
  2021-02-06  0:56   ` Randy Dunlap
  2021-02-05 13:25 ` [PATCH v10 4/7] ACPI: Add a convenience function to tell a device is in low " Sakari Ailus
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:25 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

Document the use of the _DSE object for setting desirable power state
during probe.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 Documentation/firmware-guide/acpi/index.rst   |  1 +
 .../firmware-guide/acpi/low-power-probe.rst   | 69 +++++++++++++++++++
 2 files changed, 70 insertions(+)
 create mode 100644 Documentation/firmware-guide/acpi/low-power-probe.rst

diff --git a/Documentation/firmware-guide/acpi/index.rst b/Documentation/firmware-guide/acpi/index.rst
index f72b5f1769fb2..d02712acccbc0 100644
--- a/Documentation/firmware-guide/acpi/index.rst
+++ b/Documentation/firmware-guide/acpi/index.rst
@@ -25,5 +25,6 @@ ACPI Support
    acpi-lid
    lpit
    video_extension
+   low-power-probe
    extcon-intel-int3496
    intel-pmc-mux
diff --git a/Documentation/firmware-guide/acpi/low-power-probe.rst b/Documentation/firmware-guide/acpi/low-power-probe.rst
new file mode 100644
index 0000000000000..b96804d959a6c
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/low-power-probe.rst
@@ -0,0 +1,69 @@
+.. 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.
+
+How it works
+============
+
+The _DSE (Device State for Enumeration) object that evaluates to integer 0 may
+be used to tell Linux the highest allowed D state for a device during probe. If
+the driver indicates its support for this by setting the
+I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags field and the
+_DSE object evaluates to integer higher than the D state of the device, the
+device will not be powered on (put in D0 state) for probe.
+
+The D states and thus also the allowed values for _DSE are listed below. Refer
+to [1] for more information on device power states.
+
+.. code-block:: text
+
+	Number	State	Description
+	0	D0	Device fully powered on
+	1	D1
+	2	D2
+	3	D3hot
+	4	D3cold	Off
+
+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.
+
+References
+==========
+
+[1] https://uefi.org/specifications/ACPI/6.4/02_Definition_of_Terms/Definition_of_Terms.html#device-power-state-definitions
+
+Example
+=======
+
+An ASL example describing an ACPI device using _DSE object to tell Operating
+System the device should remain powered off during probe looks like this. Some
+objects not relevant from the example point of view have been omitted.
+
+.. code-block:: text
+
+	Device (CAM0)
+        {
+		Name (_HID, "SONY319A")
+		Name (_UID, Zero)
+		Name (_CRS, ResourceTemplate ()
+		{
+			I2cSerialBus(0x0020, ControllerInitiated, 0x00061A80,
+				     AddressingMode7Bit, "\\_SB.PCI0.I2C0",
+				     0x00, ResourceConsumer)
+		})
+		Name (_DSE, 0, NotSerialized)
+		{
+			Return (0x4)
+                }
+	}
-- 
2.20.1


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

* [PATCH v10 4/7] ACPI: Add a convenience function to tell a device is in low power state
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
                   ` (2 preceding siblings ...)
  2021-02-05 13:25 ` [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state Sakari Ailus
@ 2021-02-05 13:25 ` Sakari Ailus
  2021-02-05 13:25 ` [PATCH v10 5/7] ov5670: Support probe whilst the device is in a " Sakari Ailus
                   ` (2 subsequent siblings)
  6 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:25 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

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>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/acpi/device_pm.c | 23 +++++++++++++++++++++++
 include/linux/acpi.h     |  6 ++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index 096153761ebc3..5a58be779a8cd 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1336,4 +1336,27 @@ 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);
+
+	if (!adev)
+		return false;
+
+	return adev->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 4703daafcce9e..864786adb567c 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -982,6 +982,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_subsys_runtime_suspend(struct device *dev) { return 0; }
 static inline int acpi_subsys_runtime_resume(struct device *dev) { return 0; }
@@ -989,6 +990,11 @@ 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 related	[flat|nested] 25+ messages in thread

* [PATCH v10 5/7] ov5670: Support probe whilst the device is in a low power state
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
                   ` (3 preceding siblings ...)
  2021-02-05 13:25 ` [PATCH v10 4/7] ACPI: Add a convenience function to tell a device is in low " Sakari Ailus
@ 2021-02-05 13:25 ` Sakari Ailus
  2021-02-05 13:25 ` [PATCH v10 6/7] media: i2c: imx319: Support probe while the device is off Sakari Ailus
  2021-02-05 13:25 ` [PATCH v10 7/7] at24: Support probing while off Sakari Ailus
  6 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:25 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

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.

Also do identification on the first access of the device, whether in probe
or when starting streaming.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/media/i2c/ov5670.c | 76 +++++++++++++++++++++++---------------
 1 file changed, 46 insertions(+), 30 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 866c8c2e8f59a..eb0d599f3318b 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -1832,6 +1832,8 @@ struct ov5670 {
 
 	/* Streaming on/off */
 	bool streaming;
+	/* True if the device has been identified */
+	bool identified;
 };
 
 #define to_ov5670(_sd)	container_of(_sd, struct ov5670, sd)
@@ -2271,6 +2273,32 @@ static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
 	return 0;
 }
 
+/* Verify chip ID */
+static int ov5670_identify_module(struct ov5670 *ov5670)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
+	int ret;
+	u32 val;
+
+	if (ov5670->identified)
+		return 0;
+
+	ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
+			      OV5670_REG_VALUE_24BIT, &val);
+	if (ret)
+		return ret;
+
+	if (val != OV5670_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
+			OV5670_CHIP_ID, val);
+		return -ENXIO;
+	}
+
+	ov5670->identified = true;
+
+	return 0;
+}
+
 /* Prepare streaming by writing default values and customized values */
 static int ov5670_start_streaming(struct ov5670 *ov5670)
 {
@@ -2279,6 +2307,10 @@ static int ov5670_start_streaming(struct ov5670 *ov5670)
 	int link_freq_index;
 	int ret;
 
+	ret = ov5670_identify_module(ov5670);
+	if (ret)
+		return ret;
+
 	/* Get out of from software reset */
 	ret = ov5670_write_reg(ov5670, OV5670_REG_SOFTWARE_RST,
 			       OV5670_REG_VALUE_08BIT, OV5670_SOFTWARE_RST);
@@ -2400,27 +2432,6 @@ static int __maybe_unused ov5670_resume(struct device *dev)
 	return 0;
 }
 
-/* Verify chip ID */
-static int ov5670_identify_module(struct ov5670 *ov5670)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
-	int ret;
-	u32 val;
-
-	ret = ov5670_read_reg(ov5670, OV5670_REG_CHIP_ID,
-			      OV5670_REG_VALUE_24BIT, &val);
-	if (ret)
-		return ret;
-
-	if (val != OV5670_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x\n",
-			OV5670_CHIP_ID, val);
-		return -ENXIO;
-	}
-
-	return 0;
-}
-
 static const struct v4l2_subdev_video_ops ov5670_video_ops = {
 	.s_stream = ov5670_set_stream,
 };
@@ -2455,6 +2466,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);
@@ -2471,11 +2483,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);
@@ -2512,10 +2527,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);
 
@@ -2557,7 +2572,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 */ }
 };
 
@@ -2572,6 +2587,7 @@ static struct i2c_driver ov5670_i2c_driver = {
 	},
 	.probe_new = ov5670_probe,
 	.remove = ov5670_remove,
+	.flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
 };
 
 module_i2c_driver(ov5670_i2c_driver);
-- 
2.20.1


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

* [PATCH v10 6/7] media: i2c: imx319: Support probe while the device is off
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
                   ` (4 preceding siblings ...)
  2021-02-05 13:25 ` [PATCH v10 5/7] ov5670: Support probe whilst the device is in a " Sakari Ailus
@ 2021-02-05 13:25 ` Sakari Ailus
  2021-02-05 13:25 ` [PATCH v10 7/7] at24: Support probing while off Sakari Ailus
  6 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:25 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

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>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
Reviewed-by: Bingbu Cao <bingbu.cao@intel.com>
---
 drivers/media/i2c/imx319.c | 72 +++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 28 deletions(-)

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 8473c0bbb35d6..e0b22e9318fed 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 low_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;
+	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);
@@ -2491,10 +2506,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);
 
@@ -2547,6 +2562,7 @@ static struct i2c_driver imx319_i2c_driver = {
 	},
 	.probe_new = imx319_probe,
 	.remove = imx319_remove,
+	.flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
 };
 module_i2c_driver(imx319_i2c_driver);
 
-- 
2.20.1


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

* [PATCH v10 7/7] at24: Support probing while off
  2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
                   ` (5 preceding siblings ...)
  2021-02-05 13:25 ` [PATCH v10 6/7] media: i2c: imx319: Support probe while the device is off Sakari Ailus
@ 2021-02-05 13:25 ` Sakari Ailus
  2021-02-08 16:44   ` Bartosz Golaszewski
  6 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-05 13:25 UTC (permalink / raw)
  To: linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

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>
Reviewed-by: Tomasz Figa <tfiga@chromium.org>
---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 926408b41270c..69a5e4023d9e1 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -595,6 +595,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 off_during_probe;
 	struct regmap *regmap;
 	bool writable;
 	u8 test_byte;
@@ -750,14 +751,16 @@ static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
-	err = regulator_enable(at24->vcc_reg);
-	if (err) {
-		dev_err(dev, "Failed to enable vcc regulator\n");
-		return err;
-	}
+	off_during_probe = acpi_dev_state_low_power(&client->dev);
+	if (!off_during_probe) {
+		err = regulator_enable(at24->vcc_reg);
+		if (err) {
+			dev_err(dev, "Failed to enable vcc regulator\n");
+			return err;
+		}
 
-	/* enable runtime pm */
-	pm_runtime_set_active(dev);
+		pm_runtime_set_active(dev);
+	}
 	pm_runtime_enable(dev);
 
 	at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
@@ -768,14 +771,17 @@ static int at24_probe(struct i2c_client *client)
 	}
 
 	/*
-	 * 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);
-	if (err) {
-		pm_runtime_disable(dev);
-		regulator_disable(at24->vcc_reg);
-		return -ENODEV;
+	if (!off_during_probe) {
+		err = at24_read(at24, 0, &test_byte, 1);
+		if (err) {
+			pm_runtime_disable(dev);
+			regulator_disable(at24->vcc_reg);
+			return -ENODEV;
+		}
 	}
 
 	pm_runtime_idle(dev);
@@ -795,9 +801,11 @@ static int at24_remove(struct i2c_client *client)
 	struct at24_data *at24 = i2c_get_clientdata(client);
 
 	pm_runtime_disable(&client->dev);
-	if (!pm_runtime_status_suspended(&client->dev))
-		regulator_disable(at24->vcc_reg);
-	pm_runtime_set_suspended(&client->dev);
+	if (!acpi_dev_state_low_power(&client->dev)) {
+		if (!pm_runtime_status_suspended(&client->dev))
+			regulator_disable(at24->vcc_reg);
+		pm_runtime_set_suspended(&client->dev);
+	}
 
 	return 0;
 }
@@ -834,6 +842,7 @@ static struct i2c_driver at24_driver = {
 	.probe_new = at24_probe,
 	.remove = at24_remove,
 	.id_table = at24_ids,
+	.flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
 };
 
 static int __init at24_init(void)
-- 
2.20.1


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

* Re: [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state
  2021-02-05 13:25 ` [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state Sakari Ailus
@ 2021-02-06  0:56   ` Randy Dunlap
  2021-02-08  8:01     ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Randy Dunlap @ 2021-02-06  0:56 UTC (permalink / raw)
  To: Sakari Ailus, linux-i2c
  Cc: Wolfram Sang, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

On 2/5/21 5:25 AM, Sakari Ailus wrote:
> Document the use of the _DSE object for setting desirable power state
> during probe.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> ---
>  Documentation/firmware-guide/acpi/index.rst   |  1 +
>  .../firmware-guide/acpi/low-power-probe.rst   | 69 +++++++++++++++++++
>  2 files changed, 70 insertions(+)
>  create mode 100644 Documentation/firmware-guide/acpi/low-power-probe.rst
> 

> diff --git a/Documentation/firmware-guide/acpi/low-power-probe.rst b/Documentation/firmware-guide/acpi/low-power-probe.rst
> new file mode 100644
> index 0000000000000..b96804d959a6c
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/low-power-probe.rst
> @@ -0,0 +1,69 @@
> +.. 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.
> +
> +How it works
> +============
>

Hi,

Please don't use ============ underlines for all section levels.
Here is what Documentation/doc-guide/sphinx.rst says:

Specific guidelines for the kernel documentation
------------------------------------------------

Here are some specific guidelines for the kernel documentation:

* Please don't go overboard with reStructuredText markup. Keep it
  simple. For the most part the documentation should be plain text with
  just enough consistency in formatting that it can be converted to
  other formats.

* Please keep the formatting changes minimal when converting existing
  documentation to reStructuredText.

* Also update the content, not just the formatting, when converting
  documentation.

* Please stick to this order of heading adornments:

  1. ``=`` with overline for document title::

       ==============
       Document title
       ==============

  2. ``=`` for chapters::

       Chapters
       ========

  3. ``-`` for sections::

       Section
       -------

  4. ``~`` for subsections::

       Subsection
       ~~~~~~~~~~

  Although RST doesn't mandate a specific order ("Rather than imposing a fixed
  number and order of section title adornment styles, the order enforced will be
  the order as encountered."), having the higher levels the same overall makes
  it easier to follow the documents.



thanks.
-- 
~Randy


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

* Re: [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state
  2021-02-06  0:56   ` Randy Dunlap
@ 2021-02-08  8:01     ` Sakari Ailus
  2021-02-08 15:52       ` Randy Dunlap
  0 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-08  8:01 UTC (permalink / raw)
  To: Randy Dunlap
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

Hi Randy,

On Fri, Feb 05, 2021 at 04:56:47PM -0800, Randy Dunlap wrote:
> On 2/5/21 5:25 AM, Sakari Ailus wrote:
> > Document the use of the _DSE object for setting desirable power state
> > during probe.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > ---
> >  Documentation/firmware-guide/acpi/index.rst   |  1 +
> >  .../firmware-guide/acpi/low-power-probe.rst   | 69 +++++++++++++++++++
> >  2 files changed, 70 insertions(+)
> >  create mode 100644 Documentation/firmware-guide/acpi/low-power-probe.rst
> > 
> 
> > diff --git a/Documentation/firmware-guide/acpi/low-power-probe.rst b/Documentation/firmware-guide/acpi/low-power-probe.rst
> > new file mode 100644
> > index 0000000000000..b96804d959a6c
> > --- /dev/null
> > +++ b/Documentation/firmware-guide/acpi/low-power-probe.rst
> > @@ -0,0 +1,69 @@
> > +.. 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.
> > +
> > +How it works
> > +============
> >
> 
> Hi,
> 
> Please don't use ============ underlines for all section levels.

The sections under the title are intended to be on the same level.

-- 
Regards,

Sakari Ailus

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

* Re: [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state
  2021-02-08  8:01     ` Sakari Ailus
@ 2021-02-08 15:52       ` Randy Dunlap
  0 siblings, 0 replies; 25+ messages in thread
From: Randy Dunlap @ 2021-02-08 15:52 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Wolfram Sang, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

On 2/8/21 12:01 AM, Sakari Ailus wrote:
> Hi Randy,
> 
> On Fri, Feb 05, 2021 at 04:56:47PM -0800, Randy Dunlap wrote:
>> On 2/5/21 5:25 AM, Sakari Ailus wrote:
>>> Document the use of the _DSE object for setting desirable power state
>>> during probe.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
>>> ---
>>>  Documentation/firmware-guide/acpi/index.rst   |  1 +
>>>  .../firmware-guide/acpi/low-power-probe.rst   | 69 +++++++++++++++++++
>>>  2 files changed, 70 insertions(+)
>>>  create mode 100644 Documentation/firmware-guide/acpi/low-power-probe.rst
>>>
>>
>>> diff --git a/Documentation/firmware-guide/acpi/low-power-probe.rst b/Documentation/firmware-guide/acpi/low-power-probe.rst
>>> new file mode 100644
>>> index 0000000000000..b96804d959a6c
>>> --- /dev/null
>>> +++ b/Documentation/firmware-guide/acpi/low-power-probe.rst
>>> @@ -0,0 +1,69 @@
>>> +.. 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.
>>> +
>>> +How it works
>>> +============
>>>
>>
>> Hi,
>>
>> Please don't use ============ underlines for all section levels.
> 
> The sections under the title are intended to be on the same level.
> 

Oops. My bad. Sorry about that.

-- 
~Randy


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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-05 13:25 ` [PATCH v10 7/7] at24: Support probing while off Sakari Ailus
@ 2021-02-08 16:44   ` Bartosz Golaszewski
  2021-02-08 16:54     ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Bartosz Golaszewski @ 2021-02-08 16:44 UTC (permalink / raw)
  To: Sakari Ailus, Rafael J. Wysocki
  Cc: linux-i2c, Wolfram Sang, ACPI Devel Maling List, LKML,
	Greg Kroah-Hartman, Rajmohan Mani, Tomasz Figa, Bingbu Cao,
	Chiranjeevi Rapolu, Hyungwoo Yang, linux-media

On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> 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>
> Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> ---

I'll ack this but I still claim that the name
acpi_dev_state_low_power() is super misleading for this use-case and
I've been saying that for 10 versions now with everyone just ignoring
my remarks. :/

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-08 16:44   ` Bartosz Golaszewski
@ 2021-02-08 16:54     ` Rafael J. Wysocki
  2021-02-09 15:49       ` Bartosz Golaszewski
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2021-02-08 16:54 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Sakari Ailus, Rafael J. Wysocki, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > 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>
> > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > ---
>
> I'll ack this but I still claim that the name
> acpi_dev_state_low_power() is super misleading for this use-case and
> I've been saying that for 10 versions now with everyone just ignoring
> my remarks. :/

Well, the function in question simply checks if the current ACPI power
state of the device is different from "full power", so its name
appears to be quite adequate to me.

If the way in which it is used is confusing, though, I guess
explaining what's going on would be welcome.

> Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-08 16:54     ` Rafael J. Wysocki
@ 2021-02-09 15:49       ` Bartosz Golaszewski
  2021-02-09 16:23         ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Bartosz Golaszewski @ 2021-02-09 15:49 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Sakari Ailus, linux-i2c, Wolfram Sang, ACPI Devel Maling List,
	LKML, Greg Kroah-Hartman, Rajmohan Mani, Tomasz Figa, Bingbu Cao,
	Chiranjeevi Rapolu, Hyungwoo Yang, linux-media

On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > 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>
> > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > ---
> >
> > I'll ack this but I still claim that the name
> > acpi_dev_state_low_power() is super misleading for this use-case and
> > I've been saying that for 10 versions now with everyone just ignoring
> > my remarks. :/
>
> Well, the function in question simply checks if the current ACPI power
> state of the device is different from "full power", so its name
> appears to be quite adequate to me.
>
> If the way in which it is used is confusing, though, I guess
> explaining what's going on would be welcome.
>

Yes, I have explained it multiple time already - last time at v9 of this series:

    https://www.spinics.net/lists/kernel/msg3816807.html

Bartosz

> > Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-09 15:49       ` Bartosz Golaszewski
@ 2021-02-09 16:23         ` Sakari Ailus
  2021-02-09 16:42           ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-09 16:23 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Rafael J. Wysocki, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

Hi Bartosz, Rafael,

On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > >
> > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > 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>
> > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > ---
> > >
> > > I'll ack this but I still claim that the name
> > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > I've been saying that for 10 versions now with everyone just ignoring
> > > my remarks. :/
> >
> > Well, the function in question simply checks if the current ACPI power
> > state of the device is different from "full power", so its name
> > appears to be quite adequate to me.
> >
> > If the way in which it is used is confusing, though, I guess
> > explaining what's going on would be welcome.
> >
> 
> Yes, I have explained it multiple time already - last time at v9 of this series:
> 
>     https://www.spinics.net/lists/kernel/msg3816807.html

How about adding this to the description of acpi_dev_state_low_power():

-----------8<--------------
 * This function is intended to be used by drivers to tell whether the device
 * is in low power state (D1--D3cold) in driver's probe or remove function. See
 * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
-----------8<--------------

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-09 16:23         ` Sakari Ailus
@ 2021-02-09 16:42           ` Rafael J. Wysocki
  2021-02-09 16:54             ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2021-02-09 16:42 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Bartosz Golaszewski, Rafael J. Wysocki, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Bartosz, Rafael,
>
> On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> > > >
> > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > 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>
> > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > ---
> > > >
> > > > I'll ack this but I still claim that the name
> > > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > > I've been saying that for 10 versions now with everyone just ignoring
> > > > my remarks. :/
> > >
> > > Well, the function in question simply checks if the current ACPI power
> > > state of the device is different from "full power", so its name
> > > appears to be quite adequate to me.
> > >
> > > If the way in which it is used is confusing, though, I guess
> > > explaining what's going on would be welcome.
> > >
> >
> > Yes, I have explained it multiple time already - last time at v9 of this series:
> >
> >     https://www.spinics.net/lists/kernel/msg3816807.html
>
> How about adding this to the description of acpi_dev_state_low_power():
>
> -----------8<--------------
>  * This function is intended to be used by drivers to tell whether the device
>  * is in low power state (D1--D3cold) in driver's probe or remove function. See
>  * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
> -----------8<--------------

This information is already there in the kerneldoc description of that
function AFAICS.

I was thinking about adding an explanation comment to the caller.

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-09 16:42           ` Rafael J. Wysocki
@ 2021-02-09 16:54             ` Sakari Ailus
  2021-02-09 16:58               ` Rafael J. Wysocki
  0 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-09 16:54 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Bartosz Golaszewski, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Bartosz, Rafael,
> >
> > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > > > <bgolaszewski@baylibre.com> wrote:
> > > > >
> > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > 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>
> > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > > ---
> > > > >
> > > > > I'll ack this but I still claim that the name
> > > > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > > > I've been saying that for 10 versions now with everyone just ignoring
> > > > > my remarks. :/
> > > >
> > > > Well, the function in question simply checks if the current ACPI power
> > > > state of the device is different from "full power", so its name
> > > > appears to be quite adequate to me.
> > > >
> > > > If the way in which it is used is confusing, though, I guess
> > > > explaining what's going on would be welcome.
> > > >
> > >
> > > Yes, I have explained it multiple time already - last time at v9 of this series:
> > >
> > >     https://www.spinics.net/lists/kernel/msg3816807.html
> >
> > How about adding this to the description of acpi_dev_state_low_power():
> >
> > -----------8<--------------
> >  * This function is intended to be used by drivers to tell whether the device
> >  * is in low power state (D1--D3cold) in driver's probe or remove function. See
> >  * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
> > -----------8<--------------
> 
> This information is already there in the kerneldoc description of that
> function AFAICS.

Ok, the D states are mentioned already. But how to use it is not, nor
there's a reference to the ReST file. I think that wouldn't hurt.

> 
> I was thinking about adding an explanation comment to the caller.

I think it'd be best if the function name would convey that without a
comment that should then be added to all callers. How about calling the
function e.g. acpi_dev_state_d0() and negating the return value? The D0
state is well defined and we could do this without adding new terms.

-- 
Sakari Ailus

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-09 16:54             ` Sakari Ailus
@ 2021-02-09 16:58               ` Rafael J. Wysocki
  2021-02-10  8:41                 ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Rafael J. Wysocki @ 2021-02-09 16:58 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Rafael J. Wysocki, Bartosz Golaszewski, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Bartosz, Rafael,
> > >
> > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > >
> > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > >
> > > > > > > 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>
> > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > > > ---
> > > > > >
> > > > > > I'll ack this but I still claim that the name
> > > > > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > > > > I've been saying that for 10 versions now with everyone just ignoring
> > > > > > my remarks. :/
> > > > >
> > > > > Well, the function in question simply checks if the current ACPI power
> > > > > state of the device is different from "full power", so its name
> > > > > appears to be quite adequate to me.
> > > > >
> > > > > If the way in which it is used is confusing, though, I guess
> > > > > explaining what's going on would be welcome.
> > > > >
> > > >
> > > > Yes, I have explained it multiple time already - last time at v9 of this series:
> > > >
> > > >     https://www.spinics.net/lists/kernel/msg3816807.html
> > >
> > > How about adding this to the description of acpi_dev_state_low_power():
> > >
> > > -----------8<--------------
> > >  * This function is intended to be used by drivers to tell whether the device
> > >  * is in low power state (D1--D3cold) in driver's probe or remove function. See
> > >  * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
> > > -----------8<--------------
> >
> > This information is already there in the kerneldoc description of that
> > function AFAICS.
>
> Ok, the D states are mentioned already. But how to use it is not, nor
> there's a reference to the ReST file. I think that wouldn't hurt.
>
> >
> > I was thinking about adding an explanation comment to the caller.
>
> I think it'd be best if the function name would convey that without a
> comment that should then be added to all callers. How about calling the
> function e.g. acpi_dev_state_d0() and negating the return value? The D0
> state is well defined and we could do this without adding new terms.

That would work for me.

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

* Re: [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe
  2021-02-05 13:25 ` [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe Sakari Ailus
@ 2021-02-09 21:04   ` Wolfram Sang
  2021-02-10 20:46     ` Sakari Ailus
  2021-02-10 22:27     ` Sakari Ailus
  0 siblings, 2 replies; 25+ messages in thread
From: Wolfram Sang @ 2021-02-09 21:04 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

[-- Attachment #1: Type: text/plain, Size: 541 bytes --]


> + * @I2C_DRV_FL_ALLOW_LOW_POWER_PROBE: Let the ACPI driver manage the device's
> + *				      power state during probe and remove

Well, for the functional change, I am happy if the ACPI guys are happy.
The only minor nit for me would be removing the "_FL" snipplet from the
name of the define because I think it is clear enough that this is a
flag. If you need to resend anyhow, maybe it is worth a thought. It is
not a big issue, so anyway:

Acked-by: Wolfram Sang <wsa@kernel.org>

because I assume this will go in via the ACPI tree?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-09 16:58               ` Rafael J. Wysocki
@ 2021-02-10  8:41                 ` Sakari Ailus
  2021-02-10 12:26                   ` Bartosz Golaszewski
  0 siblings, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-10  8:41 UTC (permalink / raw)
  To: Rafael J. Wysocki
  Cc: Bartosz Golaszewski, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Tue, Feb 09, 2021 at 05:58:12PM +0100, Rafael J. Wysocki wrote:
> On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > Hi Bartosz, Rafael,
> > > >
> > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> > > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > >
> > > > > > > > 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>
> > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > > > > ---
> > > > > > >
> > > > > > > I'll ack this but I still claim that the name
> > > > > > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > > > > > I've been saying that for 10 versions now with everyone just ignoring
> > > > > > > my remarks. :/
> > > > > >
> > > > > > Well, the function in question simply checks if the current ACPI power
> > > > > > state of the device is different from "full power", so its name
> > > > > > appears to be quite adequate to me.
> > > > > >
> > > > > > If the way in which it is used is confusing, though, I guess
> > > > > > explaining what's going on would be welcome.
> > > > > >
> > > > >
> > > > > Yes, I have explained it multiple time already - last time at v9 of this series:
> > > > >
> > > > >     https://www.spinics.net/lists/kernel/msg3816807.html
> > > >
> > > > How about adding this to the description of acpi_dev_state_low_power():
> > > >
> > > > -----------8<--------------
> > > >  * This function is intended to be used by drivers to tell whether the device
> > > >  * is in low power state (D1--D3cold) in driver's probe or remove function. See
> > > >  * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
> > > > -----------8<--------------
> > >
> > > This information is already there in the kerneldoc description of that
> > > function AFAICS.
> >
> > Ok, the D states are mentioned already. But how to use it is not, nor
> > there's a reference to the ReST file. I think that wouldn't hurt.
> >
> > >
> > > I was thinking about adding an explanation comment to the caller.
> >
> > I think it'd be best if the function name would convey that without a
> > comment that should then be added to all callers. How about calling the
> > function e.g. acpi_dev_state_d0() and negating the return value? The D0
> > state is well defined and we could do this without adding new terms.
> 
> That would work for me.

Bartosz, would that work for you?

I'd call the temporary variable in the at24 driver e.g. "full_power".

-- 
Regards,

Sakari Ailus

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-10  8:41                 ` Sakari Ailus
@ 2021-02-10 12:26                   ` Bartosz Golaszewski
  2021-02-10 20:46                     ` Sakari Ailus
  0 siblings, 1 reply; 25+ messages in thread
From: Bartosz Golaszewski @ 2021-02-10 12:26 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: Rafael J. Wysocki, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Wed, Feb 10, 2021 at 9:41 AM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> On Tue, Feb 09, 2021 at 05:58:12PM +0100, Rafael J. Wysocki wrote:
> > On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote:
> > > > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus
> > > > <sakari.ailus@linux.intel.com> wrote:
> > > > >
> > > > > Hi Bartosz, Rafael,
> > > > >
> > > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> > > > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > >
> > > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > > > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > > >
> > > > > > > > > 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>
> > > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > > > > > ---
> > > > > > > >
> > > > > > > > I'll ack this but I still claim that the name
> > > > > > > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > > > > > > I've been saying that for 10 versions now with everyone just ignoring
> > > > > > > > my remarks. :/
> > > > > > >
> > > > > > > Well, the function in question simply checks if the current ACPI power
> > > > > > > state of the device is different from "full power", so its name
> > > > > > > appears to be quite adequate to me.
> > > > > > >
> > > > > > > If the way in which it is used is confusing, though, I guess
> > > > > > > explaining what's going on would be welcome.
> > > > > > >
> > > > > >
> > > > > > Yes, I have explained it multiple time already - last time at v9 of this series:
> > > > > >
> > > > > >     https://www.spinics.net/lists/kernel/msg3816807.html
> > > > >
> > > > > How about adding this to the description of acpi_dev_state_low_power():
> > > > >
> > > > > -----------8<--------------
> > > > >  * This function is intended to be used by drivers to tell whether the device
> > > > >  * is in low power state (D1--D3cold) in driver's probe or remove function. See
> > > > >  * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
> > > > > -----------8<--------------
> > > >
> > > > This information is already there in the kerneldoc description of that
> > > > function AFAICS.
> > >
> > > Ok, the D states are mentioned already. But how to use it is not, nor
> > > there's a reference to the ReST file. I think that wouldn't hurt.
> > >
> > > >
> > > > I was thinking about adding an explanation comment to the caller.
> > >
> > > I think it'd be best if the function name would convey that without a
> > > comment that should then be added to all callers. How about calling the
> > > function e.g. acpi_dev_state_d0() and negating the return value? The D0
> > > state is well defined and we could do this without adding new terms.
> >
> > That would work for me.
>
> Bartosz, would that work for you?
>
> I'd call the temporary variable in the at24 driver e.g. "full_power".
>

Yes, that works, go ahead and thank you.

Bartosz

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

* Re: [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe
  2021-02-09 21:04   ` Wolfram Sang
@ 2021-02-10 20:46     ` Sakari Ailus
  2021-02-10 22:27     ` Sakari Ailus
  1 sibling, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-10 20:46 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

Hi Wolfram,

On Tue, Feb 09, 2021 at 10:04:10PM +0100, Wolfram Sang wrote:
> 
> > + * @I2C_DRV_FL_ALLOW_LOW_POWER_PROBE: Let the ACPI driver manage the device's
> > + *				      power state during probe and remove
> 
> Well, for the functional change, I am happy if the ACPI guys are happy.
> The only minor nit for me would be removing the "_FL" snipplet from the
> name of the define because I think it is clear enough that this is a
> flag. If you need to resend anyhow, maybe it is worth a thought. It is

I'll remove _FL for v11.

> not a big issue, so anyway:
> 
> Acked-by: Wolfram Sang <wsa@kernel.org>

Thank you!

> 
> because I assume this will go in via the ACPI tree?

I think so, yes, if no-one has objections. Most of the changes are for ACPI
framework and docs.

-- 
Kind regards,

Sakari Ailus

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

* Re: [PATCH v10 7/7] at24: Support probing while off
  2021-02-10 12:26                   ` Bartosz Golaszewski
@ 2021-02-10 20:46                     ` Sakari Ailus
  0 siblings, 0 replies; 25+ messages in thread
From: Sakari Ailus @ 2021-02-10 20:46 UTC (permalink / raw)
  To: Bartosz Golaszewski
  Cc: Rafael J. Wysocki, linux-i2c, Wolfram Sang,
	ACPI Devel Maling List, LKML, Greg Kroah-Hartman, Rajmohan Mani,
	Tomasz Figa, Bingbu Cao, Chiranjeevi Rapolu, Hyungwoo Yang,
	linux-media

On Wed, Feb 10, 2021 at 01:26:51PM +0100, Bartosz Golaszewski wrote:
> On Wed, Feb 10, 2021 at 9:41 AM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > On Tue, Feb 09, 2021 at 05:58:12PM +0100, Rafael J. Wysocki wrote:
> > > On Tue, Feb 9, 2021 at 5:54 PM Sakari Ailus
> > > <sakari.ailus@linux.intel.com> wrote:
> > > >
> > > > On Tue, Feb 09, 2021 at 05:42:45PM +0100, Rafael J. Wysocki wrote:
> > > > > On Tue, Feb 9, 2021 at 5:23 PM Sakari Ailus
> > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > >
> > > > > > Hi Bartosz, Rafael,
> > > > > >
> > > > > > On Tue, Feb 09, 2021 at 04:49:37PM +0100, Bartosz Golaszewski wrote:
> > > > > > > On Mon, Feb 8, 2021 at 5:54 PM Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > > > >
> > > > > > > > On Mon, Feb 8, 2021 at 5:44 PM Bartosz Golaszewski
> > > > > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > > > > >
> > > > > > > > > On Fri, Feb 5, 2021 at 2:25 PM Sakari Ailus
> > > > > > > > > <sakari.ailus@linux.intel.com> wrote:
> > > > > > > > > >
> > > > > > > > > > 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>
> > > > > > > > > > Reviewed-by: Tomasz Figa <tfiga@chromium.org>
> > > > > > > > > > ---
> > > > > > > > >
> > > > > > > > > I'll ack this but I still claim that the name
> > > > > > > > > acpi_dev_state_low_power() is super misleading for this use-case and
> > > > > > > > > I've been saying that for 10 versions now with everyone just ignoring
> > > > > > > > > my remarks. :/
> > > > > > > >
> > > > > > > > Well, the function in question simply checks if the current ACPI power
> > > > > > > > state of the device is different from "full power", so its name
> > > > > > > > appears to be quite adequate to me.
> > > > > > > >
> > > > > > > > If the way in which it is used is confusing, though, I guess
> > > > > > > > explaining what's going on would be welcome.
> > > > > > > >
> > > > > > >
> > > > > > > Yes, I have explained it multiple time already - last time at v9 of this series:
> > > > > > >
> > > > > > >     https://www.spinics.net/lists/kernel/msg3816807.html
> > > > > >
> > > > > > How about adding this to the description of acpi_dev_state_low_power():
> > > > > >
> > > > > > -----------8<--------------
> > > > > >  * This function is intended to be used by drivers to tell whether the device
> > > > > >  * is in low power state (D1--D3cold) in driver's probe or remove function. See
> > > > > >  * Documentation/firmware-guide/acpi/low-power-probe.rst for more information.
> > > > > > -----------8<--------------
> > > > >
> > > > > This information is already there in the kerneldoc description of that
> > > > > function AFAICS.
> > > >
> > > > Ok, the D states are mentioned already. But how to use it is not, nor
> > > > there's a reference to the ReST file. I think that wouldn't hurt.
> > > >
> > > > >
> > > > > I was thinking about adding an explanation comment to the caller.
> > > >
> > > > I think it'd be best if the function name would convey that without a
> > > > comment that should then be added to all callers. How about calling the
> > > > function e.g. acpi_dev_state_d0() and negating the return value? The D0
> > > > state is well defined and we could do this without adding new terms.
> > >
> > > That would work for me.
> >
> > Bartosz, would that work for you?
> >
> > I'd call the temporary variable in the at24 driver e.g. "full_power".
> >
> 
> Yes, that works, go ahead and thank you.

Thanks!

I'll send v11 soon.

-- 
Sakari Ailus

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

* Re: [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe
  2021-02-09 21:04   ` Wolfram Sang
  2021-02-10 20:46     ` Sakari Ailus
@ 2021-02-10 22:27     ` Sakari Ailus
  2021-02-12  7:18       ` Wolfram Sang
  1 sibling, 1 reply; 25+ messages in thread
From: Sakari Ailus @ 2021-02-10 22:27 UTC (permalink / raw)
  To: Wolfram Sang, linux-i2c, Rafael J. Wysocki, linux-acpi,
	linux-kernel, Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

On Tue, Feb 09, 2021 at 10:04:10PM +0100, Wolfram Sang wrote:
> 
> > + * @I2C_DRV_FL_ALLOW_LOW_POWER_PROBE: Let the ACPI driver manage the device's
> > + *				      power state during probe and remove
> 
> Well, for the functional change, I am happy if the ACPI guys are happy.
> The only minor nit for me would be removing the "_FL" snipplet from the

I'm actually renaming this as I2C_DRV_ACPI_WAIVE_D0_PROBE, with similar
changes to the function names. I opportunistically assume the ack holds
still. :-) I'll post v11 soon.

> name of the define because I think it is clear enough that this is a
> flag. If you need to resend anyhow, maybe it is worth a thought. It is
> not a big issue, so anyway:
> 
> Acked-by: Wolfram Sang <wsa@kernel.org>
> 
> because I assume this will go in via the ACPI tree?
> 

-- 
Sakari Ailus

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

* Re: [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe
  2021-02-10 22:27     ` Sakari Ailus
@ 2021-02-12  7:18       ` Wolfram Sang
  0 siblings, 0 replies; 25+ messages in thread
From: Wolfram Sang @ 2021-02-12  7:18 UTC (permalink / raw)
  To: Sakari Ailus
  Cc: linux-i2c, Rafael J. Wysocki, linux-acpi, linux-kernel,
	Greg Kroah-Hartman, rajmohan.mani, Tomasz Figa,
	Bartosz Golaszewski, Bingbu Cao, Chiranjeevi Rapolu,
	Hyungwoo Yang, linux-media

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]


> I'm actually renaming this as I2C_DRV_ACPI_WAIVE_D0_PROBE, with similar
> changes to the function names. I opportunistically assume the ack holds
> still. :-)

Rightfully so :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

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

end of thread, other threads:[~2021-02-12  7:19 UTC | newest]

Thread overview: 25+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-05 13:24 [PATCH v10 0/7] Support running driver's probe for a device powered off Sakari Ailus
2021-02-05 13:24 ` [PATCH v10 1/7] ACPI: scan: Obtain device's desired enumeration power state Sakari Ailus
2021-02-05 13:25 ` [PATCH v10 2/7] i2c: Allow an ACPI driver to manage the device's power state during probe Sakari Ailus
2021-02-09 21:04   ` Wolfram Sang
2021-02-10 20:46     ` Sakari Ailus
2021-02-10 22:27     ` Sakari Ailus
2021-02-12  7:18       ` Wolfram Sang
2021-02-05 13:25 ` [PATCH v10 3/7] Documentation: ACPI: Document _DSE object usage for enum power state Sakari Ailus
2021-02-06  0:56   ` Randy Dunlap
2021-02-08  8:01     ` Sakari Ailus
2021-02-08 15:52       ` Randy Dunlap
2021-02-05 13:25 ` [PATCH v10 4/7] ACPI: Add a convenience function to tell a device is in low " Sakari Ailus
2021-02-05 13:25 ` [PATCH v10 5/7] ov5670: Support probe whilst the device is in a " Sakari Ailus
2021-02-05 13:25 ` [PATCH v10 6/7] media: i2c: imx319: Support probe while the device is off Sakari Ailus
2021-02-05 13:25 ` [PATCH v10 7/7] at24: Support probing while off Sakari Ailus
2021-02-08 16:44   ` Bartosz Golaszewski
2021-02-08 16:54     ` Rafael J. Wysocki
2021-02-09 15:49       ` Bartosz Golaszewski
2021-02-09 16:23         ` Sakari Ailus
2021-02-09 16:42           ` Rafael J. Wysocki
2021-02-09 16:54             ` Sakari Ailus
2021-02-09 16:58               ` Rafael J. Wysocki
2021-02-10  8:41                 ` Sakari Ailus
2021-02-10 12:26                   ` Bartosz Golaszewski
2021-02-10 20:46                     ` Sakari Ailus

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.