[PATCH v6 0/6] Support running driver's probe for a device powered off

[PATCH v6 0/6] Support running driver's probe for a device powered off

[Sakari Ailus]

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 master. They also apply to Wolfram's
i2c/for-next branch, there's a patch that affects the I²C core changes
here (see below). The patches apart from that apply to Bartosz's
at24/for-next as well as Mauro's linux-media master branch.

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 (5):
  i2c: Allow an ACPI 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 allow-low-power-probe _DSD property

 .../acpi/dsd/allow-low-power-probe.rst        | 28 +++++++
 Documentation/firmware-guide/acpi/index.rst   |  1 +
 drivers/acpi/device_pm.c                      | 31 ++++++++
 drivers/i2c/i2c-core-base.c                   | 18 ++++-
 drivers/media/i2c/imx319.c                    | 74 +++++++++++-------
 drivers/media/i2c/ov5670.c                    | 76 +++++++++++--------
 drivers/misc/eeprom/at24.c                    | 43 ++++++-----
 include/linux/acpi.h                          |  5 ++
 include/linux/i2c.h                           | 14 ++++
 9 files changed, 211 insertions(+), 79 deletions(-)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst

-- 
2.20.1

[PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Sakari Ailus]

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>
---
 drivers/i2c/i2c-core-base.c | 18 +++++++++++++++---
 include/linux/i2c.h         | 14 ++++++++++++++
 2 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 34a9609f256da..f2683790eb0d2 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -436,6 +436,15 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
 	return irq > 0 ? irq : -ENXIO;
 }
 
+static bool allow_low_power_probe(struct device *dev)
+{
+	struct i2c_driver *driver = to_i2c_driver(dev->driver);
+
+	return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE &&
+		is_acpi_node(dev_fwnode(dev)) &&
+		device_property_present(dev, "allow-low-power-probe");
+}
+
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
@@ -514,7 +523,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,
+				      !allow_low_power_probe(&client->dev));
 	if (status)
 		goto err_clear_wakeup_irq;
 
@@ -536,7 +546,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,
+			     !allow_low_power_probe(&client->dev));
 err_clear_wakeup_irq:
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -562,7 +573,8 @@ 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,
+			     !allow_low_power_probe(&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 fc55ea41d3237..fec102b2a37a3 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;
+
+	unsigned int flags;
 };
 #define to_i2c_driver(d) container_of(d, struct i2c_driver, driver)
 
-- 
2.20.1

[PATCH v6 2/6] ACPI: Add a convenience function to tell a device is in low power state

[Sakari Ailus]

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 94d91c67aeaeb..e3c488d4af0d4 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1344,4 +1344,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 1e4cdc6c7ae20..276f6af99779f 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -976,6 +976,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; }
@@ -985,6 +986,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

[PATCH v6 3/6] ov5670: Support probe whilst the device is in a low power state

[Sakari Ailus]

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>
---
 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 f26252e35e08d..74b0325c22565 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)
@@ -2270,6 +2272,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)
 {
@@ -2278,6 +2306,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);
@@ -2401,27 +2433,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,
 };
@@ -2456,6 +2467,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);
@@ -2472,11 +2484,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);
@@ -2513,10 +2528,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);
 
@@ -2558,7 +2573,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 */ }
 };
 
@@ -2573,6 +2588,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

[PATCH v6 4/6] media: i2c: imx319: Support probe while the device is off

[Sakari Ailus]

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 | 74 +++++++++++++++++++++++---------------
 1 file changed, 45 insertions(+), 29 deletions(-)

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221e..6b63bb0abb138 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);
@@ -2210,26 +2241,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,
@@ -2424,6 +2435,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 +2448,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 +2508,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 +2551,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 +2564,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

[PATCH v6 5/6] at24: Support probing while off

[Sakari Ailus]

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 | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2591c21b2b5d8..a343e55e41126 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -561,6 +561,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;
@@ -698,25 +699,30 @@ 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;
-	}
+	low_power = acpi_dev_state_low_power(&client->dev);
+	if (!low_power) {
+		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);
 
 	/*
-	 * 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 (!low_power) {
+		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);
@@ -736,9 +742,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;
 }
@@ -775,6 +783,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

[PATCH v6 6/6] Documentation: ACPI: Document allow-low-power-probe _DSD property

[Sakari Ailus]

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/allow-low-power-probe.rst        | 28 +++++++++++++++++++
 Documentation/firmware-guide/acpi/index.rst   |  1 +
 2 files changed, 29 insertions(+)
 create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst

diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
new file mode 100644
index 0000000000000..6fcc89162b898
--- /dev/null
+++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.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. Linux recognizes the _DSD property
+"allow-low-power-probe" that can be used for this purpose.
+
+How it works
+============
+
+The property "allow-low-power-probe" 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 by
+setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags
+field and the "allow-low-power-probe" 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..0070fcde9e669 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/allow-low-power-probe
    enumeration
    osi
    method-customizing
-- 
2.20.1

Re: [PATCH v6 6/6] Documentation: ACPI: Document allow-low-power-probe _DSD property

[Randy Dunlap]

On 8/26/20 4:54 AM, Sakari Ailus 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/allow-low-power-probe.rst        | 28 +++++++++++++++++++
>  Documentation/firmware-guide/acpi/index.rst   |  1 +
>  2 files changed, 29 insertions(+)
>  create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
> 
> diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
> new file mode 100644
> index 0000000000000..6fcc89162b898
> --- /dev/null
> +++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.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. Linux recognizes the _DSD property
> +"allow-low-power-probe" that can be used for this purpose.
> +
> +How it works
> +============
> +
> +The property "allow-low-power-probe" boolean property may be used to tell Linux

Drop the first "property".

> +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 by
> +setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags
> +field and the "allow-low-power-probe" property is present, the device will not
> +be powered on for probe.

thanks.
-- 
~Randy

Re: [PATCH v6 6/6] Documentation: ACPI: Document allow-low-power-probe _DSD property

[Sakari Ailus]

On Wed, Aug 26, 2020 at 11:06:22AM -0700, Randy Dunlap wrote:
> On 8/26/20 4:54 AM, Sakari Ailus 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/allow-low-power-probe.rst        | 28 +++++++++++++++++++
> >  Documentation/firmware-guide/acpi/index.rst   |  1 +
> >  2 files changed, 29 insertions(+)
> >  create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
> > 
> > diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
> > new file mode 100644
> > index 0000000000000..6fcc89162b898
> > --- /dev/null
> > +++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.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. Linux recognizes the _DSD property
> > +"allow-low-power-probe" that can be used for this purpose.
> > +
> > +How it works
> > +============
> > +
> > +The property "allow-low-power-probe" boolean property may be used to tell Linux
> 
> Drop the first "property".
> 

Thanks, will fix for v7.

> > +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 by
> > +setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags

This is leftover from v5 actually, I'll change the documentation to mention
the allow_low_power_probe as well.

> > +field and the "allow-low-power-probe" property is present, the device will not
> > +be powered on for probe.
> 

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Wolfram Sang]

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

Hi Sakari,

On Wed, Aug 26, 2020 at 02:54:27PM +0300, Sakari Ailus wrote:
> 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>
> ---
>  drivers/i2c/i2c-core-base.c | 18 +++++++++++++++---
>  include/linux/i2c.h         | 14 ++++++++++++++
>  2 files changed, 29 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 34a9609f256da..f2683790eb0d2 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -436,6 +436,15 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
>  	return irq > 0 ? irq : -ENXIO;
>  }
>  
> +static bool allow_low_power_probe(struct device *dev)
> +{
> +	struct i2c_driver *driver = to_i2c_driver(dev->driver);
> +
> +	return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE &&
> +		is_acpi_node(dev_fwnode(dev)) &&
> +		device_property_present(dev, "allow-low-power-probe");

So, I wondered about potential DT usage and I read the discussion about
that in v5 which concluded that for the DT case, the drivers can make
use of the binding individually. I can agree to that, but then the name
of the binding is probably problematic. 'allow-*' sounds like
configuration but DT is for describing HW. So, I think something in the
range of 'keep-low-power' or so might be better suited. Grepping shows
there already is a generic binding "low-power-enable". Not sure, if it
really fits, because here it is more about 'keeping' rather than
enabling. Or?

> +/**
> + * 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;
> +
> +	unsigned int flags;

Here I wonder if all this is really I2C specific? I could imagine this
being useful for other busses as well, so maybe 'struct device_driver'
is a better place?

Thanks for this work!

All the best,

   Wolfram


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

Re: [PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Sakari Ailus]

Hi Wolfram,

Thank you for the review.

On Fri, Aug 28, 2020 at 10:38:32AM +0200, Wolfram Sang wrote:
> Hi Sakari,
> 
> On Wed, Aug 26, 2020 at 02:54:27PM +0300, Sakari Ailus wrote:
> > 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>
> > ---
> >  drivers/i2c/i2c-core-base.c | 18 +++++++++++++++---
> >  include/linux/i2c.h         | 14 ++++++++++++++
> >  2 files changed, 29 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 34a9609f256da..f2683790eb0d2 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -436,6 +436,15 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
> >  	return irq > 0 ? irq : -ENXIO;
> >  }
> >  
> > +static bool allow_low_power_probe(struct device *dev)
> > +{
> > +	struct i2c_driver *driver = to_i2c_driver(dev->driver);
> > +
> > +	return driver->flags & I2C_DRV_FL_ALLOW_LOW_POWER_PROBE &&
> > +		is_acpi_node(dev_fwnode(dev)) &&
> > +		device_property_present(dev, "allow-low-power-probe");
> 
> So, I wondered about potential DT usage and I read the discussion about
> that in v5 which concluded that for the DT case, the drivers can make
> use of the binding individually. I can agree to that, but then the name
> of the binding is probably problematic. 'allow-*' sounds like
> configuration but DT is for describing HW. So, I think something in the
> range of 'keep-low-power' or so might be better suited. Grepping shows
> there already is a generic binding "low-power-enable". Not sure, if it
> really fits, because here it is more about 'keeping' rather than
> enabling. Or?

The low-power-enable appears to be telling pinctrl drivers the pin can
be configured for low power operation.

This patchset is really about changing the default of ACPI powering up I²C
devices. On OF the drivers are indeed responsible for that.

Another approach in naming the property could related to the consequence
that device accesses must be omitted during driver probe time, but the
first device access takes place when the user actually needs it, e.g.
"skip-device-probe". Due to that the device does not need to be powered on
for probe, so powering it on can be omitted. I'd still keep the naming in
the kernel as-is in that case.

> 
> > +/**
> > + * 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;
> > +
> > +	unsigned int flags;
> 
> Here I wonder if all this is really I2C specific? I could imagine this
> being useful for other busses as well, so maybe 'struct device_driver'
> is a better place?

The default power state appears to depend on the bus type on ACPI. I'd
think it's unlikely this feature would be needed elsewhere, with the
possible exception of I3C, if hardware design does not improve from the
current I²C connected cameras.

My original series had a field in struct device_driver for this purpose but
Greg K-H suggested moving it to I²C instead:

<URL:https://lore.kernel.org/linux-acpi/20190826084343.GA1095@kroah.com/>

-- 
Kind regards,

Sakari Ailus

Re: [PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Wolfram Sang]

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


> This patchset is really about changing the default of ACPI powering up I²C
> devices. On OF the drivers are indeed responsible for that.

So, maybe it makes sense then to move from 'device_property_present()'
to 'acpi_dev_get_property()' or something alike? To clearly tell this
binding is expected to be used with ACPI only. Then, we can skip this
discussion now and postpone it to when someone wants to use it with DT.
Which is hopefully never. Or does this approach have drawbacks?

> My original series had a field in struct device_driver for this purpose but
> Greg K-H suggested moving it to I²C instead:
> 
> <URL:https://lore.kernel.org/linux-acpi/20190826084343.GA1095@kroah.com/>

Ok, we can still factor it out in the unlikely case it needs to be done
again.

I still have the question via which tree this should go upstream?
It is probably more I2C than ACPI?


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

Re: [PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Sakari Ailus]

On Mon, Aug 31, 2020 at 11:31:09AM +0200, Wolfram Sang wrote:
> 
> > This patchset is really about changing the default of ACPI powering up I²C
> > devices. On OF the drivers are indeed responsible for that.
> 
> So, maybe it makes sense then to move from 'device_property_present()'
> to 'acpi_dev_get_property()' or something alike? To clearly tell this

I'll do that for v7 soon.

> binding is expected to be used with ACPI only. Then, we can skip this
> discussion now and postpone it to when someone wants to use it with DT.
> Which is hopefully never. Or does this approach have drawbacks?

The same issue in principle could be there on DT, too, as the cameras are
the same. There are a few sensor drivers supporting DT that currently don't
access the device in probe to avoid having to power it on. For cameras I
suppose that's just fine but I'd be hesitant changing the behaviour of e.g.
the at24 driver to support that use case without making it somehow
configurable.

> 
> > My original series had a field in struct device_driver for this purpose but
> > Greg K-H suggested moving it to I²C instead:
> > 
> > <URL:https://lore.kernel.org/linux-acpi/20190826084343.GA1095@kroah.com/>
> 
> Ok, we can still factor it out in the unlikely case it needs to be done
> again.
> 
> I still have the question via which tree this should go upstream?
> It is probably more I2C than ACPI?

I think so.

Rafael, would you be fine with this set being merged through the I²C tree?
There's a single patch adding an ACPI function there.

-- 
Sakari Ailus

Re: [PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Wolfram Sang]

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


> +	unsigned int flags;

u32?


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

Re: [PATCH v6 1/6] i2c: Allow an ACPI driver to manage the device's power state during probe

[Sakari Ailus]

On Mon, Aug 31, 2020 at 12:00:23PM +0200, Wolfram Sang wrote:
> 
> > +	unsigned int flags;
> 
> u32?
> 

Yes.

-- 
Sakari Ailus

Re: [PATCH v6 6/6] Documentation: ACPI: Document allow-low-power-probe _DSD property

[Sakari Ailus]

On Thu, Aug 27, 2020 at 04:28:22PM +0300, Sakari Ailus wrote:
> On Wed, Aug 26, 2020 at 11:06:22AM -0700, Randy Dunlap wrote:
> > On 8/26/20 4:54 AM, Sakari Ailus 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/allow-low-power-probe.rst        | 28 +++++++++++++++++++
> > >  Documentation/firmware-guide/acpi/index.rst   |  1 +
> > >  2 files changed, 29 insertions(+)
> > >  create mode 100644 Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
> > > 
> > > diff --git a/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.rst
> > > new file mode 100644
> > > index 0000000000000..6fcc89162b898
> > > --- /dev/null
> > > +++ b/Documentation/firmware-guide/acpi/dsd/allow-low-power-probe.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. Linux recognizes the _DSD property
> > > +"allow-low-power-probe" that can be used for this purpose.
> > > +
> > > +How it works
> > > +============
> > > +
> > > +The property "allow-low-power-probe" boolean property may be used to tell Linux
> > 
> > Drop the first "property".
> > 
> 
> Thanks, will fix for v7.
> 
> > > +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 by
> > > +setting the I2C_DRV_FL_ALLOW_LOW_POWER_PROBE flag in struct i2c_driver.flags
> 
> This is leftover from v5 actually, I'll change the documentation to mention
> the allow_low_power_probe as well.

Hmm. It seems I ended up sending the set with the flags field.

I'll keep it as-is if that's okay...

-- 
Sakari Ailus