[PATCH 1/2] i2c: mux: pca954x: change the default to deselect after each transfer

[PATCH 1/2] i2c: mux: pca954x: change the default to deselect after each transfer

[Robert Shearman]

From: Robert Shearman <robert.shearman@att.com>

The behaviour, by default, to not deselect after each transfer is
not safe/correct when there is a device with an address that conflicts
with another device either on the parent bus, or on another pca954x
mux on the same parent bus.

Therefore, default to the least surprising mode, where the mux
channels are deselected after each transaction, which is safe in the
face of one or more devices hanging off the mux with addresses that
conflict with other devices on different muxes, or on the parent
bus. On systems where it is guaranteed that this is not the case, then
the i2c-mux-no-idle-disconnect devicetree or no_deselect_on_exit
platform data options can be used to improve the performance.

Signed-off-by: Robert Shearman <robert.shearman@att.com>
---
 drivers/i2c/muxes/i2c-mux-pca954x.c   | 16 +++++++++-------
 include/linux/platform_data/pca954x.h |  6 +++---
 2 files changed, 12 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
index bfabf985e830..c2b6cff4ba3c 100644
--- a/drivers/i2c/muxes/i2c-mux-pca954x.c
+++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
@@ -351,7 +351,7 @@ static int pca954x_probe(struct i2c_client *client,
 	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
 	struct device *dev = &client->dev;
 	struct device_node *np = dev->of_node;
-	bool idle_disconnect_dt;
+	bool no_idle_disconnect_dt;
 	struct gpio_desc *gpio;
 	int num, force, class;
 	struct i2c_mux_core *muxc;
@@ -412,9 +412,10 @@ static int pca954x_probe(struct i2c_client *client,
 	}
 
 	data->last_chan = 0;		   /* force the first selection */
+	data->deselect = GENMASK(data->chip->nchans, 0);
 
-	idle_disconnect_dt = np &&
-		of_property_read_bool(np, "i2c-mux-idle-disconnect");
+	no_idle_disconnect_dt = np &&
+		of_property_read_bool(np, "i2c-mux-no-idle-disconnect");
 
 	ret = pca954x_irq_setup(muxc);
 	if (ret)
@@ -422,7 +423,7 @@ static int pca954x_probe(struct i2c_client *client,
 
 	/* Now create an adapter for each channel */
 	for (num = 0; num < data->chip->nchans; num++) {
-		bool idle_disconnect_pd = false;
+		bool no_idle_disconnect_pd = false;
 
 		force = 0;			  /* dynamic adap number */
 		class = 0;			  /* no class by default */
@@ -434,10 +435,11 @@ static int pca954x_probe(struct i2c_client *client,
 			} else
 				/* discard unconfigured channels */
 				break;
-			idle_disconnect_pd = pdata->modes[num].deselect_on_exit;
+			no_idle_disconnect_pd =
+				pdata->modes[num].no_deselect_on_exit;
 		}
-		data->deselect |= (idle_disconnect_pd ||
-				   idle_disconnect_dt) << num;
+		data->deselect &= ~((no_idle_disconnect_pd ||
+				     no_idle_disconnect_dt) << num);
 
 		ret = i2c_mux_add_adapter(muxc, force, num, class);
 		if (ret)
diff --git a/include/linux/platform_data/pca954x.h b/include/linux/platform_data/pca954x.h
index 1712677d5904..cb5359797867 100644
--- a/include/linux/platform_data/pca954x.h
+++ b/include/linux/platform_data/pca954x.h
@@ -29,13 +29,13 @@
 
 /* Per channel initialisation data:
  * @adap_id: bus number for the adapter. 0 = don't care
- * @deselect_on_exit: set this entry to 1, if your H/W needs deselection
- *                    of this channel after transaction.
+ * @no_deselect_on_exit: set this entry to 1, if your H/W doesn't need
+ *                       deselection of this channel after transaction.
  *
  */
 struct pca954x_platform_mode {
 	int		adap_id;
-	unsigned int	deselect_on_exit:1;
+	unsigned int	no_deselect_on_exit:1;
 	unsigned int	class;
 };
 
-- 
2.20.1

[PATCH 2/2] dt-bindings: i2c-mux-pca954x: Invert i2c-mux-idle-disconnect property

[Robert Shearman]

From: Robert Shearman <robert.shearman@att.com>

Update the binding docs for i2c-mux-pca954x to reflect that the driver
now defaults to idle-disconnect behaviour by removing the documentation
of the previous i2c-mux-idle-disconnect property and introducing a new
inverse i2c-mux-no-idle-disconnect property.

Signed-off-by: Robert Shearman <robert.shearman@att.com>
---
 Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
index 30ac6a60f041..c3a94de8b3ef 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-mux-pca954x.txt
@@ -25,9 +25,11 @@ Required Properties:
 Optional Properties:
 
   - reset-gpios: Reference to the GPIO connected to the reset input.
-  - i2c-mux-idle-disconnect: Boolean; if defined, forces mux to disconnect all
-    children in idle state. This is necessary for example, if there are several
-    multiplexers on the bus and the devices behind them use same I2C addresses.
+  - i2c-mux-no-idle-disconnect: Boolean; if defined, asks mux to not
+    disconnect children in idle state. This optimises I2C parent
+    operations and is useful if it is guaranteed that there are no
+    multiplexers on the bus with devices behind them using the same
+    I2C address.
   - interrupts: Interrupt mapping for IRQ.
   - interrupt-controller: Marks the device node as an interrupt controller.
   - #interrupt-cells : Should be two.
-- 
2.20.1

Re: [PATCH 1/2] i2c: mux: pca954x: change the default to deselect after each transfer

[Peter Rosin]

Hi Robert,

Thanks, for your patches.

On 2019-01-21 17:47, Robert Shearman wrote:
> From: Robert Shearman <robert.shearman@att.com>
> 
> The behaviour, by default, to not deselect after each transfer is
> not safe/correct when there is a device with an address that conflicts
> with another device either on the parent bus, or on another pca954x
> mux on the same parent bus.

Right. The current way might not be the safest, but the way I see it,
this ship has sailed a long time ago. It might take quite a while
before someone reports a regression, but when that happens and we
need to revisit or even revert this, things will get ugly.

Also, doubling or even tripling the I2C traffic (most I2C xfers
are small) for unsuspecting users with more normal I2C setups
(w/o multiple muxes) just to accommodate the more complexes cases
is arguably not the correct balance.

> Therefore, default to the least surprising mode, where the mux
> channels are deselected after each transaction, which is safe in the
> face of one or more devices hanging off the mux with addresses that
> conflict with other devices on different muxes, or on the parent

Side note, and not that it matters since it's a NACK anyway, but
having an address conflict with the parent bus is not allowed and is
just bad hardware design that simply cannot work reliably, if at all.

Cheers,
Peter

> bus. On systems where it is guaranteed that this is not the case, then
> the i2c-mux-no-idle-disconnect devicetree or no_deselect_on_exit
> platform data options can be used to improve the performance.
> 
> Signed-off-by: Robert Shearman <robert.shearman@att.com>
> ---
>  drivers/i2c/muxes/i2c-mux-pca954x.c   | 16 +++++++++-------
>  include/linux/platform_data/pca954x.h |  6 +++---
>  2 files changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-pca954x.c b/drivers/i2c/muxes/i2c-mux-pca954x.c
> index bfabf985e830..c2b6cff4ba3c 100644
> --- a/drivers/i2c/muxes/i2c-mux-pca954x.c
> +++ b/drivers/i2c/muxes/i2c-mux-pca954x.c
> @@ -351,7 +351,7 @@ static int pca954x_probe(struct i2c_client *client,
>  	struct pca954x_platform_data *pdata = dev_get_platdata(&client->dev);
>  	struct device *dev = &client->dev;
>  	struct device_node *np = dev->of_node;
> -	bool idle_disconnect_dt;
> +	bool no_idle_disconnect_dt;
>  	struct gpio_desc *gpio;
>  	int num, force, class;
>  	struct i2c_mux_core *muxc;
> @@ -412,9 +412,10 @@ static int pca954x_probe(struct i2c_client *client,
>  	}
>  
>  	data->last_chan = 0;		   /* force the first selection */
> +	data->deselect = GENMASK(data->chip->nchans, 0);
>  
> -	idle_disconnect_dt = np &&
> -		of_property_read_bool(np, "i2c-mux-idle-disconnect");
> +	no_idle_disconnect_dt = np &&
> +		of_property_read_bool(np, "i2c-mux-no-idle-disconnect");
>  
>  	ret = pca954x_irq_setup(muxc);
>  	if (ret)
> @@ -422,7 +423,7 @@ static int pca954x_probe(struct i2c_client *client,
>  
>  	/* Now create an adapter for each channel */
>  	for (num = 0; num < data->chip->nchans; num++) {
> -		bool idle_disconnect_pd = false;
> +		bool no_idle_disconnect_pd = false;
>  
>  		force = 0;			  /* dynamic adap number */
>  		class = 0;			  /* no class by default */
> @@ -434,10 +435,11 @@ static int pca954x_probe(struct i2c_client *client,
>  			} else
>  				/* discard unconfigured channels */
>  				break;
> -			idle_disconnect_pd = pdata->modes[num].deselect_on_exit;
> +			no_idle_disconnect_pd =
> +				pdata->modes[num].no_deselect_on_exit;
>  		}
> -		data->deselect |= (idle_disconnect_pd ||
> -				   idle_disconnect_dt) << num;
> +		data->deselect &= ~((no_idle_disconnect_pd ||
> +				     no_idle_disconnect_dt) << num);
>  
>  		ret = i2c_mux_add_adapter(muxc, force, num, class);
>  		if (ret)
> diff --git a/include/linux/platform_data/pca954x.h b/include/linux/platform_data/pca954x.h
> index 1712677d5904..cb5359797867 100644
> --- a/include/linux/platform_data/pca954x.h
> +++ b/include/linux/platform_data/pca954x.h
> @@ -29,13 +29,13 @@
>  
>  /* Per channel initialisation data:
>   * @adap_id: bus number for the adapter. 0 = don't care
> - * @deselect_on_exit: set this entry to 1, if your H/W needs deselection
> - *                    of this channel after transaction.
> + * @no_deselect_on_exit: set this entry to 1, if your H/W doesn't need
> + *                       deselection of this channel after transaction.
>   *
>   */
>  struct pca954x_platform_mode {
>  	int		adap_id;
> -	unsigned int	deselect_on_exit:1;
> +	unsigned int	no_deselect_on_exit:1;
>  	unsigned int	class;
>  };
>  
> 

Re: [PATCH 1/2] i2c: mux: pca954x: change the default to deselect after each transfer

[Robert Shearman]

Hi Peter,

On 22/01/2019 08:41, Peter Rosin wrote:
> Hi Robert,
> 
> Thanks, for your patches.
> 
> On 2019-01-21 17:47, Robert Shearman wrote:
>> From: Robert Shearman <robert.shearman@att.com>
>>
>> The behaviour, by default, to not deselect after each transfer is
>> not safe/correct when there is a device with an address that conflicts
>> with another device either on the parent bus, or on another pca954x
>> mux on the same parent bus.
> 
> Right. The current way might not be the safest, but the way I see it,
> this ship has sailed a long time ago. It might take quite a while
> before someone reports a regression, but when that happens and we
> need to revisit or even revert this, things will get ugly.
> 
> Also, doubling or even tripling the I2C traffic (most I2C xfers
> are small) for unsuspecting users with more normal I2C setups
> (w/o multiple muxes) just to accommodate the more complexes cases
> is arguably not the correct balance.

Ok, thanks for considering the changes. Would a device-level sysfs 
toggle or module parameter approach be more acceptable?

The use case where we are hitting this issue is on an x86_64 ACPI 
platform where we desire to use a general purpose kernel, so the 
existing devicetree and platform data toggle approaches are not ideal 
for us.

> 
>> Therefore, default to the least surprising mode, where the mux
>> channels are deselected after each transaction, which is safe in the
>> face of one or more devices hanging off the mux with addresses that
>> conflict with other devices on different muxes, or on the parent
> 
> Side note, and not that it matters since it's a NACK anyway, but
> having an address conflict with the parent bus is not allowed and is
> just bad hardware design that simply cannot work reliably, if at all.

Yes, good point.

Thanks,
Rob