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

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

[Guenter Roeck]

SMBUs access functions assume that 16 bit values are formatted as
little endian numbers. The direct i2c access functions in regmap,
however, assume that 16 bit values are formatted as big endian numbers.
As a result, the current code returns different values if an i2c chip's
16 bit registers are accessed through i2c access functions vs. SMBus
access functions.

Use regmap_smbus_read_word_swapped and regmap_smbus_write_word_swapped
for 16 bit SMBus accesses unless a chip is registered with val_format_endian
set to REGMAP_ENDIAN_LITTLE. In the latter case, keep using
regmap_smbus_write_word_data and regmap_smbus_read_word_data.

Cc: Jean Delvare <jdelvare@suse.de>
Signed-off-by: Guenter Roeck <linux@roeck-us.net>
---
Might be a candidate for -stable, though as far as I can see there is currently
no driver in the upstream kernel using regmap for 16-bit i2c accesses.

 drivers/base/regmap/regmap-i2c.c | 41 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 40 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
index 053150a..865f4f8 100644
--- a/drivers/base/regmap/regmap-i2c.c
+++ b/drivers/base/regmap/regmap-i2c.c
@@ -87,6 +87,42 @@ static struct regmap_bus regmap_smbus_word = {
 	.reg_read = regmap_smbus_word_reg_read,
 };
 
+static int regmap_smbus_word_read_swapped(void *context, unsigned int reg,
+					  unsigned int *val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+	int ret;
+
+	if (reg > 0xff)
+		return -EINVAL;
+
+	ret = i2c_smbus_read_word_swapped(i2c, reg);
+	if (ret < 0)
+		return ret;
+
+	*val = ret;
+
+	return 0;
+}
+
+static int regmap_smbus_word_write_swapped(void *context, unsigned int reg,
+					   unsigned int val)
+{
+	struct device *dev = context;
+	struct i2c_client *i2c = to_i2c_client(dev);
+
+	if (val > 0xffff || reg > 0xff)
+		return -EINVAL;
+
+	return i2c_smbus_write_word_swapped(i2c, reg, val);
+}
+
+static struct regmap_bus regmap_smbus_word_swapped = {
+	.reg_write = regmap_smbus_word_write_swapped,
+	.reg_read = regmap_smbus_word_read_swapped,
+};
+
 static int regmap_i2c_write(void *context, const void *data, size_t count)
 {
 	struct device *dev = context;
@@ -180,7 +216,10 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
 	else if (config->val_bits == 16 && config->reg_bits == 8 &&
 		 i2c_check_functionality(i2c->adapter,
 					 I2C_FUNC_SMBUS_WORD_DATA))
-		return &regmap_smbus_word;
+			if (config->val_format_endian == REGMAP_ENDIAN_LITTLE)
+				return &regmap_smbus_word;
+			else
+				return &regmap_smbus_word_swapped;
 	else if (config->val_bits == 8 && config->reg_bits == 8 &&
 		 i2c_check_functionality(i2c->adapter,
 					 I2C_FUNC_SMBUS_BYTE_DATA))
-- 
2.1.0

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

[Jean Delvare]

Hi Guenter,

On Sun,  1 Feb 2015 15:48:01 -0800, Guenter Roeck wrote:
> SMBUs access functions assume that 16 bit values are formatted as

Correct spelling is SMBus (lowercase U.) Also I think "16 bit" is
better spelled "16-bit" (more occurrences below.)

> little endian numbers. The direct i2c access functions in regmap,
> however, assume that 16 bit values are formatted as big endian numbers.
> As a result, the current code returns different values if an i2c chip's
> 16 bit registers are accessed through i2c access functions vs. SMBus
> access functions.
> 
> Use regmap_smbus_read_word_swapped and regmap_smbus_write_word_swapped
> for 16 bit SMBus accesses unless a chip is registered with val_format_endian
> set to REGMAP_ENDIAN_LITTLE. In the latter case, keep using
> regmap_smbus_write_word_data and regmap_smbus_read_word_data.
> 
> Cc: Jean Delvare <jdelvare@suse.de>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Might be a candidate for -stable, though as far as I can see there is currently
> no driver in the upstream kernel using regmap for 16-bit i2c accesses.
> 
>  drivers/base/regmap/regmap-i2c.c | 41 +++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/base/regmap/regmap-i2c.c b/drivers/base/regmap/regmap-i2c.c
> index 053150a..865f4f8 100644
> --- a/drivers/base/regmap/regmap-i2c.c
> +++ b/drivers/base/regmap/regmap-i2c.c
> @@ -87,6 +87,42 @@ static struct regmap_bus regmap_smbus_word = {
>  	.reg_read = regmap_smbus_word_reg_read,
>  };
>  
> +static int regmap_smbus_word_read_swapped(void *context, unsigned int reg,
> +					  unsigned int *val)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +	int ret;
> +
> +	if (reg > 0xff)
> +		return -EINVAL;
> +
> +	ret = i2c_smbus_read_word_swapped(i2c, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	*val = ret;
> +
> +	return 0;
> +}
> +
> +static int regmap_smbus_word_write_swapped(void *context, unsigned int reg,
> +					   unsigned int val)
> +{
> +	struct device *dev = context;
> +	struct i2c_client *i2c = to_i2c_client(dev);
> +
> +	if (val > 0xffff || reg > 0xff)
> +		return -EINVAL;
> +
> +	return i2c_smbus_write_word_swapped(i2c, reg, val);
> +}
> +
> +static struct regmap_bus regmap_smbus_word_swapped = {
> +	.reg_write = regmap_smbus_word_write_swapped,
> +	.reg_read = regmap_smbus_word_read_swapped,
> +};
> +
>  static int regmap_i2c_write(void *context, const void *data, size_t count)
>  {
>  	struct device *dev = context;
> @@ -180,7 +216,10 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
>  	else if (config->val_bits == 16 && config->reg_bits == 8 &&
>  		 i2c_check_functionality(i2c->adapter,
>  					 I2C_FUNC_SMBUS_WORD_DATA))
> -		return &regmap_smbus_word;
> +			if (config->val_format_endian == REGMAP_ENDIAN_LITTLE)
> +				return &regmap_smbus_word;
> +			else
> +				return &regmap_smbus_word_swapped;

Indentation looks wrong.

>  	else if (config->val_bits == 8 && config->reg_bits == 8 &&
>  		 i2c_check_functionality(i2c->adapter,
>  					 I2C_FUNC_SMBUS_BYTE_DATA))

The code itself looks sane, although I am not a regmap expert.

Reviewed-by: Jean Delvare <jdelvare@suse.de>

-- 
Jean Delvare
SUSE L3 Support

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

[Lars-Peter Clausen]

On 02/02/2015 12:48 AM, Guenter Roeck wrote:
[...]
>   static int regmap_i2c_write(void *context, const void *data, size_t count)
>   {
>   	struct device *dev = context;
> @@ -180,7 +216,10 @@ static const struct regmap_bus *regmap_get_i2c_bus(struct i2c_client *i2c,
>   	else if (config->val_bits == 16 && config->reg_bits == 8 &&
>   		 i2c_check_functionality(i2c->adapter,
>   					 I2C_FUNC_SMBUS_WORD_DATA))
> -		return &regmap_smbus_word;
> +			if (config->val_format_endian == REGMAP_ENDIAN_LITTLE)

This should probably use regmap_get_val_endian() and maybe also handle 
REGMAP_ENDIAN_NATIVE.

> +				return &regmap_smbus_word;
> +			else
> +				return &regmap_smbus_word_swapped;
>   	else if (config->val_bits == 8 && config->reg_bits == 8 &&
>   		 i2c_check_functionality(i2c->adapter,
>   					 I2C_FUNC_SMBUS_BYTE_DATA))
>

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

[Mark Brown]

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

On Mon, Feb 02, 2015 at 11:26:23AM +0100, Lars-Peter Clausen wrote:
> On 02/02/2015 12:48 AM, Guenter Roeck wrote:

> >-		return &regmap_smbus_word;
> >+			if (config->val_format_endian == REGMAP_ENDIAN_LITTLE)

> This should probably use regmap_get_val_endian() and maybe also handle
> REGMAP_ENDIAN_NATIVE.

Yes, we really ought to handle _NATIVE too (though the chances of it
being used with I2C are minimal, it's mostly for MMIO).  This also feels
like it's something that should be being handled further up the stack in
the serialization code but given that there's direct functions for this
in the smbus code perhaps it's better here.  Or perhaps the smbus
support ought to be transitioned to use the bus interface and set
reg_write() and reg_read() operations now that we can do that, it seems
like a better fit though it might break compatibility with wierd devices.

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

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

[Guenter Roeck]

On 02/02/2015 03:56 AM, Mark Brown wrote:
> On Mon, Feb 02, 2015 at 11:26:23AM +0100, Lars-Peter Clausen wrote:
>> On 02/02/2015 12:48 AM, Guenter Roeck wrote:
>
>>> -		return &regmap_smbus_word;
>>> +			if (config->val_format_endian == REGMAP_ENDIAN_LITTLE)
>
>> This should probably use regmap_get_val_endian() and maybe also handle
>> REGMAP_ENDIAN_NATIVE.
>
That means I'll have to make the function global and pass NULL
as bus argument, which ultimately means that it is going to return
either config->val_format_endian or REGMAP_ENDIAN_BIG. I didn't do that
because I thought it has no value, but no problem.

> Yes, we really ought to handle _NATIVE too (though the chances of it
> being used with I2C are minimal, it's mostly for MMIO).  This also feels

Well, we do; it is handled similar to the big endian case with the current
code. Do you think it should be handled differently ? If yes, how ?

> like it's something that should be being handled further up the stack in
> the serialization code but given that there's direct functions for this
> in the smbus code perhaps it's better here.  Or perhaps the smbus
> support ought to be transitioned to use the bus interface and set
> reg_write() and reg_read() operations now that we can do that, it seems
> like a better fit though it might break compatibility with wierd devices.
>
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.

Thanks,
Guenter

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

[Mark Brown]

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

On Mon, Feb 02, 2015 at 06:30:02AM -0800, Guenter Roeck wrote:
> On 02/02/2015 03:56 AM, Mark Brown wrote:

> >Yes, we really ought to handle _NATIVE too (though the chances of it
> >being used with I2C are minimal, it's mostly for MMIO).  This also feels

> Well, we do; it is handled similar to the big endian case with the current
> code. Do you think it should be handled differently ? If yes, how ?

Perhaps it just needs to be more explicit about how it's handling native
endian?  I didn't spot it.

> >like it's something that should be being handled further up the stack in
> >the serialization code but given that there's direct functions for this
> >in the smbus code perhaps it's better here.  Or perhaps the smbus
> >support ought to be transitioned to use the bus interface and set
> >reg_write() and reg_read() operations now that we can do that, it seems
> >like a better fit though it might break compatibility with wierd devices.

> 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.

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

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

[Guenter Roeck]

On 02/03/2015 03:42 AM, Mark Brown wrote:
> On Mon, Feb 02, 2015 at 06:30:02AM -0800, Guenter Roeck wrote:
>> On 02/02/2015 03:56 AM, Mark Brown wrote:
>
>>> Yes, we really ought to handle _NATIVE too (though the chances of it
>>> being used with I2C are minimal, it's mostly for MMIO).  This also feels
>
>> Well, we do; it is handled similar to the big endian case with the current
>> code. Do you think it should be handled differently ? If yes, how ?
>
> 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.

>>> like it's something that should be being handled further up the stack in
>>> the serialization code but given that there's direct functions for this
>>> in the smbus code perhaps it's better here.  Or perhaps the smbus
>>> support ought to be transitioned to use the bus interface and set
>>> reg_write() and reg_read() operations now that we can do that, it seems
>>> like a better fit though it might break compatibility with wierd devices.
>
>> 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.

Thanks,
Guenter

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

[Mark Brown]

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

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?

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

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

[Guenter Roeck]

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