[alsa-devel] [PATCH 1/2] ASoC: Remove byte swap in 4x12 SPI write
snd_soc_4_12_spi_write() contains a byte swap. Since this code was written for an Analog CODEC on a Blackfin reference board it appears that this is done because while Blackfin is little endian the CODEC is big endian (as are most CODECs).
Push this up into the generic 4x12 write function and use cpu_to_be16() to do the byte swap so things are more regular and things work on both CPU endiannesses.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-cache.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index 6e5e30a..73907e5 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -101,12 +101,11 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec, static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { - u8 data[2]; + u16 data;
- data[0] = (reg << 4) | ((value >> 8) & 0x000f); - data[1] = value & 0x00ff; + data = cpu_to_be16((reg << 12) | (value & 0xffffff));
- return do_hw_write(codec, reg, value, data, 2); + return do_hw_write(codec, reg, value, &data, 2); }
#if defined(CONFIG_SPI_MASTER) @@ -115,8 +114,8 @@ static int snd_soc_4_12_spi_write(void *control_data, const char *data, { u8 msg[2];
- msg[0] = data[1]; - msg[1] = data[0]; + msg[0] = data[0]; + msg[1] = data[1];
return do_spi_write(control_data, msg, len); }
do_spi_write() is just an open coded copy of do_spi_write() so we can delete it and just call spi_write() directly. Indeed, as a result of recent refactoring all the SPI write functions are just very long wrappers around spi_write() which don't add anything except for some pointless copies so we can just use spi_write() as the hw_write operation directly. It should be as type safe to do this as it is to do the same thing with I2C and it saves us a bunch of code.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/soc-cache.c | 132 ++----------------------------------------------- 1 files changed, 4 insertions(+), 128 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index 73907e5..01e5b98 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -20,30 +20,6 @@
#include <trace/events/asoc.h>
-#if defined(CONFIG_SPI_MASTER) -static int do_spi_write(void *control_data, const void *msg, - int len) -{ - struct spi_device *spi = control_data; - struct spi_transfer t; - struct spi_message m; - - if (len <= 0) - return 0; - - spi_message_init(&m); - memset(&t, 0, sizeof t); - - t.tx_buf = msg; - t.len = len; - - spi_message_add_tail(&t, &m); - spi_sync(spi, &m); - - return len; -} -#endif - static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value, const void *data, int len) { @@ -108,21 +84,6 @@ static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg, return do_hw_write(codec, reg, value, &data, 2); }
-#if defined(CONFIG_SPI_MASTER) -static int snd_soc_4_12_spi_write(void *control_data, const char *data, - int len) -{ - u8 msg[2]; - - msg[0] = data[0]; - msg[1] = data[1]; - - return do_spi_write(control_data, msg, len); -} -#else -#define snd_soc_4_12_spi_write NULL -#endif - static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec, unsigned int reg) { @@ -140,21 +101,6 @@ static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg, return do_hw_write(codec, reg, value, data, 2); }
-#if defined(CONFIG_SPI_MASTER) -static int snd_soc_7_9_spi_write(void *control_data, const char *data, - int len) -{ - u8 msg[2]; - - msg[0] = data[0]; - msg[1] = data[1]; - - return do_spi_write(control_data, msg, len); -} -#else -#define snd_soc_7_9_spi_write NULL -#endif - static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { @@ -173,21 +119,6 @@ static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec, return do_hw_read(codec, reg); }
-#if defined(CONFIG_SPI_MASTER) -static int snd_soc_8_8_spi_write(void *control_data, const char *data, - int len) -{ - u8 msg[2]; - - msg[0] = data[0]; - msg[1] = data[1]; - - return do_spi_write(control_data, msg, len); -} -#else -#define snd_soc_8_8_spi_write NULL -#endif - static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) { @@ -206,22 +137,6 @@ static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec, return do_hw_read(codec, reg); }
-#if defined(CONFIG_SPI_MASTER) -static int snd_soc_8_16_spi_write(void *control_data, const char *data, - int len) -{ - u8 msg[3]; - - msg[0] = data[0]; - msg[1] = data[1]; - msg[2] = data[2]; - - return do_spi_write(control_data, msg, len); -} -#else -#define snd_soc_8_16_spi_write NULL -#endif - #if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE)) static unsigned int do_i2c_read(struct snd_soc_codec *codec, void *reg, int reglen, @@ -322,22 +237,6 @@ static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg, return do_hw_write(codec, reg, value, data, 3); }
-#if defined(CONFIG_SPI_MASTER) -static int snd_soc_16_8_spi_write(void *control_data, const char *data, - int len) -{ - u8 msg[3]; - - msg[0] = data[0]; - msg[1] = data[1]; - msg[2] = data[2]; - - return do_spi_write(control_data, msg, len); -} -#else -#define snd_soc_16_8_spi_write NULL -#endif - #if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE)) static unsigned int snd_soc_16_16_read_i2c(struct snd_soc_codec *codec, unsigned int r) @@ -374,23 +273,6 @@ static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg, return do_hw_write(codec, reg, value, data, 4); }
-#if defined(CONFIG_SPI_MASTER) -static int snd_soc_16_16_spi_write(void *control_data, const char *data, - int len) -{ - u8 msg[4]; - - msg[0] = data[0]; - msg[1] = data[1]; - msg[2] = data[2]; - msg[3] = data[3]; - - return do_spi_write(control_data, msg, len); -} -#else -#define snd_soc_16_16_spi_write NULL -#endif - /* Primitive bulk write support for soc-cache. The data pointed to by * `data' needs to already be in the form the hardware expects * including any leading register specific data. Any data written @@ -420,7 +302,7 @@ static int snd_soc_hw_bulk_write_raw(struct snd_soc_codec *codec, unsigned int r #endif #if defined(CONFIG_SPI_MASTER) case SND_SOC_SPI: - ret = do_spi_write(codec->control_data, data, len); + ret = spi_write(codec->control_data, data, len); break; #endif default: @@ -439,43 +321,36 @@ static struct { int addr_bits; int data_bits; int (*write)(struct snd_soc_codec *codec, unsigned int, unsigned int); - int (*spi_write)(void *, const char *, int); unsigned int (*read)(struct snd_soc_codec *, unsigned int); unsigned int (*i2c_read)(struct snd_soc_codec *, unsigned int); } io_types[] = { { .addr_bits = 4, .data_bits = 12, .write = snd_soc_4_12_write, .read = snd_soc_4_12_read, - .spi_write = snd_soc_4_12_spi_write, }, { .addr_bits = 7, .data_bits = 9, .write = snd_soc_7_9_write, .read = snd_soc_7_9_read, - .spi_write = snd_soc_7_9_spi_write, }, { .addr_bits = 8, .data_bits = 8, .write = snd_soc_8_8_write, .read = snd_soc_8_8_read, .i2c_read = snd_soc_8_8_read_i2c, - .spi_write = snd_soc_8_8_spi_write, }, { .addr_bits = 8, .data_bits = 16, .write = snd_soc_8_16_write, .read = snd_soc_8_16_read, .i2c_read = snd_soc_8_16_read_i2c, - .spi_write = snd_soc_8_16_spi_write, }, { .addr_bits = 16, .data_bits = 8, .write = snd_soc_16_8_write, .read = snd_soc_16_8_read, .i2c_read = snd_soc_16_8_read_i2c, - .spi_write = snd_soc_16_8_spi_write, }, { .addr_bits = 16, .data_bits = 16, .write = snd_soc_16_16_write, .read = snd_soc_16_16_read, .i2c_read = snd_soc_16_16_read_i2c, - .spi_write = snd_soc_16_16_spi_write, }, };
@@ -536,8 +411,9 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, break;
case SND_SOC_SPI: - if (io_types[i].spi_write) - codec->hw_write = io_types[i].spi_write; +#ifdef CONFIG_SPI_MASTER + codec->hw_write = (hw_write_t)spi_write; +#endif
codec->control_data = container_of(codec->dev, struct spi_device,
On 05/11/2011 04:53 AM, Mark Brown wrote:
do_spi_write() is just an open coded copy of do_spi_write() so we can delete it and just call spi_write() directly. Indeed, as a result of recent refactoring all the SPI write functions are just very long wrappers around spi_write() which don't add anything except for some pointless copies so we can just use spi_write() as the hw_write operation directly.
This won't work. spi_write returns 0 or an error, but the snd_soc_cache framework expects the hw_write callback to return the number of bytes transferred. We need to keep do_spi_write to get i2c_transfer like semantics for the return value. The two patches[1] I send last weekm which do basically the same as your patch, but keep do_spi_write, work fine though.
- Lars
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2011-May/039467.html http://mailman.alsa-project.org/pipermail/alsa-devel/2011-May/039468.html
On Wed, May 11, 2011 at 10:16:04AM -0700, Lars-Peter Clausen wrote:
This won't work. spi_write returns 0 or an error, but the snd_soc_cache framework expects the hw_write callback to return the number of bytes transferred. We need to keep do_spi_write to get i2c_transfer like semantics for the return value. The two patches[1] I send last weekm which do basically the same as your patch, but keep do_spi_write, work fine though.
Oh, fail. We can still improve on the previous code by actually using spi_write() rather than open coding it ourselves which is verbose, I'll send a patch shortly.
On Wed, 2011-05-11 at 13:53 +0200, Mark Brown wrote:
snd_soc_4_12_spi_write() contains a byte swap. Since this code was written for an Analog CODEC on a Blackfin reference board it appears that this is done because while Blackfin is little endian the CODEC is big endian (as are most CODECs).
Push this up into the generic 4x12 write function and use cpu_to_be16() to do the byte swap so things are more regular and things work on both CPU endiannesses.
Signed-off-by: Mark Brown broonie@opensource.wolfsonmicro.com
sound/soc/soc-cache.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index 6e5e30a..73907e5 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -101,12 +101,11 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec, static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) {
- u8 data[2];
- u16 data;
- data[0] = (reg << 4) | ((value >> 8) & 0x000f);
- data[1] = value & 0x00ff;
- data = cpu_to_be16((reg << 12) | (value & 0xffffff));
- return do_hw_write(codec, reg, value, data, 2);
- return do_hw_write(codec, reg, value, &data, 2);
}
#if defined(CONFIG_SPI_MASTER) @@ -115,8 +114,8 @@ static int snd_soc_4_12_spi_write(void *control_data, const char *data, { u8 msg[2];
- msg[0] = data[1];
- msg[1] = data[0];
msg[0] = data[0];
msg[1] = data[1];
return do_spi_write(control_data, msg, len);
}
Both
Acked-by: Liam Girdwood lrg@ti.com
On 05/11/2011 04:53 AM, Mark Brown wrote:
snd_soc_4_12_spi_write() contains a byte swap. Since this code was written for an Analog CODEC on a Blackfin reference board it appears that this is done because while Blackfin is little endian the CODEC is big endian (as are most CODECs).
I've probably not properly explained the problem in my previous patch. The problem is not due to the codec being big endian and the cpu being little endian. The problem is due to how 16-bit SPI transfers work on linux and how snd_soc_cache stores the data which is going to be send.
To quote from include/linux/spi/spi.h * In-memory data values are always in native CPU byte order, translated * from the wire byte order (big-endian except with SPI_LSB_FIRST). So * for example when bits_per_word is sixteen, buffers are 2N bytes long * (@len = 2N) and hold N sixteen bit words in CPU byte order.
So when we work with a 16-bit transfer size, the data which is going to be transfer has to be a u16 array in _native_ endianes.
ASoC though uses a u8 array. So when this array is interpreted as a u16 array instead the values will be in big-endian. As a result using 16-bit spi transfers on a little-endian machine will send out the data byte-swapped. Your patch does not change since.
sound/soc/soc-cache.c | 11 +++++------ 1 files changed, 5 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index 6e5e30a..73907e5 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -101,12 +101,11 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec, static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value) {
- u8 data[2];
- u16 data;
- data[0] = (reg << 4) | ((value >> 8) & 0x000f);
- data[1] = value & 0x00ff;
Since this is basically an open-coded cpu_to_be16 ...
- data = cpu_to_be16((reg << 12) | (value & 0xffffff));
... with this the result will be the same as before. Both on big- nor little-endian. (Except for that the mask is wrong and should rather be 0xfff)
- return do_hw_write(codec, reg, value, data, 2);
- return do_hw_write(codec, reg, value, &data, 2);
}
On Wed, May 11, 2011 at 10:35:37AM -0700, Lars-Peter Clausen wrote:
I've probably not properly explained the problem in my previous patch. endian.
This is totally unrelated to your previous patch (as is the last one), I'm doing some different work on the physical I/O code and frankly hadn't thought you had any patches you hadn't resent and got applied yet.
The problem is due to how 16-bit SPI transfers work on linux and how snd_soc_cache stores the data which is going to be send.
We shouldn't be using 16 bit transfers at all, that's insane. If the board has gone and done that that's silly, and if drivers are relying on the board having done that then it's at best incredibly fragile. There certainly aren't any which check the word size.
ASoC though uses a u8 array. So when this array is interpreted as a u16 array instead the values will be in big-endian. As a result using 16-bit spi transfers on a little-endian machine will send out the data byte-swapped. Your patch does not change since.
Right, if the SPI controllers are being set up with anything other than 8 bit word sizes in transfers this is going to fall over. However, since we can't rely on SPI controllers supporting anything other than 8 bit transfers and we don't currently have a sensible way of negotiating the word size with the controller driver anyway I'd really expect us to only ever use 8 bit words without a bunch more infrastructure work.
On 05/11/2011 10:48 AM, Mark Brown wrote:
On Wed, May 11, 2011 at 10:35:37AM -0700, Lars-Peter Clausen wrote:
I've probably not properly explained the problem in my previous patch. endian.
This is totally unrelated to your previous patch (as is the last one), I'm doing some different work on the physical I/O code and frankly hadn't thought you had any patches you hadn't resent and got applied yet.
Ok, since the patches were basically identical to the patches I send the other week, I thought they were related.
The problem is due to how 16-bit SPI transfers work on linux and how snd_soc_cache stores the data which is going to be send.
We shouldn't be using 16 bit transfers at all, that's insane. If the board has gone and done that that's silly, and if drivers are relying on the board having done that then it's at best incredibly fragile. There certainly aren't any which check the word size.
ASoC though uses a u8 array. So when this array is interpreted as a u16 array instead the values will be in big-endian. As a result using 16-bit spi transfers on a little-endian machine will send out the data byte-swapped. Your patch does not change since.
Right, if the SPI controllers are being set up with anything other than 8 bit word sizes in transfers this is going to fall over. However, since we can't rely on SPI controllers supporting anything other than 8 bit transfers and we don't currently have a sensible way of negotiating the word size with the controller driver anyway I'd really expect us to only ever use 8 bit words without a bunch more infrastructure work.
Ok, could you comment again on this[1] then? Because what the patch did was switching the codec from 16-bit to 8-bit transfers, because of the issues described above.
Thanks - Lars
[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2011-May/039469.html
On Wed, May 11, 2011 at 11:02:26AM -0700, Lars-Peter Clausen wrote:
Ok, could you comment again on this[1] then? Because what the patch did was switching the codec from 16-bit to 8-bit transfers, because of the issues described above.
That's exactly what I'm saying we should do, yes.
participants (3)
-
Lars-Peter Clausen -
Liam Girdwood -
Mark Brown