RE: [PATCH] i2c: mux: mlxcpld: fix i2c mux selection caching

[Vadim Pasternak]

> -----Original Message-----
> From: Peter Rosin [mailto:peda@xxxxxxxxxx]
> Sent: Saturday, December 17, 2016 10:29 PM
> To: linux-kernel@xxxxxxxxxxxxxxx
> Cc: Peter Rosin <peda@xxxxxxxxxx>; Vadim Pasternak
> <vadimp@xxxxxxxxxxxx>; Michael Shych <michaelsh@xxxxxxxxxxxx>;
> Wolfram Sang <wsa@xxxxxxxxxxxxx>; linux-i2c@xxxxxxxxxxxxxxx
> Subject: [PATCH] i2c: mux: mlxcpld: fix i2c mux selection caching
> 
> smbus functions return -ve on error, 0 on success.  However,
> __i2c_transfer() have a different return signature - -ve on error, or number
> of buffers transferred (which may be zero or greater.)
> 
> The upshot of this is that the sense of the text is reversed when using the
> mux on a bus supporting the master_xfer method: we cache the value and
> never retry if we fail to transfer any buffers, but if we succeed, we clear the
> cached value
> 
> Fix this by making mlxcpld_mux_reg_write() return a -ve error code for all
> failure cases, just as was done in commit 7f638c1cb0a1 ("i2c: mux:
> pca954x: fix i2c mux selection caching")
> 
> This also aligns the implementations of these two muxes in this area.
> 
> Signed-off-by: Peter Rosin <peda@xxxxxxxxxx>
> ---

Acked-by: Vadim Pasternak <vadimp@xxxxxxxxxxxx>

> 
> Hi!
> 
> I audited the other i2c muxes for similar problems after the recent regression
> report for the pca954x driver [1], and found only this.
> 
> I obviously don't have the hardware to test this patch.
> 
> Anyway, this driver is probably lower profile than the pca954x, but this still
> needs fixing in my opinion...
> 
> Cheers,
> peda
> 
> [1] http://www.spinics.net/lists/linux-i2c/msg27586.html
> 
>  drivers/i2c/muxes/i2c-mux-mlxcpld.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/muxes/i2c-mux-mlxcpld.c b/drivers/i2c/muxes/i2c-
> mux-mlxcpld.c
> index 3ab654bbfab5..b7ca249ec9c3 100644
> --- a/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> +++ b/drivers/i2c/muxes/i2c-mux-mlxcpld.c
> @@ -95,6 +95,7 @@ static int mlxcpld_mux_reg_write(struct i2c_adapter
> *adap,
>  				 struct i2c_client *client, u8 val)  {
>  	struct mlxcpld_mux_plat_data *pdata = dev_get_platdata(&client-
> >dev);
> +	int ret = -ENODEV;
> 
>  	if (adap->algo->master_xfer) {
>  		struct i2c_msg msg;
> @@ -104,17 +105,21 @@ static int mlxcpld_mux_reg_write(struct
> i2c_adapter *adap,
>  		msg.flags = 0;
>  		msg.len = 2;
>  		msg.buf = msgbuf;
> -		return __i2c_transfer(adap, &msg, 1);
> +		ret = __i2c_transfer(adap, &msg, 1);
> +
> +		if (ret >= 0 && ret != 1)
> +			ret = -EREMOTEIO;
>  	} else if (adap->algo->smbus_xfer) {
>  		union i2c_smbus_data data;
> 
>  		data.byte = val;
> -		return adap->algo->smbus_xfer(adap, client->addr,
> -					      client->flags, I2C_SMBUS_WRITE,
> -					      pdata->sel_reg_addr,
> -					      I2C_SMBUS_BYTE_DATA, &data);
> -	} else
> -		return -ENODEV;
> +		ret = adap->algo->smbus_xfer(adap, client->addr,
> +					     client->flags, I2C_SMBUS_WRITE,
> +					     pdata->sel_reg_addr,
> +					     I2C_SMBUS_BYTE_DATA, &data);
> +	}
> +
> +	return ret;
>  }
> 
>  static int mlxcpld_mux_select_chan(struct i2c_mux_core *muxc, u32 chan)
> @@ -127,10 +132,7 @@ static int mlxcpld_mux_select_chan(struct
> i2c_mux_core *muxc, u32 chan)
>  	/* Only select the channel if its different from the last channel */
>  	if (data->last_chan != regval) {
>  		err = mlxcpld_mux_reg_write(muxc->parent, client, regval);
> -		if (err)
> -			data->last_chan = 0;
> -		else
> -			data->last_chan = regval;
> +		data->last_chan = err < 0 ? 0 : regval;
>  	}
> 
>  	return err;
> --
> 2.1.4