diff mbox series

pinctrl: pistachio: Use scope-based resource management in pistachio_gpio_register()

Message ID 3f6fc17e-2ab4-43f2-b166-2393a369a263@web.de
State New
Headers show
Series pinctrl: pistachio: Use scope-based resource management in pistachio_gpio_register() | expand

Commit Message

Markus Elfring June 5, 2024, 4:02 p.m. UTC 
From: Markus Elfring <elfring@users.sourceforge.net>
Date: Wed, 5 Jun 2024 17:46:52 +0200

Scope-based resource management became supported also for another
programming interface by contributions of Jonathan Cameron on 2024-02-17.
See also the commit 59ed5e2d505bf5f9b4af64d0021cd0c96aec1f7c ("device
property: Add cleanup.h based fwnode_handle_put() scope based cleanup.").

* Thus use the attribute “__free(fwnode_handle)”.

* Reduce the scope for the local variable “child”.

* Omit explicit fwnode_handle_put() calls accordingly.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/pinctrl/pinctrl-pistachio.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

--
2.45.1

Comments

Linus Walleij June 17, 2024, 8:32 a.m. UTC | #1 
On Wed, Jun 5, 2024 at 6:02 PM Markus Elfring <Markus.Elfring@web.de> wrote:

> From: Markus Elfring <elfring@users.sourceforge.net>
> Date: Wed, 5 Jun 2024 17:46:52 +0200
>
> Scope-based resource management became supported also for another
> programming interface by contributions of Jonathan Cameron on 2024-02-17.
> See also the commit 59ed5e2d505bf5f9b4af64d0021cd0c96aec1f7c ("device
> property: Add cleanup.h based fwnode_handle_put() scope based cleanup.").
>
> * Thus use the attribute “__free(fwnode_handle)”.
>
> * Reduce the scope for the local variable “child”.
>
> * Omit explicit fwnode_handle_put() calls accordingly.
>
> Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>

Looks reasonable but I'd like Jonathan's and Andy's review tags on this.

Yours,
Linus Walleij
Jonathan Cameron June 17, 2024, 8:19 p.m. UTC | #2 
On Mon, 17 Jun 2024 10:32:03 +0200
Linus Walleij <linus.walleij@linaro.org> wrote:

> On Wed, Jun 5, 2024 at 6:02 PM Markus Elfring <Markus.Elfring@web.de> wrote:
> 
> > From: Markus Elfring <elfring@users.sourceforge.net>
> > Date: Wed, 5 Jun 2024 17:46:52 +0200
> >
> > Scope-based resource management became supported also for another
> > programming interface by contributions of Jonathan Cameron on 2024-02-17.
> > See also the commit 59ed5e2d505bf5f9b4af64d0021cd0c96aec1f7c ("device
> > property: Add cleanup.h based fwnode_handle_put() scope based cleanup.").
> >
> > * Thus use the attribute “__free(fwnode_handle)”.
> >
> > * Reduce the scope for the local variable “child”.
> >
> > * Omit explicit fwnode_handle_put() calls accordingly.
> >
> > Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>  
> 
> Looks reasonable but I'd like Jonathan's and Andy's review tags on this.
> 
> Yours,
> Linus Walleij

I took a look and it makes me a little nervous. Markus' patch isn't
changing behaviour (I think) but i is mixing scoped handling with non
scoped handling which I'm not keen on. I'm also struggling to understand
how the code was right prior to this patch.  In particular what cleans
up the fwnode_handle after the point where Markus has used no_free_ptr()?

Whilst it's assigned to the gpio_chip that chip hasn't been added when
girq->parents is allocated so even if there is some magic path to clearing
it up I'm not immediately seeing how that can be working here.

So this feels like there are more gremlins hiding here and Markus' patch
may not help flush them out.

Jonathan
diff mbox series

Patch

diff --git a/drivers/pinctrl/pinctrl-pistachio.c b/drivers/pinctrl/pinctrl-pistachio.c
index 53408344927a..d0a18296fb27 100644
--- a/drivers/pinctrl/pinctrl-pistachio.c
+++ b/drivers/pinctrl/pinctrl-pistachio.c
@@ -1368,11 +1368,12 @@  static int pistachio_gpio_register(struct pistachio_pinctrl *pctl)

 	for (i = 0; i < pctl->nbanks; i++) {
 		char child_name[sizeof("gpioXX")];
-		struct fwnode_handle *child;
 		struct gpio_irq_chip *girq;

 		snprintf(child_name, sizeof(child_name), "gpio%d", i);
-		child = device_get_named_child_node(pctl->dev, child_name);
+
+		struct fwnode_handle *child __free(fwnode_handle)
+					    = device_get_named_child_node(pctl->dev, child_name);
 		if (!child) {
 			dev_err(pctl->dev, "No node for bank %u\n", i);
 			ret = -ENODEV;
@@ -1380,7 +1381,6 @@  static int pistachio_gpio_register(struct pistachio_pinctrl *pctl)
 		}

 		if (!fwnode_property_present(child, "gpio-controller")) {
-			fwnode_handle_put(child);
 			dev_err(pctl->dev,
 				"No gpio-controller property for bank %u\n", i);
 			ret = -ENODEV;
@@ -1389,12 +1389,10 @@  static int pistachio_gpio_register(struct pistachio_pinctrl *pctl)

 		ret = fwnode_irq_get(child, 0);
 		if (ret < 0) {
-			fwnode_handle_put(child);
 			dev_err(pctl->dev, "Failed to retrieve IRQ for bank %u\n", i);
 			goto err;
 		}
 		if (!ret) {
-			fwnode_handle_put(child);
 			dev_err(pctl->dev, "No IRQ for bank %u\n", i);
 			ret = -EINVAL;
 			goto err;
@@ -1406,7 +1404,7 @@  static int pistachio_gpio_register(struct pistachio_pinctrl *pctl)
 		bank->base = pctl->base + GPIO_BANK_BASE(i);

 		bank->gpio_chip.parent = pctl->dev;
-		bank->gpio_chip.fwnode = child;
+		bank->gpio_chip.fwnode = no_free_ptr(child);

 		girq = &bank->gpio_chip.irq;
 		gpio_irq_chip_set_chip(girq, &pistachio_gpio_irq_chip);