[PATCH 1/5] ASoC: Tegra: wm8903 probe: Don't call machine_is_*()

[PATCH 1/5] ASoC: Tegra: wm8903 probe: Don't call machine_is_*()

[Stephen Warren]

This machine driver is a platform driver, and hence will only be
instantiated on the correct machines. Hence, there is no need to
check the current machine during probe.

(Applying Mark's TrimSlice review comments to the existing driver)

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 sound/soc/tegra/tegra_wm8903.c |    7 -------
 1 files changed, 0 insertions(+), 7 deletions(-)

diff --git a/sound/soc/tegra/tegra_wm8903.c b/sound/soc/tegra/tegra_wm8903.c
index 37f6010..1eb0632 100644
--- a/sound/soc/tegra/tegra_wm8903.c
+++ b/sound/soc/tegra/tegra_wm8903.c
@@ -391,13 +391,6 @@ static __devinit int tegra_wm8903_driver_probe(struct platform_device *pdev)
 	struct tegra_wm8903_platform_data *pdata;
 	int ret;
 
-	if (!machine_is_harmony() && !machine_is_ventana() &&
-	    !machine_is_seaboard() && !machine_is_kaen() &&
-	    !machine_is_aebl()) {
-		dev_err(&pdev->dev, "Not running on a supported board!\n");
-		return -ENODEV;
-	}
-
 	pdata = pdev->dev.platform_data;
 	if (!pdata) {
 		dev_err(&pdev->dev, "No platform data supplied\n");
-- 
1.7.1

[PATCH 2/5] ASoC: Tegra: wm8903: Remove redundant drvdata clears

[Stephen Warren]

When the driver is not initialized/registered, nothing should be touching
these fields anyway, so there's no point clearing them out.

(Applying Mark's TrimSlice review comments to the existing driver)

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 sound/soc/tegra/tegra_wm8903.c |   11 ++---------
 1 files changed, 2 insertions(+), 9 deletions(-)

diff --git a/sound/soc/tegra/tegra_wm8903.c b/sound/soc/tegra/tegra_wm8903.c
index 1eb0632..b12b1fd 100644
--- a/sound/soc/tegra/tegra_wm8903.c
+++ b/sound/soc/tegra/tegra_wm8903.c
@@ -417,15 +417,12 @@ static __devinit int tegra_wm8903_driver_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n",
 			ret);
-		goto err_clear_drvdata;
+		goto err_fini_utils;
 	}
 
 	return 0;
 
-err_clear_drvdata:
-	snd_soc_card_set_drvdata(card, NULL);
-	platform_set_drvdata(pdev, NULL);
-	card->dev = NULL;
+err_fini_utils:
 	tegra_asoc_utils_fini(&machine->util_data);
 err_free_machine:
 	kfree(machine);
@@ -440,10 +437,6 @@ static int __devexit tegra_wm8903_driver_remove(struct platform_device *pdev)
 
 	snd_soc_unregister_card(card);
 
-	snd_soc_card_set_drvdata(card, NULL);
-	platform_set_drvdata(pdev, NULL);
-	card->dev = NULL;
-
 	tegra_asoc_utils_fini(&machine->util_data);
 
 	if (machine->gpio_requested & GPIO_EXT_MIC_EN)
-- 
1.7.1

[PATCH 3/5] ASoC: Tegra: Don't return mclk_changed from utils_set_rate

[Stephen Warren]

Only the clock programming code needs to know whether the clocks changed,
and that is encapsulated within tegra_asoc_utils_set_rate(). The machine
driver's call to snd_soc_dai_set_sysclk(codec_dai, ...) is safe
irrespective of whether the clocks changed.

(Applying Mark's TrimSlice review comments to the existing driver)

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 sound/soc/tegra/tegra_asoc_utils.c |    9 +++++----
 sound/soc/tegra/tegra_asoc_utils.h |    2 +-
 sound/soc/tegra/tegra_wm8903.c     |   17 +++++++----------
 3 files changed, 13 insertions(+), 15 deletions(-)

diff --git a/sound/soc/tegra/tegra_asoc_utils.c b/sound/soc/tegra/tegra_asoc_utils.c
index 52f0a3f..dfa85cb 100644
--- a/sound/soc/tegra/tegra_asoc_utils.c
+++ b/sound/soc/tegra/tegra_asoc_utils.c
@@ -28,9 +28,10 @@
 #include "tegra_asoc_utils.h"
 
 int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate,
-			      int mclk, int *mclk_change)
+			      int mclk)
 {
 	int new_baseclock;
+	bool clk_change;
 	int err;
 
 	switch (srate) {
@@ -52,10 +53,10 @@ int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate,
 		return -EINVAL;
 	}
 
-	*mclk_change = ((new_baseclock != data->set_baseclock) ||
+	clk_change = ((new_baseclock != data->set_baseclock) ||
 			(mclk != data->set_mclk));
-	if (!*mclk_change)
-	    return 0;
+	if (!clk_change)
+		return 0;
 
 	data->set_baseclock = 0;
 	data->set_mclk = 0;
diff --git a/sound/soc/tegra/tegra_asoc_utils.h b/sound/soc/tegra/tegra_asoc_utils.h
index bbba7af..4818195 100644
--- a/sound/soc/tegra/tegra_asoc_utils.h
+++ b/sound/soc/tegra/tegra_asoc_utils.h
@@ -36,7 +36,7 @@ struct tegra_asoc_utils_data {
 };
 
 int tegra_asoc_utils_set_rate(struct tegra_asoc_utils_data *data, int srate,
-			      int mclk, int *mclk_change);
+			      int mclk);
 int tegra_asoc_utils_init(struct tegra_asoc_utils_data *data,
 			  struct device *dev);
 void tegra_asoc_utils_fini(struct tegra_asoc_utils_data *data);
diff --git a/sound/soc/tegra/tegra_wm8903.c b/sound/soc/tegra/tegra_wm8903.c
index b12b1fd..988ff50 100644
--- a/sound/soc/tegra/tegra_wm8903.c
+++ b/sound/soc/tegra/tegra_wm8903.c
@@ -72,7 +72,7 @@ static int tegra_wm8903_hw_params(struct snd_pcm_substream *substream,
 	struct snd_soc_codec *codec = rtd->codec;
 	struct snd_soc_card *card = codec->card;
 	struct tegra_wm8903 *machine = snd_soc_card_get_drvdata(card);
-	int srate, mclk, mclk_change;
+	int srate, mclk;
 	int err;
 
 	srate = params_rate(params);
@@ -90,8 +90,7 @@ static int tegra_wm8903_hw_params(struct snd_pcm_substream *substream,
 	while (mclk < 6000000)
 		mclk *= 2;
 
-	err = tegra_asoc_utils_set_rate(&machine->util_data, srate, mclk,
-					&mclk_change);
+	err = tegra_asoc_utils_set_rate(&machine->util_data, srate, mclk);
 	if (err < 0) {
 		dev_err(card->dev, "Can't configure clocks\n");
 		return err;
@@ -115,13 +114,11 @@ static int tegra_wm8903_hw_params(struct snd_pcm_substream *substream,
 		return err;
 	}
 
-	if (mclk_change) {
-		err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
-					     SND_SOC_CLOCK_IN);
-		if (err < 0) {
-			dev_err(card->dev, "codec_dai clock not set\n");
-			return err;
-		}
+	err = snd_soc_dai_set_sysclk(codec_dai, 0, mclk,
+					SND_SOC_CLOCK_IN);
+	if (err < 0) {
+		dev_err(card->dev, "codec_dai clock not set\n");
+		return err;
 	}
 
 	return 0;
-- 
1.7.1

[PATCH 4/5] ASoC: Process card->widgets after all probe routines

[Stephen Warren]

Without this, if tegra_wm8903 is modified to provide controls,
widgets, and routes through data rather than calling registration
functions, tegra_wm8903_event_* will crash, because w->codec==NULL.

Within snd_soc_instantiate_card, something inside soc_probe_dai_link is
required for snd_soc_dapm_new_controls to set up the new widgets
correctly, although I haven't tracked down what exactly. (For
tegra_wm8903, there is no card->probe, and no aux_devs, so those calls
aren't relevant).

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 sound/soc/soc-core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 3b3a377..8ce40a1 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -1878,10 +1878,6 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 	INIT_WORK(&card->deferred_resume_work, soc_resume_deferred);
 #endif
 
-	if (card->dapm_widgets)
-		snd_soc_dapm_new_controls(&card->dapm, card->dapm_widgets,
-					  card->num_dapm_widgets);
-
 	/* initialise the sound card only once */
 	if (card->probe) {
 		ret = card->probe(card);
@@ -1907,6 +1903,10 @@ static void snd_soc_instantiate_card(struct snd_soc_card *card)
 		}
 	}
 
+	if (card->dapm_widgets)
+		snd_soc_dapm_new_controls(&card->dapm, card->dapm_widgets,
+					  card->num_dapm_widgets);
+
 	/* We should have a non-codec control add function but we don't */
 	if (card->controls)
 		snd_soc_add_controls(list_first_entry(&card->codec_dev_list,
-- 
1.7.1

Re: [PATCH 4/5] ASoC: Process card->widgets after all probe routines

[Mark Brown]

On Tue, Apr 19, 2011 at 12:46:16PM -0600, Stephen Warren wrote:

> Without this, if tegra_wm8903 is modified to provide controls,
> widgets, and routes through data rather than calling registration
> functions, tegra_wm8903_event_* will crash, because w->codec==NULL.

This will break other things as it means that we can't route to the DAPM
widgets that are added from machine init functions (eg, those that set
up jacks) which is a more common case - see the changelog for the code
you're modifying.  

There's also the fact that these widgets should be being created in the
card DAPM context so I'm a bit concerned that they're getting a CODEC at
all.  The dereferencing you're trying to do isn't good, you're looking
for a CODEC to find the card for widgets in the card DAPM context which
shouldn't be associated with any particular CODEC in the first place.
You should just find the card based on the DAPM context you're in.

Re: [PATCH 4/5] ASoC: Process card->widgets after all probe routines

[Stephen Warren]

Mark Brown wrote at Tuesday, April 19, 2011 12:53 PM:
> On Tue, Apr 19, 2011 at 12:46:16PM -0600, Stephen Warren wrote:
> 
> > Without this, if tegra_wm8903 is modified to provide controls,
> > widgets, and routes through data rather than calling registration
> > functions, tegra_wm8903_event_* will crash, because w->codec==NULL.
> 
> This will break other things as it means that we can't route to the DAPM
> widgets that are added from machine init functions (eg, those that set
> up jacks) which is a more common case - see the changelog for the code
> you're modifying.
> 
> There's also the fact that these widgets should be being created in the
> card DAPM context so I'm a bit concerned that they're getting a CODEC at
> all.  The dereferencing you're trying to do isn't good, you're looking
> for a CODEC to find the card for widgets in the card DAPM context which
> shouldn't be associated with any particular CODEC in the first place.
> You should just find the card based on the DAPM context you're in.

OK. I dropped that patch and fixed up the event functions so they work
correctly.

However, I do notice one other regression moving the tegra_wm8903 driver
to data instead of code for widgets/controls/routes:

Before that change, the "Int Spk" control both works fine, and when I
modify the value in alsamixer, exit, and restart alsamixer, the value I
set is maintained.

However, after that change, the control works fine (i.e. mutes/umutes
speakers while speaker-test is running in the background), but when I
quit and restart alsamixer, the most recent set value is not shown.

Do you have any clue what I should look at for that?

Thanks!

-- 
nvpublic

Re: [PATCH 4/5] ASoC: Process card->widgets after all probe routines

[Mark Brown]

On Tue, Apr 19, 2011 at 12:19:37PM -0700, Stephen Warren wrote:

> Before that change, the "Int Spk" control both works fine, and when I
> modify the value in alsamixer, exit, and restart alsamixer, the value I
> set is maintained.

> However, after that change, the control works fine (i.e. mutes/umutes
> speakers while speaker-test is running in the background), but when I
> quit and restart alsamixer, the most recent set value is not shown.

> Do you have any clue what I should look at for that?

Cross-context DAPM will be broken - I already found several issues there
since I started using it myself, it's probable that there's some more in
there.

[PATCH 5/5] ASoC: Tegra: wm8903: s/code/data/ for control/widget/maps

[Stephen Warren]

Replace calls to a variety of registration functions by updating
struct snd_soc_card snd_soc_tegra_wm8903 to directly point at the
various control/widget/map tables instead. The ASoC core now
performs any required registration based on these data fields.

(Applying Mark's TrimSlice review comments to the existing driver)

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 sound/soc/tegra/tegra_wm8903.c |   41 ++++++++++++++++++---------------------
 1 files changed, 19 insertions(+), 22 deletions(-)

diff --git a/sound/soc/tegra/tegra_wm8903.c b/sound/soc/tegra/tegra_wm8903.c
index 988ff50..fe83b38 100644
--- a/sound/soc/tegra/tegra_wm8903.c
+++ b/sound/soc/tegra/tegra_wm8903.c
@@ -294,28 +294,6 @@ static int tegra_wm8903_init(struct snd_soc_pcm_runtime *rtd)
 		gpio_direction_output(pdata->gpio_ext_mic_en, 0);
 	}
 
-	ret = snd_soc_add_controls(codec, tegra_wm8903_controls,
-				   ARRAY_SIZE(tegra_wm8903_controls));
-	if (ret < 0)
-		return ret;
-
-	snd_soc_dapm_new_controls(dapm, tegra_wm8903_dapm_widgets,
-					ARRAY_SIZE(tegra_wm8903_dapm_widgets));
-
-	if (machine_is_harmony() || machine_is_ventana()) {
-		snd_soc_dapm_add_routes(dapm, harmony_audio_map,
-					ARRAY_SIZE(harmony_audio_map));
-	} else if (machine_is_seaboard()) {
-		snd_soc_dapm_add_routes(dapm, seaboard_audio_map,
-					ARRAY_SIZE(seaboard_audio_map));
-	} else if (machine_is_kaen()) {
-		snd_soc_dapm_add_routes(dapm, kaen_audio_map,
-					ARRAY_SIZE(kaen_audio_map));
-	} else {
-		snd_soc_dapm_add_routes(dapm, aebl_audio_map,
-					ARRAY_SIZE(aebl_audio_map));
-	}
-
 	if (gpio_is_valid(pdata->gpio_hp_det)) {
 		tegra_wm8903_hp_jack_gpio.gpio = pdata->gpio_hp_det;
 		snd_soc_jack_new(codec, "Headphone Jack", SND_JACK_HEADPHONE,
@@ -379,6 +357,11 @@ static struct snd_soc_card snd_soc_tegra_wm8903 = {
 	.name = "tegra-wm8903",
 	.dai_link = &tegra_wm8903_dai,
 	.num_links = 1,
+
+	.controls = tegra_wm8903_controls,
+	.num_controls = ARRAY_SIZE(tegra_wm8903_controls),
+	.dapm_widgets = tegra_wm8903_dapm_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(tegra_wm8903_dapm_widgets),
 };
 
 static __devinit int tegra_wm8903_driver_probe(struct platform_device *pdev)
@@ -410,6 +393,20 @@ static __devinit int tegra_wm8903_driver_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, card);
 	snd_soc_card_set_drvdata(card, machine);
 
+	if (machine_is_harmony() || machine_is_ventana()) {
+		card->dapm_routes = harmony_audio_map;
+		card->num_dapm_routes = ARRAY_SIZE(harmony_audio_map);
+	} else if (machine_is_seaboard()) {
+		card->dapm_routes = seaboard_audio_map;
+		card->num_dapm_routes = ARRAY_SIZE(seaboard_audio_map);
+	} else if (machine_is_kaen()) {
+		card->dapm_routes = kaen_audio_map;
+		card->num_dapm_routes = ARRAY_SIZE(kaen_audio_map);
+	} else {
+		card->dapm_routes = aebl_audio_map;
+		card->num_dapm_routes = ARRAY_SIZE(aebl_audio_map);
+	}
+
 	ret = snd_soc_register_card(card);
 	if (ret) {
 		dev_err(&pdev->dev, "snd_soc_register_card failed (%d)\n",
-- 
1.7.1