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

[Jean Delvare <jdelvare@suse.de>]

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.

> +}
> +
>  /* 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.

> +
>  		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;


-- 
Jean Delvare
SUSE L3 Support