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@xxxxxxxxx>
>> ---
>> 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);
>
>
