[PATCH v6 RESEND 0/2] i2c: muxes: pca954x: Reset if (de)select fails

[PATCH v6 RESEND 0/2] i2c: muxes: pca954x: Reset if (de)select fails

[Wojciech Siudy]

The pca954x mux might not respond under certain circumstances, like
device behind it holding SDA after recovery loop or some internal issue
in mux itself. Those situations are indicated by ETIMEDOUT returned
from I2C transaction attempting selecting or deselecting the channel.
According to device documentation the reset pulse restores I2C
subsystem of the mux and deselects the channel.

Since the mux switches using transistor switches, the failure of line
behind mux that is currently connected prevents sending commands to mux
itself, so external reset signal is required. 

The following series of patches removes legacy gpio-based reset calls,
because reset controller framework already uses reset-gpios property
and resets the device when channel selection or deselection fails.

Even through the current transaction will fail anyway, the next will
be possible to perform without resetting the whole system.

---
Changelog:
v2:
  * Removed mail header from the commit log
  * Decreased reset pulse hold time from 10 to 1 ms
v3:
  * Make this functionality enabled by appropriate property in
    devicetree
v4:
  * Fix missing condition check from devicetree
v5:
  * Declare dependency of a new property
v6:
  * Don't add any new properties, use reset controller only
---

Wojciech Siudy (2):
  i2c: muxes: pca954x: Use reset controller only
  i2c: muxes: pca954x: Reset if (de)select fails

 drivers/i2c/muxes/i2c-mux-pca954x.c | 49 ++++++++++++++++-------------
 1 file changed, 27 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v6 RESEND 1/2] i2c: muxes: pca954x: Use reset controller only

[Wojciech Siudy]

Fallback to legacy reset-gpios is no more needed, because the framework
can use reset-gpios properity now as well.

Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 16 +---------------
 1 file changed, 1 insertion(+), 15 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index db95113a5b49..e09230df7059 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -118,7 +118,6 @@ struct pca954x {
 	raw_spinlock_t lock;
 	struct regulator *supply;
 
-	struct gpio_desc *reset_gpio;
 	struct reset_control *reset_cont;
 };
 
@@ -528,17 +527,6 @@ static int pca954x_get_reset(struct device *dev, struct pca954x *data)
 	if (IS_ERR(data->reset_cont))
 		return dev_err_probe(dev, PTR_ERR(data->reset_cont),
 				     "Failed to get reset\n");
-	else if (data->reset_cont)
-		return 0;
-
-	/*
-	 * fallback to legacy reset-gpios
-	 */
-	data->reset_gpio = devm_gpiod_get_optional(dev, "reset", GPIOD_OUT_HIGH);
-	if (IS_ERR(data->reset_gpio)) {
-		return dev_err_probe(dev, PTR_ERR(data->reset_gpio),
-					"Failed to get reset gpio");
-	}
 
 	return 0;
 }
@@ -547,8 +535,6 @@ static void pca954x_reset_deassert(struct pca954x *data)
 {
 	if (data->reset_cont)
 		reset_control_deassert(data->reset_cont);
-	else
-		gpiod_set_value_cansleep(data->reset_gpio, 0);
 }
 
 /*
@@ -590,7 +576,7 @@ static int pca954x_probe(struct i2c_client *client)
 	if (ret)
 		goto fail_cleanup;
 
-	if (data->reset_cont || data->reset_gpio) {
+	if (data->reset_cont) {
 		udelay(1);
 		pca954x_reset_deassert(data);
 		/* Give the chip some time to recover. */
-- 
2.34.1

[PATCH v6 RESEND 2/2] i2c: muxes: pca954x: Reset if (de)select fails

[Wojciech Siudy]

If the channel selection or deselection times out, it indicates
a failure in the mux's I2C subsystem. Without sending a reset pulse,
a power-on-reset of the entire device would be required to restore
communication.

The datasheet specifies a minimum hold time of 4 ns for the reset
pulse, but due to the path's capacitance and themux having its own
clock, it is recommended to extend this to approximately 1 us.

Since reset controller is used, there is no need to take care of other
devices sharing the same reset line.

Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>
---
Changelog:
v2:
  * Removed mail header from the commit log
  * Decreased reset pulse hold time from 10 to 1 ms
v3:
  * Make this functionality enabled by appropriate property in
    devicetree
v4:
  * Fix missing condition check from devicetree
v5:
  * Declare dependency of a new property
v6:
  * Don't add any new properties, use reset controller only
---
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 33 +++++++++++++++++++++++------
 1 file changed, 26 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index e09230df7059..eb19a3566d35 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -315,6 +315,26 @@ static u8 pca954x_regval(struct pca954x *data, u8 chan)
 		return 1 << chan;
 }
 
+static void pca954x_reset_assert(struct pca954x *data)
+{
+	if (data->reset_cont)
+		reset_control_assert(data->reset_cont);
+}
+
+static void pca954x_reset_deassert(struct pca954x *data)
+{
+	if (data->reset_cont)
+		reset_control_deassert(data->reset_cont);
+}
+
+static void pca954x_reset_mux(struct pca954x *data)
+{
+	dev_warn(&data->client->dev, "resetting the device\n");
+	pca954x_reset_assert(data);
+	udelay(1);
+	pca954x_reset_deassert(data);
+}
+
 static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 {
 	struct pca954x *data = i2c_mux_priv(muxc);
@@ -328,6 +348,8 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
 		ret = pca954x_reg_write(muxc->parent, client, regval);
 		data->last_chan = ret < 0 ? 0 : regval;
 	}
+	if (ret == -ETIMEDOUT && (data->reset_cont))
+		pca954x_reset_mux(data);
 
 	return ret;
 }
@@ -337,6 +359,7 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	struct pca954x *data = i2c_mux_priv(muxc);
 	struct i2c_client *client = data->client;
 	s32 idle_state;
+	int ret = 0;
 
 	idle_state = READ_ONCE(data->idle_state);
 	if (idle_state >= 0)
@@ -346,8 +369,10 @@ static int pca954x_deselect_mux(struct i2c_mux_core *muxc, u32 chan)
 	if (idle_state == MUX_IDLE_DISCONNECT) {
 		/* Deselect active channel */
 		data->last_chan = 0;
-		return pca954x_reg_write(muxc->parent, client,
+		ret = pca954x_reg_write(muxc->parent, client,
 					 data->last_chan);
+		if (ret == -ETIMEDOUT && (data->reset_cont))
+			pca954x_reset_mux(data);
 	}
 
 	/* otherwise leave as-is */
@@ -531,12 +556,6 @@ static int pca954x_get_reset(struct device *dev, struct pca954x *data)
 	return 0;
 }
 
-static void pca954x_reset_deassert(struct pca954x *data)
-{
-	if (data->reset_cont)
-		reset_control_deassert(data->reset_cont);
-}
-
 /*
  * I2C init/probing/exit functions
  */
-- 
2.34.1

Re: [PATCH v6 RESEND 1/2] i2c: muxes: pca954x: Use reset controller only

[Wolfram Sang]

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

On Tue, Jun 03, 2025 at 02:47:20PM +0200, Wojciech Siudy wrote:
> Fallback to legacy reset-gpios is no more needed, because the framework
> can use reset-gpios properity now as well.
> 
> Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>

Nice cleanup. But I can't apply the patch on top of 6.16-rc7. Against
which kernel was this developed?


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

Re: [PATCH v6 RESEND 2/2] i2c: muxes: pca954x: Reset if (de)select fails

[Wolfram Sang]

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

Hi,

thanks for improving this driver!

> +static void pca954x_reset_mux(struct pca954x *data)
> +{
> +	dev_warn(&data->client->dev, "resetting the device\n");

To me, this is not the proper place for such a report. Can't the reset
framework be instrumented in some way?

> +	pca954x_reset_assert(data);
> +	udelay(1);
> +	pca954x_reset_deassert(data);
> +}
> +
>  static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  {
>  	struct pca954x *data = i2c_mux_priv(muxc);
> @@ -328,6 +348,8 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
>  		ret = pca954x_reg_write(muxc->parent, client, regval);
>  		data->last_chan = ret < 0 ? 0 : regval;
>  	}
> +	if (ret == -ETIMEDOUT && (data->reset_cont))

Braces around 'data...' not needed. Use '--strict' with checkpatch to
get notified.

> +		pca954x_reset_mux(data);
>  
>  	return ret;
>  }
> -		return pca954x_reg_write(muxc->parent, client,
> +		ret = pca954x_reg_write(muxc->parent, client,
>  					 data->last_chan);

Indentation of the above line needs to be adapted.

Rest looks good to me.

Happy hacking,

   Wolfram


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

Re: [PATCH v6 RESEND 1/2] i2c: muxes: pca954x: Use reset controller only

[Wolfram Sang]

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

On Tue, Jun 03, 2025 at 02:47:20PM +0200, Wojciech Siudy wrote:
> Fallback to legacy reset-gpios is no more needed, because the framework
> can use reset-gpios properity now as well.
> 
> Signed-off-by: Wojciech Siudy <wojciech.siudy@nokia.com>

Okay, since this email address bounces meanwhile, I fixed the patches to
prevent them getting lost.

> -					"Failed to get reset gpio");

Here was a custom whitespace change which prevented it from applying :(

With that fixed:

Applied to for-next, thanks!


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

Re: [PATCH v6 RESEND 2/2] i2c: muxes: pca954x: Reset if (de)select fails

[Wolfram Sang]

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


> > +static void pca954x_reset_mux(struct pca954x *data)
> > +{
> > +	dev_warn(&data->client->dev, "resetting the device\n");
> 
> To me, this is not the proper place for such a report. Can't the reset
> framework be instrumented in some way?

I removed it.

> 
> > +	pca954x_reset_assert(data);
> > +	udelay(1);
> > +	pca954x_reset_deassert(data);
> > +}
> > +
> >  static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> >  {
> >  	struct pca954x *data = i2c_mux_priv(muxc);
> > @@ -328,6 +348,8 @@ static int pca954x_select_chan(struct i2c_mux_core *muxc, u32 chan)
> >  		ret = pca954x_reg_write(muxc->parent, client, regval);
> >  		data->last_chan = ret < 0 ? 0 : regval;
> >  	}
> > +	if (ret == -ETIMEDOUT && (data->reset_cont))
> 
> Braces around 'data...' not needed. Use '--strict' with checkpatch to
> get notified.

Fixed this, too.

> > -		return pca954x_reg_write(muxc->parent, client,
> > +		ret = pca954x_reg_write(muxc->parent, client,
> >  					 data->last_chan);
> 
> Indentation of the above line needs to be adapted.

Ditto.

Applied to for-next, thanks!


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