[v4] misc: eeprom: at24: support pm_runtime control
diff mbox series

Message ID 20191018082557.3696-1-bibby.hsieh@mediatek.com
State Superseded
Headers show
Series
  • [v4] misc: eeprom: at24: support pm_runtime control
Related show

Commit Message

Bibby Hsieh Oct. 18, 2019, 8:25 a.m. UTC
Although in the most platforms, the power of eeprom and i2c
are alway on, some platforms disable the eeprom and i2c power
in order to meet low power request.
This patch add the pm_runtime ops to control power to support
all platforms.

Changes since v3:
 - remove redundant calling function
 - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
 - change supply name

Changes since v2:
 - rebase onto v5.4-rc1
 - pm_runtime_disable and regulator_bulk_disable at
   err return in probe function

Changes since v1:
 - remove redundant code
 - fixup coding style

Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
---
 drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 64 insertions(+)

Comments

Tomasz Figa Oct. 18, 2019, 9:24 a.m. UTC | #1
Hi Bibby,

On Fri, Oct 18, 2019 at 5:26 PM Bibby Hsieh <bibby.hsieh@mediatek.com> wrote:
>
> Although in the most platforms, the power of eeprom and i2c
> are alway on, some platforms disable the eeprom and i2c power
> in order to meet low power request.
> This patch add the pm_runtime ops to control power to support
> all platforms.
>
> Changes since v3:
>  - remove redundant calling function
>  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
>  - change supply name
>
> Changes since v2:
>  - rebase onto v5.4-rc1
>  - pm_runtime_disable and regulator_bulk_disable at
>    err return in probe function
>
> Changes since v1:
>  - remove redundant code
>  - fixup coding style
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2cccd82a3106..68ced4f25916 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -22,6 +22,7 @@
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/gpio/consumer.h>
>
>  /* Address pointer is 16 bit. */
> @@ -67,6 +68,12 @@
>   * which won't work on pure SMBus systems.
>   */
>
> +static const char * const at24_supply_names[] = {
> +       "vcc", "i2c",
> +};
> +
> +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> +
>  struct at24_client {
>         struct i2c_client *client;
>         struct regmap *regmap;
> @@ -91,6 +98,8 @@ struct at24_data {
>
>         struct gpio_desc *wp_gpio;
>
> +       bool has_supplies;
> +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
>         /*
>          * Some chips tie up multiple I2C addresses; dummy devices reserve
>          * them for us, and we'll use them with SMBus calls.
> @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
>         at24->client[0].client = client;
>         at24->client[0].regmap = regmap;
>
> +       regulator_bulk_set_supply_names(at24->supplies,
> +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> +                                      AT24_NUM_SUPPLIES, at24->supplies);
> +       if (err == -ENODEV)
> +               at24->has_supplies = NULL;

has_supplies is a bool, so the right value would be false.

> +       else if (err == 0)

nit: One would typically use !err here as the condition.

> +               at24->has_supplies = !err;

In this branch, err is always 0, so !err is always true and we can
just directly assign true to the field.

> +       else
> +               return err;
> +
>         at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
>         if (IS_ERR(at24->wp_gpio))
>                 return PTR_ERR(at24->wp_gpio);
> @@ -701,6 +721,14 @@ static int at24_probe(struct i2c_client *client)
>
>         i2c_set_clientdata(client, at24);
>
> +       if (at24->has_supplies) {
> +               err = regulator_bulk_enable(AT24_NUM_SUPPLIES, at24->supplies);
> +               if (err) {
> +                       dev_err(dev, "Failed to enable power regulators\n");
> +                       return err;
> +               }
> +       }
> +
>         /* enable runtime pm */
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
> @@ -713,6 +741,9 @@ static int at24_probe(struct i2c_client *client)
>         pm_runtime_idle(dev);
>         if (err) {
>                 pm_runtime_disable(dev);
> +               if (at24->has_supplies)
> +                       regulator_bulk_disable(AT24_NUM_SUPPLIES,
> +                                              at24->supplies);
>                 return -ENODEV;
>         }
>
> @@ -725,15 +756,48 @@ static int at24_probe(struct i2c_client *client)
>
>  static int at24_remove(struct i2c_client *client)
>  {
> +       struct at24_data *at24 = i2c_get_clientdata(client);
> +
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
> +       if (at24->has_supplies)
> +               regulator_bulk_disable(AT24_NUM_SUPPLIES, at24->supplies);

It's a weird behavior, but pm_runtime_disable() doesn't guarantee that
the device is actually resumed after the call returns. See [1].
We should move the regulator disable before we call
pm_runtime_set_suspended() and add !pm_runtime_status_suspended() as
an additional condition to the if.

By the way, that behavior is actually contradicting other parts of the
runtime PM core. For example pm_runtime_active() returns true if
dev->power.disable_depth is non-zero, but as per the above, the device
could as well be suspended. Rafael, is this expected?

[1] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/base/power/runtime.c#L1316

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused at24_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct at24_data *at24 = i2c_get_clientdata(client);
> +
> +       if (at24->has_supplies)
> +               return regulator_bulk_disable(AT24_NUM_SUPPLIES,
> +                                             at24->supplies);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused at24_resume(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct at24_data *at24 = i2c_get_clientdata(client);
> +
> +       if (at24->has_supplies)
> +               return regulator_bulk_enable(AT24_NUM_SUPPLIES,
> +                                            at24->supplies);
>
>         return 0;
>  }
>
> +static const struct dev_pm_ops at24_pm_ops = {
> +       SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)

Do we also need pm_runtime_force_suspend() and
pm_runtime_force_resume() as system sleep PM ops or it isn't possible
for the device to be runtime active when entering the system suspend?

Best regards,
Tomasz
Bartosz Golaszewski Oct. 18, 2019, 10:11 a.m. UTC | #2
pt., 18 paź 2019 o 11:25 Tomasz Figa <tfiga@chromium.org> napisał(a):
>
> Hi Bibby,
>
> On Fri, Oct 18, 2019 at 5:26 PM Bibby Hsieh <bibby.hsieh@mediatek.com> wrote:
> >
> > Although in the most platforms, the power of eeprom and i2c
> > are alway on, some platforms disable the eeprom and i2c power
> > in order to meet low power request.
> > This patch add the pm_runtime ops to control power to support
> > all platforms.
> >
> > Changes since v3:
> >  - remove redundant calling function
> >  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
> >  - change supply name
> >
> > Changes since v2:
> >  - rebase onto v5.4-rc1
> >  - pm_runtime_disable and regulator_bulk_disable at
> >    err return in probe function
> >
> > Changes since v1:
> >  - remove redundant code
> >  - fixup coding style
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 2cccd82a3106..68ced4f25916 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/nvmem-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/gpio/consumer.h>
> >
> >  /* Address pointer is 16 bit. */
> > @@ -67,6 +68,12 @@
> >   * which won't work on pure SMBus systems.
> >   */
> >
> > +static const char * const at24_supply_names[] = {
> > +       "vcc", "i2c",
> > +};
> > +
> > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > +
> >  struct at24_client {
> >         struct i2c_client *client;
> >         struct regmap *regmap;
> > @@ -91,6 +98,8 @@ struct at24_data {
> >
> >         struct gpio_desc *wp_gpio;
> >
> > +       bool has_supplies;
> > +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
> >         /*
> >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> >          * them for us, and we'll use them with SMBus calls.
> > @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
> >         at24->client[0].client = client;
> >         at24->client[0].regmap = regmap;
> >
> > +       regulator_bulk_set_supply_names(at24->supplies,
> > +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > +       if (err == -ENODEV)
> > +               at24->has_supplies = NULL;
>
> has_supplies is a bool, so the right value would be false.
>

Well I admit I did say 'assign NULL to has_supplies' in my previous
review, but I really meant 'false' and it's pretty clear it was by
mistake, or so I thought. :)

Bart

> > +       else if (err == 0)
>
> nit: One would typically use !err here as the condition.
>
> > +               at24->has_supplies = !err;
>
> In this branch, err is always 0, so !err is always true and we can
> just directly assign true to the field.
>
> > +       else
> > +               return err;
> > +
> >         at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
> >         if (IS_ERR(at24->wp_gpio))
> >                 return PTR_ERR(at24->wp_gpio);
> > @@ -701,6 +721,14 @@ static int at24_probe(struct i2c_client *client)
> >
> >         i2c_set_clientdata(client, at24);
> >
> > +       if (at24->has_supplies) {
> > +               err = regulator_bulk_enable(AT24_NUM_SUPPLIES, at24->supplies);
> > +               if (err) {
> > +                       dev_err(dev, "Failed to enable power regulators\n");
> > +                       return err;
> > +               }
> > +       }
> > +
> >         /* enable runtime pm */
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> > @@ -713,6 +741,9 @@ static int at24_probe(struct i2c_client *client)
> >         pm_runtime_idle(dev);
> >         if (err) {
> >                 pm_runtime_disable(dev);
> > +               if (at24->has_supplies)
> > +                       regulator_bulk_disable(AT24_NUM_SUPPLIES,
> > +                                              at24->supplies);
> >                 return -ENODEV;
> >         }
> >
> > @@ -725,15 +756,48 @@ static int at24_probe(struct i2c_client *client)
> >
> >  static int at24_remove(struct i2c_client *client)
> >  {
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> >         pm_runtime_disable(&client->dev);
> >         pm_runtime_set_suspended(&client->dev);
> > +       if (at24->has_supplies)
> > +               regulator_bulk_disable(AT24_NUM_SUPPLIES, at24->supplies);
>
> It's a weird behavior, but pm_runtime_disable() doesn't guarantee that
> the device is actually resumed after the call returns. See [1].
> We should move the regulator disable before we call
> pm_runtime_set_suspended() and add !pm_runtime_status_suspended() as
> an additional condition to the if.
>
> By the way, that behavior is actually contradicting other parts of the
> runtime PM core. For example pm_runtime_active() returns true if
> dev->power.disable_depth is non-zero, but as per the above, the device
> could as well be suspended. Rafael, is this expected?
>
> [1] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/base/power/runtime.c#L1316
>
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused at24_suspend(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> > +       if (at24->has_supplies)
> > +               return regulator_bulk_disable(AT24_NUM_SUPPLIES,
> > +                                             at24->supplies);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused at24_resume(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> > +       if (at24->has_supplies)
> > +               return regulator_bulk_enable(AT24_NUM_SUPPLIES,
> > +                                            at24->supplies);
> >
> >         return 0;
> >  }
> >
> > +static const struct dev_pm_ops at24_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
>
> Do we also need pm_runtime_force_suspend() and
> pm_runtime_force_resume() as system sleep PM ops or it isn't possible
> for the device to be runtime active when entering the system suspend?
>
> Best regards,
> Tomasz
Bartosz Golaszewski Oct. 21, 2019, 4:53 p.m. UTC | #3
pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> Although in the most platforms, the power of eeprom and i2c
> are alway on, some platforms disable the eeprom and i2c power
> in order to meet low power request.
> This patch add the pm_runtime ops to control power to support
> all platforms.
>
> Changes since v3:
>  - remove redundant calling function
>  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
>  - change supply name
>
> Changes since v2:
>  - rebase onto v5.4-rc1
>  - pm_runtime_disable and regulator_bulk_disable at
>    err return in probe function
>
> Changes since v1:
>  - remove redundant code
>  - fixup coding style
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 64 insertions(+)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2cccd82a3106..68ced4f25916 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -22,6 +22,7 @@
>  #include <linux/nvmem-provider.h>
>  #include <linux/regmap.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  #include <linux/gpio/consumer.h>
>
>  /* Address pointer is 16 bit. */
> @@ -67,6 +68,12 @@
>   * which won't work on pure SMBus systems.
>   */
>
> +static const char * const at24_supply_names[] = {
> +       "vcc", "i2c",
> +};
> +
> +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> +
>  struct at24_client {
>         struct i2c_client *client;
>         struct regmap *regmap;
> @@ -91,6 +98,8 @@ struct at24_data {
>
>         struct gpio_desc *wp_gpio;
>
> +       bool has_supplies;
> +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
>         /*
>          * Some chips tie up multiple I2C addresses; dummy devices reserve
>          * them for us, and we'll use them with SMBus calls.
> @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
>         at24->client[0].client = client;
>         at24->client[0].regmap = regmap;
>
> +       regulator_bulk_set_supply_names(at24->supplies,
> +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> +                                      AT24_NUM_SUPPLIES, at24->supplies);
> +       if (err == -ENODEV)
> +               at24->has_supplies = NULL;

I just gave this a spin and noticed that this will never happen - the
regulator core will use a dummy regulator if none is defined in DT.
The only way for this to make sense would be to use
regulator_get_optional() for each supply separately. But actually I
think we should just leave it this way and remove this if. In the end:
this chip needs some power supply, so dummy regulator makes sense.

Bart

> +       else if (err == 0)
> +               at24->has_supplies = !err;
> +       else
> +               return err;
> +
>         at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
>         if (IS_ERR(at24->wp_gpio))
>                 return PTR_ERR(at24->wp_gpio);
> @@ -701,6 +721,14 @@ static int at24_probe(struct i2c_client *client)
>
>         i2c_set_clientdata(client, at24);
>
> +       if (at24->has_supplies) {
> +               err = regulator_bulk_enable(AT24_NUM_SUPPLIES, at24->supplies);
> +               if (err) {
> +                       dev_err(dev, "Failed to enable power regulators\n");
> +                       return err;
> +               }
> +       }
> +
>         /* enable runtime pm */
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
> @@ -713,6 +741,9 @@ static int at24_probe(struct i2c_client *client)
>         pm_runtime_idle(dev);
>         if (err) {
>                 pm_runtime_disable(dev);
> +               if (at24->has_supplies)
> +                       regulator_bulk_disable(AT24_NUM_SUPPLIES,
> +                                              at24->supplies);
>                 return -ENODEV;
>         }
>
> @@ -725,15 +756,48 @@ static int at24_probe(struct i2c_client *client)
>
>  static int at24_remove(struct i2c_client *client)
>  {
> +       struct at24_data *at24 = i2c_get_clientdata(client);
> +
>         pm_runtime_disable(&client->dev);
>         pm_runtime_set_suspended(&client->dev);
> +       if (at24->has_supplies)
> +               regulator_bulk_disable(AT24_NUM_SUPPLIES, at24->supplies);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused at24_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct at24_data *at24 = i2c_get_clientdata(client);
> +
> +       if (at24->has_supplies)
> +               return regulator_bulk_disable(AT24_NUM_SUPPLIES,
> +                                             at24->supplies);
> +
> +       return 0;
> +}
> +
> +static int __maybe_unused at24_resume(struct device *dev)
> +{
> +       struct i2c_client *client = to_i2c_client(dev);
> +       struct at24_data *at24 = i2c_get_clientdata(client);
> +
> +       if (at24->has_supplies)
> +               return regulator_bulk_enable(AT24_NUM_SUPPLIES,
> +                                            at24->supplies);
>
>         return 0;
>  }
>
> +static const struct dev_pm_ops at24_pm_ops = {
> +       SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
> +};
> +
>  static struct i2c_driver at24_driver = {
>         .driver = {
>                 .name = "at24",
> +               .pm = &at24_pm_ops,
>                 .of_match_table = at24_of_match,
>                 .acpi_match_table = ACPI_PTR(at24_acpi_ids),
>         },
> --
> 2.18.0
>
Bibby Hsieh Oct. 22, 2019, 2:23 a.m. UTC | #4
On Mon, 2019-10-21 at 18:53 +0200, Bartosz Golaszewski wrote:
> pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> >
> > Although in the most platforms, the power of eeprom and i2c
> > are alway on, some platforms disable the eeprom and i2c power
> > in order to meet low power request.
> > This patch add the pm_runtime ops to control power to support
> > all platforms.
> >
> > Changes since v3:
> >  - remove redundant calling function
> >  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
> >  - change supply name
> >
> > Changes since v2:
> >  - rebase onto v5.4-rc1
> >  - pm_runtime_disable and regulator_bulk_disable at
> >    err return in probe function
> >
> > Changes since v1:
> >  - remove redundant code
> >  - fixup coding style
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 2cccd82a3106..68ced4f25916 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/nvmem-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/gpio/consumer.h>
> >
> >  /* Address pointer is 16 bit. */
> > @@ -67,6 +68,12 @@
> >   * which won't work on pure SMBus systems.
> >   */
> >
> > +static const char * const at24_supply_names[] = {
> > +       "vcc", "i2c",
> > +};
> > +
> > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > +
> >  struct at24_client {
> >         struct i2c_client *client;
> >         struct regmap *regmap;
> > @@ -91,6 +98,8 @@ struct at24_data {
> >
> >         struct gpio_desc *wp_gpio;
> >
> > +       bool has_supplies;
> > +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
> >         /*
> >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> >          * them for us, and we'll use them with SMBus calls.
> > @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
> >         at24->client[0].client = client;
> >         at24->client[0].regmap = regmap;
> >
> > +       regulator_bulk_set_supply_names(at24->supplies,
> > +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > +       if (err == -ENODEV)
> > +               at24->has_supplies = NULL;
> 
> I just gave this a spin and noticed that this will never happen - the
> regulator core will use a dummy regulator if none is defined in DT.
> The only way for this to make sense would be to use
> regulator_get_optional() for each supply separately. But actually I
> think we should just leave it this way and remove this if. In the end:
> this chip needs some power supply, so dummy regulator makes sense.
> 
> Bart
OK, I will remove this if in next version.

Thanks for the experiment.:D

Bibby

> 
> > +       else if (err == 0)
> > +               at24->has_supplies = !err;
> > +       else
> > +               return err;
> > +
> >         at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
> >         if (IS_ERR(at24->wp_gpio))
> >                 return PTR_ERR(at24->wp_gpio);
> > @@ -701,6 +721,14 @@ static int at24_probe(struct i2c_client *client)
> >
> >         i2c_set_clientdata(client, at24);
> >
> > +       if (at24->has_supplies) {
> > +               err = regulator_bulk_enable(AT24_NUM_SUPPLIES, at24->supplies);
> > +               if (err) {
> > +                       dev_err(dev, "Failed to enable power regulators\n");
> > +                       return err;
> > +               }
> > +       }
> > +
> >         /* enable runtime pm */
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> > @@ -713,6 +741,9 @@ static int at24_probe(struct i2c_client *client)
> >         pm_runtime_idle(dev);
> >         if (err) {
> >                 pm_runtime_disable(dev);
> > +               if (at24->has_supplies)
> > +                       regulator_bulk_disable(AT24_NUM_SUPPLIES,
> > +                                              at24->supplies);
> >                 return -ENODEV;
> >         }
> >
> > @@ -725,15 +756,48 @@ static int at24_probe(struct i2c_client *client)
> >
> >  static int at24_remove(struct i2c_client *client)
> >  {
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> >         pm_runtime_disable(&client->dev);
> >         pm_runtime_set_suspended(&client->dev);
> > +       if (at24->has_supplies)
> > +               regulator_bulk_disable(AT24_NUM_SUPPLIES, at24->supplies);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused at24_suspend(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> > +       if (at24->has_supplies)
> > +               return regulator_bulk_disable(AT24_NUM_SUPPLIES,
> > +                                             at24->supplies);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused at24_resume(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> > +       if (at24->has_supplies)
> > +               return regulator_bulk_enable(AT24_NUM_SUPPLIES,
> > +                                            at24->supplies);
> >
> >         return 0;
> >  }
> >
> > +static const struct dev_pm_ops at24_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
> > +};
> > +
> >  static struct i2c_driver at24_driver = {
> >         .driver = {
> >                 .name = "at24",
> > +               .pm = &at24_pm_ops,
> >                 .of_match_table = at24_of_match,
> >                 .acpi_match_table = ACPI_PTR(at24_acpi_ids),
> >         },
> > --
> > 2.18.0
> >
Bibby Hsieh Oct. 22, 2019, 2:25 a.m. UTC | #5
On Fri, 2019-10-18 at 18:24 +0900, Tomasz Figa wrote:
> Hi Bibby,
> 
> On Fri, Oct 18, 2019 at 5:26 PM Bibby Hsieh <bibby.hsieh@mediatek.com> wrote:
> >
> > Although in the most platforms, the power of eeprom and i2c
> > are alway on, some platforms disable the eeprom and i2c power
> > in order to meet low power request.
> > This patch add the pm_runtime ops to control power to support
> > all platforms.
> >
> > Changes since v3:
> >  - remove redundant calling function
> >  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
> >  - change supply name
> >
> > Changes since v2:
> >  - rebase onto v5.4-rc1
> >  - pm_runtime_disable and regulator_bulk_disable at
> >    err return in probe function
> >
> > Changes since v1:
> >  - remove redundant code
> >  - fixup coding style
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 2cccd82a3106..68ced4f25916 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/nvmem-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/gpio/consumer.h>
> >
> >  /* Address pointer is 16 bit. */
> > @@ -67,6 +68,12 @@
> >   * which won't work on pure SMBus systems.
> >   */
> >
> > +static const char * const at24_supply_names[] = {
> > +       "vcc", "i2c",
> > +};
> > +
> > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > +
> >  struct at24_client {
> >         struct i2c_client *client;
> >         struct regmap *regmap;
> > @@ -91,6 +98,8 @@ struct at24_data {
> >
> >         struct gpio_desc *wp_gpio;
> >
> > +       bool has_supplies;
> > +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
> >         /*
> >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> >          * them for us, and we'll use them with SMBus calls.
> > @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
> >         at24->client[0].client = client;
> >         at24->client[0].regmap = regmap;
> >
> > +       regulator_bulk_set_supply_names(at24->supplies,
> > +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > +       if (err == -ENODEV)
> > +               at24->has_supplies = NULL;
> 
> has_supplies is a bool, so the right value would be false.
> 
> > +       else if (err == 0)
> 
> nit: One would typically use !err here as the condition.
> 
> > +               at24->has_supplies = !err;
> 
> In this branch, err is always 0, so !err is always true and we can
> just directly assign true to the field.

Got it.
> 
> > +       else
> > +               return err;
> > +
> >         at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
> >         if (IS_ERR(at24->wp_gpio))
> >                 return PTR_ERR(at24->wp_gpio);
> > @@ -701,6 +721,14 @@ static int at24_probe(struct i2c_client *client)
> >
> >         i2c_set_clientdata(client, at24);
> >
> > +       if (at24->has_supplies) {
> > +               err = regulator_bulk_enable(AT24_NUM_SUPPLIES, at24->supplies);
> > +               if (err) {
> > +                       dev_err(dev, "Failed to enable power regulators\n");
> > +                       return err;
> > +               }
> > +       }
> > +
> >         /* enable runtime pm */
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> > @@ -713,6 +741,9 @@ static int at24_probe(struct i2c_client *client)
> >         pm_runtime_idle(dev);
> >         if (err) {
> >                 pm_runtime_disable(dev);
> > +               if (at24->has_supplies)
> > +                       regulator_bulk_disable(AT24_NUM_SUPPLIES,
> > +                                              at24->supplies);
> >                 return -ENODEV;
> >         }
> >
> > @@ -725,15 +756,48 @@ static int at24_probe(struct i2c_client *client)
> >
> >  static int at24_remove(struct i2c_client *client)
> >  {
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> >         pm_runtime_disable(&client->dev);
> >         pm_runtime_set_suspended(&client->dev);
> > +       if (at24->has_supplies)
> > +               regulator_bulk_disable(AT24_NUM_SUPPLIES, at24->supplies);
> 
> It's a weird behavior, but pm_runtime_disable() doesn't guarantee that
> the device is actually resumed after the call returns. See [1].
> We should move the regulator disable before we call
> pm_runtime_set_suspended() and add !pm_runtime_status_suspended() as
> an additional condition to the if.
> 
OK, I will modify it in the next version.

> By the way, that behavior is actually contradicting other parts of the
> runtime PM core. For example pm_runtime_active() returns true if
> dev->power.disable_depth is non-zero, but as per the above, the device
> could as well be suspended. Rafael, is this expected?
> 
> [1] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/base/power/runtime.c#L1316
> 
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused at24_suspend(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> > +       if (at24->has_supplies)
> > +               return regulator_bulk_disable(AT24_NUM_SUPPLIES,
> > +                                             at24->supplies);
> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused at24_resume(struct device *dev)
> > +{
> > +       struct i2c_client *client = to_i2c_client(dev);
> > +       struct at24_data *at24 = i2c_get_clientdata(client);
> > +
> > +       if (at24->has_supplies)
> > +               return regulator_bulk_enable(AT24_NUM_SUPPLIES,
> > +                                            at24->supplies);
> >
> >         return 0;
> >  }
> >
> > +static const struct dev_pm_ops at24_pm_ops = {
> > +       SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
> 
> Do we also need pm_runtime_force_suspend() and
> pm_runtime_force_resume() as system sleep PM ops or it isn't possible
> for the device to be runtime active when entering the system suspend?
Yes, you're right, I will add those two function as system sleep PM ops.

> 
> Best regards,
> Tomasz
Tomasz Figa Oct. 22, 2019, 7:26 a.m. UTC | #6
On Tue, Oct 22, 2019 at 1:53 AM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> >
> > Although in the most platforms, the power of eeprom and i2c
> > are alway on, some platforms disable the eeprom and i2c power
> > in order to meet low power request.
> > This patch add the pm_runtime ops to control power to support
> > all platforms.
> >
> > Changes since v3:
> >  - remove redundant calling function
> >  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
> >  - change supply name
> >
> > Changes since v2:
> >  - rebase onto v5.4-rc1
> >  - pm_runtime_disable and regulator_bulk_disable at
> >    err return in probe function
> >
> > Changes since v1:
> >  - remove redundant code
> >  - fixup coding style
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 64 insertions(+)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 2cccd82a3106..68ced4f25916 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -22,6 +22,7 @@
> >  #include <linux/nvmem-provider.h>
> >  #include <linux/regmap.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >  #include <linux/gpio/consumer.h>
> >
> >  /* Address pointer is 16 bit. */
> > @@ -67,6 +68,12 @@
> >   * which won't work on pure SMBus systems.
> >   */
> >
> > +static const char * const at24_supply_names[] = {
> > +       "vcc", "i2c",
> > +};
> > +
> > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > +
> >  struct at24_client {
> >         struct i2c_client *client;
> >         struct regmap *regmap;
> > @@ -91,6 +98,8 @@ struct at24_data {
> >
> >         struct gpio_desc *wp_gpio;
> >
> > +       bool has_supplies;
> > +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
> >         /*
> >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> >          * them for us, and we'll use them with SMBus calls.
> > @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
> >         at24->client[0].client = client;
> >         at24->client[0].regmap = regmap;
> >
> > +       regulator_bulk_set_supply_names(at24->supplies,
> > +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > +       if (err == -ENODEV)
> > +               at24->has_supplies = NULL;
>
> I just gave this a spin and noticed that this will never happen - the
> regulator core will use a dummy regulator if none is defined in DT.
> The only way for this to make sense would be to use
> regulator_get_optional() for each supply separately. But actually I
> think we should just leave it this way and remove this if. In the end:
> this chip needs some power supply, so dummy regulator makes sense.

Thanks for testing. I'd still like to make sure what happens on non-DT
platforms.

I can see that the core returns the dummy regulator if
have_full_constraints() [1]. That is always true for DT systems, but
for others it's false by default, unless someone explicitly calls
regulator_has_full_constraints() [2].

[1] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/regulator/core.c#L1787
[2] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/regulator/core.c#L120

Best regards,
Tomasz
Bartosz Golaszewski Oct. 22, 2019, 9 a.m. UTC | #7
wt., 22 paź 2019 o 09:27 Tomasz Figa <tfiga@chromium.org> napisał(a):
>
> On Tue, Oct 22, 2019 at 1:53 AM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > >
> > > Although in the most platforms, the power of eeprom and i2c
> > > are alway on, some platforms disable the eeprom and i2c power
> > > in order to meet low power request.
> > > This patch add the pm_runtime ops to control power to support
> > > all platforms.
> > >
> > > Changes since v3:
> > >  - remove redundant calling function
> > >  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
> > >  - change supply name
> > >
> > > Changes since v2:
> > >  - rebase onto v5.4-rc1
> > >  - pm_runtime_disable and regulator_bulk_disable at
> > >    err return in probe function
> > >
> > > Changes since v1:
> > >  - remove redundant code
> > >  - fixup coding style
> > >
> > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > ---
> > >  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 64 insertions(+)
> > >
> > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > > index 2cccd82a3106..68ced4f25916 100644
> > > --- a/drivers/misc/eeprom/at24.c
> > > +++ b/drivers/misc/eeprom/at24.c
> > > @@ -22,6 +22,7 @@
> > >  #include <linux/nvmem-provider.h>
> > >  #include <linux/regmap.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > >  #include <linux/gpio/consumer.h>
> > >
> > >  /* Address pointer is 16 bit. */
> > > @@ -67,6 +68,12 @@
> > >   * which won't work on pure SMBus systems.
> > >   */
> > >
> > > +static const char * const at24_supply_names[] = {
> > > +       "vcc", "i2c",
> > > +};
> > > +
> > > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > > +
> > >  struct at24_client {
> > >         struct i2c_client *client;
> > >         struct regmap *regmap;
> > > @@ -91,6 +98,8 @@ struct at24_data {
> > >
> > >         struct gpio_desc *wp_gpio;
> > >
> > > +       bool has_supplies;
> > > +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
> > >         /*
> > >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> > >          * them for us, and we'll use them with SMBus calls.
> > > @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
> > >         at24->client[0].client = client;
> > >         at24->client[0].regmap = regmap;
> > >
> > > +       regulator_bulk_set_supply_names(at24->supplies,
> > > +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> > > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > > +       if (err == -ENODEV)
> > > +               at24->has_supplies = NULL;
> >
> > I just gave this a spin and noticed that this will never happen - the
> > regulator core will use a dummy regulator if none is defined in DT.
> > The only way for this to make sense would be to use
> > regulator_get_optional() for each supply separately. But actually I
> > think we should just leave it this way and remove this if. In the end:
> > this chip needs some power supply, so dummy regulator makes sense.
>
> Thanks for testing. I'd still like to make sure what happens on non-DT
> platforms.
>
> I can see that the core returns the dummy regulator if
> have_full_constraints() [1]. That is always true for DT systems, but
> for others it's false by default, unless someone explicitly calls
> regulator_has_full_constraints() [2].
>

Not tested yet, but from the code it looks like it will then keep
returning EPROBE_DEFER which doesn't sound right really, especially
since we're printing an error message too. Shouldn't it be -ENODEV?

Bart

> [1] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/regulator/core.c#L1787
> [2] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/regulator/core.c#L120
>
> Best regards,
> Tomasz
Tomasz Figa Oct. 22, 2019, 10:33 a.m. UTC | #8
On Tue, Oct 22, 2019 at 6:00 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> wt., 22 paź 2019 o 09:27 Tomasz Figa <tfiga@chromium.org> napisał(a):
> >
> > On Tue, Oct 22, 2019 at 1:53 AM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > >
> > > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > >
> > > > Although in the most platforms, the power of eeprom and i2c
> > > > are alway on, some platforms disable the eeprom and i2c power
> > > > in order to meet low power request.
> > > > This patch add the pm_runtime ops to control power to support
> > > > all platforms.
> > > >
> > > > Changes since v3:
> > > >  - remove redundant calling function
> > > >  - change SIMPLE_DEV_PM_OPS to SET_RUNTIME_PM_OPS
> > > >  - change supply name
> > > >
> > > > Changes since v2:
> > > >  - rebase onto v5.4-rc1
> > > >  - pm_runtime_disable and regulator_bulk_disable at
> > > >    err return in probe function
> > > >
> > > > Changes since v1:
> > > >  - remove redundant code
> > > >  - fixup coding style
> > > >
> > > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > > ---
> > > >  drivers/misc/eeprom/at24.c | 64 ++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 64 insertions(+)
> > > >
> > > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > > > index 2cccd82a3106..68ced4f25916 100644
> > > > --- a/drivers/misc/eeprom/at24.c
> > > > +++ b/drivers/misc/eeprom/at24.c
> > > > @@ -22,6 +22,7 @@
> > > >  #include <linux/nvmem-provider.h>
> > > >  #include <linux/regmap.h>
> > > >  #include <linux/pm_runtime.h>
> > > > +#include <linux/regulator/consumer.h>
> > > >  #include <linux/gpio/consumer.h>
> > > >
> > > >  /* Address pointer is 16 bit. */
> > > > @@ -67,6 +68,12 @@
> > > >   * which won't work on pure SMBus systems.
> > > >   */
> > > >
> > > > +static const char * const at24_supply_names[] = {
> > > > +       "vcc", "i2c",
> > > > +};
> > > > +
> > > > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > > > +
> > > >  struct at24_client {
> > > >         struct i2c_client *client;
> > > >         struct regmap *regmap;
> > > > @@ -91,6 +98,8 @@ struct at24_data {
> > > >
> > > >         struct gpio_desc *wp_gpio;
> > > >
> > > > +       bool has_supplies;
> > > > +       struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
> > > >         /*
> > > >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> > > >          * them for us, and we'll use them with SMBus calls.
> > > > @@ -662,6 +671,17 @@ static int at24_probe(struct i2c_client *client)
> > > >         at24->client[0].client = client;
> > > >         at24->client[0].regmap = regmap;
> > > >
> > > > +       regulator_bulk_set_supply_names(at24->supplies,
> > > > +                                       at24_supply_names, AT24_NUM_SUPPLIES);
> > > > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > > > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > > > +       if (err == -ENODEV)
> > > > +               at24->has_supplies = NULL;
> > >
> > > I just gave this a spin and noticed that this will never happen - the
> > > regulator core will use a dummy regulator if none is defined in DT.
> > > The only way for this to make sense would be to use
> > > regulator_get_optional() for each supply separately. But actually I
> > > think we should just leave it this way and remove this if. In the end:
> > > this chip needs some power supply, so dummy regulator makes sense.
> >
> > Thanks for testing. I'd still like to make sure what happens on non-DT
> > platforms.
> >
> > I can see that the core returns the dummy regulator if
> > have_full_constraints() [1]. That is always true for DT systems, but
> > for others it's false by default, unless someone explicitly calls
> > regulator_has_full_constraints() [2].
> >
>
> Not tested yet, but from the code it looks like it will then keep
> returning EPROBE_DEFER which doesn't sound right really, especially
> since we're printing an error message too. Shouldn't it be -ENODEV?

That's an interesting finding. Liam, Mark, what's the proper way to
bulk get optional regulators?

Best regards,
Tomasz

>
> Bart
>
> > [1] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/regulator/core.c#L1787
> > [2] https://elixir.bootlin.com/linux/v5.4-rc2/source/drivers/regulator/core.c#L120
> >
> > Best regards,
> > Tomasz
Mark Brown Oct. 22, 2019, 11:19 a.m. UTC | #9
On Tue, Oct 22, 2019 at 07:33:54PM +0900, Tomasz Figa wrote:
> On Tue, Oct 22, 2019 at 6:00 PM Bartosz Golaszewski

> > > I can see that the core returns the dummy regulator if
> > > have_full_constraints() [1]. That is always true for DT systems, but
> > > for others it's false by default, unless someone explicitly calls
> > > regulator_has_full_constraints() [2].

ACPI systems are also always marked as having full constraints, only
systems with board files will see this.

> > Not tested yet, but from the code it looks like it will then keep
> > returning EPROBE_DEFER which doesn't sound right really, especially
> > since we're printing an error message too. Shouldn't it be -ENODEV?

> That's an interesting finding. Liam, Mark, what's the proper way to
> bulk get optional regulators?

The ambiguously named regulator_get_optional().  This should *only* be
used for regulators that may be physically absent in the system, other
regulators should use normal regulator_get().  It is vanishingly
unlikely that all the supplies for a device will be optional.
Bartosz Golaszewski Oct. 22, 2019, 12:13 p.m. UTC | #10
wt., 22 paź 2019 o 13:19 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Oct 22, 2019 at 07:33:54PM +0900, Tomasz Figa wrote:
> > On Tue, Oct 22, 2019 at 6:00 PM Bartosz Golaszewski
>
> > > > I can see that the core returns the dummy regulator if
> > > > have_full_constraints() [1]. That is always true for DT systems, but
> > > > for others it's false by default, unless someone explicitly calls
> > > > regulator_has_full_constraints() [2].
>
> ACPI systems are also always marked as having full constraints, only
> systems with board files will see this.
>
> > > Not tested yet, but from the code it looks like it will then keep
> > > returning EPROBE_DEFER which doesn't sound right really, especially
> > > since we're printing an error message too. Shouldn't it be -ENODEV?
>
> > That's an interesting finding. Liam, Mark, what's the proper way to
> > bulk get optional regulators?
>
> The ambiguously named regulator_get_optional().  This should *only* be
> used for regulators that may be physically absent in the system, other
> regulators should use normal regulator_get().  It is vanishingly
> unlikely that all the supplies for a device will be optional.

I take it that this driver should also use regular regulator_bulk_get() then?

I think the question about the return value of
regulator_resolve_supply() still stands though: is it normal that it
returns EPROBE_DEFER if it can't resolve the supply on systems without
full_constraints? This will cause the driver to loop on probe
deferral, right?

Bart
Mark Brown Oct. 22, 2019, 3:03 p.m. UTC | #11
On Tue, Oct 22, 2019 at 02:13:29PM +0200, Bartosz Golaszewski wrote:
> wt., 22 paź 2019 o 13:19 Mark Brown <broonie@kernel.org> napisał(a):

> > The ambiguously named regulator_get_optional().  This should *only* be
> > used for regulators that may be physically absent in the system, other
> > regulators should use normal regulator_get().  It is vanishingly
> > unlikely that all the supplies for a device will be optional.

> I take it that this driver should also use regular regulator_bulk_get() then?

It's really up to whoever maintains the driver but I'd recommend it
since it tends to be easier.

> I think the question about the return value of
> regulator_resolve_supply() still stands though: is it normal that it
> returns EPROBE_DEFER if it can't resolve the supply on systems without
> full_constraints? This will cause the driver to loop on probe
> deferral, right?

Yes, that's right.  The idea is that anything using regulators will call
that once it's finished setting up constraints, if you're not using
regulators at all you should compile out the API entirely.
Bartosz Golaszewski Oct. 22, 2019, 3:42 p.m. UTC | #12
wt., 22 paź 2019 o 17:03 Mark Brown <broonie@kernel.org> napisał(a):
>
> On Tue, Oct 22, 2019 at 02:13:29PM +0200, Bartosz Golaszewski wrote:
> > wt., 22 paź 2019 o 13:19 Mark Brown <broonie@kernel.org> napisał(a):
>
> > > The ambiguously named regulator_get_optional().  This should *only* be
> > > used for regulators that may be physically absent in the system, other
> > > regulators should use normal regulator_get().  It is vanishingly
> > > unlikely that all the supplies for a device will be optional.
>
> > I take it that this driver should also use regular regulator_bulk_get() then?
>
> It's really up to whoever maintains the driver but I'd recommend it
> since it tends to be easier.
>
> > I think the question about the return value of
> > regulator_resolve_supply() still stands though: is it normal that it
> > returns EPROBE_DEFER if it can't resolve the supply on systems without
> > full_constraints? This will cause the driver to loop on probe
> > deferral, right?
>
> Yes, that's right.  The idea is that anything using regulators will call
> that once it's finished setting up constraints, if you're not using
> regulators at all you should compile out the API entirely.

Ok, makes sense. Thanks Mark!

I'm just wondering if all boardfile-based systems actually do call it.
I recently had to sent a fix for old DaVinci boards where the lack of
this call caused one of the GPIO expanders to fail to probe.

Bibby: in that case please remove the entire has_supplies checking and
just rely on dummy regulators.

Bartosz
Mark Brown Oct. 22, 2019, 4:33 p.m. UTC | #13
On Tue, Oct 22, 2019 at 05:42:33PM +0200, Bartosz Golaszewski wrote:

> I'm just wondering if all boardfile-based systems actually do call it.
> I recently had to sent a fix for old DaVinci boards where the lack of
> this call caused one of the GPIO expanders to fail to probe.

Yeah, it's...  intermittent at best :/  This isn't a new thing in the
API and unfortunately we can't really tell where it's because the
constraints are currently half baked or because someone forgot to do it
so it's not safe to just make the changes blind.

Patch
diff mbox series

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2cccd82a3106..68ced4f25916 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -22,6 +22,7 @@ 
 #include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/gpio/consumer.h>
 
 /* Address pointer is 16 bit. */
@@ -67,6 +68,12 @@ 
  * which won't work on pure SMBus systems.
  */
 
+static const char * const at24_supply_names[] = {
+	"vcc", "i2c",
+};
+
+#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
+
 struct at24_client {
 	struct i2c_client *client;
 	struct regmap *regmap;
@@ -91,6 +98,8 @@  struct at24_data {
 
 	struct gpio_desc *wp_gpio;
 
+	bool has_supplies;
+	struct regulator_bulk_data supplies[AT24_NUM_SUPPLIES];
 	/*
 	 * Some chips tie up multiple I2C addresses; dummy devices reserve
 	 * them for us, and we'll use them with SMBus calls.
@@ -662,6 +671,17 @@  static int at24_probe(struct i2c_client *client)
 	at24->client[0].client = client;
 	at24->client[0].regmap = regmap;
 
+	regulator_bulk_set_supply_names(at24->supplies,
+					at24_supply_names, AT24_NUM_SUPPLIES);
+	err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
+				       AT24_NUM_SUPPLIES, at24->supplies);
+	if (err == -ENODEV)
+		at24->has_supplies = NULL;
+	else if (err == 0)
+		at24->has_supplies = !err;
+	else
+		return err;
+
 	at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
 	if (IS_ERR(at24->wp_gpio))
 		return PTR_ERR(at24->wp_gpio);
@@ -701,6 +721,14 @@  static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
+	if (at24->has_supplies) {
+		err = regulator_bulk_enable(AT24_NUM_SUPPLIES, at24->supplies);
+		if (err) {
+			dev_err(dev, "Failed to enable power regulators\n");
+			return err;
+		}
+	}
+
 	/* enable runtime pm */
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -713,6 +741,9 @@  static int at24_probe(struct i2c_client *client)
 	pm_runtime_idle(dev);
 	if (err) {
 		pm_runtime_disable(dev);
+		if (at24->has_supplies)
+			regulator_bulk_disable(AT24_NUM_SUPPLIES,
+					       at24->supplies);
 		return -ENODEV;
 	}
 
@@ -725,15 +756,48 @@  static int at24_probe(struct i2c_client *client)
 
 static int at24_remove(struct i2c_client *client)
 {
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
 	pm_runtime_disable(&client->dev);
 	pm_runtime_set_suspended(&client->dev);
+	if (at24->has_supplies)
+		regulator_bulk_disable(AT24_NUM_SUPPLIES, at24->supplies);
+
+	return 0;
+}
+
+static int __maybe_unused at24_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
+	if (at24->has_supplies)
+		return regulator_bulk_disable(AT24_NUM_SUPPLIES,
+					      at24->supplies);
+
+	return 0;
+}
+
+static int __maybe_unused at24_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
+	if (at24->has_supplies)
+		return regulator_bulk_enable(AT24_NUM_SUPPLIES,
+					     at24->supplies);
 
 	return 0;
 }
 
+static const struct dev_pm_ops at24_pm_ops = {
+	SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
+};
+
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.pm = &at24_pm_ops,
 		.of_match_table = at24_of_match,
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},