| Message ID | a5920bf7-91ef-4cf3-b6c5-0979e9325d7a@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: i801: collection of further improvements / refactorings | expand |
Hi Heiner, ... > +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, > + u8 addr, u8 hstcmd, char read_write, int command) > +{ > + int result; > + u8 hostc; > + > + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) > + return -EPROTO; > + /* > + * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here, > + * even when reading. However if SPD Write Disable is set (Lynx Point and later), > + * the read will fail if we don't set the R/#W bit. > + */ > + i801_set_hstadd(priv, addr, > + priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); > + > + /* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */ > + if (read_write == I2C_SMBUS_READ) > + outb_p(hstcmd, SMBHSTDAT1(priv)); > + else > outb_p(hstcmd, SMBHSTCMD(priv)); > - break; > + > + if (read_write == I2C_SMBUS_WRITE) { > + /* set I2C_EN bit in configuration register */ > + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); > + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN); > + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { > + pci_err(priv->pci_dev, "I2C block read is unsupported!\n"); > + return -EOPNOTSUPP; > } These two if...else blocks can be merged. But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing something different from the original code. E.g. if command = I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a functional change. Or am I getting confused? Thanks, Andi
On 30.01.2024 01:09, Andi Shyti wrote: > Hi Heiner, > > ... > >> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, >> + u8 addr, u8 hstcmd, char read_write, int command) >> +{ >> + int result; >> + u8 hostc; >> + >> + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) >> + return -EPROTO; >> + /* >> + * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here, >> + * even when reading. However if SPD Write Disable is set (Lynx Point and later), >> + * the read will fail if we don't set the R/#W bit. >> + */ >> + i801_set_hstadd(priv, addr, >> + priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); >> + >> + /* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */ >> + if (read_write == I2C_SMBUS_READ) >> + outb_p(hstcmd, SMBHSTDAT1(priv)); >> + else >> outb_p(hstcmd, SMBHSTCMD(priv)); >> - break; >> + >> + if (read_write == I2C_SMBUS_WRITE) { >> + /* set I2C_EN bit in configuration register */ >> + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); >> + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN); >> + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { >> + pci_err(priv->pci_dev, "I2C block read is unsupported!\n"); >> + return -EOPNOTSUPP; >> } > > These two if...else blocks can be merged. > Yes, but I didn't do it because they cover different functionality. IMO it's better readable this way. > But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing > something different from the original code. E.g. if command = > I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a > functional change. Or am I getting confused? > At least there's no intentional functional change. Can you describe the functional change you see? Then it's easier to comment. And yes: All the strange and misleading function argument naming makes it quite confusing. This starts in I2C core: smbus_xfer() has an argument "command", which is typically a data value. See i2c_smbus_write_byte() Argument "size" however is actually the command. > Thanks, > Andi Heiner
On 30.01.2024 01:09, Andi Shyti wrote: > Hi Heiner, > > ... > >> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, >> + u8 addr, u8 hstcmd, char read_write, int command) >> +{ >> + int result; >> + u8 hostc; >> + >> + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) >> + return -EPROTO; >> + /* >> + * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here, >> + * even when reading. However if SPD Write Disable is set (Lynx Point and later), >> + * the read will fail if we don't set the R/#W bit. >> + */ >> + i801_set_hstadd(priv, addr, >> + priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); >> + >> + /* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */ >> + if (read_write == I2C_SMBUS_READ) >> + outb_p(hstcmd, SMBHSTDAT1(priv)); >> + else >> outb_p(hstcmd, SMBHSTCMD(priv)); >> - break; >> + >> + if (read_write == I2C_SMBUS_WRITE) { >> + /* set I2C_EN bit in configuration register */ >> + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); >> + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN); >> + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { >> + pci_err(priv->pci_dev, "I2C block read is unsupported!\n"); >> + return -EOPNOTSUPP; >> } > > These two if...else blocks can be merged. > > But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing > something different from the original code. E.g. if command = > I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a > functional change. Or am I getting confused? > I2C_SMBUS_BLOCK_DATA is handled by the new function i801_smbus_block_transaction(). What may contribute to the confusion is that there's also the command I2C_SMBUS_I2C_BLOCK_DATA, which is handled by i801_i2c_block_transaction() now. > Thanks, > Andi
Hi Heiner, On Tue, Jan 30, 2024 at 12:20:26PM +0100, Heiner Kallweit wrote: > On 30.01.2024 01:09, Andi Shyti wrote: > >> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, > >> + u8 addr, u8 hstcmd, char read_write, int command) > >> +{ > >> + int result; > >> + u8 hostc; > >> + > >> + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) > >> + return -EPROTO; > >> + /* > >> + * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here, > >> + * even when reading. However if SPD Write Disable is set (Lynx Point and later), > >> + * the read will fail if we don't set the R/#W bit. > >> + */ > >> + i801_set_hstadd(priv, addr, > >> + priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); > >> + > >> + /* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */ > >> + if (read_write == I2C_SMBUS_READ) > >> + outb_p(hstcmd, SMBHSTDAT1(priv)); > >> + else > >> outb_p(hstcmd, SMBHSTCMD(priv)); > >> - break; > >> + > >> + if (read_write == I2C_SMBUS_WRITE) { > >> + /* set I2C_EN bit in configuration register */ > >> + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); > >> + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN); > >> + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { > >> + pci_err(priv->pci_dev, "I2C block read is unsupported!\n"); > >> + return -EOPNOTSUPP; > >> } > > > > These two if...else blocks can be merged. > > > Yes, but I didn't do it because they cover different functionality. > IMO it's better readable this way. it's OK, this is a matter of taste. > > But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing > > something different from the original code. E.g. if command = > > I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a > > functional change. Or am I getting confused? > > > > At least there's no intentional functional change. > Can you describe the functional change you see? > Then it's easier to comment. I wrote it :-) when command is I2C_SMBUS_BLOCK_DATA, before it was simply doing: i801_set_hstadd(priv, addr, read_write); outb_p(hstcmd, SMBHSTCMD(priv)); while now it does: i801_set_hstadd(priv, addr, priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); if (read_write == I2C_SMBUS_READ) outb_p(hstcmd, SMBHSTDAT1(priv)); else outb_p(hstcmd, SMBHSTCMD(priv)); > And yes: All the strange and misleading function argument naming > makes it quite confusing. This starts in I2C core: you could try to play around with different diff algorithms when generating the patch. Some of them perform better when renaming functions. Andi PS. I'm not sure, though, this patch is improving readability, but I will check it again. > smbus_xfer() has an argument "command", which is typically > a data value. See i2c_smbus_write_byte() > Argument "size" however is actually the command. > > > Thanks, > > Andi > > Heiner
On 30.01.2024 23:07, Andi Shyti wrote: > Hi Heiner, > > On Tue, Jan 30, 2024 at 12:20:26PM +0100, Heiner Kallweit wrote: >> On 30.01.2024 01:09, Andi Shyti wrote: >>>> +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, >>>> + u8 addr, u8 hstcmd, char read_write, int command) >>>> +{ >>>> + int result; >>>> + u8 hostc; >>>> + >>>> + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) >>>> + return -EPROTO; >>>> + /* >>>> + * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here, >>>> + * even when reading. However if SPD Write Disable is set (Lynx Point and later), >>>> + * the read will fail if we don't set the R/#W bit. >>>> + */ >>>> + i801_set_hstadd(priv, addr, >>>> + priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); >>>> + >>>> + /* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */ >>>> + if (read_write == I2C_SMBUS_READ) >>>> + outb_p(hstcmd, SMBHSTDAT1(priv)); >>>> + else >>>> outb_p(hstcmd, SMBHSTCMD(priv)); >>>> - break; >>>> + >>>> + if (read_write == I2C_SMBUS_WRITE) { >>>> + /* set I2C_EN bit in configuration register */ >>>> + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); >>>> + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN); >>>> + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { >>>> + pci_err(priv->pci_dev, "I2C block read is unsupported!\n"); >>>> + return -EOPNOTSUPP; >>>> } >>> >>> These two if...else blocks can be merged. >>> >> Yes, but I didn't do it because they cover different functionality. >> IMO it's better readable this way. > > it's OK, this is a matter of taste. > >>> But here the case of "command == I2C_SMBUS_BLOCK_DATA" is doing >>> something different from the original code. E.g. if command = >>> I2C_SMBUS_BLOCK_DATA and read_write = READ, then there is a >>> functional change. Or am I getting confused? >>> >> >> At least there's no intentional functional change. >> Can you describe the functional change you see? >> Then it's easier to comment. > > I wrote it :-) > > when command is I2C_SMBUS_BLOCK_DATA, before it was simply doing: > > i801_set_hstadd(priv, addr, read_write); > outb_p(hstcmd, SMBHSTCMD(priv)); > > while now it does: > > i801_set_hstadd(priv, addr, > priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); > if (read_write == I2C_SMBUS_READ) > outb_p(hstcmd, SMBHSTDAT1(priv)); > else > outb_p(hstcmd, SMBHSTCMD(priv)); > That's a code snippet from new function i801_i2c_block_transaction() and not the path taken in case of I2C_SMBUS_BLOCK_DATA. I think the diff is hard to read. It's easier to look at new function i801_smbus_block_transaction() after applying the patch. Due to the change in i801_access() now i801_smbus_block_transaction() is called in case of I2C_SMBUS_BLOCK_DATA. Because of the split this function became quite simple. It does the same as before for I2C_SMBUS_BLOCK_DATA. static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, 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; else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) return -EPROTO; if (command == I2C_SMBUS_BLOCK_PROC_CALL) /* Needs to be flagged as write transaction */ i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE); else i801_set_hstadd(priv, addr, read_write); outb_p(hstcmd, SMBHSTCMD(priv)); if (priv->features & FEATURE_BLOCK_BUFFER) return i801_block_transaction_by_block(priv, data, read_write, command); else return i801_block_transaction_byte_by_byte(priv, data, read_write, command); } >> And yes: All the strange and misleading function argument naming >> makes it quite confusing. This starts in I2C core: > > you could try to play around with different diff algorithms when > generating the patch. Some of them perform better when renaming > functions. > > Andi > > PS. I'm not sure, though, this patch is improving readability, > but I will check it again. > > >> smbus_xfer() has an argument "command", which is typically >> a data value. See i2c_smbus_write_byte() >> Argument "size" however is actually the command. >> >>> Thanks, >>> Andi >> >> Heiner
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c index 915dd07e1..a9d3dfd9e 100644 --- a/drivers/i2c/busses/i2c-i801.c +++ b/drivers/i2c/busses/i2c-i801.c @@ -801,77 +801,65 @@ static int i801_simple_transaction(struct i801_priv *priv, union i2c_smbus_data return 0; } -/* Block transaction function */ -static int i801_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, - u8 addr, u8 hstcmd, char read_write, int command) +static int i801_smbus_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, + u8 addr, u8 hstcmd, char read_write, int command) { - int result = 0; - unsigned char hostc; - if (read_write == I2C_SMBUS_READ && command == I2C_SMBUS_BLOCK_DATA) data->block[0] = I2C_SMBUS_BLOCK_MAX; else if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) return -EPROTO; - switch (command) { - case I2C_SMBUS_BLOCK_DATA: - i801_set_hstadd(priv, addr, read_write); - outb_p(hstcmd, SMBHSTCMD(priv)); - break; - case I2C_SMBUS_I2C_BLOCK_DATA: - /* - * NB: page 240 of ICH5 datasheet shows that the R/#W - * bit should be cleared here, even when reading. - * However if SPD Write Disable is set (Lynx Point and later), - * the read will fail if we don't set the R/#W bit. - */ - i801_set_hstadd(priv, addr, - priv->original_hstcfg & SMBHSTCFG_SPD_WD ? - read_write : I2C_SMBUS_WRITE); - if (read_write == I2C_SMBUS_READ) { - /* NB: page 240 of ICH5 datasheet also shows - * that DATA1 is the cmd field when reading - */ - outb_p(hstcmd, SMBHSTDAT1(priv)); - } else - outb_p(hstcmd, SMBHSTCMD(priv)); - - if (read_write == I2C_SMBUS_WRITE) { - /* set I2C_EN bit in configuration register */ - pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); - pci_write_config_byte(priv->pci_dev, SMBHSTCFG, - hostc | SMBHSTCFG_I2C_EN); - } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { - dev_err(&priv->pci_dev->dev, - "I2C block read is unsupported!\n"); - return -EOPNOTSUPP; - } - break; - case I2C_SMBUS_BLOCK_PROC_CALL: + if (command == I2C_SMBUS_BLOCK_PROC_CALL) /* Needs to be flagged as write transaction */ i801_set_hstadd(priv, addr, I2C_SMBUS_WRITE); + else + i801_set_hstadd(priv, addr, read_write); + outb_p(hstcmd, SMBHSTCMD(priv)); + + if (priv->features & FEATURE_BLOCK_BUFFER) + return i801_block_transaction_by_block(priv, data, read_write, command); + else + return i801_block_transaction_byte_by_byte(priv, data, read_write, command); +} + +static int i801_i2c_block_transaction(struct i801_priv *priv, union i2c_smbus_data *data, + u8 addr, u8 hstcmd, char read_write, int command) +{ + int result; + u8 hostc; + + if (data->block[0] < 1 || data->block[0] > I2C_SMBUS_BLOCK_MAX) + return -EPROTO; + /* + * NB: page 240 of ICH5 datasheet shows that the R/#W bit should be cleared here, + * even when reading. However if SPD Write Disable is set (Lynx Point and later), + * the read will fail if we don't set the R/#W bit. + */ + i801_set_hstadd(priv, addr, + priv->original_hstcfg & SMBHSTCFG_SPD_WD ? read_write : I2C_SMBUS_WRITE); + + /* NB: page 240 of ICH5 datasheet shows that DATA1 is the cmd field when reading */ + if (read_write == I2C_SMBUS_READ) + outb_p(hstcmd, SMBHSTDAT1(priv)); + else outb_p(hstcmd, SMBHSTCMD(priv)); - break; + + if (read_write == I2C_SMBUS_WRITE) { + /* set I2C_EN bit in configuration register */ + pci_read_config_byte(priv->pci_dev, SMBHSTCFG, &hostc); + pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc | SMBHSTCFG_I2C_EN); + } else if (!(priv->features & FEATURE_I2C_BLOCK_READ)) { + pci_err(priv->pci_dev, "I2C block read is unsupported!\n"); + return -EOPNOTSUPP; } - /* Experience has shown that the block buffer can only be used for - SMBus (not I2C) block transactions, even though the datasheet - doesn't mention this limitation. */ - if ((priv->features & FEATURE_BLOCK_BUFFER) && - command != I2C_SMBUS_I2C_BLOCK_DATA) - result = i801_block_transaction_by_block(priv, data, - read_write, - command); - else - result = i801_block_transaction_byte_by_byte(priv, data, - read_write, - command); + /* Block buffer isn't supported for I2C block transactions */ + result = i801_block_transaction_byte_by_byte(priv, data, read_write, command); - if (command == I2C_SMBUS_I2C_BLOCK_DATA - && read_write == I2C_SMBUS_WRITE) { - /* restore saved configuration register value */ + /* restore saved configuration register value */ + if (read_write == I2C_SMBUS_WRITE) pci_write_config_byte(priv->pci_dev, SMBHSTCFG, hostc); - } + return result; } @@ -902,10 +890,10 @@ static s32 i801_access(struct i2c_adapter *adap, u16 addr, outb_p(inb_p(SMBAUXCTL(priv)) & (~SMBAUXCTL_CRC), SMBAUXCTL(priv)); - if (size == I2C_SMBUS_BLOCK_DATA || - size == I2C_SMBUS_I2C_BLOCK_DATA || - size == I2C_SMBUS_BLOCK_PROC_CALL) - ret = i801_block_transaction(priv, data, addr, command, read_write, size); + if (size == I2C_SMBUS_BLOCK_DATA || size == I2C_SMBUS_BLOCK_PROC_CALL) + ret = i801_smbus_block_transaction(priv, data, addr, command, read_write, size); + else if (size == I2C_SMBUS_I2C_BLOCK_DATA) + ret = i801_i2c_block_transaction(priv, data, addr, command, read_write, size); else ret = i801_simple_transaction(priv, data, addr, command, read_write, size);
i2c and smbus block transaction handling have little in common, therefore split this function to improve code readability. Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/i2c/busses/i2c-i801.c | 112 +++++++++++++++------------------- 1 file changed, 50 insertions(+), 62 deletions(-)