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/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?

> 
> 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?

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

> +	},
>   	{}
>   };
>   
> @@ -1046,6 +1080,14 @@ static struct snd_soc_acpi_mach sst_bxtp_devdata[] = {
>   		.machine_quirk = snd_soc_acpi_codec_list,
>   		.quirk_data = &bxt_codecs,
>   	},
> +	{
> +		.id = "HDA_GEN",
> +		.drv_name = "skl_hda_generic",
> +		.fw_filename = "intel/dsp_fw_bxtn.bin",
> +		.machine_quirk = NULL,
> +		.quirk_data = NULL,
> +		.pdata = &cnl_pdata,
> +	},
>   	{}
>   };
>   
> @@ -1100,7 +1142,14 @@ static struct snd_soc_acpi_mach sst_kbl_devdata[] = {
>   		.quirk_data = &kbl_7219_98357_codecs,
>   		.pdata = &skl_dmic_data
>   	},
> -
> +	{
> +		.id = "HDA_GEN",
> +		.drv_name = "skl_hda_generic",
> +		.fw_filename = "intel/dsp_fw_kbl.bin",
> +		.machine_quirk = NULL,
> +		.quirk_data = NULL,
> +		.pdata = &cnl_pdata,
> +	},
>   	{}
>   };
>   
>