| Message ID | 20191018082557.3696-1-bibby.hsieh@mediatek.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v4] misc: eeprom: at24: support pm_runtime control | expand |
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
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
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 >
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 > >
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
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
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
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
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.
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
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.
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
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.
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), },
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(+)