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