[PATCH v1] ASoC: Intel: cirrus-common: Use UID to map correct amp to prefix

[PATCH v1] ASoC: Intel: cirrus-common: Use UID to map correct amp to prefix

[Stefan Binding]

Since the order of the amps in the ACPI determines the device name,
and the ACPI order may change depending on hardware configuration,
use UID to dynamically compute the dai links, allowing dynamic
assignment of the name_prefix.

Signed-off-by: Stefan Binding <sbinding@opensource.cirrus.com>
Signed-off-by: Vitaly Rodionov <vitalyr@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_cirrus_common.c | 73 +++++++++-------------
 1 file changed, 31 insertions(+), 42 deletions(-)

diff --git a/sound/soc/intel/boards/sof_cirrus_common.c b/sound/soc/intel/boards/sof_cirrus_common.c
index f4192df962d6..a96bc70a34d7 100644
--- a/sound/soc/intel/boards/sof_cirrus_common.c
+++ b/sound/soc/intel/boards/sof_cirrus_common.c
@@ -10,6 +10,9 @@
 #include "../../codecs/cs35l41.h"
 #include "sof_cirrus_common.h"
 
+#define CS35L41_HID "CSC3541"
+#define CS35L41_MAX_AMPS 4
+
 /*
  * Cirrus Logic CS35L41/CS35L53
  */
@@ -35,50 +38,12 @@ static const struct snd_soc_dapm_route cs35l41_dapm_routes[] = {
 	{"TR Spk", NULL, "TR SPK"},
 };
 
-static struct snd_soc_dai_link_component cs35l41_components[] = {
-	{
-		.name = CS35L41_DEV0_NAME,
-		.dai_name = CS35L41_CODEC_DAI,
-	},
-	{
-		.name = CS35L41_DEV1_NAME,
-		.dai_name = CS35L41_CODEC_DAI,
-	},
-	{
-		.name = CS35L41_DEV2_NAME,
-		.dai_name = CS35L41_CODEC_DAI,
-	},
-	{
-		.name = CS35L41_DEV3_NAME,
-		.dai_name = CS35L41_CODEC_DAI,
-	},
-};
+static struct snd_soc_dai_link_component cs35l41_components[CS35L41_MAX_AMPS];
 
 /*
  * Mapping between ACPI instance id and speaker position.
- *
- * Four speakers:
- *         0: Tweeter left, 1: Woofer left
- *         2: Tweeter right, 3: Woofer right
  */
-static struct snd_soc_codec_conf cs35l41_codec_conf[] = {
-	{
-		.dlc = COMP_CODEC_CONF(CS35L41_DEV0_NAME),
-		.name_prefix = "TL",
-	},
-	{
-		.dlc = COMP_CODEC_CONF(CS35L41_DEV1_NAME),
-		.name_prefix = "WL",
-	},
-	{
-		.dlc = COMP_CODEC_CONF(CS35L41_DEV2_NAME),
-		.name_prefix = "TR",
-	},
-	{
-		.dlc = COMP_CODEC_CONF(CS35L41_DEV3_NAME),
-		.name_prefix = "WR",
-	},
-};
+static struct snd_soc_codec_conf cs35l41_codec_conf[CS35L41_MAX_AMPS];
 
 static int cs35l41_init(struct snd_soc_pcm_runtime *rtd)
 {
@@ -117,10 +82,10 @@ static int cs35l41_init(struct snd_soc_pcm_runtime *rtd)
 static const struct {
 	unsigned int rx[2];
 } cs35l41_channel_map[] = {
-	{.rx = {0, 1}}, /* TL */
 	{.rx = {0, 1}}, /* WL */
-	{.rx = {1, 0}}, /* TR */
 	{.rx = {1, 0}}, /* WR */
+	{.rx = {0, 1}}, /* TL */
+	{.rx = {1, 0}}, /* TR */
 };
 
 static int cs35l41_hw_params(struct snd_pcm_substream *substream,
@@ -175,8 +140,32 @@ static const struct snd_soc_ops cs35l41_ops = {
 	.hw_params = cs35l41_hw_params,
 };
 
+static const char * const cs35l41_name_prefixes[] = { "WL", "WR", "TL", "TR" };
+
+static const char * const cs35l41_uid_strings[] = { "0", "1", "2", "3" };
+
+static void cs35l41_compute_codec_conf(void)
+{
+	int uid;
+	struct acpi_device *adev;
+	struct device *physdev;
+
+	for (uid = 0; uid < CS35L41_MAX_AMPS; uid++) {
+		adev = acpi_dev_get_first_match_dev(CS35L41_HID, cs35l41_uid_strings[uid], -1);
+		if (!adev)
+			return;
+		physdev = get_device(acpi_get_first_physical_node(adev));
+		cs35l41_components[uid].name = dev_name(physdev);
+		cs35l41_components[uid].dai_name = CS35L41_CODEC_DAI;
+		cs35l41_codec_conf[uid].dlc.name = dev_name(physdev);
+		cs35l41_codec_conf[uid].name_prefix = cs35l41_name_prefixes[uid];
+		acpi_dev_put(adev);
+	}
+}
+
 void cs35l41_set_dai_link(struct snd_soc_dai_link *link)
 {
+	cs35l41_compute_codec_conf();
 	link->codecs = cs35l41_components;
 	link->num_codecs = ARRAY_SIZE(cs35l41_components);
 	link->init = cs35l41_init;
-- 
2.34.1

Re: [PATCH v1] ASoC: Intel: cirrus-common: Use UID to map correct amp to prefix

[Pierre-Louis Bossart]

On 7/26/22 08:46, Stefan Binding wrote:
> Since the order of the amps in the ACPI determines the device name,
> and the ACPI order may change depending on hardware configuration,
> use UID to dynamically compute the dai links, allowing dynamic
> assignment of the name_prefix.

This is interesting, I didn't even know what this _UID thingy could be
used for. ACPI is the gift that keeps on giving after 30 years, eh?

I think you should add an explanation on what this _UID is, nothing says
it actually needs to be an integer, I see e.g. this sort of 'id' in
various DSDT

Name (_UID, Zero)  // _UID: Unique ID
Name (_UID, 0x05)  // _UID: Unique ID
Name (_UID, "SerialIoUart0")  // _UID: Unique ID
Name (_UID, "PCHRESV")  // _UID: Unique ID
Name (_UID, "IoTraps")  // _UID: Unique ID
Name (_UID, "SADDLESTRING")  // _UID: Unique ID

and my favorite

Name (_UID, "TestDev")  // _UID: Unique ID


>  /*
>   * Mapping between ACPI instance id and speaker position.
> - *
> - * Four speakers:
> - *         0: Tweeter left, 1: Woofer left
> - *         2: Tweeter right, 3: Woofer right
>   */
> -static struct snd_soc_codec_conf cs35l41_codec_conf[] = {
> -	{
> -		.dlc = COMP_CODEC_CONF(CS35L41_DEV0_NAME),
> -		.name_prefix = "TL",
> -	},
> -	{
> -		.dlc = COMP_CODEC_CONF(CS35L41_DEV1_NAME),
> -		.name_prefix = "WL",
> -	},
> -	{
> -		.dlc = COMP_CODEC_CONF(CS35L41_DEV2_NAME),
> -		.name_prefix = "TR",
> -	},
> -	{
> -		.dlc = COMP_CODEC_CONF(CS35L41_DEV3_NAME),
> -		.name_prefix = "WR",
> -	},
> -};
> +static struct snd_soc_codec_conf cs35l41_codec_conf[CS35L41_MAX_AMPS];
>  
>  static int cs35l41_init(struct snd_soc_pcm_runtime *rtd)
>  {
> @@ -117,10 +82,10 @@ static int cs35l41_init(struct snd_soc_pcm_runtime *rtd)
>  static const struct {
>  	unsigned int rx[2];
>  } cs35l41_channel_map[] = {
> -	{.rx = {0, 1}}, /* TL */
>  	{.rx = {0, 1}}, /* WL */
> -	{.rx = {1, 0}}, /* TR */
>  	{.rx = {1, 0}}, /* WR */
> +	{.rx = {0, 1}}, /* TL */
> +	{.rx = {1, 0}}, /* TR */
>  };
>  
>  static int cs35l41_hw_params(struct snd_pcm_substream *substream,
> @@ -175,8 +140,32 @@ static const struct snd_soc_ops cs35l41_ops = {
>  	.hw_params = cs35l41_hw_params,
>  };
>  
> +static const char * const cs35l41_name_prefixes[] = { "WL", "WR", "TL", "TR" };
> +
> +static const char * const cs35l41_uid_strings[] = { "0", "1", "2", "3" };

I must admit not understanding why you changed the order.

I vaguely recall Brent Lu added this on TL, WL, TR, WR order on purpose
and that it matches the order in the SOF topology. Brent, can you please
comment on this?

I don't really care about the order selected, just want to make sure we
don't introduce a channel swap with what the firmware does.

> +static void cs35l41_compute_codec_conf(void)
> +{
> +	int uid;
> +	struct acpi_device *adev;
> +	struct device *physdev;
> +
> +	for (uid = 0; uid < CS35L41_MAX_AMPS; uid++) {
> +		adev = acpi_dev_get_first_match_dev(CS35L41_HID, cs35l41_uid_strings[uid], -1);
> +		if (!adev)
> +			return;

shouldn't you log an error or something telling the user that their DSDT
configuration is incorrect?

If I understand the code above, is the expectation that the UID expected
in the DSDT should be:

Name (_UID, "0")  // _UID: Unique ID for WL
Name (_UID, "1")  // _UID: Unique ID for WR
Name (_UID, "2")  // _UID: Unique ID for TL
Name (_UID, "3")  // _UID: Unique ID for TR

Is yes that's worthy of a comment for future generations.

> +		physdev = get_device(acpi_get_first_physical_node(adev));
> +		cs35l41_components[uid].name = dev_name(physdev);
> +		cs35l41_components[uid].dai_name = CS35L41_CODEC_DAI;
> +		cs35l41_codec_conf[uid].dlc.name = dev_name(physdev);
> +		cs35l41_codec_conf[uid].name_prefix = cs35l41_name_prefixes[uid];
> +		acpi_dev_put(adev);
> +	}
> +}
> +
>  void cs35l41_set_dai_link(struct snd_soc_dai_link *link)
>  {
> +	cs35l41_compute_codec_conf();
>  	link->codecs = cs35l41_components;
>  	link->num_codecs = ARRAY_SIZE(cs35l41_components);
>  	link->init = cs35l41_init;

RE: [PATCH v1] ASoC: Intel: cirrus-common: Use UID to map correct amp to prefix

[Lu, Brent]

> >
> >  static int cs35l41_init(struct snd_soc_pcm_runtime *rtd)  { @@
> > -117,10 +82,10 @@ static int cs35l41_init(struct snd_soc_pcm_runtime
> > *rtd)  static const struct {
> >  	unsigned int rx[2];
> >  } cs35l41_channel_map[] = {
> > -	{.rx = {0, 1}}, /* TL */
> >  	{.rx = {0, 1}}, /* WL */
> > -	{.rx = {1, 0}}, /* TR */
> >  	{.rx = {1, 0}}, /* WR */
> > +	{.rx = {0, 1}}, /* TL */
> > +	{.rx = {1, 0}}, /* TR */
> >  };
> >
> >  static int cs35l41_hw_params(struct snd_pcm_substream *substream,
> @@
> > -175,8 +140,32 @@ static const struct snd_soc_ops cs35l41_ops = {
> >  	.hw_params = cs35l41_hw_params,
> >  };
> >
> > +static const char * const cs35l41_name_prefixes[] = { "WL", "WR",
> > +"TL", "TR" };
> > +
> > +static const char * const cs35l41_uid_strings[] = { "0", "1", "2",
> > +"3" };
> 
> I must admit not understanding why you changed the order.
> 
> I vaguely recall Brent Lu added this on TL, WL, TR, WR order on purpose and
> that it matches the order in the SOF topology. Brent, can you please
> comment on this?
> 
> I don't really care about the order selected, just want to make sure we don't
> introduce a channel swap with what the firmware does.
> 
The order here does not related to SOF topology or firmware. This is smart amp
so the SSP port is using I2S format to send data.

The order TL/WL/TR/WR is to match the enumeration order in SSDT table. Since
this patch is using UID to specify the amplifier, the order change should be fine.


Brent