Re: [PATCH] ASoC: Intel: kbl: Move codec sysclk config to codec_init function

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 05/24/2018 04:45 AM, Adam Thomson wrote:
> On 23 May 2018 07:14, Mac Chiang wrote:
>
>> From: Mac Chiang <mac.chiang@intel.com>
>>
>> unexpected error: PLL input clock 0 below valid range
>>
>> backport from broonie's tree
>> commit fd0f237572ad
>> ("ASoC: Intel: bxt: Move codec sysclk config to codec_init function")
>>
>> The MCLK for DA7219 does not change in this platform, but is
>> currently being configured everytime as part of the platform_clock
>> event handler for DAPM. The upshot of this is that we have
>> unnecessary calls to this function, and it also means that if
>> a stream hasn't yet been started, DA7219 driver does not have the
>> correct MCLK rates programmed and so the HP detection feature does
>> not operate as expected.
>>
>> This patch rectifies this issue by moving the sysclk call to
>> codec_init function so it's only called once at initialisation.
>>
>> Signed-off-by: Adam Thomson <Adam.Thomson.Opensource@diasemi.com>
>> Acked-by: Sathyanarayana Nujella <sathyanarayana.nujella@intel.com>
>> Singed-off-by: Mac Chiang <mac.chiang@intel.com>
Mac, I provided you internal feedback that your commit message was 
misleading.
This is not a 'backport', the intent here is to fix a problem on KBL 
using the same solution already merged for ApolloLake with commit 
fd0f237572ad ("ASoC: Intel: bxt: Move codec sysclk config to codec_init 
function"). You also want to be careful when including someone else's 
Signed-off-by and check for typos, if you run checkpatch.pl --strict 
you'd see this:

WARNING: 'everytime' may be misspelled - perhaps 'every time'?
#14:
currently being configured everytime as part of the platform_clock

WARNING: Non-standard signature: Singed-off-by:
#26:
Singed-off-by: Mac Chiang <mac.chiang@intel.com>

CHECK: Alignment should match open parenthesis
#57: FILE: sound/soc/intel/boards/kbl_da7219_max98357a.c:170:
+    ret = snd_soc_dai_set_sysclk(codec_dai, DA7219_CLKSRC_MCLK, 19200000,
+                        SND_SOC_CLOCK_IN);

total: 0 errors, 2 warnings, 1 checks, 32 lines checked

In addition, both Adam and I mentioned that the clocks are different 
between the two platforms, so this patch is incorrect and cannot be 
applied as is, see below.

>> ---
>>   sound/soc/intel/boards/kbl_da7219_max98357a.c | 17 +++++++++--------
>>   1 file changed, 9 insertions(+), 8 deletions(-)
>>
>> diff --git a/sound/soc/intel/boards/kbl_da7219_max98357a.c
>> b/sound/soc/intel/boards/kbl_da7219_max98357a.c
>> index 60e739f..88fbc6a 100644
>> --- a/sound/soc/intel/boards/kbl_da7219_max98357a.c
>> +++ b/sound/soc/intel/boards/kbl_da7219_max98357a.c
>> @@ -65,14 +65,6 @@ static int platform_clock_control(struct
>> snd_soc_dapm_widget *w,
>>   		return -EIO;
>>   	}
>>
>> -	/* Configure sysclk for codec */
>> -	ret = snd_soc_dai_set_sysclk(codec_dai, DA7219_CLKSRC_MCLK, 24576000,
>> -				     SND_SOC_CLOCK_IN);
> Code here is setting the MCLK rate for the codec as being 24.576MHz but...
>
>> -	if (ret) {
>> -		dev_err(card->dev, "can't set codec sysclk configuration\n");
>> -		return ret;
>> -	}
>> -
>>   	if (SND_SOC_DAPM_EVENT_OFF(event)) {
>>   		ret = snd_soc_dai_set_pll(codec_dai, 0,
>>   				     DA7219_SYSCLK_MCLK, 0, 0);
>> @@ -169,9 +161,18 @@ static int kabylake_da7219_codec_init(struct
>> snd_soc_pcm_runtime *rtd)
>>   {
>>   	struct kbl_codec_private *ctx = snd_soc_card_get_drvdata(rtd->card);
>>   	struct snd_soc_component *component = rtd->codec_dai->component;
>> +	struct snd_soc_dai *codec_dai = rtd->codec_dai;
>>   	struct snd_soc_jack *jack;
>>   	int ret;
>>
>> +	/* Configure sysclk for codec */
>> +	ret = snd_soc_dai_set_sysclk(codec_dai, DA7219_CLKSRC_MCLK, 19200000,
>> +						SND_SOC_CLOCK_IN);
> ...here you're setting MCLK rate to be 19.2MHz. Is that correct?
That is clearly wrong. Likely a copy/paste from bxt_da7219_max98357a.c 
instead of moving the code within the same file.

>
>> +	if (ret) {
>> +		dev_err(rtd->dev, "can't set codec sysclk configuration\n");
>> +		return ret;
>> +	}
>> +
>>   	/*
>>   	 * Headset buttons map to the google Reference headset.
>>   	 * These can be configured by userspace.
>> --
>> 2.7.4
>>
>> _______________________________________________
>> Alsa-devel mailing list
>> Alsa-devel@alsa-project.org
>> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel@alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel