Re: [PATCH v5] i2c: imx: support DMA defer probing

[Oleksij Rempel]

On Fri, Dec 20, 2024 at 08:06:25AM +0100, Ahmad Fatoum wrote:
> Hello Carlos,
> 
> On 20.12.24 07:58, Carlos Song wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> >> Sent: Friday, December 20, 2024 2:13 PM
> >> To: Carlos Song <carlos.song@xxxxxxx>
> >> Cc: Andi Shyti <andi.shyti@xxxxxxxxxx>; Frank Li <frank.li@xxxxxxx>;
> >> kernel@xxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx; s.hauer@xxxxxxxxxxxxxx;
> >> festevam@xxxxxxxxx; linux-i2c@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> >> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Clark Wang
> >> <xiaoning.wang@xxxxxxx>; Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> >> Subject: [EXT] Re: [PATCH v5] i2c: imx: support DMA defer probing
> >>
> >> Caution: This is an external email. Please take care when clicking links or
> >> opening attachments. When in doubt, report the message using the 'Report this
> >> email' button
> >>
> >>
> >> On Fri, Dec 20, 2024 at 05:59:38AM +0000, Carlos Song wrote:
> >>>>> So we make this logic. Anyway we let the I2C controller registered
> >>>>> whether
> >>>> DMA is available or not(except defer probe).
> >>>>> Ignoring ENODEV and EPROBE_DEFER makes it looks like nothing
> >>>>> happened if
> >>>> DMA is defer probed or not enabled(This is an expected).
> >>>>> However we still need i2c DMA status is known when meet an
> >>>>> unexpected
> >>>> error, so we use dev_err_probe() to print error.
> >>>>
> >>>> Why dev_err_probe() instead of dev_err()?
> >>>>
> >>> Hi,
> >>> In patch V2 discussion, Marc suggested just return dev_err_probe(),
> >>> but I don't accept it so I choose to use dev_err_probe() to print error in V3.[1]
> >> In this case, the two APIs have the same function, do you mean dev_err() is more
> >> suitable?
> >>
> >> Yes, dev_err_probe() should be used in combination with return. For
> >> example:
> >>   return dev_err_probe(...);
> >>
> >> It will pass the return value on exit of the function and optionally print of the
> >> error message if it is not EPROBE_DEFER. Practically it replace commonly used
> >> coding pattern:
> >>   if (ret == -EPROBE_DEFER) {
> >>     return ret;
> >>   } else if (ret) {
> >>     dev_err(..);
> >>     return ret;
> >>   }
> >>
> > Hi,
> > 
> > Get your good point. I will change my code in V6:
> > +       ret = i2c_imx_dma_request(i2c_imx, phy_addr);
> > +       if (ret) {
> > +               if (ret == -EPROBE_DEFER)
> > +                       goto clk_notifier_unregister;
> > +               else if (ret == -ENODEV)
> > +                       dev_dbg(&pdev->dev, "Only use PIO mode\n");
> > +               else
> > +                       dev_err(&pdev->dev, "Failed to setup DMA, only use PIO mode\n");
> > +       }
> > 
> > I think this is what you want to see, right?
> 
> This loses the information why the error happens (ret). Using dev_err_probe
> even if no probe deferral is expected in that branch is perfectly fine
> and the kernel-doc even points it out:
> 
>   Using this helper in your probe function is totally fine even if @err
>   is known to never be -EPROBE_DEFER.

Thank you for the feedback. While I recognize the benefits of
dev_err_probe() for compact and standardized error handling, using it
without returning its result raises a red flag.

The function's primary purpose is to combine error logging with
returning the error code. If the return value is not used, it can create
confusion and suggests potential oversight or unintended behavior. This
misuse might mislead readers into thinking that the function always
returns at that point, which is not the case here.

In this scenario, using dev_err() directly is more explicit and avoids
any ambiguity about the control flow or error handling intent. It keeps
the code clear and aligned with its actual behavior.

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |