[PATCH 0/2] ASoC: xilinx: Use dev_err_probe() and drop redundant error handling

[PATCH 0/2] ASoC: xilinx: Use dev_err_probe() and drop redundant error handling

[phucduc.bui]

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

Hi all,

This series cleans up error handling in the Xilinx ASoC drivers.
It replaces dev_err() followed by return with dev_err_probe() where
appropriate in probe paths, allowing deferred probe to be handled
correctly while simplifying the code.
It also removes redundant dev_err() calls after helper functions that
already report failures, avoiding duplicate error messages.

Best regards,
Phuc

bui duc phuc (2):
  ASoC: xilinx: xlnx_i2s: Use dev_err_probe() and drop redundant error
    handling
  ASoC: xilinx: xlnx_spdif: Use dev_err_probe() and drop redundant error
    handling

 sound/soc/xilinx/xlnx_i2s.c   | 18 +++++++-----------
 sound/soc/xilinx/xlnx_spdif.c | 30 ++++++++++--------------------
 2 files changed, 17 insertions(+), 31 deletions(-)

-- 
2.43.0

[PATCH 1/2] ASoC: xilinx: xlnx_i2s: Use dev_err_probe() and drop redundant error handling

[phucduc.bui]

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

Use dev_err_probe() for probe error handling where appropriate to
simplify the code and properly handle deferred probe.
Also remove redundant error messages when the called helper already
reports failures, returning the error directly to avoid duplicate
logging.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/xilinx/xlnx_i2s.c | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
index ca915a001ad5..0676da122edd 100644
--- a/sound/soc/xilinx/xlnx_i2s.c
+++ b/sound/soc/xilinx/xlnx_i2s.c
@@ -185,17 +185,15 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 		return PTR_ERR(drv_data->base);
 
 	ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
-	if (ret < 0) {
-		dev_err(dev, "cannot get supported channels\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot get supported channels\n");
+
 	drv_data->channels *= 2;
 
 	ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
-	if (ret < 0) {
-		dev_err(dev, "cannot get data width\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot get data width\n");
+
 	switch (drv_data->data_width) {
 	case 16:
 		format = SNDRV_PCM_FMTBIT_S16_LE;
@@ -233,10 +231,8 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
 
 	ret = devm_snd_soc_register_component(&pdev->dev, &xlnx_i2s_component,
 					      &drv_data->dai_drv, 1);
-	if (ret) {
-		dev_err(&pdev->dev, "i2s component registration failed\n");
+	if (ret)
 		return ret;
-	}
 
 	dev_info(&pdev->dev, "%s DAI registered\n", drv_data->dai_drv.name);
 
-- 
2.43.0

[PATCH 2/2] ASoC: xilinx: xlnx_spdif: Use dev_err_probe() and drop redundant error handling

[phucduc.bui]

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

Use dev_err_probe() for probe error handling where appropriate to
simplify the code and properly handle deferred probe.
Also remove redundant error messages when the called helper already
reports failures, returning the error directly to avoid duplicate
logging.

Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
---
 sound/soc/xilinx/xlnx_spdif.c | 30 ++++++++++--------------------
 1 file changed, 10 insertions(+), 20 deletions(-)

diff --git a/sound/soc/xilinx/xlnx_spdif.c b/sound/soc/xilinx/xlnx_spdif.c
index 017a64ab9f1e..8f4d238be890 100644
--- a/sound/soc/xilinx/xlnx_spdif.c
+++ b/sound/soc/xilinx/xlnx_spdif.c
@@ -249,21 +249,17 @@ static int xlnx_spdif_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	ctx->axi_clk = devm_clk_get_enabled(dev, "s_axi_aclk");
-	if (IS_ERR(ctx->axi_clk)) {
-		ret = PTR_ERR(ctx->axi_clk);
-		dev_err(dev, "failed to get s_axi_aclk(%d)\n", ret);
-		return ret;
-	}
+	if (IS_ERR(ctx->axi_clk))
+		return dev_err_probe(dev, PTR_ERR(ctx->axi_clk), "failed to get s_axi_aclk\n");
 
 	ctx->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(ctx->base))
 		return PTR_ERR(ctx->base);
 
 	ret = of_property_read_u32(node, "xlnx,spdif-mode", &ctx->mode);
-	if (ret < 0) {
-		dev_err(dev, "cannot get SPDIF mode\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot get SPDIF mode\n");
+
 	if (ctx->mode) {
 		dai_drv = &xlnx_spdif_tx_dai;
 	} else {
@@ -274,29 +270,23 @@ static int xlnx_spdif_probe(struct platform_device *pdev)
 		ret = devm_request_irq(dev, ret,
 				       xlnx_spdifrx_irq_handler,
 				       0, "XLNX_SPDIF_RX", ctx);
-		if (ret) {
-			dev_err(dev, "spdif rx irq request failed\n");
-			return -ENODEV;
-		}
+		if (ret)
+			return ret;
 
 		init_waitqueue_head(&ctx->chsts_q);
 		dai_drv = &xlnx_spdif_rx_dai;
 	}
 
 	ret = of_property_read_u32(node, "xlnx,aud_clk_i", &ctx->aclk);
-	if (ret < 0) {
-		dev_err(dev, "cannot get aud_clk_i value\n");
-		return ret;
-	}
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "cannot get aud_clk_i value\n");
 
 	dev_set_drvdata(dev, ctx);
 
 	ret = devm_snd_soc_register_component(dev, &xlnx_spdif_component,
 					      dai_drv, 1);
-	if (ret) {
-		dev_err(dev, "SPDIF component registration failed\n");
+	if (ret)
 		return ret;
-	}
 
 	writel(XSPDIF_SOFT_RESET_VALUE, ctx->base + XSPDIF_SOFT_RESET_REG);
 	dev_info(dev, "%s DAI registered\n", dai_drv->name);
-- 
2.43.0

Re: [PATCH 1/2] ASoC: xilinx: xlnx_i2s: Use dev_err_probe() and drop redundant error handling

[Michal Simek]

On 7/10/26 07:02, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Use dev_err_probe() for probe error handling where appropriate to
> simplify the code and properly handle deferred probe.
> Also remove redundant error messages when the called helper already
> reports failures, returning the error directly to avoid duplicate
> logging.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>   sound/soc/xilinx/xlnx_i2s.c | 18 +++++++-----------
>   1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/sound/soc/xilinx/xlnx_i2s.c b/sound/soc/xilinx/xlnx_i2s.c
> index ca915a001ad5..0676da122edd 100644
> --- a/sound/soc/xilinx/xlnx_i2s.c
> +++ b/sound/soc/xilinx/xlnx_i2s.c
> @@ -185,17 +185,15 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
>   		return PTR_ERR(drv_data->base);
>   
>   	ret = of_property_read_u32(node, "xlnx,num-channels", &drv_data->channels);
> -	if (ret < 0) {
> -		dev_err(dev, "cannot get supported channels\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "cannot get supported channels\n");
> +
>   	drv_data->channels *= 2;
>   
>   	ret = of_property_read_u32(node, "xlnx,dwidth", &drv_data->data_width);
> -	if (ret < 0) {
> -		dev_err(dev, "cannot get data width\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "cannot get data width\n");
> +
>   	switch (drv_data->data_width) {
>   	case 16:
>   		format = SNDRV_PCM_FMTBIT_S16_LE;
> @@ -233,10 +231,8 @@ static int xlnx_i2s_probe(struct platform_device *pdev)
>   
>   	ret = devm_snd_soc_register_component(&pdev->dev, &xlnx_i2s_component,
>   					      &drv_data->dai_drv, 1);
> -	if (ret) {
> -		dev_err(&pdev->dev, "i2s component registration failed\n");
> +	if (ret)
>   		return ret;
> -	}
>   
>   	dev_info(&pdev->dev, "%s DAI registered\n", drv_data->dai_drv.name);
>   

Reviewed-by: Michal Simek <michal.simek@amd.com>

Thanks,
Michal

Re: [PATCH 2/2] ASoC: xilinx: xlnx_spdif: Use dev_err_probe() and drop redundant error handling

[Michal Simek]

On 7/10/26 07:02, phucduc.bui@gmail.com wrote:
> From: bui duc phuc <phucduc.bui@gmail.com>
> 
> Use dev_err_probe() for probe error handling where appropriate to
> simplify the code and properly handle deferred probe.
> Also remove redundant error messages when the called helper already
> reports failures, returning the error directly to avoid duplicate
> logging.
> 
> Signed-off-by: bui duc phuc <phucduc.bui@gmail.com>
> ---
>   sound/soc/xilinx/xlnx_spdif.c | 30 ++++++++++--------------------
>   1 file changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/sound/soc/xilinx/xlnx_spdif.c b/sound/soc/xilinx/xlnx_spdif.c
> index 017a64ab9f1e..8f4d238be890 100644
> --- a/sound/soc/xilinx/xlnx_spdif.c
> +++ b/sound/soc/xilinx/xlnx_spdif.c
> @@ -249,21 +249,17 @@ static int xlnx_spdif_probe(struct platform_device *pdev)
>   		return -ENOMEM;
>   
>   	ctx->axi_clk = devm_clk_get_enabled(dev, "s_axi_aclk");
> -	if (IS_ERR(ctx->axi_clk)) {
> -		ret = PTR_ERR(ctx->axi_clk);
> -		dev_err(dev, "failed to get s_axi_aclk(%d)\n", ret);
> -		return ret;
> -	}
> +	if (IS_ERR(ctx->axi_clk))
> +		return dev_err_probe(dev, PTR_ERR(ctx->axi_clk), "failed to get s_axi_aclk\n");

This is pretty long line. Message should go on the next line.

>   
>   	ctx->base = devm_platform_ioremap_resource(pdev, 0);
>   	if (IS_ERR(ctx->base))
>   		return PTR_ERR(ctx->base);
>   
>   	ret = of_property_read_u32(node, "xlnx,spdif-mode", &ctx->mode);
> -	if (ret < 0) {
> -		dev_err(dev, "cannot get SPDIF mode\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "cannot get SPDIF mode\n");
> +
>   	if (ctx->mode) {
>   		dai_drv = &xlnx_spdif_tx_dai;
>   	} else {
> @@ -274,29 +270,23 @@ static int xlnx_spdif_probe(struct platform_device *pdev)
>   		ret = devm_request_irq(dev, ret,
>   				       xlnx_spdifrx_irq_handler,
>   				       0, "XLNX_SPDIF_RX", ctx);
> -		if (ret) {
> -			dev_err(dev, "spdif rx irq request failed\n");
> -			return -ENODEV;
> -		}
> +		if (ret)
> +			return ret;

Here you are changing error value and commit message is not saying anything 
about it.

>   
>   		init_waitqueue_head(&ctx->chsts_q);
>   		dai_drv = &xlnx_spdif_rx_dai;
>   	}
>   
>   	ret = of_property_read_u32(node, "xlnx,aud_clk_i", &ctx->aclk);
> -	if (ret < 0) {
> -		dev_err(dev, "cannot get aud_clk_i value\n");
> -		return ret;
> -	}
> +	if (ret < 0)
> +		return dev_err_probe(dev, ret, "cannot get aud_clk_i value\n");
>   
>   	dev_set_drvdata(dev, ctx);
>   
>   	ret = devm_snd_soc_register_component(dev, &xlnx_spdif_component,
>   					      dai_drv, 1);
> -	if (ret) {
> -		dev_err(dev, "SPDIF component registration failed\n");
> +	if (ret)
>   		return ret;
> -	}

And this is another case. Where origin code didn't return any error which was 
wrong. That's also not described in commit message and likely this should have 
Fixed tag.

Thanks,
Michal

Re: [PATCH 2/2] ASoC: xilinx: xlnx_spdif: Use dev_err_probe() and drop redundant error handling

[Bui Duc Phuc]

Hi Michal,

Thank you for your review !

> > +     if (IS_ERR(ctx->axi_clk))
> > +             return dev_err_probe(dev, PTR_ERR(ctx->axi_clk), "failed to get s_axi_aclk\n");
>
> This is pretty long line. Message should go on the next line.
>

The line is still within the 100-character limit.
I'll split the message onto the next line in the next version.

> > @@ -274,29 +270,23 @@ static int xlnx_spdif_probe(struct platform_device *pdev)
> >               ret = devm_request_irq(dev, ret,
> >                                      xlnx_spdifrx_irq_handler,
> >                                      0, "XLNX_SPDIF_RX", ctx);
> > -             if (ret) {
> > -                     dev_err(dev, "spdif rx irq request failed\n");
> > -                     return -ENODEV;
> > -             }
> > +             if (ret)
> > +                     return ret;
>
> Here you are changing error value and commit message is not saying anything
> about it.
>

Oh, sorry. I should have explained this in the commit message.
devm_request_irq() can return various error codes, such as -EINVAL, -ENOTCONN,
-ENOMEM, -ENOSYS, and -EBUSY. The existing code overwrites all of them with
-ENODEV, which does not reflect the actual failure.
I'll update the commit message to explain this change, and add a
Fixes: tag if appropriate.

> >       ret = devm_snd_soc_register_component(dev, &xlnx_spdif_component,
> >                                             dai_drv, 1);
> > -     if (ret) {
> > -             dev_err(dev, "SPDIF component registration failed\n");
> > +     if (ret)
> >               return ret;
> > -     }
>
> And this is another case. Where origin code didn't return any error which was
> wrong. That's also not described in commit message and likely this should have
> Fixed tag.
>

Sorry, I don't think I fully understand your point.
The original code already returns the error from
devm_snd_soc_register_component():

ret = devm_snd_soc_register_component(dev, &xlnx_spdif_component,
                                                                    dai_drv, 1);
if (ret) {
dev_err(dev, "SPDIF component registration failed\n");
return ret;
}

writel(XSPDIF_SOFT_RESET_VALUE, ctx->base + XSPDIF_SOFT_RESET_REG);
dev_info(dev, "%s DAI registered\n", dai_drv->name);

return 0;

As I understand it, this patch only removes the redundant dev_err()
call and still returns
the same error code.
Could you please clarify what you mean by "the original code didn't
return any error"?

Best regards,
Phuc

Re: [PATCH 2/2] ASoC: xilinx: xlnx_spdif: Use dev_err_probe() and drop redundant error handling

[Michal Simek]

On 7/10/26 10:17, Bui Duc Phuc wrote:
> Hi Michal,
> 
> Thank you for your review !
> 
>>> +     if (IS_ERR(ctx->axi_clk))
>>> +             return dev_err_probe(dev, PTR_ERR(ctx->axi_clk), "failed to get s_axi_aclk\n");
>>
>> This is pretty long line. Message should go on the next line.
>>
> 
> The line is still within the 100-character limit.
> I'll split the message onto the next line in the next version.
> 
>>> @@ -274,29 +270,23 @@ static int xlnx_spdif_probe(struct platform_device *pdev)
>>>                ret = devm_request_irq(dev, ret,
>>>                                       xlnx_spdifrx_irq_handler,
>>>                                       0, "XLNX_SPDIF_RX", ctx);
>>> -             if (ret) {
>>> -                     dev_err(dev, "spdif rx irq request failed\n");
>>> -                     return -ENODEV;
>>> -             }
>>> +             if (ret)
>>> +                     return ret;
>>
>> Here you are changing error value and commit message is not saying anything
>> about it.
>>
> 
> Oh, sorry. I should have explained this in the commit message.
> devm_request_irq() can return various error codes, such as -EINVAL, -ENOTCONN,
> -ENOMEM, -ENOSYS, and -EBUSY. The existing code overwrites all of them with
> -ENODEV, which does not reflect the actual failure.
> I'll update the commit message to explain this change, and add a
> Fixes: tag if appropriate.

I am not saying it is wrong. I am just saying that it should be likely handled 
as separate change/patch to address this issue.

> 
>>>        ret = devm_snd_soc_register_component(dev, &xlnx_spdif_component,
>>>                                              dai_drv, 1);
>>> -     if (ret) {
>>> -             dev_err(dev, "SPDIF component registration failed\n");
>>> +     if (ret)
>>>                return ret;
>>> -     }
>>
>> And this is another case. Where origin code didn't return any error which was
>> wrong. That's also not described in commit message and likely this should have
>> Fixed tag.
>>
> 
> Sorry, I don't think I fully understand your point.
> The original code already returns the error from
> devm_snd_soc_register_component():
> 
> ret = devm_snd_soc_register_component(dev, &xlnx_spdif_component,
>                                                                      dai_drv, 1);
> if (ret) {
> dev_err(dev, "SPDIF component registration failed\n");
> return ret;
> }
> 
> writel(XSPDIF_SOFT_RESET_VALUE, ctx->base + XSPDIF_SOFT_RESET_REG);
> dev_info(dev, "%s DAI registered\n", dai_drv->name);
> 
> return 0;
> 
> As I understand it, this patch only removes the redundant dev_err()
> call and still returns
> the same error code.
> Could you please clarify what you mean by "the original code didn't
> return any error"?

Sorry I misread this part. Please ignore my comment.

M

Re: [PATCH 2/2] ASoC: xilinx: xlnx_spdif: Use dev_err_probe() and drop redundant error handling

[Bui Duc Phuc]

Hi Michal,

Thanks for the clarification.

>
> I am not saying it is wrong. I am just saying that it should be likely handled
> as separate change/patch to address this issue.
>

I'll split it into a separate patch and send a v2 shortly.

Best regards,
Phuc