[PATCH] ASoC: factor out intel_scu_ipc related read/write

[PATCH] ASoC: factor out intel_scu_ipc related read/write

[Lu Guanqun]

A new 'enum snd_soc_control_type' is added to indicate this operation.

Signed-off-by: Lu Guanqun <guanqun.lu@intel.com>
---
 include/sound/soc.h        |    1 +
 sound/soc/codecs/sn95031.c |   33 ++++++--------------------------
 sound/soc/soc-cache.c      |   46 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 27 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 6ce3e57..802b9a7 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -251,6 +251,7 @@ enum snd_soc_control_type {
 	SND_SOC_CUSTOM = 1,
 	SND_SOC_I2C,
 	SND_SOC_SPI,
+	SND_SOC_INTEL_SCU,
 };
 
 enum snd_soc_compress_type {
diff --git a/sound/soc/codecs/sn95031.c b/sound/soc/codecs/sn95031.c
index 84ffdeb..a11e324 100644
--- a/sound/soc/codecs/sn95031.c
+++ b/sound/soc/codecs/sn95031.c
@@ -29,7 +29,6 @@
 #include <linux/delay.h>
 #include <linux/slab.h>
 
-#include <asm/intel_scu_ipc.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
@@ -166,30 +165,6 @@ static unsigned int sn95031_get_mic_bias(struct snd_soc_codec *codec)
 EXPORT_SYMBOL_GPL(sn95031_get_mic_bias);
 /*end - adc helper functions */
 
-static inline unsigned int sn95031_read(struct snd_soc_codec *codec,
-			unsigned int reg)
-{
-	u8 value = 0;
-	int ret;
-
-	ret = intel_scu_ipc_ioread8(reg, &value);
-	if (ret)
-		pr_err("read of %x failed, err %d\n", reg, ret);
-	return value;
-
-}
-
-static inline int sn95031_write(struct snd_soc_codec *codec,
-			unsigned int reg, unsigned int value)
-{
-	int ret;
-
-	ret = intel_scu_ipc_iowrite8(reg, value);
-	if (ret)
-		pr_err("write of %x failed, err %d\n", reg, ret);
-	return ret;
-}
-
 static int sn95031_set_vaud_bias(struct snd_soc_codec *codec,
 		enum snd_soc_bias_level level)
 {
@@ -827,8 +802,14 @@ EXPORT_SYMBOL_GPL(sn95031_jack_detection);
 /* codec registration */
 static int sn95031_codec_probe(struct snd_soc_codec *codec)
 {
+	int ret;
+
 	pr_debug("codec_probe called\n");
 
+	ret = snd_soc_codec_set_cache_io(codec, 16, 8, SND_SOC_INTEL_SCU);
+	if (ret < 0)
+		return ret;
+
 	codec->dapm.bias_level = SND_SOC_BIAS_OFF;
 	codec->dapm.idle_bias_off = 1;
 
@@ -891,8 +872,6 @@ static int sn95031_codec_remove(struct snd_soc_codec *codec)
 struct snd_soc_codec_driver sn95031_codec = {
 	.probe		= sn95031_codec_probe,
 	.remove		= sn95031_codec_remove,
-	.read		= sn95031_read,
-	.write		= sn95031_write,
 	.set_bias_level	= sn95031_set_vaud_bias,
 	.dapm_widgets	= sn95031_dapm_widgets,
 	.num_dapm_widgets	= ARRAY_SIZE(sn95031_dapm_widgets),
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index a217db2..65986a9 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -20,6 +20,42 @@
 
 #include <trace/events/asoc.h>
 
+#if defined(CONFIG_INTEL_SCU_IPC)
+#include <asm/intel_scu_ipc.h>
+static unsigned int snd_soc_16_8_intel_scu_read(struct snd_soc_codec *codec,
+						unsigned int reg)
+{
+	u8 value = 0;
+	int ret;
+
+	ret = intel_scu_ipc_ioread8(reg, &value);
+	if (ret < 0)
+		dev_err(codec->dev, "read of %x failed, err %d\n", reg, ret);
+	return value;
+}
+
+static int snd_soc_16_8_intel_scu_write(void *control_data,
+					const char *data, int len)
+{
+	struct snd_soc_codec *codec = control_data;
+	unsigned int reg, value;
+	int ret;
+
+	reg = (data[0] << 8) | data[1];
+	value = data[2];
+
+	ret = intel_scu_ipc_iowrite8(reg, value);
+	if (ret < 0) {
+		dev_err(codec->dev, "write of %x failed, err %d\n", reg, ret);
+		return ret;
+	}
+	return len;
+}
+#else
+#define snd_soc_16_8_intel_scu_read NULL
+#define snd_soc_16_8_intel_scu_write NULL
+#endif
+
 #if defined(CONFIG_SPI_MASTER)
 static int do_spi_write(void *control_data, const void *msg,
 			int len)
@@ -544,6 +580,16 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 						   struct spi_device,
 						   dev);
 		break;
+
+	case SND_SOC_INTEL_SCU:
+		if (io_types[i].addr_bits != 16 || io_types[i].data_bits != 8)
+			return -EINVAL;
+
+		codec->hw_read = snd_soc_16_8_intel_scu_read;
+		codec->hw_write = snd_soc_16_8_intel_scu_write;
+		codec->control_data = codec;
+
+		break;
 	}
 
 	return 0;

Re: [PATCH] ASoC: factor out intel_scu_ipc related read/write

[Lu Guanqun]

Hi Mark,

How do you think of this approach?

Hi Vinod,

I don't have the mfld hardware, so it's a not tested patch. I want your
ack or dis-ack on this. :)

> +	case SND_SOC_INTEL_SCU:
> +		if (io_types[i].addr_bits != 16 || io_types[i].data_bits != 8)
> +			return -EINVAL;
> +
> +		codec->hw_read = snd_soc_16_8_intel_scu_read;
> +		codec->hw_write = snd_soc_16_8_intel_scu_write;
> +		codec->control_data = codec;

Another way to do this to simply override codec->read and codec->write
with the intel_scu_read/write stuff.  But then I realize we may later
take advantage of soc cache, therefore it's not appropriate to simply
override these two operations.

-- 
guanqun

Re: [PATCH] ASoC: factor out intel_scu_ipc related read/write

[Koul, Vinod]

On Sat, 2011-05-07 at 19:18 +0530, Lu, Guanqun wrote:
> Hi Mark,
> 
> How do you think of this approach?
> 
> Hi Vinod,
> 
> I don't have the mfld hardware, so it's a not tested patch. I want your
> ack or dis-ack on this. :)
> 
> > +	case SND_SOC_INTEL_SCU:
> > +		if (io_types[i].addr_bits != 16 || io_types[i].data_bits != 8)
> > +			return -EINVAL;
> > +
> > +		codec->hw_read = snd_soc_16_8_intel_scu_read;
> > +		codec->hw_write = snd_soc_16_8_intel_scu_write;
> > +		codec->control_data = codec;
> 
> Another way to do this to simply override codec->read and codec->write
> with the intel_scu_read/write stuff.  But then I realize we may later
> take advantage of soc cache, therefore it's not appropriate to simply
> override these two operations.
Let me test it out...

Meanwhile, i am not sure if this is a good idea.
We can try enabling cache but will it help? Have you tried that on mrst?

The reason for my paranoia is the SCU API, in past it had issues when we
do block writes it, something which syncing the cache can cause.
+ Alan for his comments...


-- 
~Vinod

Re: [PATCH] ASoC: factor out intel_scu_ipc related read/write

[Lu Guanqun]

On Mon, May 09, 2011 at 11:14:23AM +0800, Koul, Vinod wrote:
> On Sat, 2011-05-07 at 19:18 +0530, Lu, Guanqun wrote:
> > Hi Mark,
> > 
> > How do you think of this approach?
> > 
> > Hi Vinod,
> > 
> > I don't have the mfld hardware, so it's a not tested patch. I want your
> > ack or dis-ack on this. :)
> > 
> > > +	case SND_SOC_INTEL_SCU:
> > > +		if (io_types[i].addr_bits != 16 || io_types[i].data_bits != 8)
> > > +			return -EINVAL;
> > > +
> > > +		codec->hw_read = snd_soc_16_8_intel_scu_read;
> > > +		codec->hw_write = snd_soc_16_8_intel_scu_write;
> > > +		codec->control_data = codec;
> > 
> > Another way to do this to simply override codec->read and codec->write
> > with the intel_scu_read/write stuff.  But then I realize we may later
> > take advantage of soc cache, therefore it's not appropriate to simply
> > override these two operations.
> Let me test it out...
> 
> Meanwhile, i am not sure if this is a good idea.
> We can try enabling cache but will it help? Have you tried that on mrst?

As the first step, we can bypass the soc cache...
I haven't tried it on mrst yet.

> 
> The reason for my paranoia is the SCU API, in past it had issues when we
> do block writes it, something which syncing the cache can cause.
> + Alan for his comments...
> 
> 
> -- 
> ~Vinod
> 

-- 
guanqun

Re: [PATCH] ASoC: factor out intel_scu_ipc related read/write

[Mark Brown]

On Mon, May 09, 2011 at 08:44:23AM +0530, Koul, Vinod wrote:

> Meanwhile, i am not sure if this is a good idea.
> We can try enabling cache but will it help? Have you tried that on mrst?

The I/O does not force use of a cache, unless you configure a cache size
nothing will be cached.

> The reason for my paranoia is the SCU API, in past it had issues when we
> do block writes it, something which syncing the cache can cause.
> + Alan for his comments...

The cache I/O won't make any difference here.  Unless you implement bulk
operations it's not going to magically work out how to do them - you'll
just see repeated single register operations the same as you see if you
implement this directly in your driver.  All that will happen is that
the code will be shared between all drivers using the SCU.