[alsa-devel] snd soc spi read/write

[Takashi Iwai]

At Fri, 05 Aug 2011 08:11:34 +0200,
Takashi Iwai wrote:
> 
> At Fri, 5 Aug 2011 10:24:43 +0800,
> Scott Jiang wrote:
> > 
> > 2011/8/4 Mark Brown <broonie at opensource.wolfsonmicro.com>:
> > > On Thu, Aug 04, 2011 at 06:24:04PM +0800, Scott Jiang wrote:
> > >
> > >> My version is linux 3.0, I found snd_soc_spi_read/write didn't work at
> > >> 16-8 bit mode.
> > >> The addr is 16bit, the function compares reg with reg_cache_size. This
> > >> certainly causes failure.
> > >> In the former version, reg &= 0xff seems right. So how to use
> > >> snd_soc_16_8_write now?
> > >
> > > reg &= 0xff is clearly broken for 16 bit register values...
> > 
> > hw_write use 16bit reg, cache use 8bit reg, in former version
> > 
> >         data[0] = (reg >> 8) & 0xff;
> >         data[1] = reg & 0xff;
> >         data[2] = value;
> > 
> >         reg &= 0xff;
> >         if (reg < codec->reg_cache_size)
> >                 cache[reg] = value;
> > 
> >         ret = codec->hw_write(codec->control_data, data, 3);
> > 
> > now in do_hw_write
> >         if (!snd_soc_codec_volatile_register(codec, reg) &&
> >             reg < codec->driver->reg_cache_size &&
> >             !codec->cache_bypass) {
> >                 ret = snd_soc_cache_write(codec, reg, value);
> >                 if (ret < 0)
> >                         return -1;
> >         }
> > reg > reg_cache_size, so will not write to cache
> 
> The problem is that the current cache code doesn't take care of
> special register-index values like 16_8 case.  In 16-8 case, usually
> only the lower 8 bit indicates the real register index while the upper
> 8 bit is some (usually constant) control bits.  Thus, it worked well
> in the former version by masking with 0xff.

... or better to say that the driver is buggy because it declares
a flat cache that fits only for the real index numbers but the actual
values range more widely.

> A quick fix would to introduce again the 0xff masking.  Of course,
> this isn't a right thing, but would be enough for the current code,
> as regmap will replace in future anyway.

So, the right fix would be to make it a sparse register array instead
of a linear flat.  (And change the initializer and the internal
def_cache not to use flat array -- the register value can be over
0x8000.)

This assumes that the register index value is same both for read and
write.  If it changes depending on the direction, the above won't
work, of course.  Then masking would be mandatory.


> > > Note that all this code will be replaced with regmap for 3.2.
> > >
> > 
> > Do you mean I must update to 3.2 to solve this problem?
> 
> Not yet :)  regmap has the same issue for now, as far as I see.
> 
> I guess reg_mask_bits is needed in struct regmap_config in addition
> for supporting such a case.

Erm, I was obvious wrong about this -- disregard my previous comment.
The problem is in the cache code.


Takashi