On 07/22/2016 11:51 AM, Charles Keepax wrote:
On Tue, Jul 05, 2016 at 07:14:37PM +0200, Sylwester Nawrocki wrote:
This patch adds the sound machine driver for TM2 and TM2E board. Speaker and headphone playback, Main Mic capture, Bluetooth, Voice call and external accessory are supported.
Signed-off-by: Inha Song ideal.song@samsung.com [k.kozlowski: rebased on 4.1] Signed-off-by: Krzysztof Kozlowski k.kozlowski@samsung.com [s.nawrocki: rebased to 4.7, adjustment to the ASoC core changes, removed unused ops and direct calls to the max98504 function, added parsing of "audio-amplifier" and "audio-codec" properties, added TDM API calls, switched to gpiod API] Signed-off-by: Sylwester Nawrocki s.nawrocki@samsung.com
Apologies for my late response I had missed this.
Thanks a lot for your comments, this missed the 4.8 merge window anyway.
<snip> > +static int tm2_start_sysclk(struct snd_soc_card *card) > +{ > + struct tm2_machine_priv *priv = snd_soc_card_get_drvdata(card); > + struct snd_soc_codec *codec = priv->codec; > + unsigned long mclk_rate = clk_get_rate(priv->codec_mclk1); > + int ret; > + > + ret = clk_prepare_enable(priv->codec_mclk1); > + if (ret < 0) { > + dev_err(card->dev, "Failed to enable mclk: %d\n", ret); > + return ret; > + } > + > + ret = snd_soc_codec_set_pll(codec, WM5110_FLL1, > + ARIZONA_FLL_SRC_MCLK1, > + mclk_rate, > + priv->sysclk_rate); > + if (ret < 0) { > + dev_err(codec->dev, "Failed to start FLL: %d\n", ret); > + return ret; > + } > + > + ret = snd_soc_codec_set_pll(codec, WM5110_FLL1_REFCLK, > + ARIZONA_FLL_SRC_MCLK1, > + mclk_rate, > + priv->sysclk_rate); > + if (ret < 0) { > + dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret); > + return ret; > + }
It would be better to set the REFCLK first. Setting WM5110_FLL1 actually enables the FLL, where as setting WM5110_FLL1_REFCKL doesn't. So doing it this way round the first time you bring up the FLL it will enable it then reconfigure the reference path. Doing it the other way round it will always enable first time with the correct settings.
OK, thanks for the hint, I will reorder this.
+static int tm2_aif1_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- ret = snd_soc_dai_set_sysclk(codec_dai, ARIZONA_CLK_SYSCLK, 0, 0);
- if (ret < 0) {
dev_err(codec_dai->dev, "Failed to set SYSCLK: %d\n", ret);return ret;- }
If there is no intention to change which clocking domain the DAI is attached to I would put this in late probe, although it does no harm here and if that might get added in the future then here is better.
If the clocking arrangement ever needs to be changed the call could be added here again, I will move it to late_probe.
- return tm2_start_sysclk(rtd->card);
+}
+static struct snd_soc_ops tm2_aif1_ops = {
- .hw_params = tm2_aif1_hw_params,
+};
+static int tm2_aif2_hw_params(struct snd_pcm_substream *substream,
struct snd_pcm_hw_params *params)+{
- ret = snd_soc_codec_set_pll(codec, WM5110_FLL2,
ARIZONA_FLL_SRC_MCLK1,mclk_rate,asyncclk_rate);- if (ret < 0) {
dev_err(codec->dev, "Failed to start FLL: %d\n", ret);return ret;- }
- ret = snd_soc_codec_set_pll(codec, WM5110_FLL2_REFCLK,
ARIZONA_FLL_SRC_MCLK1,mclk_rate,asyncclk_rate);- if (ret < 0) {
dev_err(codec->dev, "Failed to set FLL1 Source: %d\n", ret);return ret;- }
Again as before I would set the REFCLK path first on the FLL.
Also nothing ever turns FLL2 off again here. I would either turn it off again in set_bias level or add a free callback into tm2_aif2_ops, probably adding a free callback matches the usage of this clock better I would guess.
hw_free sounds like a good option, I'll add it to ensure the WM5110_FLL2 clock is disabled when not in use.
+static int tm2_set_bias_level(struct snd_soc_card *card,
struct snd_soc_dapm_context *dapm,enum snd_soc_bias_level level)+{
- card->dapm.bias_level = level;
I believe the core does this for you these days.
Indeed, I had missed that, I'll drop this assignment.
+static int tm2_suspend_post(struct snd_soc_card *card) +{
- return tm2_stop_sysclk(card);
I think you can't really do this from these callbacks, the trouble is suspend_post is called from snd_soc_suspend which is set as the suspend callback in snd_soc_pm_ops. So when this is called you don't actually know if the SPI has already suspended or not, if it hasn't things will work but if the SPI has already suspended then this falls over.
A better solution would be to define a local copy of snd_soc_pm_ops with this functions set as the prepare and complete callbacks the other callbacks set as snd_soc_pm_ops sets them. These callback will run before anything is suspended and after everything has been resumed.
Agreed, dependency on the SPI controller seems to be not modelled and it looks like it is working by chance now. I will try to use prepare/ complete callback until better options are available.