On Fri, Dec 20, 2024 at 07:38:47AM +0000, Carlos Song wrote:
>
>
> > -----Original Message-----
> > From: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> > Sent: Friday, December 20, 2024 3:35 PM
> > To: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>
> > Cc: Carlos Song <carlos.song@xxxxxxx>; 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>
> > 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@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.
> >
>
> 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 |
