Re: [RFC 01/10] ASoC: Intel: Boards: Machine driver for Intel platforms

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

On 12/4/17 9:10 AM, Ughreja, Rakesh A wrote:
> 
> 
>> -----Original Message-----
>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>> Sent: Monday, December 4, 2017 8:20 PM
>> To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa-
>> project.org; broonie@kernel.org; tiwai@suse.de; liam.r.girdwood@linux.intel.com
>> Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio
>> <patches.audio@intel.com>
>> Subject: Re: [alsa-devel] [RFC 01/10] ASoC: Intel: Boards: Machine driver for Intel
>> platforms
>>
>> On 12/4/17 4:55 AM, Ughreja, Rakesh A wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>>>> Sent: Friday, December 1, 2017 11:28 PM
>>>> To: Ughreja, Rakesh A <rakesh.a.ughreja@intel.com>; alsa-devel@alsa-
>>>> project.org; broonie@kernel.org; tiwai@suse.de;
>>>> liam.r.girdwood@linux.intel.com
>>>> Cc: Koul, Vinod <vinod.koul@intel.com>; Patches Audio
>>>> <patches.audio@intel.com>
>>>> Subject: Re: [alsa-devel] [RFC 01/10] ASoC: Intel: Boards: Machine driver for
>>>> Intel platforms
>>>>
>>>> On 12/1/17 3:13 AM, Rakesh Ughreja wrote:
>>>>> Add machine driver for Intel platforms(SKL/KBL) with HDA and iDisp codecs.
>>>>> This patch adds support for only iDisp (HDMI/DP) codec. In the
>>>>> following patch support for HDA codec will be added.
>>>>
>>>> But to be clear this should be sufficient for headless devices with no
>>>> audio codec (Joule or UP^2) where the only audio output is HDMI/iDisp?
>>>
>>> Yes, that’s right. Do you think it is better to have a separate machine
>>> driver for headless devices where we don't have any HDA codecs ?
>>>
>>> We can load that machine driver when we find only the iDisp codec.
>>
>> not necessarily, it depends how you detect the HDaudio codec.
> 
> So you prefer to use the machine driver I posted even when we don't
> have HDA codec present on the system ? Technically it should work.
> The only thing that you may see is some extra Front End DAI Links,
> unless we use separate Topology files.

we can deal with this later, this was just a remark to think of headless 
devices.

>   
> 
>>>
>>>>
>>>>>
>>>>> This should work for other Intel platforms as well e.g. BXT,GLK,CNL
>>>>> however this series is not tested on all the platforms.
>>>>>
>>>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>>>>> ---
>>>>>     sound/soc/intel/boards/Kconfig           |  10 ++
>>>>>     sound/soc/intel/boards/Makefile          |   2 +
>>>>>     sound/soc/intel/boards/skl_hda_generic.c | 276
>>>> +++++++++++++++++++++++++++++++
>>>>
>>>> can we drop the Skylake reference? It's become a catch-all term to mean
>>>> both the platform, the IP and the driver.
>>>
>>> Suggest some name. I have no problem.
>>
>> HiFi3 ?
>> iDisp ?
>> HDAudio-DSP ?
> 
> hda_dsp_generic.c -- For the main file
> hda_dsp_common.c -- for common functions
> 
> Does it look fine ?

works for me.

> 
>>
>>>
>>>>
>>>>>     3 files changed, 288 insertions(+)
>>>>>     create mode 100644 sound/soc/intel/boards/skl_hda_generic.c
>>>>>
>>>>> diff --git a/sound/soc/intel/boards/Kconfig
>>>> b/sound/soc/intel/boards/Kconfig
>>>>> index 6f75470..4f8bd02 100644
>>>>> --- a/sound/soc/intel/boards/Kconfig
>>>>> +++ b/sound/soc/intel/boards/Kconfig
>>>>> @@ -262,4 +262,14 @@ config
>>>> SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH
>>>>>               Say Y if you have such a device.
>>>>>               If unsure select "N".
>>>>>
>>>>> +config SND_SOC_INTEL_SKL_HDA_GENERIC_MACH
>>>>> +        tristate "ASoC Audio driver for SKL/KBL with HDA Codecs"
>>>>> +        select SND_SOC_INTEL_SST
>>>>> +        depends on SND_SOC_INTEL_SKYLAKE
>>>>
>>>> that's also going to have to be reworked to allow for an SOF-based
>>>> platform driver initializing this machine driver.
>>>
>>> Is this what you are fixing with your latest KConfig patches ?
>>
>> yes
>>
>>>
>>>>
>>>>> +        select SND_SOC_HDAC_HDMI
>>>>> +        help
>>>>> +          This adds support for ASoC Onboard Codec HDA machine driver.
>>>> This will
>>>>> +          create an alsa sound card for HDA Codecs.
>>>>> +          Say Y if you have such a device.
>>>>> +          If unsure select "N".
>>>>>     endif
>>>>> diff --git a/sound/soc/intel/boards/Makefile
>>>> b/sound/soc/intel/boards/Makefile
>>>>> index 69d2dfa..4686727 100644
>>>>> --- a/sound/soc/intel/boards/Makefile
>>>>> +++ b/sound/soc/intel/boards/Makefile
>>>>> @@ -17,6 +17,7 @@ snd-soc-sst-byt-cht-nocodec-objs :=
>>>> bytcht_nocodec.o
>>>>>     snd-soc-kbl_rt5663_max98927-objs := kbl_rt5663_max98927.o
>>>>>     snd-soc-kbl_rt5663_rt5514_max98927-objs :=
>>>> kbl_rt5663_rt5514_max98927.o
>>>>>     snd-soc-skl_rt286-objs := skl_rt286.o
>>>>> +snd-soc-skl_hda_generic-objs := skl_hda_generic.o
>>>>>     snd-skl_nau88l25_max98357a-objs := skl_nau88l25_max98357a.o
>>>>>     snd-soc-skl_nau88l25_ssm4567-objs := skl_nau88l25_ssm4567.o
>>>>>
>>>>> @@ -40,3 +41,4 @@ obj-
>>>> $(CONFIG_SND_SOC_INTEL_KBL_RT5663_RT5514_MAX98927_MACH) +=
>>>> snd-soc-kbl_rt566
>>>>>     obj-$(CONFIG_SND_SOC_INTEL_SKL_RT286_MACH) += snd-soc-
>>>> skl_rt286.o
>>>>>     obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_MAX98357A_MACH) +=
>>>> snd-skl_nau88l25_max98357a.o
>>>>>     obj-$(CONFIG_SND_SOC_INTEL_SKL_NAU88L25_SSM4567_MACH) +=
>>>> snd-soc-skl_nau88l25_ssm4567.o
>>>>> +obj-$(CONFIG_SND_SOC_INTEL_SKL_HDA_GENERIC_MACH) += snd-soc-
>>>> skl_hda_generic.o
>>>>> diff --git a/sound/soc/intel/boards/skl_hda_generic.c
>>>> b/sound/soc/intel/boards/skl_hda_generic.c
>>>>> new file mode 100644
>>>>> index 0000000..ece39b5
>>>>> --- /dev/null
>>>>> +++ b/sound/soc/intel/boards/skl_hda_generic.c
>>>>> @@ -0,0 +1,276 @@
>>>>> +/*
>>>>> + * Intel Machine Driver for SKL/KBL platforms with HDA Codecs
>>>>> + *
>>>>> + * Copyright (C) 2017, Intel Corporation. All rights reserved.
>>>>> + *
>>>>> + *
>>>>> + * This program is free software; you can redistribute it and/or
>>>>> + * modify it under the terms of the GNU General Public License version
>>>>> + * 2 as published by the Free Software Foundation.
>>>>> + *
>>>>> + * This program is distributed in the hope that it will be useful,
>>>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>>>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>>>>> + * GNU General Public License for more details.
>>>>> + */
>>>>> +
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/platform_device.h>
>>>>> +#include <sound/core.h>
>>>>> +#include <sound/jack.h>
>>>>> +#include <sound/pcm.h>
>>>>> +#include <sound/pcm_params.h>
>>>>> +#include <sound/soc.h>
>>>>> +#include "../../codecs/hdac_hdmi.h"
>>>>> +#include "../skylake/skl.h"
>>>>> +
>>>>> +static struct snd_soc_card skl_hda_audio_card;
>>>>> +static struct snd_soc_jack skl_hda_hdmi_jack[3];
>>>>
>>>> The code from here to ...
>>>>> +
>>>>> +struct skl_hda_hdmi_pcm {
>>>>> +	struct list_head head;
>>>>> +	struct snd_soc_dai *codec_dai;
>>>>> +	int device;
>>>>> +};
>>>>> +
>>>>> +struct skl_hda_private {
>>>>> +	struct list_head hdmi_pcm_list;
>>>>> +};
>>>>> +
>>>>> +enum {
>>>>> +	SKL_HDA_DPCM_AUDIO_HDMI1_PB = 0,
>>>>> +	SKL_HDA_DPCM_AUDIO_HDMI2_PB,
>>>>> +	SKL_HDA_DPCM_AUDIO_HDMI3_PB,
>>>>> +};
>>>>> +
>>>>> +static const struct snd_soc_dapm_route skl_hda_map[] = {
>>>>> +
>>>>> +	{ "hifi3", NULL, "iDisp3 Tx"},
>>>>> +	{ "iDisp3 Tx", NULL, "iDisp3_out"},
>>>>> +	{ "hifi2", NULL, "iDisp2 Tx"},
>>>>> +	{ "iDisp2 Tx", NULL, "iDisp2_out"},
>>>>> +	{ "hifi1", NULL, "iDisp1 Tx"},
>>>>> +	{ "iDisp1 Tx", NULL, "iDisp1_out"},
>>>>> +
>>>>> +};
>>>>> +
>>>>> +static int skl_hda_hdmi1_init(struct snd_soc_pcm_runtime *rtd)
>>>>> +{
>>>>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
>>>>> +	struct snd_soc_dai *dai = rtd->codec_dai;
>>>>> +	struct skl_hda_hdmi_pcm *pcm;
>>>>> +
>>>>> +	pcm = devm_kzalloc(rtd->card->dev, sizeof(*pcm), GFP_KERNEL);
>>>>> +	if (!pcm)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	pcm->device = SKL_HDA_DPCM_AUDIO_HDMI1_PB;
>>>>> +	pcm->codec_dai = dai;
>>>>> +
>>>>> +	list_add_tail(&pcm->head, &ctx->hdmi_pcm_list);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int skl_hda_hdmi2_init(struct snd_soc_pcm_runtime *rtd)
>>>>> +{
>>>>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
>>>>> +	struct snd_soc_dai *dai = rtd->codec_dai;
>>>>> +	struct skl_hda_hdmi_pcm *pcm;
>>>>> +
>>>>> +	pcm = devm_kzalloc(rtd->card->dev, sizeof(*pcm), GFP_KERNEL);
>>>>> +	if (!pcm)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	pcm->device = SKL_HDA_DPCM_AUDIO_HDMI2_PB;
>>>>> +	pcm->codec_dai = dai;
>>>>> +
>>>>> +	list_add_tail(&pcm->head, &ctx->hdmi_pcm_list);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>> +static int skl_hda_hdmi3_init(struct snd_soc_pcm_runtime *rtd)
>>>>> +{
>>>>> +	struct skl_hda_private *ctx = snd_soc_card_get_drvdata(rtd->card);
>>>>> +	struct snd_soc_dai *dai = rtd->codec_dai;
>>>>> +	struct skl_hda_hdmi_pcm *pcm;
>>>>> +
>>>>> +	pcm = devm_kzalloc(rtd->card->dev, sizeof(*pcm), GFP_KERNEL);
>>>>> +	if (!pcm)
>>>>> +		return -ENOMEM;
>>>>> +
>>>>> +	pcm->device = SKL_HDA_DPCM_AUDIO_HDMI3_PB;
>>>>> +	pcm->codec_dai = dai;
>>>>> +
>>>>> +	list_add_tail(&pcm->head, &ctx->hdmi_pcm_list);
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>
>>>> .. here is pretty much copy/pasted across machine drivers, can we move
>>>> it to a common include file? The experience from Baytrail/Cherrytrail is
>>>> that the more we wait the more complicated it is to factor the code.
>>>
>>> That’s a good point. I can do that in my next series. But this would be
>>> specific to SKL onwards platforms. I am not sure if it would work for
>>> BYT/CHT Etc.
>>
>> this code would not appl for BYT/CHT since the path doesn't go through
>> the DSP, so no ASoC and machine driver?
> 
> Yes, I think we don't have ASoC Machine drivers for BYT/CHT HDMI/HDA
> related things. It's all legacy way.
> 
>>
>>>
>>>>
>>>>> +/* skl_hda_digital audio interface glue - connects codec <--> CPU */
>>>>> +static struct snd_soc_dai_link skl_hda_dais[] = {
>>>>> +	/* Front End DAI links */
>>>>> +	[SKL_HDA_DPCM_AUDIO_HDMI1_PB] = {
>>>>
>>>> Are those hard-coded links required for all platforms, I thought the
>>>> newer ones were based on topology?
>>>
>>> Yes, I agree. In the latest patches from Guneshwor, the DAI Links
>>> are coming from Topology. I can fix it in the next revision
>>
>> I was wondering if this change to topology is across the board.
> 
> In my Topology file, I didn't have FE DAI, DAI Links. That’s why those are
> created manually in the Machine driver. I will check and get back how
> it works when I put it into Topology file.
> 
> 

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