[PATCH 01/12] ASoC: intel: sof_sdw: Printk's should end with a newline

[PATCH 01/12] ASoC: intel: sof_sdw: Printk's should end with a newline

[Charles Keepax]

Add the missing new lines.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index fd27e211211bd..8f3329dcf7062 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -524,7 +524,7 @@ int sdw_prepare(struct snd_pcm_substream *substream)
 
 	sdw_stream = snd_soc_dai_get_stream(dai, substream->stream);
 	if (IS_ERR(sdw_stream)) {
-		dev_err(rtd->dev, "no stream found for DAI %s", dai->name);
+		dev_err(rtd->dev, "no stream found for DAI %s\n", dai->name);
 		return PTR_ERR(sdw_stream);
 	}
 
@@ -543,7 +543,7 @@ int sdw_trigger(struct snd_pcm_substream *substream, int cmd)
 
 	sdw_stream = snd_soc_dai_get_stream(dai, substream->stream);
 	if (IS_ERR(sdw_stream)) {
-		dev_err(rtd->dev, "no stream found for DAI %s", dai->name);
+		dev_err(rtd->dev, "no stream found for DAI %s\n", dai->name);
 		return PTR_ERR(sdw_stream);
 	}
 
@@ -565,7 +565,7 @@ int sdw_trigger(struct snd_pcm_substream *substream, int cmd)
 	}
 
 	if (ret)
-		dev_err(rtd->dev, "%s trigger %d failed: %d", __func__, cmd, ret);
+		dev_err(rtd->dev, "%s trigger %d failed: %d\n", __func__, cmd, ret);
 
 	return ret;
 }
@@ -630,7 +630,7 @@ int sdw_hw_free(struct snd_pcm_substream *substream)
 
 	sdw_stream = snd_soc_dai_get_stream(dai, substream->stream);
 	if (IS_ERR(sdw_stream)) {
-		dev_err(rtd->dev, "no stream found for DAI %s", dai->name);
+		dev_err(rtd->dev, "no stream found for DAI %s\n", dai->name);
 		return PTR_ERR(sdw_stream);
 	}
 
@@ -1339,7 +1339,7 @@ static int get_slave_info(const struct snd_soc_acpi_link_adr *adr_link,
 			return -EINVAL;
 
 		if (index >= SDW_MAX_CPU_DAIS) {
-			dev_err(dev, " cpu_dai_id array overflows");
+			dev_err(dev, "cpu_dai_id array overflows\n");
 			return -EINVAL;
 		}
 
@@ -1490,7 +1490,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 				return -ENOMEM;
 
 			if (cpu_dai_index >= sdw_cpu_dai_num) {
-				dev_err(dev, "invalid cpu dai index %d",
+				dev_err(dev, "invalid cpu dai index %d\n",
 					cpu_dai_index);
 				return -EINVAL;
 			}
@@ -1503,12 +1503,12 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		 * not be larger than sdw_be_num
 		 */
 		if (*link_index >= sdw_be_num) {
-			dev_err(dev, "invalid dai link index %d", *link_index);
+			dev_err(dev, "invalid dai link index %d\n", *link_index);
 			return -EINVAL;
 		}
 
 		if (*cpu_id >= sdw_cpu_dai_num) {
-			dev_err(dev, " invalid cpu dai index %d", *cpu_id);
+			dev_err(dev, "invalid cpu dai index %d\n", *cpu_id);
 			return -EINVAL;
 		}
 
@@ -1531,7 +1531,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		ret = set_codec_init_func(card, adr_link, dai_links + (*link_index)++,
 					  playback, group_id, adr_index, dai_index);
 		if (ret < 0) {
-			dev_err(dev, "failed to init codec %d", codec_index);
+			dev_err(dev, "failed to init codec %d\n", codec_index);
 			return ret;
 		}
 
@@ -1675,7 +1675,7 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
 
 			endpoint = adr_link->adr_d[i].endpoints;
 			if (endpoint->aggregated && !endpoint->group_id) {
-				dev_err(dev, "invalid group id on link %x",
+				dev_err(dev, "invalid group id on link %x\n",
 					adr_link->mask);
 				continue;
 			}
@@ -1698,7 +1698,7 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
 							 &be_id, &codec_conf_index,
 							 &ignore_pch_dmic, append_dai_type, i, j);
 				if (ret < 0) {
-					dev_err(dev, "failed to create dai link %d", link_index);
+					dev_err(dev, "failed to create dai link %d\n", link_index);
 					return ret;
 				}
 			}
-- 
2.30.2

[PATCH 02/12] ASoC: intel: sof_sdw: Remove duplicate NULL check on adr_link

[Charles Keepax]

get_dailink_info already checked if the adr_link pointer was NULL so
there is no need to recheck later in sof_card_dai_links_create.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 8f3329dcf7062..89614d08d0918 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1632,10 +1632,6 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
 	if (!sdw_be_num)
 		goto SSP;
 
-	adr_link = mach_params->links;
-	if (!adr_link)
-		return -EINVAL;
-
 	for (i = 0; i < SDW_MAX_LINKS; i++)
 		sdw_pin_index[i] = SDW_INTEL_BIDIR_PDI_BASE;
 
-- 
2.30.2

[PATCH 03/12] ASoC: intel: sof_sdw: Check link mask validity in get_dailink_info

[Charles Keepax]

As get_dailink_info spins through all the links anyway simply check the
link masks there. This saves an extra check and means the code will
fail earlier if the mask is invalid.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 12 ++++--------
 1 file changed, 4 insertions(+), 8 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 89614d08d0918..268629d5505c3 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1053,6 +1053,10 @@ static int get_dailink_info(struct device *dev,
 		int stream;
 		u64 adr;
 
+		/* make sure the link mask has a single bit set */
+		if (!is_power_of_2(adr_link->mask))
+			return -EINVAL;
+
 		for (i = 0; i < adr_link->num_adr; i++) {
 			adr = adr_link->adr_d[i].adr;
 			codec_index = find_codec_info_part(adr);
@@ -1302,10 +1306,6 @@ static int get_slave_info(const struct snd_soc_acpi_link_adr *adr_link,
 	no_aggregation = sof_sdw_quirk & SOF_SDW_NO_AGGREGATION;
 	adr_d = &adr_link->adr_d[adr_index];
 
-	/* make sure the link mask has a single bit set */
-	if (!is_power_of_2(adr_link->mask))
-		return -EINVAL;
-
 	cpu_dai_id[index++] = ffs(adr_link->mask) - 1;
 	if (!adr_d->endpoints->aggregated || no_aggregation) {
 		*cpu_dai_num = 1;
@@ -1334,10 +1334,6 @@ static int get_slave_info(const struct snd_soc_acpi_link_adr *adr_link,
 		    endpoint->group_id != *group_id)
 			continue;
 
-		/* make sure the link mask has a single bit set */
-		if (!is_power_of_2(adr_next->mask))
-			return -EINVAL;
-
 		if (index >= SDW_MAX_CPU_DAIS) {
 			dev_err(dev, "cpu_dai_id array overflows\n");
 			return -EINVAL;
-- 
2.30.2

[PATCH 04/12] ASoC: intel: sof-sdw: Move check for valid group id to get_dailink_info

[Charles Keepax]

Move the check for a valid group id into get_dailink_info as
well. This does cause a slight change in behaviour in that the system
will return an error rather than just ignoring the link with an
invalid group id. There are presently no systems with invalid group
ids in mainline and failing seems more appropriate since it will
better highlight the code needs fixing.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 268629d5505c3..b250fb7be4bff 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1074,6 +1074,11 @@ static int get_dailink_info(struct device *dev,
 			}
 
 			endpoint = adr_link->adr_d[i].endpoints;
+			if (endpoint->aggregated && !endpoint->group_id) {
+				dev_err(dev, "invalid group id on link %x\n",
+					adr_link->mask);
+				return -EINVAL;
+			}
 
 			for (j = 0; j < codec_info->dai_num; j++) {
 				/* count DAI number for playback and capture */
@@ -1666,11 +1671,6 @@ static int sof_card_dai_links_create(struct snd_soc_card *card)
 			const struct snd_soc_acpi_endpoint *endpoint;
 
 			endpoint = adr_link->adr_d[i].endpoints;
-			if (endpoint->aggregated && !endpoint->group_id) {
-				dev_err(dev, "invalid group id on link %x\n",
-					adr_link->mask);
-				continue;
-			}
 
 			/* this group has been generated */
 			if (endpoint->aggregated &&
-- 
2.30.2

[PATCH 05/12] ASoC: intel: sof_sdw: Add helper to create a single codec DLC

[Charles Keepax]

Add a helper function to create a single codec DAI link component
structure. This sets things up for more refactoring of the creating of
the DAI links.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 80 +++++++++++++++++---------------
 1 file changed, 43 insertions(+), 37 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index b250fb7be4bff..ba4775e778072 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1160,6 +1160,43 @@ static bool is_unique_device(const struct snd_soc_acpi_link_adr *adr_link,
 	return true;
 }
 
+static int fill_sdw_codec_dlc(struct device *dev,
+			      const struct snd_soc_acpi_link_adr *adr_link,
+			      struct snd_soc_dai_link_component *codec,
+			      int codec_index, int adr_index, int dai_index)
+{
+	unsigned int sdw_version, unique_id, mfg_id, link_id, part_id, class_id;
+	u64 adr = adr_link->adr_d[adr_index].adr;
+
+	sdw_version = SDW_VERSION(adr);
+	link_id = SDW_DISCO_LINK_ID(adr);
+	unique_id = SDW_UNIQUE_ID(adr);
+	mfg_id = SDW_MFG_ID(adr);
+	part_id = SDW_PART_ID(adr);
+	class_id = SDW_CLASS_ID(adr);
+
+	if (codec_info_list[codec_index].codec_name)
+		codec->name = devm_kstrdup(dev,
+					   codec_info_list[codec_index].codec_name,
+					   GFP_KERNEL);
+	else if (is_unique_device(adr_link, sdw_version, mfg_id, part_id,
+				  class_id, adr_index))
+		codec->name = devm_kasprintf(dev, GFP_KERNEL,
+					     "sdw:%01x:%04x:%04x:%02x", link_id,
+					     mfg_id, part_id, class_id);
+	else
+		codec->name = devm_kasprintf(dev, GFP_KERNEL,
+					     "sdw:%01x:%04x:%04x:%02x:%01x", link_id,
+					     mfg_id, part_id, class_id, unique_id);
+
+	if (!codec->name)
+		return -ENOMEM;
+
+	codec->dai_name = codec_info_list[codec_index].dais[dai_index].dai_name;
+
+	return 0;
+}
+
 static int create_codec_dai_name(struct device *dev,
 				 const struct snd_soc_acpi_link_adr *adr_link,
 				 struct snd_soc_dai_link_component *codec,
@@ -1171,7 +1208,7 @@ static int create_codec_dai_name(struct device *dev,
 				 int dai_index)
 {
 	int _codec_index = -1;
-	int i;
+	int i, ret;
 
 	/* sanity check */
 	if (*codec_conf_index + adr_link->num_adr - adr_index > codec_count) {
@@ -1180,13 +1217,8 @@ static int create_codec_dai_name(struct device *dev,
 	}
 
 	for (i = adr_index; i < adr_link->num_adr; i++) {
-		unsigned int sdw_version, unique_id, mfg_id;
-		unsigned int link_id, part_id, class_id;
 		int codec_index, comp_index;
-		char *codec_str;
-		u64 adr;
-
-		adr = adr_link->adr_d[i].adr;
+		u64 adr = adr_link->adr_d[i].adr;
 
 		codec_index = find_codec_info_part(adr);
 		if (codec_index < 0)
@@ -1197,38 +1229,12 @@ static int create_codec_dai_name(struct device *dev,
 		}
 		_codec_index = codec_index;
 
-		sdw_version = SDW_VERSION(adr);
-		link_id = SDW_DISCO_LINK_ID(adr);
-		unique_id = SDW_UNIQUE_ID(adr);
-		mfg_id = SDW_MFG_ID(adr);
-		part_id = SDW_PART_ID(adr);
-		class_id = SDW_CLASS_ID(adr);
-
 		comp_index = i - adr_index + offset;
-		if (codec_info_list[codec_index].codec_name) {
-			codec[comp_index].name =
-				devm_kstrdup(dev, codec_info_list[codec_index].codec_name,
-					     GFP_KERNEL);
-		} else if (is_unique_device(adr_link, sdw_version, mfg_id, part_id,
-				     class_id, i)) {
-			codec_str = "sdw:%01x:%04x:%04x:%02x";
-			codec[comp_index].name =
-				devm_kasprintf(dev, GFP_KERNEL, codec_str,
-					       link_id, mfg_id, part_id,
-					       class_id);
-		} else {
-			codec_str = "sdw:%01x:%04x:%04x:%02x:%01x";
-			codec[comp_index].name =
-				devm_kasprintf(dev, GFP_KERNEL, codec_str,
-					       link_id, mfg_id, part_id,
-					       class_id, unique_id);
-		}
 
-		if (!codec[comp_index].name)
-			return -ENOMEM;
-
-		codec[comp_index].dai_name =
-			codec_info_list[codec_index].dais[dai_index].dai_name;
+		ret = fill_sdw_codec_dlc(dev, adr_link, &codec[comp_index],
+					 codec_index, i, dai_index);
+		if (ret)
+			return ret;
 
 		codec_conf[*codec_conf_index].dlc = codec[comp_index];
 		codec_conf[*codec_conf_index].name_prefix = adr_link->adr_d[i].name_prefix;
-- 
2.30.2

[PATCH 06/12] ASoC: intel: sof_sdw: Pull device loop up into create_sdw_dailink

[Charles Keepax]

The loops which fill the codec DAI link component structures are split
across create_sdw_dailink and create_codec_dai_name. This causes the
code to be rather confusing, needing to return out the function to allow
the upper loop to iterate. Remove the create_codec_dai_name helper and
pull its code up into create_sdw_dailink, this makes it more obvious
what is happening in the code. This patch makes no functional change
just hoists the code up a level.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 89 +++++++++++++-------------------
 1 file changed, 35 insertions(+), 54 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index ba4775e778072..5c154628236c7 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1197,54 +1197,6 @@ static int fill_sdw_codec_dlc(struct device *dev,
 	return 0;
 }
 
-static int create_codec_dai_name(struct device *dev,
-				 const struct snd_soc_acpi_link_adr *adr_link,
-				 struct snd_soc_dai_link_component *codec,
-				 int offset,
-				 struct snd_soc_codec_conf *codec_conf,
-				 int codec_count,
-				 int *codec_conf_index,
-				 int adr_index,
-				 int dai_index)
-{
-	int _codec_index = -1;
-	int i, ret;
-
-	/* sanity check */
-	if (*codec_conf_index + adr_link->num_adr - adr_index > codec_count) {
-		dev_err(dev, "codec_conf: out-of-bounds access requested\n");
-		return -EINVAL;
-	}
-
-	for (i = adr_index; i < adr_link->num_adr; i++) {
-		int codec_index, comp_index;
-		u64 adr = adr_link->adr_d[i].adr;
-
-		codec_index = find_codec_info_part(adr);
-		if (codec_index < 0)
-			return codec_index;
-		if (_codec_index != -1 && codec_index != _codec_index) {
-			dev_dbg(dev, "Different devices on the same sdw link\n");
-			break;
-		}
-		_codec_index = codec_index;
-
-		comp_index = i - adr_index + offset;
-
-		ret = fill_sdw_codec_dlc(dev, adr_link, &codec[comp_index],
-					 codec_index, i, dai_index);
-		if (ret)
-			return ret;
-
-		codec_conf[*codec_conf_index].dlc = codec[comp_index];
-		codec_conf[*codec_conf_index].name_prefix = adr_link->adr_d[i].name_prefix;
-
-		++*codec_conf_index;
-	}
-
-	return 0;
-}
-
 static int set_codec_init_func(struct snd_soc_card *card,
 			       const struct snd_soc_acpi_link_adr *adr_link,
 			       struct snd_soc_dai_link *dai_links,
@@ -1401,8 +1353,8 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 	int codec_num;
 	int stream;
 	int i = 0;
+	int j, k;
 	int ret;
-	int k;
 
 	ret = get_slave_info(adr_link, dev, cpu_dai_id, &cpu_dai_num, &codec_num,
 			     &group_id, adr_index);
@@ -1417,6 +1369,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 	for (adr_link_next = adr_link; adr_link_next && adr_link_next->num_adr &&
 	     i < cpu_dai_num; adr_link_next++) {
 		const struct snd_soc_acpi_endpoint *endpoints;
+		int _codec_index = -1;
 
 		endpoints = adr_link_next->adr_d->endpoints;
 		if (group_id && (!endpoints->aggregated ||
@@ -1427,11 +1380,39 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		if (cpu_dai_id[i] != ffs(adr_link_next->mask) - 1)
 			continue;
 
-		ret = create_codec_dai_name(dev, adr_link_next, codecs, codec_dlc_index,
-					    codec_conf, codec_count, codec_conf_index,
-					    adr_index, dai_index);
-		if (ret < 0)
-			return ret;
+		/* sanity check */
+		if (*codec_conf_index + adr_link_next->num_adr - adr_index > codec_count) {
+			dev_err(dev, "codec_conf: out-of-bounds access requested\n");
+			return -EINVAL;
+		}
+
+		for (j = adr_index; j < adr_link_next->num_adr; j++) {
+			int codec_index, comp_index;
+			u64 adr = adr_link_next->adr_d[j].adr;
+
+			codec_index = find_codec_info_part(adr);
+			if (codec_index < 0)
+				return codec_index;
+			if (_codec_index != -1 && codec_index != _codec_index) {
+				dev_dbg(dev, "Different devices on the same sdw link\n");
+				break;
+			}
+			_codec_index = codec_index;
+
+			comp_index = j - adr_index + codec_dlc_index;
+
+			ret = fill_sdw_codec_dlc(dev, adr_link_next,
+						 &codecs[comp_index],
+						 codec_index, j, dai_index);
+			if (ret)
+				return ret;
+
+			codec_conf[*codec_conf_index].dlc = codecs[comp_index];
+			codec_conf[*codec_conf_index].name_prefix =
+					adr_link_next->adr_d[j].name_prefix;
+
+			(*codec_conf_index)++;
+		}
 
 		/* check next link to create codec dai in the processed group */
 		i++;
-- 
2.30.2

[PATCH 07/12] ASoC: intel: sof_sdw: Update DLC index each time one is added

[Charles Keepax]

In create_sdw_dailink, rather than bulk updating the index into the
DAI link components array, at the end of processing a link, do so each
time the code adds a new component. This simplifies things slightly,
as an intermediate variable is no longer needed to track the current
place in the DAI link components array.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 5c154628236c7..b381fb2619943 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1387,7 +1387,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		}
 
 		for (j = adr_index; j < adr_link_next->num_adr; j++) {
-			int codec_index, comp_index;
+			int codec_index;
 			u64 adr = adr_link_next->adr_d[j].adr;
 
 			codec_index = find_codec_info_part(adr);
@@ -1399,24 +1399,22 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 			}
 			_codec_index = codec_index;
 
-			comp_index = j - adr_index + codec_dlc_index;
-
 			ret = fill_sdw_codec_dlc(dev, adr_link_next,
-						 &codecs[comp_index],
+						 &codecs[codec_dlc_index],
 						 codec_index, j, dai_index);
 			if (ret)
 				return ret;
 
-			codec_conf[*codec_conf_index].dlc = codecs[comp_index];
+			codec_conf[*codec_conf_index].dlc = codecs[codec_dlc_index];
 			codec_conf[*codec_conf_index].name_prefix =
 					adr_link_next->adr_d[j].name_prefix;
 
+			codec_dlc_index++;
 			(*codec_conf_index)++;
 		}
 
 		/* check next link to create codec dai in the processed group */
 		i++;
-		codec_dlc_index += adr_link_next->num_adr;
 	}
 
 	/* find codec info to create BE DAI */
-- 
2.30.2

[PATCH 08/12] ASoC: intel: sof_sdw: Move range check of codec_conf into inner loop

[Charles Keepax]

There are two problems with the current range check on the codec_conf
array.

Firstly, adr_link_next->num_adr refers to the number of devices
on the current SoundWire link, but adr_index refers to the first
SoundWire link involved in the DAI link. This means that subtracting
these two numbers is only meaningful on the first SoundWire link in the
DAI and broken on later links.

Secondly, the intention of the range check is to add the number
of remaining devices on the currently link to the current index
and ensure enough space remains. However, this assumes that all
remaining devices on the SoundWire link will be added to the current
DAI link. Ideally this would not be the case, and devices could be
grouped as the user desired.

Moving the range check into the inner loop both simplifies the code (no
need to add and subtract offsets) and allows future refactoring such
that devices on a single SoundWire link don't have to all be grouped onto
a single DAI link. The check will be processed slightly more often since
it is processed for each device rather each link but this is probe time
and the numbers involved are very small here (4 links, likely no more
than 2-4 devices per link).

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index b381fb2619943..0401516f35de6 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1380,12 +1380,6 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		if (cpu_dai_id[i] != ffs(adr_link_next->mask) - 1)
 			continue;
 
-		/* sanity check */
-		if (*codec_conf_index + adr_link_next->num_adr - adr_index > codec_count) {
-			dev_err(dev, "codec_conf: out-of-bounds access requested\n");
-			return -EINVAL;
-		}
-
 		for (j = adr_index; j < adr_link_next->num_adr; j++) {
 			int codec_index;
 			u64 adr = adr_link_next->adr_d[j].adr;
@@ -1399,6 +1393,12 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 			}
 			_codec_index = codec_index;
 
+			/* sanity check */
+			if (*codec_conf_index >= codec_count) {
+				dev_err(dev, "codec_conf array overflowed\n");
+				return -EINVAL;
+			}
+
 			ret = fill_sdw_codec_dlc(dev, adr_link_next,
 						 &codecs[codec_dlc_index],
 						 codec_index, j, dai_index);
-- 
2.30.2

[PATCH 09/12] ASoC: intel: sof_sdw: Device loop should not always start at adr_index

[Charles Keepax]

The current loops at the top of create_sdw_dailink process the devices
on each link starting from device index adr_index. But adr_index is only
meaningful on the first on these SoundWire links, as it is the index of
the current device on that link. This means devices will be skipped on
later links.

Say for example the system looks like this:

SDW0 - Codec (Not Aggregated), Amp 1 (Aggregated, Group 1)
SDW1 - Amp 2 (Aggregated, Group 1), Amp 3 (Aggregated, Group 1)

The code should create 2 DAI links, one for the CODEC and one for the
aggregated amps. It will create the DAI link for the codec no problem.
When it creates the DAI link for Group 1 however, create_sdw_dailink
will be called with an adr_index of 1, since that is the index of Amp
1 on SDW0.  However, as the loop in create_sdw_dailink moves onto SDW1
it will again start from adr_index, skipping Amp 2. Resulting in the amp
DAI link only have amps 1 and 3 in it.

It is reasonable to start at adr_index on the first link, since
earlier devices have by definition already been processed. However,
update the code when processing later links to handle all devices.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 0401516f35de6..767c49022eae4 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1366,6 +1366,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		return -ENOMEM;
 
 	/* generate codec name on different links in the same group */
+	j = adr_index;
 	for (adr_link_next = adr_link; adr_link_next && adr_link_next->num_adr &&
 	     i < cpu_dai_num; adr_link_next++) {
 		const struct snd_soc_acpi_endpoint *endpoints;
@@ -1380,7 +1381,8 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		if (cpu_dai_id[i] != ffs(adr_link_next->mask) - 1)
 			continue;
 
-		for (j = adr_index; j < adr_link_next->num_adr; j++) {
+		/* j reset after loop, adr_index only applies to first link */
+		for (; j < adr_link_next->num_adr; j++) {
 			int codec_index;
 			u64 adr = adr_link_next->adr_d[j].adr;
 
@@ -1412,6 +1414,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 			codec_dlc_index++;
 			(*codec_conf_index)++;
 		}
+		j = 0;
 
 		/* check next link to create codec dai in the processed group */
 		i++;
-- 
2.30.2

[PATCH 10/12] ASoC: intel: sof_sdw: Support multiple groups on the same link

[Charles Keepax]

The current code checks the first device on a link and assumes
that all the other devices on the link will have the same endpoint
aggregation status and endpoint group ID.

Say for example a system looked like:

SDW0 - Amp 1 (Aggregated, Group 1), Mic 1 (Aggregated, Group 2)
SDW1 - Amp 2 (Aggregated, Group 1), Mic 2 (Aggregated, Group 2)

The current code would create the DAI link for the aggregated amps,
although it is worth noting that the only reason Mic 2 is not added is
the additional check that aborts processing the link when the device
changes. Then when processing the DAI link for the microphones, Mic
2 would not be added, as the check will only be done on the first
device, which would be Amp 2 and thus the wrong group, causing the
whole link to be skipped.

Move the endpoint check to be for each device rather than the first
device on each link.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 42 ++++++++++++++++----------------
 1 file changed, 21 insertions(+), 21 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 767c49022eae4..357946365e76f 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1288,25 +1288,24 @@ static int get_slave_info(const struct snd_soc_acpi_link_adr *adr_link,
 	}
 
 	/* gather other link ID of slaves in the same group */
-	for (adr_next = adr_link + 1; adr_next && adr_next->num_adr;
-		adr_next++) {
-		const struct snd_soc_acpi_endpoint *endpoint;
-
-		endpoint = adr_next->adr_d->endpoints;
-		if (!endpoint->aggregated ||
-		    endpoint->group_id != *group_id)
-			continue;
+	for (adr_next = adr_link + 1; adr_next && adr_next->num_adr; adr_next++) {
+		unsigned int link_codecs = 0;
 
-		if (index >= SDW_MAX_CPU_DAIS) {
-			dev_err(dev, "cpu_dai_id array overflows\n");
-			return -EINVAL;
-		}
-
-		cpu_dai_id[index++] = ffs(adr_next->mask) - 1;
 		for (i = 0; i < adr_next->num_adr; i++) {
 			if (adr_next->adr_d[i].endpoints->aggregated &&
 			    adr_next->adr_d[i].endpoints->group_id == *group_id)
-				(*codec_num)++;
+				link_codecs++;
+		}
+
+		if (link_codecs) {
+			*codec_num += link_codecs;
+
+			if (index >= SDW_MAX_CPU_DAIS) {
+				dev_err(dev, "cpu_dai_id array overflowed\n");
+				return -EINVAL;
+			}
+
+			cpu_dai_id[index++] = ffs(adr_next->mask) - 1;
 		}
 	}
 
@@ -1369,20 +1368,15 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 	j = adr_index;
 	for (adr_link_next = adr_link; adr_link_next && adr_link_next->num_adr &&
 	     i < cpu_dai_num; adr_link_next++) {
-		const struct snd_soc_acpi_endpoint *endpoints;
 		int _codec_index = -1;
 
-		endpoints = adr_link_next->adr_d->endpoints;
-		if (group_id && (!endpoints->aggregated ||
-				 endpoints->group_id != group_id))
-			continue;
-
 		/* skip the link excluded by this processed group */
 		if (cpu_dai_id[i] != ffs(adr_link_next->mask) - 1)
 			continue;
 
 		/* j reset after loop, adr_index only applies to first link */
 		for (; j < adr_link_next->num_adr; j++) {
+			const struct snd_soc_acpi_endpoint *endpoints;
 			int codec_index;
 			u64 adr = adr_link_next->adr_d[j].adr;
 
@@ -1395,6 +1389,12 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 			}
 			_codec_index = codec_index;
 
+			endpoints = adr_link_next->adr_d[j].endpoints;
+
+			if (group_id && (!endpoints->aggregated ||
+					 endpoints->group_id != group_id))
+				continue;
+
 			/* sanity check */
 			if (*codec_conf_index >= codec_count) {
 				dev_err(dev, "codec_conf array overflowed\n");
-- 
2.30.2

[PATCH 11/12] ASoC: intel: sof_sdw: Allow different devices on the same link

[Charles Keepax]

If the current code encounters a new type of device on a SoundWire
link, it will abort processing that link and move onto the next
link. However, there is no reason to disallow this setup, it would
appear this was being disallowed to work around issues introduced
by only the first endpoint on each link being checked, which is now
fixed.

The device type shouldn't determine which DAI link it is connected to,
the group ID and aggregation status should.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 22 +++++++---------------
 1 file changed, 7 insertions(+), 15 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 357946365e76f..296de5baee3d2 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1163,10 +1163,15 @@ static bool is_unique_device(const struct snd_soc_acpi_link_adr *adr_link,
 static int fill_sdw_codec_dlc(struct device *dev,
 			      const struct snd_soc_acpi_link_adr *adr_link,
 			      struct snd_soc_dai_link_component *codec,
-			      int codec_index, int adr_index, int dai_index)
+			      int adr_index, int dai_index)
 {
 	unsigned int sdw_version, unique_id, mfg_id, link_id, part_id, class_id;
 	u64 adr = adr_link->adr_d[adr_index].adr;
+	int codec_index;
+
+	codec_index = find_codec_info_part(adr);
+	if (codec_index < 0)
+		return codec_index;
 
 	sdw_version = SDW_VERSION(adr);
 	link_id = SDW_DISCO_LINK_ID(adr);
@@ -1368,8 +1373,6 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 	j = adr_index;
 	for (adr_link_next = adr_link; adr_link_next && adr_link_next->num_adr &&
 	     i < cpu_dai_num; adr_link_next++) {
-		int _codec_index = -1;
-
 		/* skip the link excluded by this processed group */
 		if (cpu_dai_id[i] != ffs(adr_link_next->mask) - 1)
 			continue;
@@ -1377,17 +1380,6 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 		/* j reset after loop, adr_index only applies to first link */
 		for (; j < adr_link_next->num_adr; j++) {
 			const struct snd_soc_acpi_endpoint *endpoints;
-			int codec_index;
-			u64 adr = adr_link_next->adr_d[j].adr;
-
-			codec_index = find_codec_info_part(adr);
-			if (codec_index < 0)
-				return codec_index;
-			if (_codec_index != -1 && codec_index != _codec_index) {
-				dev_dbg(dev, "Different devices on the same sdw link\n");
-				break;
-			}
-			_codec_index = codec_index;
 
 			endpoints = adr_link_next->adr_d[j].endpoints;
 
@@ -1403,7 +1395,7 @@ static int create_sdw_dailink(struct snd_soc_card *card, int *link_index,
 
 			ret = fill_sdw_codec_dlc(dev, adr_link_next,
 						 &codecs[codec_dlc_index],
-						 codec_index, j, dai_index);
+						 j, dai_index);
 			if (ret)
 				return ret;
 
-- 
2.30.2

[PATCH 12/12] ASoC: intel: sof_sdw: Simplify get_slave_info

[Charles Keepax]

Now the first device on a link is not treated specially there is no
need to have a separate loop to handle the current link over the
future links, as the logic is identical. Combine this all into a
single processing loop.

Reviewed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---
 sound/soc/intel/boards/sof_sdw.c | 38 ++++++++++----------------------
 1 file changed, 12 insertions(+), 26 deletions(-)

diff --git a/sound/soc/intel/boards/sof_sdw.c b/sound/soc/intel/boards/sof_sdw.c
index 296de5baee3d2..f283c0d528dfc 100644
--- a/sound/soc/intel/boards/sof_sdw.c
+++ b/sound/soc/intel/boards/sof_sdw.c
@@ -1265,57 +1265,43 @@ static int get_slave_info(const struct snd_soc_acpi_link_adr *adr_link,
 			  int *codec_num, unsigned int *group_id,
 			  int adr_index)
 {
-	const struct snd_soc_acpi_adr_device *adr_d;
-	const struct snd_soc_acpi_link_adr *adr_next;
-	bool no_aggregation;
-	int index = 0;
+	bool no_aggregation = sof_sdw_quirk & SOF_SDW_NO_AGGREGATION;
 	int i;
 
-	no_aggregation = sof_sdw_quirk & SOF_SDW_NO_AGGREGATION;
-	adr_d = &adr_link->adr_d[adr_index];
-
-	cpu_dai_id[index++] = ffs(adr_link->mask) - 1;
-	if (!adr_d->endpoints->aggregated || no_aggregation) {
+	if (!adr_link->adr_d[adr_index].endpoints->aggregated || no_aggregation) {
+		cpu_dai_id[0] = ffs(adr_link->mask) - 1;
 		*cpu_dai_num = 1;
 		*codec_num = 1;
 		*group_id = 0;
 		return 0;
 	}
 
-	*group_id = adr_d->endpoints->group_id;
-
-	/* Count endpoints with the same group_id in the adr_link */
 	*codec_num = 0;
-	for (i = 0; i < adr_link->num_adr; i++) {
-		if (adr_link->adr_d[i].endpoints->aggregated &&
-		    adr_link->adr_d[i].endpoints->group_id == *group_id)
-			(*codec_num)++;
-	}
+	*cpu_dai_num = 0;
+	*group_id = adr_link->adr_d[adr_index].endpoints->group_id;
 
-	/* gather other link ID of slaves in the same group */
-	for (adr_next = adr_link + 1; adr_next && adr_next->num_adr; adr_next++) {
+	/* Count endpoints with the same group_id in the adr_link */
+	for (; adr_link && adr_link->num_adr; adr_link++) {
 		unsigned int link_codecs = 0;
 
-		for (i = 0; i < adr_next->num_adr; i++) {
-			if (adr_next->adr_d[i].endpoints->aggregated &&
-			    adr_next->adr_d[i].endpoints->group_id == *group_id)
+		for (i = 0; i < adr_link->num_adr; i++) {
+			if (adr_link->adr_d[i].endpoints->aggregated &&
+			    adr_link->adr_d[i].endpoints->group_id == *group_id)
 				link_codecs++;
 		}
 
 		if (link_codecs) {
 			*codec_num += link_codecs;
 
-			if (index >= SDW_MAX_CPU_DAIS) {
+			if (*cpu_dai_num >= SDW_MAX_CPU_DAIS) {
 				dev_err(dev, "cpu_dai_id array overflowed\n");
 				return -EINVAL;
 			}
 
-			cpu_dai_id[index++] = ffs(adr_next->mask) - 1;
+			cpu_dai_id[(*cpu_dai_num)++] = ffs(adr_link->mask) - 1;
 		}
 	}
 
-	*cpu_dai_num = index;
-
 	return 0;
 }
 
-- 
2.30.2

Re: [PATCH 01/12] ASoC: intel: sof_sdw: Printk's should end with a newline

[Mark Brown]

On Tue, 08 Aug 2023 14:20:02 +0100, Charles Keepax wrote:
> Add the missing new lines.
> 
> 

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[01/12] ASoC: intel: sof_sdw: Printk's should end with a newline
        commit: c307ca16c9bffc18dbf37ae64c71d935a2923c3a
[02/12] ASoC: intel: sof_sdw: Remove duplicate NULL check on adr_link
        commit: 3003ea9cb7bd6399ca9962e0b3dd0ac58b151c2e
[03/12] ASoC: intel: sof_sdw: Check link mask validity in get_dailink_info
        commit: e1cfd5fef3d6eaf0ddbc54da70ddf54462b23592
[04/12] ASoC: intel: sof-sdw: Move check for valid group id to get_dailink_info
        commit: 87608d3e9de18331c5d3c9ecb915b0ff3d03c089
[05/12] ASoC: intel: sof_sdw: Add helper to create a single codec DLC
        commit: 92e9f10a093529f85b7557b0627531728d89afa2
[06/12] ASoC: intel: sof_sdw: Pull device loop up into create_sdw_dailink
        commit: c3d7e29ad82ee689b1adf5ea7806b9d06eb098c0
[07/12] ASoC: intel: sof_sdw: Update DLC index each time one is added
        commit: 0e82229fb74a26cfaf6ae3772cbdefdb643f98a5
[08/12] ASoC: intel: sof_sdw: Move range check of codec_conf into inner loop
        commit: 59736ca62e1eeb4466ace99e167cbe7a0f9bc0fa
[09/12] ASoC: intel: sof_sdw: Device loop should not always start at adr_index
        commit: f3eb3d45fdfd693dc004e664544f978ae8d38f48
[10/12] ASoC: intel: sof_sdw: Support multiple groups on the same link
        commit: f82742dd479dfec7dc6a30a84f165a258c51ce09
[11/12] ASoC: intel: sof_sdw: Allow different devices on the same link
        commit: 317dcdecaf7a42febb78c564df15fd817bf720b2
[12/12] ASoC: intel: sof_sdw: Simplify get_slave_info
        commit: 7f5cf19703ccb05ac4965d1cfc1422e38bec93aa

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark