> -----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);
+ }
We can both know the error and also use dev_err() directly.
> --
> Pengutronix e.K. |
> |
> Steuerwalder Str. 21 |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7Cae12fdac9f63
> 4b7d7d9408dd20c8d8b6%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638702769209008171%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
> %3D%3D%7C0%7C%7C%7C&sdata=DKjI9z82LWvymmNmnTRo33F9F%2FBOBb
> Wc7In0RS1xAPs%3D&reserved=0 |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
