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

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

[Eddie James]

Some systems connect several PCA954x devices to a single reset GPIO. For
these devices to get out of reset and probe successfully, only the first
device probed should change the GPIO. Add this functionality by checking
for EBUSY when getting the GPIO fails. Then, retry getting the GPIO with
the non-exclusive flag and wait for the reset line to drop. This prevents
the later probes from proceding while the device is still reset.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 43 +++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 4ad665757dd8..840667a82f71 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -39,6 +39,7 @@
 #include <linux/i2c-mux.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
+#include <linux/ktime.h>
 #include <linux/module.h>
 #include <linux/pm.h>
 #include <linux/property.h>
@@ -414,6 +415,8 @@ static int pca954x_init(struct i2c_client *client, struct pca954x *data)
 static int pca954x_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
+	enum gpiod_flags flags = GPIOD_OUT_HIGH;
+	const char *reset_gpio_name = "reset";
 	struct i2c_adapter *adap = client->adapter;
 	struct device *dev = &client->dev;
 	struct gpio_desc *gpio;
@@ -435,9 +438,43 @@ static int pca954x_probe(struct i2c_client *client,
 	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);
+	gpio = devm_gpiod_get_optional(dev, reset_gpio_name, flags);
+	if (IS_ERR(gpio)) {
+		ret = PTR_ERR(gpio);
+		/*
+		 * In the case that multiple muxes share a single reset line,
+		 * only one should toggle the reset. The other muxes should
+		 * continue probing, waiting for the reset line to drop.
+		 */
+		if (ret == -EBUSY) {
+			ktime_t exp;
+
+			flags |= GPIOD_FLAGS_BIT_NONEXCLUSIVE;
+			gpio = gpiod_get(dev, reset_gpio_name, flags);
+			if (IS_ERR(gpio))
+				return PTR_ERR(gpio);
+
+			exp = ktime_add_us(ktime_get(), 1000);
+			do {
+				ret = gpiod_get_value_cansleep(gpio);
+				if (ret <= 0)
+					break;
+				usleep_range(5, 50);
+			} while (ktime_before(ktime_get(), exp));
+
+			gpiod_put(gpio);
+			if (ret) {
+				if (ret > 0)
+					ret = -ETIMEDOUT;
+
+				return ret;
+			}
+
+			gpio = NULL;
+		} else {
+			return ret;
+		}
+	}
 	if (gpio) {
 		udelay(1);
 		gpiod_set_value_cansleep(gpio, 0);
-- 
2.27.0

Re: [PATCH] i2c: mux: pca954x: Support multiple devices on a single reset line

[Peter Rosin]

Hi!

On 2021-05-05 23:59, Eddie James wrote:
> Some systems connect several PCA954x devices to a single reset GPIO. For
> these devices to get out of reset and probe successfully, only the first
> device probed should change the GPIO. Add this functionality by checking
> for EBUSY when getting the GPIO fails. Then, retry getting the GPIO with
> the non-exclusive flag and wait for the reset line to drop. This prevents
> the later probes from proceding while the device is still reset.

(nit: proceeding)

The patch assumes that all muxes with interconnected resets are only
ever reset "in symphony". But there is no guarantee anywhere that this
actually holds.

So, I don't see how this can be safe. Sure, it may very well work in the
majority of cases, but it seems very dangerous. If one instance resets
muxes controlled by other instances, any cached value is destroyed in
those instances and anything can happen. Sure, if you have HW like this,
then you have what you have. But I don't see any good way to handle
this case in an elegant way. If this scheme is allowed the dangers of
relying on it at minimum needs to be documented.

And what if the second instance reads the gpio just a few ns after the
reset is released? The first instance waits for 1us before proceeding
to give the chip some time to recover from the reset, but that respite
may be lost to other instances.

What if the first instance does the reset but then fails the probe later,
possibly because the chip isn't there, but then other instances manages
to time their probe just so that the gpio is busy at the right point,
and then proceeds without holding a reference to the gpio. Then the first
instance also lets go of the gpio and you end up with a bunch of instances
relying on a pin that noone holds a reference to. Or, yet another instance
enters the picture and finds the gpio free and pulls a reset behind the
back of the intermediate instances which have already proceeded.

Or am I reading something wrong?

Cheers,
Peter

Re: [PATCH] i2c: mux: pca954x: Support multiple devices on a single reset line

[Eddie James]

On Fri, 2021-05-07 at 00:08 +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-05 23:59, Eddie James wrote:
> > Some systems connect several PCA954x devices to a single reset
> > GPIO. For
> > these devices to get out of reset and probe successfully, only the
> > first
> > device probed should change the GPIO. Add this functionality by
> > checking
> > for EBUSY when getting the GPIO fails. Then, retry getting the GPIO
> > with
> > the non-exclusive flag and wait for the reset line to drop. This
> > prevents
> > the later probes from proceding while the device is still reset.
> 
> (nit: proceeding)
> 
> The patch assumes that all muxes with interconnected resets are only
> ever reset "in symphony". But there is no guarantee anywhere that
> this
> actually holds.

Thanks for your comments Peter, you are quite right, this won't do.
I've finally come back around to this and will send a new patch that
handles it differently - please let me know what you think.

Thanks!
Eddie

> 
> So, I don't see how this can be safe. Sure, it may very well work in
> the
> majority of cases, but it seems very dangerous. If one instance
> resets
> muxes controlled by other instances, any cached value is destroyed in
> those instances and anything can happen. Sure, if you have HW like
> this,
> then you have what you have. But I don't see any good way to handle
> this case in an elegant way. If this scheme is allowed the dangers of
> relying on it at minimum needs to be documented.
> 
> And what if the second instance reads the gpio just a few ns after
> the
> reset is released? The first instance waits for 1us before proceeding
> to give the chip some time to recover from the reset, but that
> respite
> may be lost to other instances.
> 
> What if the first instance does the reset but then fails the probe
> later,
> possibly because the chip isn't there, but then other instances
> manages
> to time their probe just so that the gpio is busy at the right point,
> and then proceeds without holding a reference to the gpio. Then the
> first
> instance also lets go of the gpio and you end up with a bunch of
> instances
> relying on a pin that noone holds a reference to. Or, yet another
> instance
> enters the picture and finds the gpio free and pulls a reset behind
> the
> back of the intermediate instances which have already proceeded.
> 
> Or am I reading something wrong?
> 
> Cheers,
> Peter
>