Re: ASoC updates for 2.6.37

[Takashi Iwai <tiwai@suse.de>]

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