[alsa-devel] [PATCH 1/5] ASoC: Tegra: wm8903 probe: Don't call machine_is_*()
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@nvidia.com --- 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");
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@nvidia.com --- 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)
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@nvidia.com --- 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;
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@nvidia.com --- 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,
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.
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!
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.
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@nvidia.com --- 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",
participants (2)
-
Mark Brown -
Stephen Warren