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

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

On 3/11/18 1:19 PM, Hans de Goede wrote:
> 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?

yes.

> 
> 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 ?

Dunno. We may be able to test with SOF as well and check what formats work.

> 
>> 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.

well things are broken in the 16-bit mode then?

> 
> Regards,
> 
> Hans
> 
> _______________________________________________
> 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