| Message ID | 9d06c7d6-8c62-4e44-9e3e-334ac14e38a1@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: collection of further improvements / refactorings | expand |
On Thu, Feb 01, 2024 at 09:12:33PM +0100, Heiner Kallweit wrote: > Add a sentinel length value that is used to check whether we should > read and use the length value provided by the slave device. > This simplifies the currently used checks. ... > + /* At transfer start i801_smbus_block_transaction() marks > + * the block length as invalid. Check for this sentinel value > + * and read the block length from SMBHSTDAT0. > + */ /* * May we use the correct multi-line * comment style, please? */ ... > + /* At transfer start i801_smbus_block_transaction() marks > + * the block length as invalid. Check for this sentinel value > + * and read the block length from SMBHSTDAT0. > + */ Ditto. ... > if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA) > - data->block[0] = I2C_SMBUS_BLOCK_MAX; > + data->block[0] = SMBUS_LEN_SENTINEL; /* Mark block length as invalid */ I would add a separated comment line on top. > else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) > return -EPROTO;
On 01.02.2024 21:48, Andy Shevchenko wrote: > On Thu, Feb 01, 2024 at 09:12:33PM +0100, Heiner Kallweit wrote: >> Add a sentinel length value that is used to check whether we should >> read and use the length value provided by the slave device. >> This simplifies the currently used checks. > > ... > >> + /* At transfer start i801_smbus_block_transaction() marks >> + * the block length as invalid. Check for this sentinel value >> + * and read the block length from SMBHSTDAT0. >> + */ > > /* > * May we use the correct multi-line > * comment style, please? > */ > Right, everybody outside netdev uses this comment style > ... > >> + /* At transfer start i801_smbus_block_transaction() marks >> + * the block length as invalid. Check for this sentinel value >> + * and read the block length from SMBHSTDAT0. >> + */ > > Ditto. > > ... > >> if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA) >> - data->block[0] = I2C_SMBUS_BLOCK_MAX; >> + data->block[0] = SMBUS_LEN_SENTINEL; /* Mark block length as invalid */ > > I would add a separated comment line on top. > OK >> else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) >> return -EPROTO; >
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 24eb187db..514711406 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -205,6 +205,8 @@ #define STATUS_FLAGS (SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \ STATUS_ERROR_FLAGS) +#define SMBUS_LEN_SENTINEL (I2C_SMBUS_BLOCK_MAX + 1) + /* Older devices have their ID defined in <linux/pci_ids.h> */ #define PCI_DEVICE_ID_INTEL_COMETLAKE_SMBUS 0x02a3 #define PCI_DEVICE_ID_INTEL_COMETLAKE_H_SMBUS 0x06a3 @@ -541,9 +543,11 @@ static int i801_block_transaction_by_block(struct i801_priv *priv, static void i801_isr_byte_done(struct i801_priv *priv) { if (priv->is_read) { - /* For SMBus block reads, length is received with first byte */ - if (((priv->cmd & 0x1c) == I801_BLOCK_DATA) && - (priv->count == 0)) { + /* At transfer start i801_smbus_block_transaction() marks + * the block length as invalid. Check for this sentinel value + * and read the block length from SMBHSTDAT0. + */ + if (priv->len == SMBUS_LEN_SENTINEL) { priv->len = inb_p(SMBHSTDAT0(priv)); if (priv->len < 1 || priv->len > I2C_SMBUS_BLOCK_MAX) { dev_err(&priv->pci_dev->dev, @@ -698,8 +702,11 @@ static int i801_block_transaction_byte_by_byte(struct i801_priv *priv, if (status) return status; - if (i == 1 && read_write == I2C_SMBUS_READ - && command != I2C_SMBUS_I2C_BLOCK_DATA) { + /* At transfer start i801_smbus_block_transaction() marks + * the block length as invalid. Check for this sentinel value + * and read the block length from SMBHSTDAT0. + */ + if (len == SMBUS_LEN_SENTINEL) { len = inb_p(SMBHSTDAT0(priv)); if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) { dev_err(&priv->pci_dev->dev, @@ -806,7 +813,7 @@ static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_ u8 addr, u8 hstcmd, char read_write, int command) { if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA) - data->block[0] = I2C_SMBUS_BLOCK_MAX; + data->block[0] = SMBUS_LEN_SENTINEL; /* Mark block length as invalid */ else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) return -EPROTO;
Add a sentinel length value that is used to check whether we should read and use the length value provided by the slave device. This simplifies the currently used checks. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- v2: - add comments --- drivers/i2c/busses/i2c-i801.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-)