[PATCH 00/11] MFD/ASoC/Input: twl4030-audio submodule DT support

Hello,

The following series adds DT support for the twl4030 audio submodule which
provides audio codec and vibra functionality.

The MFD core driver is probed via DT, it will create the needed child devices
based on the provided information in the DT blob.
Child drivers (vibra, ASoC codec) will parse the core's node if needed to get
the needed parameters for their configuration.

In the ASoC codec driver the hs_extmute callback (which was used to toggle a
GPIO line) has been removed. The codec driver will receive the GPIO number
(if it is needed on the platform) and also the level to disable the mute.

If the series is OK (and no objections from the maintainers), it would be good
if this can go via audio. Changed files are well contained within the
twl4030-audio stack so I do not expect merge issues later.

The series has been tested on BeagleBoard (with the McBSP DT series, and with
the upcoming DT audio support for BeagleBoard).

Regards,
Peter
---
Peter Ujfalusi (11):
  MFD: twl4030-audio: Clean up MODULE_* and platform_driver part
  MFD: twl4030-audio: Convert to use devm_kzalloc
  MFD: twl4030-audio: Rearange and clean-up the probe function
  MFD: twl4030-audio: Add DT support
  Input: twl4030-vibra: Support for DT booted kernel
  ASoC: twl4030: Move hs_extmute GPIO handling to driver
  ARM: OMAP/ASoC: Zoom2: Let the codec to handle the hs_extmute GPIO
  ASoC/MFD: twl4030: Remove set_hs_extmute callback from platform data
  ASoC: twl4030: Convert to use devm_kzalloc
  ASoC: twl4030: Add pointer to pdata within the private data
  ASoC: twl4030: Support for DT booted kernel

 .../devicetree/bindings/mfd/twl4030-audio.txt      |   51 +++++++++
 arch/arm/mach-omap2/board-zoom-peripherals.c       |    9 +-
 arch/arm/mach-omap2/include/mach/board-zoom.h      |    2 -
 drivers/input/misc/twl4030-vibra.c                 |   20 +++-
 drivers/mfd/twl4030-audio.c                        |  114 +++++++++++++------
 include/linux/i2c/twl.h                            |    4 +-
 sound/soc/codecs/twl4030.c                         |  117 ++++++++++++++++----
 sound/soc/omap/zoom2.c                             |    4 -
 8 files changed, 250 insertions(+), 71 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/twl4030-audio.txt

-- 
1.7.8.6

To facilitate the device tree support the probe function need to be rearanged.
Small cleanup in the APLL frequency selection part as well.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/mfd/twl4030-audio.c |   34 ++++++++++++++++------------------
 1 files changed, 16 insertions(+), 18 deletions(-)

diff --git a/drivers/mfd/twl4030-audio.c b/drivers/mfd/twl4030-audio.c
index efa2d42..ca2d669 100644
--- a/drivers/mfd/twl4030-audio.c
+++ b/drivers/mfd/twl4030-audio.c

@@ -169,35 +169,30 @@ static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
+	audio = devm_kzalloc(&pdev->dev, sizeof(struct twl4030_audio),
+			     GFP_KERNEL);
+	if (!audio)
+		return -ENOMEM;
+
+	mutex_init(&audio->mutex);
+	audio->audio_mclk = pdata->audio_mclk;
+
 	/* Configure APLL_INFREQ and disable APLL if enabled */
-	val = 0;
-	switch (pdata->audio_mclk) {
+	switch (audio->audio_mclk) {
 	case 19200000:
-		val |= TWL4030_APLL_INFREQ_19200KHZ;
+		val = TWL4030_APLL_INFREQ_19200KHZ;
 		break;
 	case 26000000:
-		val |= TWL4030_APLL_INFREQ_26000KHZ;
+		val = TWL4030_APLL_INFREQ_26000KHZ;
 		break;
 	case 38400000:
-		val |= TWL4030_APLL_INFREQ_38400KHZ;
+		val = TWL4030_APLL_INFREQ_38400KHZ;
 		break;
 	default:
 		dev_err(&pdev->dev, "Invalid audio_mclk\n");
 		return -EINVAL;
 	}
-	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
-					val, TWL4030_REG_APLL_CTL);
-
-	audio = devm_kzalloc(&pdev->dev, sizeof(struct twl4030_audio),
-			     GFP_KERNEL);
-	if (!audio)
-		return -ENOMEM;
-
-	platform_set_drvdata(pdev, audio);
-
-	twl4030_audio_dev = pdev;
-	mutex_init(&audio->mutex);
-	audio->audio_mclk = pdata->audio_mclk;
+	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE, val, TWL4030_REG_APLL_CTL);
 
 	/* Codec power */
 	audio->resource[TWL4030_AUDIO_RES_POWER].reg = TWL4030_REG_CODEC_MODE;

@@ -222,6 +217,9 @@ static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 		childs++;
 	}
 
+	platform_set_drvdata(pdev, audio);
+	twl4030_audio_dev = pdev;
+
 	if (childs)
 		ret = mfd_add_devices(&pdev->dev, pdev->id, audio->cells,
 				      childs, NULL, 0);

-- 
1.7.8.6

Support for loading the twl4030 audio module via devicetree.
Sub devices for codec and vibra will be created as mfd devices once the
core MFD driver is loaded when the kernel is booted with a DT blob.

Signed-off-by: Peter Ujfalusi <redacted>
---
 .../devicetree/bindings/mfd/twl4030-audio.txt      |   51 +++++++++++++++
 drivers/mfd/twl4030-audio.c                        |   65 +++++++++++++++++---
 2 files changed, 108 insertions(+), 8 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/mfd/twl4030-audio.txt

diff --git a/Documentation/devicetree/bindings/mfd/twl4030-audio.txt b/Documentation/devicetree/bindings/mfd/twl4030-audio.txt
new file mode 100644
index 0000000..bc45025
--- /dev/null
+++ b/Documentation/devicetree/bindings/mfd/twl4030-audio.txt

@@ -0,0 +1,51 @@
+Texas Instruments TWL family (twl4030) audio module
+
+The audio module inside the TWL family consist of an audio codec and a vibra
+driver.
+
+Required properties:
+- compatible : must be "ti,twl4030-audio"
+- ti,audio_mclk: Master clock frequency used on the board. Supported
+	         frequencies: 19200000, 26000000 and 38400000
+
+Optional properties, nodes:
+
+Audio functionality:
+- codec { }: Need to be present if the audio functionality is used. Within this
+	     section the following options can be used:
+- ti,digimic_delay: Delay need after enabling the digimic to reduce artifacts
+		    from the start of the recorded sample (in ms)
+-ti,ramp_delay_value: HS ramp delay configuration to reduce pop noise
+-ti,hs_extmute: Use external mute for HS pop reduction
+-ti,hs_extmute_gpio: Use external GPIO to control the external mute
+-ti,hs_extmute_disable_level: The desired level of the GPIO line when the
+			      external mute is disabled. valuse: 0 or 1
+-ti,offset_cncl_path: Offset cancellation path selection, refer to TRM for the
+		      valid values.
+
+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.
+
+Example:
+&i2c1 {
+	clock-frequency = <2600000>;
+
+	twl: twl at 48 {
+		reg = <0x48>;
+		interrupts = <7>; /* SYS_NIRQ cascaded to intc */
+		interrupt-parent = <&intc>;
+
+		twl_audio: audio {
+			compatible = "ti,twl4030-audio";
+			ti,audio_mclk = <26000000>;
+
+			ti,enable-vibra = <1>;
+
+			codec {
+				ti,ramp_delay_value = <3>;
+			};
+
+		};
+	};
+};

diff --git a/drivers/mfd/twl4030-audio.c b/drivers/mfd/twl4030-audio.c
index ca2d669..a4df8b3a 100644
--- a/drivers/mfd/twl4030-audio.c
+++ b/drivers/mfd/twl4030-audio.c

@@ -28,6 +28,8 @@
 #include <linux/kernel.h>
 #include <linux/fs.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/i2c/twl.h>
 #include <linux/mfd/core.h>
 #include <linux/mfd/twl4030-audio.h>

@@ -156,15 +158,46 @@ unsigned int twl4030_audio_get_mclk(void)
 }
 EXPORT_SYMBOL_GPL(twl4030_audio_get_mclk);
 
+static bool twl4030_audio_has_codec(struct twl4030_audio_data *pdata,
+			      struct device_node *node)
+{
+	if (pdata && pdata->codec)
+		return true;
+
+#ifdef CONFIG_OF
+	if (of_find_node_by_name(node, "codec"))
+		return true;
+#endif
+
+	return false;
+}
+
+static bool twl4030_audio_has_vibra(struct twl4030_audio_data *pdata,
+			      struct device_node *node)
+{
+	int vibra;
+
+	if (pdata && pdata->vibra)
+		return true;
+
+#ifdef CONFIG_OF
+	if (!of_property_read_u32(node, "ti,enable-vibra", &vibra) && vibra)
+		return true;
+#endif
+
+	return false;
+}
+
 static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 {
 	struct twl4030_audio *audio;
 	struct twl4030_audio_data *pdata = pdev->dev.platform_data;
+	struct device_node *node = pdev->dev.of_node;
 	struct mfd_cell *cell = NULL;
 	int ret, childs = 0;
 	u8 val;
 
-	if (!pdata) {
+	if (!pdata && !node) {
 		dev_err(&pdev->dev, "Platform data is missing\n");
 		return -EINVAL;
 	}

@@ -175,7 +208,12 @@ static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	mutex_init(&audio->mutex);
-	audio->audio_mclk = pdata->audio_mclk;
+	if (pdata)
+		audio->audio_mclk = pdata->audio_mclk;
+#ifdef CONFIG_OF
+	else
+		of_property_read_u32(node, "ti,audio_mclk", &audio->audio_mclk);
+#endif
 
 	/* Configure APLL_INFREQ and disable APLL if enabled */
 	switch (audio->audio_mclk) {

@@ -202,18 +240,22 @@ static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 	audio->resource[TWL4030_AUDIO_RES_APLL].reg = TWL4030_REG_APLL_CTL;
 	audio->resource[TWL4030_AUDIO_RES_APLL].mask = TWL4030_APLL_EN;
 
-	if (pdata->codec) {
+	if (twl4030_audio_has_codec(pdata, node)) {
 		cell = &audio->cells[childs];
 		cell->name = "twl4030-codec";
-		cell->platform_data = pdata->codec;
-		cell->pdata_size = sizeof(*pdata->codec);
+		if (pdata) {
+			cell->platform_data = pdata->codec;
+			cell->pdata_size = sizeof(*pdata->codec);
+		}
 		childs++;
 	}
-	if (pdata->vibra) {
+	if (twl4030_audio_has_vibra(pdata, node)) {
 		cell = &audio->cells[childs];
 		cell->name = "twl4030-vibra";
-		cell->platform_data = pdata->vibra;
-		cell->pdata_size = sizeof(*pdata->vibra);
+		if (pdata) {
+			cell->platform_data = pdata->vibra;
+			cell->pdata_size = sizeof(*pdata->vibra);
+		}
 		childs++;
 	}
 

@@ -245,10 +287,17 @@ static int __devexit twl4030_audio_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct of_device_id twl4030_audio_of_match[] = {
+	{.compatible = "ti,twl4030-audio", },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, twl4030_audio_of_match);
+
 static struct platform_driver twl4030_audio_driver = {
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= "twl4030-audio",
+		.of_match_table = twl4030_audio_of_match,
 	},
 	.probe		= twl4030_audio_probe,
 	.remove		= __devexit_p(twl4030_audio_remove),

-- 
1.7.8.6

The external mute (if it is in use) is handled by a GPIO line. Prepare to
remove the set_hs_extmute callback and replace it with two sets of platform
data:
hs_extmute_gpio: the GPIO number to use for external mute
hs_extmute_disable_level: the GPIO level to use when the external mute is
not enabled.

The codec driver can handle the external mute functionality based on these
parameters and when the users of set_hs_extmute has been converted the
callback can be removed.

Signed-off-by: Peter Ujfalusi <redacted>
---
 include/linux/i2c/twl.h    |    6 +++++-
 sound/soc/codecs/twl4030.c |   36 ++++++++++++++++++++++++++++++++++--
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 7ea898c..75ce6d1 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h

@@ -666,7 +666,11 @@ struct twl4030_codec_data {
 	unsigned int check_defaults:1;
 	unsigned int reset_registers:1;
 	unsigned int hs_extmute:1;
-	void (*set_hs_extmute)(int mute);
+	void (*set_hs_extmute)(int mute); /* Deprecated, use hs_extmute_gpio and
+					     hs_extmute_disable_level */
+	int hs_extmute_gpio;
+	unsigned int hs_extmute_disable_level:1; /* the GPIO state to disable
+						    the extmute */
 };
 
 struct twl4030_vibra_data {

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 391fcfc..0630040 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c

@@ -28,6 +28,7 @@
 #include <linux/platform_device.h>
 #include <linux/i2c/twl.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
 #include <sound/core.h>
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>

@@ -302,6 +303,24 @@ static void twl4030_init_chip(struct snd_soc_codec *codec)
 	u8 reg, byte;
 	int i = 0;
 
+	if (pdata && pdata->hs_extmute &&
+	    gpio_is_valid(pdata->hs_extmute_gpio)) {
+		int ret, level;
+
+		if (!pdata->hs_extmute_gpio)
+			dev_warn(codec->dev,
+				 "Extmute GPIO is 0 is this correct?\n");
+
+		level = pdata->hs_extmute_disable_level;
+		ret = gpio_request_one(pdata->hs_extmute_gpio, level ?
+				       GPIOF_OUT_INIT_HIGH : GPIOF_OUT_INIT_LOW,
+				       "hs_extmute");
+		if (ret) {
+			dev_err(codec->dev, "Failed to get hs_extmute GPIO\n");
+			pdata->hs_extmute_gpio = -1;
+		}
+	}
+
 	/* Check defaults, if instructed before anything else */
 	if (pdata && pdata->check_defaults)
 		twl4030_check_defaults(codec);

@@ -748,7 +767,11 @@ static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 	/* Enable external mute control, this dramatically reduces
 	 * the pop-noise */
 	if (pdata && pdata->hs_extmute) {
-		if (pdata->set_hs_extmute) {
+		if (gpio_is_valid(pdata->hs_extmute_gpio)) {
+			gpio_set_value(pdata->hs_extmute_gpio,
+				       !pdata->hs_extmute_disable_level);
+		} else if (pdata->set_hs_extmute) {
+			dev_warn(codec->dev, "set_hs_extmute is deprecated\n");
 			pdata->set_hs_extmute(1);
 		} else {
 			hs_pop |= TWL4030_EXTMUTE;

@@ -786,7 +809,11 @@ static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 
 	/* Disable external mute */
 	if (pdata && pdata->hs_extmute) {
-		if (pdata->set_hs_extmute) {
+		if (gpio_is_valid(pdata->hs_extmute_gpio)) {
+			gpio_set_value(pdata->hs_extmute_gpio,
+				       pdata->hs_extmute_disable_level);
+		} else if (pdata->set_hs_extmute) {
+			dev_warn(codec->dev, "set_hs_extmute is deprecated\n");
 			pdata->set_hs_extmute(0);
 		} else {
 			hs_pop &= ~TWL4030_EXTMUTE;

@@ -2230,12 +2257,17 @@ static int twl4030_soc_probe(struct snd_soc_codec *codec)
 
 static int twl4030_soc_remove(struct snd_soc_codec *codec)
 {
+	struct twl4030_codec_data *pdata = dev_get_platdata(codec->dev);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
 
 	/* Reset registers to their chip default before leaving */
 	twl4030_reset_registers(codec);
 	twl4030_set_bias_level(codec, SND_SOC_BIAS_OFF);
 	kfree(twl4030);
+
+	if (pdata && pdata->hs_extmute && gpio_is_valid(pdata->hs_extmute_gpio))
+		gpio_free(pdata->hs_extmute_gpio);
+
 	return 0;
 }
 

-- 
1.7.8.6

Allocate the private data with devm_kzalloc.

Signed-off-by: Peter Ujfalusi <redacted>
---
 sound/soc/codecs/twl4030.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 1247c4a..6a00c01 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c

@@ -2235,7 +2235,8 @@ static int twl4030_soc_probe(struct snd_soc_codec *codec)
 {
 	struct twl4030_priv *twl4030;
 
-	twl4030 = kzalloc(sizeof(struct twl4030_priv), GFP_KERNEL);
+	twl4030 = devm_kzalloc(codec->dev, sizeof(struct twl4030_priv),
+			       GFP_KERNEL);
 	if (twl4030 == NULL) {
 		dev_err(codec->dev, "Can not allocate memory\n");
 		return -ENOMEM;

@@ -2257,7 +2258,6 @@ static int twl4030_soc_remove(struct snd_soc_codec *codec)
 	/* Reset registers to their chip default before leaving */
 	twl4030_reset_registers(codec);
 	twl4030_set_bias_level(codec, SND_SOC_BIAS_OFF);
-	kfree(twl4030);
 
 	if (pdata && pdata->hs_extmute && gpio_is_valid(pdata->hs_extmute_gpio))
 		gpio_free(pdata->hs_extmute_gpio);

-- 
1.7.8.6

When the kernel has been booted with DT blob the platform data is NULL for
the driver.
We need to construct the pdata based on the DT information for runtime use.

Signed-off-by: Peter Ujfalusi <redacted>
---
 sound/soc/codecs/twl4030.c |   67 ++++++++++++++++++++++++++++++++++++++-----
 1 files changed, 59 insertions(+), 8 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index bf0240d..68495a2 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c

@@ -26,6 +26,7 @@
 #include <linux/pm.h>
 #include <linux/i2c.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include <linux/i2c/twl.h>
 #include <linux/slab.h>
 #include <linux/gpio.h>

@@ -295,13 +296,70 @@ static inline void twl4030_reset_registers(struct snd_soc_codec *codec)
 
 }
 
-static void twl4030_init_chip(struct snd_soc_codec *codec)
+static void twl4030_setup_pdata_of(struct twl4030_codec_data *pdata,
+				   struct device_node *node)
+{
+	int value;
+
+	of_property_read_u32(node, "ti,digimic_delay",
+			     &pdata->digimic_delay);
+	of_property_read_u32(node, "ti,ramp_delay_value",
+			     &pdata->ramp_delay_value);
+	of_property_read_u32(node, "ti,offset_cncl_path",
+			     &pdata->offset_cncl_path);
+	if (!of_property_read_u32(node, "ti,hs_extmute", &value))
+		pdata->hs_extmute = value;
+	else
+		pdata->hs_extmute = 0;
+
+	if (pdata->hs_extmute) {
+		int ret = of_property_read_u32(node, "ti,hs_extmute_gpio",
+					       &pdata->hs_extmute_gpio);
+		if (!ret) {
+			if (!of_property_read_u32(node,
+						  "ti,hs_extmute_disable_level",
+						  &value)) {
+				pdata->hs_extmute_disable_level = value;
+			}
+		} else {
+			pdata->hs_extmute_gpio = -1;
+		}
+	}
+}
+
+static struct twl4030_codec_data *twl4030_get_pdata(struct snd_soc_codec *codec)
 {
 	struct twl4030_codec_data *pdata = dev_get_platdata(codec->dev);
+	struct device_node *twl4030_codec_node = NULL;
+
+#ifdef CONFIG_OF
+	twl4030_codec_node = of_find_node_by_name(codec->dev->parent->of_node,
+						  "codec");
+#endif
+
+	if (!pdata && twl4030_codec_node) {
+		pdata = devm_kzalloc(codec->dev,
+				     sizeof(struct twl4030_codec_data),
+				     GFP_KERNEL);
+		if (!pdata) {
+			dev_err(codec->dev, "Can not allocate memory\n");
+			return NULL;
+		}
+		twl4030_setup_pdata_of(pdata, twl4030_codec_node);
+	}
+
+	return pdata;
+}
+
+static void twl4030_init_chip(struct snd_soc_codec *codec)
+{
+	struct twl4030_codec_data *pdata;
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
 	u8 reg, byte;
 	int i = 0;
 
+	pdata = twl4030_get_pdata(codec);
+
 	if (pdata && pdata->hs_extmute &&
 	    gpio_is_valid(pdata->hs_extmute_gpio)) {
 		int ret, level;

@@ -2288,13 +2346,6 @@ static struct snd_soc_codec_driver soc_codec_dev_twl4030 = {
 
 static int __devinit twl4030_codec_probe(struct platform_device *pdev)
 {
-	struct twl4030_codec_data *pdata = pdev->dev.platform_data;
-
-	if (!pdata) {
-		dev_err(&pdev->dev, "platform_data is missing\n");
-		return -EINVAL;
-	}
-
 	return snd_soc_register_codec(&pdev->dev, &soc_codec_dev_twl4030,
 			twl4030_dai, ARRAY_SIZE(twl4030_dai));
 }

-- 
1.7.8.6

Access the pdata via a pointer within the twl4030_priv structure.
In preparation for DeviceTree support.

Signed-off-by: Peter Ujfalusi <redacted>
---
 sound/soc/codecs/twl4030.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 6a00c01..bf0240d 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c

@@ -153,8 +153,7 @@ struct twl4030_priv {
 	u8 predrivel_enabled, predriver_enabled;
 	u8 carkitl_enabled, carkitr_enabled;
 
-	/* Delay needed after enabling the digimic interface */
-	unsigned int digimic_delay;
+	struct twl4030_codec_data *pdata;
 };
 
 /*

@@ -350,7 +349,7 @@ static void twl4030_init_chip(struct snd_soc_codec *codec)
 	if (!pdata)
 		return;
 
-	twl4030->digimic_delay = pdata->digimic_delay;
+	twl4030->pdata = pdata;
 
 	reg = twl4030_read_reg_cache(codec, TWL4030_REG_HS_POPN_SET);
 	reg &= ~TWL4030_RAMP_DELAY;

@@ -751,9 +750,9 @@ static int aif_event(struct snd_soc_dapm_widget *w,
 
 static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 {
-	struct twl4030_codec_data *pdata = codec->dev->platform_data;
 	unsigned char hs_gain, hs_pop;
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	struct twl4030_codec_data *pdata = twl4030->pdata;
 	/* Base values for ramp delay calculation: 2^19 - 2^26 */
 	unsigned int ramp_base[] = {524288, 1048576, 2097152, 4194304,
 				    8388608, 16777216, 33554432, 67108864};

@@ -868,9 +867,10 @@ static int digimic_event(struct snd_soc_dapm_widget *w,
 		struct snd_kcontrol *kcontrol, int event)
 {
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(w->codec);
+	struct twl4030_codec_data *pdata = twl4030->pdata;
 
-	if (twl4030->digimic_delay)
-		twl4030_wait_ms(twl4030->digimic_delay);
+	if (pdata && pdata->digimic_delay)
+		twl4030_wait_ms(pdata->digimic_delay);
 	return 0;
 }
 

@@ -2252,8 +2252,8 @@ static int twl4030_soc_probe(struct snd_soc_codec *codec)
 
 static int twl4030_soc_remove(struct snd_soc_codec *codec)
 {
-	struct twl4030_codec_data *pdata = dev_get_platdata(codec->dev);
 	struct twl4030_priv *twl4030 = snd_soc_codec_get_drvdata(codec);
+	struct twl4030_codec_data *pdata = twl4030->pdata;
 
 	/* Reset registers to their chip default before leaving */
 	twl4030_reset_registers(codec);

-- 
1.7.8.6

We no longer have users for the set_hs_extmute callback which has been
replaced by hs_extmute_gpio/hs_extmute_disable_level parameters so the
codec driver can handle the external mute if it is needed by the board.

Signed-off-by: Peter Ujfalusi <redacted>
---
 include/linux/i2c/twl.h    |    2 --
 sound/soc/codecs/twl4030.c |    6 ------
 2 files changed, 0 insertions(+), 8 deletions(-)

diff --git a/include/linux/i2c/twl.h b/include/linux/i2c/twl.h
index 75ce6d1..c9c1fd2 100644
--- a/include/linux/i2c/twl.h
+++ b/include/linux/i2c/twl.h

@@ -666,8 +666,6 @@ struct twl4030_codec_data {
 	unsigned int check_defaults:1;
 	unsigned int reset_registers:1;
 	unsigned int hs_extmute:1;
-	void (*set_hs_extmute)(int mute); /* Deprecated, use hs_extmute_gpio and
-					     hs_extmute_disable_level */
 	int hs_extmute_gpio;
 	unsigned int hs_extmute_disable_level:1; /* the GPIO state to disable
 						    the extmute */

diff --git a/sound/soc/codecs/twl4030.c b/sound/soc/codecs/twl4030.c
index 0630040..1247c4a 100644
--- a/sound/soc/codecs/twl4030.c
+++ b/sound/soc/codecs/twl4030.c

@@ -770,9 +770,6 @@ static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 		if (gpio_is_valid(pdata->hs_extmute_gpio)) {
 			gpio_set_value(pdata->hs_extmute_gpio,
 				       !pdata->hs_extmute_disable_level);
-		} else if (pdata->set_hs_extmute) {
-			dev_warn(codec->dev, "set_hs_extmute is deprecated\n");
-			pdata->set_hs_extmute(1);
 		} else {
 			hs_pop |= TWL4030_EXTMUTE;
 			twl4030_write(codec, TWL4030_REG_HS_POPN_SET, hs_pop);

@@ -812,9 +809,6 @@ static void headset_ramp(struct snd_soc_codec *codec, int ramp)
 		if (gpio_is_valid(pdata->hs_extmute_gpio)) {
 			gpio_set_value(pdata->hs_extmute_gpio,
 				       pdata->hs_extmute_disable_level);
-		} else if (pdata->set_hs_extmute) {
-			dev_warn(codec->dev, "set_hs_extmute is deprecated\n");
-			pdata->set_hs_extmute(0);
 		} else {
 			hs_pop &= ~TWL4030_EXTMUTE;
 			twl4030_write(codec, TWL4030_REG_HS_POPN_SET, hs_pop);

-- 
1.7.8.6

Remove the use of set_hs_extmute callback and let the codec driver to
handle the extmute GPIO.

Signed-off-by: Peter Ujfalusi <redacted>
---
 arch/arm/mach-omap2/board-zoom-peripherals.c  |    9 ++-------
 arch/arm/mach-omap2/include/mach/board-zoom.h |    2 --
 sound/soc/omap/zoom2.c                        |    4 ----
 3 files changed, 2 insertions(+), 13 deletions(-)

diff --git a/arch/arm/mach-omap2/board-zoom-peripherals.c b/arch/arm/mach-omap2/board-zoom-peripherals.c
index b797cb2..a7d3b04 100644
--- a/arch/arm/mach-omap2/board-zoom-peripherals.c
+++ b/arch/arm/mach-omap2/board-zoom-peripherals.c

@@ -34,6 +34,7 @@
 #include "common-board-devices.h"
 
 #define OMAP_ZOOM_WLAN_PMENA_GPIO	(101)
+#define ZOOM2_HEADSET_EXTMUTE_GPIO	(153)
 #define OMAP_ZOOM_WLAN_IRQ_GPIO		(162)
 
 #define LCD_PANEL_ENABLE_GPIO		(7 + OMAP_MAX_GPIO_LINES)

@@ -244,12 +245,6 @@ static int zoom_twl_gpio_setup(struct device *dev,
 	return ret;
 }
 
-/* EXTMUTE callback function */
-static void zoom2_set_hs_extmute(int mute)
-{
-	gpio_set_value(ZOOM2_HEADSET_EXTMUTE_GPIO, mute);
-}
-
 static struct twl4030_gpio_platform_data zoom_gpio_data = {
 	.gpio_base	= OMAP_MAX_GPIO_LINES,
 	.irq_base	= TWL4030_GPIO_IRQ_BASE,

@@ -279,7 +274,7 @@ static int __init omap_i2c_init(void)
 
 		codec_data->ramp_delay_value = 3;	/* 161 ms */
 		codec_data->hs_extmute = 1;
-		codec_data->set_hs_extmute = zoom2_set_hs_extmute;
+		codec_data->hs_extmute_gpio = ZOOM2_HEADSET_EXTMUTE_GPIO;
 	}
 	omap_pmic_init(1, 2400, "twl5030", INT_34XX_SYS_NIRQ, &zoom_twldata);
 	omap_register_i2c_bus(2, 400, NULL, 0);

diff --git a/arch/arm/mach-omap2/include/mach/board-zoom.h b/arch/arm/mach-omap2/include/mach/board-zoom.h
index 775fdc3..2e94869 100644
--- a/arch/arm/mach-omap2/include/mach/board-zoom.h
+++ b/arch/arm/mach-omap2/include/mach/board-zoom.h

@@ -8,5 +8,3 @@
 extern int __init zoom_debugboard_init(void);
 extern void __init zoom_peripherals_init(void);
 extern void __init zoom_display_init(void);
-
-#define ZOOM2_HEADSET_EXTMUTE_GPIO	153

diff --git a/sound/soc/omap/zoom2.c b/sound/soc/omap/zoom2.c
index 920e0d9..df97a41 100644
--- a/sound/soc/omap/zoom2.c
+++ b/sound/soc/omap/zoom2.c

@@ -191,9 +191,6 @@ static int __init zoom2_soc_init(void)
 	BUG_ON(gpio_request(ZOOM2_HEADSET_MUX_GPIO, "hs_mux") < 0);
 	gpio_direction_output(ZOOM2_HEADSET_MUX_GPIO, 0);
 
-	BUG_ON(gpio_request(ZOOM2_HEADSET_EXTMUTE_GPIO, "ext_mute") < 0);
-	gpio_direction_output(ZOOM2_HEADSET_EXTMUTE_GPIO, 0);
-
 	return 0;
 
 err1:

@@ -207,7 +204,6 @@ module_init(zoom2_soc_init);
 static void __exit zoom2_soc_exit(void)
 {
 	gpio_free(ZOOM2_HEADSET_MUX_GPIO);
-	gpio_free(ZOOM2_HEADSET_EXTMUTE_GPIO);
 
 	platform_device_unregister(zoom2_snd_device);
 }

-- 
1.7.8.6

Add support when the kernel has been booted with DT blob. In this case the
pdata is NULL, we need to reach up to the core node and check if the codec
part has been enabled to determine if we need to coexist with the codec or
not.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/input/misc/twl4030-vibra.c |   20 ++++++++++++++++++--
 1 files changed, 18 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/twl4030-vibra.c b/drivers/input/misc/twl4030-vibra.c
index d1e1a7e..2cfc6c7 100644
--- a/drivers/input/misc/twl4030-vibra.c
+++ b/drivers/input/misc/twl4030-vibra.c

@@ -26,6 +26,7 @@
 #include <linux/module.h>
 #include <linux/jiffies.h>
 #include <linux/platform_device.h>
+#include <linux/of.h>
 #include <linux/workqueue.h>
 #include <linux/i2c/twl.h>
 #include <linux/mfd/twl4030-audio.h>

@@ -194,13 +195,28 @@ static int twl4030_vibra_resume(struct device *dev)
 static SIMPLE_DEV_PM_OPS(twl4030_vibra_pm_ops,
 			 twl4030_vibra_suspend, twl4030_vibra_resume);
 
+static bool twl4030_vibra_check_coexist(struct twl4030_vibra_data *pdata,
+			      struct device_node *node)
+{
+	if (pdata && pdata->coexist)
+		return true;
+
+#ifdef CONFIG_OF
+	if (of_find_node_by_name(node, "codec"))
+		return true;
+#endif
+
+	return false;
+}
+
 static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
 {
 	struct twl4030_vibra_data *pdata = pdev->dev.platform_data;
+	struct device_node *twl4030_core_node = pdev->dev.parent->of_node;
 	struct vibra_info *info;
 	int ret;
 
-	if (!pdata) {
+	if (!pdata && !twl4030_core_node) {
 		dev_dbg(&pdev->dev, "platform_data not available\n");
 		return -EINVAL;
 	}

@@ -210,7 +226,7 @@ static int __devinit twl4030_vibra_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	info->dev = &pdev->dev;
-	info->coexist = pdata->coexist;
+	info->coexist = twl4030_vibra_check_coexist(pdata, twl4030_core_node);
 	INIT_WORK(&info->play_work, vibra_play_work);
 
 	info->input_dev = input_allocate_device();

-- 
1.7.8.6

Place the MODULE_* lines in the same block and add MODULE_DESCRIPTION.
Rearange the platform_driver structure at the same time.

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/mfd/twl4030-audio.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/mfd/twl4030-audio.c b/drivers/mfd/twl4030-audio.c
index 838ce4e..ac04b4f 100644
--- a/drivers/mfd/twl4030-audio.c
+++ b/drivers/mfd/twl4030-audio.c

@@ -250,18 +250,18 @@ static int __devexit twl4030_audio_remove(struct platform_device *pdev)
 	return 0;
 }
 
-MODULE_ALIAS("platform:twl4030-audio");
-
 static struct platform_driver twl4030_audio_driver = {
-	.probe		= twl4030_audio_probe,
-	.remove		= __devexit_p(twl4030_audio_remove),
 	.driver		= {
 		.owner	= THIS_MODULE,
 		.name	= "twl4030-audio",
 	},
+	.probe		= twl4030_audio_probe,
+	.remove		= __devexit_p(twl4030_audio_remove),
 };
 
 module_platform_driver(twl4030_audio_driver);
 
 MODULE_AUTHOR("Peter Ujfalusi <peter.ujfalusi@ti.com>");
+MODULE_DESCRIPTION("TWL4030 audio block MFD driver");
 MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:twl4030-audio");

-- 
1.7.8.6

Signed-off-by: Peter Ujfalusi <redacted>
---
 drivers/mfd/twl4030-audio.c |   15 ++++++---------
 1 files changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/twl4030-audio.c b/drivers/mfd/twl4030-audio.c
index ac04b4f..efa2d42 100644
--- a/drivers/mfd/twl4030-audio.c
+++ b/drivers/mfd/twl4030-audio.c

@@ -188,7 +188,8 @@ static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 	twl_i2c_write_u8(TWL4030_MODULE_AUDIO_VOICE,
 					val, TWL4030_REG_APLL_CTL);
 
-	audio = kzalloc(sizeof(struct twl4030_audio), GFP_KERNEL);
+	audio = devm_kzalloc(&pdev->dev, sizeof(struct twl4030_audio),
+			     GFP_KERNEL);
 	if (!audio)
 		return -ENOMEM;
 

@@ -229,22 +230,18 @@ static int __devinit twl4030_audio_probe(struct platform_device *pdev)
 		ret = -ENODEV;
 	}
 
-	if (!ret)
-		return 0;
+	if (ret) {
+		platform_set_drvdata(pdev, NULL);
+		twl4030_audio_dev = NULL;
+	}
 
-	platform_set_drvdata(pdev, NULL);
-	kfree(audio);
-	twl4030_audio_dev = NULL;
 	return ret;
 }
 
 static int __devexit twl4030_audio_remove(struct platform_device *pdev)
 {
-	struct twl4030_audio *audio = platform_get_drvdata(pdev);
-
 	mfd_remove_devices(&pdev->dev);
 	platform_set_drvdata(pdev, NULL);
-	kfree(audio);
 	twl4030_audio_dev = NULL;
 
 	return 0;

-- 
1.7.8.6

Hi Peter,

On 08/08/2012 11:41 AM, Peter Ujfalusi wrote:

Since you care about the frequency only you can use the generic
attribute: clock-frequency (in your example, I2C is using it).


That being said, since we do have the clock binding now, maybe we should
create a clock provider at board level for the sys_clk and thus you will
just have to create a phandle to it in your audio description.

In theory OMAP and other components will thus have to use that sys_clock
reference as well.

Regards,
Benoit

Hi Benoit,

On 08/08/2012 02:50 PM, Benoit Cousson wrote:

quoted

+Required properties:
+- compatible : must be "ti,twl4030-audio"
+- ti,audio_mclk: Master clock frequency used on the board. Supported
+	         frequencies: 19200000, 26000000 and 38400000

Since you care about the frequency only you can use the generic
attribute: clock-frequency (in your example, I2C is using it).

I see, I'll use replace the property with clock-frequency.

That being said, since we do have the clock binding now, maybe we should
create a clock provider at board level for the sys_clk and thus you will
just have to create a phandle to it in your audio description.

In theory OMAP and other components will thus have to use that sys_clock
reference as well.

And in the driver I would need to do for example:
clkp = clk_get(NULL, "audio_mclk");
clk_rate = clk_get_rate(clkp);
clk_put(clkp);

to get the frequency in this way?
Note that the twl4030-audio stack itself does not handle clocks at the moment
since the clock needed for the audio module is always running (HFCLKIN ->
HFCLKOUT -> AUDIO_MCLK (USB_MCLK)). All of this is within twl4030. Do we
really want to model this?

-- 
P?ter

On Wed, Aug 08, 2012 at 01:50:53PM +0200, Benoit Cousson wrote:

That being said, since we do have the clock binding now, maybe we should
create a clock provider at board level for the sys_clk and thus you will
just have to create a phandle to it in your audio description.

Yes, we should be doing this.

On Wed, Aug 08, 2012 at 12:41:20PM +0300, Peter Ujfalusi wrote:

+Required properties:
+- compatible : must be "ti,twl4030-audio"

So, as I mentioned before I find this sort of direct mapping of the
Linux device representation into the device tree really troubling.
I'm just way too aware of the fact that even the Linux split of these
things can change over time and often represents internal Linux
considerations.

+-ti,hs_extmute: Use external mute for HS pop reduction
+-ti,hs_extmute_gpio: Use external GPIO to control the external mute
+-ti,hs_extmute_disable_level: The desired level of the GPIO line when the
+			      external mute is disabled. valuse: 0 or 1

This doesn't seem like something that should be in the CODEC driver
really, there's a general need for something which can unmute controls
at the end of the power up sequence and mute before power down.  Also,
if this is going to be part of the binding shouldn't we just omit the
first property and simply check for the presence of the property which
specifies the GPIO?

+#ifdef CONFIG_OF
+	if (of_find_node_by_name(node, "codec"))
+		return true;
+#endif

This really seems like we should be stubbing out of_find_node_by_name()
to return false in non-OF cases.

+#ifdef CONFIG_OF
+	if (!of_property_read_u32(node, "ti,enable-vibra", &vibra) && vibra)
+		return true;
+#endif

Similarly here.

On 08/08/2012 04:13 PM, Mark Brown wrote:

On Wed, Aug 08, 2012 at 12:41:20PM +0300, Peter Ujfalusi wrote:

quoted

+Required properties:
+- compatible : must be "ti,twl4030-audio"

So, as I mentioned before I find this sort of direct mapping of the
Linux device representation into the device tree really troubling.
I'm just way too aware of the fact that even the Linux split of these
things can change over time and often represents internal Linux
considerations.

Yes, I remember. This is however a bit different case compared to the twl6040.
The twl4030 series of PMIC includes the following parts:
twl4030, twl5030, twl5031, tps65950, tps65930 - contains audio block
tps65920, tps65921 - does not contain audio block

So the audio block within these PMICs are really a block, it is present on
some PMIC and it is missing on other's.

This is what we discussed regarding to twl6040. I only expose the audio block
and not the Linux implementation (that we have separate driver for vibra and
codec sub functionality).

quoted

+-ti,hs_extmute: Use external mute for HS pop reduction
+-ti,hs_extmute_gpio: Use external GPIO to control the external mute
+-ti,hs_extmute_disable_level: The desired level of the GPIO line when the
+			      external mute is disabled. valuse: 0 or 1

This doesn't seem like something that should be in the CODEC driver
really, there's a general need for something which can unmute controls
at the end of the power up sequence and mute before power down.  Also,
if this is going to be part of the binding shouldn't we just omit the
first property and simply check for the presence of the property which
specifies the GPIO?

The hs_extmute is used for reducing pop noise on the headset. The sequence is
described in the TRM and it need to be done within the codec driver since it
is within the sequence.
Now there's two ways for boards to implement it:
using the dedicated pin from twl4030 PMIC (MUTE/GPIO6) - if only
"ti,hs_extmute" present we use this mode.
Some boards are using the GPIO6 from twl4030 for other purposes, in this case
an external GPIO is used for the same purpose - thus we have
ti,hs_extmute_gpio and ti,hs_extmute_disable_level to tell the driver this.
As for the ti,hs_extmute_disable_level: some boards chosen that the mute is
disabled when the signal is high on the GPIO (don't ask me why). This property
tells this.

All of this has to be in the driver due to the sequencing requirements.

quoted

+#ifdef CONFIG_OF
+	if (of_find_node_by_name(node, "codec"))
+		return true;
+#endif

This really seems like we should be stubbing out of_find_node_by_name()
to return false in non-OF cases.

Yes, most likely the of_find_node_by_name() deserves the same treatment as
some selected of_* function in case CONFIG_OF is not selected.
But at the moment this is not the case, we need to protect with ifdef since we
might break other randconfigs where CONFIG_OF is not set.

quoted

+#ifdef CONFIG_OF
+	if (!of_property_read_u32(node, "ti,enable-vibra", &vibra) && vibra)
+		return true;
+#endif

Similarly here.

Here the ifdef is no needed. of_property_read_u32() return -ENOSYS in case
CONFIG_OF is not selected.

-- 
P?ter

On Wed, Aug 08, 2012 at 04:43:16PM +0300, Peter Ujfalusi wrote:

On 08/08/2012 04:13 PM, Mark Brown wrote:

quoted

This doesn't seem like something that should be in the CODEC driver
really, there's a general need for something which can unmute controls
at the end of the power up sequence and mute before power down.  Also,
if this is going to be part of the binding shouldn't we just omit the
first property and simply check for the presence of the property which
specifies the GPIO?

The hs_extmute is used for reducing pop noise on the headset. The sequence is
described in the TRM and it need to be done within the codec driver since it
is within the sequence.

Is the point it's at in the sequence the end?

quoted

quoted

+#ifdef CONFIG_OF
+	if (of_find_node_by_name(node, "codec"))
+		return true;
+#endif

quoted

This really seems like we should be stubbing out of_find_node_by_name()
to return false in non-OF cases.

Yes, most likely the of_find_node_by_name() deserves the same treatment as
some selected of_* function in case CONFIG_OF is not selected.
But at the moment this is not the case, we need to protect with ifdef since we
might break other randconfigs where CONFIG_OF is not set.

We can always add a patch stubbing it out as part of the series, or in a
branch in the OF tree that we can pull in.

quoted

quoted

+#ifdef CONFIG_OF
+	if (!of_property_read_u32(node, "ti,enable-vibra", &vibra) && vibra)
+		return true;
+#endif

Similarly here.

Here the ifdef is no needed. of_property_read_u32() return -ENOSYS in case
CONFIG_OF is not selected.

-- 
P?ter

On 08/08/2012 04:52 PM, Mark Brown wrote:

quoted

The hs_extmute is used for reducing pop noise on the headset. The sequence is
described in the TRM and it need to be done within the codec driver since it
is within the sequence.

Is the point it's at in the sequence the end?

On both ends :o
On HS enable: the extmute has to be enabled, run the power on sequence for the
HS, wait for ramp, disable the HS extmute.

On stop: enable the extmute, run the sequence, wait for ramp, release the extmute.

We can always add a patch stubbing it out as part of the series, or in a
branch in the OF tree that we can pull in.

If you are OK with the hassle I can add a patch to the series for
include/linux/of.h to handle this.

-- 
P?ter

On Wed, Aug 08, 2012 at 05:16:58PM +0300, Peter Ujfalusi wrote:

On 08/08/2012 04:52 PM, Mark Brown wrote:

quoted

Is the point it's at in the sequence the end?

On both ends :o
On HS enable: the extmute has to be enabled, run the power on sequence for the
HS, wait for ramp, disable the HS extmute.

On stop: enable the extmute, run the sequence, wait for ramp, release the extmute.

I'm struggling to see any reason not to just hold the external mute on
for the entire time the headset is powered off?

quoted

We can always add a patch stubbing it out as part of the series, or in a
branch in the OF tree that we can pull in.

If you are OK with the hassle I can add a patch to the series for
include/linux/of.h to handle this.

Yes, please.

On 08/08/2012 05:18 PM, Mark Brown wrote:

I'm struggling to see any reason not to just hold the external mute on
for the entire time the headset is powered off?

I think because it would mean in most cases that we keep a GPIO line enabled
during the time the HS is not in use, which burns power.
In most cases the mute is enabled when the GPIO line is up.

quoted

If you are OK with the hassle I can add a patch to the series for
include/linux/of.h to handle this.

Yes, please.

I'll wait for more comments and I'll include it to v2.

-- 
P?ter

On 08/08/2012 03:52 PM, Mark Brown wrote:

On Wed, Aug 08, 2012 at 01:50:53PM +0200, Benoit Cousson wrote:

quoted

That being said, since we do have the clock binding now, maybe we should
create a clock provider at board level for the sys_clk and thus you will
just have to create a phandle to it in your audio description.

Yes, we should be doing this.

Does this needs the common clock framework on the platform? OMAP3 certainly
does not have it enabled since it has not been adopted.

-- 
P?ter

On Wed, Aug 08, 2012 at 05:35:34PM +0300, Peter Ujfalusi wrote:

Does this needs the common clock framework on the platform? OMAP3 certainly
does not have it enabled since it has not been adopted.

I'd hope at least the binding code for finding clocks has been done at
the clkdev level so it can be used on other platforms but perhaps that's
not the case.

On Wed, Aug 08, 2012 at 05:31:19PM +0300, Peter Ujfalusi wrote:

On 08/08/2012 05:18 PM, Mark Brown wrote:

quoted

I'm struggling to see any reason not to just hold the external mute on
for the entire time the headset is powered off?

I think because it would mean in most cases that we keep a GPIO line enabled
during the time the HS is not in use, which burns power.
In most cases the mute is enabled when the GPIO line is up.

That makes sense if the GPIO is actively driven, open drain should be
better here, but it's still a generic thing which it'd be nice to
extract.

Still, the main thing here was the fact that we should be able to drop
the flag for the feature in the first place.

On 08/08/2012 04:35 PM, Peter Ujfalusi wrote:

On 08/08/2012 03:52 PM, Mark Brown wrote:

quoted

On Wed, Aug 08, 2012 at 01:50:53PM +0200, Benoit Cousson wrote:

quoted

That being said, since we do have the clock binding now, maybe we should
create a clock provider at board level for the sys_clk and thus you will
just have to create a phandle to it in your audio description.

Yes, we should be doing this.

Does this needs the common clock framework on the platform? OMAP3 certainly
does not have it enabled since it has not been adopted.

Yeah, good point, we are still not there yet. Rajendra is still working
on it and I hope we can get something for 3.7, but let's avoid having a
strong dependency on that migration for the moment.

Regards,
Benoit

On 08/08/2012 05:49 PM, Mark Brown wrote:

That makes sense if the GPIO is actively driven, open drain should be
better here, but it's still a generic thing which it'd be nice to
extract.

Yes, the idea in it's core is generic, but right now I can not think of a
generic implementation which would work.
In case of twl4030 this is how it looks like:

1. if extmute is requested
 1.1. if GPIO number is not valid
   Use the internal MUTE functionality (HS_POPN_SET register, EXTMUTE bit)
 1.2. if GPIO number is valid
   set the GPIO to 1
2. Execute the sequence for the power up/down
3. if extmute is requested
 3.1. if GPIO number is not valid
   Use the internal MUTE functionality (HS_POPN_SET register, EXTMUTE bit)
 3.2. if GPIO number is valid
   set the GPIO to 0

For other devices it could be simpler:
1. Set GPIO for mute
2. Toggle a bit in the codec
3. Clear the GPIO to unmute

another device would need a sequence in step 2 instead of a single bit change.

To cover all of this in a generic way is not that straight forward IMHO.

Still, the main thing here was the fact that we should be able to drop
the flag for the feature in the first place.

Which flag you are referring?
I can (and have dropped) the hs_extmute_disable_level flag, but separate
hs_extmute, hs_extmute_gpio is needed for this device.

hs_extmute tells that the board implemented some sort of HS extmute functionality.
hs_extmute_gpio tells if the board is _not_ using the built in functionality,
but it is using external GPIO line to enable/disable the mute.

Sure I could do this:
hs_extmute: if only this is set we shall use the chip built in functionality
hs_extmute_gpio: if this is set we use the extmute feature but with external
                 GPIO.

But both need to be documented and supported.

-- 
P?ter

On Thu, Aug 09, 2012 at 01:18:50PM +0300, Peter Ujfalusi wrote:

On 08/08/2012 05:49 PM, Mark Brown wrote:

quoted

That makes sense if the GPIO is actively driven, open drain should be
better here, but it's still a generic thing which it'd be nice to
extract.

To cover all of this in a generic way is not that straight forward IMHO.

The sequence is just:

  1. Enable mutes (at _PRE time)
  2. Do whatever the device needs
  3. Disable the mutes (at _POST time)

I'm not sure there's any reason for you not to use the internal mute
even if the external mute is present but if there is that's the only
thing that's weird here.  If there's no reason not to do it it just goes
into step 2 and then it's fine, even if there is you can make it
conditional in step 2.

Sure I could do this:
hs_extmute: if only this is set we shall use the chip built in functionality
hs_extmute_gpio: if this is set we use the extmute feature but with external
                 GPIO.

But both need to be documented and supported.

Is there any actual case where an external mute is supplied via a
mechanism other than a GPIO, and if there is would it not either need
its own DT property or already need to interact with the driver from
code, making the DT property redundant?  My thinking here is that the
flag should be redundant because we already need to specify how we do
the mute, what I'd expect is that we activate the external mute
functionality as a result of being given another way of doing it so we
don't need to provide a flag.

On 08/09/2012 01:36 PM, Mark Brown wrote:

On Thu, Aug 09, 2012 at 01:18:50PM +0300, Peter Ujfalusi wrote:

quoted

On 08/08/2012 05:49 PM, Mark Brown wrote:

quoted

quoted

That makes sense if the GPIO is actively driven, open drain should be
better here, but it's still a generic thing which it'd be nice to
extract.

quoted

To cover all of this in a generic way is not that straight forward IMHO.

The sequence is just:

  1. Enable mutes (at _PRE time)
  2. Do whatever the device needs
  3. Disable the mutes (at _POST time)

I'm not sure there's any reason for you not to use the internal mute
even if the external mute is present but if there is that's the only
thing that's weird here.  If there's no reason not to do it it just goes
into step 2 and then it's fine, even if there is you can make it
conditional in step 2.

Not sure, but it should not cause issues. The PIN is multiplexed between
GPIO6/PWM0/MUTE functionality.
For that matter probably I could just don't care about flags here and
configure the extmute (the internal one) all the time. Not sure, it has been a
long time I have dealt with the twl4030...

quoted

Sure I could do this:
hs_extmute: if only this is set we shall use the chip built in functionality
hs_extmute_gpio: if this is set we use the extmute feature but with external
                 GPIO.

quoted

But both need to be documented and supported.

Is there any actual case where an external mute is supplied via a
mechanism other than a GPIO, and if there is would it not either need
its own DT property or already need to interact with the driver from
code, making the DT property redundant?

Not with my knowledge. The only board using it is the zoom2 upstream. I know
other boards (not in upstream) which either uses the internal mute or GPIO.

My thinking here is that the
flag should be redundant because we already need to specify how we do
the mute, what I'd expect is that we activate the external mute
functionality as a result of being given another way of doing it so we
don't need to provide a flag.

I perfectly understand your point. However how would you imagine this in the core?
We should have something similar to DAPM_SUPPLY which we can attach to the
widget which needs this sort of mute, but how big change we would need in the
core to do this I'm not sure.
I can take a look at this, but I would do it as a follow up series.

-- 
P?ter

* Peter Ujfalusi [off-list ref] [120808 02:42]:

Remove the use of set_hs_extmute callback and let the codec driver to
handle the extmute GPIO.

Acked-by: Tony Lindgren <tony@atomide.com>

On Thu, Aug 09, 2012 at 04:53:26PM +0300, Peter Ujfalusi wrote:

On 08/09/2012 01:36 PM, Mark Brown wrote:

quoted

Is there any actual case where an external mute is supplied via a
mechanism other than a GPIO, and if there is would it not either need
its own DT property or already need to interact with the driver from
code, making the DT property redundant?

Not with my knowledge. The only board using it is the zoom2 upstream. I know
other boards (not in upstream) which either uses the internal mute or GPIO.

OK, so we can probably only support GPIO for now then worry about other
external mutes when there's something to work with.