diff mbox series

[v3,2/2] pca954x: Reset if channel select fails

Message ID DB6PR07MB35097CC094E6995B2F66A17A9D652@DB6PR07MB3509.eurprd07.prod.outlook.com
State Superseded
Headers show
Series [v3,1/2] dt-bindings: i2c: pca954x: Add timeout reset property | expand

Commit Message

Wojciech Siudy (Nokia) Sept. 13, 2024, 6:51 a.m. UTC 
From: Wojciech Siudy <wojciech.siudy@nokia.com>

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 the mux having its own
clock, it is recommended to extend this to approximately 1 us.

This option can be enabled using the i2c-mux-timeout-reset property
in the device tree and should only be used if the reset line is not
shared with other devices.

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
---
 drivers/i2c/muxes/i2c-mux-pca954x.c | 48 ++++++++++++++++++++++-------
 1 file changed, 37 insertions(+), 11 deletions(-)

Comments

Laurent Pinchart Sept. 13, 2024, 9:57 a.m. UTC | #1 
Hi Wojciech,

Thank you for the patch.

On Fri, Sep 13, 2024 at 06:51:16AM +0000, Wojciech Siudy (Nokia) wrote:
> From: Wojciech Siudy <wojciech.siudy@nokia.com>
> 
> 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 the mux having its own
> clock, it is recommended to extend this to approximately 1 us.
> 
> This option can be enabled using the i2c-mux-timeout-reset property
> in the device tree and should only be used if the reset line is not
> shared with other devices.
> 
> 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
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c | 48 ++++++++++++++++++++++-------
>  1 file changed, 37 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index 6f84018258c4..c9ac0f9c9408 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -110,6 +110,7 @@ struct pca954x {
>  	u8 last_chan;		/* last register value */
>  	/* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
>  	s32 idle_state;
> +	u8 timeout_reset;
>  
>  	struct i2c_client *client;
>  
> @@ -316,6 +317,30 @@ static u8 pca954x_regval(struct pca954x *data, u8 chan)
>  		return 1 << chan;
>  }
>  
> +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);
> +}
> +
> +static void pca954x_reset_assert(struct pca954x *data)
> +{
> +	if (data->reset_cont)
> +		reset_control_assert(data->reset_cont);
> +	else
> +		gpiod_set_value_cansleep(data->reset_gpio, 1);
> +}
> +
> +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);
> @@ -329,6 +354,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 || data->reset_gpio))
> +		pca954x_reset_mux(data);
>  
>  	return ret;
>  }
> @@ -338,6 +365,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)
> @@ -347,13 +375,14 @@ 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,
> -					 data->last_chan);
> +		ret = pca954x_reg_write(muxc->parent, client, data->last_chan);
> +		if (ret == -ETIMEDOUT && (data->reset_cont || data->reset_gpio))
> +			pca954x_reset_mux(data);
>  	}
>  
>  	/* otherwise leave as-is */
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static ssize_t idle_state_show(struct device *dev,
> @@ -543,14 +572,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);
> -	else
> -		gpiod_set_value_cansleep(data->reset_gpio, 0);
> -}
> -
>  /*
>   * I2C init/probing/exit functions
>   */
> @@ -625,6 +646,11 @@ static int pca954x_probe(struct i2c_client *client)
>  			data->idle_state = MUX_IDLE_DISCONNECT;
>  	}
>  
> +	if (device_property_read_bool(dev, "i2c-mux-timeout-reset"))
> +		data->timeout_reset = 1;
> +	else
> +		data->timeout_reset = 0;

timeout_reset is set but never used. Am I missing something ?

> +
>  	/*
>  	 * Write the mux register at addr to verify
>  	 * that the mux is in fact present. This also
Wojciech Siudy (Nokia) Sept. 13, 2024, 10:23 a.m. UTC | #2 
Hi!

> timeout_reset is set but never used. Am I missing something?
Unfortunately you are right, it should be in this line as additional condition:
if (ret == -ETIMEDOUT && (data->reset_cont || data->reset_gpio))

Please look forward for another patch.

Regards,
Wojciech
diff mbox series

Patch

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index 6f84018258c4..c9ac0f9c9408 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -110,6 +110,7 @@  struct pca954x {
 	u8 last_chan;		/* last register value */
 	/* MUX_IDLE_AS_IS, MUX_IDLE_DISCONNECT or >= 0 for channel */
 	s32 idle_state;
+	u8 timeout_reset;
 
 	struct i2c_client *client;
 
@@ -316,6 +317,30 @@  static u8 pca954x_regval(struct pca954x *data, u8 chan)
 		return 1 << chan;
 }
 
+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);
+}
+
+static void pca954x_reset_assert(struct pca954x *data)
+{
+	if (data->reset_cont)
+		reset_control_assert(data->reset_cont);
+	else
+		gpiod_set_value_cansleep(data->reset_gpio, 1);
+}
+
+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);
@@ -329,6 +354,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 || data->reset_gpio))
+		pca954x_reset_mux(data);
 
 	return ret;
 }
@@ -338,6 +365,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)
@@ -347,13 +375,14 @@  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,
-					 data->last_chan);
+		ret = pca954x_reg_write(muxc->parent, client, data->last_chan);
+		if (ret == -ETIMEDOUT && (data->reset_cont || data->reset_gpio))
+			pca954x_reset_mux(data);
 	}
 
 	/* otherwise leave as-is */
 
-	return 0;
+	return ret;
 }
 
 static ssize_t idle_state_show(struct device *dev,
@@ -543,14 +572,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);
-	else
-		gpiod_set_value_cansleep(data->reset_gpio, 0);
-}
-
 /*
  * I2C init/probing/exit functions
  */
@@ -625,6 +646,11 @@  static int pca954x_probe(struct i2c_client *client)
 			data->idle_state = MUX_IDLE_DISCONNECT;
 	}
 
+	if (device_property_read_bool(dev, "i2c-mux-timeout-reset"))
+		data->timeout_reset = 1;
+	else
+		data->timeout_reset = 0;
+
 	/*
 	 * Write the mux register at addr to verify
 	 * that the mux is in fact present. This also