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

[PATCH 0/5] 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.

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

Sakari Ailus (4):
  ACPI: Enable driver and firmware hints to control power at probe time
  ACPI: Add a convenience function to tell a device is suspended in
    probe
  ov5670: Support probe whilst the device is off
  at24: Support probing while off

 drivers/acpi/device_pm.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
 drivers/media/i2c/imx319.c | 25 ++++++++++++++-----------
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 drivers/misc/eeprom/at24.c | 30 ++++++++++++++++++++----------
 include/linux/acpi.h       |  5 +++++
 include/linux/device.h     |  6 ++++++
 6 files changed, 99 insertions(+), 34 deletions(-)

-- 
2.11.0

[PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Allow drivers and firmware tell ACPI that there's no need to power on a
device for probe. This requires both a hint from the firmware as well as
an indication from a driver to leave the device off.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/acpi/device_pm.c | 9 +++++++--
 include/linux/device.h   | 6 ++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index b859d75eaf9f6..ca2409a30d26d 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1225,7 +1225,9 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
 	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
 		dev_pm_domain_set(dev, NULL);
 		acpi_remove_pm_notifier(adev);
-		if (power_off) {
+		if (power_off &&
+		    !(dev->driver->probe_powered_off &&
+		      device_property_present(dev, "avoid-power-probe"))) {
 			/*
 			 * If the device's PM QoS resume latency limit or flags
 			 * have been exposed to user space, they have to be
@@ -1273,7 +1275,10 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 
 	acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
 	dev_pm_domain_set(dev, &acpi_general_pm_domain);
-	if (power_on) {
+
+	if (power_on &&
+	    !(dev->driver->probe_powered_off &&
+	      device_property_present(dev, "avoid-power-probe"))) {
 		acpi_dev_pm_full_power(adev);
 		acpi_device_wakeup_disable(adev);
 	}
diff --git a/include/linux/device.h b/include/linux/device.h
index e85264fb66161..2a459fd5b954a 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -245,6 +245,11 @@ enum probe_type {
  * @owner:	The module owner.
  * @mod_name:	Used for built-in modules.
  * @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @probe_powered_off: The driver supports its probe function being called while
+ *		       the device is powered off, independently of the expected
+ *		       behaviour on combination of a given bus and firmware
+ *		       interface etc. The driver is responsible for powering the
+ *		       device on using runtime PM in such case.
  * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
  * @of_match_table: The open firmware table.
  * @acpi_match_table: The ACPI match table.
@@ -282,6 +287,7 @@ struct device_driver {
 	const char		*mod_name;	/* used for built-in modules */
 
 	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
+	bool probe_powered_off;
 	enum probe_type probe_type;
 
 	const struct of_device_id	*of_match_table;
-- 
2.11.0

[PATCH 2/5] ACPI: Add a convenience function to tell a device is suspended in probe

[Sakari Ailus]

Add a convenience function to tell whether a device is suspended for probe
or remove, for busses where the custom is that drivers don't need to
resume devices in probe, or suspend them in their remove handlers.

Returns false on non-ACPI systems.

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

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index ca2409a30d26d..774bf4380afd3 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -1287,4 +1287,37 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
 	return 1;
 }
 EXPORT_SYMBOL_GPL(acpi_dev_pm_attach);
+
+/**
+ * acpi_dev_powered_off_for_probe - Tell if a device is powered off for probe
+ * @dev: The device
+ *
+ * Tell whether a given device has not been powered on for probe or remove.
+ *
+ * Drivers of devices on certain busses such as I²C can generally assume (on
+ * ACPI based systems) that the devices they control are powered on without
+ * driver having to do anything about it. Using struct device_driver.probe_off
+ * and "avoid-power-probe" property, this can be negated and the driver has full
+ * control of the device power management. Always returns false on non-ACPI
+ * based systems. True is returned on ACPI based systems iff the device is
+ * powered off.
+ */
+bool acpi_dev_powered_off_for_probe(struct device *dev)
+{
+	int power_state;
+	int ret;
+
+	if (!is_acpi_device_node(dev_fwnode(dev)))
+		return false;
+
+	ret = acpi_device_get_power(ACPI_COMPANION(dev), &power_state);
+	if (ret) {
+		dev_warn(dev, "Cannot obtain power state (%d)\n", ret);
+		return false;
+	}
+
+	return power_state != ACPI_STATE_D0;
+}
+EXPORT_SYMBOL_GPL(acpi_dev_powered_off_for_probe);
+
 #endif /* CONFIG_PM */
diff --git a/include/linux/acpi.h b/include/linux/acpi.h
index e22c237be46ae..fc6b97aadcf17 100644
--- a/include/linux/acpi.h
+++ b/include/linux/acpi.h
@@ -913,6 +913,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_powered_off_for_probe(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; }
@@ -922,6 +923,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 {
 	return 0;
 }
+static inline bool acpi_dev_powered_off_for_probe(struct device *dev)
+{
+	return false;
+}
 #endif
 
 #if defined(CONFIG_ACPI) && defined(CONFIG_PM_SLEEP)
-- 
2.11.0

[PATCH 3/5] ov5670: Support probe whilst the device is off

[Sakari Ailus]

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: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebdf..57e8b92f90e09 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
 	struct ov5670 *ov5670;
 	const char *err_msg;
 	u32 input_clk = 0;
+	bool powered_off;
 	int ret;
 
 	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
@@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
 
-	/* Check module identity */
-	ret = ov5670_identify_module(ov5670);
-	if (ret) {
-		err_msg = "ov5670_identify_module() error";
-		goto error_print;
+	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+	if (!powered_off) {
+		/* Check module identity */
+		ret = ov5670_identify_module(ov5670);
+		if (ret) {
+			err_msg = "ov5670_identify_module() error";
+			goto error_print;
+		}
 	}
 
 	mutex_init(&ov5670->mutex);
@@ -2500,11 +2504,9 @@ 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.
-	 */
-	pm_runtime_set_active(&client->dev);
+	/* Don't set the device's state to active if it's powered off. */
+	if (!powered_off)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 
@@ -2546,7 +2548,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 */ }
 };
 
@@ -2556,6 +2558,7 @@ MODULE_DEVICE_TABLE(acpi, ov5670_acpi_ids);
 static struct i2c_driver ov5670_i2c_driver = {
 	.driver = {
 		.name = "ov5670",
+		.probe_powered_off = true,
 		.pm = &ov5670_pm_ops,
 		.acpi_match_table = ACPI_PTR(ov5670_acpi_ids),
 	},
-- 
2.11.0

[PATCH 4/5] 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 | 25 ++++++++++++++-----------
 1 file changed, 14 insertions(+), 11 deletions(-)

diff --git a/drivers/media/i2c/imx319.c b/drivers/media/i2c/imx319.c
index 17c2e4b41221e..e929de86a8e22 100644
--- a/drivers/media/i2c/imx319.c
+++ b/drivers/media/i2c/imx319.c
@@ -2424,6 +2424,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
 static int imx319_probe(struct i2c_client *client)
 {
 	struct imx319 *imx319;
+	bool powered_off;
 	int ret;
 	u32 i;
 
@@ -2436,11 +2437,14 @@ static int imx319_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx319->sd, client, &imx319_subdev_ops);
 
-	/* Check module identity */
-	ret = imx319_identify_module(imx319);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		goto error_probe;
+	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+	if (!powered_off) {
+		/* 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);
@@ -2492,11 +2496,9 @@ static int imx319_probe(struct i2c_client *client)
 	if (ret < 0)
 		goto error_media_entity;
 
-	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
-	 */
-	pm_runtime_set_active(&client->dev);
+	/* Don't set the device's state to active if it's powered off. */
+	if (!powered_off)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 
@@ -2536,7 +2538,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);
@@ -2544,6 +2546,7 @@ MODULE_DEVICE_TABLE(acpi, imx319_acpi_ids);
 static struct i2c_driver imx319_i2c_driver = {
 	.driver = {
 		.name = "imx319",
+		.probe_powered_off = true,
 		.pm = &imx319_pm_ops,
 		.acpi_match_table = ACPI_PTR(imx319_acpi_ids),
 	},
-- 
2.11.0

[PATCH 5/5] 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 | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 63aa541c96088..b9dbe5b6a97be 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -579,6 +579,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 powered_off;
 	struct regmap *regmap;
 	size_t at24_size;
 	bool writable;
@@ -701,19 +702,24 @@ static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
-	/* enable runtime pm */
-	pm_runtime_set_active(dev);
+	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+	if (!powered_off)
+		pm_runtime_set_active(dev);
+
 	pm_runtime_enable(dev);
 
 	/*
-	 * Perform a one-byte test read to verify that the
-	 * chip is functional.
+	 * Perform a one-byte test read to verify that the chip is functional,
+	 * unless powering on the device is to be avoided during probe (i.e.
+	 * it's powered off right now).
 	 */
-	err = at24_read(at24, 0, &test_byte, 1);
-	pm_runtime_idle(dev);
-	if (err) {
-		err = -ENODEV;
-		goto err_clients;
+	if (!powered_off) {
+		err = at24_read(at24, 0, &test_byte, 1);
+		pm_runtime_idle(dev);
+		if (err) {
+			err = -ENODEV;
+			goto err_clients;
+		}
 	}
 
 	nvmem_config.name = dev_name(dev);
@@ -752,12 +758,15 @@ static int at24_probe(struct i2c_client *client)
 static int at24_remove(struct i2c_client *client)
 {
 	struct at24_data *at24;
+	bool powered_off;
 
 	at24 = i2c_get_clientdata(client);
 
 	at24_remove_dummy_clients(at24);
 	pm_runtime_disable(&client->dev);
-	pm_runtime_set_suspended(&client->dev);
+	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
+	if (!powered_off)
+		pm_runtime_set_suspended(&client->dev);
 
 	return 0;
 }
@@ -765,6 +774,7 @@ static int at24_remove(struct i2c_client *client)
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.probe_powered_off = true,
 		.of_match_table = at24_of_match,
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},
-- 
2.11.0

Re: [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Rafael J. Wysocki]

On Friday, May 10, 2019 12:09:26 PM CEST Sakari Ailus wrote:
> Allow drivers and firmware tell ACPI that there's no need to power on a
> device for probe. This requires both a hint from the firmware as well as
> an indication from a driver to leave the device off.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/acpi/device_pm.c | 9 +++++++--
>  include/linux/device.h   | 6 ++++++
>  2 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
> index b859d75eaf9f6..ca2409a30d26d 100644
> --- a/drivers/acpi/device_pm.c
> +++ b/drivers/acpi/device_pm.c
> @@ -1225,7 +1225,9 @@ static void acpi_dev_pm_detach(struct device *dev, bool power_off)
>  	if (adev && dev->pm_domain == &acpi_general_pm_domain) {
>  		dev_pm_domain_set(dev, NULL);
>  		acpi_remove_pm_notifier(adev);
> -		if (power_off) {
> +		if (power_off &&
> +		    !(dev->driver->probe_powered_off &&
> +		      device_property_present(dev, "avoid-power-probe"))) {
>  			/*
>  			 * If the device's PM QoS resume latency limit or flags
>  			 * have been exposed to user space, they have to be
> @@ -1273,7 +1275,10 @@ int acpi_dev_pm_attach(struct device *dev, bool power_on)
>  
>  	acpi_add_pm_notifier(adev, dev, acpi_pm_notify_work_func);
>  	dev_pm_domain_set(dev, &acpi_general_pm_domain);
> -	if (power_on) {
> +
> +	if (power_on &&
> +	    !(dev->driver->probe_powered_off &&
> +	      device_property_present(dev, "avoid-power-probe"))) {
>  		acpi_dev_pm_full_power(adev);
>  		acpi_device_wakeup_disable(adev);
>  	}
> diff --git a/include/linux/device.h b/include/linux/device.h
> index e85264fb66161..2a459fd5b954a 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -245,6 +245,11 @@ enum probe_type {
>   * @owner:	The module owner.
>   * @mod_name:	Used for built-in modules.
>   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> + * @probe_powered_off: The driver supports its probe function being called while
> + *		       the device is powered off, independently of the expected
> + *		       behaviour on combination of a given bus and firmware
> + *		       interface etc. The driver is responsible for powering the
> + *		       device on using runtime PM in such case.
>   * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
>   * @of_match_table: The open firmware table.
>   * @acpi_match_table: The ACPI match table.
> @@ -282,6 +287,7 @@ struct device_driver {
>  	const char		*mod_name;	/* used for built-in modules */
>  
>  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> +	bool probe_powered_off;

This is a bit of a misnomer IMO, because it is not just about devices that are completely off.
From the ACPI perspective that is about all devices not in D0, which may mean gated clocks
etc.

I would call it probe_low_power or similar and analogously in patch [2/5], and apart from this
I have no objections against this series, but I would suggest to CC the next iteration of it
to Greg K-H and the LKML as it touches the driver core.

>  	enum probe_type probe_type;
>  
>  	const struct of_device_id	*of_match_table;
> 

Re: [PATCH 3/5] ov5670: Support probe whilst the device is off

[Tomasz Figa]

Hi Sakari,

On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote:
> 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: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
>  1 file changed, 14 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 041fcbb4eebdf..57e8b92f90e09 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
>  	struct ov5670 *ov5670;
>  	const char *err_msg;
>  	u32 input_clk = 0;
> +	bool powered_off;
>  	int ret;
>  
>  	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> @@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
>  	/* Initialize subdev */
>  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
>  
> -	/* Check module identity */
> -	ret = ov5670_identify_module(ov5670);
> -	if (ret) {
> -		err_msg = "ov5670_identify_module() error";
> -		goto error_print;
> +	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
> +	if (!powered_off) {
> +		/* Check module identity */
> +		ret = ov5670_identify_module(ov5670);
> +		if (ret) {
> +			err_msg = "ov5670_identify_module() error";
> +			goto error_print;
> +		}
>  	}

I don't like the fact that we can't detect any hardware connection issue
here anymore and we would actually get some obscure failure when we
actually start streaming.

Wouldn't it be possible to still keep this behavior of not powering on
the device at boot-up if no driver is bound and then have this driver
built as a module and loaded later when the camera is to be used for the
first time after the system boots?

Best regards,
Tomasz

Re: [PATCH 3/5] ov5670: Support probe whilst the device is off

[Sakari Ailus]

Hi Tomasz,

On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote:
> Hi Sakari,
> 
> On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote:
> > 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: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
> >  1 file changed, 14 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index 041fcbb4eebdf..57e8b92f90e09 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
> >  	struct ov5670 *ov5670;
> >  	const char *err_msg;
> >  	u32 input_clk = 0;
> > +	bool powered_off;
> >  	int ret;
> >  
> >  	device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > @@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
> >  	/* Initialize subdev */
> >  	v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> >  
> > -	/* Check module identity */
> > -	ret = ov5670_identify_module(ov5670);
> > -	if (ret) {
> > -		err_msg = "ov5670_identify_module() error";
> > -		goto error_print;
> > +	powered_off = acpi_dev_powered_off_for_probe(&client->dev);
> > +	if (!powered_off) {
> > +		/* Check module identity */
> > +		ret = ov5670_identify_module(ov5670);
> > +		if (ret) {
> > +			err_msg = "ov5670_identify_module() error";
> > +			goto error_print;
> > +		}
> >  	}
> 
> I don't like the fact that we can't detect any hardware connection issue
> here anymore and we would actually get some obscure failure when we
> actually start streaming.
> 
> Wouldn't it be possible to still keep this behavior of not powering on
> the device at boot-up if no driver is bound and then have this driver
> built as a module and loaded later when the camera is to be used for the
> first time after the system boots?

That'd be a way to work around this, but the downside would be that the
user space would need to know not only which drivers to load, but also
which drivers _not_ to load. The user space could obtain the former from
the kernel but not the latter, it'd be system specific configuration.

Moving the responsibility of loading the driver to user space would also
not address figuring out whether the sensor is accessible through its
control bus: you have to power it on to do that. In fact, if you want to be
sure that the hardware is all right, you have to start streaming on the
device first and that is not a part of a typical driver initialisation
sequence. Just checking the sensor is accessible over I²C is not enough.

The proposed solution addresses the problem without user space changes.

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 1/5] ACPI: Enable driver and firmware hints to control power at probe time

[Sakari Ailus]

Hi Rafael,

On Fri, May 31, 2019 at 11:17:10AM +0200, Rafael J. Wysocki wrote:
> On Friday, May 10, 2019 12:09:26 PM CEST Sakari Ailus wrote:
...
> > diff --git a/include/linux/device.h b/include/linux/device.h
> > index e85264fb66161..2a459fd5b954a 100644
> > --- a/include/linux/device.h
> > +++ b/include/linux/device.h
> > @@ -245,6 +245,11 @@ enum probe_type {
> >   * @owner:	The module owner.
> >   * @mod_name:	Used for built-in modules.
> >   * @suppress_bind_attrs: Disables bind/unbind via sysfs.
> > + * @probe_powered_off: The driver supports its probe function being called while
> > + *		       the device is powered off, independently of the expected
> > + *		       behaviour on combination of a given bus and firmware
> > + *		       interface etc. The driver is responsible for powering the
> > + *		       device on using runtime PM in such case.
> >   * @probe_type:	Type of the probe (synchronous or asynchronous) to use.
> >   * @of_match_table: The open firmware table.
> >   * @acpi_match_table: The ACPI match table.
> > @@ -282,6 +287,7 @@ struct device_driver {
> >  	const char		*mod_name;	/* used for built-in modules */
> >  
> >  	bool suppress_bind_attrs;	/* disables bind/unbind via sysfs */
> > +	bool probe_powered_off;
> 
> This is a bit of a misnomer IMO, because it is not just about devices that are completely off.
> From the ACPI perspective that is about all devices not in D0, which may mean gated clocks
> etc.
> 
> I would call it probe_low_power or similar and analogously in patch [2/5], and apart from this
> I have no objections against this series, but I would suggest to CC the next iteration of it
> to Greg K-H and the LKML as it touches the driver core.

Ack. I'll do that for v2.

Thanks for the review!

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/5] ov5670: Support probe whilst the device is off

[Tomasz Figa]

On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz,
>
> On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote:
> > Hi Sakari,
> >
> > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote:
> > > 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: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > ---
> > >  drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
> > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > index 041fcbb4eebdf..57e8b92f90e09 100644
> > > --- a/drivers/media/i2c/ov5670.c
> > > +++ b/drivers/media/i2c/ov5670.c
> > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
> > >     struct ov5670 *ov5670;
> > >     const char *err_msg;
> > >     u32 input_clk = 0;
> > > +   bool powered_off;
> > >     int ret;
> > >
> > >     device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > > @@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
> > >     /* Initialize subdev */
> > >     v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> > >
> > > -   /* Check module identity */
> > > -   ret = ov5670_identify_module(ov5670);
> > > -   if (ret) {
> > > -           err_msg = "ov5670_identify_module() error";
> > > -           goto error_print;
> > > +   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
> > > +   if (!powered_off) {
> > > +           /* Check module identity */
> > > +           ret = ov5670_identify_module(ov5670);
> > > +           if (ret) {
> > > +                   err_msg = "ov5670_identify_module() error";
> > > +                   goto error_print;
> > > +           }
> > >     }
> >
> > I don't like the fact that we can't detect any hardware connection issue
> > here anymore and we would actually get some obscure failure when we
> > actually start streaming.
> >
> > Wouldn't it be possible to still keep this behavior of not powering on
> > the device at boot-up if no driver is bound and then have this driver
> > built as a module and loaded later when the camera is to be used for the
> > first time after the system boots?
>
> That'd be a way to work around this, but the downside would be that the
> user space would need to know not only which drivers to load, but also
> which drivers _not_ to load. The user space could obtain the former from
> the kernel but not the latter, it'd be system specific configuration.
>
> Moving the responsibility of loading the driver to user space would also
> not address figuring out whether the sensor is accessible through its
> control bus: you have to power it on to do that. In fact, if you want to be
> sure that the hardware is all right, you have to start streaming on the
> device first and that is not a part of a typical driver initialisation
> sequence. Just checking the sensor is accessible over I涎 is not enough.
>
> The proposed solution addresses the problem without user space changes.

I guess that makes sense indeed. If going this way, why not just move
all the hardware access from probe to streamon and avoid any
conditional checks at all?

Best regards,
Tomasz

Re: [PATCH 3/5] ov5670: Support probe whilst the device is off

[Sakari Ailus]

Hi Tomasz,

On Fri, Jun 07, 2019 at 04:54:06PM +0900, Tomasz Figa wrote:
> On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus
> <sakari.ailus@linux.intel.com> wrote:
> >
> > Hi Tomasz,
> >
> > On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote:
> > > Hi Sakari,
> > >
> > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote:
> > > > 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: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > ---
> > > >  drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
> > > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > > index 041fcbb4eebdf..57e8b92f90e09 100644
> > > > --- a/drivers/media/i2c/ov5670.c
> > > > +++ b/drivers/media/i2c/ov5670.c
> > > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
> > > >     struct ov5670 *ov5670;
> > > >     const char *err_msg;
> > > >     u32 input_clk = 0;
> > > > +   bool powered_off;
> > > >     int ret;
> > > >
> > > >     device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > > > @@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
> > > >     /* Initialize subdev */
> > > >     v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> > > >
> > > > -   /* Check module identity */
> > > > -   ret = ov5670_identify_module(ov5670);
> > > > -   if (ret) {
> > > > -           err_msg = "ov5670_identify_module() error";
> > > > -           goto error_print;
> > > > +   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
> > > > +   if (!powered_off) {
> > > > +           /* Check module identity */
> > > > +           ret = ov5670_identify_module(ov5670);
> > > > +           if (ret) {
> > > > +                   err_msg = "ov5670_identify_module() error";
> > > > +                   goto error_print;
> > > > +           }
> > > >     }
> > >
> > > I don't like the fact that we can't detect any hardware connection issue
> > > here anymore and we would actually get some obscure failure when we
> > > actually start streaming.
> > >
> > > Wouldn't it be possible to still keep this behavior of not powering on
> > > the device at boot-up if no driver is bound and then have this driver
> > > built as a module and loaded later when the camera is to be used for the
> > > first time after the system boots?
> >
> > That'd be a way to work around this, but the downside would be that the
> > user space would need to know not only which drivers to load, but also
> > which drivers _not_ to load. The user space could obtain the former from
> > the kernel but not the latter, it'd be system specific configuration.
> >
> > Moving the responsibility of loading the driver to user space would also
> > not address figuring out whether the sensor is accessible through its
> > control bus: you have to power it on to do that. In fact, if you want to be
> > sure that the hardware is all right, you have to start streaming on the
> > device first and that is not a part of a typical driver initialisation
> > sequence. Just checking the sensor is accessible over I涎 is not enough.
> >
> > The proposed solution addresses the problem without user space changes.
> 
> I guess that makes sense indeed. If going this way, why not just move
> all the hardware access from probe to streamon and avoid any
> conditional checks at all?

My apologies for the late answer.

In that case there would be no way to verify the hardware actually is
there, even on systems where there is no adverse effect from doing that.
For a sensor driver this could be just fine, but I have doubts it'd be
appropriate for e.g. the at24 driver.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 3/5] ov5670: Support probe whilst the device is off

[Tomasz Figa]

On Mon, Aug 26, 2019 at 5:38 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
>
> Hi Tomasz,
>
> On Fri, Jun 07, 2019 at 04:54:06PM +0900, Tomasz Figa wrote:
> > On Wed, Jun 5, 2019 at 7:15 PM Sakari Ailus
> > <sakari.ailus@linux.intel.com> wrote:
> > >
> > > Hi Tomasz,
> > >
> > > On Wed, Jun 05, 2019 at 04:07:52PM +0900, Tomasz Figa wrote:
> > > > Hi Sakari,
> > > >
> > > > On Fri, May 10, 2019 at 01:09:28PM +0300, Sakari Ailus wrote:
> > > > > 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: Sakari Ailus <sakari.ailus@linux.intel.com>
> > > > > ---
> > > > >  drivers/media/i2c/ov5670.c | 25 ++++++++++++++-----------
> > > > >  1 file changed, 14 insertions(+), 11 deletions(-)
> > > > >
> > > > > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > > > > index 041fcbb4eebdf..57e8b92f90e09 100644
> > > > > --- a/drivers/media/i2c/ov5670.c
> > > > > +++ b/drivers/media/i2c/ov5670.c
> > > > > @@ -2444,6 +2444,7 @@ static int ov5670_probe(struct i2c_client *client)
> > > > >     struct ov5670 *ov5670;
> > > > >     const char *err_msg;
> > > > >     u32 input_clk = 0;
> > > > > +   bool powered_off;
> > > > >     int ret;
> > > > >
> > > > >     device_property_read_u32(&client->dev, "clock-frequency", &input_clk);
> > > > > @@ -2460,11 +2461,14 @@ static int ov5670_probe(struct i2c_client *client)
> > > > >     /* Initialize subdev */
> > > > >     v4l2_i2c_subdev_init(&ov5670->sd, client, &ov5670_subdev_ops);
> > > > >
> > > > > -   /* Check module identity */
> > > > > -   ret = ov5670_identify_module(ov5670);
> > > > > -   if (ret) {
> > > > > -           err_msg = "ov5670_identify_module() error";
> > > > > -           goto error_print;
> > > > > +   powered_off = acpi_dev_powered_off_for_probe(&client->dev);
> > > > > +   if (!powered_off) {
> > > > > +           /* Check module identity */
> > > > > +           ret = ov5670_identify_module(ov5670);
> > > > > +           if (ret) {
> > > > > +                   err_msg = "ov5670_identify_module() error";
> > > > > +                   goto error_print;
> > > > > +           }
> > > > >     }
> > > >
> > > > I don't like the fact that we can't detect any hardware connection issue
> > > > here anymore and we would actually get some obscure failure when we
> > > > actually start streaming.
> > > >
> > > > Wouldn't it be possible to still keep this behavior of not powering on
> > > > the device at boot-up if no driver is bound and then have this driver
> > > > built as a module and loaded later when the camera is to be used for the
> > > > first time after the system boots?
> > >
> > > That'd be a way to work around this, but the downside would be that the
> > > user space would need to know not only which drivers to load, but also
> > > which drivers _not_ to load. The user space could obtain the former from
> > > the kernel but not the latter, it'd be system specific configuration.
> > >
> > > Moving the responsibility of loading the driver to user space would also
> > > not address figuring out whether the sensor is accessible through its
> > > control bus: you have to power it on to do that. In fact, if you want to be
> > > sure that the hardware is all right, you have to start streaming on the
> > > device first and that is not a part of a typical driver initialisation
> > > sequence. Just checking the sensor is accessible over I涎 is not enough.
> > >
> > > The proposed solution addresses the problem without user space changes.
> >
> > I guess that makes sense indeed. If going this way, why not just move
> > all the hardware access from probe to streamon and avoid any
> > conditional checks at all?
>
> My apologies for the late answer.
>
> In that case there would be no way to verify the hardware actually is
> there, even on systems where there is no adverse effect from doing that.
> For a sensor driver this could be just fine, but I have doubts it'd be
> appropriate for e.g. the at24 driver.

For an eeprom, the first read could fail. That said, I agree that for
systems on which there is no such concern it would still be desirable
to check if the device is accessible in probe.

Anyway, I think you convinced me. :)

Reviewed-by: Tomasz Figa <tfiga@chromium.org>

Best regards,
Tomasz