Re: [PATCH 3/8] i2c: i801: make FEATURE_BLOCK_PROC dependent on FEATURE_BLOCK_BUFFER

[Heiner Kallweit <hkallweit1@gmail.com>]

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@gmail.com>
>> ---
>>  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,