[v8,3/4] misc: eeprom: at24: support pm_runtime control
diff mbox series

Message ID 20191213081230.23494-4-bibby.hsieh@mediatek.com
State Superseded
Headers show
Series
  • add power control in i2c and at24
Related show

Commit Message

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

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

Comments

Bartosz Golaszewski Dec. 13, 2019, 9:02 a.m. UTC | #1
pt., 13 gru 2019 o 09:47 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> Although in the most platforms, the power of eeprom are alway
> on, some platforms disable the eeprom power in order to meet
> low power request. This patch add the pm_runtime ops to control
> power to support all platforms.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  drivers/misc/eeprom/at24.c | 40 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 0681d5fdd538..06ae2cc32f79 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. */
> @@ -91,6 +92,7 @@ struct at24_data {
>
>         struct gpio_desc *wp_gpio;
>
> +       struct regulator *vcc_reg;
>         /*
>          * Some chips tie up multiple I2C addresses; dummy devices reserve
>          * them for us, and we'll use them with SMBus calls.
> @@ -662,6 +664,12 @@ static int at24_probe(struct i2c_client *client)
>         at24->client[0].client = client;
>         at24->client[0].regmap = regmap;
>
> +       at24->vcc_reg = devm_regulator_get(dev, "vcc");
> +       if (IS_ERR(at24->vcc_reg)) {
> +               dev_err(dev, "failed to get at24 VCC regulator\n");

The regulator core is quite verbose in its error messages when calling
regulator_get() - you don't need to add yours here. Just return the
error code.

> +               return PTR_ERR(at24->vcc_reg);
> +       }
> +
>         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 +709,12 @@ 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 at24 vcc regulator\n");

Drop the at24 name - dev_err() will print the device name for you.

> +               return err;
> +       }
> +
>         /* enable runtime pm */
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
> @@ -713,6 +727,7 @@ static int at24_probe(struct i2c_client *client)
>         pm_runtime_idle(dev);
>         if (err) {
>                 pm_runtime_disable(dev);
> +               regulator_disable(at24->vcc_reg);
>                 return -ENODEV;
>         }
>
> @@ -729,14 +744,39 @@ static int at24_probe(struct i2c_client *client)
>  static int at24_remove(struct i2c_client *client)
>  {
>         pm_runtime_disable(&client->dev);
> +       if (pm_runtime_status_suspended(&client->dev))
> +               regulator_disable(at24->vcc_reg);

Why didn't you fix the inverted logic here as I pointed out back in v6
of this series?

Bart

>         pm_runtime_set_suspended(&client->dev);
>
>         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);
> +
> +       return regulator_disable(at24->vcc_reg);
> +}
> +
> +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);
> +
> +       return regulator_enable(at24->vcc_reg);
> +}
> +
> +static const struct dev_pm_ops at24_pm_ops = {
> +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> +                               pm_runtime_force_resume)
> +       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 Dec. 13, 2019, 9:08 a.m. UTC | #2
On Fri, 2019-12-13 at 10:02 +0100, Bartosz Golaszewski wrote:
> pt., 13 gru 2019 o 09:47 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> >
> > Although in the most platforms, the power of eeprom are alway
> > on, some platforms disable the eeprom power in order to meet
> > low power request. This patch add the pm_runtime ops to control
> > power to support all platforms.
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  drivers/misc/eeprom/at24.c | 40 ++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 0681d5fdd538..06ae2cc32f79 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. */
> > @@ -91,6 +92,7 @@ struct at24_data {
> >
> >         struct gpio_desc *wp_gpio;
> >
> > +       struct regulator *vcc_reg;
> >         /*
> >          * Some chips tie up multiple I2C addresses; dummy devices reserve
> >          * them for us, and we'll use them with SMBus calls.
> > @@ -662,6 +664,12 @@ static int at24_probe(struct i2c_client *client)
> >         at24->client[0].client = client;
> >         at24->client[0].regmap = regmap;
> >
> > +       at24->vcc_reg = devm_regulator_get(dev, "vcc");
> > +       if (IS_ERR(at24->vcc_reg)) {
> > +               dev_err(dev, "failed to get at24 VCC regulator\n");
> 
> The regulator core is quite verbose in its error messages when calling
> regulator_get() - you don't need to add yours here. Just return the
> error code.

Ok, will remove it.

> 
> > +               return PTR_ERR(at24->vcc_reg);
> > +       }
> > +
> >         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 +709,12 @@ 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 at24 vcc regulator\n");
> 
> Drop the at24 name - dev_err() will print the device name for you.
> 
Ok.
> > +               return err;
> > +       }
> > +
> >         /* enable runtime pm */
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> > @@ -713,6 +727,7 @@ static int at24_probe(struct i2c_client *client)
> >         pm_runtime_idle(dev);
> >         if (err) {
> >                 pm_runtime_disable(dev);
> > +               regulator_disable(at24->vcc_reg);
> >                 return -ENODEV;
> >         }
> >
> > @@ -729,14 +744,39 @@ static int at24_probe(struct i2c_client *client)
> >  static int at24_remove(struct i2c_client *client)
> >  {
> >         pm_runtime_disable(&client->dev);
> > +       if (pm_runtime_status_suspended(&client->dev))
> > +               regulator_disable(at24->vcc_reg);
> 
> Why didn't you fix the inverted logic here as I pointed out back in v6
> of this series?
> 
Sorry for missing it again...
I will add invert next version.

Bibby
> Bart
> 
> >         pm_runtime_set_suspended(&client->dev);
> >
> >         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);
> > +
> > +       return regulator_disable(at24->vcc_reg);
> > +}
> > +
> > +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);
> > +
> > +       return regulator_enable(at24->vcc_reg);
> > +}
> > +
> > +static const struct dev_pm_ops at24_pm_ops = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > +                               pm_runtime_force_resume)
> > +       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

Patch
diff mbox series

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538..06ae2cc32f79 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. */
@@ -91,6 +92,7 @@  struct at24_data {
 
 	struct gpio_desc *wp_gpio;
 
+	struct regulator *vcc_reg;
 	/*
 	 * Some chips tie up multiple I2C addresses; dummy devices reserve
 	 * them for us, and we'll use them with SMBus calls.
@@ -662,6 +664,12 @@  static int at24_probe(struct i2c_client *client)
 	at24->client[0].client = client;
 	at24->client[0].regmap = regmap;
 
+	at24->vcc_reg = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(at24->vcc_reg)) {
+		dev_err(dev, "failed to get at24 VCC regulator\n");
+		return PTR_ERR(at24->vcc_reg);
+	}
+
 	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 +709,12 @@  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 at24 vcc regulator\n");
+		return err;
+	}
+
 	/* enable runtime pm */
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -713,6 +727,7 @@  static int at24_probe(struct i2c_client *client)
 	pm_runtime_idle(dev);
 	if (err) {
 		pm_runtime_disable(dev);
+		regulator_disable(at24->vcc_reg);
 		return -ENODEV;
 	}
 
@@ -729,14 +744,39 @@  static int at24_probe(struct i2c_client *client)
 static int at24_remove(struct i2c_client *client)
 {
 	pm_runtime_disable(&client->dev);
+	if (pm_runtime_status_suspended(&client->dev))
+		regulator_disable(at24->vcc_reg);
 	pm_runtime_set_suspended(&client->dev);
 
 	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);
+
+	return regulator_disable(at24->vcc_reg);
+}
+
+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);
+
+	return regulator_enable(at24->vcc_reg);
+}
+
+static const struct dev_pm_ops at24_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	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),
 	},