[PATCH v12 0/2] add power control in i2c and at24

[PATCH v12 0/2] add power control in i2c and at24

[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 v11:
 - use suspend_late/resume_early instead of suspend/resume
 - rebase onto v5.6-rc1

Changes since v10:
 - fixup some worng codes

Changes since v9:
 - fixup build error
 - remove redundant code

Changes since v8:
 - fixup some wrong code
 - remove redundant message

Changes since v7:
 - add binding describe supply property in i2c and at24.
 - move i2c bus supply control in i2c-core.
 - rebase onto v5.5-rc1

Changes since v6:
 - add back error check for devm_regulator_bulk_get()

        [... snip ...]

Bibby Hsieh (2):
  dt-binding: i2c: add bus-supply property
  i2c: core: support bus regulator controlling in adapter

 Documentation/devicetree/bindings/i2c/i2c.txt |  3 +
 drivers/i2c/i2c-core-base.c                   | 81 +++++++++++++++++++
 include/linux/i2c.h                           |  2 +
 3 files changed, 86 insertions(+)

-- 
2.18.0

[PATCH v12 1/2] dt-binding: i2c: add bus-supply property

[Bibby Hsieh]

In some platforms, they disable the power-supply of i2c due
to power consumption reduction. This patch add bus-supply property.

Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 Documentation/devicetree/bindings/i2c/i2c.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c.txt b/Documentation/devicetree/bindings/i2c/i2c.txt
index 9a53df4243c6..89d827805e89 100644
--- a/Documentation/devicetree/bindings/i2c/i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c.txt
@@ -98,6 +98,9 @@ wants to support one of the below features, it should adapt the bindings below.
 	Names of map programmable addresses.
 	It can contain any map needing another address than default one.
 
+- bus-supply
+	phandle to the regulator that provides power to i2c.
+
 Binding may contain optional "interrupts" property, describing interrupts
 used by the device. I2C core will assign "irq" interrupt (or the very first
 interrupt if not using interrupt names) as primary interrupt for the slave.
-- 
2.18.0

Re: [PATCH v12 1/2] dt-binding: i2c: add bus-supply property

[Wolfram Sang]

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

Hi,

thanks for the patches!

I have a high level question about them, first.

> +- bus-supply
> +	phandle to the regulator that provides power to i2c.

I think this is not clear enough. I assume it means the regulator for
driving SCL/SDA? In the coverletter you mention also an EEPROM. In your
case, this is driven by the same regulator? I am skeptical we can
abstract it like this because I2C devices could be driven by various
regulators in the system, so there couldn't be one "bus regulator". The
regulator for the EEPROM should be described in the EEPROM node. So,
this "bus supply" is only for driving SCL/SDA?

Kind regards,

   Wolfram


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

Re: [PATCH v12 1/2] dt-binding: i2c: add bus-supply property

[Tomasz Figa]

On Sun, Mar 22, 2020 at 2:07 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
> Hi,
>
> thanks for the patches!
>
> I have a high level question about them, first.
>
> > +- bus-supply
> > +     phandle to the regulator that provides power to i2c.
>
> I think this is not clear enough. I assume it means the regulator for
> driving SCL/SDA? In the coverletter you mention also an EEPROM. In your
> case, this is driven by the same regulator? I am skeptical we can
> abstract it like this because I2C devices could be driven by various
> regulators in the system, so there couldn't be one "bus regulator". The
> regulator for the EEPROM should be described in the EEPROM node. So,
> this "bus supply" is only for driving SCL/SDA?

In our case the bus-supply regulator drives the voltage rail to which
SCL/SDA are pulled up and there is an EEPROM on the bus, powered by
yet another rail. There is also another slave on the bus which uses
the same regulator as the bus-supply for its own power.

In other words, bus-supply only ensures that SCL and SDA are in a
usable state. Other consumers need to refer to the regulator in their
own supplies if they need it for their own power.

Does this answer your questions?

Best regards,
Tomasz

Re: [PATCH v12 1/2] dt-binding: i2c: add bus-supply property

[Wolfram Sang]

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


> > > +- bus-supply
> > > +     phandle to the regulator that provides power to i2c.
> >
> > I think this is not clear enough. I assume it means the regulator for
> > driving SCL/SDA? In the coverletter you mention also an EEPROM. In your
> > case, this is driven by the same regulator? I am skeptical we can
> > abstract it like this because I2C devices could be driven by various
> > regulators in the system, so there couldn't be one "bus regulator". The
> > regulator for the EEPROM should be described in the EEPROM node. So,
> > this "bus supply" is only for driving SCL/SDA?
> 
> In our case the bus-supply regulator drives the voltage rail to which
> SCL/SDA are pulled up and there is an EEPROM on the bus, powered by
> yet another rail. There is also another slave on the bus which uses
> the same regulator as the bus-supply for its own power.
> 
> In other words, bus-supply only ensures that SCL and SDA are in a
> usable state. Other consumers need to refer to the regulator in their
> own supplies if they need it for their own power.
> 
> Does this answer your questions?

Yes, this was exactly what I was assuming.

So, I think the above documentation should be changed: s|i2c|SCL/SDA|

D'accord?


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

Re: [PATCH v12 1/2] dt-binding: i2c: add bus-supply property

[Bibby Hsieh]

On Tue, 2020-03-24 at 11:34 +0100, Wolfram Sang wrote:
> > > > +- bus-supply
> > > > +     phandle to the regulator that provides power to i2c.
> > >
> > > I think this is not clear enough. I assume it means the regulator for
> > > driving SCL/SDA? In the coverletter you mention also an EEPROM. In your
> > > case, this is driven by the same regulator? I am skeptical we can
> > > abstract it like this because I2C devices could be driven by various
> > > regulators in the system, so there couldn't be one "bus regulator". The
> > > regulator for the EEPROM should be described in the EEPROM node. So,
> > > this "bus supply" is only for driving SCL/SDA?
> > 
> > In our case the bus-supply regulator drives the voltage rail to which
> > SCL/SDA are pulled up and there is an EEPROM on the bus, powered by
> > yet another rail. There is also another slave on the bus which uses
> > the same regulator as the bus-supply for its own power.
> > 
> > In other words, bus-supply only ensures that SCL and SDA are in a
> > usable state. Other consumers need to refer to the regulator in their
> > own supplies if they need it for their own power.
> > 
> > Does this answer your questions?
> 
> Yes, this was exactly what I was assuming.
> 
> So, I think the above documentation should be changed: s|i2c|SCL/SDA|
> 
> D'accord?
> 
Yes, I think SCL/SDA is better than i2c

Tomasz, How about it?

Thanks

Bibby

Re: [PATCH v12 1/2] dt-binding: i2c: add bus-supply property

[Tomasz Figa]

On Wed, Apr 15, 2020 at 10:50 AM Bibby Hsieh <bibby.hsieh@mediatek.com> wrote:
>
> On Tue, 2020-03-24 at 11:34 +0100, Wolfram Sang wrote:
> > > > > +- bus-supply
> > > > > +     phandle to the regulator that provides power to i2c.
> > > >
> > > > I think this is not clear enough. I assume it means the regulator for
> > > > driving SCL/SDA? In the coverletter you mention also an EEPROM. In your
> > > > case, this is driven by the same regulator? I am skeptical we can
> > > > abstract it like this because I2C devices could be driven by various
> > > > regulators in the system, so there couldn't be one "bus regulator". The
> > > > regulator for the EEPROM should be described in the EEPROM node. So,
> > > > this "bus supply" is only for driving SCL/SDA?
> > >
> > > In our case the bus-supply regulator drives the voltage rail to which
> > > SCL/SDA are pulled up and there is an EEPROM on the bus, powered by
> > > yet another rail. There is also another slave on the bus which uses
> > > the same regulator as the bus-supply for its own power.
> > >
> > > In other words, bus-supply only ensures that SCL and SDA are in a
> > > usable state. Other consumers need to refer to the regulator in their
> > > own supplies if they need it for their own power.
> > >
> > > Does this answer your questions?
> >
> > Yes, this was exactly what I was assuming.
> >
> > So, I think the above documentation should be changed: s|i2c|SCL/SDA|
> >
> > D'accord?
> >
> Yes, I think SCL/SDA is better than i2c
>
> Tomasz, How about it?

Sounds good to me.

Best regards,
Tomasz

[PATCH v12 2/2] i2c: core: support bus regulator controlling in adapter

[Bibby Hsieh]

Although in the most platforms, the bus power of i2c
are alway on, some platforms disable the i2c bus power
in order to meet low power request.

We get and enable bulk regulator in i2c adapter device.

Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
---
 drivers/i2c/i2c-core-base.c | 81 +++++++++++++++++++++++++++++++++++++
 include/linux/i2c.h         |  2 +
 2 files changed, 83 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index cefad0881942..4a1cee8389b4 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -313,6 +313,7 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
 static int i2c_device_probe(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_adapter	*adap = client->adapter;
 	struct i2c_driver	*driver;
 	int status;
 
@@ -378,6 +379,12 @@ static int i2c_device_probe(struct device *dev)
 
 	dev_dbg(dev, "probe\n");
 
+	status = regulator_enable(adap->bus_regulator);
+	if (status != 0) {
+		dev_err(&adap->dev, "Failed to enable power regulator\n");
+		goto err_clear_wakeup_irq;
+	}
+
 	status = of_clk_set_defaults(dev->of_node, false);
 	if (status < 0)
 		goto err_clear_wakeup_irq;
@@ -414,6 +421,7 @@ static int i2c_device_probe(struct device *dev)
 static int i2c_device_remove(struct device *dev)
 {
 	struct i2c_client	*client = i2c_verify_client(dev);
+	struct i2c_adapter      *adap = client->adapter;
 	struct i2c_driver	*driver;
 	int status = 0;
 
@@ -427,6 +435,8 @@ static int i2c_device_remove(struct device *dev)
 	}
 
 	dev_pm_domain_detach(&client->dev, true);
+	if (!pm_runtime_status_suspended(&client->dev))
+		regulator_disable(adap->bus_regulator);
 
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -438,6 +448,71 @@ static int i2c_device_remove(struct device *dev)
 	return status;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int i2c_resume_early(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	if (pm_runtime_status_suspended(&client->dev)) {
+		err = regulator_enable(adap->bus_regulator);
+		if (err)
+			return err;
+	}
+
+	return pm_generic_resume_early(&client->dev);
+}
+
+static int i2c_suspend_late(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	if (!pm_runtime_status_suspended(&client->dev)) {
+		err = regulator_disable(adap->bus_regulator);
+		if (err)
+			return err;
+	}
+
+	return pm_generic_suspend_late(&client->dev);
+}
+#endif
+
+#ifdef CONFIG_PM
+static int i2c_runtime_resume(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	err = regulator_enable(adap->bus_regulator);
+	if (err)
+		return err;
+
+	return pm_generic_runtime_resume(&client->dev);
+}
+
+static int i2c_runtime_suspend(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	err = pm_generic_runtime_suspend(&client->dev);
+	if (err)
+		return err;
+
+	return regulator_disable(adap->bus_regulator);
+}
+#endif
+
+static const struct dev_pm_ops i2c_device_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend_late, i2c_resume_early)
+	SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
+};
+
 static void i2c_device_shutdown(struct device *dev)
 {
 	struct i2c_client *client = i2c_verify_client(dev);
@@ -495,6 +570,7 @@ struct bus_type i2c_bus_type = {
 	.probe		= i2c_device_probe,
 	.remove		= i2c_device_remove,
 	.shutdown	= i2c_device_shutdown,
+	.pm		= &i2c_device_pm,
 };
 EXPORT_SYMBOL_GPL(i2c_bus_type);
 
@@ -1334,6 +1410,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
 		goto out_reg;
 
 	dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
+	adap->bus_regulator = devm_regulator_get(&adap->dev, "bus");
+	if (IS_ERR(adap->bus_regulator)) {
+		res = PTR_ERR(adap->bus_regulator);
+		goto out_reg;
+	}
 
 	pm_runtime_no_callbacks(&adap->dev);
 	pm_suspend_ignore_children(&adap->dev, true);
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index f834687989f7..d603077083fd 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -15,6 +15,7 @@
 #include <linux/device.h>	/* for struct device */
 #include <linux/sched.h>	/* for completion */
 #include <linux/mutex.h>
+#include <linux/regulator/consumer.h>
 #include <linux/rtmutex.h>
 #include <linux/irqdomain.h>		/* for Host Notify IRQ */
 #include <linux/of.h>		/* for struct device_node */
@@ -713,6 +714,7 @@ struct i2c_adapter {
 	const struct i2c_adapter_quirks *quirks;
 
 	struct irq_domain *host_notify_domain;
+	struct regulator *bus_regulator;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
2.18.0

Re: [PATCH v12 2/2] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

Hi Bibby,

On Fri, Mar 6, 2020 at 12:49 PM Bibby Hsieh <bibby.hsieh@mediatek.com> wrote:
>
> Although in the most platforms, the bus power of i2c
> are alway on, some platforms disable the i2c bus power
> in order to meet low power request.
>
> We get and enable bulk regulator in i2c adapter device.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> ---
>  drivers/i2c/i2c-core-base.c | 81 +++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  2 +
>  2 files changed, 83 insertions(+)
>

Thanks for the patch. Unfortunately there are still some issues left.
Please check my comments inline.

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index cefad0881942..4a1cee8389b4 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -313,6 +313,7 @@ static int i2c_smbus_host_notify_to_irq(const struct i2c_client *client)
>  static int i2c_device_probe(struct device *dev)
>  {
>         struct i2c_client       *client = i2c_verify_client(dev);
> +       struct i2c_adapter      *adap = client->adapter;
>         struct i2c_driver       *driver;
>         int status;
>
> @@ -378,6 +379,12 @@ static int i2c_device_probe(struct device *dev)
>
>         dev_dbg(dev, "probe\n");
>
> +       status = regulator_enable(adap->bus_regulator);
> +       if (status != 0) {

I'd make the check < 0, as the API doesn't specify the non-negative
values explicitly.

> +               dev_err(&adap->dev, "Failed to enable power regulator\n");
> +               goto err_clear_wakeup_irq;
> +       }
> +
>         status = of_clk_set_defaults(dev->of_node, false);
>         if (status < 0)
>                 goto err_clear_wakeup_irq;
> @@ -414,6 +421,7 @@ static int i2c_device_probe(struct device *dev)
>  static int i2c_device_remove(struct device *dev)
>  {
>         struct i2c_client       *client = i2c_verify_client(dev);
> +       struct i2c_adapter      *adap = client->adapter;
>         struct i2c_driver       *driver;
>         int status = 0;
>
> @@ -427,6 +435,8 @@ static int i2c_device_remove(struct device *dev)
>         }
>
>         dev_pm_domain_detach(&client->dev, true);
> +       if (!pm_runtime_status_suspended(&client->dev))
> +               regulator_disable(adap->bus_regulator);
>
>         dev_pm_clear_wake_irq(&client->dev);
>         device_init_wakeup(&client->dev, false);
> @@ -438,6 +448,71 @@ static int i2c_device_remove(struct device *dev)
>         return status;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume_early(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (pm_runtime_status_suspended(&client->dev)) {
> +               err = regulator_enable(adap->bus_regulator);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_resume_early(&client->dev);
> +}
> +
> +static int i2c_suspend_late(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (!pm_runtime_status_suspended(&client->dev)) {
> +               err = regulator_disable(adap->bus_regulator);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_suspend_late(&client->dev);

Let's think a bit more about the order of operations above. It could
end up with following sequence:

- regulator_disable()
- pm_generic_suspend_late()
  - some_i2c_device_suspend_late()
    - i2c_transfer() - ERROR because the bus regulator is disabled

We can only disable the regulator once the i2c client driver finished
its callback and suspended fully, which should guarantee no further
I2C accesses.

> +}
> +#endif
> +
> +#ifdef CONFIG_PM
> +static int i2c_runtime_resume(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       err = regulator_enable(adap->bus_regulator);
> +       if (err)
> +               return err;
> +
> +       return pm_generic_runtime_resume(&client->dev);
> +}
> +
> +static int i2c_runtime_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       err = pm_generic_runtime_suspend(&client->dev);
> +       if (err)
> +               return err;
> +
> +       return regulator_disable(adap->bus_regulator);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend_late, i2c_resume_early)

We wanted to set the late/early ops, but this macro sets the regular
suspend/resume ops. SET_LATE_SYSTEM_SLEEP_PM_OPS should be used
instead.

> +       SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
> +};
> +
>  static void i2c_device_shutdown(struct device *dev)
>  {
>         struct i2c_client *client = i2c_verify_client(dev);
> @@ -495,6 +570,7 @@ struct bus_type i2c_bus_type = {
>         .probe          = i2c_device_probe,
>         .remove         = i2c_device_remove,
>         .shutdown       = i2c_device_shutdown,
> +       .pm             = &i2c_device_pm,
>  };
>  EXPORT_SYMBOL_GPL(i2c_bus_type);
>
> @@ -1334,6 +1410,11 @@ static int i2c_register_adapter(struct i2c_adapter *adap)
>                 goto out_reg;
>
>         dev_dbg(&adap->dev, "adapter [%s] registered\n", adap->name);
> +       adap->bus_regulator = devm_regulator_get(&adap->dev, "bus");
> +       if (IS_ERR(adap->bus_regulator)) {
> +               res = PTR_ERR(adap->bus_regulator);
> +               goto out_reg;
> +       }

Wouldn't it be better to move this code before the dev_dbg() call? It
could be confusing if the debug log says "registered" and then the
devm_regulator_get() fails.

Best regards,
Tomasz