linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/2] i2c: mux: pca954x: Support multiple devices on a single reset line
@ 2021-07-27 16:03 Eddie James
  2021-07-27 16:03 ` [PATCH v2 1/2] dt-bindings: i2c: i2c-mux-pca954x: Define the reset-shared-hogged gpio Eddie James
  2021-07-27 16:03 ` [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
  0 siblings, 2 replies; 10+ messages in thread
From: Eddie James @ 2021-07-27 16:03 UTC (permalink / raw)
  To: linux-i2c; +Cc: devicetree, linux-kernel, peda, robh+dt, 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.

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(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 1/2] dt-bindings: i2c: i2c-mux-pca954x: Define the reset-shared-hogged gpio
  2021-07-27 16:03 [PATCH v2 0/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
@ 2021-07-27 16:03 ` Eddie James
  2021-07-27 16:03 ` [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
  1 sibling, 0 replies; 10+ messages in thread
From: Eddie James @ 2021-07-27 16:03 UTC (permalink / raw)
  To: linux-i2c; +Cc: devicetree, linux-kernel, peda, robh+dt, Eddie James

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

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
index 9f1726d0356b..06b9f8385bd7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.yaml
@@ -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
-- 
2.27.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-07-27 16:03 [PATCH v2 0/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
  2021-07-27 16:03 ` [PATCH v2 1/2] dt-bindings: i2c: i2c-mux-pca954x: Define the reset-shared-hogged gpio Eddie James
@ 2021-07-27 16:03 ` Eddie James
  2021-08-02 20:46   ` Rob Herring
  1 sibling, 1 reply; 10+ messages in thread
From: Eddie James @ 2021-07-27 16:03 UTC (permalink / raw)
  To: linux-i2c; +Cc: devicetree, linux-kernel, peda, robh+dt, 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 <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;
+	} 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);
-- 
2.27.0


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-07-27 16:03 ` [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
@ 2021-08-02 20:46   ` Rob Herring
  2021-08-02 21:51     ` Eddie James
  0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2021-08-02 20:46 UTC (permalink / raw)
  To: Eddie James; +Cc: linux-i2c, devicetree, linux-kernel, peda

On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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, 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.

> +	} 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);
> -- 
> 2.27.0
> 
> 

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-08-02 20:46   ` Rob Herring
@ 2021-08-02 21:51     ` Eddie James
  2021-08-02 22:35       ` Rob Herring
  2021-08-04  7:50       ` Peter Rosin
  0 siblings, 2 replies; 10+ messages in thread
From: Eddie James @ 2021-08-02 21:51 UTC (permalink / raw)
  To: Rob Herring; +Cc: linux-i2c, devicetree, linux-kernel, peda

On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:
> On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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, 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.

Thanks,
Eddie

> 
> > +	} 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);
> > -- 
> > 2.27.0
> > 
> > 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-08-02 21:51     ` Eddie James
@ 2021-08-02 22:35       ` Rob Herring
  2021-08-04  7:50       ` Peter Rosin
  1 sibling, 0 replies; 10+ messages in thread
From: Rob Herring @ 2021-08-02 22:35 UTC (permalink / raw)
  To: Eddie James; +Cc: Linux I2C, devicetree, linux-kernel, Peter Rosin

On Mon, Aug 2, 2021 at 3:51 PM Eddie James <eajames@linux.ibm.com> wrote:
>
> On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:
> > On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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, 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.

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-08-02 21:51     ` Eddie James
  2021-08-02 22:35       ` Rob Herring
@ 2021-08-04  7:50       ` Peter Rosin
  2021-08-04 13:28         ` Rob Herring
  1 sibling, 1 reply; 10+ messages in thread
From: Peter Rosin @ 2021-08-04  7:50 UTC (permalink / raw)
  To: Eddie James, Rob Herring; +Cc: linux-i2c, devicetree, linux-kernel

On 2021-08-02 23:51, Eddie James wrote:
> On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:
>> On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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, 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.

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

> Thanks,
> Eddie
> 
>>
>>> +	} 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);
>>> -- 
>>> 2.27.0
>>>
>>>
> 

-- 
Peter Rosin
+46 730 746 224
Axentia Technologies AB

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-08-04  7:50       ` Peter Rosin
@ 2021-08-04 13:28         ` Rob Herring
  2021-08-04 15:12           ` Eddie James
  0 siblings, 1 reply; 10+ messages in thread
From: Rob Herring @ 2021-08-04 13:28 UTC (permalink / raw)
  To: Peter Rosin; +Cc: Eddie James, Linux I2C, devicetree, linux-kernel

On Wed, Aug 4, 2021 at 1:50 AM Peter Rosin <peda@axentia.se> wrote:
>
> On 2021-08-02 23:51, Eddie James wrote:
> > On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:
> >> On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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, 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.
>
> 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.

> 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-08-04 13:28         ` Rob Herring
@ 2021-08-04 15:12           ` Eddie James
  2021-08-04 16:35             ` Rob Herring
  0 siblings, 1 reply; 10+ messages in thread
From: Eddie James @ 2021-08-04 15:12 UTC (permalink / raw)
  To: Rob Herring, Peter Rosin; +Cc: Linux I2C, devicetree, linux-kernel

On Wed, 2021-08-04 at 07:28 -0600, Rob Herring wrote:
> On Wed, Aug 4, 2021 at 1:50 AM Peter Rosin <peda@axentia.se> wrote:
> > On 2021-08-02 23:51, Eddie James wrote:
> > > On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:
> > > > On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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,
> > > > > 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.

> > 
> > 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.

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

> 
> Rob


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line
  2021-08-04 15:12           ` Eddie James
@ 2021-08-04 16:35             ` Rob Herring
  0 siblings, 0 replies; 10+ messages in thread
From: Rob Herring @ 2021-08-04 16:35 UTC (permalink / raw)
  To: Eddie James; +Cc: Peter Rosin, Linux I2C, devicetree, linux-kernel

On Wed, Aug 4, 2021 at 9:12 AM Eddie James <eajames@linux.ibm.com> wrote:
>
> On Wed, 2021-08-04 at 07:28 -0600, Rob Herring wrote:
> > On Wed, Aug 4, 2021 at 1:50 AM Peter Rosin <peda@axentia.se> wrote:
> > > On 2021-08-02 23:51, Eddie James wrote:
> > > > On Mon, 2021-08-02 at 14:46 -0600, Rob Herring wrote:
> > > > > On Tue, Jul 27, 2021 at 11:03:15AM -0500, 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,
> > > > > > 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.
>
> > >
> > > 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.
>
> >
> > 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

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2021-08-04 16:35 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-27 16:03 [PATCH v2 0/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
2021-07-27 16:03 ` [PATCH v2 1/2] dt-bindings: i2c: i2c-mux-pca954x: Define the reset-shared-hogged gpio Eddie James
2021-07-27 16:03 ` [PATCH v2 2/2] i2c: mux: pca954x: Support multiple devices on a single reset line Eddie James
2021-08-02 20:46   ` Rob Herring
2021-08-02 21:51     ` Eddie James
2021-08-02 22:35       ` Rob Herring
2021-08-04  7:50       ` Peter Rosin
2021-08-04 13:28         ` Rob Herring
2021-08-04 15:12           ` Eddie James
2021-08-04 16:35             ` Rob Herring

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).