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