On 07.06.2022 16:13, Jean Delvare wrote:
> Hi Heiner,
>
> On Fri, 15 Apr 2022 18:55:46 +0200, Heiner Kallweit wrote:
>> According to the datasheet the block process call requires block
>> buffer mode. The user may disable block buffer mode by module
>> parameter disable_features, in such a case we have to clear
>> FEATURE_BLOCK_PROC.
>
> In which datasheet are you seeing this? Can you point me to the
> specific section and/or quote the statement? I can't find it in the
> datasheet I'm looking at (ICH9, Intel document 316972-002) but it is
> huge and I may just be missing it.
>
I used the following datasheet:
Intel 9 Series Chipset Family PCH
330550-002
On page 211 the block process call is described.
There's a note: E32B bit in the Auxiliary Control register must be set when using this protocol.
The same note can be found on page 663.
> Also, same request as previous patch, I'd like a comment in the code,
> so that developers don't have to read the git log to figure out why this
> piece of code is there.
>
OK
> Furthermore, as far as I can see, the FEATURE_BLOCK_PROC flag only
> affects the value returned by i801_func(). i801_access() does not
> verify whether this flag is set before processing a command where size
> == I2C_SMBUS_BLOCK_PROC_CALL. I think it should? Otherwise your fix is
> only partial (will work if the device driver calls .functionality as it
> is supposed to, will fail with - I suppose - unpredictable results if
> the device driver calls .smbus_xfer directly).
>
If FEATURE_BLOCK_PROC isn't set then we would call
i801_block_transaction_byte_by_byte() according to the following code
in i801_block_transaction().
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);
And i801_block_transaction_byte_by_byte() immediately returns
-EOPNOTSUPP in case of command == I2C_SMBUS_BLOCK_PROC_CALL.
Having said that the requested check is there, it's just executed
a little bit later.
>> Signed-off-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
>> ---
>> drivers/i2c/busses/i2c-i801.c | 3 +++
>> 1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
>> index eccdc7035..1d8182901 100644
>> --- a/drivers/i2c/busses/i2c-i801.c
>> +++ b/drivers/i2c/busses/i2c-i801.c
>> @@ -1675,6 +1675,9 @@ static int i801_probe(struct pci_dev *dev, const struct pci_device_id *id)
>> }
>> priv->features &= ~disable_features;
>>
>> + if (!(priv->features & FEATURE_BLOCK_BUFFER))
>> + priv->features &= ~FEATURE_BLOCK_PROC;
>> +
>> err = pcim_enable_device(dev);
>> if (err) {
>> dev_err(&dev->dev, "Failed to enable SMBus PCI device (%d)\n",
>
> Thanks,
