[PATCH 0/9] i2c: qcom-cci: fixes and updates

[PATCH 0/9] i2c: qcom-cci: fixes and updates

[Vladimir Zapolskiy]

The main intention of the patch series is to add support of vbus
regulators, which are commonly connected to CCI I2C busses.

The new bus adapter specific bus_regulator from commit 5a7b95fb993e
("i2c: core: support bus regulator controlling in adapter") is reused,
however its control is connected to runtime pm of the I2C master
controller rather than runtime pm of slaves.

In addition the series adds new compatible value for CCI found on QCOM
SM8450 SoC.

Vladimir Zapolskiy (9):
  dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible
  dt-bindings: i2c: qcom-cci: add description of a vbus-supply property
  i2c: qcom-cci: don't delete an unregistered adapter
  i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()
  i2c: qcom-cci: initialize CCI controller after registration of adapters
  i2c: qcom-cci: simplify probe by removing one loop over busses
  i2c: qcom-cci: simplify access to bus data structure
  i2c: qcom-cci: add support of optional vbus-supply regulators
  i2c: qcom-cci: add sm8450 compatible

 .../devicetree/bindings/i2c/i2c-qcom-cci.txt  |   9 +-
 drivers/i2c/busses/i2c-qcom-cci.c             | 159 ++++++++++++------
 2 files changed, 114 insertions(+), 54 deletions(-)

-- 
2.33.0

[PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Vladimir Zapolskiy]

The change adds QCOM SM8450 compatible value to the list of QCOM CCI
controller compatibles, the controller found on the SoC is equal to
the ones found on previous SoC generations.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
index 7b9fc0c22eaf..924ad8c03464 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -10,6 +10,7 @@ PROPERTIES:
 		"qcom,msm8996-cci"
 		"qcom,sdm845-cci"
 		"qcom,sm8250-cci"
+		"qcom,sm8450-cci"
 
 - reg
 	Usage: required
@@ -43,7 +44,8 @@ PROPERTIES:
 SUBNODES:
 
 The CCI provides I2C masters for one (msm8916) or two i2c busses (msm8996,
-sdm845 and sm8250), described as subdevices named "i2c-bus@0" and "i2c-bus@1".
+sdm845, sm8250 and sm8450), described as subdevices named "i2c-bus@0" and
+"i2c-bus@1".
 
 PROPERTIES:
 
-- 
2.33.0

[PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Vladimir Zapolskiy]

Quite regularly I2C bus lines on QCOM CCI controller require an external
pull-up to a regulator powered line, to be able to define all such
cases an additional vbus-supply property of a bus subnode is wanted.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
index 924ad8c03464..9f5b321748f1 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
@@ -60,6 +60,11 @@ PROPERTIES:
 	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
 		    kHz if omitted.
 
+- vbus-supply:
+	Usage: optional
+	Value type: phandle
+	Definition: Regulator that provides power to SCL/SDA lines
+
 Example:
 
 	cci@a0c000 {
-- 
2.33.0

[PATCH 3/9] i2c: qcom-cci: don't delete an unregistered adapter

[Vladimir Zapolskiy]

If i2c_add_adapter() fails to add an I2C adapter found on QCOM CCI
controller, on error path i2c_del_adapter() is still called.

Fortunately there is a sanity check in the I2C core, so the only
visible implication is a printed debug level message:

    i2c-core: attempting to delete unregistered adapter [Qualcomm-CCI]

Nevertheless it would be reasonable to correct the probe error path.

Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index c1de8eb66169..fd4260d18577 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -655,7 +655,7 @@ static int cci_probe(struct platform_device *pdev)
 	return 0;
 
 error_i2c:
-	for (; i >= 0; i--) {
+	for (--i ; i >= 0; i--) {
 		if (cci->master[i].cci)
 			i2c_del_adapter(&cci->master[i].adap);
 	}
-- 
2.33.0

[PATCH 4/9] i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()

[Vladimir Zapolskiy]

There is a minor chance for a race, if a pointer to an i2c-bus subnode
is stored and then reused after releasing its reference, and it would
be sufficient to get one more reference under a loop over children
subnodes.

Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index fd4260d18577..cf54f1cb4c57 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -558,7 +558,7 @@ static int cci_probe(struct platform_device *pdev)
 		cci->master[idx].adap.quirks = &cci->data->quirks;
 		cci->master[idx].adap.algo = &cci_algo;
 		cci->master[idx].adap.dev.parent = dev;
-		cci->master[idx].adap.dev.of_node = child;
+		cci->master[idx].adap.dev.of_node = of_node_get(child);
 		cci->master[idx].master = idx;
 		cci->master[idx].cci = cci;
 
@@ -643,8 +643,10 @@ static int cci_probe(struct platform_device *pdev)
 			continue;
 
 		ret = i2c_add_adapter(&cci->master[i].adap);
-		if (ret < 0)
+		if (ret < 0) {
+			of_node_put(cci->master[i].adap.dev.of_node);
 			goto error_i2c;
+		}
 	}
 
 	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
@@ -656,8 +658,10 @@ static int cci_probe(struct platform_device *pdev)
 
 error_i2c:
 	for (--i ; i >= 0; i--) {
-		if (cci->master[i].cci)
+		if (cci->master[i].cci) {
 			i2c_del_adapter(&cci->master[i].adap);
+			of_node_put(cci->master[i].adap.dev.of_node);
+		}
 	}
 error:
 	disable_irq(cci->irq);
@@ -673,8 +677,10 @@ static int cci_remove(struct platform_device *pdev)
 	int i;
 
 	for (i = 0; i < cci->data->num_masters; i++) {
-		if (cci->master[i].cci)
+		if (cci->master[i].cci) {
 			i2c_del_adapter(&cci->master[i].adap);
+			of_node_put(cci->master[i].adap.dev.of_node);
+		}
 		cci_halt(cci, i);
 	}
 
-- 
2.33.0

[PATCH 5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

[Vladimir Zapolskiy]

The change is wanted to postpone initialization of busses on CCI controller
by cci_init() and cci_reset() till adapters are registered, the later is
needed for adding I2C bus devices and get correspondent vbus regulators.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index cf54f1cb4c57..eebf9603d3d1 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
 	val = readl(cci->base + CCI_HW_VERSION);
 	dev_dbg(dev, "CCI HW version = 0x%08x", val);
 
-	ret = cci_reset(cci);
-	if (ret < 0)
-		goto error;
-
-	ret = cci_init(cci);
-	if (ret < 0)
-		goto error;
-
 	for (i = 0; i < cci->data->num_masters; i++) {
 		if (!cci->master[i].cci)
 			continue;
@@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = cci_reset(cci);
+	if (ret < 0)
+		goto error_i2c;
+
+	ret = cci_init(cci);
+	if (ret < 0)
+		goto error_i2c;
+
 	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
 	pm_runtime_use_autosuspend(dev);
 	pm_runtime_set_active(dev);
@@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
 			of_node_put(cci->master[i].adap.dev.of_node);
 		}
 	}
-error:
 	disable_irq(cci->irq);
 disable_clocks:
 	cci_disable_clocks(cci);
-- 
2.33.0

[PATCH 6/9] i2c: qcom-cci: simplify probe by removing one loop over busses

[Vladimir Zapolskiy]

It's possible to slightly simplify cci_probe() function by merging
a loop over I2C busses found on CCI controller with a loop which
actually registers I2C adapters, the data per I2C bus collected early
on probe is not used before calling i2c_add_adapter() since cci_reset()
and cci_init() calls were moved to the end of probe.

The change is intended to be non-functional.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 82 +++++++++++++++----------------
 1 file changed, 39 insertions(+), 43 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index eebf9603d3d1..cffc01b2285b 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -540,44 +540,6 @@ static int cci_probe(struct platform_device *pdev)
 	if (!cci->data)
 		return -ENOENT;
 
-	for_each_available_child_of_node(dev->of_node, child) {
-		u32 idx;
-
-		ret = of_property_read_u32(child, "reg", &idx);
-		if (ret) {
-			dev_err(dev, "%pOF invalid 'reg' property", child);
-			continue;
-		}
-
-		if (idx >= cci->data->num_masters) {
-			dev_err(dev, "%pOF invalid 'reg' value: %u (max is %u)",
-				child, idx, cci->data->num_masters - 1);
-			continue;
-		}
-
-		cci->master[idx].adap.quirks = &cci->data->quirks;
-		cci->master[idx].adap.algo = &cci_algo;
-		cci->master[idx].adap.dev.parent = dev;
-		cci->master[idx].adap.dev.of_node = of_node_get(child);
-		cci->master[idx].master = idx;
-		cci->master[idx].cci = cci;
-
-		i2c_set_adapdata(&cci->master[idx].adap, &cci->master[idx]);
-		snprintf(cci->master[idx].adap.name,
-			 sizeof(cci->master[idx].adap.name), "Qualcomm-CCI");
-
-		cci->master[idx].mode = I2C_MODE_STANDARD;
-		ret = of_property_read_u32(child, "clock-frequency", &val);
-		if (!ret) {
-			if (val == I2C_MAX_FAST_MODE_FREQ)
-				cci->master[idx].mode = I2C_MODE_FAST;
-			else if (val == I2C_MAX_FAST_MODE_PLUS_FREQ)
-				cci->master[idx].mode = I2C_MODE_FAST_PLUS;
-		}
-
-		init_completion(&cci->master[idx].irq_complete);
-	}
-
 	/* Memory */
 
 	r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -630,13 +592,47 @@ static int cci_probe(struct platform_device *pdev)
 	val = readl(cci->base + CCI_HW_VERSION);
 	dev_dbg(dev, "CCI HW version = 0x%08x", val);
 
-	for (i = 0; i < cci->data->num_masters; i++) {
-		if (!cci->master[i].cci)
+	for_each_available_child_of_node(dev->of_node, child) {
+		u32 idx;
+
+		ret = of_property_read_u32(child, "reg", &idx);
+		if (ret) {
+			dev_err(dev, "%pOF invalid 'reg' property", child);
 			continue;
+		}
+
+		if (idx >= cci->data->num_masters) {
+			dev_err(dev, "%pOF invalid 'reg' value: %u (max is %u)",
+				child, idx, cci->data->num_masters - 1);
+			continue;
+		}
+
+		cci->master[idx].adap.quirks = &cci->data->quirks;
+		cci->master[idx].adap.algo = &cci_algo;
+		cci->master[idx].adap.dev.parent = dev;
+		cci->master[idx].adap.dev.of_node = of_node_get(child);
+		cci->master[idx].master = idx;
+		cci->master[idx].cci = cci;
+
+		i2c_set_adapdata(&cci->master[idx].adap, &cci->master[idx]);
+		snprintf(cci->master[idx].adap.name,
+			 sizeof(cci->master[idx].adap.name), "Qualcomm-CCI");
 
-		ret = i2c_add_adapter(&cci->master[i].adap);
+		cci->master[idx].mode = I2C_MODE_STANDARD;
+		ret = of_property_read_u32(child, "clock-frequency", &val);
+		if (!ret) {
+			if (val == I2C_MAX_FAST_MODE_FREQ)
+				cci->master[idx].mode = I2C_MODE_FAST;
+			else if (val == I2C_MAX_FAST_MODE_PLUS_FREQ)
+				cci->master[idx].mode = I2C_MODE_FAST_PLUS;
+		}
+
+		init_completion(&cci->master[idx].irq_complete);
+
+		ret = i2c_add_adapter(&cci->master[idx].adap);
 		if (ret < 0) {
-			of_node_put(cci->master[i].adap.dev.of_node);
+			of_node_put(child);
+			cci->master[idx].cci = NULL;
 			goto error_i2c;
 		}
 	}
@@ -657,7 +653,7 @@ static int cci_probe(struct platform_device *pdev)
 	return 0;
 
 error_i2c:
-	for (--i ; i >= 0; i--) {
+	for (i = 0; i < cci->data->num_masters; i++) {
 		if (cci->master[i].cci) {
 			i2c_del_adapter(&cci->master[i].adap);
 			of_node_put(cci->master[i].adap.dev.of_node);
-- 
2.33.0

[PATCH 7/9] i2c: qcom-cci: simplify access to bus data structure

[Vladimir Zapolskiy]

Trivial non-functional change, which adds an alias to a widely
used data location.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 32 ++++++++++++++++---------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index cffc01b2285b..775945f7b4cd 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -593,6 +593,7 @@ static int cci_probe(struct platform_device *pdev)
 	dev_dbg(dev, "CCI HW version = 0x%08x", val);
 
 	for_each_available_child_of_node(dev->of_node, child) {
+		struct cci_master *master;
 		u32 idx;
 
 		ret = of_property_read_u32(child, "reg", &idx);
@@ -607,32 +608,33 @@ static int cci_probe(struct platform_device *pdev)
 			continue;
 		}
 
-		cci->master[idx].adap.quirks = &cci->data->quirks;
-		cci->master[idx].adap.algo = &cci_algo;
-		cci->master[idx].adap.dev.parent = dev;
-		cci->master[idx].adap.dev.of_node = of_node_get(child);
-		cci->master[idx].master = idx;
-		cci->master[idx].cci = cci;
+		master = &cci->master[idx];
+		master->adap.quirks = &cci->data->quirks;
+		master->adap.algo = &cci_algo;
+		master->adap.dev.parent = dev;
+		master->adap.dev.of_node = of_node_get(child);
+		master->master = idx;
+		master->cci = cci;
 
-		i2c_set_adapdata(&cci->master[idx].adap, &cci->master[idx]);
-		snprintf(cci->master[idx].adap.name,
-			 sizeof(cci->master[idx].adap.name), "Qualcomm-CCI");
+		i2c_set_adapdata(&master->adap, master);
+		snprintf(master->adap.name,
+			 sizeof(master->adap.name), "Qualcomm-CCI");
 
-		cci->master[idx].mode = I2C_MODE_STANDARD;
+		master->mode = I2C_MODE_STANDARD;
 		ret = of_property_read_u32(child, "clock-frequency", &val);
 		if (!ret) {
 			if (val == I2C_MAX_FAST_MODE_FREQ)
-				cci->master[idx].mode = I2C_MODE_FAST;
+				master->mode = I2C_MODE_FAST;
 			else if (val == I2C_MAX_FAST_MODE_PLUS_FREQ)
-				cci->master[idx].mode = I2C_MODE_FAST_PLUS;
+				master->mode = I2C_MODE_FAST_PLUS;
 		}
 
-		init_completion(&cci->master[idx].irq_complete);
+		init_completion(&master->irq_complete);
 
-		ret = i2c_add_adapter(&cci->master[idx].adap);
+		ret = i2c_add_adapter(&master->adap);
 		if (ret < 0) {
 			of_node_put(child);
-			cci->master[idx].cci = NULL;
+			master->cci = NULL;
 			goto error_i2c;
 		}
 	}
-- 
2.33.0

[PATCH 8/9] i2c: qcom-cci: add support of optional vbus-supply regulators

[Vladimir Zapolskiy]

The change adds handling of optional vbus regulators in the driver.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 775945f7b4cd..2fc7f1f2616f 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/regulator/consumer.h>
 
 #define CCI_HW_VERSION				0x0
 #define CCI_RESET_CMD				0x004
@@ -480,6 +481,20 @@ static void cci_disable_clocks(struct cci *cci)
 static int __maybe_unused cci_suspend_runtime(struct device *dev)
 {
 	struct cci *cci = dev_get_drvdata(dev);
+	struct regulator *bus_regulator;
+	unsigned int i;
+
+	for (i = 0; i < cci->data->num_masters; i++) {
+		if (!cci->master[i].cci)
+			continue;
+
+		bus_regulator = cci->master[i].adap.bus_regulator;
+		if (!bus_regulator)
+			continue;
+
+		if (regulator_is_enabled(bus_regulator) > 0)
+			regulator_disable(bus_regulator);
+	}
 
 	cci_disable_clocks(cci);
 	return 0;
@@ -488,12 +503,30 @@ static int __maybe_unused cci_suspend_runtime(struct device *dev)
 static int __maybe_unused cci_resume_runtime(struct device *dev)
 {
 	struct cci *cci = dev_get_drvdata(dev);
+	struct regulator *bus_regulator;
+	unsigned int i;
 	int ret;
 
 	ret = cci_enable_clocks(cci);
 	if (ret)
 		return ret;
 
+	for (i = 0; i < cci->data->num_masters; i++) {
+		if (!cci->master[i].cci)
+			continue;
+
+		bus_regulator = cci->master[i].adap.bus_regulator;
+		if (!bus_regulator)
+			continue;
+
+		if (!regulator_is_enabled(bus_regulator)) {
+			ret = regulator_enable(bus_regulator);
+			if (ret)
+				dev_err(dev, "failed to enable regulator: %d\n",
+					ret);
+		}
+	}
+
 	cci_init(cci);
 	return 0;
 }
@@ -593,6 +626,7 @@ static int cci_probe(struct platform_device *pdev)
 	dev_dbg(dev, "CCI HW version = 0x%08x", val);
 
 	for_each_available_child_of_node(dev->of_node, child) {
+		struct regulator *bus_regulator;
 		struct cci_master *master;
 		u32 idx;
 
@@ -637,6 +671,21 @@ static int cci_probe(struct platform_device *pdev)
 			master->cci = NULL;
 			goto error_i2c;
 		}
+
+		/*
+		 * It might be possible to find an optional vbus supply, but
+		 * it requires to pass the registration of an I2C adapter
+		 * device and its association with a bus device tree node.
+		 */
+		bus_regulator = devm_regulator_get_optional(&master->adap.dev,
+							    "vbus");
+		if (IS_ERR(bus_regulator)) {
+			ret = PTR_ERR(bus_regulator);
+			if (ret == -EPROBE_DEFER)
+				goto error_i2c;
+			bus_regulator = NULL;
+		}
+		master->adap.bus_regulator = bus_regulator;
 	}
 
 	ret = cci_reset(cci);
-- 
2.33.0

[PATCH 9/9] i2c: qcom-cci: add sm8450 compatible

[Vladimir Zapolskiy]

Add QCOM SM8450 specific compatible for CCI controller, which is
equal to CCI controllers found on QCOM SDM845 and QCOM SM8250 SoCs.

Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
---
 drivers/i2c/busses/i2c-qcom-cci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
index 2fc7f1f2616f..e625857fde41 100644
--- a/drivers/i2c/busses/i2c-qcom-cci.c
+++ b/drivers/i2c/busses/i2c-qcom-cci.c
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0
 // Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
-// Copyright (c) 2017-20 Linaro Limited.
+// Copyright (c) 2017-2022 Linaro Limited.
 
 #include <linux/clk.h>
 #include <linux/completion.h>
@@ -822,6 +822,7 @@ static const struct of_device_id cci_dt_match[] = {
 	{ .compatible = "qcom,msm8996-cci", .data = &cci_v2_data},
 	{ .compatible = "qcom,sdm845-cci", .data = &cci_v2_data},
 	{ .compatible = "qcom,sm8250-cci", .data = &cci_v2_data},
+	{ .compatible = "qcom,sm8450-cci", .data = &cci_v2_data},
 	{}
 };
 MODULE_DEVICE_TABLE(of, cci_dt_match);
-- 
2.33.0

Re: [PATCH 5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

[Loic Poulain]

Hi Vladimir,

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> The change is wanted to postpone initialization of busses on CCI controller
> by cci_init() and cci_reset() till adapters are registered, the later is
> needed for adding I2C bus devices and get correspondent vbus regulators.

This is odd, I don't think it's a good idea to register an adapter
which is not yet initialized. Could you elaborate on why you need to
do this, if you can't access the controller without this regulator
enabled, maybe it is more than vbus supply, and, in that case, it
should be enabled from your probe function.

Regards,
Loic





>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index cf54f1cb4c57..eebf9603d3d1 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>         val = readl(cci->base + CCI_HW_VERSION);
>         dev_dbg(dev, "CCI HW version = 0x%08x", val);
>
> -       ret = cci_reset(cci);
> -       if (ret < 0)
> -               goto error;
> -
> -       ret = cci_init(cci);
> -       if (ret < 0)
> -               goto error;
> -
>         for (i = 0; i < cci->data->num_masters; i++) {
>                 if (!cci->master[i].cci)
>                         continue;
> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>                 }
>         }
>
> +       ret = cci_reset(cci);
> +       if (ret < 0)
> +               goto error_i2c;
> +
> +       ret = cci_init(cci);
> +       if (ret < 0)
> +               goto error_i2c;
> +
>         pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>         pm_runtime_use_autosuspend(dev);
>         pm_runtime_set_active(dev);
> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>                         of_node_put(cci->master[i].adap.dev.of_node);
>                 }
>         }
> -error:
>         disable_irq(cci->irq);
>  disable_clocks:
>         cci_disable_clocks(cci);
> --
> 2.33.0
>

Re: [PATCH 5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

[Vladimir Zapolskiy]

Hi Loic,

On 2/3/22 7:29 PM, Loic Poulain wrote:
> Hi Vladimir,
> 
> On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
>>
>> The change is wanted to postpone initialization of busses on CCI controller
>> by cci_init() and cci_reset() till adapters are registered, the later is
>> needed for adding I2C bus devices and get correspondent vbus regulators.
> 
> This is odd, I don't think it's a good idea to register an adapter
> which is not yet initialized. Could you elaborate on why you need to
> do this, if you can't access the controller without this regulator
> enabled, maybe it is more than vbus supply, and, in that case, it
> should be enabled from your probe function.

thank you for review, the controller can be accessed without a vbus regulator,
but I2C devices connected to the master controller can not.

The registration of a master controller device done by i2c_add_adapter()
should be safe to defer IMO, because there shall be no communication on
the bus at this point, there are no slaves before probe completion, thus
cci_init()/cci_reset() can be safely called afterwards.

The rationale of the change is to merge two loops over busses, see change 6/9,
keeping two loops extremely complicates the proper resource management.

--
Best wishes,
Vladimir

>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
>> index cf54f1cb4c57..eebf9603d3d1 100644
>> --- a/drivers/i2c/busses/i2c-qcom-cci.c
>> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
>> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>>          val = readl(cci->base + CCI_HW_VERSION);
>>          dev_dbg(dev, "CCI HW version = 0x%08x", val);
>>
>> -       ret = cci_reset(cci);
>> -       if (ret < 0)
>> -               goto error;
>> -
>> -       ret = cci_init(cci);
>> -       if (ret < 0)
>> -               goto error;
>> -
>>          for (i = 0; i < cci->data->num_masters; i++) {
>>                  if (!cci->master[i].cci)
>>                          continue;
>> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>>                  }
>>          }
>>
>> +       ret = cci_reset(cci);
>> +       if (ret < 0)
>> +               goto error_i2c;
>> +
>> +       ret = cci_init(cci);
>> +       if (ret < 0)
>> +               goto error_i2c;
>> +
>>          pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>>          pm_runtime_use_autosuspend(dev);
>>          pm_runtime_set_active(dev);
>> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>>                          of_node_put(cci->master[i].adap.dev.of_node);
>>                  }
>>          }
>> -error:
>>          disable_irq(cci->irq);
>>   disable_clocks:
>>          cci_disable_clocks(cci);
>> --
>> 2.33.0
>>

Re: [PATCH 3/9] i2c: qcom-cci: don't delete an unregistered adapter

[Robert Foss]

Hey Vladimir,

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> If i2c_add_adapter() fails to add an I2C adapter found on QCOM CCI
> controller, on error path i2c_del_adapter() is still called.
>
> Fortunately there is a sanity check in the I2C core, so the only
> visible implication is a printed debug level message:
>
>     i2c-core: attempting to delete unregistered adapter [Qualcomm-CCI]
>
> Nevertheless it would be reasonable to correct the probe error path.
>
> Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index c1de8eb66169..fd4260d18577 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -655,7 +655,7 @@ static int cci_probe(struct platform_device *pdev)
>         return 0;
>
>  error_i2c:
> -       for (; i >= 0; i--) {
> +       for (--i ; i >= 0; i--) {
>                 if (cci->master[i].cci)
>                         i2c_del_adapter(&cci->master[i].adap);
>         }
> --
> 2.33.0
>


This chunk of code seems to be re-worked later in the series too. But
explicitly fixing the issue still makes sense in this case.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 4/9] i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()

[Robert Foss]

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> There is a minor chance for a race, if a pointer to an i2c-bus subnode
> is stored and then reused after releasing its reference, and it would
> be sufficient to get one more reference under a loop over children
> subnodes.
>
> Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index fd4260d18577..cf54f1cb4c57 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -558,7 +558,7 @@ static int cci_probe(struct platform_device *pdev)
>                 cci->master[idx].adap.quirks = &cci->data->quirks;
>                 cci->master[idx].adap.algo = &cci_algo;
>                 cci->master[idx].adap.dev.parent = dev;
> -               cci->master[idx].adap.dev.of_node = child;
> +               cci->master[idx].adap.dev.of_node = of_node_get(child);
>                 cci->master[idx].master = idx;
>                 cci->master[idx].cci = cci;
>
> @@ -643,8 +643,10 @@ static int cci_probe(struct platform_device *pdev)
>                         continue;
>
>                 ret = i2c_add_adapter(&cci->master[i].adap);
> -               if (ret < 0)
> +               if (ret < 0) {
> +                       of_node_put(cci->master[i].adap.dev.of_node);
>                         goto error_i2c;
> +               }
>         }
>
>         pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> @@ -656,8 +658,10 @@ static int cci_probe(struct platform_device *pdev)
>
>  error_i2c:
>         for (--i ; i >= 0; i--) {
> -               if (cci->master[i].cci)
> +               if (cci->master[i].cci) {
>                         i2c_del_adapter(&cci->master[i].adap);
> +                       of_node_put(cci->master[i].adap.dev.of_node);
> +               }
>         }
>  error:
>         disable_irq(cci->irq);
> @@ -673,8 +677,10 @@ static int cci_remove(struct platform_device *pdev)
>         int i;
>
>         for (i = 0; i < cci->data->num_masters; i++) {
> -               if (cci->master[i].cci)
> +               if (cci->master[i].cci) {
>                         i2c_del_adapter(&cci->master[i].adap);
> +                       of_node_put(cci->master[i].adap.dev.of_node);
> +               }
>                 cci_halt(cci, i);
>         }
>
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 6/9] i2c: qcom-cci: simplify probe by removing one loop over busses

[Robert Foss]

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> It's possible to slightly simplify cci_probe() function by merging
> a loop over I2C busses found on CCI controller with a loop which
> actually registers I2C adapters, the data per I2C bus collected early
> on probe is not used before calling i2c_add_adapter() since cci_reset()
> and cci_init() calls were moved to the end of probe.
>
> The change is intended to be non-functional.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 82 +++++++++++++++----------------
>  1 file changed, 39 insertions(+), 43 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index eebf9603d3d1..cffc01b2285b 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -540,44 +540,6 @@ static int cci_probe(struct platform_device *pdev)
>         if (!cci->data)
>                 return -ENOENT;
>
> -       for_each_available_child_of_node(dev->of_node, child) {
> -               u32 idx;
> -
> -               ret = of_property_read_u32(child, "reg", &idx);
> -               if (ret) {
> -                       dev_err(dev, "%pOF invalid 'reg' property", child);
> -                       continue;
> -               }
> -
> -               if (idx >= cci->data->num_masters) {
> -                       dev_err(dev, "%pOF invalid 'reg' value: %u (max is %u)",
> -                               child, idx, cci->data->num_masters - 1);
> -                       continue;
> -               }
> -
> -               cci->master[idx].adap.quirks = &cci->data->quirks;
> -               cci->master[idx].adap.algo = &cci_algo;
> -               cci->master[idx].adap.dev.parent = dev;
> -               cci->master[idx].adap.dev.of_node = of_node_get(child);
> -               cci->master[idx].master = idx;
> -               cci->master[idx].cci = cci;
> -
> -               i2c_set_adapdata(&cci->master[idx].adap, &cci->master[idx]);
> -               snprintf(cci->master[idx].adap.name,
> -                        sizeof(cci->master[idx].adap.name), "Qualcomm-CCI");
> -
> -               cci->master[idx].mode = I2C_MODE_STANDARD;
> -               ret = of_property_read_u32(child, "clock-frequency", &val);
> -               if (!ret) {
> -                       if (val == I2C_MAX_FAST_MODE_FREQ)
> -                               cci->master[idx].mode = I2C_MODE_FAST;
> -                       else if (val == I2C_MAX_FAST_MODE_PLUS_FREQ)
> -                               cci->master[idx].mode = I2C_MODE_FAST_PLUS;
> -               }
> -
> -               init_completion(&cci->master[idx].irq_complete);
> -       }
> -
>         /* Memory */
>
>         r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -630,13 +592,47 @@ static int cci_probe(struct platform_device *pdev)
>         val = readl(cci->base + CCI_HW_VERSION);
>         dev_dbg(dev, "CCI HW version = 0x%08x", val);
>
> -       for (i = 0; i < cci->data->num_masters; i++) {
> -               if (!cci->master[i].cci)
> +       for_each_available_child_of_node(dev->of_node, child) {
> +               u32 idx;
> +
> +               ret = of_property_read_u32(child, "reg", &idx);
> +               if (ret) {
> +                       dev_err(dev, "%pOF invalid 'reg' property", child);
>                         continue;
> +               }
> +
> +               if (idx >= cci->data->num_masters) {
> +                       dev_err(dev, "%pOF invalid 'reg' value: %u (max is %u)",
> +                               child, idx, cci->data->num_masters - 1);
> +                       continue;
> +               }
> +
> +               cci->master[idx].adap.quirks = &cci->data->quirks;
> +               cci->master[idx].adap.algo = &cci_algo;
> +               cci->master[idx].adap.dev.parent = dev;
> +               cci->master[idx].adap.dev.of_node = of_node_get(child);
> +               cci->master[idx].master = idx;
> +               cci->master[idx].cci = cci;
> +
> +               i2c_set_adapdata(&cci->master[idx].adap, &cci->master[idx]);
> +               snprintf(cci->master[idx].adap.name,
> +                        sizeof(cci->master[idx].adap.name), "Qualcomm-CCI");
>
> -               ret = i2c_add_adapter(&cci->master[i].adap);
> +               cci->master[idx].mode = I2C_MODE_STANDARD;
> +               ret = of_property_read_u32(child, "clock-frequency", &val);
> +               if (!ret) {
> +                       if (val == I2C_MAX_FAST_MODE_FREQ)
> +                               cci->master[idx].mode = I2C_MODE_FAST;
> +                       else if (val == I2C_MAX_FAST_MODE_PLUS_FREQ)
> +                               cci->master[idx].mode = I2C_MODE_FAST_PLUS;
> +               }
> +
> +               init_completion(&cci->master[idx].irq_complete);
> +
> +               ret = i2c_add_adapter(&cci->master[idx].adap);
>                 if (ret < 0) {
> -                       of_node_put(cci->master[i].adap.dev.of_node);
> +                       of_node_put(child);
> +                       cci->master[idx].cci = NULL;
>                         goto error_i2c;
>                 }
>         }
> @@ -657,7 +653,7 @@ static int cci_probe(struct platform_device *pdev)
>         return 0;
>
>  error_i2c:
> -       for (--i ; i >= 0; i--) {
> +       for (i = 0; i < cci->data->num_masters; i++) {
>                 if (cci->master[i].cci) {
>                         i2c_del_adapter(&cci->master[i].adap);
>                         of_node_put(cci->master[i].adap.dev.of_node);
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 7/9] i2c: qcom-cci: simplify access to bus data structure

[Robert Foss]

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Trivial non-functional change, which adds an alias to a widely
> used data location.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 32 ++++++++++++++++---------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index cffc01b2285b..775945f7b4cd 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -593,6 +593,7 @@ static int cci_probe(struct platform_device *pdev)
>         dev_dbg(dev, "CCI HW version = 0x%08x", val);
>
>         for_each_available_child_of_node(dev->of_node, child) {
> +               struct cci_master *master;
>                 u32 idx;
>
>                 ret = of_property_read_u32(child, "reg", &idx);
> @@ -607,32 +608,33 @@ static int cci_probe(struct platform_device *pdev)
>                         continue;
>                 }
>
> -               cci->master[idx].adap.quirks = &cci->data->quirks;
> -               cci->master[idx].adap.algo = &cci_algo;
> -               cci->master[idx].adap.dev.parent = dev;
> -               cci->master[idx].adap.dev.of_node = of_node_get(child);
> -               cci->master[idx].master = idx;
> -               cci->master[idx].cci = cci;
> +               master = &cci->master[idx];
> +               master->adap.quirks = &cci->data->quirks;
> +               master->adap.algo = &cci_algo;
> +               master->adap.dev.parent = dev;
> +               master->adap.dev.of_node = of_node_get(child);
> +               master->master = idx;
> +               master->cci = cci;
>
> -               i2c_set_adapdata(&cci->master[idx].adap, &cci->master[idx]);
> -               snprintf(cci->master[idx].adap.name,
> -                        sizeof(cci->master[idx].adap.name), "Qualcomm-CCI");
> +               i2c_set_adapdata(&master->adap, master);
> +               snprintf(master->adap.name,
> +                        sizeof(master->adap.name), "Qualcomm-CCI");
>
> -               cci->master[idx].mode = I2C_MODE_STANDARD;
> +               master->mode = I2C_MODE_STANDARD;
>                 ret = of_property_read_u32(child, "clock-frequency", &val);
>                 if (!ret) {
>                         if (val == I2C_MAX_FAST_MODE_FREQ)
> -                               cci->master[idx].mode = I2C_MODE_FAST;
> +                               master->mode = I2C_MODE_FAST;
>                         else if (val == I2C_MAX_FAST_MODE_PLUS_FREQ)
> -                               cci->master[idx].mode = I2C_MODE_FAST_PLUS;
> +                               master->mode = I2C_MODE_FAST_PLUS;
>                 }
>
> -               init_completion(&cci->master[idx].irq_complete);
> +               init_completion(&master->irq_complete);
>
> -               ret = i2c_add_adapter(&cci->master[idx].adap);
> +               ret = i2c_add_adapter(&master->adap);
>                 if (ret < 0) {
>                         of_node_put(child);
> -                       cci->master[idx].cci = NULL;
> +                       master->cci = NULL;
>                         goto error_i2c;
>                 }
>         }
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 8/9] i2c: qcom-cci: add support of optional vbus-supply regulators

[Robert Foss]

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> The change adds handling of optional vbus regulators in the driver.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 775945f7b4cd..2fc7f1f2616f 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>
>  #define CCI_HW_VERSION                         0x0
>  #define CCI_RESET_CMD                          0x004
> @@ -480,6 +481,20 @@ static void cci_disable_clocks(struct cci *cci)
>  static int __maybe_unused cci_suspend_runtime(struct device *dev)
>  {
>         struct cci *cci = dev_get_drvdata(dev);
> +       struct regulator *bus_regulator;
> +       unsigned int i;
> +
> +       for (i = 0; i < cci->data->num_masters; i++) {
> +               if (!cci->master[i].cci)
> +                       continue;
> +
> +               bus_regulator = cci->master[i].adap.bus_regulator;
> +               if (!bus_regulator)
> +                       continue;
> +
> +               if (regulator_is_enabled(bus_regulator) > 0)

Is this check needed? No matter the current status of the regulator,
we'd like to disable it, and from my reading regulator_disable can be
called for already disabled regulators.

> +                       regulator_disable(bus_regulator);
> +       }
>
>         cci_disable_clocks(cci);
>         return 0;
> @@ -488,12 +503,30 @@ static int __maybe_unused cci_suspend_runtime(struct device *dev)
>  static int __maybe_unused cci_resume_runtime(struct device *dev)
>  {
>         struct cci *cci = dev_get_drvdata(dev);
> +       struct regulator *bus_regulator;
> +       unsigned int i;
>         int ret;
>
>         ret = cci_enable_clocks(cci);
>         if (ret)
>                 return ret;
>
> +       for (i = 0; i < cci->data->num_masters; i++) {
> +               if (!cci->master[i].cci)
> +                       continue;
> +
> +               bus_regulator = cci->master[i].adap.bus_regulator;
> +               if (!bus_regulator)
> +                       continue;
> +
> +               if (!regulator_is_enabled(bus_regulator)) {
> +                       ret = regulator_enable(bus_regulator);
> +                       if (ret)
> +                               dev_err(dev, "failed to enable regulator: %d\n",
> +                                       ret);
> +               }
> +       }
> +
>         cci_init(cci);
>         return 0;
>  }
> @@ -593,6 +626,7 @@ static int cci_probe(struct platform_device *pdev)
>         dev_dbg(dev, "CCI HW version = 0x%08x", val);
>
>         for_each_available_child_of_node(dev->of_node, child) {
> +               struct regulator *bus_regulator;
>                 struct cci_master *master;
>                 u32 idx;
>
> @@ -637,6 +671,21 @@ static int cci_probe(struct platform_device *pdev)
>                         master->cci = NULL;
>                         goto error_i2c;
>                 }
> +
> +               /*
> +                * It might be possible to find an optional vbus supply, but
> +                * it requires to pass the registration of an I2C adapter
> +                * device and its association with a bus device tree node.
> +                */
> +               bus_regulator = devm_regulator_get_optional(&master->adap.dev,
> +                                                           "vbus");
> +               if (IS_ERR(bus_regulator)) {
> +                       ret = PTR_ERR(bus_regulator);
> +                       if (ret == -EPROBE_DEFER)
> +                               goto error_i2c;
> +                       bus_regulator = NULL;
> +               }
> +               master->adap.bus_regulator = bus_regulator;
>         }
>
>         ret = cci_reset(cci);
> --
> 2.33.0
>

With the above nit sorted, feel free to add my r-b.

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 9/9] i2c: qcom-cci: add sm8450 compatible

[Robert Foss]

On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Add QCOM SM8450 specific compatible for CCI controller, which is
> equal to CCI controllers found on QCOM SDM845 and QCOM SM8250 SoCs.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 2fc7f1f2616f..e625857fde41 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -1,6 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0
>  // Copyright (c) 2012-2016, The Linux Foundation. All rights reserved.
> -// Copyright (c) 2017-20 Linaro Limited.
> +// Copyright (c) 2017-2022 Linaro Limited.
>
>  #include <linux/clk.h>
>  #include <linux/completion.h>
> @@ -822,6 +822,7 @@ static const struct of_device_id cci_dt_match[] = {
>         { .compatible = "qcom,msm8996-cci", .data = &cci_v2_data},
>         { .compatible = "qcom,sdm845-cci", .data = &cci_v2_data},
>         { .compatible = "qcom,sm8250-cci", .data = &cci_v2_data},
> +       { .compatible = "qcom,sm8450-cci", .data = &cci_v2_data},
>         {}
>  };
>  MODULE_DEVICE_TABLE(of, cci_dt_match);
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Robert Foss]

On Thu, 3 Feb 2022 at 17:46, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> The change adds QCOM SM8450 compatible value to the list of QCOM CCI
> controller compatibles, the controller found on the SoC is equal to
> the ones found on previous SoC generations.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 7b9fc0c22eaf..924ad8c03464 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -10,6 +10,7 @@ PROPERTIES:
>                 "qcom,msm8996-cci"
>                 "qcom,sdm845-cci"
>                 "qcom,sm8250-cci"
> +               "qcom,sm8450-cci"
>
>  - reg
>         Usage: required
> @@ -43,7 +44,8 @@ PROPERTIES:
>  SUBNODES:
>
>  The CCI provides I2C masters for one (msm8916) or two i2c busses (msm8996,
> -sdm845 and sm8250), described as subdevices named "i2c-bus@0" and "i2c-bus@1".
> +sdm845, sm8250 and sm8450), described as subdevices named "i2c-bus@0" and
> +"i2c-bus@1".
>
>  PROPERTIES:
>
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Robert Foss]

On Thu, 3 Feb 2022 at 17:46, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Quite regularly I2C bus lines on QCOM CCI controller require an external
> pull-up to a regulator powered line, to be able to define all such
> cases an additional vbus-supply property of a bus subnode is wanted.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 924ad8c03464..9f5b321748f1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -60,6 +60,11 @@ PROPERTIES:
>         Definition: Desired I2C bus clock frequency in Hz, defaults to 100
>                     kHz if omitted.
>
> +- vbus-supply:
> +       Usage: optional
> +       Value type: phandle
> +       Definition: Regulator that provides power to SCL/SDA lines
> +
>  Example:
>
>         cci@a0c000 {
> --
> 2.33.0
>

Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

[Loic Poulain]

On Thu, 3 Feb 2022 at 19:45, Vladimir Zapolskiy
<vladimir.zapolskiy@linaro.org> wrote:
>
> Hi Loic,
>
> On 2/3/22 7:29 PM, Loic Poulain wrote:
> > Hi Vladimir,
> >
> > On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> > <vladimir.zapolskiy@linaro.org> wrote:
> >>
> >> The change is wanted to postpone initialization of busses on CCI controller
> >> by cci_init() and cci_reset() till adapters are registered, the later is
> >> needed for adding I2C bus devices and get correspondent vbus regulators.
> >
> > This is odd, I don't think it's a good idea to register an adapter
> > which is not yet initialized. Could you elaborate on why you need to
> > do this, if you can't access the controller without this regulator
> > enabled, maybe it is more than vbus supply, and, in that case, it
> > should be enabled from your probe function.
>
> thank you for review, the controller can be accessed without a vbus regulator,
> but I2C devices connected to the master controller can not.
>
> The registration of a master controller device done by i2c_add_adapter()
> should be safe to defer IMO, because there shall be no communication on
> the bus at this point, there are no slaves before probe completion, thus
> cci_init()/cci_reset() can be safely called afterwards.
>
> The rationale of the change is to merge two loops over busses, see change 6/9,
> keeping two loops extremely complicates the proper resource management.

OK, I see, I'm sure it works, but I still think that registering the
adapter should be the last step, without making assumptions on when
the i2c core is going to use it. Maybe the point here is the initial
bad design/implementation of cci_reset and cci_init.

cci_reset() is a global reset and then should not depend on subnode
initialization in order to be executed early in the probe.You can e.g
add an 'irq_complete' completion to the cci struct. The CCI_IRQ_MASK_0
bit should probably be added here as well.

cci_init() should be subnode/master specific (cci_init_master) so that
you call it in the registering loop, updating master specific timings
and irq-mask bits.

Thoughs?

Regards,
Loic

Re: [PATCH 8/9] i2c: qcom-cci: add support of optional vbus-supply regulators

[Loic Poulain]

On Fri, 4 Feb 2022 at 12:03, Robert Foss <robert.foss@linaro.org> wrote:
>
> On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> <vladimir.zapolskiy@linaro.org> wrote:
> >
> > The change adds handling of optional vbus regulators in the driver.
> >
> > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> > ---
> >  drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++
> >  1 file changed, 49 insertions(+)
> >
> > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > index 775945f7b4cd..2fc7f1f2616f 100644
> > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm_runtime.h>
> > +#include <linux/regulator/consumer.h>
> >
> >  #define CCI_HW_VERSION                         0x0
> >  #define CCI_RESET_CMD                          0x004
> > @@ -480,6 +481,20 @@ static void cci_disable_clocks(struct cci *cci)
> >  static int __maybe_unused cci_suspend_runtime(struct device *dev)
> >  {
> >         struct cci *cci = dev_get_drvdata(dev);
> > +       struct regulator *bus_regulator;
> > +       unsigned int i;
> > +
> > +       for (i = 0; i < cci->data->num_masters; i++) {
> > +               if (!cci->master[i].cci)
> > +                       continue;
> > +
> > +               bus_regulator = cci->master[i].adap.bus_regulator;
> > +               if (!bus_regulator)
> > +                       continue;
> > +
> > +               if (regulator_is_enabled(bus_regulator) > 0)
>
> Is this check needed? No matter the current status of the regulator,
> we'd like to disable it, and from my reading regulator_disable can be
> called for already disabled regulators.

+1, but why do we even assign this regulator to each adapter, a
simpler solution would be to have the regulator part of the cci
struct, and simply disable/enable it on runtime suspend/resume,
without extra loop/check. I2C core does nothing with
adap.bus_regulator anyway (5a7b95fb993e has been partially reverted).

>
> > +                       regulator_disable(bus_regulator);
> > +       }
> >
> >         cci_disable_clocks(cci);
> >         return 0;
> > @@ -488,12 +503,30 @@ static int __maybe_unused cci_suspend_runtime(struct device *dev)
> >  static int __maybe_unused cci_resume_runtime(struct device *dev)
> >  {
> >         struct cci *cci = dev_get_drvdata(dev);
> > +       struct regulator *bus_regulator;
> > +       unsigned int i;
> >         int ret;
> >
> >         ret = cci_enable_clocks(cci);
> >         if (ret)
> >                 return ret;
> >
> > +       for (i = 0; i < cci->data->num_masters; i++) {
> > +               if (!cci->master[i].cci)
> > +                       continue;
> > +
> > +               bus_regulator = cci->master[i].adap.bus_regulator;
> > +               if (!bus_regulator)
> > +                       continue;
> > +
> > +               if (!regulator_is_enabled(bus_regulator)) {
> > +                       ret = regulator_enable(bus_regulator);
> > +                       if (ret)
> > +                               dev_err(dev, "failed to enable regulator: %d\n",
> > +                                       ret);
> > +               }
> > +       }
> > +
> >         cci_init(cci);
> >         return 0;
> >  }
> > @@ -593,6 +626,7 @@ static int cci_probe(struct platform_device *pdev)
> >         dev_dbg(dev, "CCI HW version = 0x%08x", val);
> >
> >         for_each_available_child_of_node(dev->of_node, child) {
> > +               struct regulator *bus_regulator;
> >                 struct cci_master *master;
> >                 u32 idx;
> >
> > @@ -637,6 +671,21 @@ static int cci_probe(struct platform_device *pdev)
> >                         master->cci = NULL;
> >                         goto error_i2c;
> >                 }
> > +
> > +               /*
> > +                * It might be possible to find an optional vbus supply, but
> > +                * it requires to pass the registration of an I2C adapter
> > +                * device and its association with a bus device tree node.
> > +                */
> > +               bus_regulator = devm_regulator_get_optional(&master->adap.dev,
> > +                                                           "vbus");
> > +               if (IS_ERR(bus_regulator)) {
> > +                       ret = PTR_ERR(bus_regulator);
> > +                       if (ret == -EPROBE_DEFER)
> > +                               goto error_i2c;
> > +                       bus_regulator = NULL;
> > +               }
> > +               master->adap.bus_regulator = bus_regulator;
> >         }
> >
> >         ret = cci_reset(cci);
> > --
> > 2.33.0
> >
>
> With the above nit sorted, feel free to add my r-b.
>
> Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Bjorn Andersson]

On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:

> Quite regularly I2C bus lines on QCOM CCI controller require an external
> pull-up to a regulator powered line, to be able to define all such
> cases an additional vbus-supply property of a bus subnode is wanted.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> index 924ad8c03464..9f5b321748f1 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt
> @@ -60,6 +60,11 @@ PROPERTIES:
>  	Definition: Desired I2C bus clock frequency in Hz, defaults to 100
>  		    kHz if omitted.
>  
> +- vbus-supply:

I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
something like that would be better.

But there's a bigger question here, this is not a supply for the
i2c master, it's simply a supply for pulling up the bus. So it's not
entirely correct to specify it as a supply for the CCI node (which is
also the reason why the name isn't obvious).

Typically we don't don't mention the bus-supply because it happens to be
pulled up either by io-supply for the block, or by some always-on
regulator in the system.

Looping in Linus and Mark in hope they have seen this need elsewhere.

Regards,
Bjorn

> +	Usage: optional
> +	Value type: phandle
> +	Definition: Regulator that provides power to SCL/SDA lines
> +
>  Example:
>  
>  	cci@a0c000 {
> -- 
> 2.33.0
> 

Re: [PATCH 3/9] i2c: qcom-cci: don't delete an unregistered adapter

[Bjorn Andersson]

On Thu 03 Feb 08:47 PST 2022, Vladimir Zapolskiy wrote:

> If i2c_add_adapter() fails to add an I2C adapter found on QCOM CCI
> controller, on error path i2c_del_adapter() is still called.
> 
> Fortunately there is a sanity check in the I2C core, so the only
> visible implication is a printed debug level message:
> 
>     i2c-core: attempting to delete unregistered adapter [Qualcomm-CCI]
> 
> Nevertheless it would be reasonable to correct the probe error path.
> 
> Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Fixes like this should either come first in the series, or be sent on
their own, as it could be merged without considering the remainder of
the series


Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index c1de8eb66169..fd4260d18577 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -655,7 +655,7 @@ static int cci_probe(struct platform_device *pdev)
>  	return 0;
>  
>  error_i2c:
> -	for (; i >= 0; i--) {
> +	for (--i ; i >= 0; i--) {
>  		if (cci->master[i].cci)
>  			i2c_del_adapter(&cci->master[i].adap);
>  	}
> -- 
> 2.33.0
> 

Re: [PATCH 4/9] i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()

[Bjorn Andersson]

On Thu 03 Feb 08:47 PST 2022, Vladimir Zapolskiy wrote:

> There is a minor chance for a race, if a pointer to an i2c-bus subnode
> is stored and then reused after releasing its reference, and it would
> be sufficient to get one more reference under a loop over children
> subnodes.
> 
> Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Regards,
Bjorn

> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 14 ++++++++++----
>  1 file changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index fd4260d18577..cf54f1cb4c57 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -558,7 +558,7 @@ static int cci_probe(struct platform_device *pdev)
>  		cci->master[idx].adap.quirks = &cci->data->quirks;
>  		cci->master[idx].adap.algo = &cci_algo;
>  		cci->master[idx].adap.dev.parent = dev;
> -		cci->master[idx].adap.dev.of_node = child;
> +		cci->master[idx].adap.dev.of_node = of_node_get(child);
>  		cci->master[idx].master = idx;
>  		cci->master[idx].cci = cci;
>  
> @@ -643,8 +643,10 @@ static int cci_probe(struct platform_device *pdev)
>  			continue;
>  
>  		ret = i2c_add_adapter(&cci->master[i].adap);
> -		if (ret < 0)
> +		if (ret < 0) {
> +			of_node_put(cci->master[i].adap.dev.of_node);
>  			goto error_i2c;
> +		}
>  	}
>  
>  	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
> @@ -656,8 +658,10 @@ static int cci_probe(struct platform_device *pdev)
>  
>  error_i2c:
>  	for (--i ; i >= 0; i--) {
> -		if (cci->master[i].cci)
> +		if (cci->master[i].cci) {
>  			i2c_del_adapter(&cci->master[i].adap);
> +			of_node_put(cci->master[i].adap.dev.of_node);
> +		}
>  	}
>  error:
>  	disable_irq(cci->irq);
> @@ -673,8 +677,10 @@ static int cci_remove(struct platform_device *pdev)
>  	int i;
>  
>  	for (i = 0; i < cci->data->num_masters; i++) {
> -		if (cci->master[i].cci)
> +		if (cci->master[i].cci) {
>  			i2c_del_adapter(&cci->master[i].adap);
> +			of_node_put(cci->master[i].adap.dev.of_node);
> +		}
>  		cci_halt(cci, i);
>  	}
>  
> -- 
> 2.33.0
> 

Re: [PATCH 8/9] i2c: qcom-cci: add support of optional vbus-supply regulators

[Bjorn Andersson]

On Thu 03 Feb 08:47 PST 2022, Vladimir Zapolskiy wrote:

> The change adds handling of optional vbus regulators in the driver.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index 775945f7b4cd..2fc7f1f2616f 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> +#include <linux/regulator/consumer.h>
>  
>  #define CCI_HW_VERSION				0x0
>  #define CCI_RESET_CMD				0x004
> @@ -480,6 +481,20 @@ static void cci_disable_clocks(struct cci *cci)
>  static int __maybe_unused cci_suspend_runtime(struct device *dev)
>  {
>  	struct cci *cci = dev_get_drvdata(dev);
> +	struct regulator *bus_regulator;
> +	unsigned int i;
> +
> +	for (i = 0; i < cci->data->num_masters; i++) {
> +		if (!cci->master[i].cci)
> +			continue;
> +
> +		bus_regulator = cci->master[i].adap.bus_regulator;
> +		if (!bus_regulator)
> +			continue;
> +
> +		if (regulator_is_enabled(bus_regulator) > 0)
> +			regulator_disable(bus_regulator);
> +	}
>  
>  	cci_disable_clocks(cci);
>  	return 0;
> @@ -488,12 +503,30 @@ static int __maybe_unused cci_suspend_runtime(struct device *dev)
>  static int __maybe_unused cci_resume_runtime(struct device *dev)
>  {
>  	struct cci *cci = dev_get_drvdata(dev);
> +	struct regulator *bus_regulator;
> +	unsigned int i;
>  	int ret;
>  
>  	ret = cci_enable_clocks(cci);
>  	if (ret)
>  		return ret;
>  
> +	for (i = 0; i < cci->data->num_masters; i++) {
> +		if (!cci->master[i].cci)
> +			continue;
> +
> +		bus_regulator = cci->master[i].adap.bus_regulator;
> +		if (!bus_regulator)
> +			continue;
> +
> +		if (!regulator_is_enabled(bus_regulator)) {

regulator_is_enabled() tests if the regulator is enabled, not if you
have enabled it. So if this is a shared regulator you might learn that
it's already on, skip your regulator_enable() and then the other
consumer turns it off behind your back.

If you want the regulator to be on, you should regulator_enable().

> +			ret = regulator_enable(bus_regulator);
> +			if (ret)
> +				dev_err(dev, "failed to enable regulator: %d\n",
> +					ret);
> +		}
> +	}
> +
>  	cci_init(cci);
>  	return 0;
>  }
> @@ -593,6 +626,7 @@ static int cci_probe(struct platform_device *pdev)
>  	dev_dbg(dev, "CCI HW version = 0x%08x", val);
>  
>  	for_each_available_child_of_node(dev->of_node, child) {
> +		struct regulator *bus_regulator;
>  		struct cci_master *master;
>  		u32 idx;
>  
> @@ -637,6 +671,21 @@ static int cci_probe(struct platform_device *pdev)
>  			master->cci = NULL;
>  			goto error_i2c;
>  		}
> +
> +		/*
> +		 * It might be possible to find an optional vbus supply, but
> +		 * it requires to pass the registration of an I2C adapter
> +		 * device and its association with a bus device tree node.
> +		 */

I'm afraid that I don't understand this comment. The regulator is
optional because most of the time we don't control it explicitly.

So a comment like "Control of IO supply is optional" seems more
relevant.

Regards,
Bjorn

> +		bus_regulator = devm_regulator_get_optional(&master->adap.dev,
> +							    "vbus");
> +		if (IS_ERR(bus_regulator)) {
> +			ret = PTR_ERR(bus_regulator);
> +			if (ret == -EPROBE_DEFER)
> +				goto error_i2c;
> +			bus_regulator = NULL;
> +		}
> +		master->adap.bus_regulator = bus_regulator;
>  	}
>  
>  	ret = cci_reset(cci);
> -- 
> 2.33.0
> 

Re: [PATCH 8/9] i2c: qcom-cci: add support of optional vbus-supply regulators

[Bjorn Andersson]

On Fri 04 Feb 03:41 PST 2022, Loic Poulain wrote:

> On Fri, 4 Feb 2022 at 12:03, Robert Foss <robert.foss@linaro.org> wrote:
> >
> > On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> > <vladimir.zapolskiy@linaro.org> wrote:
> > >
> > > The change adds handling of optional vbus regulators in the driver.
> > >
> > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> > > ---
> > >  drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > > index 775945f7b4cd..2fc7f1f2616f 100644
> > > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > >  #define CCI_HW_VERSION                         0x0
> > >  #define CCI_RESET_CMD                          0x004
> > > @@ -480,6 +481,20 @@ static void cci_disable_clocks(struct cci *cci)
> > >  static int __maybe_unused cci_suspend_runtime(struct device *dev)
> > >  {
> > >         struct cci *cci = dev_get_drvdata(dev);
> > > +       struct regulator *bus_regulator;
> > > +       unsigned int i;
> > > +
> > > +       for (i = 0; i < cci->data->num_masters; i++) {
> > > +               if (!cci->master[i].cci)
> > > +                       continue;
> > > +
> > > +               bus_regulator = cci->master[i].adap.bus_regulator;
> > > +               if (!bus_regulator)
> > > +                       continue;
> > > +
> > > +               if (regulator_is_enabled(bus_regulator) > 0)
> >
> > Is this check needed? No matter the current status of the regulator,
> > we'd like to disable it, and from my reading regulator_disable can be
> > called for already disabled regulators.
> 
> +1, but why do we even assign this regulator to each adapter, a
> simpler solution would be to have the regulator part of the cci
> struct, and simply disable/enable it on runtime suspend/resume,
> without extra loop/check.

But that implies that you always will have the same io-supply for your
two busses. Something that seems likely but I don't see that it's a
requirement.

Although as proposed "vbus" is acquired from the cci node and not the
individual bus anyways...

> I2C core does nothing with
> adap.bus_regulator anyway (5a7b95fb993e has been partially reverted).
> 

Thanks for the pointer, that looks like a much better and generic
solution. In particular if we specify the regulator per bus.

Regards,
Bjorn

> >
> > > +                       regulator_disable(bus_regulator);
> > > +       }
> > >
> > >         cci_disable_clocks(cci);
> > >         return 0;
> > > @@ -488,12 +503,30 @@ static int __maybe_unused cci_suspend_runtime(struct device *dev)
> > >  static int __maybe_unused cci_resume_runtime(struct device *dev)
> > >  {
> > >         struct cci *cci = dev_get_drvdata(dev);
> > > +       struct regulator *bus_regulator;
> > > +       unsigned int i;
> > >         int ret;
> > >
> > >         ret = cci_enable_clocks(cci);
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       for (i = 0; i < cci->data->num_masters; i++) {
> > > +               if (!cci->master[i].cci)
> > > +                       continue;
> > > +
> > > +               bus_regulator = cci->master[i].adap.bus_regulator;
> > > +               if (!bus_regulator)
> > > +                       continue;
> > > +
> > > +               if (!regulator_is_enabled(bus_regulator)) {
> > > +                       ret = regulator_enable(bus_regulator);
> > > +                       if (ret)
> > > +                               dev_err(dev, "failed to enable regulator: %d\n",
> > > +                                       ret);
> > > +               }
> > > +       }
> > > +
> > >         cci_init(cci);
> > >         return 0;
> > >  }
> > > @@ -593,6 +626,7 @@ static int cci_probe(struct platform_device *pdev)
> > >         dev_dbg(dev, "CCI HW version = 0x%08x", val);
> > >
> > >         for_each_available_child_of_node(dev->of_node, child) {
> > > +               struct regulator *bus_regulator;
> > >                 struct cci_master *master;
> > >                 u32 idx;
> > >
> > > @@ -637,6 +671,21 @@ static int cci_probe(struct platform_device *pdev)
> > >                         master->cci = NULL;
> > >                         goto error_i2c;
> > >                 }
> > > +
> > > +               /*
> > > +                * It might be possible to find an optional vbus supply, but
> > > +                * it requires to pass the registration of an I2C adapter
> > > +                * device and its association with a bus device tree node.
> > > +                */
> > > +               bus_regulator = devm_regulator_get_optional(&master->adap.dev,
> > > +                                                           "vbus");
> > > +               if (IS_ERR(bus_regulator)) {
> > > +                       ret = PTR_ERR(bus_regulator);
> > > +                       if (ret == -EPROBE_DEFER)
> > > +                               goto error_i2c;
> > > +                       bus_regulator = NULL;
> > > +               }
> > > +               master->adap.bus_regulator = bus_regulator;
> > >         }
> > >
> > >         ret = cci_reset(cci);
> > > --
> > > 2.33.0
> > >
> >
> > With the above nit sorted, feel free to add my r-b.
> >
> > Reviewed-by: Robert Foss <robert.foss@linaro.org>

Re: [PATCH 5/9] i2c: qcom-cci: initialize CCI controller after registration of adapters

[Bjorn Andersson]

On Thu 03 Feb 08:47 PST 2022, Vladimir Zapolskiy wrote:

> The change is wanted to postpone initialization of busses on CCI controller
> by cci_init() and cci_reset() till adapters are registered, the later is
> needed for adding I2C bus devices and get correspondent vbus regulators.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  drivers/i2c/busses/i2c-qcom-cci.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> index cf54f1cb4c57..eebf9603d3d1 100644
> --- a/drivers/i2c/busses/i2c-qcom-cci.c
> +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> @@ -630,14 +630,6 @@ static int cci_probe(struct platform_device *pdev)
>  	val = readl(cci->base + CCI_HW_VERSION);
>  	dev_dbg(dev, "CCI HW version = 0x%08x", val);
>  
> -	ret = cci_reset(cci);
> -	if (ret < 0)
> -		goto error;
> -
> -	ret = cci_init(cci);
> -	if (ret < 0)
> -		goto error;
> -
>  	for (i = 0; i < cci->data->num_masters; i++) {
>  		if (!cci->master[i].cci)
>  			continue;
> @@ -649,6 +641,14 @@ static int cci_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ret = cci_reset(cci);

i2c_add_adapter() will register and probe child devices, which might
want to access the bus in their probe functions. Don't you break that by
initializing the master after the children?

Regards,
Bjorn

> +	if (ret < 0)
> +		goto error_i2c;
> +
> +	ret = cci_init(cci);
> +	if (ret < 0)
> +		goto error_i2c;
> +
>  	pm_runtime_set_autosuspend_delay(dev, MSEC_PER_SEC);
>  	pm_runtime_use_autosuspend(dev);
>  	pm_runtime_set_active(dev);
> @@ -663,7 +663,6 @@ static int cci_probe(struct platform_device *pdev)
>  			of_node_put(cci->master[i].adap.dev.of_node);
>  		}
>  	}
> -error:
>  	disable_irq(cci->irq);
>  disable_clocks:
>  	cci_disable_clocks(cci);
> -- 
> 2.33.0
> 

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:

> > +- vbus-supply:

> I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
> something like that would be better.

> But there's a bigger question here, this is not a supply for the
> i2c master, it's simply a supply for pulling up the bus. So it's not
> entirely correct to specify it as a supply for the CCI node (which is
> also the reason why the name isn't obvious).

Does the device (controller?) not have a supply that the I2C bus is
referenced to?  If so that supply should be named.

> Typically we don't don't mention the bus-supply because it happens to be
> pulled up either by io-supply for the block, or by some always-on
> regulator in the system.

If the bus is being pulled up to some supply other than the supply that
the bus is referenced to that doesn't sound like the greatest electrical
engineering ever...  without any context it's hard to comment about this
particular system.

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Bjorn Andersson]

On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:

> On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> > On Thu 03 Feb 08:46 PST 2022, Vladimir Zapolskiy wrote:
> 
> > > +- vbus-supply:
> 
> > I don't think "vbus" is an appropriate name for his. Perhaps "vddio" or
> > something like that would be better.
> 
> > But there's a bigger question here, this is not a supply for the
> > i2c master, it's simply a supply for pulling up the bus. So it's not
> > entirely correct to specify it as a supply for the CCI node (which is
> > also the reason why the name isn't obvious).
> 
> Does the device (controller?) not have a supply that the I2C bus is
> referenced to?  If so that supply should be named.
> 

No, for some reason the regulator in question is not connected to either
the master or the client devices, it's only used for pull up of a few
of the i2c busses.

> > Typically we don't don't mention the bus-supply because it happens to be
> > pulled up either by io-supply for the block, or by some always-on
> > regulator in the system.
> 
> If the bus is being pulled up to some supply other than the supply that
> the bus is referenced to that doesn't sound like the greatest electrical
> engineering ever...  without any context it's hard to comment about this
> particular system.

That's what the schematics says...

Regards,
Bjorn

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Fri, Feb 04, 2022 at 11:02:04AM -0800, Bjorn Andersson wrote:
> On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:
> > On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:

> > > Typically we don't don't mention the bus-supply because it happens to be
> > > pulled up either by io-supply for the block, or by some always-on
> > > regulator in the system.

> > If the bus is being pulled up to some supply other than the supply that
> > the bus is referenced to that doesn't sound like the greatest electrical
> > engineering ever...  without any context it's hard to comment about this
> > particular system.

> That's what the schematics says...

Oh, good.  I forsee no problems here.  Probably this is something that
should be in the I2C core if it's going to be dynamically managed,
though just setting the supply as always on is probably more expedient.

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Vladimir Zapolskiy]

Hi Bjorn, Mark,

On 2/4/22 9:32 PM, Mark Brown wrote:
> On Fri, Feb 04, 2022 at 11:02:04AM -0800, Bjorn Andersson wrote:
>> On Fri 04 Feb 10:42 PST 2022, Mark Brown wrote:
>>> On Fri, Feb 04, 2022 at 10:05:47AM -0800, Bjorn Andersson wrote:
> 
>>>> Typically we don't don't mention the bus-supply because it happens to be
>>>> pulled up either by io-supply for the block, or by some always-on
>>>> regulator in the system.
> 
>>> If the bus is being pulled up to some supply other than the supply that
>>> the bus is referenced to that doesn't sound like the greatest electrical
>>> engineering ever...  without any context it's hard to comment about this
>>> particular system.
> 
>> That's what the schematics says...
> 
> Oh, good.  I forsee no problems here.  Probably this is something that
> should be in the I2C core if it's going to be dynamically managed,
> though just setting the supply as always on is probably more expedient.
> 

vbus-supply property has been added recently to another I2C master controller,
see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").
It serves right the same purpose, and its handling is going to be done in i2c
core, however since the latter is not yet completed, I would propose to add
the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
later on both the property and its generic support would be better to see in
i2c core.

--
Best wishes,
Vladimir

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Mon, Feb 07, 2022 at 04:08:01PM +0200, Vladimir Zapolskiy wrote:
> On 2/4/22 9:32 PM, Mark Brown wrote:

> > Oh, good.  I forsee no problems here.  Probably this is something that
> > should be in the I2C core if it's going to be dynamically managed,
> > though just setting the supply as always on is probably more expedient.

> vbus-supply property has been added recently to another I2C master controller,
> see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").

Note that some devices do have supplies that I/O is referenced against
and it's not clear that this isn't what's goin on here.

> It serves right the same purpose, and its handling is going to be done in i2c
> core, however since the latter is not yet completed, I would propose to add
> the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
> later on both the property and its generic support would be better to see in
> i2c core.

The bindings are ABI, it doesn't seem like a good idea to add new ABI as
a temporary bodge.

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Vladimir Zapolskiy]

On 2/7/22 4:39 PM, Mark Brown wrote:
> On Mon, Feb 07, 2022 at 04:08:01PM +0200, Vladimir Zapolskiy wrote:
>> On 2/4/22 9:32 PM, Mark Brown wrote:
> 
>>> Oh, good.  I forsee no problems here.  Probably this is something that
>>> should be in the I2C core if it's going to be dynamically managed,
>>> though just setting the supply as always on is probably more expedient.
> 
>> vbus-supply property has been added recently to another I2C master controller,
>> see commit c021087c43c8 ("dt-binding: i2c: mt65xx: add vbus-supply property").
> 
> Note that some devices do have supplies that I/O is referenced against
> and it's not clear that this isn't what's goin on here.
>
>> It serves right the same purpose, and its handling is going to be done in i2c
>> core, however since the latter is not yet completed, I would propose to add
>> the property to i2c-bus subnodes of QCOM CCI and its support in the driver,
>> later on both the property and its generic support would be better to see in
>> i2c core.
> 
> The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> a temporary bodge.

The bindings are supposed to describe hardware, thus it's natural to extend
them, I believe there is a trilemma in this particular case:
1) add optional vbus-supply property to all I2C master controllers or I2C
    busses in case of multiple I2C busses managed by a single controller,
2) add optional vbus-supply property to all I2C slave devices,
3) ignore peculiarities of particular (multiple in fact) PCB designs and
    a necessity of adding a regulator finely described as a pull-up for I2C
    bus lines.

My assumption is that a decision should be generic for all similar cases,
Wolfram, could you share your point of view on the subject?

--
Best wishes,
Vladimir

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> On 2/7/22 4:39 PM, Mark Brown wrote:

> > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > a temporary bodge.

> The bindings are supposed to describe hardware, thus it's natural to extend
> them, I believe there is a trilemma in this particular case:
> 1) add optional vbus-supply property to all I2C master controllers or I2C
>    busses in case of multiple I2C busses managed by a single controller,
> 2) add optional vbus-supply property to all I2C slave devices,

If you add a named supply to all I2C controllers or devices then if any
of them have an actual vbus supply there will be a namespace collision.

> 3) ignore peculiarities of particular (multiple in fact) PCB designs and
>    a necessity of adding a regulator finely described as a pull-up for I2C
>    bus lines.

There's also the option of representing this as a separate thing on or
part of the bus.

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
>
> On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > On 2/7/22 4:39 PM, Mark Brown wrote:
>
> > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > a temporary bodge.

It's not a temporary bodge. The i2c-core piece was reverted, but not
the mediatek driver code/bindings.
Vladimir has provided a replacement for the i2c-core code handling the
vbus-regulator. When thee code will be back, the code from i2c-cci can
be removed. The bindings will be the same.

>
> > The bindings are supposed to describe hardware, thus it's natural to extend
> > them, I believe there is a trilemma in this particular case:
> > 1) add optional vbus-supply property to all I2C master controllers or I2C
> >    busses in case of multiple I2C busses managed by a single controller,
> > 2) add optional vbus-supply property to all I2C slave devices,
>
> If you add a named supply to all I2C controllers or devices then if any
> of them have an actual vbus supply there will be a namespace collision.
>
> > 3) ignore peculiarities of particular (multiple in fact) PCB designs and
> >    a necessity of adding a regulator finely described as a pull-up for I2C
> >    bus lines.
>
> There's also the option of representing this as a separate thing on or
> part of the bus.

4) (which you have implemented in your patch). Add support for  the
vbus-supplies property for the I2C CCI controllers.

This is the option I'd vote for.

-- 
With best wishes
Dmitry

Re: [PATCH 8/9] i2c: qcom-cci: add support of optional vbus-supply regulators

[Dmitry Baryshkov]

On Fri, 4 Feb 2022 at 14:42, Loic Poulain <loic.poulain@linaro.org> wrote:
>
> On Fri, 4 Feb 2022 at 12:03, Robert Foss <robert.foss@linaro.org> wrote:
> >
> > On Thu, 3 Feb 2022 at 17:47, Vladimir Zapolskiy
> > <vladimir.zapolskiy@linaro.org> wrote:
> > >
> > > The change adds handling of optional vbus regulators in the driver.
> > >
> > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> > > ---
> > >  drivers/i2c/busses/i2c-qcom-cci.c | 49 +++++++++++++++++++++++++++++++
> > >  1 file changed, 49 insertions(+)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-qcom-cci.c b/drivers/i2c/busses/i2c-qcom-cci.c
> > > index 775945f7b4cd..2fc7f1f2616f 100644
> > > --- a/drivers/i2c/busses/i2c-qcom-cci.c
> > > +++ b/drivers/i2c/busses/i2c-qcom-cci.c
> > > @@ -11,6 +11,7 @@
> > >  #include <linux/of.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm_runtime.h>
> > > +#include <linux/regulator/consumer.h>
> > >
> > >  #define CCI_HW_VERSION                         0x0
> > >  #define CCI_RESET_CMD                          0x004
> > > @@ -480,6 +481,20 @@ static void cci_disable_clocks(struct cci *cci)
> > >  static int __maybe_unused cci_suspend_runtime(struct device *dev)
> > >  {
> > >         struct cci *cci = dev_get_drvdata(dev);
> > > +       struct regulator *bus_regulator;
> > > +       unsigned int i;
> > > +
> > > +       for (i = 0; i < cci->data->num_masters; i++) {
> > > +               if (!cci->master[i].cci)
> > > +                       continue;
> > > +
> > > +               bus_regulator = cci->master[i].adap.bus_regulator;
> > > +               if (!bus_regulator)
> > > +                       continue;
> > > +
> > > +               if (regulator_is_enabled(bus_regulator) > 0)
> >
> > Is this check needed? No matter the current status of the regulator,
> > we'd like to disable it, and from my reading regulator_disable can be
> > called for already disabled regulators.
>
> +1, but why do we even assign this regulator to each adapter, a
> simpler solution would be to have the regulator part of the cci
> struct, and simply disable/enable it on runtime suspend/resume,
> without extra loop/check. I2C core does nothing with
> adap.bus_regulator anyway (5a7b95fb993e has been partially reverted).

I like the idea of having the regulator per bus (rather than per
controller). However instead of pushing handling these changes to the
CCI controller, I'd suggest to move this code to the i2c-core itself.
The original patch tried to do the regulator control per client.
Instead it should be done per adapter. I think this should also solve
the reported issue for AMD controllers (since that i2c adapters won't
have vbus-supply).

>
> >
> > > +                       regulator_disable(bus_regulator);
> > > +       }
> > >
> > >         cci_disable_clocks(cci);
> > >         return 0;
> > > @@ -488,12 +503,30 @@ static int __maybe_unused cci_suspend_runtime(struct device *dev)
> > >  static int __maybe_unused cci_resume_runtime(struct device *dev)
> > >  {
> > >         struct cci *cci = dev_get_drvdata(dev);
> > > +       struct regulator *bus_regulator;
> > > +       unsigned int i;
> > >         int ret;
> > >
> > >         ret = cci_enable_clocks(cci);
> > >         if (ret)
> > >                 return ret;
> > >
> > > +       for (i = 0; i < cci->data->num_masters; i++) {
> > > +               if (!cci->master[i].cci)
> > > +                       continue;
> > > +
> > > +               bus_regulator = cci->master[i].adap.bus_regulator;
> > > +               if (!bus_regulator)
> > > +                       continue;
> > > +
> > > +               if (!regulator_is_enabled(bus_regulator)) {
> > > +                       ret = regulator_enable(bus_regulator);
> > > +                       if (ret)
> > > +                               dev_err(dev, "failed to enable regulator: %d\n",
> > > +                                       ret);
> > > +               }
> > > +       }
> > > +
> > >         cci_init(cci);
> > >         return 0;
> > >  }
> > > @@ -593,6 +626,7 @@ static int cci_probe(struct platform_device *pdev)
> > >         dev_dbg(dev, "CCI HW version = 0x%08x", val);
> > >
> > >         for_each_available_child_of_node(dev->of_node, child) {
> > > +               struct regulator *bus_regulator;
> > >                 struct cci_master *master;
> > >                 u32 idx;
> > >
> > > @@ -637,6 +671,21 @@ static int cci_probe(struct platform_device *pdev)
> > >                         master->cci = NULL;
> > >                         goto error_i2c;
> > >                 }
> > > +
> > > +               /*
> > > +                * It might be possible to find an optional vbus supply, but
> > > +                * it requires to pass the registration of an I2C adapter
> > > +                * device and its association with a bus device tree node.
> > > +                */
> > > +               bus_regulator = devm_regulator_get_optional(&master->adap.dev,
> > > +                                                           "vbus");
> > > +               if (IS_ERR(bus_regulator)) {
> > > +                       ret = PTR_ERR(bus_regulator);
> > > +                       if (ret == -EPROBE_DEFER)
> > > +                               goto error_i2c;
> > > +                       bus_regulator = NULL;
> > > +               }
> > > +               master->adap.bus_regulator = bus_regulator;
> > >         }
> > >
> > >         ret = cci_reset(cci);
> > > --
> > > 2.33.0
> > >
> >
> > With the above nit sorted, feel free to add my r-b.
> >
> > Reviewed-by: Robert Foss <robert.foss@linaro.org>



-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Thu, Feb 10, 2022 at 06:33:09PM +0300, Dmitry Baryshkov wrote:
> On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
> > On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > > On 2/7/22 4:39 PM, Mark Brown wrote:

> > > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > > a temporary bodge.

> It's not a temporary bodge. The i2c-core piece was reverted, but not
> the mediatek driver code/bindings.
> Vladimir has provided a replacement for the i2c-core code handling the
> vbus-regulator. When thee code will be back, the code from i2c-cci can
> be removed. The bindings will be the same.

I would hope it's a temporary thing given the namespace collision
issues...

> > There's also the option of representing this as a separate thing on or
> > part of the bus.

> 4) (which you have implemented in your patch). Add support for  the
> vbus-supplies property for the I2C CCI controllers.

> This is the option I'd vote for.

Do these controllers actually have a supply called vbus?

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 06:33:09PM +0300, Dmitry Baryshkov wrote:
> > On Tue, 8 Feb 2022 at 16:16, Mark Brown <broonie@kernel.org> wrote:
> > > On Mon, Feb 07, 2022 at 08:31:30PM +0200, Vladimir Zapolskiy wrote:
> > > > On 2/7/22 4:39 PM, Mark Brown wrote:
>
> > > > > The bindings are ABI, it doesn't seem like a good idea to add new ABI as
> > > > > a temporary bodge.
>
> > It's not a temporary bodge. The i2c-core piece was reverted, but not
> > the mediatek driver code/bindings.
> > Vladimir has provided a replacement for the i2c-core code handling the
> > vbus-regulator. When thee code will be back, the code from i2c-cci can
> > be removed. The bindings will be the same.
>
> I would hope it's a temporary thing given the namespace collision
> issues...

Which collision? CCI doesn't have a separate vbus power input (and
probably never will).

>
> > > There's also the option of representing this as a separate thing on or
> > > part of the bus.
>
> > 4) (which you have implemented in your patch). Add support for  the
> > vbus-supplies property for the I2C CCI controllers.
>
> > This is the option I'd vote for.
>
> Do these controllers actually have a supply called vbus?

No. It's a separate entity, a regulator-controller pull-up for the bus.
So far we'd like to hear better suggestions. Using regulator-always-on
doesn't sound like a good idea, it will increase unnecessary power
drain.

-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Thu, Feb 10, 2022 at 08:32:09PM +0300, Dmitry Baryshkov wrote:
> On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:

> > I would hope it's a temporary thing given the namespace collision
> > issues...

> Which collision? CCI doesn't have a separate vbus power input (and
> probably never will).

That "probably" there is doing some work, and if you're doing something
at the I2C core level (as it seems should be done) it needs to cope with
all possible controllers and devices.

> > Do these controllers actually have a supply called vbus?

> No. It's a separate entity, a regulator-controller pull-up for the bus.
> So far we'd like to hear better suggestions. Using regulator-always-on
> doesn't sound like a good idea, it will increase unnecessary power
> drain.

Please see my suggestions elsewhere in the thread.

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 20:36, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 08:32:09PM +0300, Dmitry Baryshkov wrote:
> > On Thu, 10 Feb 2022 at 18:45, Mark Brown <broonie@kernel.org> wrote:
>
> > > I would hope it's a temporary thing given the namespace collision
> > > issues...
>
> > Which collision? CCI doesn't have a separate vbus power input (and
> > probably never will).
>
> That "probably" there is doing some work, and if you're doing something
> at the I2C core level (as it seems should be done) it needs to cope with
> all possible controllers and devices.
>
> > > Do these controllers actually have a supply called vbus?
>
> > No. It's a separate entity, a regulator-controller pull-up for the bus.
> > So far we'd like to hear better suggestions. Using regulator-always-on
> > doesn't sound like a good idea, it will increase unnecessary power
> > drain.
>
> Please see my suggestions elsewhere in the thread.

Please excuse me. I missed the e-mail suggesting to move support for
that into the core level.
I'd second a request to handle the adapter->bus_regulator in the core code.
Would you be ok with the 'external-sda-scl-supply' property? Would you
demand that it's completely handled by the core layer (including DT
parsing) or should we let a driver parse the DT property?


-- 
With best wishes
Dmitry

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Mark Brown]

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

On Thu, Feb 10, 2022 at 09:21:42PM +0300, Dmitry Baryshkov wrote:

> Please excuse me. I missed the e-mail suggesting to move support for
> that into the core level.

No problem.

> I'd second a request to handle the adapter->bus_regulator in the core code.
> Would you be ok with the 'external-sda-scl-supply' property? Would you
> demand that it's completely handled by the core layer (including DT
> parsing) or should we let a driver parse the DT property?

I'm not super worried about how it's implemented so long as the binding
is good for the long term - if doing it in a driver helps get things
done that's fixable later on without breaking ABI.

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

Re: [PATCH 2/9] dt-bindings: i2c: qcom-cci: add description of a vbus-supply property

[Dmitry Baryshkov]

On Thu, 10 Feb 2022 at 21:26, Mark Brown <broonie@kernel.org> wrote:
>
> On Thu, Feb 10, 2022 at 09:21:42PM +0300, Dmitry Baryshkov wrote:
>
> > I'd second a request to handle the adapter->bus_regulator in the core code.
> > Would you be ok with the 'external-sda-scl-supply' property? Would you
> > demand that it's completely handled by the core layer (including DT
> > parsing) or should we let a driver parse the DT property?
>
> I'm not super worried about how it's implemented so long as the binding
> is good for the long term - if doing it in a driver helps get things
> done that's fixable later on without breaking ABI.

So, 'external-sda-scl-supply'?



-- 
With best wishes
Dmitry

Re: [PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Rob Herring]

On Thu, 03 Feb 2022 18:46:28 +0200, Vladimir Zapolskiy wrote:
> The change adds QCOM SM8450 compatible value to the list of QCOM CCI
> controller compatibles, the controller found on the SoC is equal to
> the ones found on previous SoC generations.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>
> ---
>  Documentation/devicetree/bindings/i2c/i2c-qcom-cci.txt | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>

Re: [PATCH 3/9] i2c: qcom-cci: don't delete an unregistered adapter

[Wolfram Sang]

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

On Thu, Feb 03, 2022 at 06:47:00PM +0200, Vladimir Zapolskiy wrote:
> If i2c_add_adapter() fails to add an I2C adapter found on QCOM CCI
> controller, on error path i2c_del_adapter() is still called.
> 
> Fortunately there is a sanity check in the I2C core, so the only
> visible implication is a printed debug level message:
> 
>     i2c-core: attempting to delete unregistered adapter [Qualcomm-CCI]
> 
> Nevertheless it would be reasonable to correct the probe error path.
> 
> Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Applied to for-current, thanks!


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

Re: [PATCH 4/9] i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()

[Wolfram Sang]

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

On Thu, Feb 03, 2022 at 06:47:03PM +0200, Vladimir Zapolskiy wrote:
> There is a minor chance for a race, if a pointer to an i2c-bus subnode
> is stored and then reused after releasing its reference, and it would
> be sufficient to get one more reference under a loop over children
> subnodes.
> 
> Fixes: e517526195de ("i2c: Add Qualcomm CCI I2C driver")
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Applied to for-current, thanks!


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

Re: [PATCH 3/9] i2c: qcom-cci: don't delete an unregistered adapter

[Wolfram Sang]

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


> Fixes like this should either come first in the series, or be sent on
> their own, as it could be merged without considering the remainder of
> the series

True. Luckily, I found the two fixes and applied them.


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

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

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

Hi,

> The new bus adapter specific bus_regulator from commit 5a7b95fb993e
> ("i2c: core: support bus regulator controlling in adapter") is reused,

Reusing is nice, of course, but I hope you noticed that I needed to
revert this feature:

a19f75de73c2 ("Revert "i2c: core: support bus regulator controlling in adapter"")

The thread to get it re-applied is currently here:

https://lore.kernel.org/all/20220106122452.18719-1-wsa@kernel.org/

Happy hacking,

   Wolfram


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

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Vladimir Zapolskiy]

Hi Wolfram,

On 2/11/22 7:49 PM, Wolfram Sang wrote:
> Hi,
> 
>> The new bus adapter specific bus_regulator from commit 5a7b95fb993e
>> ("i2c: core: support bus regulator controlling in adapter") is reused,
> 
> Reusing is nice, of course, but I hope you noticed that I needed to
> revert this feature:
> 
> a19f75de73c2 ("Revert "i2c: core: support bus regulator controlling in adapter"")

yes, I've seen it, and as far as I understand it's expected to get it
back after the regression fixes.

> The thread to get it re-applied is currently here:
> 
> https://lore.kernel.org/all/20220106122452.18719-1-wsa@kernel.org/
> 

A presented change in the series is I2C controller specific, so it works well
on top of the reverted feature, however it has a potential to be simplified
after the re-application.

Wolfram, can you please share your opinion on device tree binding name and
placement for an SDA/SDC pull-up controlled by a regulator?

See https://lore.kernel.org/all/682b7ffe-e162-bcf7-3c07-36b3a39c25ab@linaro.org

Thank you in advance.

--
Best wishes,
Vladimir

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

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


> > Reusing is nice, of course, but I hope you noticed that I needed to
> > revert this feature:
> > 
> > a19f75de73c2 ("Revert "i2c: core: support bus regulator controlling in adapter"")
> 
> yes, I've seen it, and as far as I understand it's expected to get it
> back after the regression fixes.

True, but work on this has stalled, sadly. I am gathering interested
parties for the topic here :)

> Wolfram, can you please share your opinion on device tree binding name and
> placement for an SDA/SDC pull-up controlled by a regulator?

For efficiency reasons, not before bus regulator has been applied again
because the above question depends on it IIUC. Until then, I'll work on
other items of my too long todo list.


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

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

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


> Vladimir Zapolskiy (9):
>   dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible
>   dt-bindings: i2c: qcom-cci: add description of a vbus-supply property
>   i2c: qcom-cci: don't delete an unregistered adapter
>   i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()
>   i2c: qcom-cci: initialize CCI controller after registration of adapters
>   i2c: qcom-cci: simplify probe by removing one loop over busses
>   i2c: qcom-cci: simplify access to bus data structure
>   i2c: qcom-cci: add support of optional vbus-supply regulators
>   i2c: qcom-cci: add sm8450 compatible

Patches 3+4 are already upstream. I wonder if patches 1+9 could be
applied to for-next also? Or is the vbus-supply a hard dependency here?
Patches 6+7 could probably also be resent individually after some
rebasing.


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

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Vladimir Zapolskiy]

Hi Wolfram,

On 2/17/22 9:57 PM, Wolfram Sang wrote:
> 
>> Vladimir Zapolskiy (9):
>>    dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible
>>    dt-bindings: i2c: qcom-cci: add description of a vbus-supply property
>>    i2c: qcom-cci: don't delete an unregistered adapter
>>    i2c: qcom-cci: don't put a device tree node before i2c_add_adapter()
>>    i2c: qcom-cci: initialize CCI controller after registration of adapters
>>    i2c: qcom-cci: simplify probe by removing one loop over busses
>>    i2c: qcom-cci: simplify access to bus data structure
>>    i2c: qcom-cci: add support of optional vbus-supply regulators
>>    i2c: qcom-cci: add sm8450 compatible
> 
> Patches 3+4 are already upstream. I wonder if patches 1+9 could be
> applied to for-next also? Or is the vbus-supply a hard dependency here?
> Patches 6+7 could probably also be resent individually after some
> rebasing.
> 

thank you for applying the fixes, 1/9 and 9/9 are also good to be applied
for-next, there is no dependency on vbus-supply, so I would appreciate, if
you take two more changes.

As you suggested I'd start working on a generic support of such an optional
bus supplier property, I believe at the moment everything is quite clear,
I'll start from testing the previous solution, however my preference is
to connect regulator on/off to master controller pm ops rather than slave
pm ops. Additionally I am not quite satisfied with 'vbus-supply' name,
some name without a confusing starting 'v' should be preferred IMO, like
'i2c-bus-supply' or 'sda-scl-supply', name suggestions are welcome.

--
Best wishes,
Vladimir

Re: [PATCH 1/9] dt-bindings: i2c: qcom-cci: add QCOM SM8450 compatible

[Wolfram Sang]

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

On Thu, Feb 03, 2022 at 06:46:28PM +0200, Vladimir Zapolskiy wrote:
> The change adds QCOM SM8450 compatible value to the list of QCOM CCI
> controller compatibles, the controller found on the SoC is equal to
> the ones found on previous SoC generations.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Applied to for-next, thanks!


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

Re: [PATCH 9/9] i2c: qcom-cci: add sm8450 compatible

[Wolfram Sang]

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

On Thu, Feb 03, 2022 at 06:47:13PM +0200, Vladimir Zapolskiy wrote:
> Add QCOM SM8450 specific compatible for CCI controller, which is
> equal to CCI controllers found on QCOM SDM845 and QCOM SM8250 SoCs.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org>

Applied to for-next, thanks!


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

Re: [PATCH 0/9] i2c: qcom-cci: fixes and updates

[Wolfram Sang]

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


> thank you for applying the fixes, 1/9 and 9/9 are also good to be applied
> for-next, there is no dependency on vbus-supply, so I would appreciate, if
> you take two more changes.

Done now.

> As you suggested I'd start working on a generic support of such an optional
> bus supplier property, I believe at the moment everything is quite clear,
> I'll start from testing the previous solution, however my preference is
> to connect regulator on/off to master controller pm ops rather than slave
> pm ops. Additionally I am not quite satisfied with 'vbus-supply' name,

Did I get this right? You want to reimplement bus regulator handling in
the bus driver when we already have pending patches to add it to the I2C
core?


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