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