Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.

From: Daniel Stodden <daniel.stodden@gmail.com>
To: Wolfram Sang <wsa@kernel.org>
Cc: linux-i2c@vger.kernel.org, jdelvare@suse.de
Subject: Re: [RFC PATCH] i2c: Support Smbus 3.0 block sizes up to 255 bytes.
Date: Tue, 28 Jul 2020 04:40:22 -0700	[thread overview]
Message-ID: <14454F49-740B-4423-A2F9-4B00B18A9A74@gmail.com> (raw)
In-Reply-To: <20200728111602.GC980@ninjato>

> On Jul 28, 2020, at 4:16 AM, Wolfram Sang <wsa@kernel.org> wrote:
> 
> 
>> * Allocated bit 4 (I2C_SMBUS3_BLOCK=0x10), to simplify Smbus2
>>   compatibility: I2C_SMBUS_*BLOCK* = (<old type>|0x10)
> 
> I think the code becomes easier to understand, if we use new transfer
> types a bit more explicitly. Also, I am not sure of the extra bit
> because it is not clearly visible that types >= 16 and <= 31 will have a
> special meaning. We could do like this if we sacrifice one number for
> an unused BROKEN with 255 byte:
> 
> -#define I2C_SMBUS_BLOCK_DATA	    5
> +#define I2C_SMBUS2_BLOCK_DATA	    5 /* 32 byte only, deprecated */
> -#define I2C_SMBUS_I2C_BLOCK_BROKEN  6
> +#define I2C_SMBUS2_I2C_BLOCK_BROKEN  6 /* 32 byte only, deprecated */
> -#define I2C_SMBUS_BLOCK_PROC_CALL   7		/* SMBus 2.0 */
> +#define I2C_SMBUS2_BLOCK_PROC_CALL   7		/* SMBus 2.0, 32 byte only, deprecated */
> -#define I2C_SMBUS_I2C_BLOCK_DATA    8
> +#define I2C_SMBUS2_I2C_BLOCK_DATA    8 /* 32 byte only, deprecated */
> 
> +#define I2C_SMBUS_BLOCK_DATA		9
> +#define I2C_SMBUS_I2C_BLOCK_BROKEN	10 /* FIXME: probably say "don't use" here
> +#define I2C_SMBUS_BLOCK_PROC_CALL	11 /* SMBus >= 2.0 */
> +#define I2C_SMBUS_I2C_BLOCK_DATA	12
>> 
> 
>> +	user_len = kmalloc_array(nmsgs, sizeof(*user_len), GFP_KERNEL);
>> +	if (!user_len) {
>> +		res = -ENOMEM;
>> +		goto out;
>> +	}
> 
> Maybe on stack? I2C_RDWR_IOCTL_MAX_MSGS will ensure this will stay at a
> sane value.
> 
>> @@ -313,7 +357,19 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>> 		union i2c_smbus_data __user *data)
>> {
>> 	union i2c_smbus_data temp = {};
>> -	int datasize, res;
>> +	int block_max, datasize, res;
>> +
> 
> 'size' is really a misleading name :(

Yep. :/

> +	if (size <= I2C_SMBUS2_I2C_BLOCK_DATA) {
> +		if (size >= I2C_SMBUS2_BLOCK_DATA)
> +			size += I2C_SMBUS_BLOCK_DATA - I2C_SMBUS2_BLOCK_DATA;
> +		block_max = I2C_SMBUS_BLOCK_MAX;
> +	} else {
> +		block_max = I2C_SMBUS3_BLOCK_MAX;
> +	}
> 
> Would this work, too?

“3” ;)

But I get what you mean.

I’m not too passionate about the bit flip. Adding relative offsets would work for me too.

In fact, if we just want to keep a full switch (size) {} and map {9, 10, 11} to {5, 7, 8},
(i.e. no dummy-broken), my world wouldn’t collapse yet.

Daniel