| Message ID | 20250311180254.149484-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | Superseded |
| 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
Hi all, On 2025. 03. 11. 19:02, Bence Csókás wrote: > 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> So, will this be merged for the MW of 6.15? I just looked at the history of this patch, and there haven't been any major changes since v1, which was sent on 8 Dec last year. Despite this, it didn't make it into 6.14, probably because it was forgot amid the MW rush, so now I'm anxious as to whether it will happen again. Bence
Le 11/03/2025 à 19:02, 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 > > 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..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"); Why removing the trailing \n everywhere? Any reference esxplaing why it is correct? 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"); > } > > - 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 = {
Hi, On 2025. 03. 22. 20:13, Christophe JAILLET wrote: > Le 11/03/2025 à 19:02, 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 >> >> 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..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"); > > Why removing the trailing \n everywhere? > > Any reference esxplaing why it is correct? > > CJ Well, printk() will add a newline unless LOG_CONT is used. However, while looking up this feature, it seems that the consensus is to keep the `\n`s, even though they are not strictly needed anymore. I'll update this. 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 = {