Re: [PATCH v3 19/22] ASoC: Intel: bytcr_rt5651: Add support for Bay Trail CR / SSP0 using boards

From: Hans de Goede <hdegoede@redhat.com>
To: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>,
	Mark Brown <broonie@kernel.org>, Bard Liao <bardliao@realtek.com>,
	Oder Chiou <oder_chiou@realtek.com>
Cc: alsa-devel@alsa-project.org, Carlo Caione <carlo@endlessm.com>,
	Takashi Iwai <tiwai@suse.com>
Subject: Re: [PATCH v3 19/22] ASoC: Intel: bytcr_rt5651: Add support for Bay Trail CR / SSP0 using boards
Date: Sun, 11 Mar 2018 19:19:34 +0100	[thread overview]
Message-ID: <2abd9dba-7e02-3bd4-847f-c63e98f8ad8b@redhat.com> (raw)
In-Reply-To: <e4558b54-415e-a7f0-e696-27b9b9eb0a31@linux.intel.com>

Hi,

On 10-03-18 00:30, Pierre-Louis Bossart wrote:
> 
> Sorry about the delay, I've only found the time to look at this machine driver changes this afternoon while I was doing the SOF integration
> 
>> @@ -291,9 +332,16 @@ static int byt_rt5651_aif1_hw_params(struct snd_pcm_substream *substream,
>>   {
>>       struct snd_soc_pcm_runtime *rtd = substream->private_data;
>>       struct snd_soc_dai *codec_dai = rtd->codec_dai;
>> +    snd_pcm_format_t format = params_format(params);
>>       int rate = params_rate(params);
>> +    int bclk_ratio;
>> -    return byt_rt5651_prepare_and_enable_pll1(codec_dai, rate, 50);
>> +    if (format == SNDRV_PCM_FORMAT_S16_LE)
>> +        bclk_ratio = 32;
> 
> I don't think we can generate a 32x ratio with a 19.2 MHz clock, even less so with a 25MHz reference.
> And in addition in the bytcr_rt5640 case we could never get any audio on bytcr which uses the SSP0 link without the mclk enabled, so this SSP0+blck mode was never tested (maybe precisely because this ratio was wrong...)
> 
> In other words there is a test gap here for the case using SSP0+bclk as a reference with the rt5651 as well. Does anyone have access to a BYT-CR platform w/ rt5651 to test this?

Yes I've access to a BYT-CR platform w/ rt5651, I assume that you want me to test there with the
BYT_RT5651_MCLK_EN quirk unset?

What should I try to fix things if they don't work?  Or maybe just put a big fat
warning printf in the driver if SSP0 is used without bclk and remove the handling
for the bclk + SSP0 case as its broken anyways ?

> The other comment I have is that this run-time definition of the bclk_ratio is inconsistent with the hard-coded value I see in platform_clock_control()
> ret = byt_rt5651_prepare_and_enable_pll1(codec_dai, 48000, 50);

Yes the assumption here is that we simply need "a" clock when the "Platform Clock"
gets enabled and that the clock then will later get set to the correct speed.

AFAIK you did not want duplication of the SSP0 handling outside the fixup
function and platform_clock_control() does not have access to the format,
since that is likely not even setup by then.

Regards,

Hans

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