i2c: mux: pca954x: Support multiple devices on a single reset line

PATCH In review Eddie James 3 participants 10 messages Jul 27, 2021 – Aug 4, 2021

Subsystems (3)

Device tree bindings I2C subsystem host drivers I2C muxes

lore

Share & tools

lore

Eddie James

author

2021-07-27 16:03 UTC #0 lore raw

v2 cover i2c: mux: pca954x: Support multiple devices on a single reset line

Some systems connect several PCA954x devices to a single reset GPIO. For these devices to get out of reset and probe successfully, each device must defer the probe until the GPIO has been hogged. Accomplish this by attempting to grab a new "reset-shared-hogged" devicetree property, but expect it to fail with EPROBE_DEFER or EBUSY.

Changes since v1:
- Rework the patch to use a new devicetree property that we don't expect to successfully obtain

Eddie James (2):
dt-bindings: i2c: i2c-mux-pca954x: Define the reset-shared-hogged gpio
i2c: mux: pca954x: Support multiple devices on a single reset line

 .../bindings/i2c/i2c-mux-pca954x.yaml         |  5 ++
 drivers/i2c/muxes/i2c-mux-pca954x.c           | 46 +++++++++++++++----
 2 files changed, 42 insertions(+), 9 deletions(-)

Signature · 1 line

2.27.0

Eddie James

author

2021-07-27 16:03 UTC #1 lore raw

PATCH v2 1/2 dt-bindings: i2c: i2c-mux-pca954x: Define the reset-shared-hogged gpio

Add a property to describe a reset input that is shared between multiple PCA953x muxes.

Signed-off-by Eddie James <[email protected]>

---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml | 5 +++++
 1 file changed, 5 insertions(+)

Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml +5 -0

index 9f1726d0356b..06b9f8385bd7 100644
@@ -49,6 +49,11 @@ properties:
   reset-gpios:
     maxItems: 1
 
+  reset-shared-hogged-gpios:
+    maxItems: 1
+    description: Reference to a shared, hogged GPIO connected to the reset
+      input of several PCA954x muxes.
+
   i2c-mux-idle-disconnect:
     type: boolean
     description: Forces mux to disconnect all children in idle state. This is

Signature · 1 line

2.27.0

Eddie James

author

2021-07-27 16:09 UTC #2 lore raw

PATCH v2 2/2 i2c: mux: pca954x: Support multiple devices on a single reset line

In reply to Eddie James

Signed-off-by Eddie James <[email protected]>

---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 9 deletions(-)

drivers/i2c/muxes/i2c-mux-pca954x.c +37 -9

index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, muxc);
 	data->client = client;
 
-	/* Reset the mux if a reset GPIO is specified. */
-	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
-	if (gpio) {
-		udelay(1);
-		gpiod_set_value_cansleep(gpio, 0);
-		/* Give the chip some time to recover. */
-		udelay(1);
+	/*
+	 * Grab the shared, hogged gpio that controls the mux reset. We expect
+	 * this to fail with either EPROBE_DEFER or EBUSY. The only purpose of
+	 * trying to get it is to make sure the gpio controller has probed up
+	 * and hogged the line to take the mux out of reset, meaning that the
+	 * mux is ready to be probed up. Don't try and set the line any way; in
+	 * the event we actually successfully get the line (if it wasn't
+	 * hogged) then we immediately release it, since there is no way to
+	 * sync up the line between muxes.
+	 */
+	gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+	if (IS_ERR(gpio)) {
+		ret = PTR_ERR(gpio);
+		if (ret != -EBUSY)
+			return ret;
+	} else {
+		if (gpio) {
+			/* This is really a problem since now we don't know the
+			 * state of the gpio. Log a warning and keep trying to
+			 * probe the mux just in case it works.
+			 */
+			dev_warn(dev, "got hogged reset line, expect error\n");
+			gpiod_put(gpio);
+		} else {
+			/* Reset the mux if a reset GPIO is specified. */
+			gpio = devm_gpiod_get_optional(dev, "reset",
+						       GPIOD_OUT_HIGH);
+			if (IS_ERR(gpio))
+				return PTR_ERR(gpio);
+
+			if (gpio) {
+				udelay(1);
+				gpiod_set_value_cansleep(gpio, 0);
+				/* Give the chip some time to recover. */
+				udelay(1);
+			}
+		}
 	}
 
 	data->chip = device_get_match_data(dev);

Signature · 1 line

2.27.0

Rob Herring

maintainer

2021-08-02 20:46 UTC #3 lore raw

Quoted 33 lines · Eddie James

Signed-off-by Eddie James <[email protected]>

---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46 +++++++++++++++++++++++------
 1 file changed, 37 insertions(+), 9 deletions(-)

drivers/i2c/muxes/i2c-mux-pca954x.c +5 -9

index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct i2c_client *client,
 	i2c_set_clientdata(client, muxc);
 	data->client = client;
 
-	/* Reset the mux if a reset GPIO is specified. */
-	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
-	if (gpio) {
-		udelay(1);
-		gpiod_set_value_cansleep(gpio, 0);
-		/* Give the chip some time to recover. */
-		udelay(1);
+	/*
+	 * Grab the shared, hogged gpio that controls the mux reset. We expect
+	 * this to fail with either EPROBE_DEFER or EBUSY. The only purpose of
+	 * trying to get it is to make sure the gpio controller has probed up
+	 * and hogged the line to take the mux out of reset, meaning that the

drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451

Quoted 10 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +10 -0

+	 * mux is ready to be probed up. Don't try and set the line any way; in
+	 * the event we actually successfully get the line (if it wasn't
+	 * hogged) then we immediately release it, since there is no way to
+	 * sync up the line between muxes.
+	 */
+	gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+	if (IS_ERR(gpio)) {
+		ret = PTR_ERR(gpio);
+		if (ret != -EBUSY)
+			return ret;

Why can't you just do this with the existing 'reset-gpios' property? What's the usecase where you'd want to fail probe because EBUSY other than an error in your DT.

Quoted 29 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +22 -0

+	} else {
+		if (gpio) {
+			/* This is really a problem since now we don't know the
+			 * state of the gpio. Log a warning and keep trying to
+			 * probe the mux just in case it works.
+			 */
+			dev_warn(dev, "got hogged reset line, expect error\n");
+			gpiod_put(gpio);
+		} else {
+			/* Reset the mux if a reset GPIO is specified. */
+			gpio = devm_gpiod_get_optional(dev, "reset",
+						       GPIOD_OUT_HIGH);
+			if (IS_ERR(gpio))
+				return PTR_ERR(gpio);
+
+			if (gpio) {
+				udelay(1);
+				gpiod_set_value_cansleep(gpio, 0);
+				/* Give the chip some time to recover. */
+				udelay(1);
+			}
+		}
 	}
 
 	data->chip = device_get_match_data(dev);

Signature · 1 line

2.27.0

Eddie James

author

2021-08-02 21:51 UTC #4 lore raw

Quoted 54 lines · Rob Herring

Quoted 53 lines · Eddie James
Some systems connect several PCA954x devices to a single reset GPIO. For
these devices to get out of reset and probe successfully, each device must
defer the probe until the GPIO has been hogged. Accomplish this by attempting to grab a new "reset-shared-hogged" devicetree property, but
expect it to fail with EPROBE_DEFER or EBUSY.
Signed-off-by Eddie James <[email protected]>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46 +++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -9
index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct i2c_client
 *client,
 	i2c_set_clientdata(client, muxc);
 	data->client = client;
 
-	/* Reset the mux if a reset GPIO is specified. */
-	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
-	if (gpio) {
-		udelay(1);
-		gpiod_set_value_cansleep(gpio, 0);
-		/* Give the chip some time to recover. */
-		udelay(1);
+	/*
+	 * Grab the shared, hogged gpio that controls the mux reset. We
expect
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * this to fail with either EPROBE_DEFER or EBUSY. The only
purpose of
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * trying to get it is to make sure the gpio controller has
probed up
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * and hogged the line to take the mux out of reset, meaning
that the
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * mux is ready to be probed up. Don't try and set the line any
way; in
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * the event we actually successfully get the line (if it
wasn't
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * hogged) then we immediately release it, since there is no
way to
drivers/i2c/muxes/i2c-mux-pca954x.c +4 -0
+	 * sync up the line between muxes.
+	 */
+	gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+	if (IS_ERR(gpio)) {

Quoted 8 lines

drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451
Quoted 3 lines
drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0
+		ret = PTR_ERR(gpio);
+		if (ret != -EBUSY)
+			return ret;
Why can't you just do this with the existing 'reset-gpios' property? What's the usecase where you'd want to fail probe because EBUSY other
than an error in your DT.

Hi, thanks for the reply.

Are you suggesting I use "reset-gpios" and change the driver to ignore EBUSY? I don't know any other usecase, I just didn't think it would be acceptable to ignore EBUSY on that, but perhaps it is a better solution.

Thanks,
Eddie

Quoted 35 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+	} else {
+		if (gpio) {
+			/* This is really a problem since now we don't

know the

drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0

+			 * state of the gpio. Log a warning and keep

trying to

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+			 * probe the mux just in case it works.
+			 */
+			dev_warn(dev, "got hogged reset line, expect

error\n");

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+			gpiod_put(gpio);
+		} else {
+			/* Reset the mux if a reset GPIO is specified.

drivers/i2c/muxes/i2c-mux-pca954x.c +9 -0

+			gpio = devm_gpiod_get_optional(dev, "reset",
+						       GPIOD_OUT_HIGH);
+			if (IS_ERR(gpio))
+				return PTR_ERR(gpio);
+
+			if (gpio) {
+				udelay(1);
+				gpiod_set_value_cansleep(gpio, 0);
+				/* Give the chip some time to recover.

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+				udelay(1);
+			}
+		}
 	}
 
 	data->chip = device_get_match_data(dev);

Signature · 1 line

2.27.0

Rob Herring

maintainer

2021-08-02 22:35 UTC #5 lore raw

Quoted 65 lines · Eddie James

Quoted 54 lines · Rob Herring
Quoted 53 lines · Eddie James
Some systems connect several PCA954x devices to a single reset GPIO. For
these devices to get out of reset and probe successfully, each device must
defer the probe until the GPIO has been hogged. Accomplish this by attempting to grab a new "reset-shared-hogged" devicetree property, but
expect it to fail with EPROBE_DEFER or EBUSY.
Signed-off-by Eddie James <[email protected]>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46 +++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -9
index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct i2c_client
 *client,
    i2c_set_clientdata(client, muxc);
    data->client = client;
 
-   /* Reset the mux if a reset GPIO is specified. */
-   gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-   if (IS_ERR(gpio))
-           return PTR_ERR(gpio);
-   if (gpio) {
-           udelay(1);
-           gpiod_set_value_cansleep(gpio, 0);
-           /* Give the chip some time to recover. */
-           udelay(1);
+   /*
+    * Grab the shared, hogged gpio that controls the mux reset. We
expect
+ * this to fail with either EPROBE_DEFER or EBUSY. The only
purpose of
+ * trying to get it is to make sure the gpio controller has
probed up
+ * and hogged the line to take the mux out of reset, meaning
that the
+ * mux is ready to be probed up. Don't try and set the line any
way; in
+ * the event we actually successfully get the line (if it
wasn't
+ * hogged) then we immediately release it, since there is no
way to
drivers/i2c/muxes/i2c-mux-pca954x.c +4 -0
+    * sync up the line between muxes.
+    */
+   gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+   if (IS_ERR(gpio)) {
Quoted 8 lines
drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451
Quoted 3 lines
drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0
+           ret = PTR_ERR(gpio);
+           if (ret != -EBUSY)
+                   return ret;
Why can't you just do this with the existing 'reset-gpios' property? What's the usecase where you'd want to fail probe because EBUSY other
than an error in your DT.

Quoted 6 lines

Hi, thanks for the reply.
Are you suggesting I use "reset-gpios" and change the driver to ignore EBUSY? I don't know any other usecase, I just didn't think it would be acceptable to ignore EBUSY on that, but perhaps it is a better solution.

Yes. I'm assuming that's the only way EBUSY is returned? A DT already contains everything needed to know multiple 'reset-gpios' are sharing a GPIO line. It's a kernel problem to figure that out.

(Really, I'd like to make 'reset-gpios' be handled by the reset subsystem which handles shared resets already. Then 'reset-gpios' or 'resets' could be used in nodes and drivers don't know the difference.)

Rob

Peter Rosin

2021-08-04 07:50 UTC #6 lore raw

In reply to Eddie James

Quoted 64 lines · Eddie James

Quoted 54 lines · Rob Herring
Quoted 53 lines · Eddie James
Some systems connect several PCA954x devices to a single reset GPIO. For
these devices to get out of reset and probe successfully, each device must
defer the probe until the GPIO has been hogged. Accomplish this by attempting to grab a new "reset-shared-hogged" devicetree property, but
expect it to fail with EPROBE_DEFER or EBUSY.
Signed-off-by Eddie James <[email protected]>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46 +++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -9
index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct i2c_client
 *client,
 	i2c_set_clientdata(client, muxc);
 	data->client = client;
 
-	/* Reset the mux if a reset GPIO is specified. */
-	gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(gpio))
-		return PTR_ERR(gpio);
-	if (gpio) {
-		udelay(1);
-		gpiod_set_value_cansleep(gpio, 0);
-		/* Give the chip some time to recover. */
-		udelay(1);
+	/*
+	 * Grab the shared, hogged gpio that controls the mux reset. We
expect
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * this to fail with either EPROBE_DEFER or EBUSY. The only
purpose of
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * trying to get it is to make sure the gpio controller has
probed up
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * and hogged the line to take the mux out of reset, meaning
that the
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * mux is ready to be probed up. Don't try and set the line any
way; in
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * the event we actually successfully get the line (if it
wasn't
drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0
+	 * hogged) then we immediately release it, since there is no
way to
drivers/i2c/muxes/i2c-mux-pca954x.c +4 -0
+	 * sync up the line between muxes.
+	 */
+	gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+	if (IS_ERR(gpio)) {
Quoted 8 lines
drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451
Quoted 3 lines
drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0
+		ret = PTR_ERR(gpio);
+		if (ret != -EBUSY)
+			return ret;
Why can't you just do this with the existing 'reset-gpios' property? What's the usecase where you'd want to fail probe because EBUSY other
than an error in your DT.

Quoted 6 lines

Hi, thanks for the reply.
Are you suggesting I use "reset-gpios" and change the driver to ignore EBUSY? I don't know any other usecase, I just didn't think it would be acceptable to ignore EBUSY on that, but perhaps it is a better solution.

Hi!

From a device-tree point of view that might seem simple. But it becomes a mess when several driver instances need to coordinate. If one instance is grabbing the reset line but is then stalled while other instances race ahead, they might be clobbered by a late reset from the stalled first instance.

And while it might be possible to arrange the code such that those dragons are dodged and that the reset is properly coordinated, what if the gpio is supposed to be shared with some other totally unrelated driver? It might seem to work when everything is normal, but as soon as anything out of the ordinary happens, all bets are off. I expect subtle problems in the furture.

I see no simple solution to this, and I also expect that if gpios need to be shared, there will eventually need to be some kind of layer that helps with coordination such that it becomes explicit rather than implicit and fragile.

Cheers,
Peter

Quoted 39 lines

Thanks,
Eddie

Quoted 35 lines

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+	} else {
+		if (gpio) {
+			/* This is really a problem since now we don't

know the

drivers/i2c/muxes/i2c-mux-pca954x.c +1 -0

+			 * state of the gpio. Log a warning and keep

trying to

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+			 * probe the mux just in case it works.
+			 */
+			dev_warn(dev, "got hogged reset line, expect

error\n");

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+			gpiod_put(gpio);
+		} else {
+			/* Reset the mux if a reset GPIO is specified.

drivers/i2c/muxes/i2c-mux-pca954x.c +9 -0

+			gpio = devm_gpiod_get_optional(dev, "reset",
+						       GPIOD_OUT_HIGH);
+			if (IS_ERR(gpio))
+				return PTR_ERR(gpio);
+
+			if (gpio) {
+				udelay(1);
+				gpiod_set_value_cansleep(gpio, 0);
+				/* Give the chip some time to recover.

drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0

+				udelay(1);
+			}
+		}
 	}
 
 	data->chip = device_get_match_data(dev);

Signature · 1 line

2.27.0

Signature · 3 lines

Peter Rosin
+46 730 746 224
Axentia Technologies AB

Rob Herring

maintainer

2021-08-04 13:28 UTC #7 lore raw

Quoted 81 lines · Peter Rosin

Quoted 64 lines · Eddie James
Quoted 54 lines · Rob Herring
Quoted 53 lines · Eddie James
Some systems connect several PCA954x devices to a single reset GPIO. For
these devices to get out of reset and probe successfully, each device must
defer the probe until the GPIO has been hogged. Accomplish this by attempting to grab a new "reset-shared-hogged" devicetree property, but
expect it to fail with EPROBE_DEFER or EBUSY.
Signed-off-by Eddie James <[email protected]>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46 +++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -9
index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct i2c_client
 *client,
    i2c_set_clientdata(client, muxc);
    data->client = client;
 
-   /* Reset the mux if a reset GPIO is specified. */
-   gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-   if (IS_ERR(gpio))
-           return PTR_ERR(gpio);
-   if (gpio) {
-           udelay(1);
-           gpiod_set_value_cansleep(gpio, 0);
-           /* Give the chip some time to recover. */
-           udelay(1);
+   /*
+    * Grab the shared, hogged gpio that controls the mux reset. We
expect
+ * this to fail with either EPROBE_DEFER or EBUSY. The only
purpose of
+ * trying to get it is to make sure the gpio controller has
probed up
+ * and hogged the line to take the mux out of reset, meaning
that the
+ * mux is ready to be probed up. Don't try and set the line any
way; in
+ * the event we actually successfully get the line (if it
wasn't
+ * hogged) then we immediately release it, since there is no
way to
drivers/i2c/muxes/i2c-mux-pca954x.c +4 -0
+    * sync up the line between muxes.
+    */
+   gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+   if (IS_ERR(gpio)) {
Quoted 8 lines
drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451
Quoted 3 lines
drivers/i2c/muxes/i2c-mux-pca954x.c +3 -0
+           ret = PTR_ERR(gpio);
+           if (ret != -EBUSY)
+                   return ret;
Why can't you just do this with the existing 'reset-gpios' property? What's the usecase where you'd want to fail probe because EBUSY other
than an error in your DT.
Quoted 6 lines
Hi, thanks for the reply.
Are you suggesting I use "reset-gpios" and change the driver to ignore EBUSY? I don't know any other usecase, I just didn't think it would be acceptable to ignore EBUSY on that, but perhaps it is a better solution.
Hi!
From a device-tree point of view that might seem simple. But it becomes a mess when several driver instances need to coordinate. If one instance is grabbing the reset line but is then stalled while other instances race ahead, they might be clobbered by a late reset from the stalled first instance.

Quoted 6 lines

And while it might be possible to arrange the code such that those dragons are dodged and that the reset is properly coordinated, what if the gpio is supposed to be shared with some other totally unrelated driver? It might seem to work when everything is normal, but as soon as anything out of the ordinary happens, all bets are off. I expect subtle problems in the furture.

All of this is true, but a different reset GPIO property name does nothing to solve it.

Quoted 4 lines

I see no simple solution to this, and I also expect that if gpios need to be shared, there will eventually need to be some kind of layer that helps with coordination such that it becomes explicit rather than implicit and fragile.

Yes, like making the reset subsystem handle 'reset-gpios' properties as I suggested.

Rob

Eddie James

author

2021-08-04 15:12 UTC #8 lore raw

Quoted 91 lines · Rob Herring

Quoted 90 lines · Peter Rosin
Quoted 78 lines · Eddie James
Quoted 68 lines · Rob Herring
Quoted 67 lines · Eddie James
Some systems connect several PCA954x devices to a single reset
GPIO. For
these devices to get out of reset and probe successfully, each
device must
defer the probe until the GPIO has been hogged. Accomplish this by
attempting to grab a new "reset-shared-hogged" devicetree property,
but
expect it to fail with EPROBE_DEFER or EBUSY.
Signed-off-by Eddie James <[email protected]>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46
+++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)
drivers/i2c/muxes/i2c-mux-pca954x.c +0 -0
index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct
i2c_client
*client,
    i2c_set_clientdata(client, muxc);
    data->client = client;
drivers/i2c/muxes/i2c-mux-pca954x.c +0 -2
-   /* Reset the mux if a reset GPIO is specified. */
-   gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -7
-   if (IS_ERR(gpio))
-           return PTR_ERR(gpio);
-   if (gpio) {
-           udelay(1);
-           gpiod_set_value_cansleep(gpio, 0);
-           /* Give the chip some time to recover. */
-           udelay(1);
+   /*
+    * Grab the shared, hogged gpio that controls the mux
reset. We
expect
+ * this to fail with either EPROBE_DEFER or EBUSY. The
only
purpose of
+ * trying to get it is to make sure the gpio controller
has
probed up
+ * and hogged the line to take the mux out of reset,
meaning
that the
+ * mux is ready to be probed up. Don't try and set the
line any
way; in
+ * the event we actually successfully get the line (if it
wasn't
+ * hogged) then we immediately release it, since there is
no
way to
drivers/i2c/muxes/i2c-mux-pca954x.c +5 -0
+    * sync up the line between muxes.
+    */
+   gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+   if (IS_ERR(gpio)) {
+           ret = PTR_ERR(gpio);
Quoted 8 lines
drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451
Quoted 2 lines
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -0
+           if (ret != -EBUSY)
+                   return ret;
Why can't you just do this with the existing 'reset-gpios' property?
What's the usecase where you'd want to fail probe because EBUSY other
than an error in your DT.
Quoted 8 lines
Hi, thanks for the reply.
Are you suggesting I use "reset-gpios" and change the driver to ignore
EBUSY? I don't know any other usecase, I just didn't think it would be
acceptable to ignore EBUSY on that, but perhaps it is a better solution.
Hi!

Quoted 9 lines

Quoted 9 lines
From a device-tree point of view that might seem simple. But it becomes
a mess when several driver instances need to coordinate. If one instance
is grabbing the reset line but is then stalled while other instances
race ahead, they might be clobbered by a late reset from the stalled
first instance.

Hi,

Well this isn't a concern if the line is hogged - once it's hogged it shouldn't change, and all driver instances should get the same thing - EBUSY. Before it's hogged all driver instances should get EPROBE_DEFER.

Quoted 11 lines

Quoted 11 lines
And while it might be possible to arrange the code such that those dragons
are dodged and that the reset is properly coordinated, what if the gpio is
supposed to be shared with some other totally unrelated driver? It might
seem to work when everything is normal, but as soon as anything out of the
ordinary happens, all bets are off. I expect subtle problems in the furture.

Unless another driver uses the non-exclusive flag (which can cause many subtle problems in all sorts of areas anyway), the GPIO shouldn't change. Now, the driver that does the GPIO hogging might go away, which definitely would be a problem. I suppose I feel it's an error path anyway, so all bets are off for dependent devices.

Quoted 3 lines

All of this is true, but a different reset GPIO property name does nothing to solve it.

This is part of why I chose a new property that specifically indicates that it's hogged.

Quoted 10 lines

Quoted 7 lines
I see no simple solution to this, and I also expect that if gpios need
to be shared, there will eventually need to be some kind of layer that
helps with coordination such that it becomes explicit rather than implicit and fragile.
Yes, like making the reset subsystem handle 'reset-gpios' properties as I suggested.

That would be nice... if anyone has the time for such an extensive addition :(

Thanks,
Eddie

Quoted 2 lines

Rob

Rob Herring

maintainer

2021-08-04 16:35 UTC #9 lore raw

Quoted 128 lines · Eddie James

Quoted 91 lines · Rob Herring
Quoted 90 lines · Peter Rosin
Quoted 78 lines · Eddie James
Quoted 68 lines · Rob Herring
Quoted 67 lines · Eddie James
Some systems connect several PCA954x devices to a single reset
GPIO. For
these devices to get out of reset and probe successfully, each
device must
defer the probe until the GPIO has been hogged. Accomplish this by
attempting to grab a new "reset-shared-hogged" devicetree property,
but
expect it to fail with EPROBE_DEFER or EBUSY.
Signed-off-by Eddie James <[email protected]>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46
+++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)
drivers/i2c/muxes/i2c-mux-pca954x.c +0 -0
index 4ad665757dd8..376b54ffb590 100644
@@ -434,15 +434,43 @@ static int pca954x_probe(struct
i2c_client
*client,
    i2c_set_clientdata(client, muxc);
    data->client = client;
drivers/i2c/muxes/i2c-mux-pca954x.c +0 -2
-   /* Reset the mux if a reset GPIO is specified. */
-   gpio = devm_gpiod_get_optional(dev, "reset",
GPIOD_OUT_HIGH);
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -7
-   if (IS_ERR(gpio))
-           return PTR_ERR(gpio);
-   if (gpio) {
-           udelay(1);
-           gpiod_set_value_cansleep(gpio, 0);
-           /* Give the chip some time to recover. */
-           udelay(1);
+   /*
+    * Grab the shared, hogged gpio that controls the mux
reset. We
expect
+ * this to fail with either EPROBE_DEFER or EBUSY. The
only
purpose of
+ * trying to get it is to make sure the gpio controller
has
probed up
+ * and hogged the line to take the mux out of reset,
meaning
that the
+ * mux is ready to be probed up. Don't try and set the
line any
way; in
+ * the event we actually successfully get the line (if it
wasn't
+ * hogged) then we immediately release it, since there is
no
way to
drivers/i2c/muxes/i2c-mux-pca954x.c +5 -0
+    * sync up the line between muxes.
+    */
+   gpio = gpiod_get_optional(dev, "reset-shared-hogged", 0);
+   if (IS_ERR(gpio)) {
+           ret = PTR_ERR(gpio);
Quoted 8 lines
drivers/i2c/muxes/i2c-mux-pca954x.c static int pca954x_probe(struct i2c_client *client, · line 451
Quoted 2 lines
drivers/i2c/muxes/i2c-mux-pca954x.c +2 -0
+           if (ret != -EBUSY)
+                   return ret;
Why can't you just do this with the existing 'reset-gpios' property?
What's the usecase where you'd want to fail probe because EBUSY other
than an error in your DT.
Quoted 8 lines
Hi, thanks for the reply.
Are you suggesting I use "reset-gpios" and change the driver to ignore
EBUSY? I don't know any other usecase, I just didn't think it would be
acceptable to ignore EBUSY on that, but perhaps it is a better solution.
Hi!
Quoted 9 lines
Quoted 9 lines
From a device-tree point of view that might seem simple. But it becomes
a mess when several driver instances need to coordinate. If one instance
is grabbing the reset line but is then stalled while other instances
race ahead, they might be clobbered by a late reset from the stalled
first instance.
Hi,
Well this isn't a concern if the line is hogged - once it's hogged it shouldn't change, and all driver instances should get the same thing - EBUSY. Before it's hogged all driver instances should get EPROBE_DEFER.
Quoted 11 lines
Quoted 11 lines
And while it might be possible to arrange the code such that those dragons
are dodged and that the reset is properly coordinated, what if the gpio is
supposed to be shared with some other totally unrelated driver? It might
seem to work when everything is normal, but as soon as anything out of the
ordinary happens, all bets are off. I expect subtle problems in the furture.
Unless another driver uses the non-exclusive flag (which can cause many subtle problems in all sorts of areas anyway), the GPIO shouldn't change. Now, the driver that does the GPIO hogging might go away, which definitely would be a problem. I suppose I feel it's an error path anyway, so all bets are off for dependent devices.
Quoted 1 line

Quoted 5 lines

Quoted 2 lines
All of this is true, but a different reset GPIO property name does nothing to solve it.
This is part of why I chose a new property that specifically indicates that it's hogged.

I'm not really a fan of GPIO hog stuff and using it here definitely seems like a hack. I thought the purpose was for GPIOs not controlled elsewhere... Regardless, you still have all the information you need in DT already. We don't need to duplicate it. You know it is a hog and can also tell it is shared. Information doesn't have to be in the node associated with a driver.

Rob