[RFC 0/3] simple-card: Improve probe's error messages

[RFC 0/3] simple-card: Improve probe's error messages

[Fabio Forni]

Hello,

this patch tries to improve errors messages that are printed by the
probe function of simple-card. Some messages were missing and I think
that made debugging difficult, others were printed even though the
underlying error is unrelated (e.g. when devm_snd_soc_register_card
is called).

This series is tagged as RFC because it's my first contribution. Plus,
patch number 3 touches many lines. I'm not sure I'm allowed to touch
so many lines in one run, also I need more than a pair of eyes because
even though patch #3 is just a refactor, I'm not 100% familiar with ASoC.

Fabio Forni (3):
  ASoC: simple-card: Do not print "parse error" twice
  ASoC: simple-card: Replace "parse error" with a more descriptive msg
  ASoC: simple-card: Split alloc and init logics in probe function

 sound/soc/generic/simple-card-utils.c |   9 ++
 sound/soc/generic/simple-card.c       | 141 +++++++++++++-------------
 2 files changed, 82 insertions(+), 68 deletions(-)

-- 
2.52.0

[RFC 1/3] ASoC: simple-card: Do not print "parse error" twice

[Fabio Forni]

If an error occurred on line 747, we'd print "parse error" and then go
to the `err` label, where we print the same message.
Let's remove the duplicated print.

Signed-off-by: Fabio Forni <development@redaril.me>
---
 sound/soc/generic/simple-card.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 5af6d1b308f2..8d36cbbfca1d 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -745,10 +745,8 @@ static int simple_probe(struct platform_device *pdev)
 	if (np && of_device_is_available(np)) {
 
 		ret = simple_parse_of(priv, li);
-		if (ret < 0) {
-			dev_err_probe(dev, ret, "parse error\n");
+		if (ret < 0)
 			goto err;
-		}
 
 	} else {
 		struct simple_util_info *cinfo;
-- 
2.52.0

Re: [RFC 1/3] ASoC: simple-card: Do not print "parse error" twice

[Kuninori Morimoto]

Hi Fabio

Thank you for your patch

> If an error occurred on line 747, we'd print "parse error" and then go
> to the `err` label, where we print the same message.
> Let's remove the duplicated print.
> 
> Signed-off-by: Fabio Forni <development@redaril.me>
> ---

I think this patch effect will be removed/freshed by [3/3] patch anyway ?
Same comment to [2/3] patch

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [RFC 1/3] ASoC: simple-card: Do not print "parse error" twice

[Fabio]

Thank you for your fast review, Kuninori!

> I think this patch effect will be removed/freshed by [3/3] patch anyway ?
> Same comment to [2/3] patch

Yes, definitely. I kept my commit history to document my flow of thought and to address specific problems.
At this point I'll just squash #1 and #2 into one commit in v2. 

[RFC 2/3] ASoC: simple-card: Replace "parse error" with a more descriptive msg

[Fabio Forni]

"parse error" is misleading: it made me think the device tree contains
syntax errors, or phandles of the wrong type were passed to the
simple-audio-card node. Instead in my case, the `sound-dai` subnode of
`simple-audio-card,cpu` was referring to an uninitialized device, because
its kernel module wasn't loaded yet.

As far as I understood, simple_probe fails when components composing
the sound card are missing or simply not ready.

Signed-off-by: Fabio Forni <development@redaril.me>
---
 sound/soc/generic/simple-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 8d36cbbfca1d..298f8cc98130 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -806,7 +806,7 @@ static int simple_probe(struct platform_device *pdev)
 err:
 	simple_util_clean_reference(card);
 end:
-	return dev_err_probe(dev, ret, "parse error\n");
+	return dev_err_probe(dev, ret, "components missing or uninitialized\n");
 }
 
 static const struct of_device_id simple_of_match[] = {
-- 
2.52.0

[RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function

[Fabio Forni]

It wasn't clear to me when different simple_probe's gotos were to be used,
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@redaril.me>
---
 sound/soc/generic/simple-card-utils.c |   9 ++
 sound/soc/generic/simple-card.c       | 137 ++++++++++++++------------
 2 files changed, 81 insertions(+), 65 deletions(-)

diff --git a/sound/soc/generic/simple-card-utils.c b/sound/soc/generic/simple-card-utils.c
index 355f7ec8943c..46a8ee04c6ef 100644
--- a/sound/soc/generic/simple-card-utils.c
+++ b/sound/soc/generic/simple-card-utils.c
@@ -888,6 +888,15 @@ static struct simple_util_dai dummy_util_dais = {
 	.name = "dummy_util_dais",
 };
 
+/**
+ * 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.
+ */
 int simple_util_init_priv(struct simple_util_priv *priv,
 			  struct link_info *li)
 {
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;
 
-	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");
 
 	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");
 
 	} else {
 		struct simple_util_info *cinfo;
@@ -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)
+			return dev_err_probe(
+				dev, -EINVAL,
+				"insufficient simple_util_info settings\n");
+
+		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;
+		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));
+	}
 
-		if (!cinfo->name ||
-		    !cinfo->codec_dai.name ||
-		    !cinfo->codec ||
-		    !cinfo->platform ||
-		    !cinfo->cpu_dai.name) {
-			dev_err(dev, "insufficient simple_util_info settings\n");
-			goto err;
-		}
+	return 0;
+}
 
-		cpus			= dai_link->cpus;
-		cpus->dai_name		= cinfo->cpu_dai.name;
+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";
 
-		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;
+
+	ret = simple_util_init_priv(priv, li);
+	if (ret < 0)
+		return ret;
+
+	ret = simple_probe_merge_components(dev, priv, li);
+	if (ret < 0)
+		goto err;
 
 	snd_soc_card_set_drvdata(card, priv);
 
 	simple_util_debug_info(priv);
 
 	ret = devm_snd_soc_register_card(dev, card);
-	if (ret < 0)
+	if (ret < 0) {
+		dev_err_probe(dev, ret, "failed to register card");
 		goto err;
+	}
 
 	return 0;
 err:
 	simple_util_clean_reference(card);
-end:
-	return dev_err_probe(dev, ret, "components missing or uninitialized\n");
+	return ret;
 }
 
 static const struct of_device_id simple_of_match[] = {
-- 
2.52.0

Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function

[Kuninori Morimoto]

Hi Fabio

Thank you for your patch

> It wasn't clear to me when different simple_probe's gotos were to be used,
> 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@redaril.me>
> ---
(snip)
> +/**
> + * 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

Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function

[Fabio]

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?

>>   	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");
> 
[snip]
> 
> 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,
>> 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@redaril.me>
>> ---
> (snip)
>> +/**
>> + * 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

Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function

[Kuninori Morimoto]

Hi Fabio

Sorry for my late response

> >>   	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");
> > 
> [snip]
> > 
> > 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().

To be honest, I don't super mind about error message :P

But I have thought that the style like below can easy for people ?
(A) part checks something and return error, then indicate error reasons.
(B) part returns error because the called function returns error.

	func () {
		...
(A)		if (...)
			error("indicate error reason1")
(A)		if (...)
			error("indicate error reason2")
		...
	}


	main() {
		...
		ret = func();
(B)		if (ret < 0)
			error("func() error")
		...
	}

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function

[Fabio]

Hi, here I am.

> But I have thought that the style like below can easy for people ?
> (A) part checks something and return error, then indicate error reasons.
> (B) part returns error because the called function returns error.
> 
> 	func () {
> 		...
> (A)		if (...)
> 			error("indicate error reason1")
> (A)		if (...)
> 			error("indicate error reason2")
> 		...
> 	}
> 
> 
> 	main() {
> 		...
> 		ret = func();
> (B)		if (ret < 0)
> 			error("func() error")
> 		...
> 	}

Hm, I think you're being too abstract for my comprehension, sorry about that.
Let me assume main() is the probe function, and func() is whatever function
that probe() calls. If my assumptions are correct, then:

- In (A), are we letting child functions print the reason of their own error? If so,
   I like the idea! In C we only return errors codes without a message, so the actual
   reason of, say, EINVAL is only known where the error was thrown. Outside of the
   context of the function an EINVAL just means "something was invalid". So yeah, I'd choose
   option (A).
- In (B), I think it's the approach already being used for simple_parse_of(), isn't it?
   You cannot know the reason of the error for sure, again because of error codes without
   a message being carried together. The best you can do here is thinking of a generic error
   message like I did.

Option (A) requires more work, of course.

Re: [RFC 3/3] ASoC: simple-card: Split alloc and init logics in probe function

[Kuninori Morimoto]

Hi Fabio

> > 	func () {
> > 		...
> > (A)		if (...)
> > 			error("indicate error reason1")
> > (A)		if (...)
> > 			error("indicate error reason2")
> > 		...
> > 	}
> > 
> > 
> > 	main() {
> > 		...
> > 		ret = func();
> > (B)		if (ret < 0)
> > 			error("func() error")
> > 		...
> > 	}
> 
> Hm, I think you're being too abstract for my comprehension, sorry about that.
> Let me assume main() is the probe function, and func() is whatever function
> that probe() calls. If my assumptions are correct, then:

Sorry for my not good explanation, indeed it is too abstract.
Above (A) and (B) exists in the same time, I meant.

> - In (A), are we letting child functions print the reason of their own error?

Yes. indicate why it become error in each function.
Because size is too small, memory alloc failed, etc, etc...

> - In (B), I think it's the approach already being used for
> simple_parse_of(), isn't it?

(B) indicates func() returns error. I think this can indicate error path,
like below

	xxx size failed (error reason)		// (A)
	funcB() error				// (B)
	funcA() error				// (B)

Developer can understand that funcA() calls funcB() and funcB() returns
error from this message.
But this is not mandatory, just an idea.

I think simple_ret() can help you, but I'm not sure whether you can use it
on probe function.

Thank you for your help !!

Best regards
---
Kuninori Morimoto