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(-)
diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 8f5de5f10bbea..2d24e33788d7d 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
bool i2c_fn_i2c, i2c_fn_block;
unsigned int i, num_addresses;
struct at24_data *at24;
+ bool 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
On Thu, Sep 3, 2020 at 10:15 AM Sakari Ailus
<[email protected]> wrote:
>
> In certain use cases (where the chip is part of a camera module, and the
> camera module is wired together with a camera privacy LED), powering on
> the device during probe is undesirable. Add support for the at24 to
> execute probe while being powered off. For this to happen, a hint in form
> of a device property is required from the firmware.
>
> Signed-off-by: Sakari Ailus <[email protected]>
> ---
> drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 8f5de5f10bbea..2d24e33788d7d 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
> bool i2c_fn_i2c, i2c_fn_block;
> unsigned int i, num_addresses;
> struct at24_data *at24;
> + bool 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?
Bartosz
> 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.
On Wed, Sep 9, 2020 at 1:11 PM Wolfram Sang <[email protected]> wrote:
>
>
> > 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.
>
Sakari,
can you rebase the at24 driver patch on top of Wolfram's tree as soon
as he merges my PR with at24 fixes?
Bartosz
On Wed, Sep 09, 2020 at 01:56:34PM +0200, Bartosz Golaszewski wrote:
> On Wed, Sep 9, 2020 at 1:11 PM Wolfram Sang <[email protected]> wrote:
> >
> >
> > > 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.
> >
>
> Sakari,
>
> can you rebase the at24 driver patch on top of Wolfram's tree as soon
> as he merges my PR with at24 fixes?
Sure! This would be of course better than postponing this patch.
--
Regards,
Sakari Ailus
Bartosz, Wolfram,
On Wed, Sep 09, 2020 at 01:56:34PM +0200, Bartosz Golaszewski wrote:
> On Wed, Sep 9, 2020 at 1:11 PM Wolfram Sang <[email protected]> wrote:
> >
> >
> > > 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.
> >
>
> Sakari,
>
> can you rebase the at24 driver patch on top of Wolfram's tree as soon
> as he merges my PR with at24 fixes?
I need some additional time with the set, and that may take it beyond
v5.10. Either way, I'll keep you posted.
Thanks.
--
Kind regards,
Sakari Ailus
On Tue, Oct 6, 2020 at 1:20 PM Tomasz Figa <[email protected]> wrote:
>
> Hi Sakari,
>
> On Thu, Sep 3, 2020 at 10:15 AM Sakari Ailus
> <[email protected]> wrote:
> >
> > In certain use cases (where the chip is part of a camera module, and the
> > camera module is wired together with a camera privacy LED), powering on
> > the device during probe is undesirable. Add support for the at24 to
> > execute probe while being powered off. For this to happen, a hint in form
> > of a device property is required from the firmware.
> >
> > Signed-off-by: Sakari Ailus <[email protected]>
> > ---
> > drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
> > 1 file changed, 26 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 8f5de5f10bbea..2d24e33788d7d 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
> > bool i2c_fn_i2c, i2c_fn_block;
> > unsigned int i, num_addresses;
> > struct at24_data *at24;
> > + bool 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);
> >
>
> 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
>
> > /*
> > - * 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);
Actually never mind. Someone pointed out to me that at24_read() calls
pm_runtime_get_sync() internally, so we should be fine. Sorry, for the
noise.
Best regards,
Tomasz
> > - 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,
On Thu, Sep 3, 2020 at 10:15 AM Sakari Ailus
<[email protected]> wrote:
>
> In certain use cases (where the chip is part of a camera module, and the
> camera module is wired together with a camera privacy LED), powering on
> the device during probe is undesirable. Add support for the at24 to
> execute probe while being powered off. For this to happen, a hint in form
> of a device property is required from the firmware.
>
> Signed-off-by: Sakari Ailus <[email protected]>
> ---
> drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++---------------
> 1 file changed, 26 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 8f5de5f10bbea..2d24e33788d7d 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -595,6 +595,7 @@ static int at24_probe(struct i2c_client *client)
> bool i2c_fn_i2c, i2c_fn_block;
> unsigned int i, num_addresses;
> struct at24_data *at24;
> + bool 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);
>
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
> /*
> - * 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
>