[PATCH 0/3] ASoC: twl4030: support routine to external VOICE source.

[PATCH 0/3] ASoC: twl4030: support routine to external VOICE source.

[NeilBrown]

The 'voice' port of the twl4030 on my board is connected to
a GSM modem, not to the CPU.  As such it is not visible to ASLA
and normal approaches to configuring the interface (as one end of a
DAI) don't apply.

I need a way to tell the twl4030 that the connected device will
be master of 'clk' and 'FRM', and whether they are inverted.
Using device tree seems the correct approach - I am describing the
properties of the hardware attached the 'voice' port.

Using magic numbers from include/sound/soc-dai.h almost certainly
is not the right way to represent this information, but it isn't clear
to me what a good way would be.

So the current patch uses magic numbers as a proof of concept, and I'm
asking if there is any establish precedent, or any suggestions on
how to record in device tree the 'master' and 'polarity' of CLK and
FRM signals for an external device.
[I currently use (SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_CBS_CFS)]

This is in the second patch.  The first silences a pointless warning
(when it is pointless) and the last allowed the voice linked to be
turned on or off via ALSA controls.

If the first and last can be applied without finalizing the
device-tree part yet, that would be great.

Thanks,
NeilBrown

---

NeilBrown (3):
      ASoC: twl4030: don't report EBUSY if no change requested.
      ASoC: twl4030: allow voice port to be connected externally.
      ASoC: twl4030: enable routing audio to 'voice' interface.


 .../devicetree/bindings/mfd/twl4030-audio.txt      |    7 ++
 include/linux/i2c/twl.h                            |    3 +
 sound/soc/codecs/twl4030.c                         |   78 ++++++++++++++++++--
 3 files changed, 82 insertions(+), 6 deletions(-)

--
Signature

[PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[NeilBrown]

"mode" must not be changed when active.
However if a request is received to set the mode to what it currently
is, that is also rejected when active, which causes confusing
error messages.

So check first and if no change is actually requested, don't report
an error.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 sound/soc/codecs/twl4030.c |    6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index b6b0cb399599..c873f5887486 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -957,6 +957,12 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	int val = ucontrol->value.integer.value[0];
+
+	if (!!(twl4030_read(codec, TWL4030_REG_CODEC_MODE)
+	       & TWL4030_OPTION_1) == !!val)
+		/* No change */
+		return 0;
 
 	if (twl4030->configured) {
 		dev_err(codec->dev,

[PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[NeilBrown]

If voice port on twl4030 is not connected to a McBSP (or similar)
then we cannot configure the format the way we normally do for a DAI.

In this case, allow the platform data/devicetree to specify a format
which is put into effect when the 'voice' mode is selected.

If there is a voice connection, then we keep the twl side tri-stated
when not being driven.  This allows for the device to also be
connected to a McBSP if desired.

This is needed if the 'voice' port directly connects to a mobile-phone
module.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 .../devicetree/bindings/mfd/twl4030-audio.txt      |    7 ++++
 include/linux/i2c/twl.h                            |    3 ++
 sound/soc/codecs/twl4030.c                         |   34 +++++++++++++++++++-
 3 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/mfd/twl4030-audio.txt b/Documentation/devicetree/bindings/mfd/twl4030-audio.txt
index 414d2ae0adf6..f133cd9db6aa 100644
--- a/Documentation/devicetree/bindings/mfd/twl4030-audio.txt
+++ b/Documentation/devicetree/bindings/mfd/twl4030-audio.txt
@@ -19,6 +19,13 @@ Audio functionality:
 -ti,offset_cncl_path: Offset cancellation path selection, refer to TRM for the
 		      valid values.
 
+Voice functionality:
+- ti,voice_fmt:  The separate 'voice' interface is connected to a
+		 separate device such as a GSM modem.  This field
+		 sets the format expected by that device (who masters
+		 the clock/FRM and what their polarity is).
+
+
 Vibra functionality
 - ti,enable-vibra: Need to be set to <1> if the vibra functionality is used. if
 		   missing or it is 0, the vibra functionality is disabled.
diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 8cfb50f38529..0a59acd597b7 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h
@@ -688,6 +688,9 @@ struct twl4030_codec_data {
 	unsigned int offset_cncl_path;
 	unsigned int hs_extmute:1;
 	int hs_extmute_gpio;
+	unsigned int voice_fmt;	/* If != 0, gives format for external
+				 * voice connection.
+				 */
 };
 
 struct twl4030_vibra_data {
diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index c873f5887486..39b6d24377e5 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -221,6 +221,9 @@ static void twl4030_setup_pdata_of(struct twl4030_codec_data *pdata,
 	if (!of_property_read_u32(node, "ti,hs_extmute", &value))
 		pdata->hs_extmute = value;
 
+	if (of_property_read_u32(node, "ti,voice_fmt", &value) == 0)
+		pdata->voice_fmt = value;
+
 	pdata->hs_extmute_gpio = of_get_named_gpio(node,
 						   "ti,hs_extmute_gpio", 0);
 	if (gpio_is_valid(pdata->hs_extmute_gpio))
@@ -249,6 +252,8 @@ static struct twl4030_codec_data *twl4030_get_pdata(struct snd_soc_codec *codec)
 	return pdata;
 }
 
+static int twl4030_voice_set_codec_fmt(struct snd_soc_codec *codec,
+				       unsigned int fmt);
 static void twl4030_init_chip(struct snd_soc_codec *codec)
 {
 	struct twl4030_codec_data *pdata;
@@ -338,6 +343,16 @@ static void twl4030_init_chip(struct snd_soc_codec *codec)
 		 ((byte & TWL4030_CNCL_OFFSET_START) ==
 		  TWL4030_CNCL_OFFSET_START));
 
+	if (twl4030->pdata->voice_fmt) {
+		/* Configure voice format but keep interface
+		 * tri-state until enabled */
+		twl4030_voice_set_codec_fmt(codec,
+					    twl4030->pdata->voice_fmt);
+		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
+				 TWL4030_VIF_TRI_EN,
+				 TWL4030_REG_VOICE_IF);
+	}
+
 	twl4030_codec_enable(codec, 0);
 }
 
@@ -969,6 +984,15 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 			"operation mode cannot be changed on-the-fly\n");
 		return -EBUSY;
 	}
+	if (twl4030->pdata->voice_fmt) {
+		/* There is no SND interface to voice, we need to control
+		 * it here.
+		 */
+		/* If 'val' then voice is disabled, so tri-state it as well */
+		snd_soc_update_bits(codec, TWL4030_REG_VOICE_IF,
+				    TWL4030_VIF_TRI_EN,
+				    val ? 0xff : 0);
+	}
 
 	return snd_soc_put_enum_double(kcontrol, ucontrol);
 }
@@ -2038,10 +2062,9 @@ static int twl4030_voice_set_dai_sysclk(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
-static int twl4030_voice_set_dai_fmt(struct snd_soc_dai *codec_dai,
+static int twl4030_voice_set_codec_fmt(struct snd_soc_codec *codec,
 				     unsigned int fmt)
 {
-	struct snd_soc_codec *codec = codec_dai->codec;
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
 	u8 old_format, format;
 
@@ -2090,6 +2113,13 @@ static int twl4030_voice_set_dai_fmt(struct snd_soc_dai *codec_dai,
 	return 0;
 }
 
+static int twl4030_voice_set_dai_fmt(struct snd_soc_dai *codec_dai,
+		unsigned int fmt)
+{
+	struct snd_soc_codec *codec = codec_dai->codec;
+	return twl4030_voice_set_codec_fmt(codec, fmt);
+}
+
 static int twl4030_voice_set_tristate(struct snd_soc_dai *dai, int tristate)
 {
 	struct snd_soc_codec *codec = dai->codec;

[PATCH 3/3] ASoC: twl4030: enable routing audio to 'voice' interface.

[NeilBrown]

When 'voice' is some external connection, we power up
the downlink when the downline amplifier is not muted,
and power up the uplink when a new switch "Voice Uplink" is
turned on.

Signed-off-by: NeilBrown <neilb@suse.de>
---
 sound/soc/codecs/twl4030.c |   58 +++++++++++++++++++++++++++++++++-----------
 1 file changed, 44 insertions(+), 14 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 39b6d24377e5..f51fb749087f 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -348,9 +348,9 @@ static void twl4030_init_chip(struct snd_soc_codec *codec)
 		 * tri-state until enabled */
 		twl4030_voice_set_codec_fmt(codec,
 					    twl4030->pdata->voice_fmt);
-		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
-				 TWL4030_VIF_TRI_EN,
-				 TWL4030_REG_VOICE_IF);
+		/* These pins only relevant when voice_fmt set */
+		snd_soc_dapm_disable_pin(&codec->dapm, "VOICEIN");
+		snd_soc_dapm_disable_pin(&codec->dapm, "VOICEOUT");
 	}
 
 	twl4030_codec_enable(codec, 0);
@@ -985,15 +985,17 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 		return -EBUSY;
 	}
 	if (twl4030->pdata->voice_fmt) {
-		/* There is no SND interface to voice, we need to control
-		 * it here.
-		 */
-		/* If 'val' then voice is disabled, so tri-state it as well */
-		snd_soc_update_bits(codec, TWL4030_REG_VOICE_IF,
-				    TWL4030_VIF_TRI_EN,
-				    val ? 0xff : 0);
+		if (val) {
+			/* Voice now disabled */
+			snd_soc_dapm_disable_pin(&codec->dapm, "VOICEIN");
+			snd_soc_dapm_disable_pin(&codec->dapm, "VOICEOUT");
+		} else {
+			/* Voice now enabled */
+			snd_soc_dapm_enable_pin(&codec->dapm, "VOICEIN");
+			snd_soc_dapm_enable_pin(&codec->dapm, "VOICEOUT");
+		}
+		snd_soc_dapm_sync(&codec->dapm);
 	}
-
 	return snd_soc_put_enum_double(kcontrol, ucontrol);
 }
 
@@ -1016,6 +1018,13 @@ static DECLARE_TLV_DB_SCALE(digital_coarse_tlv, 0, 600, 0);
  */
 static DECLARE_TLV_DB_SCALE(digital_voice_downlink_tlv, -3700, 100, 1);
 
+static const struct snd_kcontrol_new twl4030_dapm_vdown_control =
+	SOC_DAPM_SINGLE_TLV("Volume",
+			    TWL4030_REG_VRXPGA, 0, 0x31, 0,
+			    digital_voice_downlink_tlv);
+
+static const struct snd_kcontrol_new twl4030_dapm_vup_control =
+	SOC_DAPM_SINGLE("Switch", TWL4030_REG_VOICE_IF, 5, 1, 0);
 /*
  * Analog playback gain
  * -24 dB to 12 dB in 2 dB steps
@@ -1126,9 +1135,6 @@ static const struct snd_kcontrol_new twl4030_snd_controls[] = {
 		TWL4030_REG_ARXL2_APGA_CTL, TWL4030_REG_ARXR2_APGA_CTL,
 		1, 1, 0),
 
-	/* Common voice downlink gain controls */
-	SOC_SINGLE_TLV("DAC Voice Digital Downlink Volume",
-		TWL4030_REG_VRXPGA, 0, 0x31, 0, digital_voice_downlink_tlv),
 
 	SOC_SINGLE_TLV("DAC Voice Analog Downlink Volume",
 		TWL4030_REG_VDL_APGA_CTL, 3, 0x12, 1, analog_tlv),
@@ -1188,6 +1194,7 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	/* Digital microphones (Stereo) */
 	SND_SOC_DAPM_INPUT("DIGIMIC0"),
 	SND_SOC_DAPM_INPUT("DIGIMIC1"),
+	SND_SOC_DAPM_INPUT("VOICEIN"),
 
 	/* Outputs */
 	SND_SOC_DAPM_OUTPUT("EARPIECE"),
@@ -1200,6 +1207,7 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	SND_SOC_DAPM_OUTPUT("HFL"),
 	SND_SOC_DAPM_OUTPUT("HFR"),
 	SND_SOC_DAPM_OUTPUT("VIBRA"),
+	SND_SOC_DAPM_OUTPUT("VOICEOUT"),
 
 	/* AIF and APLL clocks for running DAIs (including loopback) */
 	SND_SOC_DAPM_OUTPUT("Virtual HiFi OUT"),
@@ -1216,6 +1224,10 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 	SND_SOC_DAPM_AIF_IN("VAIFIN", "Voice Playback", 0,
 			    TWL4030_REG_VOICE_IF, 6, 0),
 
+	SND_SOC_DAPM_SUPPLY("VoiceEnable", TWL4030_REG_VOICE_IF, 6, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("VoicePlay", TWL4030_REG_OPTION, 4, 0, NULL, 0),
+	SND_SOC_DAPM_SUPPLY("VoiceCapture", TWL4030_REG_OPTION, 2, 0, NULL, 0),
+
 	/* Analog bypasses */
 	SND_SOC_DAPM_SWITCH("Right1 Analog Loopback", SND_SOC_NOPM, 0, 0,
 			&twl4030_dapm_abypassr1_control),
@@ -1395,9 +1407,27 @@ static const struct snd_soc_dapm_widget twl4030_dapm_widgets[] = {
 			    TWL4030_REG_MICBIAS_CTL, 2, 0, NULL, 0),
 
 	SND_SOC_DAPM_SUPPLY("VIF Enable", TWL4030_REG_VOICE_IF, 0, 0, NULL, 0),
+
+	SND_SOC_DAPM_SUPPLY("Voice NO-tri", TWL4030_REG_VOICE_IF, 2, 1, NULL, 0),
+
+	SND_SOC_DAPM_SWITCH("DAC Voice Digital Downlink", SND_SOC_NOPM,
+			    0, 0, &twl4030_dapm_vdown_control),
+	SND_SOC_DAPM_SWITCH("Voice Uplink", SND_SOC_NOPM,
+			    0, 0, &twl4030_dapm_vup_control),
 };
 
 static const struct snd_soc_dapm_route intercon[] = {
+	{"DAC Voice", NULL, "DAC Voice Digital Downlink"},
+	{"DAC Voice", NULL, "VoiceEnable"},
+	{"DAC Voice", NULL, "VoicePlay"},
+	{"DAC Voice Digital Downlink", "Volume", "VOICEIN"},
+
+	{"VOICEOUT", NULL, "Voice Uplink"},
+	{"VOICEOUT", NULL, "VoiceEnable"},
+	{"VOICEOUT", NULL, "VoiceCapture"},
+	{"VOICEOUT", NULL, "Voice NO-tri"},
+	{"Voice Uplink", "Switch", "TX2 Capture Route"},
+
 	/* Stream -> DAC mapping */
 	{"DAC Right1", NULL, "HiFi Playback"},
 	{"DAC Left1", NULL, "HiFi Playback"},

[PATCH 0/3] ASoC: twl4030: support routine to external VOICE source.

[NeilBrown]

The 'voice' port of the twl4030 on my board is connected to
a GSM modem, not to the CPU.  As such it is not visible to ASLA
and normal approaches to configuring the interface (as one end of a
DAI) don't apply.

I need a way to tell the twl4030 that the connected device will
be master of 'clk' and 'FRM', and whether they are inverted.
Using device tree seems the correct approach - I am describing the
properties of the hardware attached the 'voice' port.

Using magic numbers from include/sound/soc-dai.h almost certainly
is not the right way to represent this information, but it isn't clear
to me what a good way would be.

So the current patch uses magic numbers as a proof of concept, and I'm
asking if there is any establish precedent, or any suggestions on
how to record in device tree the 'master' and 'polarity' of CLK and
FRM signals for an external device.
[I currently use (SND_SOC_DAIFMT_IB_NF | SND_SOC_DAIFMT_CBS_CFS)]

This is in the second patch.  The first silences a pointless warning
(when it is pointless) and the last allowed the voice linked to be
turned on or off via ALSA controls.

If the first and last can be applied without finalizing the
device-tree part yet, that would be great.

Thanks,
NeilBrown

---

NeilBrown (3):
      ASoC: twl4030: don't report EBUSY if no change requested.
      ASoC: twl4030: allow voice port to be connected externally.
      ASoC: twl4030: enable routing audio to 'voice' interface.


 .../devicetree/bindings/mfd/twl4030-audio.txt      |    7 ++
 include/linux/i2c/twl.h                            |    3 +
 sound/soc/codecs/twl4030.c                         |   78 ++++++++++++++++++--
 3 files changed, 82 insertions(+), 6 deletions(-)

--
Signature

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:

> +	int val = ucontrol->value.integer.value[0];
> +
> +	if (!!(twl4030_read(codec, TWL4030_REG_CODEC_MODE)
> +	       & TWL4030_OPTION_1) == !!val)
> +		/* No change */
> +		return 0;

We shouldn't be accepting attempts to set out of range values so there
should be no need for the !! on val (which confused me when I was
reading this).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]

On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:

> +	int val = ucontrol->value.integer.value[0];
> +
> +	if (!!(twl4030_read(codec, TWL4030_REG_CODEC_MODE)
> +	       & TWL4030_OPTION_1) == !!val)
> +		/* No change */
> +		return 0;

We shouldn't be accepting attempts to set out of range values so there
should be no need for the !! on val (which confused me when I was
reading this).

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 482 bytes --]

On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:

> If voice port on twl4030 is not connected to a McBSP (or similar)
> then we cannot configure the format the way we normally do for a DAI.

Yes we can, you need to represent the DAI link to whatever else the
device is connected to in the driver like we do anything else - and in
any case this isn't a device specific issue so we shouldn't be doing
something driver specific to solve it.  Look at something like speyside.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 482 bytes --]

On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:

> If voice port on twl4030 is not connected to a McBSP (or similar)
> then we cannot configure the format the way we normally do for a DAI.

Yes we can, you need to represent the DAI link to whatever else the
device is connected to in the driver like we do anything else - and in
any case this isn't a device specific issue so we shouldn't be doing
something driver specific to solve it.  Look at something like speyside.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ASoC: twl4030: enable routing audio to 'voice' interface.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 529 bytes --]

On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:

> -		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
> -				 TWL4030_VIF_TRI_EN,
> -				 TWL4030_REG_VOICE_IF);
> +		/* These pins only relevant when voice_fmt set */
> +		snd_soc_dapm_disable_pin(&codec->dapm, "VOICEIN");
> +		snd_soc_dapm_disable_pin(&codec->dapm, "VOICEOUT");

Given your previous patch are these trying to control a digital link by
any chance?  If they are they should be removed, and in any case this
sort of thing looks like a machine driver issue.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2887 bytes --]

On Sat, 8 Nov 2014 09:26:22 +0000 Mark Brown <broonie@kernel.org> wrote:

> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
> 
> > If voice port on twl4030 is not connected to a McBSP (or similar)
> > then we cannot configure the format the way we normally do for a DAI.
> 
> Yes we can, you need to represent the DAI link to whatever else the
> device is connected to in the driver like we do anything else - and in
> any case this isn't a device specific issue so we shouldn't be doing
> something driver specific to solve it.  Look at something like speyside.

Hi Mark,
 thanks for the reply ... I might need a little bit more help though.

I had a look at sound/soc/samsung/speyside.c, but I'm not entirely sure what
I'm looking for.
Presumably this is an audio processor not unlike the audio module in the
twl4030.

I see that there are 3 dai-links:
  CPU-DSP
  DSP-CODEC
  Baseband

Presumably "Baseband" is similar, in purpose at least, to the "voice"
interface on the twl4030.

Each dai-link has a "cpu_dai_name" and a "codec_dai_name", even though it
appears that only "CPU-DSP" is connected to the CPU.  Maybe that naming is
the source of some of my confusion.

"Baseband" declares
                .cpu_dai_name = "wm8996-aif2",
so wm8996 is something with 2 audio interfaces, (aif), and this is the second
one?  Maybe the  wm8996 is the audio module, so what is the "speyside"?

http://opensource.wolfsonmicro.com/content/speyside-audio

says it is a "reference platform".  Does that mean it is a board with a bunch
of chips soldered onto it?  If it were a board it should be described by a
dts file, not by a pile of C code (I thought), so I must be wrong about that.


In my case, I have a board with a GSM module and the twl4030 module.  Each
has an audio interface and these are connected.  I assume that I need to
express this connection in the dts file.
The GSM module doesn't currently appear in the dts file as it is usb-attached.
However I've been thinking that we will need to add it so we can express
power-on controls (twiddling some GPIOs).  So let's suppose we have the GSM
module in the dts file (child of a USB interface) and the twl4030 as well
(beneath an i2c interface).

The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
module declares that these are.  So presumably we need some sort of linkage.
Ahhhh... I found Documentation/devicetree/bindings/sound/simple-card.txt

So I need to make the "voice" port on the twl4030 look like a "cpu" end of a
dai-link, and create a "codec" end in the GSM module, and use "sound-dai" to
point from the twl4030 to the GSM module.
Then I use frame-master, bitclock-master, bitclock-inversion, frame-inversion
for the settings I need.

I suspect I can make that work.

Am I on the right track?

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[NeilBrown]

[-- Attachment #1.1: Type: text/plain, Size: 2887 bytes --]

On Sat, 8 Nov 2014 09:26:22 +0000 Mark Brown <broonie@kernel.org> wrote:

> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
> 
> > If voice port on twl4030 is not connected to a McBSP (or similar)
> > then we cannot configure the format the way we normally do for a DAI.
> 
> Yes we can, you need to represent the DAI link to whatever else the
> device is connected to in the driver like we do anything else - and in
> any case this isn't a device specific issue so we shouldn't be doing
> something driver specific to solve it.  Look at something like speyside.

Hi Mark,
 thanks for the reply ... I might need a little bit more help though.

I had a look at sound/soc/samsung/speyside.c, but I'm not entirely sure what
I'm looking for.
Presumably this is an audio processor not unlike the audio module in the
twl4030.

I see that there are 3 dai-links:
  CPU-DSP
  DSP-CODEC
  Baseband

Presumably "Baseband" is similar, in purpose at least, to the "voice"
interface on the twl4030.

Each dai-link has a "cpu_dai_name" and a "codec_dai_name", even though it
appears that only "CPU-DSP" is connected to the CPU.  Maybe that naming is
the source of some of my confusion.

"Baseband" declares
                .cpu_dai_name = "wm8996-aif2",
so wm8996 is something with 2 audio interfaces, (aif), and this is the second
one?  Maybe the  wm8996 is the audio module, so what is the "speyside"?

http://opensource.wolfsonmicro.com/content/speyside-audio

says it is a "reference platform".  Does that mean it is a board with a bunch
of chips soldered onto it?  If it were a board it should be described by a
dts file, not by a pile of C code (I thought), so I must be wrong about that.


In my case, I have a board with a GSM module and the twl4030 module.  Each
has an audio interface and these are connected.  I assume that I need to
express this connection in the dts file.
The GSM module doesn't currently appear in the dts file as it is usb-attached.
However I've been thinking that we will need to add it so we can express
power-on controls (twiddling some GPIOs).  So let's suppose we have the GSM
module in the dts file (child of a USB interface) and the twl4030 as well
(beneath an i2c interface).

The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
module declares that these are.  So presumably we need some sort of linkage.
Ahhhh... I found Documentation/devicetree/bindings/sound/simple-card.txt

So I need to make the "voice" port on the twl4030 look like a "cpu" end of a
dai-link, and create a "codec" end in the GSM module, and use "sound-dai" to
point from the twl4030 to the GSM module.
Then I use frame-master, bitclock-master, bitclock-inversion, frame-inversion
for the settings I need.

I suspect I can make that work.

Am I on the right track?

Thanks,
NeilBrown

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 3/3] ASoC: twl4030: enable routing audio to 'voice' interface.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1492 bytes --]

On Sat, 8 Nov 2014 09:27:56 +0000 Mark Brown <broonie@kernel.org> wrote:

> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
> 
> > -		twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
> > -				 TWL4030_VIF_TRI_EN,
> > -				 TWL4030_REG_VOICE_IF);
> > +		/* These pins only relevant when voice_fmt set */
> > +		snd_soc_dapm_disable_pin(&codec->dapm, "VOICEIN");
> > +		snd_soc_dapm_disable_pin(&codec->dapm, "VOICEOUT");
> 
> Given your previous patch are these trying to control a digital link by
> any chance?  If they are they should be removed, and in any case this
> sort of thing looks like a machine driver issue.


Depends on what you mean by "control".
They declare that a digital link is, or is not, active so that the related
amplifiers, DACs, etc can be powered up or down.

The "VIF_TRI_EN" puts the digital interface in 'tristate' mode which
effectively disables it.  So that bit certainly "controls" the digital link.


If I shouldn't have these controls here, where should I have them?  How
should I turn on/off the widgets that drive the VOICE interface?

You say it looks like "a machine driver issue".
alsa/soc/machine.txt says that "machine" is a synonym for "board".
I thought we were getting rid of board files and replacing them with
devicetree.  You seem to be implying that we are keeping board files (under
the name "machine driver") for the audio config.

Is that correct?  What is the reason for that?

Thanks,
NeilBrown

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2050 bytes --]

On Sat, 8 Nov 2014 09:22:42 +0000 Mark Brown <broonie@kernel.org> wrote:

> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
> 
> > +	int val = ucontrol->value.integer.value[0];
> > +
> > +	if (!!(twl4030_read(codec, TWL4030_REG_CODEC_MODE)
> > +	       & TWL4030_OPTION_1) == !!val)
> > +		/* No change */
> > +		return 0;
> 
> We shouldn't be accepting attempts to set out of range values so there
> should be no need for the !! on val (which confused me when I was
> reading this).

True, the '!!' on val isn't necessary.

I took the opportunity to look around a bit more and discovered there was a
snd_soc_get_enum_double, which I really should use in preference to
twl4030_read.

So here is the new version.  Can it be taken out of the email, or should I
resend stand-alone?

Thanks,
NeilBrown


From: NeilBrown <neilb@suse.de>
Date: Wed, 2 Oct 2013 14:45:52 +1000
Subject: [PATCH] ASoC: twl4030: don't report EBUSY if no change requested.

"mode" must not be changed when active.
However if a request is received to set the mode to what it currently
is, that is also rejected when active, which causes confusing
error messages.

So check first and if no change is actually requested, don't report
an error.

Signed-off-by: NeilBrown <neilb@suse.de>

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index b6b0cb399599..613b61cee081 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	struct snd_ctl_elem_value currentval;
+	int ret;
+
+	ret = snd_soc_get_enum_double(kcontrol, &currentval);
+	if (ret)
+		return ret;
+	if (ucontrol->value.enumerated.item[0] ==
+	    currentval.value.enumerated.item[0])
+		/* No change */
+		return 0;
 
 	if (twl4030->configured) {
 		dev_err(codec->dev,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 2121 bytes --]

On Sat, 8 Nov 2014 09:22:42 +0000 Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
> 
> > +	int val = ucontrol->value.integer.value[0];
> > +
> > +	if (!!(twl4030_read(codec, TWL4030_REG_CODEC_MODE)
> > +	       & TWL4030_OPTION_1) == !!val)
> > +		/* No change */
> > +		return 0;
> 
> We shouldn't be accepting attempts to set out of range values so there
> should be no need for the !! on val (which confused me when I was
> reading this).

True, the '!!' on val isn't necessary.

I took the opportunity to look around a bit more and discovered there was a
snd_soc_get_enum_double, which I really should use in preference to
twl4030_read.

So here is the new version.  Can it be taken out of the email, or should I
resend stand-alone?

Thanks,
NeilBrown


From: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>
Date: Wed, 2 Oct 2013 14:45:52 +1000
Subject: [PATCH] ASoC: twl4030: don't report EBUSY if no change requested.

"mode" must not be changed when active.
However if a request is received to set the mode to what it currently
is, that is also rejected when active, which causes confusing
error messages.

So check first and if no change is actually requested, don't report
an error.

Signed-off-by: NeilBrown <neilb-l3A5Bk7waGM@public.gmane.org>

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index b6b0cb399599..613b61cee081 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	struct snd_ctl_elem_value currentval;
+	int ret;
+
+	ret = snd_soc_get_enum_double(kcontrol, &currentval);
+	if (ret)
+		return ret;
+	if (ucontrol->value.enumerated.item[0] ==
+	    currentval.value.enumerated.item[0])
+		/* No change */
+		return 0;
 
 	if (twl4030->configured) {
 		dev_err(codec->dev,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [Gta04-owner] [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Dr. H. Nikolaus Schaller]

Am 10.11.2014 um 00:25 schrieb NeilBrown <neilb@suse.de>:

> On Sat, 8 Nov 2014 09:26:22 +0000 Mark Brown <broonie@kernel.org> wrote:
> 
>> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
>> 
>>> If voice port on twl4030 is not connected to a McBSP (or similar)
>>> then we cannot configure the format the way we normally do for a DAI.
>> 
>> Yes we can, you need to represent the DAI link to whatever else the
>> device is connected to in the driver like we do anything else - and in
>> any case this isn't a device specific issue so we shouldn't be doing
>> something driver specific to solve it.  Look at something like speyside.
> 
> Hi Mark,
> thanks for the reply ... I might need a little bit more help though.
> 
> I had a look at sound/soc/samsung/speyside.c, but I'm not entirely sure what
> I'm looking for.
> Presumably this is an audio processor not unlike the audio module in the
> twl4030.
> 
> I see that there are 3 dai-links:
>  CPU-DSP
>  DSP-CODEC
>  Baseband
> 
> Presumably "Baseband" is similar, in purpose at least, to the "voice"
> interface on the twl4030.
> 
> Each dai-link has a "cpu_dai_name" and a "codec_dai_name", even though it
> appears that only "CPU-DSP" is connected to the CPU.  Maybe that naming is
> the source of some of my confusion.
> 
> "Baseband" declares
>                .cpu_dai_name = "wm8996-aif2",
> so wm8996 is something with 2 audio interfaces, (aif), and this is the second
> one?  Maybe the  wm8996 is the audio module, so what is the "speyside"?
> 
> http://opensource.wolfsonmicro.com/content/speyside-audio
> 
> says it is a "reference platform".  Does that mean it is a board with a bunch
> of chips soldered onto it?  If it were a board it should be described by a
> dts file, not by a pile of C code (I thought), so I must be wrong about that.
> 
> 
> In my case, I have a board with a GSM module and the twl4030 module.  Each
> has an audio interface and these are connected.  I assume that I need to
> express this connection in the dts file.
> The GSM module doesn't currently appear in the dts file as it is usb-attached.
> However I've been thinking that we will need to add it so we can express
> power-on controls (twiddling some GPIOs).  So let's suppose we have the GSM
> module in the dts file (child of a USB interface)

It is a quite complex connection pattern and I am not sure if the modem is really
a logical child of the USB interface. Powering up/down the USB interface has nothing
to do with the modem power. Rather, it continues to operate if USB is suspended
and the modem notifies USB that it has become active.

The connections on this GTA04 board are:

Modem USB <—> CPU USB
Modem PCM <—> TWL4030 Voice <—> OMAP3 McBSP4 (yes, it is a 3-way connection)
Modem Power control <—> 1 or 2 GPIOs (depending on board variant)

The reason for the 3-way connection is that user space can chose if the GSM
voice is routed directly to the TWL codec (low latency, independent of CPU) or
goes through the CPU (e.g. for DTAM or voice scrambling by software) and
then through the other PCM link between the CPU and the TWL.

This is why I would make the McBSP4 a separate sound card.

And, this is why it needs some control and tri-state of the TWL4030 and OMAP3
McBSP4 since only one can provide a digital PCM stream to the modem.

One more thing to consider for a general solution is that there are modem modules
that communicate data through UART or SPI - but otherwise have a similar connection
for digital audio. So forcing the power control driver to be a subnode of USB doesn’t
appear general enough to me.

Finally, this connection pattern is not specific to this modem (OPTION GTM601) on this
GTA04 board. We plan to use a Gemalto PHS8  in the Pyra-Handheld and the Neo900
devices - but the general connection pattern as defined above remains the same.

So my proposal is to have a specific wwan-power driver for this type of modems.
And power control can and should be kept separately from USB (except the case
where only 1 GPIO exists and USB must be tapped to detect the modem power
state). You can find work in progress for this approach here:

<http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=drivers/misc/wwan-on-off.c;h=e13f47dbd734d14f4e38ea9cf622982dc3550212;hb=154bcd388a8f5bcc7fcf0e57e93b6388daa16980>

> and the twl4030 as well
> (beneath an i2c interface).
> 
> The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
> module declares that these are.  So presumably we need some sort of linkage.
> Ahhhh... I found Documentation/devicetree/bindings/sound/simple-card.txt
> 
> So I need to make the "voice" port on the twl4030 look like a "cpu" end of a
> dai-link, and create a "codec" end in the GSM module, and use "sound-dai" to
> point from the twl4030 to the GSM module.

What I wonder a little is that we have all these dai-links working since your 3.7
kernel for GTA04. Is it necessary to re-invent a solution or should we just make
the solution device tree compatible?

> Then I use frame-master, bitclock-master, bitclock-inversion, frame-inversion
> for the settings I need.
> 
> I suspect I can make that work.
> 
> Am I on the right track?
> 
> Thanks,
> NeilBrown
> _______________________________________________
> Gta04-owner mailing list
> Gta04-owner@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Re: [Gta04-owner] [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Dr. H. Nikolaus Schaller]

Am 10.11.2014 um 00:25 schrieb NeilBrown <neilb@suse.de>:

> On Sat, 8 Nov 2014 09:26:22 +0000 Mark Brown <broonie@kernel.org> wrote:
> 
>> On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:
>> 
>>> If voice port on twl4030 is not connected to a McBSP (or similar)
>>> then we cannot configure the format the way we normally do for a DAI.
>> 
>> Yes we can, you need to represent the DAI link to whatever else the
>> device is connected to in the driver like we do anything else - and in
>> any case this isn't a device specific issue so we shouldn't be doing
>> something driver specific to solve it.  Look at something like speyside.
> 
> Hi Mark,
> thanks for the reply ... I might need a little bit more help though.
> 
> I had a look at sound/soc/samsung/speyside.c, but I'm not entirely sure what
> I'm looking for.
> Presumably this is an audio processor not unlike the audio module in the
> twl4030.
> 
> I see that there are 3 dai-links:
>  CPU-DSP
>  DSP-CODEC
>  Baseband
> 
> Presumably "Baseband" is similar, in purpose at least, to the "voice"
> interface on the twl4030.
> 
> Each dai-link has a "cpu_dai_name" and a "codec_dai_name", even though it
> appears that only "CPU-DSP" is connected to the CPU.  Maybe that naming is
> the source of some of my confusion.
> 
> "Baseband" declares
>                .cpu_dai_name = "wm8996-aif2",
> so wm8996 is something with 2 audio interfaces, (aif), and this is the second
> one?  Maybe the  wm8996 is the audio module, so what is the "speyside"?
> 
> http://opensource.wolfsonmicro.com/content/speyside-audio
> 
> says it is a "reference platform".  Does that mean it is a board with a bunch
> of chips soldered onto it?  If it were a board it should be described by a
> dts file, not by a pile of C code (I thought), so I must be wrong about that.
> 
> 
> In my case, I have a board with a GSM module and the twl4030 module.  Each
> has an audio interface and these are connected.  I assume that I need to
> express this connection in the dts file.
> The GSM module doesn't currently appear in the dts file as it is usb-attached.
> However I've been thinking that we will need to add it so we can express
> power-on controls (twiddling some GPIOs).  So let's suppose we have the GSM
> module in the dts file (child of a USB interface)

It is a quite complex connection pattern and I am not sure if the modem is really
a logical child of the USB interface. Powering up/down the USB interface has nothing
to do with the modem power. Rather, it continues to operate if USB is suspended
and the modem notifies USB that it has become active.

The connections on this GTA04 board are:

Modem USB <—> CPU USB
Modem PCM <—> TWL4030 Voice <—> OMAP3 McBSP4 (yes, it is a 3-way connection)
Modem Power control <—> 1 or 2 GPIOs (depending on board variant)

The reason for the 3-way connection is that user space can chose if the GSM
voice is routed directly to the TWL codec (low latency, independent of CPU) or
goes through the CPU (e.g. for DTAM or voice scrambling by software) and
then through the other PCM link between the CPU and the TWL.

This is why I would make the McBSP4 a separate sound card.

And, this is why it needs some control and tri-state of the TWL4030 and OMAP3
McBSP4 since only one can provide a digital PCM stream to the modem.

One more thing to consider for a general solution is that there are modem modules
that communicate data through UART or SPI - but otherwise have a similar connection
for digital audio. So forcing the power control driver to be a subnode of USB doesn’t
appear general enough to me.

Finally, this connection pattern is not specific to this modem (OPTION GTM601) on this
GTA04 board. We plan to use a Gemalto PHS8  in the Pyra-Handheld and the Neo900
devices - but the general connection pattern as defined above remains the same.

So my proposal is to have a specific wwan-power driver for this type of modems.
And power control can and should be kept separately from USB (except the case
where only 1 GPIO exists and USB must be tapped to detect the modem power
state). You can find work in progress for this approach here:

<http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=drivers/misc/wwan-on-off.c;h=e13f47dbd734d14f4e38ea9cf622982dc3550212;hb=154bcd388a8f5bcc7fcf0e57e93b6388daa16980>

> and the twl4030 as well
> (beneath an i2c interface).
> 
> The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
> module declares that these are.  So presumably we need some sort of linkage.
> Ahhhh... I found Documentation/devicetree/bindings/sound/simple-card.txt
> 
> So I need to make the "voice" port on the twl4030 look like a "cpu" end of a
> dai-link, and create a "codec" end in the GSM module, and use "sound-dai" to
> point from the twl4030 to the GSM module.

What I wonder a little is that we have all these dai-links working since your 3.7
kernel for GTA04. Is it necessary to re-invent a solution or should we just make
the solution device tree compatible?

> Then I use frame-master, bitclock-master, bitclock-inversion, frame-inversion
> for the settings I need.
> 
> I suspect I can make that work.
> 
> Am I on the right track?
> 
> Thanks,
> NeilBrown
> _______________________________________________
> Gta04-owner mailing list
> Gta04-owner@goldelico.com
> http://lists.goldelico.com/mailman/listinfo.cgi/gta04-owner

Re: [alsa-devel] [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[Lars-Peter Clausen]

On 11/10/2014 01:45 AM, NeilBrown wrote:
> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> index b6b0cb399599..613b61cee081 100644
> --- a/sound/soc/codecs/twl4030.c
> +++ b/sound/soc/codecs/twl4030.c
> @@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
>   {
>   	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
>   	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
> +	struct snd_ctl_elem_value currentval;
>

snd_ctl_elem_value is a bit to big to be put onto the kernel stack. Just 
using twl4030_read() should be fine.

Re: [PATCH 3/3] ASoC: twl4030: enable routing audio to 'voice' interface.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1652 bytes --]

On Mon, Nov 10, 2014 at 10:54:38AM +1100, NeilBrown wrote:
> On Sat, 8 Nov 2014 09:27:56 +0000 Mark Brown <broonie@kernel.org> wrote:
> > On Sat, Nov 08, 2014 at 11:38:03AM +1100, NeilBrown wrote:

> > Given your previous patch are these trying to control a digital link by
> > any chance?  If they are they should be removed, and in any case this
> > sort of thing looks like a machine driver issue.

> Depends on what you mean by "control".
> They declare that a digital link is, or is not, active so that the related
> amplifiers, DACs, etc can be powered up or down.

OK, then the driver needs to be fixed so that this is an actual DAI and
not analogue.

> If I shouldn't have these controls here, where should I have them?  How
> should I turn on/off the widgets that drive the VOICE interface?

Via AIF widgets, supply widgets or something else.

> You say it looks like "a machine driver issue".
> alsa/soc/machine.txt says that "machine" is a synonym for "board".
> I thought we were getting rid of board files and replacing them with
> devicetree.  You seem to be implying that we are keeping board files (under
> the name "machine driver") for the audio config.

> Is that correct?  What is the reason for that?

Yes.  The board design for advanced audio subsystems (like those found
it smartphones) is non-trivial and worth representing as a device in
itself.  Please see previous and repeated discussions on list, I'm fed
up of having to go over this with everyone individually.

Note also that even where some generic code that applies to multiple
boards is used you *still* need something out side the driver to join
everything together.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Mon, Nov 10, 2014 at 10:25:51AM +1100, NeilBrown wrote:

> says it is a "reference platform".  Does that mean it is a board with a bunch
> of chips soldered onto it?  If it were a board it should be described by a
> dts file, not by a pile of C code (I thought), so I must be wrong about that.

No, like I say please see previous discussion ad nauseum about this.

> The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
> module declares that these are.  So presumably we need some sort of linkage.
> Ahhhh... I found Documentation/devicetree/bindings/sound/simple-card.txt

Or just write a machine driver.  simple-card is one such machine driver
but it's not going to be suitable for non-trivial systems.  Possibly
your system is simple enough, possibly it isn't - I don't know anything
about it really.  Regardless of how you do this it shouldn't have
individual devices needing to open code configuration for links, it's
something essentially every device is going to need to do and forcing
the DT to configure the same thing twice for a link isn't a good idea
either.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH 2/3] ASoC: twl4030: allow voice port to be connected externally.

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1099 bytes --]

On Mon, Nov 10, 2014 at 10:25:51AM +1100, NeilBrown wrote:

> says it is a "reference platform".  Does that mean it is a board with a bunch
> of chips soldered onto it?  If it were a board it should be described by a
> dts file, not by a pile of C code (I thought), so I must be wrong about that.

No, like I say please see previous discussion ad nauseum about this.

> The twl4030 needs to know the master/polarity of the clk/frm lines.  The GSM
> module declares that these are.  So presumably we need some sort of linkage.
> Ahhhh... I found Documentation/devicetree/bindings/sound/simple-card.txt

Or just write a machine driver.  simple-card is one such machine driver
but it's not going to be suitable for non-trivial systems.  Possibly
your system is simple enough, possibly it isn't - I don't know anything
about it really.  Regardless of how you do this it shouldn't have
individual devices needing to open code configuration for links, it's
something essentially every device is going to need to do and forcing
the DT to configure the same thing twice for a link isn't a good idea
either.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [alsa-devel] [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

On Mon, 10 Nov 2014 08:07:50 +0100 Lars-Peter Clausen <lars@metafoo.de> wrote:

> On 11/10/2014 01:45 AM, NeilBrown wrote:
> > diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> > index b6b0cb399599..613b61cee081 100644
> > --- a/sound/soc/codecs/twl4030.c
> > +++ b/sound/soc/codecs/twl4030.c
> > @@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
> >   {
> >   	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
> >   	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
> > +	struct snd_ctl_elem_value currentval;
> >
> 
> snd_ctl_elem_value is a bit to big to be put onto the kernel stack. Just 
> using twl4030_read() should be fine.

That's a shame, it looked so neat....

Using twl4030_read forces i2c access and misses out on the regmap caching.

What do you think of this?

Thanks,
NeilBrown

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index b6b0cb399599..bdb47a045aa5 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -957,6 +957,18 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	const struct soc_enum *e = &twl4030_op_modes_enum;
+	unsigned int reg_val;
+	int ret;
+
+	ret = snd_soc_component_read(snd_kcontrol_chip(kcontrol),
+				     e->reg, &reg_val);
+	if (ret)
+		return ret;
+	if (ucontrol->value.enumerated.item[0] ==
+	    ((reg_val >> e->shift_l) & e->mask))
+		/* no change requested, so do nothing */
+		return 0;
 
 	if (twl4030->configured) {
 		dev_err(codec->dev,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [alsa-devel] [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1794 bytes --]

On Mon, 10 Nov 2014 08:07:50 +0100 Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:

> On 11/10/2014 01:45 AM, NeilBrown wrote:
> > diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> > index b6b0cb399599..613b61cee081 100644
> > --- a/sound/soc/codecs/twl4030.c
> > +++ b/sound/soc/codecs/twl4030.c
> > @@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
> >   {
> >   	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
> >   	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
> > +	struct snd_ctl_elem_value currentval;
> >
> 
> snd_ctl_elem_value is a bit to big to be put onto the kernel stack. Just 
> using twl4030_read() should be fine.

That's a shame, it looked so neat....

Using twl4030_read forces i2c access and misses out on the regmap caching.

What do you think of this?

Thanks,
NeilBrown

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index b6b0cb399599..bdb47a045aa5 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c
@@ -957,6 +957,18 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
 {
 	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	const struct soc_enum *e = &twl4030_op_modes_enum;
+	unsigned int reg_val;
+	int ret;
+
+	ret = snd_soc_component_read(snd_kcontrol_chip(kcontrol),
+				     e->reg, &reg_val);
+	if (ret)
+		return ret;
+	if (ucontrol->value.enumerated.item[0] ==
+	    ((reg_val >> e->shift_l) & e->mask))
+		/* no change requested, so do nothing */
+		return 0;
 
 	if (twl4030->configured) {
 		dev_err(codec->dev,

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [alsa-devel] [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[Lars-Peter Clausen]

On 11/10/2014 10:45 PM, NeilBrown wrote:
> On Mon, 10 Nov 2014 08:07:50 +0100 Lars-Peter Clausen <lars@metafoo.de> wrote:
>
>> On 11/10/2014 01:45 AM, NeilBrown wrote:
>>> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
>>> index b6b0cb399599..613b61cee081 100644
>>> --- a/sound/soc/codecs/twl4030.c
>>> +++ b/sound/soc/codecs/twl4030.c
>>> @@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
>>>    {
>>>    	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
>>>    	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
>>> +	struct snd_ctl_elem_value currentval;
>>>
>>
>> snd_ctl_elem_value is a bit to big to be put onto the kernel stack. Just
>> using twl4030_read() should be fine.
>
> That's a shame, it looked so neat....
>
> Using twl4030_read forces i2c access and misses out on the regmap caching.
>
> What do you think of this?

That looks good, but either snd_soc_kcontrol_component() instead of 
snd_kcontrol_chip() or just snd_soc_read(codec, ...

>
> Thanks,
> NeilBrown
>
> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> index b6b0cb399599..bdb47a045aa5 100644
> --- a/sound/soc/codecs/twl4030.c
> +++ b/sound/soc/codecs/twl4030.c
> @@ -957,6 +957,18 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
>   {
>   	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
>   	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
> +	const struct soc_enum *e = &twl4030_op_modes_enum;
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = snd_soc_component_read(snd_kcontrol_chip(kcontrol),
> +				     e->reg, &reg_val);
> +	if (ret)
> +		return ret;
> +	if (ucontrol->value.enumerated.item[0] ==
> +	    ((reg_val >> e->shift_l) & e->mask))
> +		/* no change requested, so do nothing */
> +		return 0;
>
>   	if (twl4030->configured) {
>   		dev_err(codec->dev,
>

Re: [alsa-devel] [PATCH 1/3] ASoC: twl4030: don't report EBUSY if no change requested.

[Lars-Peter Clausen]

On 11/10/2014 10:45 PM, NeilBrown wrote:
> On Mon, 10 Nov 2014 08:07:50 +0100 Lars-Peter Clausen <lars-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> wrote:
>
>> On 11/10/2014 01:45 AM, NeilBrown wrote:
>>> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
>>> index b6b0cb399599..613b61cee081 100644
>>> --- a/sound/soc/codecs/twl4030.c
>>> +++ b/sound/soc/codecs/twl4030.c
>>> @@ -957,6 +957,16 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
>>>    {
>>>    	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
>>>    	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
>>> +	struct snd_ctl_elem_value currentval;
>>>
>>
>> snd_ctl_elem_value is a bit to big to be put onto the kernel stack. Just
>> using twl4030_read() should be fine.
>
> That's a shame, it looked so neat....
>
> Using twl4030_read forces i2c access and misses out on the regmap caching.
>
> What do you think of this?

That looks good, but either snd_soc_kcontrol_component() instead of 
snd_kcontrol_chip() or just snd_soc_read(codec, ...

>
> Thanks,
> NeilBrown
>
> diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
> index b6b0cb399599..bdb47a045aa5 100644
> --- a/sound/soc/codecs/twl4030.c
> +++ b/sound/soc/codecs/twl4030.c
> @@ -957,6 +957,18 @@ static int snd_soc_put_twl4030_opmode_enum_double(struct snd_kcontrol *kcontrol,
>   {
>   	struct snd_soc_codec *codec = snd_soc_kcontrol_codec(kcontrol);
>   	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
> +	const struct soc_enum *e = &twl4030_op_modes_enum;
> +	unsigned int reg_val;
> +	int ret;
> +
> +	ret = snd_soc_component_read(snd_kcontrol_chip(kcontrol),
> +				     e->reg, &reg_val);
> +	if (ret)
> +		return ret;
> +	if (ucontrol->value.enumerated.item[0] ==
> +	    ((reg_val >> e->shift_l) & e->mask))
> +		/* no change requested, so do nothing */
> +		return 0;
>
>   	if (twl4030->configured) {
>   		dev_err(codec->dev,
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html