| Message ID | 20250311180254.149484-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v4] dma-engine: sun4i: Use devm functions in probe() | expand |
> Clean up error handling by using devm functions > and dev_err_probe(). This should make it easier … You may occasionally put more than 47 characters into text lines of such a change description. Regards, Markus
> Clean up error handling by using devm functions > and dev_err_probe(). … How good does such a change combination fit to the patch requirement according to separation of concerns? https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81 Regards, Markus
Dear Markus, On 2025. 03. 11. 20:33, Markus Elfring wrote: >> Clean up error handling by using devm functions >> and dev_err_probe(). This should make it easier > … > > You may occasionally put more than 47 characters into text lines > of such a change description. It was an old patch I hadn't yet reformatted to 75 cols. I used 50-60 cols before, because my mail client's preview panel is very narrow, so anything more than ~65 characters will wrap. If there will be a v5, I'll reformat it to 75 cols as well, as per the style guidelines. > How good does such a change combination fit to the patch requirement > according to separation of concerns? > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81 It is a general refactor patch, it shouldn't change any functionality. I could split it to one part introducing `devm_clk_get_enabled()` and the other `dmaenginem_async_device_register()`, but I don't feel that to be necessary, nor does it bring any advantages I believe. > Regards, > Markus Bence
>> How good does such a change combination fit to the patch requirement >> according to separation of concerns? >> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81 > > It is a general refactor patch, it shouldn't change any functionality. I could split it to one part introducing `devm_clk_get_enabled()` and the other `dmaenginem_async_device_register()`, but I don't feel that to be necessary, nor does it bring any advantages I believe. Can it matter a bit more to separate changes for the application of devm functions and the adjustment of corresponding exception handling with dev_err_probe() calls? Regards, Markus
Hi, On 2025. 03. 12. 12:44, Markus Elfring wrote: >>> How good does such a change combination fit to the patch requirement >>> according to separation of concerns? >>> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc6#n81 >> >> It is a general refactor patch, it shouldn't change any functionality. I could split it to one part introducing `devm_clk_get_enabled()` and the other `dmaenginem_async_device_register()`, but I don't feel that to be necessary, nor does it bring any advantages I believe. > Can it matter a bit more to separate changes for the application of devm functions > and the adjustment of corresponding exception handling with dev_err_probe() calls? The change in error handling is just the result of switching to devm functions, because it is no longer needed to separately dev_err(), store the error code to `ret` and goto a cleanup phase (as the whole point of using devm functions is to have auto-cleanup), you can just return with the error code (which dev_err_probe() returns for us) right away. The devm functions are used precisely _because_ they allow us to simplify this error handling. > Regards, > Markus Bence
diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c index 24796aaaddfa..b10639720efd 100644 --- a/drivers/dma/sun4i-dma.c +++ b/drivers/dma/sun4i-dma.c @@ -1249,10 +1249,9 @@ static int sun4i_dma_probe(struct platform_device *pdev) if (priv->irq < 0) return priv->irq; - priv->clk = devm_clk_get(&pdev->dev, NULL); + priv->clk = devm_clk_get_enabled(&pdev->dev, NULL); if (IS_ERR(priv->clk)) { - dev_err(&pdev->dev, "No clock specified\n"); - return PTR_ERR(priv->clk); + return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), "Couldn't start the clock"); } if (priv->cfg->has_reset) { @@ -1328,12 +1327,6 @@ static int sun4i_dma_probe(struct platform_device *pdev) vchan_init(&vchan->vc, &priv->slave); } - ret = clk_prepare_enable(priv->clk); - if (ret) { - dev_err(&pdev->dev, "Couldn't enable the clock\n"); - return ret; - } - /* * Make sure the IRQs are all disabled and accounted for. The bootloader * likes to leave these dirty @@ -1344,32 +1337,23 @@ static int sun4i_dma_probe(struct platform_device *pdev) ret = devm_request_irq(&pdev->dev, priv->irq, sun4i_dma_interrupt, 0, dev_name(&pdev->dev), priv); if (ret) { - dev_err(&pdev->dev, "Cannot request IRQ\n"); - goto err_clk_disable; + return dev_err_probe(&pdev->dev, ret, "Cannot request IRQ"); } - ret = dma_async_device_register(&priv->slave); + ret = dmaenginem_async_device_register(&priv->slave); if (ret) { - dev_warn(&pdev->dev, "Failed to register DMA engine device\n"); - goto err_clk_disable; + return dev_err_probe(&pdev->dev, ret, "Failed to register DMA engine device"); } ret = of_dma_controller_register(pdev->dev.of_node, sun4i_dma_of_xlate, priv); if (ret) { - dev_err(&pdev->dev, "of_dma_controller_register failed\n"); - goto err_dma_unregister; + return dev_err_probe(&pdev->dev, ret, "Failed to register translation function"); } dev_dbg(&pdev->dev, "Successfully probed SUN4I_DMA\n"); return 0; - -err_dma_unregister: - dma_async_device_unregister(&priv->slave); -err_clk_disable: - clk_disable_unprepare(priv->clk); - return ret; } static void sun4i_dma_remove(struct platform_device *pdev) @@ -1380,9 +1364,6 @@ static void sun4i_dma_remove(struct platform_device *pdev) disable_irq(priv->irq); of_dma_controller_free(pdev->dev.of_node); - dma_async_device_unregister(&priv->slave); - - clk_disable_unprepare(priv->clk); } static struct sun4i_dma_config sun4i_a10_dma_cfg = {