Skip to contentSkip to conversation

ASoC: factor out intel_scu_ipc related read/write

PATCH In review Lu Guanqun 3 participants 5 messages May 7, 2011 – May 9, 2011

Subsystems (2)
Sound ASoC
lore
Share & tools
lore

Lu Guanqun

author
#0 lore raw

PATCH 1/1 ASoC: factor out intel_scu_ipc related read/write

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

Signed-off-by Lu Guanqun <[email protected]>
---
 include/sound/soc.h        |    1 +
 sound/soc/codecs/sn95031.c |   33 ++++++--------------------------
 sound/soc/soc-cache.c      |   46 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 53 insertions(+), 27 deletions(-)
include/sound/soc.h +1 -0
index 6ce3e57..802b9a7 100644
@@ -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 {
sound/soc/codecs/sn95031.c +6 -27
index 84ffdeb..a11e324 100644
@@ -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),
sound/soc/soc-cache.c +46 -0

58 lines not on this page · raw message

author
#1 lore raw

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

sound/soc/soc-cache.c int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, · line 590

Quoted 7 lines
sound/soc/soc-cache.c +7 -0
+	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.

Signature · 1 line
guanqun
#2 lore raw
Quoted 9 lines · 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. :)

Quoted 12 lines

sound/soc/soc-cache.c int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, · line 590

Quoted 7 lines
sound/soc/soc-cache.c +7 -0
+	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...

Signature · 1 line
~Vinod
author
#3 lore raw
Quoted 22 lines · Vinod
Quoted 9 lines · 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. :)

Quoted 12 lines

sound/soc/soc-cache.c int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, · line 590

Quoted 7 lines
sound/soc/soc-cache.c +7 -0
+	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.

Quoted 4 lines

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.

Quoted 9 lines

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

Signature · 1 line
~Vinod
Signature · 1 line
guanqun
#4 lore raw
In reply to Koul, Vinod
Quoted 2 lines · Vinod

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.

Quoted 3 lines

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.