| Message ID | 20250324172026.370253-2-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v6] dma-engine: sun4i: Simplify error handling in probe() | expand |
> Clean up error handling by using devm functions and dev_err_probe().
…
Do any contributors care for a different patch granularity?
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n81
Will it be clearer to mention also the function name “sun4i_dma_probe”
in the summary phrases?
Regards,
Markus
Hi, On 2025. 03. 24. 18:55, Markus Elfring wrote: >> Clean up error handling by using devm functions and dev_err_probe(). > … > > Do any contributors care for a different patch granularity? > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n81 I still don't understand why you are so adamant on this. It is just a simple refactor patch changing 33 lines, mostly in one function, with no logic change. Does it break something in your system? Please explain yourself so we can understand your viewpoint better. > Will it be clearer to mention also the function name “sun4i_dma_probe” > in the summary phrases? I already added it as per your last response, did you not read the message? On 2025. 03. 24. 18:20, 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 sun4i_dma_probe(). Bence
>>> Clean up error handling by using devm functions and dev_err_probe(). >> … >> >> Do any contributors care for a different patch granularity? >> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n81 > > I still don't understand why you are so adamant on this. There are developers who tend to prefer an other change granularity. > It is just a simple refactor patch changing 33 lines, The transformation goal is fine. > mostly in one function, with no logic change. Implementation details are probably worth for another look. > Does it break something in your system? Please explain yourself so we can understand your viewpoint better. I hope that the understanding can grow also for another bit of refinement. >> Will it be clearer to mention also the function name “sun4i_dma_probe” >> in the summary phrases? > > I already added it as per your last response, did you not read the message? > > On 2025. 03. 24. 18:20, 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 sun4i_dma_probe(). Please distinguish better between information from the “changelog” and items in a message subject. Regards, Markus
Hi, On 2025. 03. 25. 9:47, Markus Elfring wrote: > Implementation details are probably worth for another look. What don't you like in the implementation? Let's discuss that then. >> On 2025. 03. 24. 18:20, 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 sun4i_dma_probe(). > Please distinguish better between information from the “changelog” > and items in a message subject. What do you mean? The email body will be the commit message. > Regards, > Markus Bence
>> Implementation details are probably worth for another look. > > What don't you like in the implementation? Let's discuss that then. I dare to point concerns out also for the development process. >> Please distinguish better between information from the “changelog” >> and items in a message subject. > > What do you mean? The email body will be the commit message. See also: https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n623 Regards, Markus
Hi Markus, I really wanted to keep out of this, but... On Tue, Mar 25, 2025 at 8:14 PM Markus Elfring <Markus.Elfring@web.de> wrote: > > >> Implementation details are probably worth for another look. > > > > What don't you like in the implementation? Let's discuss that then. > > I dare to point concerns out also for the development process. You're "concerned" about patch granularity, but this is not the sort of thing that some random person would raise, this is the sort of thing a maintainer asks for when patches are doing too many things or are unreviewable. This is neither. It is a very simple cleanup of a probe function as it says in the patch subject. Futhermore, this already has an ack from the maintainer of this file. This indicates that they're happy with it and no significant changes are required. This is also version 6 of the patch, if the maintainer was concerned about this, they'd have already provided some clear guidance on this. If you check previous versions of this patch, no such requests have been made. Your only other "concern" had already been addressed as has already been pointed out to you. > >> Please distinguish better between information from the “changelog” > >> and items in a message subject. > > > > What do you mean? The email body will be the commit message. > See also: > https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n623 The email and patch structure are following the format outlined in the document you link to _exactly_. Once again your comments are just noise, and your insistence on repeating them over and over and over and over and over again is borderline harassment. You have been told to stop this nonsense many many times, here's a link to the most recent one: https://lore.kernel.org/all/92d1a410788c54facedec033474046dda6a1a2cc.camel@sipsolutions.net/ Please stop sending these emails and go do something constructive with your life. * * * * * Bence Csókás, (I hope I've got the order of your names correct) Please block or ignore Markus, at best he's a nuisance and at worst a troll. Thanks,
> You're "concerned" about patch granularity, but this is not the sort > of thing that some random person would raise, this is the sort of > thing a maintainer asks for when patches are doing too many things or > are unreviewable. … May additional patch reviewers influence the software evolution another bit? … >>> What do you mean? … … > Once again your comments are just noise, and your insistence on > repeating them over and over and over and over and over again is > borderline harassment. Some information needs to be repeated also according to known communication difficulties. Corresponding views might be evolving further. … > Please block or ignore Markus, at best he's a nuisance and at worst a troll. I hope that development discussions can become more constructive again. Regards, Markus
Hi Julian, On 2025. 03. 25. 13:20, Julian Calaby wrote: > Hi Markus, > > I really wanted to keep out of this, but... > > On Tue, Mar 25, 2025 at 8:14 PM Markus Elfring <Markus.Elfring@web.de> wrote: >> >>>> Implementation details are probably worth for another look. >>> >>> What don't you like in the implementation? Let's discuss that then. >> >> I dare to point concerns out also for the development process. > > You're "concerned" about patch granularity, but this is not the sort > of thing that some random person would raise, this is the sort of > thing a maintainer asks for when patches are doing too many things or > are unreviewable. This is neither. It is a very simple cleanup of a > probe function as it says in the patch subject. Exactly. That's why I asked if it broke something on Markus' end, because it is a really specific thing to nitpick about, especially in a changeset this small. So far, he did not indicate the *reasons* why he thinks this should be split further. > Futhermore, this already has an ack from the maintainer of this file. > This indicates that they're happy with it and no significant changes > are required. This is also version 6 of the patch, if the maintainer > was concerned about this, they'd have already provided some clear > guidance on this. If you check previous versions of this patch, no > such requests have been made. > > Your only other "concern" had already been addressed as has already > been pointed out to you. > >>>> Please distinguish better between information from the “changelog” >>>> and items in a message subject. >>> >>> What do you mean? The email body will be the commit message. >> See also: >> https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n623 > > The email and patch structure are following the format outlined in the > document you link to _exactly_. > > > Once again your comments are just noise, and your insistence on > repeating them over and over and over and over and over again is > borderline harassment. > > You have been told to stop this nonsense many many times, here's a > link to the most recent one: > > https://lore.kernel.org/all/92d1a410788c54facedec033474046dda6a1a2cc.camel@sipsolutions.net/ > > Please stop sending these emails and go do something constructive with > your life. > > * * * * * > > Bence Csókás, (I hope I've got the order of your names correct) Either order works, Bence is the given name, and Csókás is the family name (surname). Hungarian and Japanese order follows the scientific "Surname, Given Name(s)" order, but commas broke many tools, including Git < v2.46, and b4, so I switched to the germanic "Firstname Lastname" format. > Please block or ignore Markus, at best he's a nuisance and at worst a troll. I'm still open to hear him out if, and only if, he can give *clear and valid* reasoning on why he wants to achieve this. I'm a generally understanding person. But if it's just hand-waving and linking to the same page over and over again with no explanation on why he _thinks_ I broke SubmittingPatches, then I will do exactly this. Lastly, to all other adressees, sorry for the spam. So let's end this meta-discussion here and keep the rest of the conversation professional, reasoning about the technicals. Bence
Hi Bence, On Tue, Mar 25, 2025 at 11:39 PM Csókás Bence <csokas.bence@prolan.hu> wrote: > > Hi Julian, > > On 2025. 03. 25. 13:20, Julian Calaby wrote: [snip] > > Bence Csókás, (I hope I've got the order of your names correct) > > Either order works, Bence is the given name, and Csókás is the family > name (surname). Hungarian and Japanese order follows the scientific > "Surname, Given Name(s)" order, but commas broke many tools, including > Git < v2.46, and b4, so I switched to the germanic "Firstname Lastname" > format. Thanks for that, I'll try to keep that in mind! [snip] > Lastly, to all other adressees, sorry for the spam. So let's end this > meta-discussion here and keep the rest of the conversation professional, > reasoning about the technicals. And this is why I was so hessitant to step in here. Getting back to that, your patch looks good to me and it's awesome how the devm_ functions and their frends can simplify things. I'm going to point out that this does swap the clock enable and reset deassert, but I'm assuming that is harmless. Reviewed-by: Julian Calaby <julian.calaby@gmail.com> Thanks,
diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c index 24796aaaddfa..00d2fd38d17f 100644 --- a/drivers/dma/sun4i-dma.c +++ b/drivers/dma/sun4i-dma.c @@ -1249,11 +1249,10 @@ static int sun4i_dma_probe(struct platform_device *pdev) if (priv->irq < 0) return priv->irq; - priv->clk = devm_clk_get(&pdev->dev, NULL); - if (IS_ERR(priv->clk)) { - dev_err(&pdev->dev, "No clock specified\n"); - return PTR_ERR(priv->clk); - } + priv->clk = devm_clk_get_enabled(&pdev->dev, NULL); + if (IS_ERR(priv->clk)) + return dev_err_probe(&pdev->dev, PTR_ERR(priv->clk), + "Couldn't start the clock\n"); if (priv->cfg->has_reset) { priv->rst = devm_reset_control_get_exclusive_deasserted(&pdev->dev, NULL); @@ -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 @@ -1343,33 +1336,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; - } + if (ret) + return dev_err_probe(&pdev->dev, ret, "Cannot request IRQ\n"); - ret = dma_async_device_register(&priv->slave); - if (ret) { - dev_warn(&pdev->dev, "Failed to register DMA engine device\n"); - goto err_clk_disable; - } + ret = dmaenginem_async_device_register(&priv->slave); + if (ret) + 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; - } + if (ret) + 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 +1363,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 = {