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
