[PATCH v10 0/4] add power control in i2c and at24

[PATCH v10 0/4] 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 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()

Changes since v5:
 - remove has_supplies

Changes since v4:
 - add system sleep PM ops
 - move regulator_bulk_disable before suspend()
 - fixes error handling

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

Bibby Hsieh (4):
  dt-binding: eeprom: at24: add vcc-supply property
  dt-binding: i2c: add bus-supply property
  misc: eeprom: at24: support pm_runtime control
  i2c: core: support bus regulator controlling in adapter

 .../devicetree/bindings/eeprom/at24.yaml      |  4 +
 Documentation/devicetree/bindings/i2c/i2c.txt |  3 +
 drivers/i2c/i2c-core-base.c                   | 81 +++++++++++++++++++
 drivers/misc/eeprom/at24.c                    | 40 +++++++++
 include/linux/i2c.h                           |  3 +
 5 files changed, 131 insertions(+)

-- 
2.18.0

[PATCH v10 0/4] 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 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()

Changes since v5:
 - remove has_supplies

Changes since v4:
 - add system sleep PM ops
 - move regulator_bulk_disable before suspend()
 - fixes error handling

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

Bibby Hsieh (4):
  dt-binding: eeprom: at24: add vcc-supply property
  dt-binding: i2c: add bus-supply property
  misc: eeprom: at24: support pm_runtime control
  i2c: core: support bus regulator controlling in adapter

 .../devicetree/bindings/eeprom/at24.yaml      |  4 +
 Documentation/devicetree/bindings/i2c/i2c.txt |  3 +
 drivers/i2c/i2c-core-base.c                   | 81 +++++++++++++++++++
 drivers/misc/eeprom/at24.c                    | 40 +++++++++
 include/linux/i2c.h                           |  3 +
 5 files changed, 131 insertions(+)

-- 
2.18.0

[PATCH v10 1/4] dt-binding: eeprom: at24: add vcc-supply property

[Bibby Hsieh]

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

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

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

[PATCH v10 1/4] dt-binding: eeprom: at24: add vcc-supply property

[Bibby Hsieh]

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

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

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

[PATCH v10 2/4] 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

[PATCH v10 2/4] 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

[PATCH v10 3/4] misc: eeprom: at24: support pm_runtime control

[Bibby Hsieh]

Although in the most platforms, the power of eeprom are alway
on, some platforms disable the eeprom power in order to meet
low power request. This patch add the pm_runtime ops to control
power to support all platforms.

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

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538..2fe1b5e0e034 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -22,6 +22,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/gpio/consumer.h>
 
 /* Address pointer is 16 bit. */
@@ -91,6 +92,7 @@ struct at24_data {
 
 	struct gpio_desc *wp_gpio;
 
+	struct regulator *vcc_reg;
 	/*
 	 * Some chips tie up multiple I2C addresses; dummy devices reserve
 	 * them for us, and we'll use them with SMBus calls.
@@ -662,6 +664,10 @@ static int at24_probe(struct i2c_client *client)
 	at24->client[0].client = client;
 	at24->client[0].regmap = regmap;
 
+	at24->vcc_reg = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(at24->vcc_reg))
+		return PTR_ERR(at24->vcc_reg);
+
 	at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
 	if (IS_ERR(at24->wp_gpio))
 		return PTR_ERR(at24->wp_gpio);
@@ -701,6 +707,12 @@ static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
+	err = regulator_enable(at24->vcc_reg);
+	if (err) {
+		dev_err(dev, "Failed to enable vcc regulator\n");
+		return err;
+	}
+
 	/* enable runtime pm */
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -713,6 +725,7 @@ static int at24_probe(struct i2c_client *client)
 	pm_runtime_idle(dev);
 	if (err) {
 		pm_runtime_disable(dev);
+		regulator_disable(at24->vcc_reg);
 		return -ENODEV;
 	}
 
@@ -728,15 +741,42 @@ 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);
+	if (!pm_runtime_status_suspended(&client->dev))
+		regulator_disable(at24->vcc_reg);
 	pm_runtime_set_suspended(&client->dev);
 
 	return 0;
 }
 
+static int __maybe_unused at24_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
+	return regulator_disable(at24->vcc_reg);
+}
+
+static int __maybe_unused at24_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
+	return regulator_enable(at24->vcc_reg);
+}
+
+static const struct dev_pm_ops at24_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
+};
+
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.pm = &at24_pm_ops,
 		.of_match_table = at24_of_match,
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},
-- 
2.18.0

[PATCH v10 3/4] misc: eeprom: at24: support pm_runtime control

[Bibby Hsieh]

Although in the most platforms, the power of eeprom are alway
on, some platforms disable the eeprom power in order to meet
low power request. This patch add the pm_runtime ops to control
power to support all platforms.

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

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 0681d5fdd538..2fe1b5e0e034 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -22,6 +22,7 @@
 #include <linux/nvmem-provider.h>
 #include <linux/regmap.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 #include <linux/gpio/consumer.h>
 
 /* Address pointer is 16 bit. */
@@ -91,6 +92,7 @@ struct at24_data {
 
 	struct gpio_desc *wp_gpio;
 
+	struct regulator *vcc_reg;
 	/*
 	 * Some chips tie up multiple I2C addresses; dummy devices reserve
 	 * them for us, and we'll use them with SMBus calls.
@@ -662,6 +664,10 @@ static int at24_probe(struct i2c_client *client)
 	at24->client[0].client = client;
 	at24->client[0].regmap = regmap;
 
+	at24->vcc_reg = devm_regulator_get(dev, "vcc");
+	if (IS_ERR(at24->vcc_reg))
+		return PTR_ERR(at24->vcc_reg);
+
 	at24->wp_gpio = devm_gpiod_get_optional(dev, "wp", GPIOD_OUT_HIGH);
 	if (IS_ERR(at24->wp_gpio))
 		return PTR_ERR(at24->wp_gpio);
@@ -701,6 +707,12 @@ static int at24_probe(struct i2c_client *client)
 
 	i2c_set_clientdata(client, at24);
 
+	err = regulator_enable(at24->vcc_reg);
+	if (err) {
+		dev_err(dev, "Failed to enable vcc regulator\n");
+		return err;
+	}
+
 	/* enable runtime pm */
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -713,6 +725,7 @@ static int at24_probe(struct i2c_client *client)
 	pm_runtime_idle(dev);
 	if (err) {
 		pm_runtime_disable(dev);
+		regulator_disable(at24->vcc_reg);
 		return -ENODEV;
 	}
 
@@ -728,15 +741,42 @@ 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);
+	if (!pm_runtime_status_suspended(&client->dev))
+		regulator_disable(at24->vcc_reg);
 	pm_runtime_set_suspended(&client->dev);
 
 	return 0;
 }
 
+static int __maybe_unused at24_suspend(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
+	return regulator_disable(at24->vcc_reg);
+}
+
+static int __maybe_unused at24_resume(struct device *dev)
+{
+	struct i2c_client *client = to_i2c_client(dev);
+	struct at24_data *at24 = i2c_get_clientdata(client);
+
+	return regulator_enable(at24->vcc_reg);
+}
+
+static const struct dev_pm_ops at24_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+	SET_RUNTIME_PM_OPS(at24_suspend, at24_resume, NULL)
+};
+
 static struct i2c_driver at24_driver = {
 	.driver = {
 		.name = "at24",
+		.pm = &at24_pm_ops,
 		.of_match_table = at24_of_match,
 		.acpi_match_table = ACPI_PTR(at24_acpi_ids),
 	},
-- 
2.18.0

[PATCH v10 4/4] 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         |  3 ++
 2 files changed, 84 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9333c865d4a9..9b9e96b094ca 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -306,6 +306,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;
 
@@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
 
 	dev_dbg(dev, "probe\n");
 
+	status = regulator_enable(adap->bus_reg);
+	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;
@@ -407,6 +414,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;
 
@@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
 	}
 
 	dev_pm_domain_detach(&client->dev, true);
+	if (!pm_runtime_status_suspended(&adap->dev))
+		regulator_disable(adap->bus_reg);
 
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
 	return status;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int i2c_resume(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	if (pm_runtime_status_suspended(&adap->dev)) {
+		err = regulator_enable(adap->bus_reg);
+		if (err)
+			return err;
+	}
+
+	return pm_generic_resume(dev);
+}
+
+static int i2c_suspend(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	if (!pm_runtime_status_suspended(&adap->dev)) {
+		err = regulator_disable(adap->bus_reg);
+		if (err)
+			return err;
+	}
+
+	return pm_generic_suspend(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_reg);
+	if (err)
+		return err;
+
+	return pm_generic_runtime_resume(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(dev);
+	if (err)
+		return err;
+
+	return regulator_disable(adap->bus_reg);
+}
+#endif
+
+static const struct dev_pm_ops i2c_device_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
+	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);
@@ -488,6 +563,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);
 
@@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
+	if (IS_ERR(adap->bus_reg)) {
+		res = PTR_ERR(adap->bus_reg);
+		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 d2f786706657..833b81a680da 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 */
@@ -330,6 +331,7 @@ struct i2c_client {
 	int init_irq;			/* irq set at initialization	*/
 	int irq;			/* irq issued by device		*/
 	struct list_head detected;
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
 #endif
@@ -723,6 +725,7 @@ struct i2c_adapter {
 	const struct i2c_adapter_quirks *quirks;
 
 	struct irq_domain *host_notify_domain;
+	struct regulator *bus_reg;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
2.18.0

[PATCH v10 4/4] 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         |  3 ++
 2 files changed, 84 insertions(+)

diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
index 9333c865d4a9..9b9e96b094ca 100644
--- a/drivers/i2c/i2c-core-base.c
+++ b/drivers/i2c/i2c-core-base.c
@@ -306,6 +306,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;
 
@@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
 
 	dev_dbg(dev, "probe\n");
 
+	status = regulator_enable(adap->bus_reg);
+	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;
@@ -407,6 +414,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;
 
@@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
 	}
 
 	dev_pm_domain_detach(&client->dev, true);
+	if (!pm_runtime_status_suspended(&adap->dev))
+		regulator_disable(adap->bus_reg);
 
 	dev_pm_clear_wake_irq(&client->dev);
 	device_init_wakeup(&client->dev, false);
@@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
 	return status;
 }
 
+#ifdef CONFIG_PM_SLEEP
+static int i2c_resume(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	if (pm_runtime_status_suspended(&adap->dev)) {
+		err = regulator_enable(adap->bus_reg);
+		if (err)
+			return err;
+	}
+
+	return pm_generic_resume(dev);
+}
+
+static int i2c_suspend(struct device *dev)
+{
+	struct i2c_client *client = i2c_verify_client(dev);
+	struct i2c_adapter *adap = client->adapter;
+	int err;
+
+	if (!pm_runtime_status_suspended(&adap->dev)) {
+		err = regulator_disable(adap->bus_reg);
+		if (err)
+			return err;
+	}
+
+	return pm_generic_suspend(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_reg);
+	if (err)
+		return err;
+
+	return pm_generic_runtime_resume(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(dev);
+	if (err)
+		return err;
+
+	return regulator_disable(adap->bus_reg);
+}
+#endif
+
+static const struct dev_pm_ops i2c_device_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
+	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);
@@ -488,6 +563,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);
 
@@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
+	if (IS_ERR(adap->bus_reg)) {
+		res = PTR_ERR(adap->bus_reg);
+		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 d2f786706657..833b81a680da 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 */
@@ -330,6 +331,7 @@ struct i2c_client {
 	int init_irq;			/* irq set at initialization	*/
 	int irq;			/* irq issued by device		*/
 	struct list_head detected;
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
 #endif
@@ -723,6 +725,7 @@ struct i2c_adapter {
 	const struct i2c_adapter_quirks *quirks;
 
 	struct irq_domain *host_notify_domain;
+	struct regulator *bus_reg;
 };
 #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
 
-- 
2.18.0

Re: [PATCH v10 1/4] dt-binding: eeprom: at24: add vcc-supply property

[Bartosz Golaszewski]

czw., 16 sty 2020 o 03:56 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> In some platforms, they disable the power-supply of eeprom due
> to power consumption reduction. This patch add vcc-supply property.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>
> Reviewed-by: Rob Herring <robh@kernel.org>

Patch applied.

Bartosz

Re: [PATCH v10 1/4] dt-binding: eeprom: at24: add vcc-supply property

[Bartosz Golaszewski]

czw., 16 sty 2020 o 03:56 Bibby Hsieh <bibby.hsieh-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> napisał(a):
>
> In some platforms, they disable the power-supply of eeprom due
> to power consumption reduction. This patch add vcc-supply property.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>

Patch applied.

Bartosz

Re: [PATCH v10 3/4] misc: eeprom: at24: support pm_runtime control

[Bartosz Golaszewski]

czw., 16 sty 2020 o 03:56 Bibby Hsieh <bibby.hsieh@mediatek.com> napisał(a):
>
> Although in the most platforms, the power of eeprom are alway
> on, some platforms disable the eeprom power in order to meet
> low power request. This patch add the pm_runtime ops to control
> power to support all platforms.
>
> Signed-off-by: Bibby Hsieh <bibby.hsieh@mediatek.com>

Patch applied.

Bartosz

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

On Thu, Jan 16, 2020 at 11:56 AM 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         |  3 ++
>  2 files changed, 84 insertions(+)
>

Hi Bibby,

Please see my comments inline.

Hi Wolfram,

Would you have some time to take a look and comment on the general
approach? Thanks.

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9333c865d4a9..9b9e96b094ca 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -306,6 +306,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;
>
> @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
>
>         dev_dbg(dev, "probe\n");
>
> +       status = regulator_enable(adap->bus_reg);
> +       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;
> @@ -407,6 +414,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;
>
> @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
>         }
>
>         dev_pm_domain_detach(&client->dev, true);
> +       if (!pm_runtime_status_suspended(&adap->dev))

Why adap->dev? We control the regulator in bus-level runtime PM
callbacks of the client->dev, right?

> +               regulator_disable(adap->bus_reg);
>
>         dev_pm_clear_wake_irq(&client->dev);
>         device_init_wakeup(&client->dev, false);
> @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
>         return status;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (pm_runtime_status_suspended(&adap->dev)) {

Ditto.

> +               err = regulator_enable(adap->bus_reg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_resume(dev);
> +}
> +
> +static int i2c_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (!pm_runtime_status_suspended(&adap->dev)) {

Ditto.

> +               err = regulator_disable(adap->bus_reg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_suspend(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_reg);
> +       if (err)
> +               return err;
> +
> +       return pm_generic_runtime_resume(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(dev);
> +       if (err)
> +               return err;
> +
> +       return regulator_disable(adap->bus_reg);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> +       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);
> @@ -488,6 +563,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);
>
> @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> +       if (IS_ERR(adap->bus_reg)) {
> +               res = PTR_ERR(adap->bus_reg);
> +               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 d2f786706657..833b81a680da 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 */
> @@ -330,6 +331,7 @@ struct i2c_client {
>         int init_irq;                   /* irq set at initialization    */
>         int irq;                        /* irq issued by device         */
>         struct list_head detected;
> +

nit: Unnecessary change.

Best regards,
Tomasz

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

On Thu, Jan 16, 2020 at 11:56 AM Bibby Hsieh <bibby.hsieh-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  drivers/i2c/i2c-core-base.c | 81 +++++++++++++++++++++++++++++++++++++
>  include/linux/i2c.h         |  3 ++
>  2 files changed, 84 insertions(+)
>

Hi Bibby,

Please see my comments inline.

Hi Wolfram,

Would you have some time to take a look and comment on the general
approach? Thanks.

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9333c865d4a9..9b9e96b094ca 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -306,6 +306,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;
>
> @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
>
>         dev_dbg(dev, "probe\n");
>
> +       status = regulator_enable(adap->bus_reg);
> +       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;
> @@ -407,6 +414,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;
>
> @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
>         }
>
>         dev_pm_domain_detach(&client->dev, true);
> +       if (!pm_runtime_status_suspended(&adap->dev))

Why adap->dev? We control the regulator in bus-level runtime PM
callbacks of the client->dev, right?

> +               regulator_disable(adap->bus_reg);
>
>         dev_pm_clear_wake_irq(&client->dev);
>         device_init_wakeup(&client->dev, false);
> @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
>         return status;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (pm_runtime_status_suspended(&adap->dev)) {

Ditto.

> +               err = regulator_enable(adap->bus_reg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_resume(dev);
> +}
> +
> +static int i2c_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (!pm_runtime_status_suspended(&adap->dev)) {

Ditto.

> +               err = regulator_disable(adap->bus_reg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_suspend(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_reg);
> +       if (err)
> +               return err;
> +
> +       return pm_generic_runtime_resume(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(dev);
> +       if (err)
> +               return err;
> +
> +       return regulator_disable(adap->bus_reg);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> +       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);
> @@ -488,6 +563,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);
>
> @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> +       if (IS_ERR(adap->bus_reg)) {
> +               res = PTR_ERR(adap->bus_reg);
> +               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 d2f786706657..833b81a680da 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 */
> @@ -330,6 +331,7 @@ struct i2c_client {
>         int init_irq;                   /* irq set at initialization    */
>         int irq;                        /* irq issued by device         */
>         struct list_head detected;
> +

nit: Unnecessary change.

Best regards,
Tomasz

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Wolfram Sang]

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

On Thu, Jan 16, 2020 at 10:56:37AM +0800, Bibby Hsieh 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>

...

> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume(struct device *dev)
> +{
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_adapter *adap = client->adapter;
> +	int err;
> +
> +	if (pm_runtime_status_suspended(&adap->dev)) {
> +		err = regulator_enable(adap->bus_reg);
> +		if (err)
> +			return err;
> +	}
> +
> +	return pm_generic_resume(dev);
> +}
> +
> +static int i2c_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_adapter *adap = client->adapter;
> +	int err;
> +
> +	if (!pm_runtime_status_suspended(&adap->dev)) {
> +		err = regulator_disable(adap->bus_reg);
> +		if (err)
> +			return err;
> +	}
> +
> +	return pm_generic_suspend(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_reg);
> +	if (err)
> +		return err;
> +
> +	return pm_generic_runtime_resume(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(dev);
> +	if (err)
> +		return err;
> +
> +	return regulator_disable(adap->bus_reg);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> +	SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
> +};
> +

It looks good to me, yet I am not a PM expert. An ack from someone more
into that topic would be great, if possible.

>  static void i2c_device_shutdown(struct device *dev)
>  {
>  	struct i2c_client *client = i2c_verify_client(dev);
> @@ -488,6 +563,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);
>  
> @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> +	if (IS_ERR(adap->bus_reg)) {
> +		res = PTR_ERR(adap->bus_reg);
> +		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 d2f786706657..833b81a680da 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 */
> @@ -330,6 +331,7 @@ struct i2c_client {
>  	int init_irq;			/* irq set at initialization	*/
>  	int irq;			/* irq issued by device		*/
>  	struct list_head detected;
> +
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
>  #endif
> @@ -723,6 +725,7 @@ struct i2c_adapter {
>  	const struct i2c_adapter_quirks *quirks;
>  
>  	struct irq_domain *host_notify_domain;
> +	struct regulator *bus_reg;

"bus_regulator" please. "reg" is ambigious with "register".

And what Tomasz said, of course.


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

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Wolfram Sang]

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

On Thu, Jan 16, 2020 at 10:56:37AM +0800, Bibby Hsieh 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>

...

> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume(struct device *dev)
> +{
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_adapter *adap = client->adapter;
> +	int err;
> +
> +	if (pm_runtime_status_suspended(&adap->dev)) {
> +		err = regulator_enable(adap->bus_reg);
> +		if (err)
> +			return err;
> +	}
> +
> +	return pm_generic_resume(dev);
> +}
> +
> +static int i2c_suspend(struct device *dev)
> +{
> +	struct i2c_client *client = i2c_verify_client(dev);
> +	struct i2c_adapter *adap = client->adapter;
> +	int err;
> +
> +	if (!pm_runtime_status_suspended(&adap->dev)) {
> +		err = regulator_disable(adap->bus_reg);
> +		if (err)
> +			return err;
> +	}
> +
> +	return pm_generic_suspend(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_reg);
> +	if (err)
> +		return err;
> +
> +	return pm_generic_runtime_resume(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(dev);
> +	if (err)
> +		return err;
> +
> +	return regulator_disable(adap->bus_reg);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> +	SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
> +};
> +

It looks good to me, yet I am not a PM expert. An ack from someone more
into that topic would be great, if possible.

>  static void i2c_device_shutdown(struct device *dev)
>  {
>  	struct i2c_client *client = i2c_verify_client(dev);
> @@ -488,6 +563,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);
>  
> @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> +	if (IS_ERR(adap->bus_reg)) {
> +		res = PTR_ERR(adap->bus_reg);
> +		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 d2f786706657..833b81a680da 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 */
> @@ -330,6 +331,7 @@ struct i2c_client {
>  	int init_irq;			/* irq set at initialization	*/
>  	int irq;			/* irq issued by device		*/
>  	struct list_head detected;
> +
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
>  #endif
> @@ -723,6 +725,7 @@ struct i2c_adapter {
>  	const struct i2c_adapter_quirks *quirks;
>  
>  	struct irq_domain *host_notify_domain;
> +	struct regulator *bus_reg;

"bus_regulator" please. "reg" is ambigious with "register".

And what Tomasz said, of course.


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

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

Hi Rafael,

On Thu, Jan 16, 2020 at 11:56 AM 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         |  3 ++
>  2 files changed, 84 insertions(+)
>

I posted some comments in a separate reply [1] and Wolfram confirmed
that he's fine with the approach [2]. Would you have some time to take
a look from the PM point of view? Thanks.

[1] https://patchwork.ozlabs.org/patch/1223991/#2350984
[2] https://patchwork.ozlabs.org/patch/1223991/#2351032

Please let me know if you want me to CC you directly on any of those replies.

Best regards,
Tomasz

> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> index 9333c865d4a9..9b9e96b094ca 100644
> --- a/drivers/i2c/i2c-core-base.c
> +++ b/drivers/i2c/i2c-core-base.c
> @@ -306,6 +306,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;
>
> @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
>
>         dev_dbg(dev, "probe\n");
>
> +       status = regulator_enable(adap->bus_reg);
> +       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;
> @@ -407,6 +414,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;
>
> @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
>         }
>
>         dev_pm_domain_detach(&client->dev, true);
> +       if (!pm_runtime_status_suspended(&adap->dev))
> +               regulator_disable(adap->bus_reg);
>
>         dev_pm_clear_wake_irq(&client->dev);
>         device_init_wakeup(&client->dev, false);
> @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
>         return status;
>  }
>
> +#ifdef CONFIG_PM_SLEEP
> +static int i2c_resume(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (pm_runtime_status_suspended(&adap->dev)) {
> +               err = regulator_enable(adap->bus_reg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_resume(dev);
> +}
> +
> +static int i2c_suspend(struct device *dev)
> +{
> +       struct i2c_client *client = i2c_verify_client(dev);
> +       struct i2c_adapter *adap = client->adapter;
> +       int err;
> +
> +       if (!pm_runtime_status_suspended(&adap->dev)) {
> +               err = regulator_disable(adap->bus_reg);
> +               if (err)
> +                       return err;
> +       }
> +
> +       return pm_generic_suspend(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_reg);
> +       if (err)
> +               return err;
> +
> +       return pm_generic_runtime_resume(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(dev);
> +       if (err)
> +               return err;
> +
> +       return regulator_disable(adap->bus_reg);
> +}
> +#endif
> +
> +static const struct dev_pm_ops i2c_device_pm = {
> +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> +       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);
> @@ -488,6 +563,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);
>
> @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> +       if (IS_ERR(adap->bus_reg)) {
> +               res = PTR_ERR(adap->bus_reg);
> +               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 d2f786706657..833b81a680da 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 */
> @@ -330,6 +331,7 @@ struct i2c_client {
>         int init_irq;                   /* irq set at initialization    */
>         int irq;                        /* irq issued by device         */
>         struct list_head detected;
> +
>  #if IS_ENABLED(CONFIG_I2C_SLAVE)
>         i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
>  #endif
> @@ -723,6 +725,7 @@ struct i2c_adapter {
>         const struct i2c_adapter_quirks *quirks;
>
>         struct irq_domain *host_notify_domain;
> +       struct regulator *bus_reg;
>  };
>  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>
> --
> 2.18.0

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

On Tue, Jan 28, 2020 at 3:36 PM Wolfram Sang <wsa@the-dreams.de> wrote:
>
> On Thu, Jan 16, 2020 at 10:56:37AM +0800, Bibby Hsieh 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>
>
> ...
>
> > +#ifdef CONFIG_PM_SLEEP
> > +static int i2c_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = i2c_verify_client(dev);
> > +     struct i2c_adapter *adap = client->adapter;
> > +     int err;
> > +
> > +     if (pm_runtime_status_suspended(&adap->dev)) {
> > +             err = regulator_enable(adap->bus_reg);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     return pm_generic_resume(dev);
> > +}
> > +
> > +static int i2c_suspend(struct device *dev)
> > +{
> > +     struct i2c_client *client = i2c_verify_client(dev);
> > +     struct i2c_adapter *adap = client->adapter;
> > +     int err;
> > +
> > +     if (!pm_runtime_status_suspended(&adap->dev)) {
> > +             err = regulator_disable(adap->bus_reg);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     return pm_generic_suspend(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_reg);
> > +     if (err)
> > +             return err;
> > +
> > +     return pm_generic_runtime_resume(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(dev);
> > +     if (err)
> > +             return err;
> > +
> > +     return regulator_disable(adap->bus_reg);
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops i2c_device_pm = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> > +     SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
> > +};
> > +
>
> It looks good to me, yet I am not a PM expert. An ack from someone more
> into that topic would be great, if possible.
>
> >  static void i2c_device_shutdown(struct device *dev)
> >  {
> >       struct i2c_client *client = i2c_verify_client(dev);
> > @@ -488,6 +563,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);
> >
> > @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> > +     if (IS_ERR(adap->bus_reg)) {
> > +             res = PTR_ERR(adap->bus_reg);
> > +             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 d2f786706657..833b81a680da 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 */
> > @@ -330,6 +331,7 @@ struct i2c_client {
> >       int init_irq;                   /* irq set at initialization    */
> >       int irq;                        /* irq issued by device         */
> >       struct list_head detected;
> > +
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >       i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> >  #endif
> > @@ -723,6 +725,7 @@ struct i2c_adapter {
> >       const struct i2c_adapter_quirks *quirks;
> >
> >       struct irq_domain *host_notify_domain;
> > +     struct regulator *bus_reg;
>
> "bus_regulator" please. "reg" is ambigious with "register".
>
> And what Tomasz said, of course.
>

Thanks a lot.

I've added Rafael and linux-pm in another reply quoting the whole patch.

Best regards,
Tomasz

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

On Tue, Jan 28, 2020 at 3:36 PM Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>
> On Thu, Jan 16, 2020 at 10:56:37AM +0800, Bibby Hsieh 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
>
> ...
>
> > +#ifdef CONFIG_PM_SLEEP
> > +static int i2c_resume(struct device *dev)
> > +{
> > +     struct i2c_client *client = i2c_verify_client(dev);
> > +     struct i2c_adapter *adap = client->adapter;
> > +     int err;
> > +
> > +     if (pm_runtime_status_suspended(&adap->dev)) {
> > +             err = regulator_enable(adap->bus_reg);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     return pm_generic_resume(dev);
> > +}
> > +
> > +static int i2c_suspend(struct device *dev)
> > +{
> > +     struct i2c_client *client = i2c_verify_client(dev);
> > +     struct i2c_adapter *adap = client->adapter;
> > +     int err;
> > +
> > +     if (!pm_runtime_status_suspended(&adap->dev)) {
> > +             err = regulator_disable(adap->bus_reg);
> > +             if (err)
> > +                     return err;
> > +     }
> > +
> > +     return pm_generic_suspend(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_reg);
> > +     if (err)
> > +             return err;
> > +
> > +     return pm_generic_runtime_resume(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(dev);
> > +     if (err)
> > +             return err;
> > +
> > +     return regulator_disable(adap->bus_reg);
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops i2c_device_pm = {
> > +     SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> > +     SET_RUNTIME_PM_OPS(i2c_runtime_suspend, i2c_runtime_resume, NULL)
> > +};
> > +
>
> It looks good to me, yet I am not a PM expert. An ack from someone more
> into that topic would be great, if possible.
>
> >  static void i2c_device_shutdown(struct device *dev)
> >  {
> >       struct i2c_client *client = i2c_verify_client(dev);
> > @@ -488,6 +563,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);
> >
> > @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> > +     if (IS_ERR(adap->bus_reg)) {
> > +             res = PTR_ERR(adap->bus_reg);
> > +             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 d2f786706657..833b81a680da 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 */
> > @@ -330,6 +331,7 @@ struct i2c_client {
> >       int init_irq;                   /* irq set at initialization    */
> >       int irq;                        /* irq issued by device         */
> >       struct list_head detected;
> > +
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >       i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> >  #endif
> > @@ -723,6 +725,7 @@ struct i2c_adapter {
> >       const struct i2c_adapter_quirks *quirks;
> >
> >       struct irq_domain *host_notify_domain;
> > +     struct regulator *bus_reg;
>
> "bus_regulator" please. "reg" is ambigious with "register".
>
> And what Tomasz said, of course.
>

Thanks a lot.

I've added Rafael and linux-pm in another reply quoting the whole patch.

Best regards,
Tomasz

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

Hi Rafael,

On Tue, Jan 28, 2020 at 4:31 PM Tomasz Figa <tfiga@chromium.org> wrote:
>
> Hi Rafael,
>
> On Thu, Jan 16, 2020 at 11:56 AM 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         |  3 ++
> >  2 files changed, 84 insertions(+)
> >
>
> I posted some comments in a separate reply [1] and Wolfram confirmed
> that he's fine with the approach [2]. Would you have some time to take
> a look from the PM point of view? Thanks.
>
> [1] https://patchwork.ozlabs.org/patch/1223991/#2350984
> [2] https://patchwork.ozlabs.org/patch/1223991/#2351032
>
> Please let me know if you want me to CC you directly on any of those replies.

Would you have a few minutes to take a look at this?

Thanks,
Tomasz

>
> Best regards,
> Tomasz
>
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 9333c865d4a9..9b9e96b094ca 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -306,6 +306,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;
> >
> > @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
> >
> >         dev_dbg(dev, "probe\n");
> >
> > +       status = regulator_enable(adap->bus_reg);
> > +       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;
> > @@ -407,6 +414,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;
> >
> > @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
> >         }
> >
> >         dev_pm_domain_detach(&client->dev, true);
> > +       if (!pm_runtime_status_suspended(&adap->dev))
> > +               regulator_disable(adap->bus_reg);
> >
> >         dev_pm_clear_wake_irq(&client->dev);
> >         device_init_wakeup(&client->dev, false);
> > @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
> >         return status;
> >  }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int i2c_resume(struct device *dev)
> > +{
> > +       struct i2c_client *client = i2c_verify_client(dev);
> > +       struct i2c_adapter *adap = client->adapter;
> > +       int err;
> > +
> > +       if (pm_runtime_status_suspended(&adap->dev)) {
> > +               err = regulator_enable(adap->bus_reg);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       return pm_generic_resume(dev);
> > +}
> > +
> > +static int i2c_suspend(struct device *dev)
> > +{
> > +       struct i2c_client *client = i2c_verify_client(dev);
> > +       struct i2c_adapter *adap = client->adapter;
> > +       int err;
> > +
> > +       if (!pm_runtime_status_suspended(&adap->dev)) {
> > +               err = regulator_disable(adap->bus_reg);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       return pm_generic_suspend(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_reg);
> > +       if (err)
> > +               return err;
> > +
> > +       return pm_generic_runtime_resume(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(dev);
> > +       if (err)
> > +               return err;
> > +
> > +       return regulator_disable(adap->bus_reg);
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops i2c_device_pm = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> > +       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);
> > @@ -488,6 +563,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);
> >
> > @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> > +       if (IS_ERR(adap->bus_reg)) {
> > +               res = PTR_ERR(adap->bus_reg);
> > +               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 d2f786706657..833b81a680da 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 */
> > @@ -330,6 +331,7 @@ struct i2c_client {
> >         int init_irq;                   /* irq set at initialization    */
> >         int irq;                        /* irq issued by device         */
> >         struct list_head detected;
> > +
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >         i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> >  #endif
> > @@ -723,6 +725,7 @@ struct i2c_adapter {
> >         const struct i2c_adapter_quirks *quirks;
> >
> >         struct irq_domain *host_notify_domain;
> > +       struct regulator *bus_reg;
> >  };
> >  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
> >
> > --
> > 2.18.0

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Tomasz Figa]

Hi Rafael,

On Tue, Jan 28, 2020 at 4:31 PM Tomasz Figa <tfiga-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>
> Hi Rafael,
>
> On Thu, Jan 16, 2020 at 11:56 AM Bibby Hsieh <bibby.hsieh-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org> 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> > ---
> >  drivers/i2c/i2c-core-base.c | 81 +++++++++++++++++++++++++++++++++++++
> >  include/linux/i2c.h         |  3 ++
> >  2 files changed, 84 insertions(+)
> >
>
> I posted some comments in a separate reply [1] and Wolfram confirmed
> that he's fine with the approach [2]. Would you have some time to take
> a look from the PM point of view? Thanks.
>
> [1] https://patchwork.ozlabs.org/patch/1223991/#2350984
> [2] https://patchwork.ozlabs.org/patch/1223991/#2351032
>
> Please let me know if you want me to CC you directly on any of those replies.

Would you have a few minutes to take a look at this?

Thanks,
Tomasz

>
> Best regards,
> Tomasz
>
> > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > index 9333c865d4a9..9b9e96b094ca 100644
> > --- a/drivers/i2c/i2c-core-base.c
> > +++ b/drivers/i2c/i2c-core-base.c
> > @@ -306,6 +306,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;
> >
> > @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
> >
> >         dev_dbg(dev, "probe\n");
> >
> > +       status = regulator_enable(adap->bus_reg);
> > +       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;
> > @@ -407,6 +414,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;
> >
> > @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
> >         }
> >
> >         dev_pm_domain_detach(&client->dev, true);
> > +       if (!pm_runtime_status_suspended(&adap->dev))
> > +               regulator_disable(adap->bus_reg);
> >
> >         dev_pm_clear_wake_irq(&client->dev);
> >         device_init_wakeup(&client->dev, false);
> > @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
> >         return status;
> >  }
> >
> > +#ifdef CONFIG_PM_SLEEP
> > +static int i2c_resume(struct device *dev)
> > +{
> > +       struct i2c_client *client = i2c_verify_client(dev);
> > +       struct i2c_adapter *adap = client->adapter;
> > +       int err;
> > +
> > +       if (pm_runtime_status_suspended(&adap->dev)) {
> > +               err = regulator_enable(adap->bus_reg);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       return pm_generic_resume(dev);
> > +}
> > +
> > +static int i2c_suspend(struct device *dev)
> > +{
> > +       struct i2c_client *client = i2c_verify_client(dev);
> > +       struct i2c_adapter *adap = client->adapter;
> > +       int err;
> > +
> > +       if (!pm_runtime_status_suspended(&adap->dev)) {
> > +               err = regulator_disable(adap->bus_reg);
> > +               if (err)
> > +                       return err;
> > +       }
> > +
> > +       return pm_generic_suspend(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_reg);
> > +       if (err)
> > +               return err;
> > +
> > +       return pm_generic_runtime_resume(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(dev);
> > +       if (err)
> > +               return err;
> > +
> > +       return regulator_disable(adap->bus_reg);
> > +}
> > +#endif
> > +
> > +static const struct dev_pm_ops i2c_device_pm = {
> > +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> > +       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);
> > @@ -488,6 +563,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);
> >
> > @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> > +       if (IS_ERR(adap->bus_reg)) {
> > +               res = PTR_ERR(adap->bus_reg);
> > +               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 d2f786706657..833b81a680da 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 */
> > @@ -330,6 +331,7 @@ struct i2c_client {
> >         int init_irq;                   /* irq set at initialization    */
> >         int irq;                        /* irq issued by device         */
> >         struct list_head detected;
> > +
> >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> >         i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> >  #endif
> > @@ -723,6 +725,7 @@ struct i2c_adapter {
> >         const struct i2c_adapter_quirks *quirks;
> >
> >         struct irq_domain *host_notify_domain;
> > +       struct regulator *bus_reg;
> >  };
> >  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
> >
> > --
> > 2.18.0

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Bibby Hsieh]

Hi, Rafael,

I'm Sorry to bother you.

I'm not sure if you receive our email...
So I re-sent this email to your another email.

Thanks.

Bibby

On Mon, 2020-02-17 at 18:45 +0900, Tomasz Figa wrote:
> Hi Rafael,
> 
> On Tue, Jan 28, 2020 at 4:31 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Rafael,
> >
> > On Thu, Jan 16, 2020 at 11:56 AM 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         |  3 ++
> > >  2 files changed, 84 insertions(+)
> > >
> >
> > I posted some comments in a separate reply [1] and Wolfram confirmed
> > that he's fine with the approach [2]. Would you have some time to take
> > a look from the PM point of view? Thanks.
> >
> > [1] https://patchwork.ozlabs.org/patch/1223991/#2350984
> > [2] https://patchwork.ozlabs.org/patch/1223991/#2351032
> >
> > Please let me know if you want me to CC you directly on any of those replies.
> 
> Would you have a few minutes to take a look at this?
> 
> Thanks,
> Tomasz
> 
> >
> > Best regards,
> > Tomasz
> >
> > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > > index 9333c865d4a9..9b9e96b094ca 100644
> > > --- a/drivers/i2c/i2c-core-base.c
> > > +++ b/drivers/i2c/i2c-core-base.c
> > > @@ -306,6 +306,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;
> > >
> > > @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
> > >
> > >         dev_dbg(dev, "probe\n");
> > >
> > > +       status = regulator_enable(adap->bus_reg);
> > > +       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;
> > > @@ -407,6 +414,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;
> > >
> > > @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
> > >         }
> > >
> > >         dev_pm_domain_detach(&client->dev, true);
> > > +       if (!pm_runtime_status_suspended(&adap->dev))
> > > +               regulator_disable(adap->bus_reg);
> > >
> > >         dev_pm_clear_wake_irq(&client->dev);
> > >         device_init_wakeup(&client->dev, false);
> > > @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
> > >         return status;
> > >  }
> > >
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int i2c_resume(struct device *dev)
> > > +{
> > > +       struct i2c_client *client = i2c_verify_client(dev);
> > > +       struct i2c_adapter *adap = client->adapter;
> > > +       int err;
> > > +
> > > +       if (pm_runtime_status_suspended(&adap->dev)) {
> > > +               err = regulator_enable(adap->bus_reg);
> > > +               if (err)
> > > +                       return err;
> > > +       }
> > > +
> > > +       return pm_generic_resume(dev);
> > > +}
> > > +
> > > +static int i2c_suspend(struct device *dev)
> > > +{
> > > +       struct i2c_client *client = i2c_verify_client(dev);
> > > +       struct i2c_adapter *adap = client->adapter;
> > > +       int err;
> > > +
> > > +       if (!pm_runtime_status_suspended(&adap->dev)) {
> > > +               err = regulator_disable(adap->bus_reg);
> > > +               if (err)
> > > +                       return err;
> > > +       }
> > > +
> > > +       return pm_generic_suspend(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_reg);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       return pm_generic_runtime_resume(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(dev);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       return regulator_disable(adap->bus_reg);
> > > +}
> > > +#endif
> > > +
> > > +static const struct dev_pm_ops i2c_device_pm = {
> > > +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> > > +       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);
> > > @@ -488,6 +563,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);
> > >
> > > @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> > > +       if (IS_ERR(adap->bus_reg)) {
> > > +               res = PTR_ERR(adap->bus_reg);
> > > +               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 d2f786706657..833b81a680da 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 */
> > > @@ -330,6 +331,7 @@ struct i2c_client {
> > >         int init_irq;                   /* irq set at initialization    */
> > >         int irq;                        /* irq issued by device         */
> > >         struct list_head detected;
> > > +
> > >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > >         i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> > >  #endif
> > > @@ -723,6 +725,7 @@ struct i2c_adapter {
> > >         const struct i2c_adapter_quirks *quirks;
> > >
> > >         struct irq_domain *host_notify_domain;
> > > +       struct regulator *bus_reg;
> > >  };
> > >  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
> > >
> > > --
> > > 2.18.0

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Bibby Hsieh]

Hi, Rafael,

I'm Sorry to bother you.

I'm not sure if you receive our email...
So I re-sent this email to your another email.

Thanks.

Bibby

On Mon, 2020-02-17 at 18:45 +0900, Tomasz Figa wrote:
> Hi Rafael,
> 
> On Tue, Jan 28, 2020 at 4:31 PM Tomasz Figa <tfiga@chromium.org> wrote:
> >
> > Hi Rafael,
> >
> > On Thu, Jan 16, 2020 at 11:56 AM 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         |  3 ++
> > >  2 files changed, 84 insertions(+)
> > >
> >
> > I posted some comments in a separate reply [1] and Wolfram confirmed
> > that he's fine with the approach [2]. Would you have some time to take
> > a look from the PM point of view? Thanks.
> >
> > [1] https://patchwork.ozlabs.org/patch/1223991/#2350984
> > [2] https://patchwork.ozlabs.org/patch/1223991/#2351032
> >
> > Please let me know if you want me to CC you directly on any of those replies.
> 
> Would you have a few minutes to take a look at this?
> 
> Thanks,
> Tomasz
> 
> >
> > Best regards,
> > Tomasz
> >
> > > diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
> > > index 9333c865d4a9..9b9e96b094ca 100644
> > > --- a/drivers/i2c/i2c-core-base.c
> > > +++ b/drivers/i2c/i2c-core-base.c
> > > @@ -306,6 +306,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;
> > >
> > > @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
> > >
> > >         dev_dbg(dev, "probe\n");
> > >
> > > +       status = regulator_enable(adap->bus_reg);
> > > +       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;
> > > @@ -407,6 +414,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;
> > >
> > > @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
> > >         }
> > >
> > >         dev_pm_domain_detach(&client->dev, true);
> > > +       if (!pm_runtime_status_suspended(&adap->dev))
> > > +               regulator_disable(adap->bus_reg);
> > >
> > >         dev_pm_clear_wake_irq(&client->dev);
> > >         device_init_wakeup(&client->dev, false);
> > > @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
> > >         return status;
> > >  }
> > >
> > > +#ifdef CONFIG_PM_SLEEP
> > > +static int i2c_resume(struct device *dev)
> > > +{
> > > +       struct i2c_client *client = i2c_verify_client(dev);
> > > +       struct i2c_adapter *adap = client->adapter;
> > > +       int err;
> > > +
> > > +       if (pm_runtime_status_suspended(&adap->dev)) {
> > > +               err = regulator_enable(adap->bus_reg);
> > > +               if (err)
> > > +                       return err;
> > > +       }
> > > +
> > > +       return pm_generic_resume(dev);
> > > +}
> > > +
> > > +static int i2c_suspend(struct device *dev)
> > > +{
> > > +       struct i2c_client *client = i2c_verify_client(dev);
> > > +       struct i2c_adapter *adap = client->adapter;
> > > +       int err;
> > > +
> > > +       if (!pm_runtime_status_suspended(&adap->dev)) {
> > > +               err = regulator_disable(adap->bus_reg);
> > > +               if (err)
> > > +                       return err;
> > > +       }
> > > +
> > > +       return pm_generic_suspend(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_reg);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       return pm_generic_runtime_resume(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(dev);
> > > +       if (err)
> > > +               return err;
> > > +
> > > +       return regulator_disable(adap->bus_reg);
> > > +}
> > > +#endif
> > > +
> > > +static const struct dev_pm_ops i2c_device_pm = {
> > > +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
> > > +       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);
> > > @@ -488,6 +563,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);
> > >
> > > @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
> > > +       if (IS_ERR(adap->bus_reg)) {
> > > +               res = PTR_ERR(adap->bus_reg);
> > > +               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 d2f786706657..833b81a680da 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 */
> > > @@ -330,6 +331,7 @@ struct i2c_client {
> > >         int init_irq;                   /* irq set at initialization    */
> > >         int irq;                        /* irq issued by device         */
> > >         struct list_head detected;
> > > +
> > >  #if IS_ENABLED(CONFIG_I2C_SLAVE)
> > >         i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
> > >  #endif
> > > @@ -723,6 +725,7 @@ struct i2c_adapter {
> > >         const struct i2c_adapter_quirks *quirks;
> > >
> > >         struct irq_domain *host_notify_domain;
> > > +       struct regulator *bus_reg;
> > >  };
> > >  #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
> > >
> > > --
> > > 2.18.0

Re: [PATCH v10 4/4] i2c: core: support bus regulator controlling in adapter

[Rafael J. Wysocki]

On 2/21/2020 4:22 AM, Bibby Hsieh wrote:
> Hi, Rafael,
>
> I'm Sorry to bother you.
>
> I'm not sure if you receive our email...
> So I re-sent this email to your another email.

I actually have not realized that there was a question directly for me 
in this thread.


> On Mon, 2020-02-17 at 18:45 +0900, Tomasz Figa wrote:
>> Hi Rafael,
>>
>> On Tue, Jan 28, 2020 at 4:31 PM Tomasz Figa <tfiga@chromium.org> wrote:
>>> Hi Rafael,
>>>
>>> On Thu, Jan 16, 2020 at 11:56 AM 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         |  3 ++
>>>>   2 files changed, 84 insertions(+)
>>>>
>>> I posted some comments in a separate reply [1] and Wolfram confirmed
>>> that he's fine with the approach [2]. Would you have some time to take
>>> a look from the PM point of view? Thanks.
>>>
>>> [1] https://patchwork.ozlabs.org/patch/1223991/#2350984
>>> [2] https://patchwork.ozlabs.org/patch/1223991/#2351032
>>>
>>> Please let me know if you want me to CC you directly on any of those replies.
>> Would you have a few minutes to take a look at this?
>>
>> Thanks,
>> Tomasz
>>
>>> Best regards,
>>> Tomasz
>>>
>>>> diff --git a/drivers/i2c/i2c-core-base.c b/drivers/i2c/i2c-core-base.c
>>>> index 9333c865d4a9..9b9e96b094ca 100644
>>>> --- a/drivers/i2c/i2c-core-base.c
>>>> +++ b/drivers/i2c/i2c-core-base.c
>>>> @@ -306,6 +306,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;
>>>>
>>>> @@ -371,6 +372,12 @@ static int i2c_device_probe(struct device *dev)
>>>>
>>>>          dev_dbg(dev, "probe\n");
>>>>
>>>> +       status = regulator_enable(adap->bus_reg);
>>>> +       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;
>>>> @@ -407,6 +414,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;
>>>>
>>>> @@ -420,6 +428,8 @@ static int i2c_device_remove(struct device *dev)
>>>>          }
>>>>
>>>>          dev_pm_domain_detach(&client->dev, true);
>>>> +       if (!pm_runtime_status_suspended(&adap->dev))
>>>> +               regulator_disable(adap->bus_reg);
>>>>
>>>>          dev_pm_clear_wake_irq(&client->dev);
>>>>          device_init_wakeup(&client->dev, false);
>>>> @@ -431,6 +441,71 @@ static int i2c_device_remove(struct device *dev)
>>>>          return status;
>>>>   }
>>>>
>>>> +#ifdef CONFIG_PM_SLEEP
>>>> +static int i2c_resume(struct device *dev)
>>>> +{
>>>> +       struct i2c_client *client = i2c_verify_client(dev);
>>>> +       struct i2c_adapter *adap = client->adapter;
>>>> +       int err;
>>>> +
>>>> +       if (pm_runtime_status_suspended(&adap->dev)) {

In general, this is racy, because the RPM status of the device may 
change while it is running unless there are specific reasons why that 
cannot happen.

It generally is better to do such checks at the "early resume" and 
(analogously) "late suspend" stages when RPM is disabled.

>>>> +               err = regulator_enable(adap->bus_reg);
>>>> +               if (err)
>>>> +                       return err;
>>>> +       }
>>>> +
>>>> +       return pm_generic_resume(dev);
>>>> +}
>>>> +
>>>> +static int i2c_suspend(struct device *dev)
>>>> +{
>>>> +       struct i2c_client *client = i2c_verify_client(dev);
>>>> +       struct i2c_adapter *adap = client->adapter;
>>>> +       int err;
>>>> +
>>>> +       if (!pm_runtime_status_suspended(&adap->dev)) {
>>>> +               err = regulator_disable(adap->bus_reg);
>>>> +               if (err)
>>>> +                       return err;
>>>> +       }
>>>> +
>>>> +       return pm_generic_suspend(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_reg);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       return pm_generic_runtime_resume(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(dev);
>>>> +       if (err)
>>>> +               return err;
>>>> +
>>>> +       return regulator_disable(adap->bus_reg);
>>>> +}
>>>> +#endif
>>>> +
>>>> +static const struct dev_pm_ops i2c_device_pm = {
>>>> +       SET_SYSTEM_SLEEP_PM_OPS(i2c_suspend, i2c_resume)
>>>> +       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);
>>>> @@ -488,6 +563,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);
>>>>
>>>> @@ -1351,6 +1427,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_reg = devm_regulator_get(&adap->dev, "bus");
>>>> +       if (IS_ERR(adap->bus_reg)) {
>>>> +               res = PTR_ERR(adap->bus_reg);
>>>> +               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 d2f786706657..833b81a680da 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 */
>>>> @@ -330,6 +331,7 @@ struct i2c_client {
>>>>          int init_irq;                   /* irq set at initialization    */
>>>>          int irq;                        /* irq issued by device         */
>>>>          struct list_head detected;
>>>> +
>>>>   #if IS_ENABLED(CONFIG_I2C_SLAVE)
>>>>          i2c_slave_cb_t slave_cb;        /* callback for slave mode      */
>>>>   #endif
>>>> @@ -723,6 +725,7 @@ struct i2c_adapter {
>>>>          const struct i2c_adapter_quirks *quirks;
>>>>
>>>>          struct irq_domain *host_notify_domain;
>>>> +       struct regulator *bus_reg;
>>>>   };
>>>>   #define to_i2c_adapter(d) container_of(d, struct i2c_adapter, dev)
>>>>
>>>> --
>>>> 2.18.0