Re: [PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Jean Delvare <jdelvare@suse.de>]

Hi Heiner,

On Thu, 11 Nov 2021 22:43:35 +0100, Heiner Kallweit wrote:
> If FEATURE_BLOCK_BUFFER is set I don't see how setting this bit could
> fail. Reading it back seems to be overly paranoid. Origin of this
> check seems to be 14 yrs ago when people were not completely sure
> which chip versions support block buffer mode.

Your reading of the history is correct, although "overly paranoid"
might be a somewhat exaggerated statement. When you modify a driver
used by millions and have been bitten by undocumented restrictions in
the same area, being cautious not to cause a regression doesn't seem
that bad to me.

What was wrong in that approach, I would think retrospectively, is that
i801_set_block_buffer_mode() should have been made verbose on failure,
so that we learned over time if any chipset actually failed to support
the feature in question. Because 14 years later we in fact still don't
know if the test was needed or not.

I'm fine with your change nevertheless, it should be fine, and if
anything breaks then we'll fix it.

I'll test it on my system later today.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/i2c/busses/i2c-i801.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index 4c96f1b47..608e928e9 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -521,9 +521,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	/* Set block buffer mode */
> +	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
> +
>  	inb_p(SMBHSTCNT(priv)); /* reset the data buffer index */
>  
> -	/* Use 32-byte buffer to process this transaction */
>  	if (read_write == I2C_SMBUS_WRITE) {
>  		len = data->block[0];
>  		outb_p(len, SMBHSTDAT0(priv));
> @@ -750,14 +752,6 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv,
>  	return i801_check_post(priv, status);
>  }
>  
> -static int i801_set_block_buffer_mode(struct i801_priv *priv)
> -{
> -	outb_p(inb_p(SMBAUXCTL(priv)) | SMBAUXCTL_E32B, SMBAUXCTL(priv));
> -	if ((inb_p(SMBAUXCTL(priv)) & SMBAUXCTL_E32B) == 0)
> -		return -EIO;
> -	return 0;
> -}
> -
>  /* Block transaction function */
>  static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
>  				  char read_write, int command)
> @@ -786,9 +780,8 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>  	/* Experience has shown that the block buffer can only be used for
>  	   SMBus (not I2C) block transactions, even though the datasheet
>  	   doesn't mention this limitation. */
> -	if ((priv->features & FEATURE_BLOCK_BUFFER)
> -	 && command != I2C_SMBUS_I2C_BLOCK_DATA
> -	 && i801_set_block_buffer_mode(priv) == 0)
> +	if (priv->features & FEATURE_BLOCK_BUFFER &&

No, please preserve the parentheses. Mixing "&" and "&&" without
parentheses is highly confusing (to me at least, but I suspect I'm not
alone).

> +	    command != I2C_SMBUS_I2C_BLOCK_DATA)
>  		result = i801_block_transaction_by_block(priv, data,
>  							 read_write,
>  							 command);


-- 
Jean Delvare
SUSE L3 Support