Hi Heiner,
On Mon, 19 Dec 2022 19:20:55 +0100, Heiner Kallweit wrote:
> Currently configuring command register settings is disributed over multiple
> functions. At first cetralize this for non-block commands in
Typo: cetralize -> centralize.
> i801_simple_transaction().
>
> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-i801.c | 31 ++++++++++++++-----------------
> 1 file changed, 14 insertions(+), 17 deletions(-)
I'm happy with the idea (well I kind of suggested it in the first
place, so...), and pleased to see that the diffstats confirm it was a
good idea.
> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
> index d7182f7c8..0d49e9587 100644
> --- a/drivers/i2c/busses/i2c-i801.c
> +++ b/drivers/i2c/busses/i2c-i801.c
> @@ -738,35 +738,47 @@ static void i801_set_hstadd(struct i801_priv *priv, u8 addr, char read_write)
>
> /* Single value transaction function */
> static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data *data,
> - char read_write, int command)
> + u8 addr, u8 hstcmd, char read_write, int command)
See my review of patch 5 about how I don't like some of the parameter
names. But I realize now that this problem predates your patches,
functions i801_block_transaction_by_block() and
i801_block_transaction_byte_by_byte() already carry the confusion. So
I'm fine leaving it as is for now, maybe we can clean it up later
driver-wide.
> {
> int xact, ret;
>
> switch (command) {
> case I2C_SMBUS_QUICK:
> + i801_set_hstadd(priv, addr, read_write);
> xact = I801_QUICK;
> break;
> case I2C_SMBUS_BYTE:
> + i801_set_hstadd(priv, addr, read_write);
> + if (read_write == I2C_SMBUS_WRITE)
> + outb_p(hstcmd, SMBHSTCMD(priv));
> xact = I801_BYTE;
> break;
> case I2C_SMBUS_BYTE_DATA:
> + i801_set_hstadd(priv, addr, read_write);
> if (read_write == I2C_SMBUS_WRITE)
> outb_p(data->byte, SMBHSTDAT0(priv));
> + outb_p(hstcmd, SMBHSTCMD(priv));
> xact = I801_BYTE_DATA;
> break;
> case I2C_SMBUS_WORD_DATA:
> + i801_set_hstadd(priv, addr, read_write);
> if (read_write == I2C_SMBUS_WRITE) {
> outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> }
> + outb_p(hstcmd, SMBHSTCMD(priv));
> xact = I801_WORD_DATA;
> break;
> case I2C_SMBUS_PROC_CALL:
> + i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> outb_p(data->word & 0xff, SMBHSTDAT0(priv));
> outb_p((data->word & 0xff00) >> 8, SMBHSTDAT1(priv));
> + outb_p(hstcmd, SMBHSTCMD(priv));
> + read_write = I2C_SMBUS_READ;
> xact = I801_PROC_CALL;
> break;
> default:
> + pci_err(priv->pci_dev, "Unsupported transaction %d\n", command);
> return -EOPNOTSUPP;
> }
The split between patches 7 and 8 isn't really clean, as you have dead
code here at this step, which will only become active with the next
patch. I'm willing to let it go though.
>
> @@ -857,25 +869,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
>
> switch (size) {
> case I2C_SMBUS_QUICK:
> - i801_set_hstadd(priv, addr, read_write);
> - break;
> case I2C_SMBUS_BYTE:
> - i801_set_hstadd(priv, addr, read_write);
> - if (read_write == I2C_SMBUS_WRITE)
> - outb_p(command, SMBHSTCMD(priv));
> - break;
> case I2C_SMBUS_BYTE_DATA:
> - i801_set_hstadd(priv, addr, read_write);
> - outb_p(command, SMBHSTCMD(priv));
> - break;
> case I2C_SMBUS_WORD_DATA:
> - i801_set_hstadd(priv, addr, read_write);
> - outb_p(command, SMBHSTCMD(priv));
> - break;
> case I2C_SMBUS_PROC_CALL:
> - i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE);
> - outb_p(command, SMBHSTCMD(priv));
> - read_write = I2C_SMBUS_READ;
> break;
> case I2C_SMBUS_BLOCK_DATA:
> i801_set_hstadd(priv, addr, read_write);
> @@ -922,7 +919,7 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr,
> if (block)
> ret = i801_block_transaction(priv, data, read_write, size);
> else
> - ret = i801_simple_transaction(priv, data, read_write, size);
> + ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
>
> /* Some BIOSes don't like it when PEC is enabled at reboot or resume
> * time, so we forcibly disable it after every transaction.
Reviewed-by: Jean Delvare <jdelvare@xxxxxxx>
--
Jean Delvare
SUSE L3 Support
