From: Markus Elfring <[email protected]>
Date: Mon, 5 Feb 2024 11:36:51 +0100
A wrapper function is available since the commit 890cc39a879906b63912482dfc41944579df2dc6
("drivers: provide devm_platform_get_and_ioremap_resource()").
Thus reuse existing functionality instead of keeping duplicate source code.
This issue was detected by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
drivers/iommu/ipmmu-vmsa.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ace1fc4bd34b..12685dd2dd31 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1025,8 +1025,7 @@ static int ipmmu_probe(struct platform_device *pdev)
return ret;
/* Map I/O memory and request IRQ. */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- mmu->base = devm_ioremap_resource(&pdev->dev, res);
+ mmu->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
if (IS_ERR(mmu->base))
return PTR_ERR(mmu->base);
--
2.43.0
On 2024-02-05 10:42 am, Markus Elfring wrote:
> From: Markus Elfring <[email protected]>
> Date: Mon, 5 Feb 2024 11:36:51 +0100
>
> A wrapper function is available since the commit 890cc39a879906b63912482dfc41944579df2dc6
> ("drivers: provide devm_platform_get_and_ioremap_resource()").
> Thus reuse existing functionality instead of keeping duplicate source code.
Much as I detest the get_and_ioremap_resource obfuscator, it's not even
appropriate here since nothing else is using "res".
> This issue was detected by using the Coccinelle software.
Please improve your script, or at least manually review your patches to
ensure they actually make sense in context. If you want to do cleanup,
do thorough, meaningful cleanup - leaving weird dangling local variables
does not make the code clearer, if anything it makes it subtly worse
since now the reader's wondering why we've gone out of our way to make
this unused assignment, so maybe something's missing and there's a bug?
Thanks,
Robin.
> Signed-off-by: Markus Elfring <[email protected]>
> ---
> drivers/iommu/ipmmu-vmsa.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
> index ace1fc4bd34b..12685dd2dd31 100644
> --- a/drivers/iommu/ipmmu-vmsa.c
> +++ b/drivers/iommu/ipmmu-vmsa.c
> @@ -1025,8 +1025,7 @@ static int ipmmu_probe(struct platform_device *pdev)
> return ret;
>
> /* Map I/O memory and request IRQ. */
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - mmu->base = devm_ioremap_resource(&pdev->dev, res);
> + mmu->base = devm_platform_get_and_ioremap_resource(pdev, 0, &res);
> if (IS_ERR(mmu->base))
> return PTR_ERR(mmu->base);
>
> --
> 2.43.0
>
>> Thus reuse existing functionality instead of keeping duplicate source code.
>
> Much as I detest the get_and_ioremap_resource obfuscator, it's not even appropriate here since nothing else is using "res".
I got the impression that this local variable is needed to perform
a desired function call.
Regards,
Markus
On 2024-02-05 11:52 am, Markus Elfring wrote:
>>> Thus reuse existing functionality instead of keeping duplicate source code.
>>
>> Much as I detest the get_and_ioremap_resource obfuscator, it's not even appropriate here since nothing else is using "res".
>
> I got the impression that this local variable is needed to perform
> a desired function call.
Yes, the call to devm_ioremap_resource(). Which you're proposing to
remove...
Thanks,
Robin.
>>>> Thus reuse existing functionality instead of keeping duplicate source code.
>>>
>>> Much as I detest the get_and_ioremap_resource obfuscator, it's not even appropriate here since nothing else is using "res".
>>
>> I got the impression that this local variable is needed to perform
>> a desired function call.
>
> Yes, the call to devm_ioremap_resource(). Which you're proposing to remove...
I propose to replace a specific function call combination
by an other single call for a known wrapper function.
The mentioned variable is preserved for this purpose.
Regards,
Markus
On 2024-02-05 1:02 pm, Markus Elfring wrote:
>>>>> Thus reuse existing functionality instead of keeping duplicate source code.
>>>>
>>>> Much as I detest the get_and_ioremap_resource obfuscator, it's not even appropriate here since nothing else is using "res".
>>>
>>> I got the impression that this local variable is needed to perform
>>> a desired function call.
>>
>> Yes, the call to devm_ioremap_resource(). Which you're proposing to remove...
>
> I propose to replace a specific function call combination
> by an other single call for a known wrapper function.
> The mentioned variable is preserved for this purpose.
No. If you believe in cleaning up code, please apply your brain and
clean up code *meaningfully* - others seem to have managed it just fine
(e.g. [1][2] at random from a quick search on lore). Hell, I think even
the original now-deleted script could auto-generate a proper patch for
situations where devm_platform_ioremap_resource() was appropriate -
there is zero excuse for doing a *worse* job 4 years later.
If on the other hand your interest is not to actually clean up the code
for the good of the community, but to stoke your own ego as a
"contributor" of shitty patches that annoy people and reinforce the
popular impression that you are in fact an AI, please stop, or at the
very least please refrain from doing so in any areas I review or maintain.
Thanks,
Robin.
[1] https://lore.kernel.org/all/[email protected]/
[2]
https://lore.kernel.org/all/[email protected]/
>> Thus reuse existing functionality instead of keeping duplicate source code.
>
> Much as I detest the get_and_ioremap_resource obfuscator, it's not even appropriate here since nothing else is using "res".
Another adjustment might follow for the application of the function “devm_platform_ioremap_resource”.
https://elixir.bootlin.com/linux/v6.8-rc3/source/drivers/base/platform.c#L112
Regards,
Markus
From: Markus Elfring <[email protected]>
Date: Tue, 6 Feb 2024 15:00:23 +0100
A wrapper function is available since the commit 7945f929f1a77a1c8887a97ca07f87626858ff42
("drivers: provide devm_platform_ioremap_resource()").
* Thus reuse existing functionality instead of keeping duplicate source code.
* Delete a local variable which became unnecessary with this refactoring.
This issue was transformed by using the Coccinelle software.
Signed-off-by: Markus Elfring <[email protected]>
---
v2:
The transformation pattern was adjusted based on advices by known contributors.
Examples:
* Doug Anderson
* Geert Uytterhoeven
* Robin Murphy
drivers/iommu/ipmmu-vmsa.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index ace1fc4bd34b..7e28aac1e383 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -1005,7 +1005,6 @@ static const struct of_device_id ipmmu_of_ids[] = {
static int ipmmu_probe(struct platform_device *pdev)
{
struct ipmmu_vmsa_device *mmu;
- struct resource *res;
int irq;
int ret;
@@ -1025,8 +1024,7 @@ static int ipmmu_probe(struct platform_device *pdev)
return ret;
/* Map I/O memory and request IRQ. */
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- mmu->base = devm_ioremap_resource(&pdev->dev, res);
+ mmu->base = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(mmu->base))
return PTR_ERR(mmu->base);
--
2.43.0
On Tue, Feb 6, 2024 at 3:16 PM Markus Elfring <[email protected]> wrote:
> From: Markus Elfring <[email protected]>
> Date: Tue, 6 Feb 2024 15:00:23 +0100
>
> A wrapper function is available since the commit 7945f929f1a77a1c8887a97ca07f87626858ff42
> ("drivers: provide devm_platform_ioremap_resource()").
>
> * Thus reuse existing functionality instead of keeping duplicate source code.
>
> * Delete a local variable which became unnecessary with this refactoring.
>
>
> This issue was transformed by using the Coccinelle software.
>
> Signed-off-by: Markus Elfring <[email protected]>
> ---
>
> v2:
> The transformation pattern was adjusted based on advices by known contributors.
>
> Examples:
> * Doug Anderson
> * Geert Uytterhoeven
> * Robin Murphy
Reviewed-by: Geert Uytterhoeven <[email protected]>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68korg
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds