diff mbox series

[05/20] ASoC: SOF: Intel: hda: report SSP link mask to machine driver

Message ID 20220308192610.392950-6-pierre-louis.bossart@linux.intel.com
State Accepted
Commit bd015f633b05a3d4f88a3d7099746b2819a523f5
Headers show
Series ALSA/ASoC/SOF/Intel: improve support for ES8336-based platforms | expand

Commit Message

Pierre-Louis Bossart March 8, 2022, 7:25 p.m. UTC 
For devices designed for Windows, the SSP information should be listed
in the NHLT, and when present can be used to set quirks automatically
in the machine driver.

The NHLT information exposes BT and analog audio connections
separately, for now we are only interested in the analog audio parts.

The use of dev_info() for the SSP mask is intentional so that we can
immediately flag devices with an ES8336 codec. Since NHLT is not used
for recent Chromebooks these messages should be rare.

Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Péter Ujfalusi <peter.ujfalusi@linux.intel.com>
---
 sound/soc/sof/intel/hda.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Cezary Rojewski March 9, 2022, 4:59 p.m. UTC | #1 
On 2022-03-08 8:25 PM, Pierre-Louis Bossart wrote:
> For devices designed for Windows, the SSP information should be listed
> in the NHLT, and when present can be used to set quirks automatically
> in the machine driver.
> 
> The NHLT information exposes BT and analog audio connections
> separately, for now we are only interested in the analog audio parts.
> 
> The use of dev_info() for the SSP mask is intentional so that we can
> immediately flag devices with an ES8336 codec. Since NHLT is not used
> for recent Chromebooks these messages should be rare.


...

> +static int check_nhlt_ssp_mask(struct snd_sof_dev *sdev)
> +{
> +	struct nhlt_acpi_table *nhlt;
> +	int ssp_mask = 0;
> +
> +	nhlt = intel_nhlt_init(sdev->dev);
> +	if (!nhlt)
> +		return ssp_mask;
> +
> +	if (intel_nhlt_has_endpoint_type(nhlt, NHLT_LINK_SSP)) {
> +		ssp_mask = intel_nhlt_ssp_endpoint_mask(nhlt, NHLT_DEVICE_I2S);
> +		if (ssp_mask)
> +			dev_info(sdev->dev, "NHLT_DEVICE_I2S detected, ssp_mask %#x\n", ssp_mask);
> +	}
> +	intel_nhlt_free(nhlt);


NHLT "toggling" found in this function looks weird. Why not cache NHLT 
pointer i.e.: get it once and put when driver is no longer required? 
Initializing and freeing NHLT (AKA get/put ACPI table) every time a 
request is made does not look like an optimal solution.

> +	return ssp_mask;
> +}
> +
Pierre-Louis Bossart March 9, 2022, 5:16 p.m. UTC | #2 
On 3/9/22 10:59, Cezary Rojewski wrote:
> On 2022-03-08 8:25 PM, Pierre-Louis Bossart wrote:
>> For devices designed for Windows, the SSP information should be listed
>> in the NHLT, and when present can be used to set quirks automatically
>> in the machine driver.
>>
>> The NHLT information exposes BT and analog audio connections
>> separately, for now we are only interested in the analog audio parts.
>>
>> The use of dev_info() for the SSP mask is intentional so that we can
>> immediately flag devices with an ES8336 codec. Since NHLT is not used
>> for recent Chromebooks these messages should be rare.
> 
> 
> ...
> 
>> +static int check_nhlt_ssp_mask(struct snd_sof_dev *sdev)
>> +{
>> +    struct nhlt_acpi_table *nhlt;
>> +    int ssp_mask = 0;
>> +
>> +    nhlt = intel_nhlt_init(sdev->dev);
>> +    if (!nhlt)
>> +        return ssp_mask;
>> +
>> +    if (intel_nhlt_has_endpoint_type(nhlt, NHLT_LINK_SSP)) {
>> +        ssp_mask = intel_nhlt_ssp_endpoint_mask(nhlt, NHLT_DEVICE_I2S);
>> +        if (ssp_mask)
>> +            dev_info(sdev->dev, "NHLT_DEVICE_I2S detected, ssp_mask 
>> %#x\n", ssp_mask);
>> +    }
>> +    intel_nhlt_free(nhlt);
> 
> 
> NHLT "toggling" found in this function looks weird. Why not cache NHLT 
> pointer i.e.: get it once and put when driver is no longer required? 
> Initializing and freeing NHLT (AKA get/put ACPI table) every time a 
> request is made does not look like an optimal solution.

I agree with your remark, but this is an optimization that we plan on 
doing later. There are other changes coming wrt to NHTL to extract DMIC 
blobs, and it's better to change all the functions using the same 
programming flow when we are in 'stable' state.

The concern isn't really optimization at this point but just to get 
audio to work. Keep in mind all this patchset was generated with tests 
crowdsourced to the community, and the empirical detection of the 
SSP-codec link could be broken on some platforms - the NHLT does not 
give *any* information on where the codec is actually connected.
diff mbox series

Patch

diff --git a/sound/soc/sof/intel/hda.c b/sound/soc/sof/intel/hda.c
index 711d14a821bb..eebb3b318d79 100644
--- a/sound/soc/sof/intel/hda.c
+++ b/sound/soc/sof/intel/hda.c
@@ -669,6 +669,25 @@  static int check_dmic_num(struct snd_sof_dev *sdev)
 	return dmic_num;
 }
 
+static int check_nhlt_ssp_mask(struct snd_sof_dev *sdev)
+{
+	struct nhlt_acpi_table *nhlt;
+	int ssp_mask = 0;
+
+	nhlt = intel_nhlt_init(sdev->dev);
+	if (!nhlt)
+		return ssp_mask;
+
+	if (intel_nhlt_has_endpoint_type(nhlt, NHLT_LINK_SSP)) {
+		ssp_mask = intel_nhlt_ssp_endpoint_mask(nhlt, NHLT_DEVICE_I2S);
+		if (ssp_mask)
+			dev_info(sdev->dev, "NHLT_DEVICE_I2S detected, ssp_mask %#x\n", ssp_mask);
+	}
+	intel_nhlt_free(nhlt);
+
+	return ssp_mask;
+}
+
 #if IS_ENABLED(CONFIG_SND_SOC_SOF_HDA) || IS_ENABLED(CONFIG_SND_SOC_SOF_INTEL_SOUNDWIRE)
 
 static const char *fixup_tplg_name(struct snd_sof_dev *sdev,
@@ -1391,6 +1410,9 @@  struct snd_soc_acpi_mach *hda_machine_select(struct snd_sof_dev *sdev)
 			mach->mach_params.links = mach->links;
 			mach->mach_params.link_mask = mach->link_mask;
 		}
+
+		/* report SSP link mask to machine driver */
+		mach->mach_params.i2s_link_mask = check_nhlt_ssp_mask(sdev);
 	}
 
 	/*