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

[Oleksij Rempel <o.rempel@pengutronix.de>]

On Fri, Dec 20, 2024 at 07:38:47AM +0000, Carlos Song wrote:
> 
> 
> > -----Original Message-----
> > From: Oleksij Rempel <o.rempel@pengutronix.de>
> > Sent: Friday, December 20, 2024 3:35 PM
> > To: Ahmad Fatoum <a.fatoum@pengutronix.de>
> > Cc: Carlos Song <carlos.song@nxp.com>; Andi Shyti <andi.shyti@kernel.org>;
> > Frank Li <frank.li@nxp.com>; kernel@pengutronix.de; shawnguo@kernel.org;
> > s.hauer@pengutronix.de; festevam@gmail.com; linux-i2c@vger.kernel.org;
> > imx@lists.linux.dev; linux-arm-kernel@lists.infradead.org;
> > linux-kernel@vger.kernel.org; Clark Wang <xiaoning.wang@nxp.com>
> > 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 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@pengutronix.de>
> > > >> Sent: Friday, December 20, 2024 2:13 PM
> > > >> To: Carlos Song <carlos.song@nxp.com>
> > > >> Cc: Andi Shyti <andi.shyti@kernel.org>; Frank Li
> > > >> <frank.li@nxp.com>; kernel@pengutronix.de; shawnguo@kernel.org;
> > > >> s.hauer@pengutronix.de; festevam@gmail.com;
> > > >> linux-i2c@vger.kernel.org; imx@lists.linux.dev;
> > > >> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org;
> > > >> Clark Wang <xiaoning.wang@nxp.com>; Ahmad Fatoum
> > > >> <a.fatoum@pengutronix.de>
> > > >> 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.
> >
> 
> how about this?
> 
> +       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 (%d), only use PIO mode\n", ret);
> +       }

Please use human readable version of error value. In this case it will
be:
  dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n", ERR_PTR(ret));


-- 
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 |