Re: [PATCH 5/8] i2c: i801: add helper i801_set_hstadd()

[Heiner Kallweit <hkallweit1@gmail.com>]

On 09.06.2022 15:53, Jean Delvare wrote:
> Hi Heiner,
> 
> On Fri, 15 Apr 2022 18:57:21 +0200, Heiner Kallweit wrote:
>> Factor out setting SMBHSTADD to a helper. The current code makes the
>> assumption that constant I2C_SMBUS_READ has bit 0 set, that's not ideal.
> 
> This isn't an "assumption". The values of I2C_SMBUS_WRITE and
> I2C_SMBUS_READ were chosen to match the bit position and values in the
> I2C protocol. Maybe it should have been made clearer by defining them
> as hexadecimal values instead of decimal values. Nevertheless, I find
> it unfortunate to not use this fact to optimize the code, see below.
> 
>> Therefore let the new helper explicitly check for I2C_SMBUS_READ.
>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/i2c/busses/i2c-i801.c | 41 ++++++++++++++++++-----------------
>>  1 file changed, 21 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index a9737f14d..bf77f8640 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -771,6 +771,14 @@ static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *
>>  	return result;
>>  }
>>  
>> +static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>> +{
>> +	addr <<= 1;
>> +	if (read_write == I2C_SMBUS_READ)
>> +		addr |= 0x01;
>> +	outb_p(addr, SMBHSTADD(priv));
> 
> This can be written:
> 
> 	outb_p((addr << 1) | read_write, SMBHSTADD(priv));
> 
> Net result -48 bytes of (x86_64) binary code. That's basically what the
> original code was doing, minus the useless masking.
> 
OK

>> +}
>> +
>>  /* Return negative errno on error. */
>>  static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  		       unsigned short flags, char read_write, u8 command,
>> @@ -795,28 +803,24 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  
>>  	switch (size) {
>>  	case I2C_SMBUS_QUICK:
>> -		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> -		       SMBHSTADD(priv));
>> +		i801_set_hstadd(priv, addr, read_write);
>>  		xact = I801_QUICK;
>>  		break;
>>  	case I2C_SMBUS_BYTE:
>> -		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> -		       SMBHSTADD(priv));
>> +		i801_set_hstadd(priv, addr, read_write);
>>  		if (read_write == I2C_SMBUS_WRITE)
>>  			outb_p(command, SMBHSTCMD(priv));
>>  		xact = I801_BYTE;
>>  		break;
>>  	case I2C_SMBUS_BYTE_DATA:
>> -		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> -		       SMBHSTADD(priv));
>> +		i801_set_hstadd(priv, addr, read_write);
>>  		outb_p(command, SMBHSTCMD(priv));
>>  		if (read_write == I2C_SMBUS_WRITE)
>>  			outb_p(data->byte, SMBHSTDAT0(priv));
>>  		xact = I801_BYTE_DATA;
>>  		break;
>>  	case I2C_SMBUS_WORD_DATA:
>> -		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> -		       SMBHSTADD(priv));
>> +		i801_set_hstadd(priv, addr, read_write);
>>  		outb_p(command, SMBHSTCMD(priv));
>>  		if (read_write == I2C_SMBUS_WRITE) {
>>  			outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>> @@ -825,7 +829,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  		xact = I801_WORD_DATA;
>>  		break;
>>  	case I2C_SMBUS_PROC_CALL:
>> -		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
>> +		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>>  		outb_p(command, SMBHSTCMD(priv));
>>  		outb_p(data->word & 0xff, SMBHSTDAT0(priv));
>>  		outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
>> @@ -833,8 +837,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  		read_write = I2C_SMBUS_READ;
>>  		break;
>>  	case I2C_SMBUS_BLOCK_DATA:
>> -		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>> -		       SMBHSTADD(priv));
>> +		i801_set_hstadd(priv, addr, read_write);
>>  		outb_p(command, SMBHSTCMD(priv));
>>  		block = 1;
>>  		break;
>> @@ -845,10 +848,11 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  		 * However if SPD Write Disable is set (Lynx Point and later),
>>  		 * the read will fail if we don't set the R/#W bit.
>>  		 */
>> -		outb_p(((addr & 0x7f) << 1) |
>> -		       ((priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
>> -			(read_write & 0x01) : 0),
>> -		       SMBHSTADD(priv));
>> +		if (priv->original_hstcfg & SMBHSTCFG_SPD_WD)
>> +			i801_set_hstadd(priv, addr, read_write);
>> +		else
>> +			i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> 
> Preserving the use of the ternary operator makes the generated binary
> smaller once again:
> 
> 		i801_set_hstadd(priv, addr,
> 				(priv->original_hstcfg & SMBHSTCFG_SPD_WD) ?
> 				read_write : I2C_SMBUS_WRITE);
> 
> Net result -11 bytes of (x86_64) binary code.
> 
OK

>> +
>>  		if (read_write == I2C_SMBUS_READ) {
>>  			/* NB: page 240 of ICH5 datasheet also shows
>>  			 * that DATA1 is the cmd field when reading */
>> @@ -858,11 +862,8 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>>  		block = 1;
>>  		break;
>>  	case I2C_SMBUS_BLOCK_PROC_CALL:
>> -		/*
>> -		 * Bit 0 of the slave address register always indicate a write
>> -		 * command.
>> -		 */
>> -		outb_p((addr & 0x7f) << 1, SMBHSTADD(priv));
>> +		/* Needs to be flagged as write transaction */
>> +		i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
>>  		outb_p(command, SMBHSTCMD(priv));
>>  		block = 1;
>>  		break;
> 
>