| Message ID | 07acdfa5-0ab6-4885-a588-d169a31793c4@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: collection of further improvements / refactorings | expand |
On Fri, Sep 22, 2023 at 09:40:25PM +0200, 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. > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/i2c/busses/i2c-i801.c | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c > index a9d3dfd9e..30a2f9268 100644 > --- a/drivers/i2c/busses/i2c-i801.c > +++ b/drivers/i2c/busses/i2c-i801.c > @@ -204,6 +204,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,8 +543,7 @@ 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)) { > + 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, > @@ -697,8 +698,7 @@ 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) { > + if (len == SMBUS_LEN_SENTINEL) { > len = inb_p(SMBHSTDAT0(priv)); > if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) { > dev_err(&priv->pci_dev->dev, > @@ -805,7 +805,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; This patch is good, but a few comments for each change to tell where the sentinel will be used and where the sentinel was set would help to better understand the use of the sentinel. Thanks, Andi > else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) > return -EPROTO; > > -- > 2.42.0 > >
On 30.01.2024 01:23, Andi Shyti wrote: > On Fri, Sep 22, 2023 at 09:40:25PM +0200, 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. >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/i2c/busses/i2c-i801.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c >> index a9d3dfd9e..30a2f9268 100644 >> --- a/drivers/i2c/busses/i2c-i801.c >> +++ b/drivers/i2c/busses/i2c-i801.c >> @@ -204,6 +204,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,8 +543,7 @@ 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)) { >> + 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, >> @@ -697,8 +698,7 @@ 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) { >> + if (len == SMBUS_LEN_SENTINEL) { >> len = inb_p(SMBHSTDAT0(priv)); >> if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) { >> dev_err(&priv->pci_dev->dev, >> @@ -805,7 +805,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; > > This patch is good, but a few comments for each change to tell > where the sentinel will be used and where the sentinel was set > would help to better understand the use of the sentinel. > OK > Thanks, > Andi > >> else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) >> return -EPROTO; >> >> -- >> 2.42.0 >> >>
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index a9d3dfd9e..30a2f9268 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -204,6 +204,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,8 +543,7 @@ 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)) { + 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, @@ -697,8 +698,7 @@ 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) { + if (len == SMBUS_LEN_SENTINEL) { len = inb_p(SMBHSTDAT0(priv)); if (len < 1 || len > I2C_SMBUS_BLOCK_MAX) { dev_err(&priv->pci_dev->dev, @@ -805,7 +805,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; 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> --- drivers/i2c/busses/i2c-i801.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)