| Message ID | 20250322193640.246382-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v5] dma-engine: sun4i: Use devm functions in probe() | expand |
Le 22/03/2025 à 20:36, Bence Csókás a écrit : > Clean up error handling by using devm functions and dev_err_probe(). This > should make it easier to add new code, as we can eliminate the "goto > ladder" in probe(). > > Suggested-by: Chen-Yu Tsai <wens@kernel.org> > Reviewed-by: Jernej Skrabec <jernej.skrabec@gmail.com> > Acked-by: Chen-Yu Tsai <wens@csie.org> > Signed-off-by: Bence Csókás <csokas.bence@prolan.hu> > --- > > Notes: > Changes in v2: > * rebase on current next > Changes in v3: > * rebase on current next > * collect Jernej's tag > Changes in v4: > * rebase on current next > * collect Chen-Yu's tag > Changes in v5: > * reformat msg to 75 cols > * keep `\n`s in error messages > > drivers/dma/sun4i-dma.c | 31 ++++++------------------------- > 1 file changed, 6 insertions(+), 25 deletions(-) > > diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c > index 24796aaaddfa..59ecebfc8eed 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\n"); I think that it would be better to keep the message on another line, to avoid too long lines. > } In several places, now {} around a single statement looks unneeded. CJ > > 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\n"); > } > > - 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\n"); > } > > 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\n"); > } > > 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 = {
> Clean up … and dev_err_probe(). …
I find it more reasonable to offer such a change in a separate update step.
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14-rc7#n81
Will it be clearer to mention also the function name “sun4i_dma_probe”
in the summary phrases?
Regards,
Markus
diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c index 24796aaaddfa..59ecebfc8eed 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\n"); } 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\n"); } - 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\n"); } 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\n"); } 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 = {