> -----Original Message-----
> From: Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx>
> Sent: Friday, December 20, 2024 3:46 PM
> To: Carlos Song <carlos.song@xxxxxxx>
> Cc: Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx>; 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 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));
>
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: +49-5121-206917-0
> |
> Amtsgericht Hildesheim, HRA 2686 | Fax:
> +49-5121-206917-5555 |
