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
