>-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@xxxxxxxxxxxxxxx] >Sent: Friday, December 1, 2017 11:28 PM >To: Ughreja, Rakesh A <rakesh.a.ughreja@xxxxxxxxx>; alsa-devel@alsa- >project.org; broonie@xxxxxxxxxx; tiwai@xxxxxxx; >liam.r.girdwood@xxxxxxxxxxxxxxx >Cc: Koul, Vinod <vinod.koul@xxxxxxxxx>; Patches Audio ><patches.audio@xxxxxxxxx> >Subject: Re: [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. > >> >> 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@xxxxxxxxx> >> --- >> 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. > >> 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 ? > >> + 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. > >> +/* 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. > >> + .name = "SKL HDA HDMI Port1", >> + .stream_name = "Hdmi1", >> + .cpu_dai_name = "HDMI1 Pin", >> + .codec_name = "snd-soc-dummy", >> + .codec_dai_name = "snd-soc-dummy-dai", >> + .platform_name = "0000:00:1f.3", >> + .dpcm_playback = 1, >> + .init = NULL, >> + .trigger = { >> + SND_SOC_DPCM_TRIGGER_POST, >SND_SOC_DPCM_TRIGGER_POST}, >> + .nonatomic = 1, >> + .dynamic = 1, >> + }, >> + [SKL_HDA_DPCM_AUDIO_HDMI2_PB] = { >> + .name = "SKL HDA HDMI Port2", >> + .stream_name = "Hdmi2", >> + .cpu_dai_name = "HDMI2 Pin", >> + .codec_name = "snd-soc-dummy", >> + .codec_dai_name = "snd-soc-dummy-dai", >> + .platform_name = "0000:00:1f.3", >> + .dpcm_playback = 1, >> + .init = NULL, >> + .trigger = { >> + SND_SOC_DPCM_TRIGGER_POST, >SND_SOC_DPCM_TRIGGER_POST}, >> + .nonatomic = 1, >> + .dynamic = 1, >> + }, >> + [SKL_HDA_DPCM_AUDIO_HDMI3_PB] = { >> + .name = "SKL HDA HDMI Port3", >> + .stream_name = "Hdmi3", >> + .cpu_dai_name = "HDMI3 Pin", >> + .codec_name = "snd-soc-dummy", >> + .codec_dai_name = "snd-soc-dummy-dai", >> + .platform_name = "0000:00:1f.3", >> + .trigger = { >> + SND_SOC_DPCM_TRIGGER_POST, >SND_SOC_DPCM_TRIGGER_POST}, >> + .dpcm_playback = 1, >> + .init = NULL, >> + .nonatomic = 1, >> + .dynamic = 1, >> + }, >> + >> + /* Back End DAI links */ >> + { >> + .name = "iDisp1", >> + .id = 1, >> + .cpu_dai_name = "iDisp1 Pin", >> + .codec_name = "ehdaudio0D2", >> + .codec_dai_name = "intel-hdmi-hifi1", >> + .platform_name = "0000:00:1f.3", >> + .dpcm_playback = 1, >> + .init = skl_hda_hdmi1_init, >> + .no_pcm = 1, >> + }, >> + { >> + .name = "iDisp2", >> + .id = 2, >> + .cpu_dai_name = "iDisp2 Pin", >> + .codec_name = "ehdaudio0D2", >> + .codec_dai_name = "intel-hdmi-hifi2", >> + .platform_name = "0000:00:1f.3", >> + .init = skl_hda_hdmi2_init, >> + .dpcm_playback = 1, >> + .no_pcm = 1, >> + }, >> + { >> + .name = "iDisp3", >> + .id = 3, >> + .cpu_dai_name = "iDisp3 Pin", >> + .codec_name = "ehdaudio0D2", >> + .codec_dai_name = "intel-hdmi-hifi3", >> + .platform_name = "0000:00:1f.3", >> + .init = skl_hda_hdmi3_init, >> + .dpcm_playback = 1, >> + .no_pcm = 1, >> + }, >> +}; >> + >> +#define NAME_SIZE 32 >> +static int skl_hda_card_late_probe(struct snd_soc_card *card) >> +{ >> + struct skl_hda_private *ctx = snd_soc_card_get_drvdata(card); >> + struct skl_hda_hdmi_pcm *pcm; >> + struct snd_soc_codec *codec = NULL; >> + int err, i = 0; >> + char jack_name[NAME_SIZE]; >> + >> + list_for_each_entry(pcm, &ctx->hdmi_pcm_list, head) { >> + codec = pcm->codec_dai->codec; >> + snprintf(jack_name, sizeof(jack_name), >> + "HDMI/DP, pcm=%d Jack", pcm->device); >> + err = snd_soc_card_jack_new(card, jack_name, >> + SND_JACK_AVOUT, >&skl_hda_hdmi_jack[i], >> + NULL, 0); >> + >> + if (err) >> + return err; >> + >> + err = hdac_hdmi_jack_init(pcm->codec_dai, pcm->device, >> + &skl_hda_hdmi_jack[i]); >> + if (err < 0) >> + return err; >> + >> + i++; >> + } >> + >> + if (!codec) >> + return -EINVAL; >> + >> + return hdac_hdmi_jack_port_init(codec, &card->dapm); >> +} > >this one in always the same as well, it could be factored across machine >drivers. Okay, I will put this in the common file along with other functions that you mentioned previously. > >> + >> +static struct snd_soc_card skl_hda_audio_card = { >> + .name = "skl_hda_card", >> + .owner = THIS_MODULE, >> + .dai_link = skl_hda_dais, >> + .num_links = ARRAY_SIZE(skl_hda_dais), >> + .dapm_routes = skl_hda_map, >> + .num_dapm_routes = ARRAY_SIZE(skl_hda_map), >> + .fully_routed = true, >> + .late_probe = skl_hda_card_late_probe, >> +}; >> + >> +static int skl_hda_audio_probe(struct platform_device *pdev) >> +{ >> + struct skl_hda_private *ctx; >> + >> + dev_dbg(&pdev->dev, "%s: machine driver probe got called\n", >__func__); >> + >> + ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_ATOMIC); >> + if (!ctx) >> + return -ENOMEM; >> + >> + INIT_LIST_HEAD(&ctx->hdmi_pcm_list); >> + >> + skl_hda_audio_card.dev = &pdev->dev; >> + snd_soc_card_set_drvdata(&skl_hda_audio_card, ctx); >> + >> + return devm_snd_soc_register_card(&pdev->dev, >&skl_hda_audio_card); >> +} >> + >> +static const struct platform_device_id skl_hda_board_ids[] = { >> + { .name = "skl_hda_generic" }, >> + { } >> +}; >> + >> +static struct platform_driver skl_hda_audio = { >> + .probe = skl_hda_audio_probe, >> + .driver = { >> + .name = "skl_hda_generic", >> + .pm = &snd_soc_pm_ops, >> + }, >> + .id_table = skl_hda_board_ids, >> +}; >> + >> +module_platform_driver(skl_hda_audio) >> + >> +/* Module information */ >> +MODULE_DESCRIPTION("SKL/KBL HDA Generic Machine driver"); > >again it's not limited to SKL/KBL, it'll work on APL and CNL and GLK ... Yes, technically there is nothing limiting this to work on other platforms. It is just that right now I have only SKL/KBL in my test coverage for the current RFC series. I am hoping to get some initial feedback after which I will do send this patches for some formal preint for other platforms also before sending it. _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
