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

[Michal Simek <michal.simek@amd.com>]

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