[PATCH 01/12] ASoC: intel: sof_sdw: Printk's should end with a newline
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; } }
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;
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;
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 &&
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;
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++;
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 */
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);
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++;
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");
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;
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; }
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
participants (2)
-
Charles Keepax -
Mark Brown