On Wed, Aug 4, 2021 at 9:12 AM Eddie James [off-list ref] wrote:

On Wed, 2021-08-04 at 07:28 -0600, Rob Herring wrote:

quoted

On Wed, Aug 4, 2021 at 1:50 AM Peter Rosin [off-list ref] wrote:

quoted

On 2021-08-02 23:51, Eddie James wrote:

quoted

On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:

quoted

On Tue, Jul 27, 2021 at 11:03:15AM -0500, Eddie James wrote:

quoted

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 <eajames@linux.ibm.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 46
+++++++++++++++++++++++
------
 1 file changed, 37 insertions(+), 9 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c

b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 4ad665757dd8..376b54ffb590 100644

--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c

@@ -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;

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.

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.

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

quoted

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

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