[alsa-devel] ASoC updates for 2.6.37
The following changes since commit 45605a87b3f34fb71bbc6446e2d49a469e9e10dd:
Merge branch 'for-2.6.37' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6 into topic/asoc (2010-10-05 07:50:11 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git for-2.6.37
Dimitris Papastamos (2): ASoC: soc-cache: Add spi_write support for all I/O types ASoC: WM8804: Retrieve the device revision and print it
Guennadi Liakhovetski (1): ASoC: sh: fix build error: terminate the platform device ID list
Mark Brown (1): Merge branch 'topic/asoc' of git://git.kernel.org/.../tiwai/sound-2.6 into for-2.6.37
Nicolas Kaiser (1): ASoC: remove duplicated include for nuc900
Troy Kisky (1): ALSA: ASoc: DaVinci Delay start of ASP to trigger
sound/soc/codecs/wm8804.c | 8 +++ sound/soc/davinci/davinci-i2s.c | 6 --- sound/soc/nuc900/nuc900-ac97.c | 1 - sound/soc/sh/fsi.c | 1 + sound/soc/soc-cache.c | 96 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 7 deletions(-)
At Tue, 5 Oct 2010 12:14:00 -0700, Mark Brown wrote:
The following changes since commit 45605a87b3f34fb71bbc6446e2d49a469e9e10dd:
Merge branch 'for-2.6.37' of git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6 into topic/asoc (2010-10-05 07:50:11 +0200)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound-2.6.git for-2.6.37
Pulled now. Thanks.
BTW, looking through the changes, I wonder why only snd_soc_4_12_spi_write has the byte swap at copying msg[]. Is it intended or a typo? If this is a typo, we can merge all snd_soc_*_spi_write() into a single function.
And, all snd_soc_x_y_read() and snd_soc_x_y_write() are fairly resemble, just data[] setup is different. They can be merged well, I suppose.
Takashi
Dimitris Papastamos (2): ASoC: soc-cache: Add spi_write support for all I/O types ASoC: WM8804: Retrieve the device revision and print it
Guennadi Liakhovetski (1): ASoC: sh: fix build error: terminate the platform device ID list
Mark Brown (1): Merge branch 'topic/asoc' of git://git.kernel.org/.../tiwai/sound-2.6 into for-2.6.37
Nicolas Kaiser (1): ASoC: remove duplicated include for nuc900
Troy Kisky (1): ALSA: ASoc: DaVinci Delay start of ASP to trigger
sound/soc/codecs/wm8804.c | 8 +++ sound/soc/davinci/davinci-i2s.c | 6 --- sound/soc/nuc900/nuc900-ac97.c | 1 - sound/soc/sh/fsi.c | 1 + sound/soc/soc-cache.c | 96 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 105 insertions(+), 7 deletions(-)
On Wed, Oct 06, 2010 at 08:24:44AM +0200, Takashi Iwai wrote:
BTW, looking through the changes, I wonder why only snd_soc_4_12_spi_write has the byte swap at copying msg[]. Is it intended or a typo? If this is a typo, we can merge all snd_soc_*_spi_write() into a single function.
It's not a byte swap, it's mangling part of the register and value together. The others are using whole numbers of bytes.
And, all snd_soc_x_y_read() and snd_soc_x_y_write() are fairly resemble, just data[] setup is different. They can be merged well, I suppose.
They also differ in the type of the cache variable. You could do something with macros but it'd not be terribly pleasant.
At Wed, 6 Oct 2010 07:34:01 +0100, Mark Brown wrote:
On Wed, Oct 06, 2010 at 08:24:44AM +0200, Takashi Iwai wrote:
BTW, looking through the changes, I wonder why only snd_soc_4_12_spi_write has the byte swap at copying msg[]. Is it intended or a typo? If this is a typo, we can merge all snd_soc_*_spi_write() into a single function.
It's not a byte swap, it's mangling part of the register and value together. The others are using whole numbers of bytes.
To avoid confusion, I meant the code below:
static int snd_soc_4_12_spi_write(void *control_data, const char *data, int len) { .... msg[0] = data[1]; msg[1] = data[0];
The others such as snd_soc_7_9_spi_write() (which also no-byte-aligned) are straight copy:
static int snd_soc_7_9_spi_write(void *control_data, const char *data, int len) { .... msg[0] = data[0]; msg[1] = data[1];
If the above difference is intentional, it should be commented somewhere.
And, all snd_soc_x_y_read() and snd_soc_x_y_write() are fairly resemble, just data[] setup is different. They can be merged well, I suppose.
They also differ in the type of the cache variable. You could do something with macros but it'd not be terribly pleasant.
Hm, this can be tricky. Meanwhile, we could have it always int, instead of different types. The amount of memory you save by supporting different size might be more than the waste of 3 bytes for 1 byte data array.
Just my $0.02, though.
Takashi
On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
If the above difference is intentional, it should be commented somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
They also differ in the type of the cache variable. You could do something with macros but it'd not be terribly pleasant.
Hm, this can be tricky. Meanwhile, we could have it always int, instead of different types. The amount of memory you save by supporting different size might be more than the waste of 3 bytes for 1 byte data array.
Given the size of the register maps for modern chips with visible DSPs in them the overhead can be non-trivial. Doing this also inhibits operations like doing bulk writes of data from cache (which we don't do right now but may wish to do in future) since the data can't just be passed to the bus I/O functions so it's not purely a memory efficiency thing.
At Wed, 6 Oct 2010 08:31:04 +0100, Mark Brown wrote:
On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
They also differ in the type of the cache variable. You could do something with macros but it'd not be terribly pleasant.
Hm, this can be tricky. Meanwhile, we could have it always int, instead of different types. The amount of memory you save by supporting different size might be more than the waste of 3 bytes for 1 byte data array.
Given the size of the register maps for modern chips with visible DSPs in them the overhead can be non-trivial. Doing this also inhibits operations like doing bulk writes of data from cache (which we don't do right now but may wish to do in future) since the data can't just be passed to the bus I/O functions so it's not purely a memory efficiency thing.
OK, that's another argument. We need C++ for a better implementation of such codes, heh ;)
Takashi
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
If the above difference is intentional, it should be commented somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Thanks, Dimitrios
At Wed, 06 Oct 2010 10:25:40 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
If the above difference is intentional, it should be commented somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Sorry, I don't understand your statement clearly. So, the byte-swap behavior in snd_soc_4_12_spi_write() is designed? If yes, why?
Takashi
On Wed, 2010-10-06 at 15:30 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 10:25:40 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
If the above difference is intentional, it should be commented somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Sorry, I don't understand your statement clearly. So, the byte-swap behavior in snd_soc_4_12_spi_write() is designed?
I meant to say that snd_soc_4_12_spi_write looks suspicious and that I'd expect it to behave similarly to snd_soc_7_9_spi_write. I don't see why the byte swapping is needed.
Thanks, Dimitrios
At Wed, 06 Oct 2010 14:40:04 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 15:30 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 10:25:40 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
If the above difference is intentional, it should be commented somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Sorry, I don't understand your statement clearly. So, the byte-swap behavior in snd_soc_4_12_spi_write() is designed?
I meant to say that snd_soc_4_12_spi_write looks suspicious and that I'd expect it to behave similarly to snd_soc_7_9_spi_write. I don't see why the byte swapping is needed.
OK, thanks for clarifying :)
Looking through git commits, this was introduced by a patch from Barry Song. Barry, could you check whether the current code is correct?
I guess this is because the original code accessed unsigned short while the new code is converted to a byte array. Maybe due to the endianess, but it looks wrong.
thanks,
Takashi
On Wed, 2010-10-06 at 15:46 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 14:40:04 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 15:30 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 10:25:40 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
If the above difference is intentional, it should be commented somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Sorry, I don't understand your statement clearly. So, the byte-swap behavior in snd_soc_4_12_spi_write() is designed?
I meant to say that snd_soc_4_12_spi_write looks suspicious and that I'd expect it to behave similarly to snd_soc_7_9_spi_write. I don't see why the byte swapping is needed.
OK, thanks for clarifying :)
Looking through git commits, this was introduced by a patch from Barry Song. Barry, could you check whether the current code is correct?
I guess this is because the original code accessed unsigned short while the new code is converted to a byte array. Maybe due to the endianess, but it looks wrong.
Looking at the commit now, I think his changes are consistent. Consider an example with reg = 0xf and value = 0xa, then his original unsigned short buf is equal to 0xf00a. If you follow the logic in snd_soc_4_12_write you will find that data[0] and data[1] are 0xf0 and 0x0a respectively. However on a say LE machine, his original buf is laid out in mem as 0af0 whereas the data buffer is laid out as f00a so he has to reverse that by swapping it in snd_soc_4_12_spi_write.
Thanks, Dimitrios
At Wed, 06 Oct 2010 15:13:42 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 15:46 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 14:40:04 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 15:30 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 10:25:40 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote:
> If the above difference is intentional, it should be commented > somewhere.
Meh, yes. Dimitris, please fix or add a comment as appropriate.
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Sorry, I don't understand your statement clearly. So, the byte-swap behavior in snd_soc_4_12_spi_write() is designed?
I meant to say that snd_soc_4_12_spi_write looks suspicious and that I'd expect it to behave similarly to snd_soc_7_9_spi_write. I don't see why the byte swapping is needed.
OK, thanks for clarifying :)
Looking through git commits, this was introduced by a patch from Barry Song. Barry, could you check whether the current code is correct?
I guess this is because the original code accessed unsigned short while the new code is converted to a byte array. Maybe due to the endianess, but it looks wrong.
Looking at the commit now, I think his changes are consistent. Consider an example with reg = 0xf and value = 0xa, then his original unsigned short buf is equal to 0xf00a. If you follow the logic in snd_soc_4_12_write you will find that data[0] and data[1] are 0xf0 and 0x0a respectively. However on a say LE machine, his original buf is laid out in mem as 0af0 whereas the data buffer is laid out as f00a so he has to reverse that by swapping it in snd_soc_4_12_spi_write.
Yeah, I don't say it's buggy. But it's wrong from a programming POV. The proper endianess handling seems missing in the current code...
Takashi
On Wed, 2010-10-06 at 16:24 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 15:13:42 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 15:46 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 14:40:04 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 15:30 +0200, Takashi Iwai wrote:
At Wed, 06 Oct 2010 10:25:40 +0100, Dimitris Papastamos wrote:
On Wed, 2010-10-06 at 08:31 +0100, Mark Brown wrote: On Wed, Oct 06, 2010 at 09:10:23AM +0200, Takashi Iwai wrote: > > > If the above difference is intentional, it should be commented > > somewhere. > > Meh, yes. Dimitris, please fix or add a comment as appropriate. >
In snd_soc_7_9_spi_write we prepare the tx buffer to be register followed by data packed into 16 bits. In snd_soc_4_12_spi_write the tx buffer is swapped. I'd expect this to be consistent between the two transfers.
Sorry, I don't understand your statement clearly. So, the byte-swap behavior in snd_soc_4_12_spi_write() is designed?
I meant to say that snd_soc_4_12_spi_write looks suspicious and that I'd expect it to behave similarly to snd_soc_7_9_spi_write. I don't see why the byte swapping is needed.
OK, thanks for clarifying :)
Looking through git commits, this was introduced by a patch from Barry Song. Barry, could you check whether the current code is correct?
I guess this is because the original code accessed unsigned short while the new code is converted to a byte array. Maybe due to the endianess, but it looks wrong.
Looking at the commit now, I think his changes are consistent. Consider an example with reg = 0xf and value = 0xa, then his original unsigned short buf is equal to 0xf00a. If you follow the logic in snd_soc_4_12_write you will find that data[0] and data[1] are 0xf0 and 0x0a respectively. However on a say LE machine, his original buf is laid out in mem as 0af0 whereas the data buffer is laid out as f00a so he has to reverse that by swapping it in snd_soc_4_12_spi_write.
Yeah, I don't say it's buggy. But it's wrong from a programming POV. The proper endianess handling seems missing in the current code...
Yup, I will wait for Mark to see what he has to comment on this and then I will make the required changes.
Thanks, Dimitrios
On Wed, Oct 06, 2010 at 04:24:18PM +0200, Takashi Iwai wrote:
Yeah, I don't say it's buggy. But it's wrong from a programming POV. The proper endianess handling seems missing in the current code...
I'll need to have a proper look and think but endianness handling for SPI is somewhat amusing - SPI is natually big endinan so controllers on little endian architectures do a byte swap to send, but only when they run in word oriented mode. If the word size is 8 bits they don't need to.
participants (3)
-
Dimitris Papastamos -
Mark Brown -
Takashi Iwai