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
