From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753175AbcGYOUf (ORCPT ); Mon, 25 Jul 2016 10:20:35 -0400 Received: from mailout4.w1.samsung.com ([210.118.77.14]:60291 "EHLO mailout4.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752449AbcGYOU1 (ORCPT ); Mon, 25 Jul 2016 10:20:27 -0400 X-AuditID: cbfec7f5-f792a6d000001302-78-579620279e33 Subject: Re: [alsa-devel] [PATCH v4 4/4] ASoC: samsung: Add machine driver for Exynos5433 based TM2 board To: Charles Keepax References: <1467738877-31555-1-git-send-email-s.nawrocki@samsung.com> <20160722095115.GB8680@localhost.localdomain> Cc: robh@kernel.org, alsa-devel@alsa-project.org, linux-samsung-soc@vger.kernel.org, b.zolnierkie@samsung.com, Krzysztof Kozlowski , linux-kernel@vger.kernel.org, inki.dae@samsung.com, devicetree@vger.kernel.org, broonie@kernel.org, ideal.song@samsung.com From: Sylwester Nawrocki Message-id: <57962022.6020808@samsung.com> Date: Mon, 25 Jul 2016 16:20:18 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.6.0 MIME-version: 1.0 In-reply-to: <20160722095115.GB8680@localhost.localdomain> Content-type: text/plain; charset=windows-1252 Content-transfer-encoding: 7bit X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFnrDLMWRmVeSWpSXmKPExsVy+t/xa7rqCtPCDXpnClhcuXiIyWLjjPWs FlMfPmGz+DflBrvF/CPnWC12/b3PaDHp/gQWi9cvDC0u75rDZjHj/D4mi/97drA7cHts+NzE 5rFpVSebx8uJv9k8+rasYvT4vEkugDWKyyYlNSezLLVI3y6BK+PZ3pqCXu2K9W1XWBsYfyp2 MXJwSAiYSJw5r9PFyAlkiklcuLeerYuRi0NIYCmjxO32qewQznNGicmb+llAqoQFsiV2TdrK DmKLCFhITFlyixnEFhIok1g3+z9YA7PAAiaJprYtbCAJNgFDid6jfYwgNq+AlkTvtotgzSwC qhL3vq8EGyoqECHxZO5JqBpBiR+T74HFOQWsJN7NfM0IcimzgJ7E/YtaIGFmAXmJzWveMk9g FJiFpGMWQtUsJFULGJlXMYqmliYXFCel5xrpFSfmFpfmpesl5+duYoTEwdcdjEuPWR1iFOBg VOLhVWCaGi7EmlhWXJl7iFGCg1lJhLdSZlq4EG9KYmVValF+fFFpTmrxIUZpDhYlcd6Zu96H CAmkJ5akZqemFqQWwWSZODilGhjPVBaLZbO6PPn6eF7pzyXH5j16y/Dt6VrO29yRSzO+Rxy7 mTt34b0FvReVNghUVCk/4XjGlj3F4vfe2fOjty4X03HsP5Ecds1aWXLd2bf+O1Wa6p4sjRcS Cn76MN1ZYrfwrtSu2hsPzLdFxr0LS8150nR3058PpzynhR6we91xzPH24iemd26sU2Ipzkg0 1GIuKk4EAPAkmCx/AgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> [k.kozlowski: rebased on 4.1] >> Signed-off-by: Krzysztof Kozlowski >> [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 > > Apologies for my late response I had missed this. Thanks a lot for your comments, this missed the 4.8 merge window anyway. > >> +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. -- Thanks, Sylwester