[PATCH 1/2] ASoC: spacemit: Drop redundant error messages

[PATCH 1/2] ASoC: spacemit: Drop redundant error messages

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

The called functions already log failures where appropriate. Return the
original error directly and avoid duplicate error messages.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/spacemit/k1_i2s.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sound/soc/spacemit/k1_i2s.c b/sound/soc/spacemit/k1_i2s.c
index cd461f2aa756..64510c9a1a89 100644
--- a/sound/soc/spacemit/k1_i2s.c
+++ b/sound/soc/spacemit/k1_i2s.c
@@ -478,7 +478,7 @@ static int spacemit_i2s_probe(struct platform_device *pdev)
 
 	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
 	if (IS_ERR(i2s->base))
-		return dev_err_probe(i2s->dev, PTR_ERR(i2s->base), "failed to map registers\n");
+		return PTR_ERR(i2s->base);
 
 	i2s->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
 	if (IS_ERR(i2s->reset))
@@ -495,7 +495,7 @@ static int spacemit_i2s_probe(struct platform_device *pdev)
 					      &spacemit_i2s_component,
 					      dai, 1);
 	if (ret)
-		return dev_err_probe(i2s->dev, ret, "failed to register component");
+		return ret;
 
 	return devm_snd_dmaengine_pcm_register(&pdev->dev, &spacemit_dmaengine_pcm_config, 0);
 }
-- 
2.43.0

[PATCH 2/2] ASoC: spacemit: init *dp to NULL before error paths

[phucduc.bui]

From: bui duc phuc <phucduc.bui@gmail.com>

spacemit_i2s_init_dai() takes an optional output parameter dp, but only
assigns *dp on the success path. If devm_kmemdup() fails, *dp is left
untouched.
The current caller does check the return value before using dp, so this
isn't an active bug. Still, initialize *dp to NULL upfront as a defensive
measure, consistent with how core helpers like _snd_pcm_new() handle
their optional output parameters.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/spacemit/k1_i2s.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/sound/soc/spacemit/k1_i2s.c b/sound/soc/spacemit/k1_i2s.c
index 64510c9a1a89..23037fa4233c 100644
--- a/sound/soc/spacemit/k1_i2s.c
+++ b/sound/soc/spacemit/k1_i2s.c
@@ -382,6 +382,9 @@ static int spacemit_i2s_init_dai(struct spacemit_i2s_dev *i2s,
 	struct property *dma_names;
 	const char *dma_name;
 
+	if (dp)
+		*dp = NULL;
+
 	of_property_for_each_string(node, "dma-names", dma_names, dma_name) {
 		if (!strcmp(dma_name, "tx"))
 			i2s->has_playback = true;
-- 
2.43.0

Re: [PATCH 2/2] ASoC: spacemit: init *dp to NULL before error paths

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

> The current caller does check the return value before using dp, so this
> isn't an active bug. Still, initialize *dp to NULL upfront as a defensive
> measure, consistent with how core helpers like _snd_pcm_new() handle
> their optional output parameters.

If devm_kmemdup() fails, spacemit_i2s_init_dai() returns -ENOMEM. The
sole caller checks the return value and returns immediately, so it never
accesses dai on that path. On success, dp is non-NULL and *dp is assigned
before the function returns.

Therefore, I do not see a path where initializing *dp to NULL has any
effect. Could you clarify what case this change is intended to handle?

                                            - Troy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

Re: [PATCH 2/2] ASoC: spacemit: init *dp to NULL before error paths

[Bui Duc Phuc]

Hi Troy,

Thank you for your feedback.

> > The current caller does check the return value before using dp, so this
> > isn't an active bug. Still, initialize *dp to NULL upfront as a defensive
> > measure, consistent with how core helpers like _snd_pcm_new() handle
> > their optional output parameters.
>
> If devm_kmemdup() fails, spacemit_i2s_init_dai() returns -ENOMEM. The
> sole caller checks the return value and returns immediately, so it never
> accesses dai on that path. On success, dp is non-NULL and *dp is assigned
> before the function returns.
>
> Therefore, I do not see a path where initializing *dp to NULL has any
> effect. Could you clarify what case this change is intended to handle?


My intention was to make the API a bit more defensive. While the current
implementation only has one failure path,  spacemit_i2s_init_dai() may
grow additional error paths in the future. Initializing *dp to NULL ensures
it is left in a well-defined state on any failure.

It would also avoid leaving dai uninitialized if a future caller accidentally
skipped checking the return value before using it.

Best regards,
Phuc

Re: [PATCH 2/2] ASoC: spacemit: init *dp to NULL before error paths

[Bui Duc Phuc]

Hi Troy,

By the way, I noticed that spacemit_i2s_init_dai() is quite similar to
rockchip_i2s_init_dai().

However, unlike the Rockchip driver, spacemit_i2s_dai already has most
of the playback
and capture capabilities initialized statically. As a result,
spacemit_i2s_init_dai() ends up
assigning the same values again, for example channels_min, channels_max, rates,
and formats, which seems redundant.

If you think that makes sense, I can either remove those redundant assignments,
or make it follow the Rockchip approach by keeping the static spacemit_i2s_dai
minimal and initializing those fields only in spacemit_i2s_init_dai().

Best regards,
Phuc

Re: [PATCH 2/2] ASoC: spacemit: init *dp to NULL before error paths

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]

> However, unlike the Rockchip driver, spacemit_i2s_dai already has most
> of the playback
> and capture capabilities initialized statically. As a result,
> spacemit_i2s_init_dai() ends up
> assigning the same values again, for example channels_min, channels_max,
> rates,
> and formats, which seems redundant.
>
> If you think that makes sense, I can either remove those redundant
> assignments,
> or make it follow the Rockchip approach by keeping the static
> spacemit_i2s_dai
> minimal and initializing those fields only in spacemit_i2s_init_dai().

Nice catch. Please follow the Rockchip approach: keep the static
spacemit_i2s_dai minimal and initialize the playback and capture fields
conditionally in spacemit_i2s_init_dai().

Simply removing the assignments from spacemit_i2s_init_dai() would leave
channels_min nonzero for both directions in the static template.
snd_soc_dai_stream_valid() treats a direction with a nonzero channels_min
as supported, so the driver would continue advertising capture even for
a device with only a "tx" DMA. The binding permits such a configuration.

Please preserve rate_min and rate_max when moving the capability fields,
and send this change as a separate patch.

                                            - Troy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

Re: [PATCH 2/2] ASoC: spacemit: init *dp to NULL before error paths

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1013 bytes --]

> My intention was to make the API a bit more defensive. While the current
> implementation only has one failure path, spacemit_i2s_init_dai() may
> grow additional error paths in the future. Initializing *dp to NULL
> ensures it is left in a well-defined state on any failure.
>
> It would also avoid leaving dai uninitialized if a future caller
> accidentally skipped checking the return value before using it.

I still do not think this initialization is necessary. Currently,
spacemit_i2s_init_dai() has exactly one failure path: devm_kmemdup() fails
and the function returns -ENOMEM. The sole caller checks that return value
and exits immediately.

A caller that continued after ignoring the error would itself be incorrect
and should not be accommodated. The helper is also static and has only this
one caller, so there is no current API contract that requires the output to
be initialized on failure.

I suggest dropping this patch.

                                            - Troy

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]

Re: [PATCH 1/2] ASoC: spacemit: Drop redundant error messages

[Troy Mitchell]

[-- Attachment #1: Type: text/plain, Size: 1453 bytes --]

On Fri Jul 31, 2026 at 3:15 AM PDT, phucduc.bui wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
>
> The called functions already log failures where appropriate. Return the
> original error directly and avoid duplicate error messages.
>
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>  sound/soc/spacemit/k1_i2s.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sound/soc/spacemit/k1_i2s.c b/sound/soc/spacemit/k1_i2s.c
> index cd461f2aa756..64510c9a1a89 100644
> --- a/sound/soc/spacemit/k1_i2s.c
> +++ b/sound/soc/spacemit/k1_i2s.c
> @@ -478,7 +478,7 @@ static int spacemit_i2s_probe(struct platform_device *pdev)
>  
>  	i2s->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
>  	if (IS_ERR(i2s->base))
> -		return dev_err_probe(i2s->dev, PTR_ERR(i2s->base), "failed to map registers\n");
> +		return PTR_ERR(i2s->base);
>  
>  	i2s->reset = devm_reset_control_get_exclusive(&pdev->dev, NULL);
>  	if (IS_ERR(i2s->reset))
> @@ -495,7 +495,7 @@ static int spacemit_i2s_probe(struct platform_device *pdev)
>  					      &spacemit_i2s_component,
>  					      dai, 1);
>  	if (ret)
> -		return dev_err_probe(i2s->dev, ret, "failed to register component");
> +		return ret;
>  
>  	return devm_snd_dmaengine_pcm_register(&pdev->dev, &spacemit_dmaengine_pcm_config, 0);
>  }
Reviewed-by: Troy Mitchell <troy.mitchell@linux.spacemit.com>


-- 
Troy Mitchell


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 248 bytes --]