Re: [PATCH v3 1/8] i2c: core: Add support for best effort block read emulation

[Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2726 bytes --]


> > I wonder what devices do if you do a word read beyond their end address?
> > Perhaps in odd cases we should always fall back to byte reads?
> 
> In my tests I can read beyond the end address, but I cannot be sure if this is OK for all
> devices. This was actually a suggestion from Wolfram for v1, but maybe I'm missing
> something.
> 
> Wolfram, is it safe to read one byte beyond the end address or should I better use
> only byte reads for odd lengths?

Well, from a device perspective, it is probably better to be safe than
sorry and fall back to a single byte read. That means, however, that we
need READ_WORD_DATA and READ_BYTE_DATA to be supported by the adapter.
Should be OK, I don't think there are adapters which can only read words.

> > > + */
> > > +s32 i2c_smbus_read_i2c_block_data_or_emulated(const struct i2c_client *client,
> > > +					      u8 command, u8 length, u8 *values)
> > > +{
> > > +	u8 i;
> > > +	int status;
> > > +
> > > +	if (length > I2C_SMBUS_BLOCK_MAX)
> > > +		length = I2C_SMBUS_BLOCK_MAX;
> > > +
> > > +	if (i2c_check_functionality(client->adapter,
> > > +				    I2C_FUNC_SMBUS_READ_I2C_BLOCK)) {
> > > +		return i2c_smbus_read_i2c_block_data(client, command,
> > > +						     length, values);

I am not very strict with the 80 char limit. I'd think the code is more
readable if the two statements above would be oneliners. And for some
other lines later as well.

> > > +	} else if (i2c_check_functionality(client->adapter,

No need for else since we return in the above if anyhow.

> > > +					   I2C_FUNC_SMBUS_READ_WORD_DATA)) {
> > > +		for (i = 0; i < length; i += 2) {
> > > +			status = i2c_smbus_read_word_data(client, command + i);
> > > +			if (status < 0)
> > > +				return status;
> > > +			values[i] = status & 0xff;
> > > +			if ((i + 1) < length)
> > > +				values[i + 1] = status >> 8;
> > > +		}
> > > +		if (i > length)
> > > +			return length;
> > > +		return i;
> > > +	} else if (i2c_check_functionality(client->adapter,
> > > +					   I2C_FUNC_SMBUS_READ_BYTE_DATA)) {
> > > +		for (i = 0; i < length; i++) {
> > > +			status = i2c_smbus_read_byte_data(client, command + i);
> > > +			if (status < 0)
> > > +				return status;
> > > +			values[i] = status;
> > > +		}
> > > +		return i;
> > > +	}
> > > +
> > > +	dev_err(&client->adapter->dev, "Unsupported transactions: %d,%d,%d\n",
> > > +		I2C_SMBUS_I2C_BLOCK_DATA, I2C_SMBUS_WORD_DATA,
> > > +		I2C_SMBUS_BYTE_DATA);

I don't think the %d printouts are useful. Just say that the adapter
lacks support for needed transactions. And I think the device which
reports the error should be the client device, no?

Thanks,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]