Re: [PATCH] regmap: Fix i2c word access when using SMBus access functions

[Guenter Roeck <linux@roeck-us.net>]

On Tue, Feb 03, 2015 at 04:09:28PM +0000, Mark Brown wrote:
> On Tue, Feb 03, 2015 at 06:21:45AM -0800, Guenter Roeck wrote:
> > On 02/03/2015 03:42 AM, Mark Brown wrote:
> 
> > >Perhaps it just needs to be more explicit about how it's handling native
> > >endian?  I didn't spot it.
> 
> > Thinking about it, we should actually reject requests for _NATIVE. SMBus
> > 16 bit accesses are either little endian or big endian.
> 
> Yes, that makes sense.  It's also hard to see a use case for _NATIVE and
> smbus.
> 
> > >>I thought about that, but since the smbus functions perform endianness
> > >>conversion it would mean that I would have to undo that conversion just
> > >>to have it done again.
> 
> > >No, the whole point is that by doing this you avoid any endianness
> > >conversions or formatting in the framework at all so you can just use
> > >the smbus functions to handle the formatting.
> 
> > Ah, guess I got confused. The SMBus accesses are already using reg_read
> > and reg_write.
> 
> It's also possible that you've spotted some bug, wouldn't be the first
> time.  Was this spotted by code inspection or by running into a problem?

Oh, I did find a bug; that is the whole reason for this patch. I converted a
driver (hwmon/ads7828) to use regmap. The result works fine with a real chip
connected to an i2c controller supporting direct i2c accesses. However, my
module test code using i2c-stub (which only supports smbus accesses) fails.

In the current code, the direct i2c access functions assume that the chip
reports data MSB first. The SMBus word access functions, however, assume
LSB first. The ADS7828 reports data MSB first, so it works with the i2c
access functions but not with the currently used SMBus access functions.

A simpler fix would have been to use i2c_smbus_read_word_swapped and
i2c_smbus_write_word_swapped instead of i2c_smbus_read_word_data and
i2c_smbus_write_word_data, but then we would end up not supporting
SMBus devices which _do_ report data LSB first.

Guenter