[PATCH 0/2] ASoC: Override codec compress_type from the machine driver

[PATCH 0/2] ASoC: Override codec compress_type from the machine driver

[Dimitris Papastamos]

This patch adds support for overriding the compress_type of the codec driver
from the machine driver.  This patch depends on the previous series of patches
that I've submitted with Subject "ASoC: Laying the groundwork for
compress_type overriding".

Dimitris Papastamos (2):
  ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default
  ASoC: soc-core: Allow machine drivers to override compress_type

 include/sound/soc.h   |    2 +
 sound/soc/soc-cache.c |   36 +++++++++++++-------
 sound/soc/soc-core.c  |   87 +++++++++++++++++++++++++++++++++++++++++++-----
 3 files changed, 104 insertions(+), 21 deletions(-)

-- 
1.7.3.2

[PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Dimitris Papastamos]

Make sure to use codec->reg_def_copy instead of codec_drv->reg_cache_default
wherever necessary.  This change is necessary because in the next patch we
move the cache initialization code outside snd_soc_register_codec() and by that
time any data marked as __devinitconst such as the original reg_cache_default
array might have already been freed by the kernel.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 include/sound/soc.h   |    1 +
 sound/soc/soc-cache.c |   36 ++++++++++++++++++++++++------------
 sound/soc/soc-core.c  |   17 +++++++++++++++++
 3 files changed, 42 insertions(+), 12 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 4409e97..4cdba68 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -478,6 +478,7 @@ struct snd_soc_codec {
 	hw_write_t hw_write;
 	unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int);
 	void *reg_cache;
+	const void *reg_def_copy;
 	const struct snd_soc_cache_ops *cache_ops;
 	struct mutex cache_rw_mutex;
 
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index 081221d..f8e285d 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -933,7 +933,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec)
 	rbtree_ctx = codec->reg_cache;
 	rbtree_ctx->root = RB_ROOT;
 
-	if (!codec->driver->reg_cache_default)
+	if (!codec->reg_def_copy)
 		return 0;
 
 /*
@@ -951,7 +951,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec)
 	struct snd_soc_rbtree_node *rbtree_node;			\
 									\
 	ret = 0;							\
-	cache = codec->driver->reg_cache_default;			\
+	cache = codec->reg_def_copy;					\
 	for (i = 0; i < codec->driver->reg_cache_size; ++i) {		\
 		if (!cache[i])						\
 			continue;					\
@@ -1316,13 +1316,13 @@ static int snd_soc_lzo_cache_init(struct snd_soc_codec *codec)
 	 * and remember to free it afterwards.
 	 */
 	tofree = 0;
-	if (!codec_drv->reg_cache_default)
+	if (!codec->reg_def_copy)
 		tofree = 1;
 
-	if (!codec_drv->reg_cache_default) {
-		codec_drv->reg_cache_default = kzalloc(reg_size,
+	if (!codec->reg_def_copy) {
+		codec->reg_def_copy = kzalloc(reg_size,
 						       GFP_KERNEL);
-		if (!codec_drv->reg_cache_default)
+		if (!codec->reg_def_copy)
 			return -ENOMEM;
 	}
 
@@ -1368,8 +1368,8 @@ static int snd_soc_lzo_cache_init(struct snd_soc_codec *codec)
 	}
 
 	blksize = snd_soc_lzo_get_blksize(codec);
-	p = codec_drv->reg_cache_default;
-	end = codec_drv->reg_cache_default + reg_size;
+	p = codec->reg_def_copy;
+	end = codec->reg_def_copy + reg_size;
 	/* compress the register map and fill the lzo blocks */
 	for (i = 0; i < blkcount; ++i, p += blksize) {
 		lzo_blocks[i]->src = p;
@@ -1385,14 +1385,18 @@ static int snd_soc_lzo_cache_init(struct snd_soc_codec *codec)
 			lzo_blocks[i]->src_len;
 	}
 
-	if (tofree)
-		kfree(codec_drv->reg_cache_default);
+	if (tofree) {
+		kfree(codec->reg_def_copy);
+		codec->reg_def_copy = NULL;
+	}
 	return 0;
 err:
 	snd_soc_cache_exit(codec);
 err_tofree:
-	if (tofree)
-		kfree(codec_drv->reg_cache_default);
+	if (tofree) {
+		kfree(codec->reg_def_copy);
+		codec->reg_def_copy = NULL;
+	}
 	return ret;
 }
 
@@ -1506,6 +1510,14 @@ static int snd_soc_flat_cache_init(struct snd_soc_codec *codec)
 	codec_drv = codec->driver;
 	reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
 
+	/*
+	 * for flat compression, we don't need to keep a copy of the
+	 * original defaults register cache as it will definitely not
+	 * be marked as __devinitconst
+	 */
+	kfree(codec->reg_def_copy);
+	codec->reg_def_copy = NULL;
+
 	if (codec_drv->reg_cache_default)
 		codec->reg_cache = kmemdup(codec_drv->reg_cache_default,
 					   reg_size, GFP_KERNEL);
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a6565eb..9d9364a 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3430,6 +3430,7 @@ int snd_soc_register_codec(struct device *dev,
 		struct snd_soc_codec_driver *codec_drv,
 		struct snd_soc_dai_driver *dai_drv, int num_dai)
 {
+	size_t reg_size;
 	struct snd_soc_codec *codec;
 	int ret, i;
 
@@ -3463,6 +3464,19 @@ int snd_soc_register_codec(struct device *dev,
 
 	/* allocate CODEC register cache */
 	if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
+		reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+		/* it is necessary to make a copy of the default register cache
+		 * because in the case of using a compression type that requires
+		 * the default register cache to be marked as __devinitconst the
+		 * kernel might have freed the array by the time we initialize
+		 * the cache.
+		 */
+		codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
+					      reg_size, GFP_KERNEL);
+		if (!codec->reg_def_copy) {
+			ret = -ENOMEM;
+			goto error_cache;
+		}
 		ret = snd_soc_cache_init(codec);
 		if (ret < 0) {
 			dev_err(codec->dev, "Failed to set cache compression type: %d\n",
@@ -3494,6 +3508,8 @@ int snd_soc_register_codec(struct device *dev,
 error_dais:
 	snd_soc_cache_exit(codec);
 error_cache:
+	kfree(codec->reg_def_copy);
+	codec->reg_def_copy = NULL;
 	kfree(codec->name);
 	kfree(codec);
 	return ret;
@@ -3528,6 +3544,7 @@ found:
 	pr_debug("Unregistered codec '%s'\n", codec->name);
 
 	snd_soc_cache_exit(codec);
+	kfree(codec->reg_def_copy);
 	kfree(codec->name);
 	kfree(codec);
 }
-- 
1.7.3.2

[PATCH 2/2] ASoC: soc-core: Allow machine drivers to override compress_type

[Dimitris Papastamos]

This patch allows machine drivers to override the compression type
provided by the codec driver.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 include/sound/soc.h  |    1 +
 sound/soc/soc-core.c |   74 ++++++++++++++++++++++++++++++++++++++++++-------
 2 files changed, 64 insertions(+), 11 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 4cdba68..e5b9bd7 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -472,6 +472,7 @@ struct snd_soc_codec {
 	unsigned int ac97_registered:1; /* Codec has been AC97 registered */
 	unsigned int ac97_created:1; /* Codec has been created by SoC */
 	unsigned int sysfs_registered:1; /* codec has been sysfs registered */
+	unsigned int cache_init:1; /* codec cache has been initialized */
 
 	/* codec IO */
 	void *control_data; /* codec control (i2c/3wire) data */
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 9d9364a..a9827f1 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1726,9 +1726,36 @@ static void soc_remove_aux_dev(struct snd_soc_card *card, int num)
 	}
 }
 
+static int snd_soc_init_codec_cache(struct snd_soc_codec *codec,
+				    enum snd_soc_compress_type compress_type)
+{
+	int ret;
+
+	if (codec->cache_init)
+		return 0;
+
+	/* override the compress_type if necessary */
+	if (compress_type && codec->compress_type != compress_type)
+		codec->compress_type = compress_type;
+	dev_dbg(codec->dev, "Cache compress_type for %s is %d\n",
+		codec->name, codec->compress_type);
+	ret = snd_soc_cache_init(codec);
+	if (ret < 0) {
+		dev_err(codec->dev, "Failed to set cache compression type: %d\n",
+			ret);
+		return ret;
+	}
+	codec->cache_init = 1;
+	return 0;
+}
+
+
 static void snd_soc_instantiate_card(struct snd_soc_card *card)
 {
 	struct platform_device *pdev = to_platform_device(card->dev);
+	struct snd_soc_codec *codec;
+	struct snd_soc_codec_conf *codec_conf;
+	enum snd_soc_compress_type compress_type;
 	int ret, i;
 
 	mutex_lock(&card->mutex);
@@ -1748,6 +1775,39 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 		return;
 	}
 
+	/* initialize the register cache for each available codec */
+	list_for_each_entry(codec, &codec_list, list) {
+		if (codec->cache_init)
+			continue;
+		/* check to see if we need to override the compress_type */
+		for (i = 0; i < card->num_configs; ++i) {
+			codec_conf = &card->codec_conf[i];
+			if (!strcmp(codec->name, codec_conf->dev_name)) {
+				compress_type = codec_conf->compress_type;
+				if (compress_type && compress_type
+				    != codec->compress_type)
+					break;
+			}
+		}
+		if (i == card->num_configs) {
+			/* no need to override the compress_type so
+			 * go ahead and do the standard thing */
+			ret = snd_soc_init_codec_cache(codec, 0);
+			if (ret < 0) {
+				mutex_unlock(&card->mutex);
+				return;
+			}
+			continue;
+		}
+		/* override the compress_type with the one supplied in
+		 * the machine driver */
+		ret = snd_soc_init_codec_cache(codec, compress_type);
+		if (ret < 0) {
+			mutex_unlock(&card->mutex);
+			return;
+		}
+	}
+
 	/* card bind complete so register a sound card */
 	ret = snd_card_create(SNDRV_DEFAULT_IDX1, SNDRV_DEFAULT_STR1,
 			card->owner, 0, &card->snd_card);
@@ -3475,13 +3535,7 @@ int snd_soc_register_codec(struct device *dev,
 					      reg_size, GFP_KERNEL);
 		if (!codec->reg_def_copy) {
 			ret = -ENOMEM;
-			goto error_cache;
-		}
-		ret = snd_soc_cache_init(codec);
-		if (ret < 0) {
-			dev_err(codec->dev, "Failed to set cache compression type: %d\n",
-				ret);
-			goto error_cache;
+			goto fail;
 		}
 	}
 
@@ -3494,7 +3548,7 @@ int snd_soc_register_codec(struct device *dev,
 	if (num_dai) {
 		ret = snd_soc_register_dais(dev, dai_drv, num_dai);
 		if (ret < 0)
-			goto error_dais;
+			goto fail;
 	}
 
 	mutex_lock(&client_mutex);
@@ -3505,9 +3559,7 @@ int snd_soc_register_codec(struct device *dev,
 	pr_debug("Registered codec '%s'\n", codec->name);
 	return 0;
 
-error_dais:
-	snd_soc_cache_exit(codec);
-error_cache:
+fail:
 	kfree(codec->reg_def_copy);
 	codec->reg_def_copy = NULL;
 	kfree(codec->name);
-- 
1.7.3.2

Re: [PATCH 0/2] ASoC: Override codec compress_type from the machine driver

[Liam Girdwood]

On Thu, 2010-12-02 at 16:11 +0000, Dimitris Papastamos wrote:
> This patch adds support for overriding the compress_type of the codec driver
> from the machine driver.  This patch depends on the previous series of patches
> that I've submitted with Subject "ASoC: Laying the groundwork for
> compress_type overriding".
> 
> Dimitris Papastamos (2):
>   ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default
>   ASoC: soc-core: Allow machine drivers to override compress_type
> 
>  include/sound/soc.h   |    2 +
>  sound/soc/soc-cache.c |   36 +++++++++++++-------
>  sound/soc/soc-core.c  |   87 +++++++++++++++++++++++++++++++++++++++++++-----
>  3 files changed, 104 insertions(+), 21 deletions(-)
> 

All

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH 0/2] ASoC: Override codec compress_type from the machine driver

[Mark Brown]

On Thu, Dec 02, 2010 at 04:11:04PM +0000, Dimitris Papastamos wrote:
> This patch adds support for overriding the compress_type of the codec driver
> from the machine driver.  This patch depends on the previous series of patches
> that I've submitted with Subject "ASoC: Laying the groundwork for
> compress_type overriding".

Applied, thanks.

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Timur Tabi]

On Thu, Dec 2, 2010 at 10:11 AM, Dimitris Papastamos
<dp@opensource.wolfsonmicro.com> wrote:
> Make sure to use codec->reg_def_copy instead of codec_drv->reg_cache_default
> wherever necessary.  This change is necessary because in the next patch we
> move the cache initialization code outside snd_soc_register_codec() and by that
> time any data marked as __devinitconst such as the original reg_cache_default
> array might have already been freed by the kernel.

This commit breaks cs4270.c:

Cirrus Logic CS4270 ALSA SoC Codec Driver
cs4270-codec 0-004f: found device at i2c address 4F
cs4270-codec 0-004f: hardware revision 3
Unable to handle kernel paging request for data at address 0x00000000
Faulting instruction address: 0xc001646c
Oops: Kernel access of bad area, sig: 11 [#1]
MPC86xx HPCD
last sysfs file:
Modules linked in:
NIP: c001646c LR: c006cef8 CTR: 00000001
REGS: ef03dd20 TRAP: 0300   Not tainted  (2.6.37-rc3-02080-g8e7bd55)
MSR: 00009032 <EE,ME,IR,DR>  CR: 22044028  XER: 20000000
DAR: 00000000, DSISR: 40000000
TASK = ef038000[1] 'swapper' THREAD: ef03c000
GPR00: 00000000 ef03ddd0 ef038000 ef2bb9e0 fffffffc 00000008 ef2bb9dc 00000001
GPR08: 2e302d30 00000000 fffffffe 00000006 22044082 100a38d4 00000000 3ffe7388
GPR16: 00000000 3f9a6378 00000000 3ffe6210 3ffe6210 3ffe6210 00000000 00000000
GPR24: c03c0560 ef03de20 c043c968 ef03de00 ef3da220 00000000 ef2bb9e0 00000008
NIP [c001646c] memcpy+0x1c/0x9c
LR [c006cef8] kmemdup+0x3c/0x54
Call Trace:
[ef03ddd0] [c006cee4] kmemdup+0x28/0x54 (unreliable)
[ef03ddf0] [c0250664] snd_soc_register_codec+0x278/0x390
[ef03de70] [c0256514] cs4270_i2c_probe+0xb0/0x178
[ef03de90] [c021c8a8] i2c_device_probe+0xec/0x114
[ef03deb0] [c01af400] driver_probe_device+0xa0/0x1a8
[ef03ded0] [c01af5c4] __driver_attach+0xbc/0xc0
[ef03def0] [c01aea90] bus_for_each_dev+0x70/0xac
[ef03df20] [c01af20c] driver_attach+0x24/0x34
[ef03df30] [c01ae314] bus_add_driver+0x1b8/0x298
[ef03df60] [c01af928] driver_register+0x88/0x158
[ef03df80] [c021cd38] i2c_register_driver+0x4c/0xa4
[ef03dfa0] [c040fffc] cs4270_init+0x2c/0x3c
[ef03dfb0] [c0003f6c] do_one_initcall+0x15c/0x1c4
[ef03dfe0] [c03f31e0] kernel_init+0xc0/0x164
[ef03dff0] [c001086c] kernel_thread+0x4c/0x68
Instruction dump:
38c60001 4200fff0 4e800020 7c032040 418100a0 54a7e8ff 38c3fffc 3884fffc
41820028 70c00003 7ce903a6 40820054 <80e40004> 85040008 90e60004 95060008
---[ end trace 40d01772bfab46f2 ]---
Kernel panic - not syncing: Attempted to kill init!

Which is this line:

> @@ -3463,6 +3464,19 @@ int snd_soc_register_codec(struct device *dev,
>
>        /* allocate CODEC register cache */
>        if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
> +               reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
> +               /* it is necessary to make a copy of the default register cache
> +                * because in the case of using a compression type that requires
> +                * the default register cache to be marked as __devinitconst the
> +                * kernel might have freed the array by the time we initialize
> +                * the cache.
> +                */
> +               codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
> +                                             reg_size, GFP_KERNEL);

Unlike all the other codec drivers, the CS4270 driver does not
initialize codec_drv->reg_cache_default, so the pointer is NULL during
the kmemdup() call.

What is the criteria for defining reg_cache_default?  Many drivers
don't define it:

$ grep -c reg_cache_default *.c | grep \:0
88pm860x-codec.c:0
ac97.c:0
ad1836.c:0
ad73311.c:0
ads117x.c:0
ak4104.c:0
alc5623.c:0
cq93vc.c:0
cs4270.c:0
cs42l51.c:0
cx20442.c:0
l3.c:0
max9877.c:0
pcm3008.c:0
spdif_transciever.c:0
tlv320aic26.c:0
tpa6130a2.c:0
wl1273.c:0
wm2000.c:0
wm8350.c:0
wm8400.c:0
wm8727.c:0
wm8994-tables.c:0
wm_hubs.c:0

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Mark Brown]

On Wed, Jan 05, 2011 at 03:04:31PM -0600, Timur Tabi wrote:

> Unlike all the other codec drivers, the CS4270 driver does not
> initialize codec_drv->reg_cache_default, so the pointer is NULL during
> the kmemdup() call.

> What is the criteria for defining reg_cache_default?  Many drivers
> don't define it:

In general the expectation is that unless it's got a good reason not to
a well written CODEC driver will use all the standard register cache
infrastructure, including providing a set of defaults.  Good reasons for
this include things like not having any registers and indeterminate
default hardware state.

As you will doubtless have seen when you looked at the code every other
reference to reg_cache_default checks to see if it's set before using
it.  This does rather suggest that the intention of the code is that it
be optional.

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Timur Tabi]

Mark Brown wrote:
> In general the expectation is that unless it's got a good reason not to
> a well written CODEC driver will use all the standard register cache
> infrastructure, including providing a set of defaults.  Good reasons for
> this include things like not having any registers and indeterminate
> default hardware state.

My concern is that I think it's unwise to hard-code the default values of the
registers in the source file.  Who's to say that a newer version of the chip
won't have different power-on defaults?

I do want to support a register cache, but I don't want to hard-code the default
values into cs4270.c.  Is this supported?

> As you will doubtless have seen when you looked at the code every other
> reference to reg_cache_default checks to see if it's set before using
> it.  This does rather suggest that the intention of the code is that it
> be optional.

So are you saying that there's a bug in this patch?  Perhaps that code should
look like this:

if (codec_drv->reg_cache_default)
	codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default, reg_size, GFP_KERNEL);
else {
	codec->reg_def_copy = kmalloc(reg_size, GFP_KERNEL);
	// here we somehow tell the codec driver to initialize reg_def_copy
}

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Mark Brown]

On Wed, Jan 05, 2011 at 05:08:20PM -0600, Timur Tabi wrote:

> My concern is that I think it's unwise to hard-code the default values of the
> registers in the source file.  Who's to say that a newer version of the chip
> won't have different power-on defaults?

In practical terms this is vanishingly unlikely; 

> I do want to support a register cache, but I don't want to hard-code the default
> values into cs4270.c.  Is this supported?

If something you'd like to do doesn't seem to be working or is otherwise
not there and you can see a way to implement it sensibly then the most
obvious thing would seem to be to implement it.

Please remember that this isn't a frozen commercial OS release from a
third party, you can contribute to the core code if you see a need to.
None of this is fixed in stone and a lot of it comes from looking at the
code and taking opportunities to add appropriate abstractions - look at
symmetric_rates, or the creation of soc-cache for example.

> > As you will doubtless have seen when you looked at the code every other
> > reference to reg_cache_default checks to see if it's set before using
> > it.  This does rather suggest that the intention of the code is that it
> > be optional.

> So are you saying that there's a bug in this patch?  Perhaps that code should
> look like this:

Perhaps it should, though my immediate question (without looking at the
existing code again) is why the existing tests are being removed.

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Mark Brown]

On Wed, Jan 05, 2011 at 11:29:47PM +0000, Mark Brown wrote:

> > So are you saying that there's a bug in this patch?  Perhaps that code should
> > look like this:

> Perhaps it should, though my immediate question (without looking at the
> existing code again) is why the existing tests are being removed.

Having looked slightly further it does look like something I'd expect to
work.

More generally though what I'm trying to say is more about the approach
that's being taken here; with things like this it's generally much
better to at least have a dig into what's not working for you and have
an idea of what's going on rather than just stopping and asking.  This
is normally easier all round, either you'll be able to sort stuff out
yourself or you'll be able to ask a much better question.

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Tabi Timur-B04825]

Mark Brown wrote:
> More generally though what I'm trying to say is more about the approach
> that's being taken here; with things like this it's generally much
> better to at least have a dig into what's not working for you and have
> an idea of what's going on rather than just stopping and asking.  This
> is normally easier all round, either you'll be able to sort stuff out
> yourself or you'll be able to ask a much better question.

Mark, you don't need to explain the philosophy of open source development to me. :-)

I just wanted to get the idea of what you and others intended with the current register cache code, just as a matter of context.  I like to understand these things *before* I start digging around.

-- 
Timur Tabi
Linux kernel developer

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Mark Brown]

On Thu, Jan 06, 2011 at 12:15:17AM +0000, Tabi Timur-B04825 wrote:

> I just wanted to get the idea of what you and others intended with the
> current register cache code, just as a matter of context.  I like to

So ask that question; "I looked at this and I'm not really sure if it's
supposed to work or not - thing X suggests yes, thing Y suggests no but
looks like a bug so..." and so on.

In cases like this (although not I suspect this particular one) the
answer is often that there was't any particular consideration for
whatever unusual case you're looking at.

> understand these things *before* I start digging around.

The important thing is to show that you're looking, especially in areas
where you've done something unusual in your driver.  "Looking at this
briefly..." isn't a problem, just make it clear that that's what you've
done.

Part of the reason I'm emphasising this is that stuff like this seems to
cause you to get stuch relatively often.

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Timur Tabi]

Mark Brown wrote:
> So ask that question; "I looked at this and I'm not really sure if it's
> supposed to work or not - thing X suggests yes, thing Y suggests no but
> looks like a bug so..." and so on.
> 
> In cases like this (although not I suspect this particular one) the
> answer is often that there was't any particular consideration for
> whatever unusual case you're looking at.

Ah, but I don't think my case is "unusual".  Frankly, I think it's unusual to
hard-code default register values into a driver.  I just don't see how you can
guarantee that the values will be correct for all supported revisions of the chip.

On the CS4270, for instance, one register contains the chip revision number.
There's no way I can know which revision will be used on any given board.

Anyway, I'll work on fixing this and post a patch.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Dimitris Papastamos]

On Wed, 2011-01-05 at 15:04 -0600, Timur Tabi wrote:
> Which is this line:
> 
> > @@ -3463,6 +3464,19 @@ int snd_soc_register_codec(struct device *dev,
> >
> >        /* allocate CODEC register cache */
> >        if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
> > +               reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
> > +               /* it is necessary to make a copy of the default register cache
> > +                * because in the case of using a compression type that requires
> > +                * the default register cache to be marked as __devinitconst the
> > +                * kernel might have freed the array by the time we initialize
> > +                * the cache.
> > +                */
> > +               codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default,
> > +                                             reg_size, GFP_KERNEL);

The semantics behind this code is that if the driver provides a
reg_cache_size and a reg_word_size it *should* provide a defaults
register cache as well.  If you want to manage your own register cache
in the driver which is not advised, you will have to add similar
functionality in your _priv struct.  If you require more flexible
functionality you need to consider implementing a sensible strategy and
submitting it as a patch.

Thanks,
Dimitris

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Timur Tabi]

Dimitris Papastamos wrote:
> The semantics behind this code is that if the driver provides a
> reg_cache_size and a reg_word_size it *should* provide a defaults
> register cache as well.

I *had* code which did this, but apparently it broke somewhere.  Oh well.

>  If you want to manage your own register cache
> in the driver which is not advised, you will have to add similar
> functionality in your _priv struct.  If you require more flexible
> functionality you need to consider implementing a sensible strategy and
> submitting it as a patch.

I will fix my driver to restore the functionality.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ASoC: soc-cache: Use reg_def_copy instead of reg_cache_default

[Mark Brown]

On Thu, Jan 06, 2011 at 10:26:43AM -0600, Timur Tabi wrote:
> Mark Brown wrote:

> > In cases like this (although not I suspect this particular one) the
> > answer is often that there was't any particular consideration for
> > whatever unusual case you're looking at.

> Ah, but I don't think my case is "unusual".  Frankly, I think it's unusual to
> hard-code default register values into a driver.  I just don't see how you can

*sigh*  If you look at other ASoC drivers you'll see that providing
register defaults is very much idiomatic.

> guarantee that the values will be correct for all supported revisions of the chip.

We've been doing this for rather a long time; many devices don't support
readback at all so there's not even any option for them.  There's some
fairly simple things you can do if there are issues, like write out the
new default values explicitly (which will be a noop on new silicon).

There's fairly strong pressures on chip vendors to avoid anything that
causes issues here.  Consider what a chip vendor has to do to introduce
an incompatible register change in production hardware: they need to
notify all their customers and they need to make sure that their
customers are happy and don't get upset about having to update their
software (perhaps they don't like the new default).  If the customers
aren't happy then worst case you end up having to keep both old and new
silicon in production.

> On the CS4270, for instance, one register contains the chip revision number.
> There's no way I can know which revision will be used on any given board.

So that register should be marked as volatile, the register default
value will be ignored and the cache will never be used for that register.