snd soc spi read/write

snd soc spi read/write

[Scott Jiang]

Hi Mark,

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?

Regards,
Scott

Re: snd soc spi read/write

[Mark Brown]

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...

Note that all this code will be replaced with regmap for 3.2.

Re: snd soc spi read/write

[Scott Jiang]

2011/8/4 Mark Brown <broonie@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

>
> 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?

Re: snd soc spi read/write

[Mark Brown]

On Fri, Aug 05, 2011 at 10:24:43AM +0800, Scott Jiang wrote:

> 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);

Which is just obviously insane and buggy as with that code the same
cache slot will be used for 256 different registers that differ only in
the upper byte.

> 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

Which is exactly what we'd expect - we won't have allocated a cache
beyond register reg_cache_size and the driver is telling us not to cache
those registers.

> > 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?

I don't see any problem here.  What is the problem you're experiencing?

Re: snd soc spi read/write

[Scott Jiang]

2011/8/5 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Fri, Aug 05, 2011 at 10:24:43AM +0800, Scott Jiang wrote:
>
>> 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);
>
> Which is just obviously insane and buggy as with that code the same
> cache slot will be used for 256 different registers that differ only in
> the upper byte.
>
>> 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
>
> Which is exactly what we'd expect - we won't have allocated a cache
> beyond register reg_cache_size and the driver is telling us not to cache
> those registers.
>
>> > 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?
>
> I don't see any problem here.  What is the problem you're experiencing?
>

My register address is 0x806(8bit global addr + 8bit reg addr), for
example,  reg_cache_size is 0x20.
snd_soc_cache_write will not be called. And kernel oops in do_hw_read

BUG_ON(!codec->hw_read);

That is what I found when asoc was upgraded to 3.0, my codec is ad1938.

The key issue is register is 8 bits, hardware needs 16 bits addr. If I
change reg_cache_size to 2^16=64K, I think everything goes well.
But it will waste a lot of memory for only 32 registers.

Re: snd soc spi read/write

[Mark Brown]

On Fri, Aug 05, 2011 at 02:26:33PM +0800, Scott Jiang wrote:

> My register address is 0x806(8bit global addr + 8bit reg addr), for
> example,  reg_cache_size is 0x20.
> snd_soc_cache_write will not be called. And kernel oops in do_hw_read

> BUG_ON(!codec->hw_read);

> That is what I found when asoc was upgraded to 3.0, my codec is ad1938.

> The key issue is register is 8 bits, hardware needs 16 bits addr. If I
> change reg_cache_size to 2^16=64K, I think everything goes well.
> But it will waste a lot of memory for only 32 registers.

Oh, this is just fail.  Does the hardware have readback support?

Re: snd soc spi read/write

[Scott Jiang]

>
> Why are you replying off-list?  You should implement 16x8 SPI reads in
> the core stuff I'd expect, then disable cache for the CODEC.
>

Sorry, I just forgot to CC.
I wonder if rbtree compress type can support this kind of sparse register?
And should the patch aginst 3.0 or 3.2?

Re: snd soc spi read/write

[Mark Brown]

On Fri, Aug 05, 2011 at 04:00:52PM +0800, Scott Jiang wrote:

> > Why are you replying off-list?  You should implement 16x8 SPI reads in
> > the core stuff I'd expect, then disable cache for the CODEC.

> Sorry, I just forgot to CC.
> I wonder if rbtree compress type can support this kind of sparse register?

Yes, rbtree works fine with this.  You'd still take a hit on the size of
your register defaults though.

> And should the patch aginst 3.0 or 3.2?

3.0 can't be patched, though you could try submitting a patch to the
stable guys.  Probably best to start off with 3.1 and see where we go
from there.

Re: snd soc spi read/write

[Scott Jiang]

2011/8/5 Mark Brown <broonie@opensource.wolfsonmicro.com>:
> On Fri, Aug 05, 2011 at 04:00:52PM +0800, Scott Jiang wrote:
>
>> > Why are you replying off-list?  You should implement 16x8 SPI reads in
>> > the core stuff I'd expect, then disable cache for the CODEC.
>
>> Sorry, I just forgot to CC.
>> I wonder if rbtree compress type can support this kind of sparse register?
>
> Yes, rbtree works fine with this.  You'd still take a hit on the size of
> your register defaults though.
>
>> And should the patch aginst 3.0 or 3.2?
>
> 3.0 can't be patched, though you could try submitting a patch to the
> stable guys.  Probably best to start off with 3.1 and see where we go
> from there.
>

I got a problem here. As Takashi mentioned, our registers have a
read/write bit in the upper 8bit.
There are three methods:
1. pass different registers to snd_soc_read and snd_soc_write. But
snd_kcontrol and snd_soc_dapm_widget can't work because I pass only
one register.
2. deal with this bit in hw_read, but this will be deprecated by
others whose chip doesn't have this bit.
3. I'd like to use SND_SOC_CUS type, but it has been removed since
linux 3.0. I suggest we can reserve this type, considering SPI is a
simple "de facto" standard.

Re: snd soc spi read/write

[Mark Brown]

On Tue, Aug 09, 2011 at 11:41:30AM +0800, Scott Jiang wrote:

> There are three methods:
> 1. pass different registers to snd_soc_read and snd_soc_write. But
> snd_kcontrol and snd_soc_dapm_widget can't work because I pass only
> one register.
> 2. deal with this bit in hw_read, but this will be deprecated by
> others whose chip doesn't have this bit.
> 3. I'd like to use SND_SOC_CUS type, but it has been removed since
> linux 3.0. I suggest we can reserve this type, considering SPI is a
> simple "de facto" standard.

No, like I say we just need to teach regmap about this stuff.  It's not
that odd, it's just your hardware designers seem to want to consume
extra bandwidth on the control bus for some reason AFAICT as it looks
like all the registers are 0x8xx.

Re: snd soc spi read/write

[Takashi Iwai]

At Wed, 10 Aug 2011 01:04:48 +0900,
Mark Brown wrote:
> 
> On Tue, Aug 09, 2011 at 11:41:30AM +0800, Scott Jiang wrote:
> 
> > There are three methods:
> > 1. pass different registers to snd_soc_read and snd_soc_write. But
> > snd_kcontrol and snd_soc_dapm_widget can't work because I pass only
> > one register.
> > 2. deal with this bit in hw_read, but this will be deprecated by
> > others whose chip doesn't have this bit.
> > 3. I'd like to use SND_SOC_CUS type, but it has been removed since
> > linux 3.0. I suggest we can reserve this type, considering SPI is a
> > simple "de facto" standard.
> 
> No, like I say we just need to teach regmap about this stuff.  It's not
> that odd, it's just your hardware designers seem to want to consume
> extra bandwidth on the control bus for some reason AFAICT as it looks
> like all the registers are 0x8xx.

Actually we should handle the register index only in the lower byte
for these devices.  For example, ad193x has raw registers ranged from
0 to 0x10 with 0x800 high byte, while cs4271 has from 0 to 0x08 with
0x2000 high byte.  So the raw register fits with flat array cache
well.

And, 16bit register value is needed only for SPI.  For I2C, the upper
byte is anyway dropped.


Takashi

Re: snd soc spi read/write

[Mark Brown]

On Wed, Aug 10, 2011 at 01:54:27PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > No, like I say we just need to teach regmap about this stuff.  It's not
> > that odd, it's just your hardware designers seem to want to consume
> > extra bandwidth on the control bus for some reason AFAICT as it looks
> > like all the registers are 0x8xx.

> Actually we should handle the register index only in the lower byte
> for these devices.  For example, ad193x has raw registers ranged from

That would fall within the definition of "teach the register map about
this stuff"...

Re: snd soc spi read/write

[Takashi Iwai]

At Wed, 10 Aug 2011 23:55:50 +0900,
Mark Brown wrote:
> 
> On Wed, Aug 10, 2011 at 01:54:27PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > No, like I say we just need to teach regmap about this stuff.  It's not
> > > that odd, it's just your hardware designers seem to want to consume
> > > extra bandwidth on the control bus for some reason AFAICT as it looks
> > > like all the registers are 0x8xx.
> 
> > Actually we should handle the register index only in the lower byte
> > for these devices.  For example, ad193x has raw registers ranged from
> 
> That would fall within the definition of "teach the register map about
> this stuff"...

Yeah, I expected that.  But it also would include the changes of
register definitions in the codec driver, right?


Takashi

Re: snd soc spi read/write

[Mark Brown]

On Wed, Aug 10, 2011 at 05:00:50PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > That would fall within the definition of "teach the register map about
> > this stuff"...

> Yeah, I expected that.  But it also would include the changes of
> register definitions in the codec driver, right?

The idea is that the CODEC drivers will end up using regmap directly
once it gets cache support migrated over to it.  There's nothing ALSA
specific about the cache support.  Though right just not bothering to
cache this device (it has readback support after all) is probably good
enough.

It's a shame the bodge snuck through :/

Re: snd soc spi read/write

[Takashi Iwai]

At Thu, 11 Aug 2011 00:03:18 +0900,
Mark Brown wrote:
> 
> On Wed, Aug 10, 2011 at 05:00:50PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > That would fall within the definition of "teach the register map about
> > > this stuff"...
> 
> > Yeah, I expected that.  But it also would include the changes of
> > register definitions in the codec driver, right?
> 
> The idea is that the CODEC drivers will end up using regmap directly
> once it gets cache support migrated over to it.  There's nothing ALSA
> specific about the cache support.  Though right just not bothering to
> cache this device (it has readback support after all) is probably good
> enough.

OK, I like the idea, but it sounds a bit like a long way to go.
I guess the cache-in-regmap won't be merged in 3.1 cycle?

Basically I don't care too much about this, but the fact we leave this
being broken over two release cycles doesn't appear nice, especially
when there is a quick-n-easy fix...


thanks,

Takashi

Re: snd soc spi read/write

[Mark Brown]

On Wed, Aug 10, 2011 at 05:15:21PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > The idea is that the CODEC drivers will end up using regmap directly
> > once it gets cache support migrated over to it.  There's nothing ALSA
> > specific about the cache support.  Though right just not bothering to
> > cache this device (it has readback support after all) is probably good
> > enough.

> OK, I like the idea, but it sounds a bit like a long way to go.
> I guess the cache-in-regmap won't be merged in 3.1 cycle?

Well, regmap is only in 3.2.

> Basically I don't care too much about this, but the fact we leave this
> being broken over two release cycles doesn't appear nice, especially
> when there is a quick-n-easy fix...

I don't see a problem with the idea of just making the registers
volatile.  There's no real need to cache the registers on a small SPI
device with readback support, the caches mainly benefit I2C (which is
much slower) and devices with no readback support with some other
benefits for larger devices.

That ought to work for all versions of the framework and keeps the
driver looking like a standard driver.  There's existing stuff with the
driver doing non-standard things like the fixed default setup which have
been around for quite some time and I'd rather reduce that sort of
stuff.

Re: snd soc spi read/write

[Takashi Iwai]

At Thu, 11 Aug 2011 00:34:58 +0900,
Mark Brown wrote:
> 
> On Wed, Aug 10, 2011 at 05:15:21PM +0200, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > The idea is that the CODEC drivers will end up using regmap directly
> > > once it gets cache support migrated over to it.  There's nothing ALSA
> > > specific about the cache support.  Though right just not bothering to
> > > cache this device (it has readback support after all) is probably good
> > > enough.
> 
> > OK, I like the idea, but it sounds a bit like a long way to go.
> > I guess the cache-in-regmap won't be merged in 3.1 cycle?
> 
> Well, regmap is only in 3.2.

Oh, it's merged in 3.1 :)  Just unused by no one, yeah.

> > Basically I don't care too much about this, but the fact we leave this
> > being broken over two release cycles doesn't appear nice, especially
> > when there is a quick-n-easy fix...
> 
> I don't see a problem with the idea of just making the registers
> volatile.  There's no real need to cache the registers on a small SPI
> device with readback support, the caches mainly benefit I2C (which is
> much slower) and devices with no readback support with some other
> benefits for larger devices.

Hey, I'm not against it at all.  What I do care is to see a fix
soonish.

It'd be a quick fix that shall be removed in near future by regmap
after all, so we don't need to waste time by discussing too much about
it.

So, go ahead as you like, and let's fix it quickly in a minimal way.


thanks,

Takashi

Re: snd soc spi read/write

[Mark Brown]

On Wed, Aug 10, 2011 at 06:02:33PM +0200, Takashi Iwai wrote:
> Mark Brown wrote:

> > > OK, I like the idea, but it sounds a bit like a long way to go.
> > > I guess the cache-in-regmap won't be merged in 3.1 cycle?

> > Well, regmap is only in 3.2.

> Oh, it's merged in 3.1 :)  Just unused by no one, yeah.

I meant regmap ASoC support.

> It'd be a quick fix that shall be removed in near future by regmap
> after all, so we don't need to waste time by discussing too much about
> it.

That's why I don't want to add stuff in the driver, I would like to
avoid adding more things in the driver that need removing because I
don't have confidence that we'll easily be able to back out the
workaround later on if the driver's not really being looked after.

Re: snd soc spi read/write

[Lars-Peter Clausen]

On 08/10/2011 05:34 PM, Mark Brown wrote:
> On Wed, Aug 10, 2011 at 05:15:21PM +0200, Takashi Iwai wrote:
>> Mark Brown wrote:
> 
>>> The idea is that the CODEC drivers will end up using regmap directly
>>> once it gets cache support migrated over to it.  There's nothing ALSA
>>> specific about the cache support.  Though right just not bothering to
>>> cache this device (it has readback support after all) is probably good
>>> enough.
> 
>> OK, I like the idea, but it sounds a bit like a long way to go.
>> I guess the cache-in-regmap won't be merged in 3.1 cycle?
> 
> Well, regmap is only in 3.2.
> 
>> Basically I don't care too much about this, but the fact we leave this
>> being broken over two release cycles doesn't appear nice, especially
>> when there is a quick-n-easy fix...
> 
> I don't see a problem with the idea of just making the registers
> volatile.  There's no real need to cache the registers on a small SPI
> device with readback support, the caches mainly benefit I2C (which is
> much slower) and devices with no readback support with some other
> benefits for larger devices.
> 

The problem is that there is no read-back support out of the box. Readback
requires setting the read bit in the registers address. Since this is not the
upper-most bit, the default regmap spi read wont work.

And if we have to add our own read function we could as very well add our own
write function which simply reinstates the old caching behavior.

In my opinion it would be nice if we could add a register cache base address,
which specifies the offset at which the cache-able registers start. For example
I have a codec driver in the queue where all non-volatile registers at 0x8000
and I don't really want to add 16k of zeros to the driver for the default
register cache.

- Lars

Re: snd soc spi read/write

[Mark Brown]

On Wed, Aug 10, 2011 at 11:31:06PM +0200, Lars-Peter Clausen wrote:

> The problem is that there is no read-back support out of the box. Readback
> requires setting the read bit in the registers address. Since this is not the
> upper-most bit, the default regmap spi read wont work.

Ah, so read and write aren't symmetric?  That wasn't clear.

That's not complicated, though - like I'm sure I've said to you before
it seems like we just need to make the read bit controllable by drivers.
Some other devices need that too, and a shift of the address also
(there's one 7 bit address device I saw recently which has the address
in the top 7 bits of an 8 bit value).

Like I say I'm really not happy about adding further non-standard driver
specifics to the Analog drivers, they're already not the best and they
don't really seem to be getting much attention from anyone so I'm not
confident anyone will come along and reverse any temporary bodges.  I'd
be reasonably happy with something temporary for 3.1 if we already have
a fix in place for 3.2 but I don't have much confidence that anyone will
work on that.

> And if we have to add our own read function we could as very well add our own
> write function which simply reinstates the old caching behavior.

If the driver needs its own custom I/O it should do both read and write.

> In my opinion it would be nice if we could add a register cache base address,
> which specifies the offset at which the cache-able registers start. For example
> I have a codec driver in the queue where all non-volatile registers at 0x8000
> and I don't really want to add 16k of zeros to the driver for the default
> register cache.

I don't think it's worth adding a special case like that when fixing the
more general issues for sparse registers also solves these problems - we
still need to fix the sparse register maps anyway.  Blocking registers
together in rbtree (which we do already) means that if you've got one
block of registers at a massive offset then the block with an offset
just flops out of the code.  If we also provide a more compact way of
representing the defaults that devices with sparse registers can use
then that problem will go away too.

Re: snd soc spi read/write

[Lars-Peter Clausen]

On 08/11/2011 02:33 AM, Mark Brown wrote:
> On Wed, Aug 10, 2011 at 11:31:06PM +0200, Lars-Peter Clausen wrote:
> 
> That's not complicated, though - like I'm sure I've said to you before
> it seems like we just need to make the read bit controllable by drivers.
> Some other devices need that too, and a shift of the address also
> (there's one 7 bit address device I saw recently which has the address
> in the top 7 bits of an 8 bit value).
> 

Yes, I think I brought it up during the regmap review.

> Like I say I'm really not happy about adding further non-standard driver
> specifics to the Analog drivers, they're already not the best and they
> don't really seem to be getting much attention from anyone so I'm not
> confident anyone will come along and reverse any temporary bodges.  I'd
> be reasonably happy with something temporary for 3.1 if we already have
> a fix in place for 3.2 but I don't have much confidence that anyone will
> work on that.

So, just switch the ad193x driver to a rbcache.

> 
>> And if we have to add our own read function we could as very well add our own
>> write function which simply reinstates the old caching behavior.
> 
> If the driver needs its own custom I/O it should do both read and write.

We don't need reads if we cache writes. In the old ASoC IO code there wasn't
even SPI read support.

>> In my opinion it would be nice if we could add a register cache base address,
>> which specifies the offset at which the cache-able registers start. For example
>> I have a codec driver in the queue where all non-volatile registers at 0x8000
>> and I don't really want to add 16k of zeros to the driver for the default
>> register cache.
> 
> I don't think it's worth adding a special case like that when fixing the
> more general issues for sparse registers also solves these problems - we
> still need to fix the sparse register maps anyway.  Blocking registers
> together in rbtree (which we do already) means that if you've got one
> block of registers at a massive offset then the block with an offset
> just flops out of the code.

Yes, I had a look at the rbcache the other day. The current code doesn't seem
to be optimal. For example adjacent don't seem to be joined, so for example if
I have 3 registers and write them in the order 0,2,1 I'll still end up with 2
blocks. But that's of course something that can be fixed. And I had almost been
sold on it, if there wasn't the default register issue.

> If we also provide a more compact way of
> representing the defaults that devices with sparse registers can use
> then that problem will go away too.

What do you have in mind for this? An array of pairs of register and value?

- Lars

Re: snd soc spi read/write

[Mark Brown]

On Thu, Aug 11, 2011 at 03:50:33AM +0200, Lars-Peter Clausen wrote:
> On 08/11/2011 02:33 AM, Mark Brown wrote:

> > Like I say I'm really not happy about adding further non-standard driver
> > specifics to the Analog drivers, they're already not the best and they
> > don't really seem to be getting much attention from anyone so I'm not
> > confident anyone will come along and reverse any temporary bodges.  I'd
> > be reasonably happy with something temporary for 3.1 if we already have
> > a fix in place for 3.2 but I don't have much confidence that anyone will
> > work on that.

> So, just switch the ad193x driver to a rbcache.

That's one of the suggestions I made earlier, obviously it's painful for
the register default table at present though.

> >> And if we have to add our own read function we could as very well add our own
> >> write function which simply reinstates the old caching behavior.
> > 
> > If the driver needs its own custom I/O it should do both read and write.

> We don't need reads if we cache writes. In the old ASoC IO code there wasn't
> even SPI read support.

Yeah, that's for the volatile case where we skip having a cache at all.

> Yes, I had a look at the rbcache the other day. The current code doesn't seem
> to be optimal. For example adjacent don't seem to be joined, so for example if
> I have 3 registers and write them in the order 0,2,1 I'll still end up with 2
> blocks. But that's of course something that can be fixed. And I had almost been
> sold on it, if there wasn't the default register issue.

None of the current ASoC code will coalesce register writes at all, and
in the case where you're doing writes to registers that aren't actually
adjacent it's going to be marginal if it's better to transmit the
intervening register or transmit another register address.  That only
really makes a difference during cache sync anyway.

> > If we also provide a more compact way of
> > representing the defaults that devices with sparse registers can use
> > then that problem will go away too.

> What do you have in mind for this? An array of pairs of register and value?

Yes, as I said in one of the earlier messages in this thread.  It seems
like a good combination of being writable/legible and compact.

Re: snd soc spi read/write

[Lars-Peter Clausen]

On 08/11/2011 04:46 AM, Mark Brown wrote:
> On Thu, Aug 11, 2011 at 03:50:33AM +0200, Lars-Peter Clausen wrote:
>> On 08/11/2011 02:33 AM, Mark Brown wrote:
> [...]
> 
>> Yes, I had a look at the rbcache the other day. The current code doesn't seem
>> to be optimal. For example adjacent don't seem to be joined, so for example if
>> I have 3 registers and write them in the order 0,2,1 I'll still end up with 2
>> blocks. But that's of course something that can be fixed. And I had almost been
>> sold on it, if there wasn't the default register issue.
> 
> None of the current ASoC code will coalesce register writes at all, and
> in the case where you're doing writes to registers that aren't actually
> adjacent it's going to be marginal if it's better to transmit the
> intervening register or transmit another register address.  That only
> really makes a difference during cache sync anyway.

I was think more in terms of in memory consumption and lookup time of the cache
compared to a flat cache. If you have two blocks which have a gap of one
register between them and that register gets inserted into the cache, ideally
those two blocks would be merged, which doesn't seem to be the case currently.
So instead of one rbnode with a block covering the whole register space you'll
end up with a lot of smaller rbnodes.

> 
>>> If we also provide a more compact way of
>>> representing the defaults that devices with sparse registers can use
>>> then that problem will go away too.
> 
>> What do you have in mind for this? An array of pairs of register and value?
> 
> Yes, as I said in one of the earlier messages in this thread.  It seems
> like a good combination of being writable/legible and compact.

Hm, ok I'll give it a try. Though I'm not sure yet how to efficiently implement
the default register lookup when syncing the cache.

- Lars

Re: snd soc spi read/write

[Mark Brown]

On Thu, Aug 11, 2011 at 05:09:14AM +0200, Lars-Peter Clausen wrote:
> On 08/11/2011 04:46 AM, Mark Brown wrote:

> > None of the current ASoC code will coalesce register writes at all, and
> > in the case where you're doing writes to registers that aren't actually
> > adjacent it's going to be marginal if it's better to transmit the
> > intervening register or transmit another register address.  That only
> > really makes a difference during cache sync anyway.

> I was think more in terms of in memory consumption and lookup time of the cache
> compared to a flat cache. If you have two blocks which have a gap of one
> register between them and that register gets inserted into the cache, ideally
> those two blocks would be merged, which doesn't seem to be the case currently.
> So instead of one rbnode with a block covering the whole register space you'll
> end up with a lot of smaller rbnodes.

My guess is that it's probably not worth worrying about, especially for
performance where you mostly just need to be better than physical I/O.
For small register maps the memory overhead is similarly probably not
worth worrying about, and obviously there's also LZO.

> > Yes, as I said in one of the earlier messages in this thread.  It seems
> > like a good combination of being writable/legible and compact.

> Hm, ok I'll give it a try. Though I'm not sure yet how to efficiently implement
> the default register lookup when syncing the cache.

The caches can just unpack into their data, we need to take a copy
anyway to allow the caches to be marked as __initdata and then the data
will end up stored in a format that matches the method we're using to
store the data.

Dimitris had done an initial version of the move of the cache over,
though I didn't review it properly yet and he's on holiday now.  I might
repost it, there were a few issues but it's at least 90% of the way
there IIRC from the time I had to look at it.

Re: snd soc spi read/write

[Lars-Peter Clausen]

On 08/11/2011 07:32 AM, Mark Brown wrote:
> On Thu, Aug 11, 2011 at 05:09:14AM +0200, Lars-Peter Clausen wrote:
>> On 08/11/2011 04:46 AM, Mark Brown wrote:
> 
>>> None of the current ASoC code will coalesce register writes at all, and
>>> in the case where you're doing writes to registers that aren't actually
>>> adjacent it's going to be marginal if it's better to transmit the
>>> intervening register or transmit another register address.  That only
>>> really makes a difference during cache sync anyway.
> 
>> I was think more in terms of in memory consumption and lookup time of the cache
>> compared to a flat cache. If you have two blocks which have a gap of one
>> register between them and that register gets inserted into the cache, ideally
>> those two blocks would be merged, which doesn't seem to be the case currently.
>> So instead of one rbnode with a block covering the whole register space you'll
>> end up with a lot of smaller rbnodes.
> 
> My guess is that it's probably not worth worrying about, especially for
> performance where you mostly just need to be better than physical I/O.
> For small register maps the memory overhead is similarly probably not
> worth worrying about, and obviously there's also LZO.

I think I'll just test it with my codec and see how it turns out. If it there
are dozens of small blocks, instead of a few larger blocks, I'll see if there
is a way to easily improve the situation.

> 
>>> Yes, as I said in one of the earlier messages in this thread.  It seems
>>> like a good combination of being writable/legible and compact.
> 
>> Hm, ok I'll give it a try. Though I'm not sure yet how to efficiently implement
>> the default register lookup when syncing the cache.
> 
> The caches can just unpack into their data, we need to take a copy
> anyway to allow the caches to be marked as __initdata and then the data
> will end up stored in a format that matches the method we're using to
> store the data.
> 
> Dimitris had done an initial version of the move of the cache over,
> though I didn't review it properly yet and he's on holiday now.  I might
> repost it, there were a few issues but it's at least 90% of the way
> there IIRC from the time I had to look at it.

ok, that would be great, thanks.

- Lars

Re: snd soc spi read/write

[Dimitris Papastamos]

On Thu, Aug 11, 2011 at 02:32:53PM +0900, Mark Brown wrote:
> On Thu, Aug 11, 2011 at 05:09:14AM +0200, Lars-Peter Clausen wrote:
> > On 08/11/2011 04:46 AM, Mark Brown wrote:
> 
> > > None of the current ASoC code will coalesce register writes at all, and
> > > in the case where you're doing writes to registers that aren't actually
> > > adjacent it's going to be marginal if it's better to transmit the
> > > intervening register or transmit another register address.  That only
> > > really makes a difference during cache sync anyway.
> 
> > I was think more in terms of in memory consumption and lookup time of the cache
> > compared to a flat cache. If you have two blocks which have a gap of one
> > register between them and that register gets inserted into the cache, ideally
> > those two blocks would be merged, which doesn't seem to be the case currently.
> > So instead of one rbnode with a block covering the whole register space you'll
> > end up with a lot of smaller rbnodes.

Yes, that's true.  I've got that in my TODO somewhere.  It was not
important enough during initial implementation.

> Dimitris had done an initial version of the move of the cache over,
> though I didn't review it properly yet and he's on holiday now.  I might
> repost it, there were a few issues but it's at least 90% of the way
> there IIRC from the time I had to look at it.

I'll be looking into this soon, there are a few issues to be resolved
regarding the LZO code at the moment.

Thanks,
Dimitris

Re: snd soc spi read/write

[Takashi Iwai]

At Fri, 5 Aug 2011 10:24:43 +0800,
Scott Jiang wrote:
> 
> 2011/8/4 Mark Brown <broonie@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.

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.

> > 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.


thanks,

Takashi

Re: 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@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

Re: snd soc spi read/write

[Mark Brown]

On Fri, Aug 05, 2011 at 08:11:34AM +0200, Takashi Iwai wrote:

> 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.

Oh, gods.  If that's what the code is supposed to be doing then clearly
these aren't 16x8 registers at all and it's another case like the step
size stuff where someone's done something cute but fragile and in this
case they didn't even bother to put in a comment so it just looks like
an obvious bug.  We do actually have at least one device with genuine
16x8 registers but fortunately the driver for that never tried to cache
and it's I2C only anyway.

> A quick fix would to introduce again the 0xff masking.  Of course,

I'm more inclined to say that we just don't cache for devices trying to
do this until we've got a sensible model for them, there's probably some
other code that's going to be confused by this in there.

> this isn't a right thing, but would be enough for the current code,
> as regmap will replace in future anyway.

At the minute regmap has no concept of such devices, but at the minute
it's I/O only and so it's not such an issue.  My first instinct is just
to treat these devices as having a sparse register map so probably an
rbtree cache would do the right thing.