Re: [PATCH] i2c: i801: Remove i801_set_block_buffer_mode

[Heiner Kallweit <hkallweit1@gmail.com>]

On 18.11.2021 11:23, Jean Delvare wrote:
> 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.

Indeed my statement could be read as: The guys back then didn't know
what they were doing. It definitely wasn't meant this way.

> 
> 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.
> 
ICH4 spec mentions the block buffer mode and it's hard to imagine
(even though not impossible) that single later versions dropped
this feature.

> 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).
> 
Shall I send a v2 with an adjusted commit message and these
parentheses re-added?

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