Re: [PATCH] ASoC: intel: Add Broadwell rt5650 machine driver

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

On 7/22/19 4:04 PM, Jon Flatley wrote:
> On Mon, Jul 22, 2019 at 5:34 AM Pierre-Louis Bossart
> <pierre-louis.bossart@linux.intel.com> wrote:
>>
>>
>>
>> On 7/18/19 6:12 PM, Jon Flatley wrote:
>>> From: Ben Zhang <benzh@chromium.org>
>>>
>>> Add machine driver for Broadwell + rt5650.
>>
>> Interesting. the only Broadwell device we encountered with I2S instead
>> of HDaudio was Samus w/ rt5677, can you share which model this is?
> 
> Yes, this is for the Acer Chromebase 24, aka Buddy.

Ah ok, I wasn't aware of this one, thanks for the pointer.


>>> +static int bdw_rt5650_rtd_init(struct snd_soc_pcm_runtime *rtd)
>>> +{
>>> +     struct snd_soc_component *component =
>>> +             snd_soc_rtdcom_lookup(rtd, DRV_NAME);
>>> +     struct sst_pdata *pdata = dev_get_platdata(component->dev);
>>> +     struct sst_hsw *broadwell = pdata->dsp;
>>> +     int ret;
>>> +
>>> +     /* Set ADSP SSP port settings
>>> +      * clock_divider = 4 means BCLK = MCLK/5 = 24MHz/5 = 4.8MHz
>>> +      */
>>> +     ret = sst_hsw_device_set_config(broadwell, SST_HSW_DEVICE_SSP_0,
>>> +             SST_HSW_DEVICE_MCLK_FREQ_24_MHZ,
>>> +             SST_HSW_DEVICE_TDM_CLOCK_MASTER, 4);
>>
>> this is going to break compilation if SOF is selected. this function
>> should be filtered out with
>> #if !IS_ENABLED(CONFIG_SND_SOC_SOF_BROADWELL)
>>
>> as done in bdw-rt5677
>>
>>> +     if (ret < 0) {
>>> +             dev_err(rtd->dev, "error: failed to set device config\n");
>>> +             return ret;
>>> +     }
>>> +
>>> +     return 0;
>>> +}
>>> +
>>> +static int bdw_rt5650_init(struct snd_soc_pcm_runtime *rtd)
>>> +{
>>> +     struct bdw_rt5650_priv *bdw_rt5650 =
>>> +             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;
>>> +     int ret;
>>> +
>>> +     /* Enable codec ASRC function for Stereo DAC/Stereo1 ADC/DMIC/I2S1.
>>> +      * The ASRC clock source is clk_i2s1_asrc.
>>> +      */
>>> +     rt5645_sel_asrc_clk_src(component,
>>> +                             RT5645_DA_STEREO_FILTER |
>>> +                             RT5645_DA_MONO_L_FILTER |
>>> +                             RT5645_DA_MONO_R_FILTER |
>>> +                             RT5645_AD_STEREO_FILTER |
>>> +                             RT5645_AD_MONO_L_FILTER |
>>> +                             RT5645_AD_MONO_R_FILTER,
>>> +                             RT5645_CLK_SEL_I2S1_ASRC);
>>> +
>>> +     /* TDM 4 slots 24 bit, set Rx & Tx bitmask to 4 active slots */
>>> +     ret = snd_soc_dai_set_tdm_slot(codec_dai, 0xF, 0xF, 4, 24);
>>
>> could we move this to bdw_rt5650_rtd_init() so that this doesn't impact SOF?
> 
> I'm not familiar with the differences in SOF. Can you elaborate on
> what exactly needs to be moved?

The 2 main differences are that SOF does not have any support for this 
sst_hsw_device_set_config() function - and it should have been 
abstracted away with a dai operation - and the fixed settings are 
replaced by topology-defined configurations.

we can of course do the SOF support later but you want to make sure the 
compilation issue is sorted out with the #if !IS_ENABLED() code above.