Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”

From: Markus Elfring <Markus.Elfring@web.de>
To: Julia Lawall <julia.lawall@inria.fr>,
	Coccinelle <cocci@systeme.lip6.fr>,
	Dejin Zheng <zhengdejin5@gmail.com>
Cc: Michal Marek <michal.lkml@markovi.net>,
	Gilles Muller <Gilles.Muller@lip6.fr>,
	Nicolas Palix <nicolas.palix@imag.fr>,
	kernel-janitors@vger.kernel.org,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [Cocci] [PATCH] Coccinelle: api: Add SmPL script “use_devm_platform_get_and_ioremap_resource.cocci”
Date: Mon, 7 Sep 2020 15:05:14 +0200	[thread overview]
Message-ID: <477abcea-e008-e509-d03f-f2753ebdfb20@web.de> (raw)
In-Reply-To: <alpine.DEB.2.22.394.2009071357140.2476@hadrien>

>> +@display depends on context@
>> +expression base, device1, device2, index, private, resource;
>> +@@
>> +(
>> +*resource = platform_get_resource(device1, IORESOURCE_MEM, index);
>> + base =
>> +*       devm_ioremap_resource
>> +                             (&device1->dev, resource);
>
> Why do you require these statements to be next to each other?

I would appreciate indications for a general change acceptance
also according to such a simple transformation approach.
I imagine that it will become more challenging to tolerate extra
source code between these function calls (by the specification
of special SmPL filters).

>> +|
>> +*private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
>> + base =
>> +*       devm_ioremap_resource
>> +                             (device2, private->res);
>
> Why do you have this special case?

The usage of the data structure member “res” triggers corresponding
software design consequences.
The expressions which are passed as the first function call parameters
can be different.

>> +@replacement depends on patch@
>> +expression base, device1, device2, index, private, resource;
>> +@@
>> +(
>> +-resource = platform_get_resource(device1, IORESOURCE_MEM, index);
>> + base =
>> +-       devm_ioremap_resource
>> ++       devm_platform_get_and_ioremap_resource
>> +                             (
>> +-                             &
>> +                               device1
>> +-                                     ->dev
>> +                              ,
>> +-                             resource
>> ++                             index, &resource
>> +                             );
>> +|
>> +-private->res = platform_get_resource(device1, IORESOURCE_MEM, index);
>> + base =
>> +-       devm_ioremap_resource
>> ++       devm_platform_get_and_ioremap_resource
>> +                             (device2,
>
> It is very suspicious that in one case you change the first argument of
> devm_platform_get_and_ioremap_resource and in one case you don't.

I noticed a few special cases during my source code analysis approach.

> If you don't know how to make the change in some cases, it would be better
> to do nothing at all.

How do you think about to take another look at any update candidates?

Examples:
* mvebu_sei_probe
  https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/irqchip/irq-mvebu-sei.c#L368
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/irqchip/irq-mvebu-sei.c?id=f4d51dffc6c01a9e94650d95ce0104964f8ae822#n368

* hi655x_pmic_probe
  https://elixir.bootlin.com/linux/v5.9-rc4/source/drivers/mfd/hi655x-pmic.c#L92
  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/mfd/hi655x-pmic.c?id=f4d51dffc6c01a9e94650d95ce0104964f8ae822#n92

Regards,
Markus
_______________________________________________
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci