Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function
From: Fabio
Date: Tue Jan 13 2026 - 17:32:04 EST
I don't think dev_err_probe() should be called when -ENOMEM is thrown
as I think it's obvious what failed. It also seems to do anything at all
per commit d6137f25b191. Do you agree with that commit?
[snip]if (np && of_device_is_available(np)) {
-
ret = simple_parse_of(priv, li);
if (ret < 0)
- goto err;
+ dev_err_probe(dev, ret,
+ "components missing or uninitialized");
Is it clear error message ? It failed parse DT or something ?
That's exactly the part that made me provide this patch. There's so much going
on in simple_parse_of(). I think it may fail when a phandle of the wrong type
is passed in any of the simple-card node.
But I'm certain it can also fail when the DT is correct but kernel modules
of the simple-card's components aren't loaded yet, since this very case happened
to me. "components missing or uninitialized" basically sums it up, without saying
"parse error" that suggests a purely syntactic error. If you don't agree with that
message I can dive into the rabbit hole and learn what's going on inside simple_parse_of().
with aligned. I like aligned code ;) ?
Blame clang-format style! I have autoformatting enabled. I'll manually fix the alignment :)
---
I'll fix the other errors in v2. And sorry for those missing `return`s, I shouldn't code in
the night...
On 13/01/26 03:33, Kuninori Morimoto wrote:
Hi Fabio
Thank you for your patch
It wasn't clear to me when different simple_probe's gotos were to be used,(snip)
plus some allocation errors and snd_soc* errors were printed as if they were
a parsing error. This commit:
1. Splits the allocation logic and the initialization logic, respectively in
simple_probe and simple_probe_merge_components
2. Adds more error messages to improve the debugging experience
3. Reduces the cognitive load of gotos, as there is now only one label (I concede
that's subjective).
This commit also documents simple_util_init_priv.
Signed-off-by: Fabio Forni <development@xxxxxxxxxx>
---
+/**
+ * simple_util_init_priv - Initializes private data of a simple audio card.
+ * @priv: Private data to initialize. Must be pre-allocated.
+ * @li: Links of the card. Cannot be NULL.
+ *
+ * Returns:
+ * * %0 - OK.
+ * * %-ENOMEM - Could not allocate memory.
+ */
This should be separate patch.
diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 298f8cc98130..b8004aa8f452 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -706,47 +706,26 @@ static int simple_soc_probe(struct snd_soc_card *card)
return simple_ret(priv, ret);
}
-static int simple_probe(struct platform_device *pdev)
+static int simple_probe_merge_components(const struct device *dev,
+ struct simple_util_priv *priv,
+ struct link_info *li)
{
- struct simple_util_priv *priv;
- struct device *dev = &pdev->dev;
- struct device_node *np = dev->of_node;
- struct snd_soc_card *card;
+ const struct device_node *np = dev->of_node;
+ struct snd_soc_card *card = simple_priv_to_card(priv);
int ret;
- /* Allocate the private data and the DAI link array */
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
-
- card = simple_priv_to_card(priv);
- card->owner = THIS_MODULE;
- card->dev = dev;
- card->probe = simple_soc_probe;
- card->driver_name = "simple-card";
-
- ret = -ENOMEM;
- struct link_info *li __free(kfree) = kzalloc(sizeof(*li), GFP_KERNEL);
- if (!li)
- goto end;
-
- ret = simple_get_dais_count(priv, li);
- if (ret < 0)
- goto end;
-
- ret = -EINVAL;
if (!li->link)
- goto end;
+ return -EINVAL;
no dev_err_probe() ?
- ret = simple_util_init_priv(priv, li);
+ ret = simple_get_dais_count(priv, li);
if (ret < 0)
- goto end;
+ dev_err_probe(dev, ret, "failed to count DAIs");
Here is missing return ?
if (np && of_device_is_available(np)) {
-
ret = simple_parse_of(priv, li);
if (ret < 0)
- goto err;
+ dev_err_probe(dev, ret,
+ "components missing or uninitialized");
Here is missing return ?
Is it clear error message ? It failed parse DT or something ?
@@ -756,57 +735,85 @@ static int simple_probe(struct platform_device *pdev)
struct snd_soc_dai_link *dai_link = priv->dai_link;
struct simple_dai_props *dai_props = priv->dai_props;
- ret = -EINVAL;
-
cinfo = dev->platform_data;
- if (!cinfo) {
- dev_err(dev, "no info for asoc-simple-card\n");
- goto err;
- }
+ if (!cinfo)
+ return dev_err_probe(dev, -EINVAL,
+ "no info for asoc-simple-card\n");
+
+ if (!cinfo->name || !cinfo->codec_dai.name || !cinfo->codec ||
+ !cinfo->platform || !cinfo->cpu_dai.name)
I like aligned condition, same as original code.
+ return dev_err_probe(
+ dev, -EINVAL,
+ "insufficient simple_util_info settings\n");
This can be 2 or 1 line ?
+ cpus = dai_link->cpus;
+ cpus->dai_name = cinfo->cpu_dai.name;
+
+ codecs = dai_link->codecs;
+ codecs->name = cinfo->codec;
+ codecs->dai_name = cinfo->codec_dai.name;
+
+ platform = dai_link->platforms;
+ platform->name = cinfo->platform;
+
+ card->name = (cinfo->card) ? cinfo->card : cinfo->name;
+ dai_link->name = cinfo->name;
+ dai_link->stream_name = cinfo->name;
+ dai_link->dai_fmt = cinfo->daifmt;
+ dai_link->init = simple_util_dai_init;
I like tag aligned code, same as original
+ memcpy(dai_props->cpu_dai, &cinfo->cpu_dai,
+ sizeof(*dai_props->cpu_dai));
+ memcpy(dai_props->codec_dai, &cinfo->codec_dai,
+ sizeof(*dai_props->codec_dai));
These can be 1 line (with aligned. I like aligned code ;) ?
memcpy(dai_props->cpu_dai, &cinfo->cpu_dai, sizeof(*dai_props->cpu_dai));
memcpy(dai_props->codec_dai, &cinfo->codec_dai, sizeof(*dai_props->codec_dai));
+static int simple_probe(struct platform_device *pdev)
+{
+ struct simple_util_priv *priv;
+ struct device *dev = &pdev->dev;
+ struct snd_soc_card *card;
+ int ret;
- codecs = dai_link->codecs;
- codecs->name = cinfo->codec;
- codecs->dai_name = cinfo->codec_dai.name;
+ /* Allocate the private data and the DAI link array */
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
- platform = dai_link->platforms;
- platform->name = cinfo->platform;
+ card = simple_priv_to_card(priv);
+ card->owner = THIS_MODULE;
+ card->dev = dev;
+ card->probe = simple_soc_probe;
+ card->driver_name = "simple-card";
I like to have tab align here, same as original code.
- card->name = (cinfo->card) ? cinfo->card : cinfo->name;
- dai_link->name = cinfo->name;
- dai_link->stream_name = cinfo->name;
- dai_link->dai_fmt = cinfo->daifmt;
- dai_link->init = simple_util_dai_init;
- memcpy(dai_props->cpu_dai, &cinfo->cpu_dai,
- sizeof(*dai_props->cpu_dai));
- memcpy(dai_props->codec_dai, &cinfo->codec_dai,
- sizeof(*dai_props->codec_dai));
- }
+ struct link_info *li __free(kfree) = kzalloc(sizeof(*li), GFP_KERNEL);
+ if (!li)
+ return -ENOMEM;
no dev_err_probe() ?
+ ret = simple_util_init_priv(priv, li);
+ if (ret < 0)
+ return ret;
no dev_err_probe() ?
It is calling simple_util_init_priv() (A) without calling
simple_get_dais_count() (B).
((B) simple_get_dais_count() fill li->num[x].xxx count, and
(A) simple_util_init_priv() will use it)
I think it doesn't work correctly ?
Thank you for your help !!
Best regards
---
Kuninori Morimoto