| Message ID | 1433863017-25349-1-git-send-email-rf@opensource.wolfsonmicro.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f5e2ce92bd96df99de1ef33fad05e3b3b2d34e54 |
| Headers | show |
On Tue, Jun 09, 2015 at 04:16:56PM +0100, Richard Fitzgerald wrote: > + ret = wm_adsp2_codec_probe(&priv->core.adsp[0], codec); > + if (ret) > + return ret; > + I'm still not a big fan of the double registration that's being done - if nothing else the fact that it's not also factoring out the creation of the DSP controls seems wrong.
On Tue, Jun 09, 2015 at 05:00:43PM +0100, Mark Brown wrote: > On Tue, Jun 09, 2015 at 04:16:56PM +0100, Richard Fitzgerald wrote: > > > + ret = wm_adsp2_codec_probe(&priv->core.adsp[0], codec); > > + if (ret) > > + return ret; > > + > > I'm still not a big fan of the double registration that's being done - > if nothing else the fact that it's not also factoring out the creation > of the DSP controls seems wrong. I don't see the point of trying to fight against the design of ASoC with the second probe. ASoC gives us what we need at the codec_probe stage so why try to invent something different?
On Tue, Jun 09, 2015 at 05:13:56PM +0100, Richard Fitzgerald wrote: > On Tue, Jun 09, 2015 at 05:00:43PM +0100, Mark Brown wrote: > > I'm still not a big fan of the double registration that's being done - > > if nothing else the fact that it's not also factoring out the creation > > of the DSP controls seems wrong. > I don't see the point of trying to fight against the design of ASoC with > the second probe. ASoC gives us what we need at the codec_probe stage > so why try to invent something different? Well, you could've still hung things off the struct device - it's not like the ASoC level device is a requirement here - and like I say the fact that it's not actually factoring out the initialisation that's already happening at the ASoC probe isn't good.
On Tue, Jun 09, 2015 at 05:20:45PM +0100, Mark Brown wrote: > On Tue, Jun 09, 2015 at 05:13:56PM +0100, Richard Fitzgerald wrote: > > On Tue, Jun 09, 2015 at 05:00:43PM +0100, Mark Brown wrote: > > > > I'm still not a big fan of the double registration that's being done - > > > if nothing else the fact that it's not also factoring out the creation > > > of the DSP controls seems wrong. > We can certainly look at factoring out that control creation once we have a probe function in wm_adsp to put them in. Which is what this patch creates. > > I don't see the point of trying to fight against the design of ASoC with > > the second probe. ASoC gives us what we need at the codec_probe stage > > so why try to invent something different? > > Well, you could've still hung things off the struct device - it's not > like the ASoC level device is a requirement here - and like I say the I'm doing it in the codec_probe because by that time ASoC has created its codec: debugfs node and I can put the dsp debugfs nodes under that. If I created the debugfs earlier before ASoC has probed the codec that node won't exist so I'd have to create my own debugfs node, and it seems a bit odd and untidy to have some codec debug info under the asoc node but some stuff somewhere else. > fact that it's not actually factoring out the initialisation that's > already happening at the ASoC probe isn't good.
On Tue, Jun 09, 2015 at 05:43:29PM +0100, Richard Fitzgerald wrote: > We can certainly look at factoring out that control creation once we have > a probe function in wm_adsp to put them in. Which is what this patch creates. Well, please do that then!
diff --git a/sound/soc/codecs/wm5102.c b/sound/soc/codecs/wm5102.c index 11eba0e..341d96e 100644 --- a/sound/soc/codecs/wm5102.c +++ b/sound/soc/codecs/wm5102.c @@ -1875,6 +1875,10 @@ static int wm5102_codec_probe(struct snd_soc_codec *codec) struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec); int ret; + ret = wm_adsp2_codec_probe(&priv->core.adsp[0], codec); + if (ret) + return ret; + ret = snd_soc_add_codec_controls(codec, wm_adsp2_fw_controls, 2); if (ret != 0) return ret; @@ -1893,6 +1897,8 @@ static int wm5102_codec_remove(struct snd_soc_codec *codec) { struct wm5102_priv *priv = snd_soc_codec_get_drvdata(codec); + wm_adsp2_codec_remove(&priv->core.adsp[0], codec); + priv->core.arizona->dapm = NULL; return 0; diff --git a/sound/soc/codecs/wm5110.c b/sound/soc/codecs/wm5110.c index d65364e..6e15d9c 100644 --- a/sound/soc/codecs/wm5110.c +++ b/sound/soc/codecs/wm5110.c @@ -1599,7 +1599,7 @@ static struct snd_soc_dai_driver wm5110_dai[] = { static int wm5110_codec_probe(struct snd_soc_codec *codec) { struct wm5110_priv *priv = snd_soc_codec_get_drvdata(codec); - int ret; + int i, ret; priv->core.arizona->dapm = &codec->dapm; @@ -1607,6 +1607,12 @@ static int wm5110_codec_probe(struct snd_soc_codec *codec) arizona_init_gpio(codec); arizona_init_mono(codec); + for (i = 0; i < WM5110_NUM_ADSP; ++i) { + ret = wm_adsp2_codec_probe(&priv->core.adsp[i], codec); + if (ret) + return ret; + } + ret = snd_soc_add_codec_controls(codec, wm_adsp2_fw_controls, 8); if (ret != 0) return ret; @@ -1621,6 +1627,10 @@ static int wm5110_codec_probe(struct snd_soc_codec *codec) static int wm5110_codec_remove(struct snd_soc_codec *codec) { struct wm5110_priv *priv = snd_soc_codec_get_drvdata(codec); + int i; + + for (i = 0; i < WM5110_NUM_ADSP; ++i) + wm_adsp2_codec_remove(&priv->core.adsp[i], codec); priv->core.arizona->dapm = NULL; diff --git a/sound/soc/codecs/wm_adsp.c b/sound/soc/codecs/wm_adsp.c index 6e358b5..490edb3 100644 --- a/sound/soc/codecs/wm_adsp.c +++ b/sound/soc/codecs/wm_adsp.c @@ -1906,6 +1906,18 @@ err: } EXPORT_SYMBOL_GPL(wm_adsp2_event); +int wm_adsp2_codec_probe(struct wm_adsp *dsp, struct snd_soc_codec *codec) +{ + return 0; +} +EXPORT_SYMBOL_GPL(wm_adsp2_codec_probe); + +int wm_adsp2_codec_remove(struct wm_adsp *dsp, struct snd_soc_codec *codec) +{ + return 0; +} +EXPORT_SYMBOL_GPL(wm_adsp2_codec_remove); + int wm_adsp2_init(struct wm_adsp *dsp) { int ret; diff --git a/sound/soc/codecs/wm_adsp.h b/sound/soc/codecs/wm_adsp.h index 0e5f07c..5584e34 100644 --- a/sound/soc/codecs/wm_adsp.h +++ b/sound/soc/codecs/wm_adsp.h @@ -79,6 +79,8 @@ extern const struct snd_kcontrol_new wm_adsp2_fw_controls[]; int wm_adsp1_init(struct wm_adsp *dsp); int wm_adsp2_init(struct wm_adsp *dsp); +int wm_adsp2_codec_probe(struct wm_adsp *dsp, struct snd_soc_codec *codec); +int wm_adsp2_codec_remove(struct wm_adsp *dsp, struct snd_soc_codec *codec); int wm_adsp1_event(struct snd_soc_dapm_widget *w, struct snd_kcontrol *kcontrol, int event); int wm_adsp2_early_event(struct snd_soc_dapm_widget *w,
Currently the only init function in wm_adsp is called by the codec driver early in its probe before the codec has been registered with SOC. This patch adds stubs for the codec_probe and codec_remove stages and calls them from WM5102 and WM5110 codec drivers. This allows us to hang anything that needs setup during the codec probe stage off these functions without further modification of the codec drivers. Signed-off-by: Richard Fitzgerald <rf@opensource.wolfsonmicro.com> --- sound/soc/codecs/wm5102.c | 6 ++++++ sound/soc/codecs/wm5110.c | 12 +++++++++++- sound/soc/codecs/wm_adsp.c | 12 ++++++++++++ sound/soc/codecs/wm_adsp.h | 2 ++ 4 files changed, 31 insertions(+), 1 deletions(-)