[PATCH v4] misc: eeprom: at24: support pm_runtime control

[PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bibby Hsieh]

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;
+	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

[PATCH v2] dt-binding: eeprom: at24: add supply properties

[Bibby Hsieh]

In some platforms, they disable the power-supply of eeprom and i2c due
to power consumption reduction.

This patch add two supply properties: vcc-supply, i2c-supply.

Changes since v1:
 - change supply name
 - rebase to next

Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
---
 Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
index e8778560d966..578487a5d9b7 100644
--- a/Documentation/devicetree/bindings/eeprom/at24.yaml
+++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
@@ -167,6 +167,14 @@ properties:
     minimum: 1
     maximum: 8
 
+  vcc-supply:
+    description:
+      phandle of the regulator that provides the supply voltage.
+
+  i2c-sypply:
+    description:
+      phandle to the regulator that provides power to i2c.
+
 required:
   - compatible
   - reg
-- 
2.18.0

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Bartosz Golaszewski]

pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> In some platforms, they disable the power-supply of eeprom and i2c due
> to power consumption reduction.
>
> This patch add two supply properties: vcc-supply, i2c-supply.
>
> Changes since v1:
>  - change supply name
>  - rebase to next
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> index e8778560d966..578487a5d9b7 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -167,6 +167,14 @@ properties:
>      minimum: 1
>      maximum: 8
>
> +  vcc-supply:
> +    description:
> +      phandle of the regulator that provides the supply voltage.
> +
> +  i2c-sypply:
> +    description:
> +      phandle to the regulator that provides power to i2c.
> +
>  required:
>    - compatible
>    - reg
> --
> 2.18.0
>

Acked-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>

Rob: if it looks good - can you take it through your branch together
with the conversion patch from this cycle?

Bart

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Bartosz Golaszewski]

pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> In some platforms, they disable the power-supply of eeprom and i2c due
> to power consumption reduction.
>
> This patch add two supply properties: vcc-supply, i2c-supply.
>
> Changes since v1:
>  - change supply name
>  - rebase to next
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> index e8778560d966..578487a5d9b7 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -167,6 +167,14 @@ properties:
>      minimum: 1
>      maximum: 8
>
> +  vcc-supply:
> +    description:
> +      phandle of the regulator that provides the supply voltage.
> +
> +  i2c-sypply:
> +    description:
> +      phandle to the regulator that provides power to i2c.
> +

Something was bothering me about this patch so I came back to take a
look. Can you explain what i2c actually stands for in this doc? I hope
I'm misinterpreting something and it isn't that the driver disables
the regulator powering the i2c bus controller?

Bart

>  required:
>    - compatible
>    - reg
> --
> 2.18.0
>

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Tomasz Figa]

On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> >
> > In some platforms, they disable the power-supply of eeprom and i2c due
> > to power consumption reduction.
> >
> > This patch add two supply properties: vcc-supply, i2c-supply.
> >
> > Changes since v1:
> >  - change supply name
> >  - rebase to next
> >
> > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > ---
> >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > index e8778560d966..578487a5d9b7 100644
> > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > @@ -167,6 +167,14 @@ properties:
> >      minimum: 1
> >      maximum: 8
> >
> > +  vcc-supply:
> > +    description:
> > +      phandle of the regulator that provides the supply voltage.
> > +
> > +  i2c-sypply:
> > +    description:
> > +      phandle to the regulator that provides power to i2c.
> > +
>
> Something was bothering me about this patch so I came back to take a
> look. Can you explain what i2c actually stands for in this doc? I hope
> I'm misinterpreting something and it isn't that the driver disables
> the regulator powering the i2c bus controller?

In our case it's the regulator that the I2C bus is pulled up to.

Best regards,
Tomasz

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Bartosz Golaszewski]

czw., 24 paź 2019 o 09:02 Tomasz Figa <tfiga@chromium.org> napisał(a):
>
> On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > >
> > > In some platforms, they disable the power-supply of eeprom and i2c due
> > > to power consumption reduction.
> > >
> > > This patch add two supply properties: vcc-supply, i2c-supply.
> > >
> > > Changes since v1:
> > >  - change supply name
> > >  - rebase to next
> > >
> > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > ---
> > >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> > >  1 file changed, 8 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > index e8778560d966..578487a5d9b7 100644
> > > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > @@ -167,6 +167,14 @@ properties:
> > >      minimum: 1
> > >      maximum: 8
> > >
> > > +  vcc-supply:
> > > +    description:
> > > +      phandle of the regulator that provides the supply voltage.
> > > +
> > > +  i2c-sypply:
> > > +    description:
> > > +      phandle to the regulator that provides power to i2c.
> > > +
> >
> > Something was bothering me about this patch so I came back to take a
> > look. Can you explain what i2c actually stands for in this doc? I hope
> > I'm misinterpreting something and it isn't that the driver disables
> > the regulator powering the i2c bus controller?
>
> In our case it's the regulator that the I2C bus is pulled up to.
>

Then it has nothing to do with a generic EEPROM driver IMO. I think
you need to add the control for this regulator to your i2c controller
driver and create a power domain where the EEPROM would be lower in
hierarchy.

Bart

> Best regards,
> Tomasz

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Tomasz Figa]

On Thu, Oct 24, 2019 at 5:40 PM Bartosz Golaszewski
<bgolaszewski@baylibre.com> wrote:
>
> czw., 24 paź 2019 o 09:02 Tomasz Figa <tfiga@chromium.org> napisał(a):
> >
> > On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > >
> > > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > >
> > > > In some platforms, they disable the power-supply of eeprom and i2c due
> > > > to power consumption reduction.
> > > >
> > > > This patch add two supply properties: vcc-supply, i2c-supply.
> > > >
> > > > Changes since v1:
> > > >  - change supply name
> > > >  - rebase to next
> > > >
> > > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > > ---
> > > >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> > > >  1 file changed, 8 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > index e8778560d966..578487a5d9b7 100644
> > > > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > @@ -167,6 +167,14 @@ properties:
> > > >      minimum: 1
> > > >      maximum: 8
> > > >
> > > > +  vcc-supply:
> > > > +    description:
> > > > +      phandle of the regulator that provides the supply voltage.
> > > > +
> > > > +  i2c-sypply:
> > > > +    description:
> > > > +      phandle to the regulator that provides power to i2c.
> > > > +
> > >
> > > Something was bothering me about this patch so I came back to take a
> > > look. Can you explain what i2c actually stands for in this doc? I hope
> > > I'm misinterpreting something and it isn't that the driver disables
> > > the regulator powering the i2c bus controller?
> >
> > In our case it's the regulator that the I2C bus is pulled up to.
> >
>
> Then it has nothing to do with a generic EEPROM driver IMO. I think
> you need to add the control for this regulator to your i2c controller
> driver and create a power domain where the EEPROM would be lower in
> hierarchy.

While I agree that the generic EEPROM driver may not be the best place
to do it, neither is a driver for a specific SoC i2c controller. The
hardware design is not specific to any particular i2c controller.

Perhaps we need the generic i2c core to take into account an
i2c-supply? Wolfram, any thoughts on this?

Best regards,
Tomasz

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Rob Herring]

On Thu, Oct 24, 2019 at 06:32:38PM +0900, Tomasz Figa wrote:
> On Thu, Oct 24, 2019 at 5:40 PM Bartosz Golaszewski
> <bgolaszewski@baylibre.com> wrote:
> >
> > czw., 24 paź 2019 o 09:02 Tomasz Figa <tfiga@chromium.org> napisał(a):
> > >
> > > On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> > > >
> > > > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > > >
> > > > > In some platforms, they disable the power-supply of eeprom and i2c due
> > > > > to power consumption reduction.
> > > > >
> > > > > This patch add two supply properties: vcc-supply, i2c-supply.
> > > > >
> > > > > Changes since v1:
> > > > >  - change supply name
> > > > >  - rebase to next
> > > > >
> > > > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > > > ---
> > > > >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> > > > >  1 file changed, 8 insertions(+)
> > > > >
> > > > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > index e8778560d966..578487a5d9b7 100644
> > > > > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > @@ -167,6 +167,14 @@ properties:
> > > > >      minimum: 1
> > > > >      maximum: 8
> > > > >
> > > > > +  vcc-supply:
> > > > > +    description:
> > > > > +      phandle of the regulator that provides the supply voltage.
> > > > > +
> > > > > +  i2c-sypply:
> > > > > +    description:
> > > > > +      phandle to the regulator that provides power to i2c.
> > > > > +
> > > >
> > > > Something was bothering me about this patch so I came back to take a
> > > > look. Can you explain what i2c actually stands for in this doc? I hope
> > > > I'm misinterpreting something and it isn't that the driver disables
> > > > the regulator powering the i2c bus controller?
> > >
> > > In our case it's the regulator that the I2C bus is pulled up to.
> > >
> >
> > Then it has nothing to do with a generic EEPROM driver IMO. I think
> > you need to add the control for this regulator to your i2c controller
> > driver and create a power domain where the EEPROM would be lower in
> > hierarchy.
> 
> While I agree that the generic EEPROM driver may not be the best place
> to do it, neither is a driver for a specific SoC i2c controller. The
> hardware design is not specific to any particular i2c controller.
> 
> Perhaps we need the generic i2c core to take into account an
> i2c-supply? Wolfram, any thoughts on this?

Sounds good to me. Maybe 'bus-supply' instead to indicate it's supposed 
to be for the bus and not other things. It should reside in the I2C 
controller's node (or mux ports) though.

Rob

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Bartosz Golaszewski]

pt., 25 paź 2019 o 23:10 Rob Herring <robh@kernel.org> napisał(a):
>
> On Thu, Oct 24, 2019 at 06:32:38PM +0900, Tomasz Figa wrote:
> > On Thu, Oct 24, 2019 at 5:40 PM Bartosz Golaszewski
> > <bgolaszewski@baylibre.com> wrote:
> > >
> > > czw., 24 paź 2019 o 09:02 Tomasz Figa <tfiga@chromium.org> napisał(a):
> > > >
> > > > On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
> > > > <bgolaszewski@baylibre.com> wrote:
> > > > >
> > > > > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > > > >
> > > > > > In some platforms, they disable the power-supply of eeprom and i2c due
> > > > > > to power consumption reduction.
> > > > > >
> > > > > > This patch add two supply properties: vcc-supply, i2c-supply.
> > > > > >
> > > > > > Changes since v1:
> > > > > >  - change supply name
> > > > > >  - rebase to next
> > > > > >
> > > > > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > > > > ---
> > > > > >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> > > > > >  1 file changed, 8 insertions(+)
> > > > > >
> > > > > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > index e8778560d966..578487a5d9b7 100644
> > > > > > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > @@ -167,6 +167,14 @@ properties:
> > > > > >      minimum: 1
> > > > > >      maximum: 8
> > > > > >
> > > > > > +  vcc-supply:
> > > > > > +    description:
> > > > > > +      phandle of the regulator that provides the supply voltage.
> > > > > > +
> > > > > > +  i2c-sypply:
> > > > > > +    description:
> > > > > > +      phandle to the regulator that provides power to i2c.
> > > > > > +
> > > > >
> > > > > Something was bothering me about this patch so I came back to take a
> > > > > look. Can you explain what i2c actually stands for in this doc? I hope
> > > > > I'm misinterpreting something and it isn't that the driver disables
> > > > > the regulator powering the i2c bus controller?
> > > >
> > > > In our case it's the regulator that the I2C bus is pulled up to.
> > > >
> > >
> > > Then it has nothing to do with a generic EEPROM driver IMO. I think
> > > you need to add the control for this regulator to your i2c controller
> > > driver and create a power domain where the EEPROM would be lower in
> > > hierarchy.
> >
> > While I agree that the generic EEPROM driver may not be the best place
> > to do it, neither is a driver for a specific SoC i2c controller. The
> > hardware design is not specific to any particular i2c controller.
> >
> > Perhaps we need the generic i2c core to take into account an
> > i2c-supply? Wolfram, any thoughts on this?
>
> Sounds good to me. Maybe 'bus-supply' instead to indicate it's supposed
> to be for the bus and not other things. It should reside in the I2C
> controller's node (or mux ports) though.
>
> Rob

Thanks,

in that case Bibby: please just use a single regulator for vcc-supply in at24.

Thanks,
Bartosz

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Bibby Hsieh]

On Sat, 2019-10-26 at 14:05 +0200, Bartosz Golaszewski wrote:
> pt., 25 paź 2019 o 23:10 Rob Herring <robh@kernel.org> napisał(a):
> >
> > On Thu, Oct 24, 2019 at 06:32:38PM +0900, Tomasz Figa wrote:
> > > On Thu, Oct 24, 2019 at 5:40 PM Bartosz Golaszewski
> > > <bgolaszewski@baylibre.com> wrote:
> > > >
> > > > czw., 24 paź 2019 o 09:02 Tomasz Figa <tfiga@chromium.org> napisał(a):
> > > > >
> > > > > On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
> > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > >
> > > > > > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > > > > >
> > > > > > > In some platforms, they disable the power-supply of eeprom and i2c due
> > > > > > > to power consumption reduction.
> > > > > > >
> > > > > > > This patch add two supply properties: vcc-supply, i2c-supply.
> > > > > > >
> > > > > > > Changes since v1:
> > > > > > >  - change supply name
> > > > > > >  - rebase to next
> > > > > > >
> > > > > > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > > > > > ---
> > > > > > >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> > > > > > >  1 file changed, 8 insertions(+)
> > > > > > >
> > > > > > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > > index e8778560d966..578487a5d9b7 100644
> > > > > > > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > > @@ -167,6 +167,14 @@ properties:
> > > > > > >      minimum: 1
> > > > > > >      maximum: 8
> > > > > > >
> > > > > > > +  vcc-supply:
> > > > > > > +    description:
> > > > > > > +      phandle of the regulator that provides the supply voltage.
> > > > > > > +
> > > > > > > +  i2c-sypply:
> > > > > > > +    description:
> > > > > > > +      phandle to the regulator that provides power to i2c.
> > > > > > > +
> > > > > >
> > > > > > Something was bothering me about this patch so I came back to take a
> > > > > > look. Can you explain what i2c actually stands for in this doc? I hope
> > > > > > I'm misinterpreting something and it isn't that the driver disables
> > > > > > the regulator powering the i2c bus controller?
> > > > >
> > > > > In our case it's the regulator that the I2C bus is pulled up to.
> > > > >
> > > >
> > > > Then it has nothing to do with a generic EEPROM driver IMO. I think
> > > > you need to add the control for this regulator to your i2c controller
> > > > driver and create a power domain where the EEPROM would be lower in
> > > > hierarchy.
> > >
> > > While I agree that the generic EEPROM driver may not be the best place
> > > to do it, neither is a driver for a specific SoC i2c controller. The
> > > hardware design is not specific to any particular i2c controller.
> > >
> > > Perhaps we need the generic i2c core to take into account an
> > > i2c-supply? Wolfram, any thoughts on this?
> >
> > Sounds good to me. Maybe 'bus-supply' instead to indicate it's supposed
> > to be for the bus and not other things. It should reside in the I2C
> > controller's node (or mux ports) though.
> >
> > Rob
> 
> Thanks,
> 
> in that case Bibby: please just use a single regulator for vcc-supply in at24.

To my understanding, there are something I need to do.
1. remove i2c-supply property from DT.
2. just control vcc-supply in at24 driver.
3. add i2c-supply control in i2c and i2c-supply property in DT?

Is there any mistakes?

Bibby

> 
> Thanks,
> Bartosz

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Tomasz Figa]

Hi Bibby,

On Mon, Oct 28, 2019 at 11:36 AM Bibby Hsieh <bibby.hsieh@mediatek.com> wrote:
>
> On Sat, 2019-10-26 at 14:05 +0200, Bartosz Golaszewski wrote:
> > pt., 25 paź 2019 o 23:10 Rob Herring <robh@kernel.org> napisał(a):
> > >
> > > On Thu, Oct 24, 2019 at 06:32:38PM +0900, Tomasz Figa wrote:
> > > > On Thu, Oct 24, 2019 at 5:40 PM Bartosz Golaszewski
> > > > <bgolaszewski@baylibre.com> wrote:
> > > > >
> > > > > czw., 24 paź 2019 o 09:02 Tomasz Figa <tfiga@chromium.org> napisał(a):
> > > > > >
> > > > > > On Thu, Oct 24, 2019 at 3:22 PM Bartosz Golaszewski
> > > > > > <bgolaszewski@baylibre.com> wrote:
> > > > > > >
> > > > > > > pt., 18 paź 2019 o 10:26 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
> > > > > > > >
> > > > > > > > In some platforms, they disable the power-supply of eeprom and i2c due
> > > > > > > > to power consumption reduction.
> > > > > > > >
> > > > > > > > This patch add two supply properties: vcc-supply, i2c-supply.
> > > > > > > >
> > > > > > > > Changes since v1:
> > > > > > > >  - change supply name
> > > > > > > >  - rebase to next
> > > > > > > >
> > > > > > > > Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> > > > > > > > ---
> > > > > > > >  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
> > > > > > > >  1 file changed, 8 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > > > index e8778560d966..578487a5d9b7 100644
> > > > > > > > --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > > > +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> > > > > > > > @@ -167,6 +167,14 @@ properties:
> > > > > > > >      minimum: 1
> > > > > > > >      maximum: 8
> > > > > > > >
> > > > > > > > +  vcc-supply:
> > > > > > > > +    description:
> > > > > > > > +      phandle of the regulator that provides the supply voltage.
> > > > > > > > +
> > > > > > > > +  i2c-sypply:
> > > > > > > > +    description:
> > > > > > > > +      phandle to the regulator that provides power to i2c.
> > > > > > > > +
> > > > > > >
> > > > > > > Something was bothering me about this patch so I came back to take a
> > > > > > > look. Can you explain what i2c actually stands for in this doc? I hope
> > > > > > > I'm misinterpreting something and it isn't that the driver disables
> > > > > > > the regulator powering the i2c bus controller?
> > > > > >
> > > > > > In our case it's the regulator that the I2C bus is pulled up to.
> > > > > >
> > > > >
> > > > > Then it has nothing to do with a generic EEPROM driver IMO. I think
> > > > > you need to add the control for this regulator to your i2c controller
> > > > > driver and create a power domain where the EEPROM would be lower in
> > > > > hierarchy.
> > > >
> > > > While I agree that the generic EEPROM driver may not be the best place
> > > > to do it, neither is a driver for a specific SoC i2c controller. The
> > > > hardware design is not specific to any particular i2c controller.
> > > >
> > > > Perhaps we need the generic i2c core to take into account an
> > > > i2c-supply? Wolfram, any thoughts on this?
> > >
> > > Sounds good to me. Maybe 'bus-supply' instead to indicate it's supposed
> > > to be for the bus and not other things. It should reside in the I2C
> > > controller's node (or mux ports) though.
> > >
> > > Rob
> >
> > Thanks,
> >
> > in that case Bibby: please just use a single regulator for vcc-supply in at24.
>
> To my understanding, there are something I need to do.
> 1. remove i2c-supply property from DT.
> 2. just control vcc-supply in at24 driver.
> 3. add i2c-supply control in i2c and i2c-supply property in DT?
>
> Is there any mistakes?

Sorry for the late reply, just came back from a trip.

With the replacement of i2c-supply to bus-supply in 3), as suggested
by Rob, sounds good to me.

Best regards,
Tomasz

Re: [PATCH v2] dt-binding: eeprom: at24: add supply properties

[Peter Rosin]

On 2019-10-18 10:25, Bibby Hsieh wrote:
> In some platforms, they disable the power-supply of eeprom and i2c due
> to power consumption reduction.
> 
> This patch add two supply properties: vcc-supply, i2c-supply.
> 
> Changes since v1:
>  - change supply name
>  - rebase to next
> 
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  Documentation/devicetree/bindings/eeprom/at24.yaml | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/eeprom/at24.yaml b/Documentation/devicetree/bindings/eeprom/at24.yaml
> index e8778560d966..578487a5d9b7 100644
> --- a/Documentation/devicetree/bindings/eeprom/at24.yaml
> +++ b/Documentation/devicetree/bindings/eeprom/at24.yaml
> @@ -167,6 +167,14 @@ properties:
>      minimum: 1
>      maximum: 8
>  
> +  vcc-supply:
> +    description:
> +      phandle of the regulator that provides the supply voltage.
> +
> +  i2c-sypply:

s/sypply/supply/

Cheers,
Peter

> +    description:
> +      phandle to the regulator that provides power to i2c.
> +
>  required:
>    - compatible
>    - reg
> 

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Tomasz Figa]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bartosz Golaszewski]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bibby Hsieh]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bartosz Golaszewski]

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
>

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bibby Hsieh]

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
> >

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Tomasz Figa]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bartosz Golaszewski]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Tomasz Figa]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1066 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bartosz Golaszewski]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1107 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Bartosz Golaszewski]

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

Re: [PATCH v4] misc: eeprom: at24: support pm_runtime control

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 536 bytes --]

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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]