[PATCH 1/3] spi: Add a spi_w8r16be() helper

[PATCH 1/3] spi: Add a spi_w8r16be() helper

[Lars-Peter Clausen]

This patch adds a new spi_w8r16be() helper, which is similar to spi_w8r16()
except that it converts the read data word from big endian to native endianness
before returning it. The reason for introducing this new helper is that for SPI
slave devices it is quite common that the read 16 bit data word is in big
endian. So users of spi_w8r16() have to convert the result to native endianness
manually. A second reason is that in this case the endianness of the return
value of spi_w8r16() depends on its sign. If it is negative (i.e. a error code)
it is already in native endianness, if it is positive it is in big endian. The
sparse code checker doesn't like this kind of mixed endianness and special
annotations are necessary to keep it quiet (E.g. casting to be16 using __force).
Doing the conversion to native endianness in the helper function does not
require such annotations since we are not mixing different endiannesses in the
same variable.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
I don't expect any conflicts, so in my opinion it should be fine to merge the
two users also through the SPI tree. Otherwise we can also just hold them off by
one release.
---
 include/linux/spi/spi.h | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 887116d..0e0aebd 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -823,6 +823,33 @@ static inline ssize_t spi_w8r16(struct spi_device *spi, u8 cmd)
 	return (status < 0) ? status : result;
 }
 
+/**
+ * spi_w8r16be - SPI synchronous 8 bit write followed by 16 bit big-endian read
+ * @spi: device with which data will be exchanged
+ * @cmd: command to be written before data is read back
+ * Context: can sleep
+ *
+ * This returns the (unsigned) sixteen bit number returned by the device in cpu
+ * endianness, or else a negative error code. Callable only from contexts that
+ * can sleep.
+ *
+ * This function is similar to spi_w8r16, with the exception that it will
+ * convert the read 16 bit data word from big-endian to native endianness.
+ *
+ */
+static inline ssize_t spi_w8r16be(struct spi_device *spi, u8 cmd)
+
+{
+	ssize_t status;
+	__be16 result;
+
+	status = spi_write_then_read(spi, &cmd, 1, &result, 2);
+	if (status < 0)
+		return status;
+
+	return be16_to_cpu(result);
+}
+
 /*---------------------------------------------------------------------------*/
 
 /*
-- 
1.8.0

[PATCH 2/3] hwmon: (adt7310) Use spi_w8r16be() instead spi_w8r16()

[Lars-Peter Clausen]

Using spi_w8r16be() instead of spi_w8r16() in this driver makes a code a bit
shorter and cleaner.

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/hwmon/adt7310.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/hwmon/adt7310.c b/drivers/hwmon/adt7310.c
index da5f078..5994cf6 100644
--- a/drivers/hwmon/adt7310.c
+++ b/drivers/hwmon/adt7310.c
@@ -42,13 +42,8 @@ static const u8 adt7310_reg_table[] = {
 static int adt7310_spi_read_word(struct device *dev, u8 reg)
 {
 	struct spi_device *spi = to_spi_device(dev);
-	int ret;
 
-	ret = spi_w8r16(spi, AD7310_COMMAND(reg) | ADT7310_CMD_READ);
-	if (ret < 0)
-		return ret;
-
-	return be16_to_cpu((__force __be16)ret);
+	return spi_w8r16be(spi, AD7310_COMMAND(reg) | ADT7310_CMD_READ);
 }
 
 static int adt7310_spi_write_word(struct device *dev, u8 reg, u16 data)
-- 
1.8.0

[PATCH 3/3] staging:iio:ade7753/ade7754/ade7759: Use spi_w8r16be() instead of spi_w8r16()

[Lars-Peter Clausen]

Using spi_w8r16be() will do the conversion of the result from big endian to
native endian in the helper function. This makes the code a bit smaller and also
keeps sparse happy. Fixes the following sparse warnings:

	drivers/staging/iio/meter/ade7753.c:97:29: warning: incorrect type in argument 1 (different base types)
	drivers/staging/iio/meter/ade7753.c:97:29:    expected restricted __be16 const [usertype] *p
	drivers/staging/iio/meter/ade7753.c:97:29:    got unsigned short [usertype] *val

	drivers/staging/iio/meter/ade7754.c:97:29: warning: incorrect type in argument 1 (different base types)
	drivers/staging/iio/meter/ade7754.c:97:29:    expected restricted __be16 const [usertype] *p
	drivers/staging/iio/meter/ade7754.c:97:29:    got unsigned short [usertype] *val

	drivers/staging/iio/meter/ade7759.c:97:29: warning: incorrect type in argument 1 (different base types)
	drivers/staging/iio/meter/ade7759.c:97:29:    expected restricted __be16 const [usertype] *p
	drivers/staging/iio/meter/ade7759.c:97:29:    got unsigned short [usertype] *val

Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
---
 drivers/staging/iio/meter/ade7753.c | 3 +--
 drivers/staging/iio/meter/ade7754.c | 3 +--
 drivers/staging/iio/meter/ade7759.c | 3 +--
 3 files changed, 3 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
index 6200335..00492ca 100644
--- a/drivers/staging/iio/meter/ade7753.c
+++ b/drivers/staging/iio/meter/ade7753.c
@@ -86,7 +86,7 @@ static int ade7753_spi_read_reg_16(struct device *dev,
 	struct ade7753_state *st = iio_priv(indio_dev);
 	ssize_t ret;
 
-	ret = spi_w8r16(st->us, ADE7753_READ_REG(reg_address));
+	ret = spi_w8r16be(st->us, ADE7753_READ_REG(reg_address));
 	if (ret < 0) {
 		dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
 			reg_address);
@@ -94,7 +94,6 @@ static int ade7753_spi_read_reg_16(struct device *dev,
 	}
 
 	*val = ret;
-	*val = be16_to_cpup(val);
 
 	return 0;
 }
diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
index 2e046f6..e0aa13a 100644
--- a/drivers/staging/iio/meter/ade7754.c
+++ b/drivers/staging/iio/meter/ade7754.c
@@ -86,7 +86,7 @@ static int ade7754_spi_read_reg_16(struct device *dev,
 	struct ade7754_state *st = iio_priv(indio_dev);
 	int ret;
 
-	ret = spi_w8r16(st->us, ADE7754_READ_REG(reg_address));
+	ret = spi_w8r16be(st->us, ADE7754_READ_REG(reg_address));
 	if (ret < 0) {
 		dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
 			reg_address);
@@ -94,7 +94,6 @@ static int ade7754_spi_read_reg_16(struct device *dev,
 	}
 
 	*val = ret;
-	*val = be16_to_cpup(val);
 
 	return 0;
 }
diff --git a/drivers/staging/iio/meter/ade7759.c b/drivers/staging/iio/meter/ade7759.c
index 145f896..ea0c9de 100644
--- a/drivers/staging/iio/meter/ade7759.c
+++ b/drivers/staging/iio/meter/ade7759.c
@@ -86,7 +86,7 @@ static int ade7759_spi_read_reg_16(struct device *dev,
 	struct ade7759_state *st = iio_priv(indio_dev);
 	int ret;
 
-	ret = spi_w8r16(st->us, ADE7759_READ_REG(reg_address));
+	ret = spi_w8r16be(st->us, ADE7759_READ_REG(reg_address));
 	if (ret < 0) {
 		dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
 			reg_address);
@@ -94,7 +94,6 @@ static int ade7759_spi_read_reg_16(struct device *dev,
 	}
 
 	*val = ret;
-	*val = be16_to_cpup(val);
 
 	return 0;
 }
-- 
1.8.0

Re: [PATCH 2/3] hwmon: (adt7310) Use spi_w8r16be() instead spi_w8r16()

[Guenter Roeck]

On Fri, Sep 27, 2013 at 04:34:28PM +0200, Lars-Peter Clausen wrote:
> Using spi_w8r16be() instead of spi_w8r16() in this driver makes a code a bit
> shorter and cleaner.
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>

Acked-by: Guenter Roeck <linux@roeck-us.net>

Would this patch go through the SPI tree ?

Thanks,
Guenter

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Mark Brown]

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

On Fri, Sep 27, 2013 at 04:34:27PM +0200, Lars-Peter Clausen wrote:
> This patch adds a new spi_w8r16be() helper, which is similar to spi_w8r16()
> except that it converts the read data word from big endian to native endianness
> before returning it. The reason for introducing this new helper is that for SPI

This actually feels like a bug in spi_w8r16() - I'd expect it to be
returning native endian in the first place since in SPI a word is by
default big endian so I'd expect this to have more of a register I/O
model.  It certainly seems to match what almost all of the users are
doing.

That said I'm not sure this is worth fixing in which case the new API
makes sense.  Or converting the users to regmap I guess.

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

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Lars-Peter Clausen]

On 09/27/2013 08:34 PM, Mark Brown wrote:
> On Fri, Sep 27, 2013 at 04:34:27PM +0200, Lars-Peter Clausen wrote:
>> This patch adds a new spi_w8r16be() helper, which is similar to spi_w8r16()
>> except that it converts the read data word from big endian to native endianness
>> before returning it. The reason for introducing this new helper is that for SPI
> 
> This actually feels like a bug in spi_w8r16() - I'd expect it to be
> returning native endian in the first place since in SPI a word is by
> default big endian so I'd expect this to have more of a register I/O
> model.  It certainly seems to match what almost all of the users are
> doing.

According to the documentation of spi_w8r16() it is a feature.

	* The number is returned in wire-order, which is at least sometimes
 	* big-endian.

There seem to be at least two users though which assume that the result is
in native endianness drivers/hwmon/ads7871.c and drivers/mfd/stmpe-spi.c

> 
> That said I'm not sure this is worth fixing in which case the new API
> makes sense.  Or converting the users to regmap I guess.
> 

regmap is always my first choice, but the users are all devices having
varying register sizes, so regmap is not working that nicely.

- Lars

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Mark Brown]

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

On Fri, Sep 27, 2013 at 08:46:56PM +0200, Lars-Peter Clausen wrote:

> According to the documentation of spi_w8r16() it is a feature.

> 	* The number is returned in wire-order, which is at least sometimes
>  	* big-endian.

Indeed.  I don't think that's terribly well thought through though,
especially not now we have annotations for endianness (as you noticed!).

> There seem to be at least two users though which assume that the result is
> in native endianness drivers/hwmon/ads7871.c and drivers/mfd/stmpe-spi.c

Yeah, I saw.  The ads7871 is just going to break when run on the
opposite endianness to the one it was (hopefully) tested on since it
doesn't make any effort I saw to cope with endianness.  Looking at the
history it's not terribly obvious which that was but it'd be surprising
to see a little endian register...

STMPE is doing a byte swap so it's another user for your function -
it's for ARM systems so it'll be assuming that little endian is the
native format.

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

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Lars-Peter Clausen]

On 09/27/2013 09:22 PM, Mark Brown wrote:
> On Fri, Sep 27, 2013 at 08:46:56PM +0200, Lars-Peter Clausen wrote:
> 
>> According to the documentation of spi_w8r16() it is a feature.
> 
>> 	* The number is returned in wire-order, which is at least sometimes
>>  	* big-endian.
> 
> Indeed.  I don't think that's terribly well thought through though,
> especially not now we have annotations for endianness (as you noticed!).

I wouldn't mind updating spi_w8r16() to do the conversion to big-endian.
Especially considering that a driver using the function will probably always
need to do a endian conversion anyway to work correctly on both
endiannesses. We can add a LE variant if we should ever need it.

> 
>> There seem to be at least two users though which assume that the result is
>> in native endianness drivers/hwmon/ads7871.c and drivers/mfd/stmpe-spi.c
> 
> Yeah, I saw.  The ads7871 is just going to break when run on the
> opposite endianness to the one it was (hopefully) tested on since it
> doesn't make any effort I saw to cope with endianness.  Looking at the
> history it's not terribly obvious which that was but it'd be surprising
> to see a little endian register...
> 
> STMPE is doing a byte swap so it's another user for your function -
> it's for ARM systems so it'll be assuming that little endian is the
> native format.

The STMPE driver might be a candidate for regmap. Considering that the
driver was tested on a LE system it looks as if the driver first writes a 8
bit word for the address then reads a 16 bit word and ignores the upper bits
(of the BE word) and returns the lower 8 bits as the result. But the way I
understand the datasheet the 8-bit data word would follow right after the
8-bit address has been written, so I'm not sure what's going on there.
Viresh maybe you can shed some light on this. Btw. the register write
function of the STMPE driver also seems to be not endian safe.

- Lars

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Guenter Roeck]

On Fri, Sep 27, 2013 at 08:22:33PM +0100, Mark Brown wrote:
> On Fri, Sep 27, 2013 at 08:46:56PM +0200, Lars-Peter Clausen wrote:
> 
> > According to the documentation of spi_w8r16() it is a feature.
> 
> > 	* The number is returned in wire-order, which is at least sometimes
> >  	* big-endian.
> 
> Indeed.  I don't think that's terribly well thought through though,
> especially not now we have annotations for endianness (as you noticed!).
> 
> > There seem to be at least two users though which assume that the result is
> > in native endianness drivers/hwmon/ads7871.c and drivers/mfd/stmpe-spi.c
> 
> Yeah, I saw.  The ads7871 is just going to break when run on the
> opposite endianness to the one it was (hopefully) tested on since it
> doesn't make any effort I saw to cope with endianness.  Looking at the
> history it's not terribly obvious which that was but it'd be surprising
> to see a little endian register...
> 
It might make sense to convert the ads7871 driver to an iio driver;
it doesn't look like the chip is commonly used for hardware monitoring.

At least if someone finds the time to do it ;).

Guenter

Re: [PATCH 3/3] staging:iio:ade7753/ade7754/ade7759: Use spi_w8r16be() instead of spi_w8r16()

[Jonathan Cameron]

On 09/27/13 15:34, Lars-Peter Clausen wrote:
> Using spi_w8r16be() will do the conversion of the result from big endian to
> native endian in the helper function. This makes the code a bit smaller and also
> keeps sparse happy. Fixes the following sparse warnings:
> 
> 	drivers/staging/iio/meter/ade7753.c:97:29: warning: incorrect type in argument 1 (different base types)
> 	drivers/staging/iio/meter/ade7753.c:97:29:    expected restricted __be16 const [usertype] *p
> 	drivers/staging/iio/meter/ade7753.c:97:29:    got unsigned short [usertype] *val
> 
> 	drivers/staging/iio/meter/ade7754.c:97:29: warning: incorrect type in argument 1 (different base types)
> 	drivers/staging/iio/meter/ade7754.c:97:29:    expected restricted __be16 const [usertype] *p
> 	drivers/staging/iio/meter/ade7754.c:97:29:    got unsigned short [usertype] *val
> 
> 	drivers/staging/iio/meter/ade7759.c:97:29: warning: incorrect type in argument 1 (different base types)
> 	drivers/staging/iio/meter/ade7759.c:97:29:    expected restricted __be16 const [usertype] *p
> 	drivers/staging/iio/meter/ade7759.c:97:29:    got unsigned short [usertype] *val
> 
> Signed-off-by: Lars-Peter Clausen <lars@metafoo.de>
Acked-by: Jonathan Cameron <jic23@kernel.org>

I'm also fine with the various alternative suggestions elsewhere in this thread.
> ---
>  drivers/staging/iio/meter/ade7753.c | 3 +--
>  drivers/staging/iio/meter/ade7754.c | 3 +--
>  drivers/staging/iio/meter/ade7759.c | 3 +--
>  3 files changed, 3 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/iio/meter/ade7753.c b/drivers/staging/iio/meter/ade7753.c
> index 6200335..00492ca 100644
> --- a/drivers/staging/iio/meter/ade7753.c
> +++ b/drivers/staging/iio/meter/ade7753.c
> @@ -86,7 +86,7 @@ static int ade7753_spi_read_reg_16(struct device *dev,
>  	struct ade7753_state *st = iio_priv(indio_dev);
>  	ssize_t ret;
>  
> -	ret = spi_w8r16(st->us, ADE7753_READ_REG(reg_address));
> +	ret = spi_w8r16be(st->us, ADE7753_READ_REG(reg_address));
>  	if (ret < 0) {
>  		dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
>  			reg_address);
> @@ -94,7 +94,6 @@ static int ade7753_spi_read_reg_16(struct device *dev,
>  	}
>  
>  	*val = ret;
> -	*val = be16_to_cpup(val);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/iio/meter/ade7754.c b/drivers/staging/iio/meter/ade7754.c
> index 2e046f6..e0aa13a 100644
> --- a/drivers/staging/iio/meter/ade7754.c
> +++ b/drivers/staging/iio/meter/ade7754.c
> @@ -86,7 +86,7 @@ static int ade7754_spi_read_reg_16(struct device *dev,
>  	struct ade7754_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	ret = spi_w8r16(st->us, ADE7754_READ_REG(reg_address));
> +	ret = spi_w8r16be(st->us, ADE7754_READ_REG(reg_address));
>  	if (ret < 0) {
>  		dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
>  			reg_address);
> @@ -94,7 +94,6 @@ static int ade7754_spi_read_reg_16(struct device *dev,
>  	}
>  
>  	*val = ret;
> -	*val = be16_to_cpup(val);
>  
>  	return 0;
>  }
> diff --git a/drivers/staging/iio/meter/ade7759.c b/drivers/staging/iio/meter/ade7759.c
> index 145f896..ea0c9de 100644
> --- a/drivers/staging/iio/meter/ade7759.c
> +++ b/drivers/staging/iio/meter/ade7759.c
> @@ -86,7 +86,7 @@ static int ade7759_spi_read_reg_16(struct device *dev,
>  	struct ade7759_state *st = iio_priv(indio_dev);
>  	int ret;
>  
> -	ret = spi_w8r16(st->us, ADE7759_READ_REG(reg_address));
> +	ret = spi_w8r16be(st->us, ADE7759_READ_REG(reg_address));
>  	if (ret < 0) {
>  		dev_err(&st->us->dev, "problem when reading 16 bit register 0x%02X",
>  			reg_address);
> @@ -94,7 +94,6 @@ static int ade7759_spi_read_reg_16(struct device *dev,
>  	}
>  
>  	*val = ret;
> -	*val = be16_to_cpup(val);
>  
>  	return 0;
>  }
> 

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Mark Brown]

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

On Fri, Sep 27, 2013 at 10:01:36PM +0200, Lars-Peter Clausen wrote:
> On 09/27/2013 09:22 PM, Mark Brown wrote:

> > Indeed.  I don't think that's terribly well thought through though,
> > especially not now we have annotations for endianness (as you noticed!).

> I wouldn't mind updating spi_w8r16() to do the conversion to big-endian.
> Especially considering that a driver using the function will probably always
> need to do a endian conversion anyway to work correctly on both
> endiannesses. We can add a LE variant if we should ever need it.

I think that's probably the way to go, the API seems to error prone as
it is.

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

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Lars-Peter Clausen]

On 09/29/2013 02:30 PM, Mark Brown wrote:
> On Fri, Sep 27, 2013 at 10:01:36PM +0200, Lars-Peter Clausen wrote:
>> On 09/27/2013 09:22 PM, Mark Brown wrote:
> 
>>> Indeed.  I don't think that's terribly well thought through though,
>>> especially not now we have annotations for endianness (as you noticed!).
> 
>> I wouldn't mind updating spi_w8r16() to do the conversion to big-endian.
>> Especially considering that a driver using the function will probably always
>> need to do a endian conversion anyway to work correctly on both
>> endiannesses. We can add a LE variant if we should ever need it.
> 
> I think that's probably the way to go, the API seems to error prone as
> it is.

It looks as if for the ads7871 the wire order is actually little endian.
I'm still not sure about the STMPE as the datasheet and code seem to
contradict each other. But considering that the code probably was tested on
a LE platform, the driver also assumes that spi_w8r16() returns a little
endian word. Maybe we can merge this series as it is, then add a LE version
for the ads7871 and STMPE driver and afterward remove the wire-order
spi_w8r16().

- Lars

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Mark Brown]

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

On Thu, Oct 03, 2013 at 12:39:35PM +0200, Lars-Peter Clausen wrote:
> On 09/29/2013 02:30 PM, Mark Brown wrote:

> > I think that's probably the way to go, the API seems to error prone as
> > it is.

> It looks as if for the ads7871 the wire order is actually little endian.

Why do hardware engineers get all the best drugs?

> endian word. Maybe we can merge this series as it is, then add a LE version
> for the ads7871 and STMPE driver and afterward remove the wire-order
> spi_w8r16().

Yeah, that's probably about as good as it gets.

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

Re: [PATCH 1/3] spi: Add a spi_w8r16be() helper

[Mark Brown]

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

On Fri, Sep 27, 2013 at 04:34:27PM +0200, Lars-Peter Clausen wrote:
> This patch adds a new spi_w8r16be() helper, which is similar to spi_w8r16()
> except that it converts the read data word from big endian to native endianness
> before returning it. The reason for introducing this new helper is that for SPI

Applied, thanks.

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

Re: [PATCH 2/3] hwmon: (adt7310) Use spi_w8r16be() instead spi_w8r16()

[Mark Brown]

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

On Fri, Sep 27, 2013 at 04:34:28PM +0200, Lars-Peter Clausen wrote:
> Using spi_w8r16be() instead of spi_w8r16() in this driver makes a code a bit
> shorter and cleaner.

Applied, thanks.

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