[alsa-devel] [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs
Ughreja, Rakesh A
rakesh.a.ughreja at intel.com
Tue Feb 27 04:45:04 CET 2018
>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart at linux.intel.com]
>Sent: Tuesday, February 27, 2018 12:25 AM
>To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; alsa-devel at alsa-
>project.org; broonie at kernel.org; tiwai at suse.de;
>liam.r.girdwood at linux.intel.com
>Cc: Koul, Vinod <vinod.koul at intel.com>; Patches Audio
><patches.audio at intel.com>
>Subject: Re: [alsa-devel] [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 at linux.intel.com]
>>> Sent: Friday, February 23, 2018 10:13 PM
>>> To: Ughreja, Rakesh A <rakesh.a.ughreja at intel.com>; alsa-devel at alsa-
>>> project.org; broonie at kernel.org; tiwai at suse.de;
>>> liam.r.girdwood at linux.intel.com
>>> Cc: Koul, Vinod <vinod.koul at intel.com>; Patches Audio
>>> <patches.audio at intel.com>
>>> 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 at intel.com>
>>>> ---
>>>> 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
More information about the Alsa-devel
mailing list