Support running driver's probe for a device powered off

include/linux/acpi.h static inline int acpi_dev_pm_attach(struct device *dev, bool power_on) · line 990

Quoted 4 lines

include/linux/acpi.h +0 -0

@@ -987,6 +988,10 @@ static inline int acpi_dev_pm_attach(struct device *dev, bool power_on)
 {
 	return 0;
 }

Need a blank line here, ;).

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

---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

drivers/misc/eeprom/at24.c +26 -18

index 8f5de5f10bbea..2d24e33788d7d 100644
@@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
        bool i2c_fn_i2c, i2c_fn_block;
        unsigned int i, num_addresses;
        struct at24_data *at24;
+       bool low_power;
        struct regmap *regmap;
        bool writable;
        u8 test_byte;

@@ -733,25 +734,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);

@@ -771,9 +777,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;
 }

@@ -810,6 +818,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

This currently conflicts with the fix I queued for at24 for v5.9. Which tree is going to take this series?

Quoted 4 lines

This currently conflicts with the fix I queued for at24 for v5.9. Which tree is going to take this series?

I recall we agreed on I2C.

Quoted 5 lines · Wolfram Sang

Quoted 4 lines

This currently conflicts with the fix I queued for at24 for v5.9. Which tree is going to take this series?

Quoted 2 lines

I recall we agreed on I2C.

Sakari,

can you rebase the at24 driver patch on top of Wolfram's tree as soon as he merges my PR with at24 fixes?

Quoted 5 lines · Wolfram Sang

Quoted 4 lines

This currently conflicts with the fix I queued for at24 for v5.9. Which tree is going to take this series?

Quoted 2 lines

I recall we agreed on I2C.

Sakari,

can you rebase the at24 driver patch on top of Wolfram's tree as soon as he merges my PR with at24 fixes?

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

---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

drivers/misc/eeprom/at24.c +5 -5

index 8f5de5f10bbea..2d24e33788d7d 100644
@@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
        bool i2c_fn_i2c, i2c_fn_block;
        unsigned int i, num_addresses;
        struct at24_data *at24;
+       bool low_power;
        struct regmap *regmap;
        bool writable;
        u8 test_byte;

@@ -733,25 +734,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) {

drivers/misc/eeprom/at24.c +16 -11

        /*
-        * 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);

@@ -771,9 +777,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;
 }

@@ -810,6 +818,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

Hi Sakari,

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

---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

drivers/misc/eeprom/at24.c +5 -5

index 8f5de5f10bbea..2d24e33788d7d 100644
@@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
        bool i2c_fn_i2c, i2c_fn_block;
        unsigned int i, num_addresses;
        struct at24_data *at24;
+       bool low_power;
        struct regmap *regmap;
        bool writable;
        u8 test_byte;

@@ -733,25 +734,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) {

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client) · line 748

Quoted 10 lines

drivers/misc/eeprom/at24.c +5 -2

+                       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);
 

What's the guarantee that at this point the runtime PM wouldn't suspend the device? Notice that the nvmem device is already exposed to the userspace, which could trigger pm runtime gets and puts (and thus idles as well).

Best regards,
Tomasz

Quoted 8 lines

drivers/misc/eeprom/at24.c +3 -3

        /*
-        * 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);

Quoted 40 lines

drivers/misc/eeprom/at24.c +13 -8

-       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);

@@ -771,9 +777,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;
 }

@@ -810,6 +818,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

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.

Hi Sakari,

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

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.

Apologies for having joined this discussion this late.

This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time.

I'm going to try these patches, but in my case there are a few differences that need a better understanding.

First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT.

Hi Luca,

Quoted 40 lines · Luca Ceresoli

Hi Sakari,

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

Quoted 6 lines

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.

Apologies for having joined this discussion this late.

No worries. But thanks for the comments.

Quoted 11 lines

This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time.

I'm going to try these patches, but in my case there are a few differences that need a better understanding.

First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT.

acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI power state of the device during probe or remove. It is not needed on DT since the power state of the device is controlled directly by the driver. On I²C ACPI devices, it's the framework that powers them on for probe.

You could have a helper function on DT to tell a driver what to do in probe, but the functionality in that case is unrelated.

Hi Luca,

Quoted 91 lines · Luca Ceresoli

Hi Sakari,

Quoted 64 lines · Sakari Ailus

Hi Luca,

Quoted 40 lines · Luca Ceresoli

Hi Sakari,

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

Quoted 6 lines

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.

Apologies for having joined this discussion this late.

No worries. But thanks for the comments.

Quoted 11 lines

This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time.

I'm going to try these patches, but in my case there are a few differences that need a better understanding.

First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT.

Quoted 4 lines

acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI power state of the device during probe or remove. It is not needed on DT since the power state of the device is controlled directly by the driver. On I²C ACPI devices, it's the framework that powers them on for probe.

I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.

Quoted 2 lines

You could have a helper function on DT to tell a driver what to do in probe, but the functionality in that case is unrelated.

So in case of DT we might think of a function that just tells whether the device is marked to allow low-power probe, but it's just an info from DT:

int mydriver_probe(struct i2c_client *client)

{
	...
	low_power = of_dev_state_low_power(&client->dev);
	if (!low_power) {
		mydriver_initialize(); /* power+clocks, write regs */
 	}
	...
}

Quoted 3 lines

...and, if (low_power), call mydriver_initialize() at first usage.

I'm wondering whether this might make sense in mainline.

Quite possibly, if there are drivers that would need it.

The function should probably be called differently though as what it does is quite different after all.

Unless... we did the following:

- Redefine the I²C driver flag added by this patchset into what tells the I²C framework whether the driver does its own power management independently of the I²C framework. It could be called e.g. I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power management of the device, and the I²C framework would not power on the device for probe or remove.

- Add a firmware function to tell whether the device identification should take place during probe or not. For this is what we're really doing here from driver's point of view: lazy device probing.

There are no dependencies between the two but they can be used together to implement the same functionality as this patchset currently does. This way also the differences between driver implementations for ACPI and DT can be reduced as the logic is the same.

Further on, with this approach, if other busses happen to need this functionality in the future, it would be straightforward to add support --- it only takes to query whether it's indicated by the DT or ACPI property.

Thoughts, opinions?

Hi Sakari,

Quoted 112 lines · Sakari Ailus

Hi Luca,

Quoted 91 lines · Luca Ceresoli

Hi Sakari,

Quoted 64 lines · Sakari Ailus

Hi Luca,

Quoted 40 lines · Luca Ceresoli

Hi Sakari,

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

Quoted 6 lines

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.

Apologies for having joined this discussion this late.

No worries. But thanks for the comments.

Quoted 11 lines

This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time.

I'm going to try these patches, but in my case there are a few differences that need a better understanding.

First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT.

Quoted 4 lines

acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI power state of the device during probe or remove. It is not needed on DT since the power state of the device is controlled directly by the driver. On I²C ACPI devices, it's the framework that powers them on for probe.

I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.

Quoted 2 lines

You could have a helper function on DT to tell a driver what to do in probe, but the functionality in that case is unrelated.

So in case of DT we might think of a function that just tells whether the device is marked to allow low-power probe, but it's just an info from DT:

int mydriver_probe(struct i2c_client *client)

{
	...
	low_power = of_dev_state_low_power(&client->dev);
	if (!low_power) {
		mydriver_initialize(); /* power+clocks, write regs */
 	}
	...
}

Quoted 3 lines

...and, if (low_power), call mydriver_initialize() at first usage.

I'm wondering whether this might make sense in mainline.

Quite possibly, if there are drivers that would need it.

The function should probably be called differently though as what it does is quite different after all.

Unless... we did the following:

- Redefine the I²C driver flag added by this patchset into what tells the I²C framework whether the driver does its own power management independently of the I²C framework. It could be called e.g. I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power management of the device, and the I²C framework would not power on the device for probe or remove.

Quoted 3 lines

- Add a firmware function to tell whether the device identification should take place during probe or not. For this is what we're really doing here from driver's point of view: lazy device probing.

Indeed my needs have nothing to do with power management. What I need is lazy device probing as the I2C bus may need time before it can be used. From the driver code point of view it looks similar (there's an if() around initializations in probe() and init is done later if needed), but the usage is different.

Another approach would be to add a new I2C driver operation [say init_hw()], then move code for lazy init out of probe() into init_hw(). probe() would still allocate resources. init_hw() would be called by the framework (or the controller driver?) when it knows eveything is ready. Just wild thoughts while I'm trying to focus the problem...

Hi Luca,

Quoted 125 lines · Luca Ceresoli

Hi Sakari,

Quoted 112 lines · Sakari Ailus

Hi Luca,

Quoted 91 lines · Luca Ceresoli

Hi Sakari,

Quoted 64 lines · Sakari Ailus

Hi Luca,

Quoted 40 lines · Luca Ceresoli

Hi Sakari,

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

Quoted 6 lines

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.

Apologies for having joined this discussion this late.

No worries. But thanks for the comments.

Quoted 11 lines

This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time.

I'm going to try these patches, but in my case there are a few differences that need a better understanding.

First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT.

Quoted 4 lines

acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI power state of the device during probe or remove. It is not needed on DT since the power state of the device is controlled directly by the driver. On I²C ACPI devices, it's the framework that powers them on for probe.

I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.

Quoted 2 lines

You could have a helper function on DT to tell a driver what to do in probe, but the functionality in that case is unrelated.

So in case of DT we might think of a function that just tells whether the device is marked to allow low-power probe, but it's just an info from DT:

int mydriver_probe(struct i2c_client *client)

{
	...
	low_power = of_dev_state_low_power(&client->dev);
	if (!low_power) {
		mydriver_initialize(); /* power+clocks, write regs */
 	}
	...
}

Quoted 3 lines

...and, if (low_power), call mydriver_initialize() at first usage.

I'm wondering whether this might make sense in mainline.

Quite possibly, if there are drivers that would need it.

The function should probably be called differently though as what it does is quite different after all.

Unless... we did the following:

- Redefine the I²C driver flag added by this patchset into what tells the I²C framework whether the driver does its own power management independently of the I²C framework. It could be called e.g. I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power management of the device, and the I²C framework would not power on the device for probe or remove.

Quoted 3 lines

- Add a firmware function to tell whether the device identification should take place during probe or not. For this is what we're really doing here from driver's point of view: lazy device probing.

Indeed my needs have nothing to do with power management. What I need is lazy device probing as the I2C bus may need time before it can be used. From the driver code point of view it looks similar (there's an if() around initializations in probe() and init is done later if needed), but the usage is different.

Quoted 5 lines

Another approach would be to add a new I2C driver operation [say init_hw()], then move code for lazy init out of probe() into init_hw(). probe() would still allocate resources. init_hw() would be called by the framework (or the controller driver?) when it knows eveything is ready. Just wild thoughts while I'm trying to focus the problem...

What makes the controller driver not ready to operate the controller when the client devices are probed?

Hi Luca,

Quoted 91 lines · Luca Ceresoli

Hi Sakari,

Quoted 64 lines · Sakari Ailus

Hi Luca,

Quoted 40 lines · Luca Ceresoli

Hi Sakari,

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

Quoted 6 lines

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.

Apologies for having joined this discussion this late.

No worries. But thanks for the comments.

Quoted 11 lines

This patchset seems a good base to cover a different use case, where I also cannot access the physical device at probe time.

I'm going to try these patches, but in my case there are a few differences that need a better understanding.

First, I'm using device tree, not ACPI. In addition to adding OF support similar to the work you've done for ACPI, I think instead of acpi_dev_state_low_power() we should have a function that works for both ACPI and DT.

Quoted 4 lines

acpi_dev_state_low_power() is really ACPI specific: it does tell the ACPI power state of the device during probe or remove. It is not needed on DT since the power state of the device is controlled directly by the driver. On I²C ACPI devices, it's the framework that powers them on for probe.

I see, thanks for clarifying. I'm not used to ACPI so I didn't get that.

Quoted 2 lines

You could have a helper function on DT to tell a driver what to do in probe, but the functionality in that case is unrelated.

So in case of DT we might think of a function that just tells whether the device is marked to allow low-power probe, but it's just an info from DT:

int mydriver_probe(struct i2c_client *client)

{
	...
	low_power = of_dev_state_low_power(&client->dev);
	if (!low_power) {
		mydriver_initialize(); /* power+clocks, write regs */
 	}
	...
}

Quoted 3 lines

...and, if (low_power), call mydriver_initialize() at first usage.

I'm wondering whether this might make sense in mainline.

Quite possibly, if there are drivers that would need it.

The function should probably be called differently though as what it does is quite different after all.

Unless... we did the following:

- Redefine the I²C driver flag added by this patchset into what tells the I²C framework whether the driver does its own power management independently of the I²C framework. It could be called e.g. I2C_DRV_FL_FULL_PM, to indicate the driver is responsible for all power management of the device, and the I²C framework would not power on the device for probe or remove.

- Add a firmware function to tell whether the device identification should take place during probe or not. For this is what we're really doing here from driver's point of view: lazy device probing.

There are no dependencies between the two but they can be used together to implement the same functionality as this patchset currently does. This way also the differences between driver implementations for ACPI and DT can be reduced as the logic is the same.

Further on, with this approach, if other busses happen to need this functionality in the future, it would be straightforward to add support --- it only takes to query whether it's indicated by the DT or ACPI property.

Thoughts, opinions?

I think we might be overly complicating things. IMHO the series as is with the "i2c_" prefix removed from the flags introduced would be reusable as is for any other subsystem that needs it. Of course, for now, the handling of the flag would remain implemented only in the I2C subsystem.

Quoted 7 lines

I think we might be overly complicating things. IMHO the series as is with the "i2c_" prefix removed from the flags introduced would be reusable as is for any other subsystem that needs it. Of course, for now, the handling of the flag would remain implemented only in the I2C subsystem.

Just to be clear: you are suggesting to remove "i2c" from the DSD binding "i2c-allow-low-power-probe". And you are not talking about moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I recall the latter has been NACKed by gkh so far.

Quoted 8 lines · Wolfram Sang

Quoted 7 lines

I think we might be overly complicating things. IMHO the series as is with the "i2c_" prefix removed from the flags introduced would be reusable as is for any other subsystem that needs it. Of course, for now, the handling of the flag would remain implemented only in the I2C subsystem.

Quoted 5 lines

Just to be clear: you are suggesting to remove "i2c" from the DSD binding "i2c-allow-low-power-probe". And you are not talking about moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I recall the latter has been NACKed by gkh so far.

I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all the implementation would remain where it is in the code. IOW, I'm just suggesting a naming change to avoid proliferating duplicate flags of the same meaning across subsystems.

Quoted 16 lines · Tomasz Figa

Quoted 8 lines · Wolfram Sang

Quoted 7 lines

I think we might be overly complicating things. IMHO the series as is with the "i2c_" prefix removed from the flags introduced would be reusable as is for any other subsystem that needs it. Of course, for now, the handling of the flag would remain implemented only in the I2C subsystem.

Quoted 5 lines

Just to be clear: you are suggesting to remove "i2c" from the DSD binding "i2c-allow-low-power-probe". And you are not talking about moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I recall the latter has been NACKed by gkh so far.

Quoted 4 lines

I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all the implementation would remain where it is in the code. IOW, I'm just suggesting a naming change to avoid proliferating duplicate flags of the same meaning across subsystems.

But that would indicate that the property was recognized by other subsystems which wouldn't be the case, so it would be confusing.

That's why it cannot be documented as a general property ATM too.

Quoted 23 lines · Rafael J. Wysocki

Quoted 20 lines · Tomasz Figa

Quoted 8 lines · Wolfram Sang

Quoted 7 lines

I think we might be overly complicating things. IMHO the series as is with the "i2c_" prefix removed from the flags introduced would be reusable as is for any other subsystem that needs it. Of course, for now, the handling of the flag would remain implemented only in the I2C subsystem.

Quoted 5 lines

Just to be clear: you are suggesting to remove "i2c" from the DSD binding "i2c-allow-low-power-probe". And you are not talking about moving I2C_DRV_FL_ALLOW_LOW_POWER_PROBE to struct device_driver? I recall the latter has been NACKed by gkh so far.

I'd also drop "I2C_" from "I2C_DRV_FL_ALLOW_LOW_POWER_PROBE", but all the implementation would remain where it is in the code. IOW, I'm just suggesting a naming change to avoid proliferating duplicate flags of the same meaning across subsystems.

Quoted 4 lines

But that would indicate that the property was recognized by other subsystems which wouldn't be the case, so it would be confusing.

That's why it cannot be documented as a general property ATM too.

I guess that's true. Well, this is kAPI in the end, so if we have more subsystems, it could be always renamed. So feel free to ignore my previous comment.

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.

Quoted 24 lines · Sakari Ailus

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

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

---
 drivers/acpi/scan.c     | 6 ++++++
 include/acpi/acpi_bus.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

drivers/acpi/scan.c +2 -0

index 1d7a02ee45e05..b077c645c9845 100644
@@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
 
 static void acpi_bus_get_power_flags(struct acpi_device *device)
 {
+       unsigned long long pre;
+       acpi_status status;
        u32 i;
 
        /* Presence of _PS0|_PR0 indicates 'power manageable' */

drivers/acpi/scan.c static void acpi_bus_get_power_flags(struct acpi_device *device) · line 1015

Quoted 6 lines

drivers/acpi/scan.c +3 -0

@@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
        if (acpi_has_method(device->handle, "_DSW"))
                device->power.flags.dsw_present = 1;
 
+       status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
+       if (ACPI_SUCCESS(status) && !pre)
+               device->power.flags.allow_low_power_probe = 1;

While this is what has been discussed and thanks for taking it into account, I'm now thinking that it may be cleaner to introduce a new object to return the deepest power state of the device in which it can be enumerated, say _DSE (Device State for Enumeration) such that 4 means D3cold, 3 - D3hot and so on, so the above check can be replaced with something like

status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);

if (ACPI_FAILURE(status))

        device->power.state_for_enumeratin = dse;

And then, it is a matter of comparing ->power.state_for_enumeratin with ->power.state and putting the device into D0 if the former is shallower than the latter.

What do you think?

Hi Rafael,

Thanks for the comments.

Quoted 40 lines · Rafael J. Wysocki

Quoted 24 lines · Sakari Ailus

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

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

---
 drivers/acpi/scan.c     | 6 ++++++
 include/acpi/acpi_bus.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

drivers/acpi/scan.c +2 -0

index 1d7a02ee45e05..b077c645c9845 100644
@@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
 
 static void acpi_bus_get_power_flags(struct acpi_device *device)
 {
+       unsigned long long pre;
+       acpi_status status;
        u32 i;
 
        /* Presence of _PS0|_PR0 indicates 'power manageable' */

drivers/acpi/scan.c static void acpi_bus_get_power_flags(struct acpi_device *device) · line 1015

Quoted 6 lines

drivers/acpi/scan.c +3 -0

@@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
        if (acpi_has_method(device->handle, "_DSW"))
                device->power.flags.dsw_present = 1;
 
+       status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
+       if (ACPI_SUCCESS(status) && !pre)
+               device->power.flags.allow_low_power_probe = 1;

While this is what has been discussed and thanks for taking it into account, I'm now thinking that it may be cleaner to introduce a new object to return the deepest power state of the device in which it can be enumerated, say _DSE (Device State for Enumeration) such that 4 means D3cold, 3 - D3hot and so on, so the above check can be replaced with something like

Quoted 1 line

status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);

s/_PRE/_DSE/

?

Quoted 2 lines

if (ACPI_FAILURE(status))

ACPI_SUCCESS?

Quoted 7 lines

        device->power.state_for_enumeratin = dse;

And then, it is a matter of comparing ->power.state_for_enumeratin with ->power.state and putting the device into D0 if the former is shallower than the latter.

What do you think?

Sounds good. How about calling the function e.g. acpi_device_resume_for_probe(), so runtime PM could be used to resume the device if the function returns true?

Quoted 46 lines · Sakari Ailus

Hi Rafael,

Thanks for the comments.

Quoted 40 lines · Rafael J. Wysocki

Quoted 24 lines · Sakari Ailus

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

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

---
 drivers/acpi/scan.c     | 6 ++++++
 include/acpi/acpi_bus.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

drivers/acpi/scan.c +2 -0

index 1d7a02ee45e05..b077c645c9845 100644
@@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
 
 static void acpi_bus_get_power_flags(struct acpi_device *device)
 {
+       unsigned long long pre;
+       acpi_status status;
        u32 i;
 
        /* Presence of _PS0|_PR0 indicates 'power manageable' */

drivers/acpi/scan.c static void acpi_bus_get_power_flags(struct acpi_device *device) · line 1015

Quoted 6 lines

drivers/acpi/scan.c +3 -0

@@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
        if (acpi_has_method(device->handle, "_DSW"))
                device->power.flags.dsw_present = 1;
 
+       status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
+       if (ACPI_SUCCESS(status) && !pre)
+               device->power.flags.allow_low_power_probe = 1;

While this is what has been discussed and thanks for taking it into account, I'm now thinking that it may be cleaner to introduce a new object to return the deepest power state of the device in which it can be enumerated, say _DSE (Device State for Enumeration) such that 4 means D3cold, 3 - D3hot and so on, so the above check can be replaced with something like

Quoted 5 lines

Quoted 1 line

status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);

s/_PRE/_DSE/

?

Yes, sorry.

Quoted 4 lines

Quoted 2 lines

if (ACPI_FAILURE(status))

ACPI_SUCCESS?

Yup.

Quoted 11 lines

Quoted 7 lines

        device->power.state_for_enumeratin = dse;

And then, it is a matter of comparing ->power.state_for_enumeratin with ->power.state and putting the device into D0 if the former is shallower than the latter.

What do you think?

Sounds good. How about calling the function e.g. acpi_device_resume_for_probe(), so runtime PM could be used to resume the device if the function returns true?

I'd rather try to power it up before enabling runtime PM, because in order to do the latter properly, you need to know if the device is active or suspended to start with.

So you need something like (pseudo-code)

if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) {
   acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
   pm_runtime_set_active(dev);
} else {

pm_runtime_set_suspended(dev);

}

and then you can enable PM-runtime.

Quoted 81 lines · Rafael J. Wysocki

Quoted 46 lines · Sakari Ailus

Hi Rafael,

Thanks for the comments.

Quoted 40 lines · Rafael J. Wysocki

Quoted 24 lines · Sakari Ailus

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

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

---
 drivers/acpi/scan.c     | 6 ++++++
 include/acpi/acpi_bus.h | 3 ++-
 2 files changed, 8 insertions(+), 1 deletion(-)

drivers/acpi/scan.c +2 -0

index 1d7a02ee45e05..b077c645c9845 100644
@@ -987,6 +987,8 @@ static void acpi_bus_init_power_state(struct acpi_device *device, int state)
 
 static void acpi_bus_get_power_flags(struct acpi_device *device)
 {
+       unsigned long long pre;
+       acpi_status status;
        u32 i;
 
        /* Presence of _PS0|_PR0 indicates 'power manageable' */

drivers/acpi/scan.c static void acpi_bus_get_power_flags(struct acpi_device *device) · line 1015

Quoted 6 lines

drivers/acpi/scan.c +3 -0

@@ -1008,6 +1010,10 @@ static void acpi_bus_get_power_flags(struct acpi_device *device)
        if (acpi_has_method(device->handle, "_DSW"))
                device->power.flags.dsw_present = 1;
 
+       status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &pre);
+       if (ACPI_SUCCESS(status) && !pre)
+               device->power.flags.allow_low_power_probe = 1;

While this is what has been discussed and thanks for taking it into account, I'm now thinking that it may be cleaner to introduce a new object to return the deepest power state of the device in which it can be enumerated, say _DSE (Device State for Enumeration) such that 4 means D3cold, 3 - D3hot and so on, so the above check can be replaced with something like

Quoted 5 lines

Quoted 1 line

status = acpi_evaluate_integer(device->handle, "_PRE", NULL, &dse);

s/_PRE/_DSE/

?

Yes, sorry.

Quoted 4 lines

Quoted 2 lines

if (ACPI_FAILURE(status))

ACPI_SUCCESS?

Yup.

Quoted 11 lines

Quoted 7 lines

        device->power.state_for_enumeratin = dse;

And then, it is a matter of comparing ->power.state_for_enumeratin with ->power.state and putting the device into D0 if the former is shallower than the latter.

What do you think?

Sounds good. How about calling the function e.g. acpi_device_resume_for_probe(), so runtime PM could be used to resume the device if the function returns true?

I'd rather try to power it up before enabling runtime PM, because in order to do the latter properly, you need to know if the device is active or suspended to start with.

So you need something like (pseudo-code)

Quoted 5 lines

if (this_device_needs_to_be_on(ACPI_COMPANION(dev))) {
   acpi_device_set_power(ACPI_COMPANION(dev), ACPI_STATE_D0);
   pm_runtime_set_active(dev);
} else {

pm_runtime_set_suspended(dev);

I guess the else branch isn't needed? The device remains suspended if its state hasn't been changed.

Quoted 3 lines

}

and then you can enable PM-runtime.

Yes, agreed, this is what drivers should do. The I涎 framework would use the function and conditionally power the device on before enabling runtime PM.

This is how it's implemented by the set already but I think the change in semantics requires a little more still.

From: Rajmohan Mani <[email protected]>

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

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

---
 drivers/media/i2c/imx319.c | 72 +++++++++++++++++++++++---------------
 1 file changed, 44 insertions(+), 28 deletions(-)

drivers/media/i2c/imx319.c +44 -28

index 8473c0bbb35d6..e0b22e9318fed 100644
@@ -140,6 +140,8 @@ struct imx319 {
 
 	/* Streaming on/off */
 	bool streaming;
+	/* True if the device has been identified */
+	bool identified;
 };
 
 static const struct imx319_reg imx319_global_regs[] = {

@@ -2084,6 +2086,31 @@ imx319_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+/* Verify chip ID */
+static int imx319_identify_module(struct imx319 *imx319)
+{
+	struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
+	int ret;
+	u32 val;
+
+	if (imx319->identified)
+		return 0;
+
+	ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
+	if (ret)
+		return ret;
+
+	if (val != IMX319_CHIP_ID) {
+		dev_err(&client->dev, "chip id mismatch: %x!=%x",
+			IMX319_CHIP_ID, val);
+		return -EIO;
+	}
+
+	imx319->identified = true;
+
+	return 0;
+}
+
 /* Start streaming */
 static int imx319_start_streaming(struct imx319 *imx319)
 {

@@ -2091,6 +2118,10 @@ static int imx319_start_streaming(struct imx319 *imx319)
 	const struct imx319_reg_list *reg_list;
 	int ret;
 
+	ret = imx319_identify_module(imx319);
+	if (ret)
+		return ret;
+
 	/* Global Setting */
 	reg_list = &imx319_global_setting;
 	ret = imx319_write_regs(imx319, reg_list->regs, reg_list->num_of_regs);

@@ -2208,26 +2239,6 @@ static int __maybe_unused imx319_resume(struct device *dev)
 	return ret;
 }
 
-/* Verify chip ID */
-static int imx319_identify_module(struct imx319 *imx319)
-{
-	struct i2c_client *client = v4l2_get_subdevdata(&imx319->sd);
-	int ret;
-	u32 val;
-
-	ret = imx319_read_reg(imx319, IMX319_REG_CHIP_ID, 2, &val);
-	if (ret)
-		return ret;
-
-	if (val != IMX319_CHIP_ID) {
-		dev_err(&client->dev, "chip id mismatch: %x!=%x",
-			IMX319_CHIP_ID, val);
-		return -EIO;
-	}
-
-	return 0;
-}
-
 static const struct v4l2_subdev_core_ops imx319_subdev_core_ops = {
 	.subscribe_event = v4l2_ctrl_subdev_subscribe_event,
 	.unsubscribe_event = v4l2_event_subdev_unsubscribe,

@@ -2422,6 +2433,7 @@ static struct imx319_hwcfg *imx319_get_hwcfg(struct device *dev)
 static int imx319_probe(struct i2c_client *client)
 {
 	struct imx319 *imx319;
+	bool low_power;
 	int ret;
 	u32 i;
 

@@ -2434,11 +2446,14 @@ static int imx319_probe(struct i2c_client *client)
 	/* Initialize subdev */
 	v4l2_i2c_subdev_init(&imx319->sd, client, &imx319_subdev_ops);
 
-	/* Check module identity */
-	ret = imx319_identify_module(imx319);
-	if (ret) {
-		dev_err(&client->dev, "failed to find sensor: %d", ret);
-		goto error_probe;
+	low_power = acpi_dev_state_low_power(&client->dev);
+	if (!low_power) {
+		/* Check module identity */
+		ret = imx319_identify_module(imx319);
+		if (ret) {
+			dev_err(&client->dev, "failed to find sensor: %d", ret);
+			goto error_probe;
+		}
 	}
 
 	imx319->hwcfg = imx319_get_hwcfg(&client->dev);

@@ -2491,10 +2506,10 @@ static int imx319_probe(struct i2c_client *client)
 		goto error_media_entity;
 
 	/*
-	 * Device is already turned on by i2c-core with ACPI domain PM.
-	 * Enable runtime PM and turn off the device.
+	 * Don't set the device's state to active if it's in a low power state.
 	 */
-	pm_runtime_set_active(&client->dev);
+	if (!low_power)
+		pm_runtime_set_active(&client->dev);
 	pm_runtime_enable(&client->dev);
 	pm_runtime_idle(&client->dev);
 

@@ -2547,6 +2562,7 @@ static struct i2c_driver imx319_i2c_driver = {
 	},
 	.probe_new = imx319_probe,
 	.remove = imx319_remove,
+	.flags = I2C_DRV_FL_ALLOW_LOW_POWER_PROBE,
 };
 module_i2c_driver(imx319_i2c_driver);
 

Hi Bartosz,

Thanks for the review.

Quoted 30 lines · Bartosz Golaszewski

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

---
 drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 17 deletions(-)

drivers/misc/eeprom/at24.c +1 -0

index 926408b41270c..dd0b3f24e3808 100644
@@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
        bool i2c_fn_i2c, i2c_fn_block;
        unsigned int i, num_addresses;
        struct at24_data *at24;
+       bool low_power;
        struct regmap *regmap;
        bool writable;
        u8 test_byte;

@@ -750,14 +751,16 @@ static int at24_probe(struct i2c_client *client)
 
        i2c_set_clientdata(client, at24);
 

Quoted 11 lines

drivers/misc/eeprom/at24.c static int at24_probe(struct i2c_client *client) · line 754

Quoted 6 lines

drivers/misc/eeprom/at24.c +1 -5

-       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);

I've raised my concern about the naming of this before but no discussion followed. Do we really want to name it: "low power"? This is misleading as the device can actually be powered off at probe(). "Low power" suggests some low-power state or even low battery IMO.

This was suggested by Rafael in place of "powered off" as it's not know the device is powered off. The same terms should be used in all contexts (ACPI and I²C frameworks and drivers). Others haven't expressed concerns.

ACPI spec appears to be using terms "on" and "off".

The use of the function is not limited to driver probe time.

Quoted 1 line

Quoted 2 lines

If anything: I'd prefer the 'low_power' local variable be changed to "no_test_read".

That misses the power management related suggestion now present in the name --- the device needs to be suspended using runtime PM if probe fails and it's not in "low power state".

How about "off_during_probe"?

--
Kind regards,

Sakari Ailus