[v5] i2c: imx: support DMA defer probing

Commit Message

Carlos Song Dec. 18, 2024, 4:35 a.m. UTC

Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver
have not ready yet.

Move i2c_imx_dma_request() before registering I2C adapter to avoid
infinite loop of .probe() calls to the same driver, see "e8c220fac415
Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
and "Documentation/driver-api/driver-model/driver.rst".

Use CPU mode to avoid stuck registering i2c adapter when DMA resources
are unavailable.

Signed-off-by: Carlos Song <[e-mail]>
Signed-off-by: Clark Wang <[e-mail]>
Acked-by: Ahmad Fatoum <[e-mail]>
---
Change for V5:
- Add Ahmad Acked-by. No code change.
Change for V4:
- Output "Only use PIO mode" log in debug level when no DMA configure.
Change for V3:
- According to Marc's comment, remove error print when defer probe.
  Add info log when DMA has not been enabled and add error log when
  DMA error, both won't stuck the i2c adapter register, just for reminding,
  i2c adapter is working only in PIO mode.
Change for V2:
- According to Frank's comments, wrap at 75 char and Simplify fix code
  at i2c_imx_dma_request().
- Use strict patch check, fix alignment warning at i2c_imx_dma_request()
---
 drivers/i2c/busses/i2c-imx.c | 31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

Comments

Oleksij Rempel Dec. 19, 2024, 6:04 a.m. UTC | #1

On Wed, Dec 18, 2024 at 12:35:41PM +0800, Carlos Song wrote:
> Return -EPROBE_DEFER when dma_request_slave_channel() because DMA driver
> have not ready yet.
> 
> Move i2c_imx_dma_request() before registering I2C adapter to avoid
> infinite loop of .probe() calls to the same driver, see "e8c220fac415
> Revert "i2c: imx: improve the error handling in i2c_imx_dma_request()""
> and "Documentation/driver-api/driver-model/driver.rst".
> 
> Use CPU mode to avoid stuck registering i2c adapter when DMA resources
> are unavailable.
> 
> Signed-off-by: Carlos Song <[e-mail]>
> Signed-off-by: Clark Wang <[e-mail]>
> Acked-by: Ahmad Fatoum <[e-mail]>

Acked-by: Oleksij Rempel <[e-mail]>

Thank you!

Andi Shyti Dec. 19, 2024, 12:02 p.m. UTC | #2

Hi Carlos,

> +	/*
> +	 * Init DMA config if supported, -ENODEV means DMA not enabled at
> +	 * this platform, that is not a real error, so just remind "only
> +	 * PIO mode is used". If DMA is enabled, but meet error when request
> +	 * DMA channel, error should be showed in probe error log. PIO mode
> +	 * should be available regardless of DMA.
> +	 */
> +	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_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n");

Just for understanding, should we quit in this last case, as
well?

Before we were ignoring ENODEV and EPROBE_DEFER, but now you are
making it clear that other failures like ENOMEM might happen.

Thanks,
Andi

Oleksij Rempel Dec. 19, 2024, 12:22 p.m. UTC | #3

On Thu, Dec 19, 2024 at 01:02:29PM +0100, Andi Shyti wrote:
> Hi Carlos,
> 
> > +	/*
> > +	 * Init DMA config if supported, -ENODEV means DMA not enabled at
> > +	 * this platform, that is not a real error, so just remind "only
> > +	 * PIO mode is used". If DMA is enabled, but meet error when request
> > +	 * DMA channel, error should be showed in probe error log. PIO mode
> > +	 * should be available regardless of DMA.
> > +	 */
> > +	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_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n");
> 
> Just for understanding, should we quit in this last case, as
> well?
> 
> Before we were ignoring ENODEV and EPROBE_DEFER, but now you are
> making it clear that other failures like ENOMEM might happen.

Good point, dev_err_probe() would not print an error in case of EPROBE_DEFER,
but in this case we should only print error and continue with PIO.

Carlos Song Dec. 20, 2024, 2:45 a.m. UTC | #4

> -----Original Message-----
> From: Oleksij Rempel <[e-mail]>
> Sent: Thursday, December 19, 2024 8:23 PM
> To: Andi Shyti <[e-mail]>
> Cc: Carlos Song <[e-mail]>; Frank Li <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Clark Wang
> <[e-mail]>; Ahmad Fatoum <[e-mail]>
> 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 Thu, Dec 19, 2024 at 01:02:29PM +0100, Andi Shyti wrote:
> > Hi Carlos,
> >
> > > +   /*
> > > +    * Init DMA config if supported, -ENODEV means DMA not enabled at
> > > +    * this platform, that is not a real error, so just remind "only
> > > +    * PIO mode is used". If DMA is enabled, but meet error when request
> > > +    * DMA channel, error should be showed in probe error log. PIO mode
> > > +    * should be available regardless of DMA.
> > > +    */
> > > +   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_probe(&pdev->dev, ret, "Failed to setup
> > > + DMA, only use PIO mode\n");
> >
> > Just for understanding, should we quit in this last case, as well?
> >
> > Before we were ignoring ENODEV and EPROBE_DEFER, but now you are
> > making it clear that other failures like ENOMEM might happen.
>
> Good point, dev_err_probe() would not print an error in case of EPROBE_DEFER,
> but in this case we should only print error and continue with PIO.
>
Hi,

Thank you all very much! As I comment at previous mail:
DMA mode should be optional for i2c-imx, because i2c-imx can accept DMA mode not enabled, because it still can work in CPU mode.[1]
[1]https://lore.kernel.org/imx/[e-mail]/

Also we don't want to annoy current user without DMA[2]
[2] https://lore.kernel.org/imx/[e-mail]/

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.

> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7Cf0f2b14d05e4
> 4bb9581408dd2027ef82%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638702078063818207%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiO
> nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%
> 3D%3D%7C0%7C%7C%7C&sdata=q355QWfiCHuoREtM4pkuovpn%2F6MeXynD
> hrpSEpWeCXk%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Oleksij Rempel Dec. 20, 2024, 5:32 a.m. UTC | #5

On Fri, Dec 20, 2024 at 02:45:09AM +0000, Carlos Song wrote:
> 
> 
> > -----Original Message-----
> > From: Oleksij Rempel <[e-mail]>
> > Sent: Thursday, December 19, 2024 8:23 PM
> > To: Andi Shyti <[e-mail]>
> > Cc: Carlos Song <[e-mail]>; Frank Li <[e-mail]>;
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; Clark Wang
> > <[e-mail]>; Ahmad Fatoum <[e-mail]>
> > 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 Thu, Dec 19, 2024 at 01:02:29PM +0100, Andi Shyti wrote:
> > > Hi Carlos,
> > >
> > > > +   /*
> > > > +    * Init DMA config if supported, -ENODEV means DMA not enabled at
> > > > +    * this platform, that is not a real error, so just remind "only
> > > > +    * PIO mode is used". If DMA is enabled, but meet error when request
> > > > +    * DMA channel, error should be showed in probe error log. PIO mode
> > > > +    * should be available regardless of DMA.
> > > > +    */
> > > > +   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_probe(&pdev->dev, ret, "Failed to setup
> > > > + DMA, only use PIO mode\n");
> > >
> > > Just for understanding, should we quit in this last case, as well?
> > >
> > > Before we were ignoring ENODEV and EPROBE_DEFER, but now you are
> > > making it clear that other failures like ENOMEM might happen.
> >
> > Good point, dev_err_probe() would not print an error in case of EPROBE_DEFER,
> > but in this case we should only print error and continue with PIO.
> >
> Hi,
> 
> Thank you all very much! As I comment at previous mail:
> DMA mode should be optional for i2c-imx, because i2c-imx can accept DMA mode not enabled, because it still can work in CPU mode.[1]
> [1]https://lore.kernel.org/imx/[e-mail]/
> 
> Also we don't want to annoy current user without DMA[2]
> [2] https://lore.kernel.org/imx/[e-mail]/
> 
> 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()?

Carlos Song Dec. 20, 2024, 5:59 a.m. UTC | #6

> -----Original Message-----
> From: Oleksij Rempel <[e-mail]>
> Sent: Friday, December 20, 2024 1:33 PM
> To: Carlos Song <[e-mail]>
> Cc: Andi Shyti <[e-mail]>; Frank Li <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Clark Wang
> <[e-mail]>; Ahmad Fatoum <[e-mail]>
> 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 02:45:09AM +0000, Carlos Song wrote:
> >
> >
> > > -----Original Message-----
> > > From: Oleksij Rempel <[e-mail]>
> > > Sent: Thursday, December 19, 2024 8:23 PM
> > > To: Andi Shyti <[e-mail]>
> > > Cc: Carlos Song <[e-mail]>; Frank Li <[e-mail]>;
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > Clark Wang <[e-mail]>; Ahmad Fatoum
> > > <[e-mail]>
> > > 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 Thu, Dec 19, 2024 at 01:02:29PM +0100, Andi Shyti wrote:
> > > > Hi Carlos,
> > > >
> > > > > +   /*
> > > > > +    * Init DMA config if supported, -ENODEV means DMA not enabled
> at
> > > > > +    * this platform, that is not a real error, so just remind "only
> > > > > +    * PIO mode is used". If DMA is enabled, but meet error when
> request
> > > > > +    * DMA channel, error should be showed in probe error log. PIO
> mode
> > > > > +    * should be available regardless of DMA.
> > > > > +    */
> > > > > +   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_probe(&pdev->dev, ret, "Failed to
> > > > > + setup DMA, only use PIO mode\n");
> > > >
> > > > Just for understanding, should we quit in this last case, as well?
> > > >
> > > > Before we were ignoring ENODEV and EPROBE_DEFER, but now you are
> > > > making it clear that other failures like ENOMEM might happen.
> > >
> > > Good point, dev_err_probe() would not print an error in case of
> > > EPROBE_DEFER, but in this case we should only print error and continue with
> PIO.
> > >
> > Hi,
> >
> > Thank you all very much! As I comment at previous mail:
> > DMA mode should be optional for i2c-imx, because i2c-imx can accept
> > DMA mode not enabled, because it still can work in CPU mode.[1]
> > [1]https://l/
> >
> ore.kernel.org%2Fimx%2FAM0PR0402MB39374E34FD6133B5E3D414D7E82F2
> %40AM0P
> >
> R0402MB3937.eurprd04.prod.outlook.com%2F&data=05%7C02%7Ccarlos.song
> %40
> >
> nxp.com%7C5bfbfc0a74184fa1620808dd20b7c4db%7C686ea1d3bc2b4c6fa92c
> d99c5
> >
> c301635%7C0%7C0%7C638702695851431182%7CUnknown%7CTWFpbGZsb3d
> 8eyJFbXB0e
> >
> U1hcGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIl
> d
> >
> UIjoyfQ%3D%3D%7C0%7C%7C%7C&sdata=F28DC7RUwlqd5OIyh5QMVyqa1%2
> F%2BcX2p%2
> > FsFWftgDaliA%3D&reserved=0
> >
> > Also we don't want to annoy current user without DMA[2] [2]
> > https://lore/
> > .kernel.org%2Fimx%2F20241127-analytic-azure-hamster-727fd8-mkl%40peng
> u
> >
> tronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C5bfbfc0a74184fa
> 162
> >
> 0808dd20b7c4db%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C638
> 7026958
> >
> 51449521%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiI
> wLjAuM
> >
> DAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%
> 7C&s
> >
> data=y6pkewfVx7Mb0ZrPfRwDdAo69TV1eHOkRMnRq91Ec9Q%3D&reserved=0
> >
> > 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?

[1]https://lore.kernel.org/imx/[e-mail]/#t
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C5bfbfc0a7418
> 4fa1620808dd20b7c4db%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638702695851464654%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiO
> nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%
> 3D%3D%7C0%7C%7C%7C&sdata=X2AuLskzqYH9g34wfb4%2B%2Bs2OdZEW3q5
> i49V8OwKmgtw%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Oleksij Rempel Dec. 20, 2024, 6:12 a.m. UTC | #7

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

Carlos Song Dec. 20, 2024, 6:58 a.m. UTC | #8

> -----Original Message-----
> From: Oleksij Rempel <[e-mail]>
> Sent: Friday, December 20, 2024 2:13 PM
> To: Carlos Song <[e-mail]>
> Cc: Andi Shyti <[e-mail]>; Frank Li <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Clark Wang
> <[e-mail]>; Ahmad Fatoum <[e-mail]>
> 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?

> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C2950266755a
> 241c00a9208dd20bd5cf2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638702719862691439%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
> %3D%3D%7C0%7C%7C%7C&sdata=aIuzJP0v5C6HzOCGnCHobK9Llml3thHclTwu
> CjD13IM%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Ahmad Fatoum Dec. 20, 2024, 7:06 a.m. UTC | #9

Hello Carlos,

On 20.12.24 07:58, Carlos Song wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel <[e-mail]>
>> Sent: Friday, December 20, 2024 2:13 PM
>> To: Carlos Song <[e-mail]>
>> Cc: Andi Shyti <[e-mail]>; Frank Li <[e-mail]>;
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; [e-mail];
>> [e-mail]; [e-mail]; Clark Wang
>> <[e-mail]>; Ahmad Fatoum <[e-mail]>
>> 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.

Cheers,
Ahmad

> 
>> --
>> Pengutronix e.K.                           |
>> |
>> Steuerwalder Str. 21                       |
>> http://www.pen/
>> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C2950266755a
>> 241c00a9208dd20bd5cf2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
>> %7C638702719862691439%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
>> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
>> %3D%3D%7C0%7C%7C%7C&sdata=aIuzJP0v5C6HzOCGnCHobK9Llml3thHclTwu
>> CjD13IM%3D&reserved=0  |
>> 31137 Hildesheim, Germany                  | Phone: [phone]
>> |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:
>> [phone]-5555 |
>

Oleksij Rempel Dec. 20, 2024, 7:35 a.m. UTC | #10

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 <[e-mail]>
> >> Sent: Friday, December 20, 2024 2:13 PM
> >> To: Carlos Song <[e-mail]>
> >> Cc: Andi Shyti <[e-mail]>; Frank Li <[e-mail]>;
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; [e-mail];
> >> [e-mail]; [e-mail]; Clark Wang
> >> <[e-mail]>; Ahmad Fatoum <[e-mail]>
> >> 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.

Carlos Song Dec. 20, 2024, 7:38 a.m. UTC | #11

> -----Original Message-----
> From: Oleksij Rempel <[e-mail]>
> Sent: Friday, December 20, 2024 3:35 PM
> To: Ahmad Fatoum <[e-mail]>
> Cc: Carlos Song <[e-mail]>; Andi Shyti <[e-mail]>;
> Frank Li <[e-mail]>; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; Clark Wang <[e-mail]>
> 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 <[e-mail]>
> > >> Sent: Friday, December 20, 2024 2:13 PM
> > >> To: Carlos Song <[e-mail]>
> > >> Cc: Andi Shyti <[e-mail]>; Frank Li
> > >> <[e-mail]>; [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> [e-mail]; [e-mail];
> > >> Clark Wang <[e-mail]>; Ahmad Fatoum
> > >> <[e-mail]>
> > >> 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: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Oleksij Rempel Dec. 20, 2024, 7:45 a.m. UTC | #12

On Fri, Dec 20, 2024 at 07:38:47AM +0000, Carlos Song wrote:
> 
> 
> > -----Original Message-----
> > From: Oleksij Rempel <[e-mail]>
> > Sent: Friday, December 20, 2024 3:35 PM
> > To: Ahmad Fatoum <[e-mail]>
> > Cc: Carlos Song <[e-mail]>; Andi Shyti <[e-mail]>;
> > Frank Li <[e-mail]>; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail];
> > [e-mail]; [e-mail];
> > [e-mail]; Clark Wang <[e-mail]>
> > 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 <[e-mail]>
> > > >> Sent: Friday, December 20, 2024 2:13 PM
> > > >> To: Carlos Song <[e-mail]>
> > > >> Cc: Andi Shyti <[e-mail]>; Frank Li
> > > >> <[e-mail]>; [e-mail]; [e-mail];
> > > >> [e-mail]; [e-mail];
> > > >> [e-mail]; [e-mail];
> > > >> [e-mail]; [e-mail];
> > > >> Clark Wang <[e-mail]>; Ahmad Fatoum
> > > >> <[e-mail]>
> > > >> 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));

Carlos Song Dec. 20, 2024, 8:06 a.m. UTC | #13

> -----Original Message-----
> From: Oleksij Rempel <[e-mail]>
> Sent: Friday, December 20, 2024 3:46 PM
> To: Carlos Song <[e-mail]>
> Cc: Ahmad Fatoum <[e-mail]>; Andi Shyti
> <[e-mail]>; Frank Li <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Clark Wang
> <[e-mail]>
> 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 07:38:47AM +0000, Carlos Song wrote:
> >
> >
> > > -----Original Message-----
> > > From: Oleksij Rempel <[e-mail]>
> > > Sent: Friday, December 20, 2024 3:35 PM
> > > To: Ahmad Fatoum <[e-mail]>
> > > Cc: Carlos Song <[e-mail]>; Andi Shyti
> > > <[e-mail]>; Frank Li <[e-mail]>;
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail]; [e-mail];
> > > [e-mail];
> > > [e-mail]; Clark Wang <[e-mail]>
> > > 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 <[e-mail]>
> > > > >> Sent: Friday, December 20, 2024 2:13 PM
> > > > >> To: Carlos Song <[e-mail]>
> > > > >> Cc: Andi Shyti <[e-mail]>; Frank Li
> > > > >> <[e-mail]>; [e-mail]; [e-mail];
> > > > >> [e-mail]; [e-mail];
> > > > >> [e-mail]; [e-mail];
> > > > >> [e-mail];
> > > > >> [e-mail]; Clark Wang
> > > > >> <[e-mail]>; Ahmad Fatoum
> <[e-mail]>
> > > > >> 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));
>

Hi, the ret is from i2c_imx_dma_request() and look like that ret has been converted by PTR_ERR,
So the ret error has been human readable version?

static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr)
{
...
        dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
        if (!dma)
---->           return -ENOMEM;

        dma->chan_tx = dma_request_chan(dev, "tx");
        if (IS_ERR(dma->chan_tx)) {
---->           ret = PTR_ERR(dma->chan_tx);
                if (ret != -ENODEV && ret != -EPROBE_DEFER)
                        dev_err(dev, "can't request DMA tx channel (%d)\n", ret);
                goto fail_al;
        }

        dma_sconfig.dst_addr = phy_addr +
                                (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
        dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
        dma_sconfig.dst_maxburst = 1;
        dma_sconfig.direction = DMA_MEM_TO_DEV;
---->   ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
        if (ret < 0) {
                dev_err(dev, "can't configure tx channel (%d)\n", ret);
                goto fail_tx;
        }

        dma->chan_rx = dma_request_chan(dev, "rx");
        if (IS_ERR(dma->chan_rx)) {
---->           ret = PTR_ERR(dma->chan_rx);
                if (ret != -ENODEV && ret != -EPROBE_DEFER)
                        dev_err(dev, "can't request DMA rx channel (%d)\n", ret);
                goto fail_tx;
        }

        dma_sconfig.src_addr = phy_addr +
                                (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
        dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
        dma_sconfig.src_maxburst = 1;
        dma_sconfig.direction = DMA_DEV_TO_MEM;
        ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
        if (ret < 0) {
                dev_err(dev, "can't configure rx channel (%d)\n", ret);
                goto fail_rx;
        }

        i2c_imx->dma = dma;
        init_completion(&dma->cmd_complete);
        dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
                dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));

        return 0;

fail_rx:
        dma_release_channel(dma->chan_rx);
fail_tx:
        dma_release_channel(dma->chan_tx);
fail_al:
        devm_kfree(dev, dma);

        return ret;
}
>
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7Cd77fb08d389a
> 46773df008dd20ca5fdc%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%
> 7C638702775737161676%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiO
> nRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%
> 3D%3D%7C0%7C%7C%7C&sdata=B%2BR4YbexNiAlDpf5xTf%2BWp2GbmwtfG4
> yKaW8FBvaTRI%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Ahmad Fatoum Dec. 20, 2024, 8:40 a.m. UTC | #14

Hi,

On 20.12.24 09:06, Carlos Song wrote:
>> -----Original Message-----
>> On Fri, Dec 20, 2024 at 07:38:47AM +0000, Carlos Song wrote:
>>>> -----Original Message-----
>>>> From: Oleksij Rempel <[e-mail]>
>>>> Sent: Friday, December 20, 2024 3:35 PM
>>>> To: Ahmad Fatoum <[e-mail]>
>>>> Cc: Carlos Song <[e-mail]>; Andi Shyti
>>>> <[e-mail]>; Frank Li <[e-mail]>;
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail]; [e-mail]; [e-mail];
>>>> [e-mail];
>>>> [e-mail]; Clark Wang <[e-mail]>
>>>> Subject: [EXT] Re: [PATCH v5] i2c: imx: support DMA defer probing

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

Agreed, which is what spawned this thread in the first place.

If we want to ignore errors intentionally, I think a comment
like the following would make this clearer:

  /*
   * As we can always fall back to PIO, let's ignore the error setting up
   * DMA and see if we run into errors while setting up PIO mode.
   */


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

This is a fair point. I don't mind whether we use dev_err_probe or
dev_err with a return code, it's up to you ultimately. I just wanted
the error code to be included and I think a comment would be a good
idea to avoid confusion (provided we keep behavior as-is).

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

Sounds good to me.

> Hi, the ret is from i2c_imx_dma_request() and look like that ret has been converted by PTR_ERR,
> So the ret error has been human readable version?

I am not sure I understand the question. ERR_PTR() makes an error pointer and %pe
formats that pointer as error message. So you don't need to change any function
return types unless needed, just at the end print it with %pe instead of %d
(and after error pointer conversion if needed).

Cheers,
Ahmad

Carlos Song Dec. 20, 2024, 9:23 a.m. UTC | #15

> -----Original Message-----
> From: Ahmad Fatoum <[e-mail]>
> Sent: Friday, December 20, 2024 4:40 PM
> To: Carlos Song <[e-mail]>; Oleksij Rempel
> <[e-mail]>
> Cc: Andi Shyti <[e-mail]>; Frank Li <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Clark Wang
> <[e-mail]>
> 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
>
>
> Hi,
>
> On 20.12.24 09:06, Carlos Song wrote:
> >> -----Original Message-----
> >> On Fri, Dec 20, 2024 at 07:38:47AM +0000, Carlos Song wrote:
> >>>> -----Original Message-----
> >>>> From: Oleksij Rempel <[e-mail]>
> >>>> Sent: Friday, December 20, 2024 3:35 PM
> >>>> To: Ahmad Fatoum <[e-mail]>
> >>>> Cc: Carlos Song <[e-mail]>; Andi Shyti
> >>>> <[e-mail]>; Frank Li <[e-mail]>;
> >>>> [e-mail]; [e-mail];
> [e-mail];
> >>>> [e-mail]; [e-mail]; [e-mail];
> >>>> [e-mail];
> >>>> [e-mail]; Clark Wang <[e-mail]>
> >>>> Subject: [EXT] Re: [PATCH v5] i2c: imx: support DMA defer probing
>
> >>>>>> 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.
>
> Agreed, which is what spawned this thread in the first place.
>
> If we want to ignore errors intentionally, I think a comment like the following
> would make this clearer:
>
>   /*
>    * As we can always fall back to PIO, let's ignore the error setting up
>    * DMA and see if we run into errors while setting up PIO mode.
>    */
>
>
> >>>> 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.
>
> This is a fair point. I don't mind whether we use dev_err_probe or dev_err with
> a return code, it's up to you ultimately. I just wanted the error code to be
> included and I think a comment would be a good idea to avoid confusion
> (provided we keep behavior as-is).
>
> >>> 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));
>
> Sounds good to me.
>
> > Hi, the ret is from i2c_imx_dma_request() and look like that ret has
> > been converted by PTR_ERR, So the ret error has been human readable
> version?
>
> I am not sure I understand the question. ERR_PTR() makes an error pointer
> and %pe formats that pointer as error message. So you don't need to change
> any function return types unless needed, just at the end print it with %pe
> instead of %d (and after error pointer conversion if needed).
>
> Cheers,
> Ahmad
>

Sorry, I don't know if I understand it incorrectly.
I review other driver code, most choose to return error value but not an error pointer.
Shouldn't error value ​be more readable than error pointers?
When we see -110 we know TIMEOUT and we see -12 we know NO MEM.

i2c_imx_dma_request is using PTR_ERR to convert pointer to error value[1].
I don't know why need to use ERR_PTR to reconvert the value to pointer:

dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n", ERR_PTR(ret));

Is there some strong reason?

[1] https://lore.kernel.org/imx/[e-mail]/
>
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C594497db1b5
> 44e479a8f08dd20d1e88e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638702808104903131%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
> %3D%3D%7C0%7C%7C%7C&sdata=EnhsIFlBooqjB%2FSRWF7uAqRHE3yN6rbdD
> 1yQueTrRus%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Ahmad Fatoum Dec. 20, 2024, 9:31 a.m. UTC | #16

Hi Carlos,

On 20.12.24 10:23, Carlos Song wrote:
>>> Hi, the ret is from i2c_imx_dma_request() and look like that ret has
>>> been converted by PTR_ERR, So the ret error has been human readable
>> version?
>>
>> I am not sure I understand the question. ERR_PTR() makes an error pointer
>> and %pe formats that pointer as error message. So you don't need to change
>> any function return types unless needed, just at the end print it with %pe
>> instead of %d (and after error pointer conversion if needed).
> 
> Sorry, I don't know if I understand it incorrectly.
> I review other driver code, most choose to return error value but not an error pointer.
> Shouldn't error value ​be more readable than error pointers?
> When we see -110 we know TIMEOUT and we see -12 we know NO MEM.

I know -110, but -12 I need to look up :)
Both are cryptic to end users, which is why %pe was added
on top of the existing %p:

If CONFIG_SYMBOLIC_ERRNAME is enabled %pe expands to an error string,
e.g. "ENOMEM" or "ETIMEDOUT". If it's disabled, you get the same
error number that was printed raw before.

Cheers,
Ahmad

> 
> i2c_imx_dma_request is using PTR_ERR to convert pointer to error value[1].
> I don't know why need to use ERR_PTR to reconvert the value to pointer:
> 
> dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n", ERR_PTR(ret));
> 
> Is there some strong reason?
> 
> [1] https://lore.kernel.org/imx/[e-mail]/
>>
>> --
>> Pengutronix e.K.                           |
>> |
>> Steuerwalder Str. 21                       |
>> http://www.pen/
>> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C594497db1b5
>> 44e479a8f08dd20d1e88e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
>> %7C638702808104903131%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
>> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
>> %3D%3D%7C0%7C%7C%7C&sdata=EnhsIFlBooqjB%2FSRWF7uAqRHE3yN6rbdD
>> 1yQueTrRus%3D&reserved=0  |
>> 31137 Hildesheim, Germany                  | Phone: [phone]
>> |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:
>> [phone]-5555 |

Carlos Song Dec. 20, 2024, 10:46 a.m. UTC | #17

> -----Original Message-----
> From: Ahmad Fatoum <[e-mail]>
> Sent: Friday, December 20, 2024 5:32 PM
> To: Carlos Song <[e-mail]>; Oleksij Rempel
> <[e-mail]>
> Cc: Andi Shyti <[e-mail]>; Frank Li <[e-mail]>;
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Clark Wang
> <[e-mail]>
> 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
>
>
> Hi Carlos,
>
> On 20.12.24 10:23, Carlos Song wrote:
> >>> Hi, the ret is from i2c_imx_dma_request() and look like that ret has
> >>> been converted by PTR_ERR, So the ret error has been human readable
> >> version?
> >>
> >> I am not sure I understand the question. ERR_PTR() makes an error
> >> pointer and %pe formats that pointer as error message. So you don't
> >> need to change any function return types unless needed, just at the
> >> end print it with %pe instead of %d (and after error pointer conversion if
> needed).
> >
> > Sorry, I don't know if I understand it incorrectly.
> > I review other driver code, most choose to return error value but not an error
> pointer.
> > Shouldn't error value ​be more readable than error pointers?
> > When we see -110 we know TIMEOUT and we see -12 we know NO MEM.
>
> I know -110, but -12 I need to look up :) Both are cryptic to end users, which is
> why %pe was added on top of the existing %p:
>
> If CONFIG_SYMBOLIC_ERRNAME is enabled %pe expands to an error string, e.g.
> "ENOMEM" or "ETIMEDOUT". If it's disabled, you get the same error number
> that was printed raw before.
>
> Cheers,
> Ahmad
>

Wow! Looks so cool.
Thank you very much for your patient explanation! I agree it.

Also I will change the comment from your suggestion[1]:

"
  /*
   * As we can always fall back to PIO, let's ignore the error setting up
   * DMA and see if we run into errors while setting up PIO mode.
   */
"
In fact, other errors are also from DMA setting not from setting PIO mode.
So can I comment simply like this?

        /* As we can always fall back to PIO, let's ignore the error setting up DMA. */
        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 (%pe), only use PIO mode\n", ERR_PTR(ret));
        }

[1]https://lore.kernel.org/imx/[e-mail]/

BR
Carlos
> >
> > i2c_imx_dma_request is using PTR_ERR to convert pointer to error value[1].
> > I don't know why need to use ERR_PTR to reconvert the value to pointer:
> >
> > dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n",
> > ERR_PTR(ret));
> >
> > Is there some strong reason?
> >
> > [1]
> > https://lore/
> > .kernel.org%2Fimx%2FAM0PR0402MB3937419BBB58B75FB8F8DE2DE8072%
> 40AM0PR04
> >
> 02MB3937.eurprd04.prod.outlook.com%2F&data=05%7C02%7Ccarlos.song%4
> 0nxp
> > .com%7C13a8e6532dd7435c302008dd20d92819%7C686ea1d3bc2b4c6fa92c
> d99c5c30
> >
> 1635%7C0%7C0%7C638702839227618754%7CUnknown%7CTWFpbGZsb3d8eyJ
> FbXB0eU1h
> >
> cGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIj
> >
> oyfQ%3D%3D%7C0%7C%7C%7C&sdata=2A6qPlFEuW1SmlvOZ7OhtfO7VwqysM
> cGhFL2G%2F
> > ECNek%3D&reserved=0
> >>
> >> --
> >> Pengutronix e.K.                           |
> >> |
> >> Steuerwalder Str. 21                       |
> >> http://www/.
> >>
> pen%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C13a8e6532dd7435c30
> 2008d
> >>
> d20d92819%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63870283
> 922763
> >>
> 8077%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjA
> uMDAw
> >>
> MCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&s
> da
> >>
> ta=LasLtpe7GjsG5qVUKG%2FOmVro2JueGLfwUPALw7%2FB2fg%3D&reserved=
> 0
> >>
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C594497db1b5
> >>
> 44e479a8f08dd20d1e88e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> >> %7C638702808104903131%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hc
> Gki
> >>
> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyf
> >>
> Q %3D%3D%7C0%7C%7C%7C&sdata=EnhsIFlBooqjB%2FSRWF7uAqRHE3yN6rbd
> D
> >> 1yQueTrRus%3D&reserved=0  |
> >> 31137 Hildesheim, Germany                  | Phone:
> [phone]
> >> |
> >> Amtsgericht Hildesheim, HRA 2686           | Fax:
> >> [phone]-5555 |
>
>
> --
> Pengutronix e.K.                           |
> |
> Steuerwalder Str. 21                       |
> http://www.pen/
> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C13a8e6532dd
> 7435c302008dd20d92819%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
> %7C638702839227652725%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
> %3D%3D%7C0%7C%7C%7C&sdata=Hm60QL5dJ2h3OCesOXK871ASnK7s0L4SPiY
> bK0jtSOo%3D&reserved=0  |
> 31137 Hildesheim, Germany                  | Phone: [phone]
> |
> Amtsgericht Hildesheim, HRA 2686           | Fax:
> [phone]-5555 |

Ahmad Fatoum Dec. 20, 2024, 10:51 a.m. UTC | #18

Hi Carlos,

On 20.12.24 11:46, Carlos Song wrote:
> 
> 
>> -----Original Message-----
>> From: Ahmad Fatoum <[e-mail]>
>> Sent: Friday, December 20, 2024 5:32 PM
>> To: Carlos Song <[e-mail]>; Oleksij Rempel

>> I know -110, but -12 I need to look up :) Both are cryptic to end users, which is
>> why %pe was added on top of the existing %p:
>>
>> If CONFIG_SYMBOLIC_ERRNAME is enabled %pe expands to an error string, e.g.
>> "ENOMEM" or "ETIMEDOUT". If it's disabled, you get the same error number
>> that was printed raw before.
>>
>> Cheers,
>> Ahmad
>>
> 
> Wow! Looks so cool.
> Thank you very much for your patient explanation! I agree it.
> 
> Also I will change the comment from your suggestion[1]:
> 
> "
>   /*
>    * As we can always fall back to PIO, let's ignore the error setting up
>    * DMA and see if we run into errors while setting up PIO mode.
>    */
> "
> In fact, other errors are also from DMA setting not from setting PIO mode.
> So can I comment simply like this?
> 
>         /* As we can always fall back to PIO, let's ignore the error setting up DMA. */
>         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 (%pe), only use PIO mode\n", ERR_PTR(ret));
>         }
> 
> [1]https://lore.kernel.org/imx/[e-mail]/

Sure, looks good to me. @Oleksij?

Cheers,
Ahmad

> 
> BR
> Carlos
>>>
>>> i2c_imx_dma_request is using PTR_ERR to convert pointer to error value[1].
>>> I don't know why need to use ERR_PTR to reconvert the value to pointer:
>>>
>>> dev_err(&pdev->dev, "Failed to setup DMA (%pe), only use PIO mode\n",
>>> ERR_PTR(ret));
>>>
>>> Is there some strong reason?
>>>
>>> [1]
>>> https://lore/
>>> .kernel.org%2Fimx%2FAM0PR0402MB3937419BBB58B75FB8F8DE2DE8072%
>> 40AM0PR04
>>>
>> 02MB3937.eurprd04.prod.outlook.com%2F&data=05%7C02%7Ccarlos.song%4
>> 0nxp
>>> .com%7C13a8e6532dd7435c302008dd20d92819%7C686ea1d3bc2b4c6fa92c
>> d99c5c30
>>>
>> 1635%7C0%7C0%7C638702839227618754%7CUnknown%7CTWFpbGZsb3d8eyJ
>> FbXB0eU1h
>>>
>> cGkiOnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIj
>>>
>> oyfQ%3D%3D%7C0%7C%7C%7C&sdata=2A6qPlFEuW1SmlvOZ7OhtfO7VwqysM
>> cGhFL2G%2F
>>> ECNek%3D&reserved=0
>>>>
>>>> --
>>>> Pengutronix e.K.                           |
>>>> |
>>>> Steuerwalder Str. 21                       |
>>>> http://www/.
>>>>
>> pen%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C13a8e6532dd7435c30
>> 2008d
>>>>
>> d20d92819%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C63870283
>> 922763
>>>>
>> 8077%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGkiOnRydWUsIlYiOiIwLjA
>> uMDAw
>>>>
>> MCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ%3D%3D%7C0%7C%7C%7C&s
>> da
>>>>
>> ta=LasLtpe7GjsG5qVUKG%2FOmVro2JueGLfwUPALw7%2FB2fg%3D&reserved=
>> 0
>>>>
>> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C594497db1b5
>>>>
>> 44e479a8f08dd20d1e88e%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
>>>> %7C638702808104903131%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hc
>> Gki
>>>>
>> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyf
>>>>
>> Q %3D%3D%7C0%7C%7C%7C&sdata=EnhsIFlBooqjB%2FSRWF7uAqRHE3yN6rbd
>> D
>>>> 1yQueTrRus%3D&reserved=0  |
>>>> 31137 Hildesheim, Germany                  | Phone:
>> [phone]
>>>> |
>>>> Amtsgericht Hildesheim, HRA 2686           | Fax:
>>>> [phone]-5555 |
>>
>>
>> --
>> Pengutronix e.K.                           |
>> |
>> Steuerwalder Str. 21                       |
>> http://www.pen/
>> gutronix.de%2F&data=05%7C02%7Ccarlos.song%40nxp.com%7C13a8e6532dd
>> 7435c302008dd20d92819%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0
>> %7C638702839227652725%7CUnknown%7CTWFpbGZsb3d8eyJFbXB0eU1hcGki
>> OnRydWUsIlYiOiIwLjAuMDAwMCIsIlAiOiJXaW4zMiIsIkFOIjoiTWFpbCIsIldUIjoyfQ
>> %3D%3D%7C0%7C%7C%7C&sdata=Hm60QL5dJ2h3OCesOXK871ASnK7s0L4SPiY
>> bK0jtSOo%3D&reserved=0  |
>> 31137 Hildesheim, Germany                  | Phone: [phone]
>> |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:
>> [phone]-5555 |

Oleksij Rempel Dec. 20, 2024, 11:46 a.m. UTC | #19

On Fri, Dec 20, 2024 at 11:51:27AM +0100, Ahmad Fatoum wrote:
> Hi Carlos,
> 
> On 20.12.24 11:46, Carlos Song wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Ahmad Fatoum <[e-mail]>
> >> Sent: Friday, December 20, 2024 5:32 PM
> >> To: Carlos Song <[e-mail]>; Oleksij Rempel
> 
> >> I know -110, but -12 I need to look up :) Both are cryptic to end users, which is
> >> why %pe was added on top of the existing %p:
> >>
> >> If CONFIG_SYMBOLIC_ERRNAME is enabled %pe expands to an error string, e.g.
> >> "ENOMEM" or "ETIMEDOUT". If it's disabled, you get the same error number
> >> that was printed raw before.
> >>
> >> Cheers,
> >> Ahmad
> >>
> > 
> > Wow! Looks so cool.
> > Thank you very much for your patient explanation! I agree it.
> > 
> > Also I will change the comment from your suggestion[1]:
> > 
> > "
> >   /*
> >    * As we can always fall back to PIO, let's ignore the error setting up
> >    * DMA and see if we run into errors while setting up PIO mode.
> >    */
> > "
> > In fact, other errors are also from DMA setting not from setting PIO mode.
> > So can I comment simply like this?
> > 
> >         /* As we can always fall back to PIO, let's ignore the error setting up DMA. */
> >         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 (%pe), only use PIO mode\n", ERR_PTR(ret));
> >         }
> > 
> > [1]https://lore.kernel.org/imx/[e-mail]/
> 
> Sure, looks good to me. @Oleksij?

Ack, looks good. Thank you!

Patch

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index 9d5caa032c5c..0a5ddedf4dc1 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -397,17 +397,16 @@  static void i2c_imx_reset_regs(struct imx_i2c_struct *i2c_imx)
 }
 
 /* Functions for DMA support */
-static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
-						dma_addr_t phy_addr)
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx, dma_addr_t phy_addr)
 {
 	struct imx_i2c_dma *dma;
 	struct dma_slave_config dma_sconfig;
-	struct device *dev = &i2c_imx->adapter.dev;
+	struct device *dev = i2c_imx->adapter.dev.parent;
 	int ret;
 
 	dma = devm_kzalloc(dev, sizeof(*dma), GFP_KERNEL);
 	if (!dma)
-		return;
+		return -ENOMEM;
 
 	dma->chan_tx = dma_request_chan(dev, "tx");
 	if (IS_ERR(dma->chan_tx)) {
@@ -452,7 +451,7 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dev_info(dev, "using %s (tx) and %s (rx) for DMA transfers\n",
 		dma_chan_name(dma->chan_tx), dma_chan_name(dma->chan_rx));
 
-	return;
+	return 0;
 
 fail_rx:
 	dma_release_channel(dma->chan_rx);
@@ -460,6 +459,8 @@  static void i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
 	dma_release_channel(dma->chan_tx);
 fail_al:
 	devm_kfree(dev, dma);
+
+	return ret;
 }
 
 static void i2c_imx_dma_callback(void *arg)
@@ -1804,6 +1805,23 @@  static int i2c_imx_probe(struct platform_device *pdev)
 	if (ret == -EPROBE_DEFER)
 		goto clk_notifier_unregister;
 
+	/*
+	 * Init DMA config if supported, -ENODEV means DMA not enabled at
+	 * this platform, that is not a real error, so just remind "only
+	 * PIO mode is used". If DMA is enabled, but meet error when request
+	 * DMA channel, error should be showed in probe error log. PIO mode
+	 * should be available regardless of DMA.
+	 */
+	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_probe(&pdev->dev, ret, "Failed to setup DMA, only use PIO mode\n");
+	}
+
 	/* Add I2C adapter */
 	ret = i2c_add_numbered_adapter(&i2c_imx->adapter);
 	if (ret < 0)
@@ -1818,9 +1836,6 @@  static int i2c_imx_probe(struct platform_device *pdev)
 		i2c_imx->adapter.name);
 	dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
 
-	/* Init DMA config if supported */
-	i2c_imx_dma_request(i2c_imx, phy_addr);
-
 	return 0;   /* Return OK */
 
 clk_notifier_unregister: