diff mbox series

[v1] misc: eeprom: at24: support pm_runtime control

Message ID 20190902033205.30276-1-bibby.hsieh@mediatek.com
State Superseded
Delegated to: Bartosz Golaszewski
Headers show
Series [v1] misc: eeprom: at24: support pm_runtime control | expand

Commit Message

Bibby Hsieh Sept. 2, 2019, 3:32 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 v1:
 - remove redundant code
 - fixup coding style

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

Comments

Bartosz Golaszewski Sept. 2, 2019, 7:23 a.m. UTC | #1
pon., 2 wrz 2019 o 05:32 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 v1:
>  - remove redundant code
>  - fixup coding style
>

The subject tag should be v2. Patches start at (implicit) version 1.
It's even more confusing since you're saying "Changes since v1" here
but the subject says v1 too.

The patch now looks good, but it's too late in the release cycle for
this kind of change. I'd like it to spend some time in next, so I'll
pick it up after the v5.4 merge window.

Bart

> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  drivers/misc/eeprom/at24.c | 59 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 35bf2477693d..ce17e82eedca 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -23,6 +23,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. */
> @@ -68,6 +69,12 @@
>   * which won't work on pure SMBus systems.
>   */
>
> +static const char * const at24_supply_names[] = {
> +       "power", "i2c",
> +};
> +
> +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> +
>  struct at24_client {
>         struct i2c_client *client;
>         struct regmap *regmap;
> @@ -92,6 +99,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.
> @@ -663,6 +672,15 @@ static int at24_probe(struct i2c_client *client)
>         at24->client[0].client = client;
>         at24->client[0].regmap = regmap;
>
> +       for (i = 0; i < AT24_NUM_SUPPLIES; i++)
> +               at24->supplies[i].supply = at24_supply_names[i];
> +
> +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> +                                      AT24_NUM_SUPPLIES, at24->supplies);
> +       if (err == -EPROBE_DEFER)
> +               return err;
> +       at24->has_supplies = !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);
> @@ -705,13 +723,21 @@ static int at24_probe(struct i2c_client *client)
>         /* enable runtime pm */
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
> +       pm_runtime_get_sync(dev);
> +       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;
> +               }
> +       }
>
>         /*
>          * Perform a one-byte test read to verify that the
>          * chip is functional.
>          */
>         err = at24_read(at24, 0, &test_byte, 1);
> -       pm_runtime_idle(dev);
> +       pm_runtime_put(dev);
>         if (err) {
>                 pm_runtime_disable(dev);
>                 return -ENODEV;
> @@ -726,15 +752,46 @@ 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 SIMPLE_DEV_PM_OPS(at24_pm_ops, at24_suspend, at24_resume);
> +
>  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 Sept. 2, 2019, 7:42 a.m. UTC | #2
On Mon, 2019-09-02 at 09:23 +0200, Bartosz Golaszewski wrote:
> pon., 2 wrz 2019 o 05:32 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 v1:
> >  - remove redundant code
> >  - fixup coding style
> >
> 
> The subject tag should be v2. Patches start at (implicit) version 1.
> It's even more confusing since you're saying "Changes since v1" here
> but the subject says v1 too.
> 
Ok, I will fix up it and resent version 2.

> The patch now looks good, but it's too late in the release cycle for
> this kind of change. I'd like it to spend some time in next, so I'll
> pick it up after the v5.4 merge window.
> 
It's OK, thanks.

> Bart
> 
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 59 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 58 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 35bf2477693d..ce17e82eedca 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -23,6 +23,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. */
> > @@ -68,6 +69,12 @@
> >   * which won't work on pure SMBus systems.
> >   */
> >
> > +static const char * const at24_supply_names[] = {
> > +       "power", "i2c",
> > +};
> > +
> > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > +
> >  struct at24_client {
> >         struct i2c_client *client;
> >         struct regmap *regmap;
> > @@ -92,6 +99,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.
> > @@ -663,6 +672,15 @@ static int at24_probe(struct i2c_client *client)
> >         at24->client[0].client = client;
> >         at24->client[0].regmap = regmap;
> >
> > +       for (i = 0; i < AT24_NUM_SUPPLIES; i++)
> > +               at24->supplies[i].supply = at24_supply_names[i];
> > +
> > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > +       if (err == -EPROBE_DEFER)
> > +               return err;
> > +       at24->has_supplies = !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);
> > @@ -705,13 +723,21 @@ static int at24_probe(struct i2c_client *client)
> >         /* enable runtime pm */
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> > +       pm_runtime_get_sync(dev);
> > +       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;
> > +               }
> > +       }
> >
> >         /*
> >          * Perform a one-byte test read to verify that the
> >          * chip is functional.
> >          */
> >         err = at24_read(at24, 0, &test_byte, 1);
> > -       pm_runtime_idle(dev);
> > +       pm_runtime_put(dev);
> >         if (err) {
> >                 pm_runtime_disable(dev);
> >                 return -ENODEV;
> > @@ -726,15 +752,46 @@ 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 SIMPLE_DEV_PM_OPS(at24_pm_ops, at24_suspend, at24_resume);
> > +
> >  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
> >
Bartosz Golaszewski Sept. 2, 2019, 8:11 a.m. UTC | #3
pon., 2 wrz 2019 o 09:42 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> On Mon, 2019-09-02 at 09:23 +0200, Bartosz Golaszewski wrote:
> > pon., 2 wrz 2019 o 05:32 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 v1:
> > >  - remove redundant code
> > >  - fixup coding style
> > >
> >
> > The subject tag should be v2. Patches start at (implicit) version 1.
> > It's even more confusing since you're saying "Changes since v1" here
> > but the subject says v1 too.
> >
> Ok, I will fix up it and resent version 2.
>

Please do it after v5.4-rc1 is tagged though.

Bart

> > The patch now looks good, but it's too late in the release cycle for
> > this kind of change. I'd like it to spend some time in next, so I'll
> > pick it up after the v5.4 merge window.
> >
> It's OK, thanks.
>
> > Bart
> >
> > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > ---
> > >  drivers/misc/eeprom/at24.c | 59 +++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 58 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > > index 35bf2477693d..ce17e82eedca 100644
> > > --- a/drivers/misc/eeprom/at24.c
> > > +++ b/drivers/misc/eeprom/at24.c
> > > @@ -23,6 +23,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. */
> > > @@ -68,6 +69,12 @@
> > >   * which won't work on pure SMBus systems.
> > >   */
> > >
> > > +static const char * const at24_supply_names[] = {
> > > +       "power", "i2c",
> > > +};
> > > +
> > > +#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
> > > +
> > >  struct at24_client {
> > >         struct i2c_client *client;
> > >         struct regmap *regmap;
> > > @@ -92,6 +99,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.
> > > @@ -663,6 +672,15 @@ static int at24_probe(struct i2c_client *client)
> > >         at24->client[0].client = client;
> > >         at24->client[0].regmap = regmap;
> > >
> > > +       for (i = 0; i < AT24_NUM_SUPPLIES; i++)
> > > +               at24->supplies[i].supply = at24_supply_names[i];
> > > +
> > > +       err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
> > > +                                      AT24_NUM_SUPPLIES, at24->supplies);
> > > +       if (err == -EPROBE_DEFER)
> > > +               return err;
> > > +       at24->has_supplies = !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);
> > > @@ -705,13 +723,21 @@ static int at24_probe(struct i2c_client *client)
> > >         /* enable runtime pm */
> > >         pm_runtime_set_active(dev);
> > >         pm_runtime_enable(dev);
> > > +       pm_runtime_get_sync(dev);
> > > +       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;
> > > +               }
> > > +       }
> > >
> > >         /*
> > >          * Perform a one-byte test read to verify that the
> > >          * chip is functional.
> > >          */
> > >         err = at24_read(at24, 0, &test_byte, 1);
> > > -       pm_runtime_idle(dev);
> > > +       pm_runtime_put(dev);
> > >         if (err) {
> > >                 pm_runtime_disable(dev);
> > >                 return -ENODEV;
> > > @@ -726,15 +752,46 @@ 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 SIMPLE_DEV_PM_OPS(at24_pm_ops, at24_suspend, at24_resume);
> > > +
> > >  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
> > >
>
>
Tomasz Figa Sept. 3, 2019, 6:09 a.m. UTC | #4
On Mon, Sep 2, 2019 at 5:11 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> pon., 2 wrz 2019 o 09:42 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> >
> > On Mon, 2019-09-02 at 09:23 +0200, Bartosz Golaszewski wrote:
> > > pon., 2 wrz 2019 o 05:32 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 v1:
> > > >  - remove redundant code
> > > >  - fixup coding style
> > > >
> > >
> > > The subject tag should be v2. Patches start at (implicit) version 1.
> > > It's even more confusing since you're saying "Changes since v1" here
> > > but the subject says v1 too.
> > >
> > Ok, I will fix up it and resent version 2.
> >
>
> Please do it after v5.4-rc1 is tagged though.

Thanks Bart for reviewing.

By the way, I think we also need the DT bindings to be updated, right?

Best regards,
Tomasz
Bartosz Golaszewski Sept. 3, 2019, 6:54 a.m. UTC | #5
wt., 3 wrz 2019 o 08:10 Tomasz Figa <tfiga@chromium.org> napisał(a):
>
> On Mon, Sep 2, 2019 at 5:11 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > pon., 2 wrz 2019 o 09:42 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > >
> > > On Mon, 2019-09-02 at 09:23 +0200, Bartosz Golaszewski wrote:
> > > > pon., 2 wrz 2019 o 05:32 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 v1:
> > > > >  - remove redundant code
> > > > >  - fixup coding style
> > > > >
> > > >
> > > > The subject tag should be v2. Patches start at (implicit) version 1.
> > > > It's even more confusing since you're saying "Changes since v1" here
> > > > but the subject says v1 too.
> > > >
> > > Ok, I will fix up it and resent version 2.
> > >
> >
> > Please do it after v5.4-rc1 is tagged though.
>
> Thanks Bart for reviewing.
>
> By the way, I think we also need the DT bindings to be updated, right?
>
> Best regards,
> Tomasz

Yes, definitely, thanks for pointing that out.

Bibby: the DT updates should come in a separate patch and please Cc
the device-tree maintainers.

Bart
Bibby Hsieh Sept. 3, 2019, 7:10 a.m. UTC | #6
On Tue, 2019-09-03 at 08:54 +0200, Bartosz Golaszewski wrote:
> wt., 3 wrz 2019 o 08:10 Tomasz Figa <tfiga@chromium.org> napisał(a):
> >
> > On Mon, Sep 2, 2019 at 5:11 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > >
> > > pon., 2 wrz 2019 o 09:42 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > >
> > > > On Mon, 2019-09-02 at 09:23 +0200, Bartosz Golaszewski wrote:
> > > > > pon., 2 wrz 2019 o 05:32 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 v1:
> > > > > >  - remove redundant code
> > > > > >  - fixup coding style
> > > > > >
> > > > >
> > > > > The subject tag should be v2. Patches start at (implicit) version 1.
> > > > > It's even more confusing since you're saying "Changes since v1" here
> > > > > but the subject says v1 too.
> > > > >
> > > > Ok, I will fix up it and resent version 2.
> > > >
> > >
> > > Please do it after v5.4-rc1 is tagged though.
> >
> > Thanks Bart for reviewing.
> >
> > By the way, I think we also need the DT bindings to be updated, right?
> >

Thanks, Tomasz :)

> > Best regards,
> > Tomasz
> 
> Yes, definitely, thanks for pointing that out.
> 
> Bibby: the DT updates should come in a separate patch and please Cc
> the device-tree maintainers.

Got it, I will add two optional properties (power-supply and i2c-supply)
in DT.

Bibby
> 
> Bart
Pi-Hsun Shih Sept. 24, 2019, 10:08 a.m. UTC | #7
Hi Bibby,

On 9/2/19 11:32 AM, Bibby Hsieh wrote:
> ...
> +	pm_runtime_get_sync(dev);
> +	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;
> +		}
> +	} >
>   	/*
>   	 * Perform a one-byte test read to verify that the
>   	 * chip is functional.
>   	 */
>   	err = at24_read(at24, 0, &test_byte, 1);
> -	pm_runtime_idle(dev);
> +	pm_runtime_put(dev);
>   	if (err) {
>   		pm_runtime_disable(dev);

Should there be corresponding regulator_bulk_disable in the error path 
here and below?

>   		return -ENODEV;
> @@ -726,15 +752,46 @@ 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;
>   } > ...
diff mbox series

Patch

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 35bf2477693d..ce17e82eedca 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -23,6 +23,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. */
@@ -68,6 +69,12 @@ 
  * which won't work on pure SMBus systems.
  */
 
+static const char * const at24_supply_names[] = {
+	"power", "i2c",
+};
+
+#define AT24_NUM_SUPPLIES ARRAY_SIZE(at24_supply_names)
+
 struct at24_client {
 	struct i2c_client *client;
 	struct regmap *regmap;
@@ -92,6 +99,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.
@@ -663,6 +672,15 @@  static int at24_probe(struct i2c_client *client)
 	at24->client[0].client = client;
 	at24->client[0].regmap = regmap;
 
+	for (i = 0; i < AT24_NUM_SUPPLIES; i++)
+		at24->supplies[i].supply = at24_supply_names[i];
+
+	err =  devm_regulator_bulk_get(&at24->client[0].client->dev,
+				       AT24_NUM_SUPPLIES, at24->supplies);
+	if (err == -EPROBE_DEFER)
+		return err;
+	at24->has_supplies = !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);
@@ -705,13 +723,21 @@  static int at24_probe(struct i2c_client *client)
 	/* enable runtime pm */
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
+	pm_runtime_get_sync(dev);
+	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;
+		}
+	}
 
 	/*
 	 * Perform a one-byte test read to verify that the
 	 * chip is functional.
 	 */
 	err = at24_read(at24, 0, &test_byte, 1);
-	pm_runtime_idle(dev);
+	pm_runtime_put(dev);
 	if (err) {
 		pm_runtime_disable(dev);
 		return -ENODEV;
@@ -726,15 +752,46 @@  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 SIMPLE_DEV_PM_OPS(at24_pm_ops, at24_suspend, at24_resume);
+
 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),
 	},