[RFC,02/10] ASoC: Intel: Skylake: Add entry in sst_acpi_mach for HDA codecs
diff mbox

Message ID 1512119648-2700-3-git-send-email-rakesh.a.ughreja@intel.com
State New
Headers show

Commit Message

Ughreja, Rakesh A Dec. 1, 2017, 9:14 a.m. UTC
When no I2S based codecs are detected in the BIOS, check if there are
any HDA codecs present. If yes, load the corresponding machine driver.

TODO:
support for detecting the presence of HDA codec is not implemented.
it will be implemented in the next revision.

Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
---
 sound/soc/intel/skylake/skl.c | 46 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 43 insertions(+), 3 deletions(-)

Comments

Pierre-Louis Bossart Dec. 1, 2017, 6:15 p.m. UTC | #1
On 12/1/17 3:14 AM, Rakesh Ughreja wrote:
> When no I2S based codecs are detected in the BIOS, check if there are
> any HDA codecs present. If yes, load the corresponding machine driver.

Is this completely correct?
I may be splitting hair but I believe it's possible to have an HDAudio 
codec connected to some pins and an I2S/PDM device on non-conflicting 
pins. The PDM case is very real and isn't related to I2S

You may also want to explain that you *have* to look for non-HDaudio 
stuff first, because otherwise the iDisp codec will be detected but that 
is not sufficient to load the HDaudio machine driver (could be 
iDisp+I2S+PDM)

> 
> TODO:
> support for detecting the presence of HDA codec is not implemented.
> it will be implemented in the next revision.
> 
> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
> ---
>   sound/soc/intel/skylake/skl.c | 46 ++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 43 insertions(+), 3 deletions(-)
> 
> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
> index 27462c0..568a285 100644
> --- a/sound/soc/intel/skylake/skl.c
> +++ b/sound/soc/intel/skylake/skl.c
> @@ -424,6 +424,26 @@ static int skl_free(struct hdac_bus *bus)
>   	return 0;
>   }
>   
> +static struct snd_soc_acpi_mach *skl_probe_hda_machine(
> +					struct snd_soc_acpi_mach *machines)
> +{
> +
> +	struct snd_soc_acpi_mach *mach;
> +
> +	/*
> +	 * FIXME:
> +	 * First check if there are any HDA codecs present on the system
> +	 * then search the match table.
> +	 * For now this function is not detecting the presence of any
> +	 * HDA codecs.

D'oh. so how was this patchset tested then?

> +	 */
> +	for (mach = machines; mach->id[0]; mach++) {
> +		if (!strcmp(mach->id, "HDA_GEN"))

Do you mean to say this was a temporary hack assuming a modified DSDT to 
force the detection of the codec?

And while I am at it in the hair-splitting department, in theory HDAudio 
can work with multiple codecs (each using its own SDI line), did you 
think of this case and how would it impact the machine driver selection?

> +			return mach;
> +	}
> +	return NULL;
> +}
> +
>   static int skl_machine_device_register(struct skl *skl, void *driver_data)
>   {
>   	struct hdac_bus *bus = skl_to_bus(skl);
> @@ -433,9 +453,14 @@ static int skl_machine_device_register(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_probe_hda_machine(driver_data);
> +		if (mach == NULL) {
> +			dev_err(bus->dev, "No matching machine driver found\n");
> +			return -ENODEV;
> +		}
>   	}
> +
>   	skl->fw_name = mach->fw_filename;
>   
>   	pdev = platform_device_alloc(mach->drv_name, -1);
> @@ -911,6 +936,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 = NULL
> +	},
>   	{}
>   };
>   
> @@ -973,7 +1006,14 @@ static struct snd_soc_acpi_mach sst_kbl_devdata[] = {
>   		.drv_name = "kbl_rt5663",
>   		.fw_filename = "intel/dsp_fw_kbl.bin",
>   	},
> -
> +	{
> +		.id = "HDA_GEN",
> +		.drv_name = "skl_hda_generic",
> +		.fw_filename = "intel/dsp_fw_kbl.bin",
> +		.machine_quirk = NULL,
> +		.quirk_data = NULL,
> +		.pdata = NULL
> +	},
>   	{}
>   };
>   
>
Ughreja, Rakesh A Dec. 4, 2017, 4:27 p.m. UTC | #2
>-----Original Message-----
>From: Pierre-Louis Bossart [mailto:pierre-louis.bossart@linux.intel.com]
>Sent: Friday, December 1, 2017 11:46 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: [alsa-devel] [RFC 02/10] ASoC: Intel: Skylake: Add entry in
>sst_acpi_mach for HDA codecs
>
>On 12/1/17 3:14 AM, Rakesh Ughreja wrote:
>> When no I2S based codecs are detected in the BIOS, check if there are
>> any HDA codecs present. If yes, load the corresponding machine driver.
>
>Is this completely correct?
>I may be splitting hair but I believe it's possible to have an HDAudio
>codec connected to some pins and an I2S/PDM device on non-conflicting
>pins. The PDM case is very real and isn't related to I2S

Yes, agree. In that case there should be different Machine driver.
I expected the skl_hda_generic machine driver to be loaded only when
we have iDisp and HDA codecs and nothing else.

>
>You may also want to explain that you *have* to look for non-HDaudio
>stuff first, because otherwise the iDisp codec will be detected but that
>is not sufficient to load the HDaudio machine driver (could be
>iDisp+I2S+PDM)

Yes, I have done the same thing in this patch. You can see that the
HDA_GEN entry is the last one.

>
>>
>> TODO:
>> support for detecting the presence of HDA codec is not implemented.
>> it will be implemented in the next revision.
>>
>> Signed-off-by: Rakesh Ughreja <rakesh.a.ughreja@intel.com>
>> ---
>>   sound/soc/intel/skylake/skl.c | 46
>++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 43 insertions(+), 3 deletions(-)
>>
>> diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
>> index 27462c0..568a285 100644
>> --- a/sound/soc/intel/skylake/skl.c
>> +++ b/sound/soc/intel/skylake/skl.c
>> @@ -424,6 +424,26 @@ static int skl_free(struct hdac_bus *bus)
>>   	return 0;
>>   }
>>
>> +static struct snd_soc_acpi_mach *skl_probe_hda_machine(
>> +					struct snd_soc_acpi_mach *machines)
>> +{
>> +
>> +	struct snd_soc_acpi_mach *mach;
>> +
>> +	/*
>> +	 * FIXME:
>> +	 * First check if there are any HDA codecs present on the system
>> +	 * then search the match table.
>> +	 * For now this function is not detecting the presence of any
>> +	 * HDA codecs.
>
>D'oh. so how was this patchset tested then?

I have tested this patchset on Dell XPS13 machine, where I have
only one HDA codec and one iDisp codec. The current code
does not find the match for any of the I2S codec machine
driver and so the default HDA_GEN machine driver gets loaded.

Once the FIXME is implemented HDA_GEN machine driver will
be loaded only if there is a HDA and iDisp codec present, else
no machine driver will be loaded.

>
>> +	 */
>> +	for (mach = machines; mach->id[0]; mach++) {
>> +		if (!strcmp(mach->id, "HDA_GEN"))
>
>Do you mean to say this was a temporary hack assuming a modified DSDT to
>force the detection of the codec?

There is a FIXME on the top, where I would like to detect if we
really have only HDA and iDisp codecs present on the system.

>
>And while I am at it in the hair-splitting department, in theory HDAudio
>can work with multiple codecs (each using its own SDI line), did you
>think of this case and how would it impact the machine driver selection?

As long as we have only HDA and iDisp codecs, the current machine
driver can scale. You will need more BE DAI Links in the machine driver
to bind the codecs.

Patch
diff mbox

diff --git a/sound/soc/intel/skylake/skl.c b/sound/soc/intel/skylake/skl.c
index 27462c0..568a285 100644
--- a/sound/soc/intel/skylake/skl.c
+++ b/sound/soc/intel/skylake/skl.c
@@ -424,6 +424,26 @@  static int skl_free(struct hdac_bus *bus)
 	return 0;
 }
 
+static struct snd_soc_acpi_mach *skl_probe_hda_machine(
+					struct snd_soc_acpi_mach *machines)
+{
+
+	struct snd_soc_acpi_mach *mach;
+
+	/*
+	 * FIXME:
+	 * First check if there are any HDA codecs present on the system
+	 * then search the match table.
+	 * For now this function is not detecting the presence of any
+	 * HDA codecs.
+	 */
+	for (mach = machines; mach->id[0]; mach++) {
+		if (!strcmp(mach->id, "HDA_GEN"))
+			return mach;
+	}
+	return NULL;
+}
+
 static int skl_machine_device_register(struct skl *skl, void *driver_data)
 {
 	struct hdac_bus *bus = skl_to_bus(skl);
@@ -433,9 +453,14 @@  static int skl_machine_device_register(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_probe_hda_machine(driver_data);
+		if (mach == NULL) {
+			dev_err(bus->dev, "No matching machine driver found\n");
+			return -ENODEV;
+		}
 	}
+
 	skl->fw_name = mach->fw_filename;
 
 	pdev = platform_device_alloc(mach->drv_name, -1);
@@ -911,6 +936,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 = NULL
+	},
 	{}
 };
 
@@ -973,7 +1006,14 @@  static struct snd_soc_acpi_mach sst_kbl_devdata[] = {
 		.drv_name = "kbl_rt5663",
 		.fw_filename = "intel/dsp_fw_kbl.bin",
 	},
-
+	{
+		.id = "HDA_GEN",
+		.drv_name = "skl_hda_generic",
+		.fw_filename = "intel/dsp_fw_kbl.bin",
+		.machine_quirk = NULL,
+		.quirk_data = NULL,
+		.pdata = NULL
+	},
 	{}
 };