| Message ID | 20200903081550.6012-7-sakari.ailus@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Support running driver's probe for a device powered off | expand |
On Thu, Sep 3, 2020 at 10:15 AM Sakari Ailus <sakari.ailus@linux.intel.com> 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 <sakari.ailus@linux.intel.com> > --- > drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > index 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 <wsa@the-dreams.de> 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 <wsa@the-dreams.de> 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.
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 <wsa@the-dreams.de> 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.
Hi Sakari, On Thu, Sep 3, 2020 at 10:15 AM Sakari Ailus <sakari.ailus@linux.intel.com> 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 <sakari.ailus@linux.intel.com> > --- > drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++--------------- > 1 file changed, 26 insertions(+), 17 deletions(-) > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > index 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 >
On Tue, Oct 6, 2020 at 1:20 PM Tomasz Figa <tfiga@chromium.org> wrote: > > Hi Sakari, > > On Thu, Sep 3, 2020 at 10:15 AM Sakari Ailus > <sakari.ailus@linux.intel.com> 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 <sakari.ailus@linux.intel.com> > > --- > > drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++--------------- > > 1 file changed, 26 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c > > index 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 > >
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)
In certain use cases (where the chip is part of a camera module, and the camera module is wired together with a camera privacy LED), powering on the device during probe is undesirable. Add support for the at24 to execute probe while being powered off. For this to happen, a hint in form of a device property is required from the firmware. Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com> --- drivers/misc/eeprom/at24.c | 43 +++++++++++++++++++++++--------------- 1 file changed, 26 insertions(+), 17 deletions(-)