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?

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.

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

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.