[v3,08/12] gpio: pxa: move gpio properties into child node

Commit Message

Haojian Zhuang Feb. 18, 2013, 5:12 a.m. UTC

Move gpio properties into child node. So pinctrl driver could binds to
each gpio chip with gpio range.

Signed-off-by: Haojian Zhuang <[e-mail]>
---
 drivers/gpio/gpio-pxa.c |   45 +++++++++++++++++----------------------------
 1 file changed, 17 insertions(+), 28 deletions(-)

Comments

Linus Walleij Feb. 21, 2013, 7:09 p.m. UTC | #1

On Mon, Feb 18, 2013 at 6:12 AM, Haojian Zhuang
<[e-mail]> wrote:

> Move gpio properties into child node. So pinctrl driver could binds to
> each gpio chip with gpio range.
>
> Signed-off-by: Haojian Zhuang <[e-mail]>
(...)
> +       np = pdev->dev.of_node;
> +#ifdef CONFIG_OF
> +       if (np)
> +               next = of_get_next_child(np, NULL);
> +#endif
(...)
> +#ifdef CONFIG_OF
> +               if (np) {
> +                       gc->of_node = next;
> +                       next = of_get_next_child(np, next);
> +
> +                       gc->of_xlate = pxa_gpio_of_xlate;
> +                       gc->of_gpio_n_cells = 2;
> +               }
>  #endif
(...)
> +       of_node_put(next);

So if you're #ifdef:in all the other of_* stuff, why are you not
#ifdef:in this?

Is the #ifdef really necessary by the way, or will there be stubs
handling it?

Have you considered using IS_ENABLED() from <linux/kconfig.h>?

Yours,
Linus Walleij

Haojian Zhuang Feb. 22, 2013, 1:32 a.m. UTC | #2

On 22 February 2013 03:09, Linus Walleij <[e-mail]> wrote:
> On Mon, Feb 18, 2013 at 6:12 AM, Haojian Zhuang
> <[e-mail]> wrote:
>
>> Move gpio properties into child node. So pinctrl driver could binds to
>> each gpio chip with gpio range.
>>
>> Signed-off-by: Haojian Zhuang <[e-mail]>
> (...)
>> +       np = pdev->dev.of_node;
>> +#ifdef CONFIG_OF
>> +       if (np)
>> +               next = of_get_next_child(np, NULL);
>> +#endif
> (...)
>> +#ifdef CONFIG_OF
>> +               if (np) {
>> +                       gc->of_node = next;
>> +                       next = of_get_next_child(np, next);
>> +
>> +                       gc->of_xlate = pxa_gpio_of_xlate;
>> +                       gc->of_gpio_n_cells = 2;
>> +               }
>>  #endif
> (...)
>> +       of_node_put(next);
>
> So if you're #ifdef:in all the other of_* stuff, why are you not
> #ifdef:in this?
>
> Is the #ifdef really necessary by the way, or will there be stubs
> handling it?
>
> Have you considered using IS_ENABLED() from <linux/kconfig.h>?
>
> Yours,
> Linus Walleij

I also like IS_ENABLED() macro. But the problem is that
of_get_next_child() isn't defined if CONFIG_OF isn't selected. So
I don't have the choice to use IS_ENABLED().

Regards
Haojian

Patch

diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c
index 8130e3b..5c39db3 100644
--- a/drivers/gpio/gpio-pxa.c
+++ b/drivers/gpio/gpio-pxa.c
@@ -63,10 +63,6 @@ 
 
 int pxa_last_gpio;
 
-#ifdef CONFIG_OF
-static struct device_node *pxa_gpio_of_node;
-#endif
-
 struct pxa_gpio_chip {
 	struct gpio_chip chip;
 	void __iomem	*regbase;
@@ -404,9 +400,9 @@  static struct of_device_id pxa_gpio_dt_ids[] = {
 
 static int pxa_gpio_probe_dt(struct platform_device *pdev)
 {
-	int ret, nr_banks;
+	int ret;
 	struct pxa_gpio_platform_data *pdata;
-	struct device_node *prev, *next, *np = pdev->dev.of_node;
+	struct device_node *np = pdev->dev.of_node;
 	const struct of_device_id *of_id =
 				of_match_device(pxa_gpio_dt_ids, &pdev->dev);
 
@@ -430,25 +426,7 @@  static int pxa_gpio_probe_dt(struct platform_device *pdev)
 	/* set the platform data */
 	pdev->dev.platform_data = pdata;
 
-	next = of_get_next_child(np, NULL);
-	prev = next;
-	if (!next) {
-		dev_err(&pdev->dev, "Failed to find child gpio node\n");
-		ret = -EINVAL;
-		goto err;
-	}
-	for (nr_banks = 1; ; nr_banks++) {
-		next = of_get_next_child(np, prev);
-		if (!next)
-			break;
-		prev = next;
-	}
-	of_node_put(prev);
-
 	return 0;
-err:
-	iounmap(gpio_reg_base);
-	return ret;
 }
 #else
 #define pxa_gpio_probe_dt(pdev)		(-1)
@@ -459,6 +437,7 @@  static int pxa_init_gpio_chip(struct platform_device *pdev, int gpio_end,
 {
 	int i, gpio, nbanks = gpio_to_bank(gpio_end) + 1;
 	struct pxa_gpio_chip *chips;
+	struct device_node *next = NULL, *np = NULL;
 
 	chips = devm_kzalloc(&pdev->dev, nbanks * sizeof(*chips), GFP_KERNEL);
 	if (chips == NULL) {
@@ -466,6 +445,11 @@  static int pxa_init_gpio_chip(struct platform_device *pdev, int gpio_end,
 		return -ENOMEM;
 	}
 
+	np = pdev->dev.of_node;
+#ifdef CONFIG_OF
+	if (np)
+		next = of_get_next_child(np, NULL);
+#endif
 	for (i = 0, gpio = 0; i < nbanks; i++, gpio += 32) {
 		struct gpio_chip *gc = &chips[i].chip;
 
@@ -492,13 +476,18 @@  static int pxa_init_gpio_chip(struct platform_device *pdev, int gpio_end,
 		gc->get = pxa_gpio_get;
 		gc->set = pxa_gpio_set;
 		gc->to_irq = pxa_gpio_to_irq;
-#ifdef CONFIG_OF_GPIO
-		gc->of_node = pxa_gpio_of_node;
-		gc->of_xlate = pxa_gpio_of_xlate;
-		gc->of_gpio_n_cells = 2;
+#ifdef CONFIG_OF
+		if (np) {
+			gc->of_node = next;
+			next = of_get_next_child(np, next);
+
+			gc->of_xlate = pxa_gpio_of_xlate;
+			gc->of_gpio_n_cells = 2;
+		}
 #endif
 		gpiochip_add(gc);
 	}
+	of_node_put(next);
 	pxa_gpio_chips = chips;
 	return 0;
 }