>-----Original Message----- >From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@xxxxxxxxxxxxxxx] >Sent: Tuesday, February 27, 2018 12:25 AM >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: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in >sst_acpi_mach for HDA codecs > >On 2/26/18 1:17 AM, Ughreja, Rakesh A wrote: >> >> >>> -----Original Message----- >>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@xxxxxxxxxxxxxxx] >>> Sent: Friday, February 23, 2018 10:13 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: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach >for >>> HDA codecs >>> >>> On 2/23/18 2:12 AM, Rakesh Ughreja wrote: >>>> When no I2S based codec entries are found in the BIOS, check if there are >>>> any HDA codecs detected on the bus. If there are two (i.e. iDisp + HDA) >>>> HDA codecs found on the bus, load the HDA machine driver. >>> >>> What if you have a headless device with no codec, that means no HDMI >>> support? Why is this restriction necessary? >> >> This machine driver handles HDA and iDisp combination only. >> >> If you want to handle only iDisp or only HDA or more than one HDA >> different machine driver needs to be written or this machine >> driver needs to be enhanced. >> >> Machine driver has BE DAI link information and so it is specific to those >> many devices for a given platform. > >You could pass a flag in the machine driver pdata stating if there is a >codec in addition to iDisp and add the relevant backends as needed. Yes, I can do that. Will fix it in the next revision. > >> >>> >>>> >>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@xxxxxxxxx> >>>> --- >>>> sound/soc/intel/skylake/skl.c | 59 >>> +++++++++++++++++++++++++++++++++++++++---- >>>> 1 file changed, 54 insertions(+), 5 deletions(-) >>>> >>>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c >>>> index f948f29..ac64416 100644 >>>> --- a/sound/soc/intel/skylake/skl.c >>>> +++ b/sound/soc/intel/skylake/skl.c >>>> @@ -442,6 +442,24 @@ static struct skl_ssp_clk skl_ssp_clks[] = { >>>> {.name = "ssp5_sclkfs"}, >>>> }; >>>> >>>> +static struct snd_soc_acpi_mach *skl_find_hda_machine(struct skl *skl, >>>> + struct snd_soc_acpi_mach >>> *machines) >>>> +{ >>>> + >>>> + struct snd_soc_acpi_mach *mach; >>>> + struct hdac_bus *bus = skl_to_bus(skl); >>>> + >>>> + /* check if we have two HDA codecs */ >>>> + if (hweight_long(bus->codec_mask) != 2) >>>> + return NULL; >>>> + >>>> + for (mach = machines; mach->id[0]; mach++) { >>>> + if (!strcmp(mach->id, "HDA_GEN")) >>>> + return mach; >>>> + } >>> >>> that's not really testing if there are actual HDaudio devices, is this >>> loop just there to point to a firmware file? >> >> There is a check for number of codecs in the previous line. >> By default the iDisp codec would always be present so the check >> is for count 2. > >I was referring to the for(mach= loop. I am not sure what the test on >"HDA_GEN" actually does beyond providing information for the platform >driver. Right, it just finds the entry and provides information to platform driver. > >> >>> >>>> + return NULL; >>>> +} >>>> + >>>> static int skl_find_machine(struct skl *skl, void *driver_data) >>>> { >>>> struct hdac_bus *bus = skl_to_bus(skl); >>>> @@ -450,8 +468,12 @@ static int skl_find_machine(struct skl *skl, void >>> *driver_data) >>>> >>>> mach = snd_soc_acpi_find_machine(mach); >>>> if (mach == NULL) { >>>> - dev_err(bus->dev, "No matching machine driver found\n"); >>>> - return -ENODEV; >>>> + dev_dbg(bus->dev, "No matching I2S machine driver found\n"); >>>> + mach = skl_find_hda_machine(skl, driver_data); >>>> + if (mach == NULL) { >>>> + dev_err(bus->dev, "No matching machine driver >>> found\n"); >>>> + return -ENODEV; >>>> + } >>>> } >>>> >>>> skl->mach = mach; >>>> @@ -466,8 +488,9 @@ static int skl_find_machine(struct skl *skl, void >>> *driver_data) >>>> >>>> static int skl_machine_device_register(struct skl *skl) >>>> { >>>> - struct hdac_bus *bus = skl_to_bus(skl); >>>> struct snd_soc_acpi_mach *mach = skl->mach; >>>> + struct hdac_bus *bus = skl_to_bus(skl); >>>> + struct skl_machine_pdata *pdata; >>>> struct platform_device *pdev; >>>> int ret; >>>> >>>> @@ -484,8 +507,11 @@ static int skl_machine_device_register(struct skl >*skl) >>>> return -EIO; >>>> } >>>> >>>> - if (mach->pdata) >>>> + if (mach->pdata) { >>>> + pdata = (struct skl_machine_pdata *)mach->pdata; >>>> + pdata->platform = dev_name(bus->dev); >>>> dev_set_drvdata(&pdev->dev, mach->pdata); >>>> + } >>>> >>>> skl->i2s_dev = pdev; >>>> >>>> @@ -1030,6 +1056,14 @@ static struct snd_soc_acpi_mach >sst_skl_devdata[] >>> = { >>>> .quirk_data = &skl_codecs, >>>> .pdata = &skl_dmic_data >>>> }, >>>> + { >>>> + .id = "HDA_GEN", >>>> + .drv_name = "skl_hda_generic", >>>> + .fw_filename = "intel/dsp_fw_release.bin", >>>> + .machine_quirk = NULL, >>>> + .quirk_data = NULL, >>>> + .pdata = &cnl_pdata, >>> >>> this is odd, the cnl_pdata says the topology_pcm is used, I thought this >>> was not applicable for SKL/KBL. Or put differently, why is this used for >>> the hda case only? >> >> This flag is used for using the topology framework for creating the >> FE DAIs and DAI Links. The other I2S machine drivers which are >> up-streamed some time back don't have support for creating >> DAIs and DAI Links from topology file. Based on your comments >> given in the RFC, I have added this support for HDA machine drivers >> even for SKL, KBL also. > >What prevents us from updating the machine drivers to use the topology >in all cases and avoid differences across platforms? I understand that >some early versions made a choice due to schedule, but for upstream this >can be realigned. Certainly that can be done. But that is completely independent work. That work has no dependency on this patch series. Regards, Rakesh _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
