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@xxxxxxxxxxx>
>> [k.kozlowski: rebased on 4.1]
>> Signed-off-by: Krzysztof Kozlowski <k.kozlowski@xxxxxxxxxxx>
>> [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@xxxxxxxxxxx>
>
> 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.
--
Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
