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

[Ahmad Fatoum]

Hello Carlos,

On 20.12.24 07:58, Carlos Song wrote:
> 
> 
>> -----Original Message-----
>> From: Oleksij Rempel <o.rempel at pengutronix.de>
>> Sent: Friday, December 20, 2024 2:13 PM
>> To: Carlos Song <carlos.song at nxp.com>
>> Cc: Andi Shyti <andi.shyti at kernel.org>; Frank Li <frank.li at nxp.com>;
>> kernel at pengutronix.de; shawnguo at kernel.org; s.hauer at pengutronix.de;
>> festevam at gmail.com; linux-i2c at vger.kernel.org; imx at lists.linux.dev;
>> linux-arm-kernel at lists.infradead.org; linux-kernel at vger.kernel.org; Clark Wang
>> <xiaoning.wang at nxp.com>; Ahmad Fatoum <a.fatoum at 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.

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: +49-5121-206917-0
>> |
>> Amtsgericht Hildesheim, HRA 2686           | Fax:
>> +49-5121-206917-5555 |
> 


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