Support running driver's probe for a device powered off

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 <[email protected]>

---
 drivers/i2c/i2c-core-base.c | 17 ++++++++++++++---
 include/linux/i2c.h         | 14 ++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)

drivers/i2c/i2c-core-base.c +0 -0

index 34a9609f256da..cde9cf49a07e6 100644

Quoted 20 lines · 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 <[email protected]>

---
 drivers/i2c/i2c-core-base.c | 17 ++++++++++++++---
 include/linux/i2c.h         | 14 ++++++++++++++
 2 files changed, 28 insertions(+), 3 deletions(-)

drivers/i2c/i2c-core-base.c +0 -0

index 34a9609f256da..cde9cf49a07e6 100644

drivers/i2c/i2c-core-base.c static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client) · line 444

Quoted 9 lines

drivers/i2c/i2c-core-base.c +6 -0

@@ -436,6 +436,14 @@ 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 &&
+		device_property_present(dev, "allow-low-power-probe");

I assume this change makes even the DT property "allow-low-power-probe" work in the same way. Should we have proper DT binding for that ?

This comment applies for any property using device_property_* but has no explicit DT binding ? Just asking the question to know the strategy followed. Sorry if this is redundant question, feel free to point me to the past discussions.

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

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/i2c/ov5670.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/media/i2c/ov5670.c +1 -3

index f26252e35e08d..1f75b888d2a18 100644
@@ -2456,6 +2456,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 +2473,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) {

Quoted 36 lines · 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.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/i2c/ov5670.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/media/i2c/ov5670.c +8 -5

index f26252e35e08d..1f75b888d2a18 100644
@@ -2456,6 +2456,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 +2473,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;

drivers/media/i2c/ov5670.c static int ov5670_probe(struct i2c_client *client) · line 2483

Quoted 1 line

drivers/media/i2c/ov5670.c +1 -0

+	

Sakari, thanks for your patch.
one question - With this change, there will be no chance for driver to guarantee that the camera sensor plugged in is the camera that the matched driver actually can drive until try to streaming the camera, so is it necessary to return appropriate error in .s_stream ops to notify user it is not the hardware that current driver can drive? if no other better way.

Quoted 36 lines · 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.

Signed-off-by Sakari Ailus <[email protected]>

---
 drivers/media/i2c/ov5670.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/media/i2c/ov5670.c +8 -5

index f26252e35e08d..1f75b888d2a18 100644
@@ -2456,6 +2456,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 +2473,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;

drivers/media/i2c/ov5670.c static int ov5670_probe(struct i2c_client *client) · line 2483

Quoted 1 line

drivers/media/i2c/ov5670.c +1 -0

+	

Sakari, thanks for your patch.
one question - With this change, there will be no chance for driver to guarantee that the camera sensor plugged in is the camera that the matched driver actually can drive until try to streaming the camera, so is it necessary to return appropriate error in .s_stream ops to notify user it is not the hardware that current driver can drive? if no other better way.

Hi all,

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.

Quoted 2 lines · Sakari Ailus

Hi all,

...snip...

Quoted 21 lines

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.

Quoted 4 lines

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.

Sakari, we meet one issue - once the vcm sub-device registered, the user space will try to open the VCM (I have not figure out who did that), it will also trigger the acpi pm resume/suspend, as the VCM always shares same power rail with camera sensor, so the privacy LED still has a blink.

Quoted 2 lines

...snip...

Quoted 2 lines · Sakari Ailus

Hi all,

...snip...

Quoted 21 lines

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.

Quoted 4 lines

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.

Sakari, we meet one issue - once the vcm sub-device registered, the user space will try to open the VCM (I have not figure out who did that), it will also trigger the acpi pm resume/suspend, as the VCM always shares same power rail with camera sensor, so the privacy LED still has a blink.

Quoted 33 lines · Bingbu Cao

Quoted 2 lines · Sakari Ailus

Hi all,

...snip...

Quoted 21 lines

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.

Quoted 4 lines

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.

Quoted 4 lines

Sakari, we meet one issue - once the vcm sub-device registered, the user space will try to open the VCM (I have not figure out who did that), it will also trigger the acpi pm resume/suspend, as the VCM always shares same power rail with camera sensor, so the privacy LED still has a blink.

It's not always the case, as on some designs there are multiple power rails to the sensor and one drives the LED, while the other drives the VCM. That said, it would be still good to solve it in either case.

Perhaps we need some more general discussion on the side effects of simply opening and querying a device. Most of V4L2 drivers these days are designed to avoid powering up the hardware until it's absolutely needed to do so. However, for non-streaming subdevs that are directly controlled by the userspace, like VCM, it's a common practice to power up on open and down on release. This is because they don't have a "streaming" state, so the driver has no way to determine when the power is needed. I wonder if there is a way to improve this.