Re: [PATCH v1 2/9] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs

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

On 2/26/18 1:17 AM, Ughreja, Rakesh A wrote:
> 
> 
>> -----Original Message-----
>> From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>> Sent: Friday, February 23, 2018 10:13 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: [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.

> 
>>
>>>
>>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@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.

> 
>>
>>> +	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.

> 
> Regards,
> Rakesh
>