[PATCH 0/7] ARM: shmobile: Move gpio ranges from C code to DT

[PATCH 0/7] ARM: shmobile: Move gpio ranges from C code to DT

[Geert Uytterhoeven]

	Hi Linus, Alex, Laurent, Simon, Magnus,

This patch series moves the setup of the GPIO-PFC pin mapping for
Renesas PFC/GPIO combos from C code to DT, and does some cleanups.
The move to DT is needed to make the GPIO hogging mechanism work, cfr.
the discussion following "[PATCH] [RFC] gpio: Retry deferred GPIO
hogging on pin range change" (https://lkml.org/lkml/2015/6/16/455).

The series consists of 3 parts:
  a. Patches 1-3 add the missing "gpio-ranges" properties to the dtsi
     files for all affected SoCs,
  b. Patch 4 disables the C code to set up the mapping on DT platforms
     (it's still needed on SH or ARM-legacy),
  c. Patches 5-7 do a few more cleanups in the sh-pfc gpio code.

Dependencies:
  - This series applies against next-next-20150630.
  - Part a must go in first, to avoid regressions.
    In addition, it's a prerequisite for "ARM: shmobile: armadillo800eva
    dts: Add pinctrl and gpio-hog for lcdc0", which has been queued up
    already in Simon's dt-for-v4.3 branch. Perhaps it can still be
    reordered?
  - While I didn't notice any bad behavior by having part a only, part b
    should go in immediately after part a. Hence I think it's best if
    Simon can take this one, too.
  - Part c is independent (it doesn't touch the same code), so it can go
    in before or after the other parts, or in parallel.

Thanks for applying!

Geert Uytterhoeven (7):
  ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node
  ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges" to gpio node
  ARM: shmobile: sh73a0 dtsi: Add missing "gpio-ranges" to gpio node
  pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms
  pinctrl: sh-pfc: Remove empty gpio_function_free()
  pinctrl: sh-pfc: Move sh_pfc_add_gpiochip() up
  pinctrl: sh-pfc: Confine legacy function GPIOs to SH

 arch/arm/boot/dts/r8a73a4.dtsi  |   7 +++
 arch/arm/boot/dts/r8a7740.dtsi  |   1 +
 arch/arm/boot/dts/sh73a0.dtsi   |   3 +
 drivers/pinctrl/sh-pfc/core.h   |   2 +
 drivers/pinctrl/sh-pfc/gpio.c   | 128 ++++++++++++++++++++++------------------
 drivers/pinctrl/sh-pfc/sh_pfc.h |   2 +
 6 files changed, 87 insertions(+), 56 deletions(-)

-- 
1.9.1

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

[PATCH 1/7] ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node

[Geert Uytterhoeven]

If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
doesn't work:

    requesting hog GPIO led1-high (chip r8a73a4_pfc, offset 28) failed

The actual error code is -517 == -EPROBE_DEFER.

The problem is that PFC+GPIO registration is handled in multiple steps:
  1. pinctrl_register(),
  2. gpiochip_add(),
  3. gpiochip_add_pin_range().

Configuration of the hogs is handled in gpiochip_add():

    gpiochip_add
        of_gpiochip_add
            of_gpiochip_scan_hogs
                gpiod_hog
                    gpiochip_request_own_desc
                        __gpiod_request
                            chip->request
                                pinctrl_request_gpio
                                    pinctrl_get_device_gpio_range

However, at this point the GPIO controller hasn't been added to
pinctrldev_list yet, so the range can't be found, and the operation fails
with -EPROBE_DEFER.

To fix this, add a "gpio-ranges" property to the gpio device node, so
the ranges are added by of_gpiochip_add_pin_range(), which is called by
of_gpiochip_add() before the call to of_gpiochip_scan_hogs().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r8a73a4.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi
index 5090d1a8f652e8be..cb4f7b2798fe23be 100644
--- a/arch/arm/boot/dts/r8a73a4.dtsi
+++ b/arch/arm/boot/dts/r8a73a4.dtsi
@@ -207,6 +207,13 @@
 		reg = <0 0xe6050000 0 0x9000>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges =
+			<&pfc 0 0 31>, <&pfc 32 32 9>,
+			<&pfc 64 64 22>, <&pfc 96 96 31>,
+			<&pfc 128 128 7>, <&pfc 160 160 19>,
+			<&pfc 192 192 31>, <&pfc 224 224 27>,
+			<&pfc 256 256 28>, <&pfc 288 288 21>,
+			<&pfc 320 320 10>;
 		interrupts-extended =
 			<&irqc0  0 0>, <&irqc0  1 0>, <&irqc0  2 0>, <&irqc0  3 0>,
 			<&irqc0  4 0>, <&irqc0  5 0>, <&irqc0  6 0>, <&irqc0  7 0>,
-- 
1.9.1

[PATCH 2/7] ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges" to gpio node

[Geert Uytterhoeven]

If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
doesn't work:

    requesting hog GPIO lcd0 (chip r8a7740_pfc, offset 176) failed

The actual error code is -517 == -EPROBE_DEFER.

The problem is that PFC+GPIO registration is handled in multiple steps:
  1. pinctrl_register(),
  2. gpiochip_add(),
  3. gpiochip_add_pin_range().

Configuration of the hogs is handled in gpiochip_add():

    gpiochip_add
        of_gpiochip_add
            of_gpiochip_scan_hogs
                gpiod_hog
                    gpiochip_request_own_desc
                        __gpiod_request
                            chip->request
                                pinctrl_request_gpio
                                    pinctrl_get_device_gpio_range

However, at this point the GPIO controller hasn't been added to
pinctrldev_list yet, so the range can't be found, and the operation fails
with -EPROBE_DEFER.

To fix this, add a "gpio-ranges" property to the gpio device node, so
the range is added by of_gpiochip_add_pin_range(), which is called by
of_gpiochip_add() before the call to of_gpiochip_scan_hogs().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r8a7740.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
index 3aaab195132bfc2c..15977b5834de9579 100644
--- a/arch/arm/boot/dts/r8a7740.dtsi
+++ b/arch/arm/boot/dts/r8a7740.dtsi
@@ -316,6 +316,7 @@
 		      <0xe605800c 0x20>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges = <&pfc 0 0 212>;
 		interrupts-extended =
 			<&irqpin0 0 0>, <&irqpin0 1 0>, <&irqpin0 2 0>, <&irqpin0 3 0>,
 			<&irqpin0 4 0>, <&irqpin0 5 0>, <&irqpin0 6 0>, <&irqpin0 7 0>,
-- 
1.9.1

[PATCH 3/7] ARM: shmobile: sh73a0 dtsi: Add missing "gpio-ranges" to gpio node

[Geert Uytterhoeven]

If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
doesn't work:

    requesting hog GPIO led1-high (chip sh73a0_pfc, offset 20) failed

The actual error code is -517 == -EPROBE_DEFER.

The problem is that PFC+GPIO registration is handled in multiple steps:
  1. pinctrl_register(),
  2. gpiochip_add(),
  3. gpiochip_add_pin_range().

Configuration of the hogs is handled in gpiochip_add():

    gpiochip_add
        of_gpiochip_add
            of_gpiochip_scan_hogs
                gpiod_hog
                    gpiochip_request_own_desc
                        __gpiod_request
                            chip->request
                                pinctrl_request_gpio
                                    pinctrl_get_device_gpio_range

However, at this point the GPIO controller hasn't been added to
pinctrldev_list yet, so the range can't be found, and the operation fails
with -EPROBE_DEFER.

To fix this, add a "gpio-ranges" property to the gpio device node, so
the ranges are added by of_gpiochip_add_pin_range(), which is called by
of_gpiochip_add() before the call to of_gpiochip_scan_hogs().

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/sh73a0.dtsi | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/boot/dts/sh73a0.dtsi b/arch/arm/boot/dts/sh73a0.dtsi
index 11e17c5f26e2cae2..ff7c8f298f30a58d 100644
--- a/arch/arm/boot/dts/sh73a0.dtsi
+++ b/arch/arm/boot/dts/sh73a0.dtsi
@@ -392,6 +392,9 @@
 		      <0xe605801c 0x1c>;
 		gpio-controller;
 		#gpio-cells = <2>;
+		gpio-ranges =
+			<&pfc 0 0 119>, <&pfc 128 128 37>, <&pfc 192 192 91>,
+			<&pfc 288 288 22>;
 		interrupts-extended =
 			<&irqpin0 0 0>, <&irqpin0 1 0>, <&irqpin0 2 0>, <&irqpin0 3 0>,
 			<&irqpin0 4 0>, <&irqpin0 5 0>, <&irqpin0 6 0>, <&irqpin0 7 0>,
-- 
1.9.1

[PATCH 4/7] pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms

[Geert Uytterhoeven]

On platforms where the PFC/GPIO controller is instantiated from DT, the
mapping between GPIOs and pins is set up using the "gpio-ranges"
property in DT.

Hence stop setting up the mapping from C code on DT platforms.
This code is still used for SH or ARM-legacy platforms.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index ba353735ecf2be9a..1be118e4865fd3f8 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
 
 	pfc->gpio = chip;
 
-	/* Register the GPIO to pin mappings. As pins with GPIO ports must come
-	 * first in the ranges, skip the pins without GPIO ports by stopping at
-	 * the first range that contains such a pin.
-	 */
-	for (i = 0; i < pfc->nr_ranges; ++i) {
-		const struct sh_pfc_pin_range *range = &pfc->ranges[i];
-
-		if (range->start >= pfc->nr_gpio_pins)
-			break;
-
-		ret = gpiochip_add_pin_range(&chip->gpio_chip,
-					     dev_name(pfc->dev),
-					     range->start, range->start,
-					     range->end - range->start + 1);
-		if (ret < 0)
-			return ret;
+	if (IS_ENABLED(CONFIG_SUPERH) ||
+	    IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {
+		/*
+		 * Register the GPIO to pin mappings. As pins with GPIO ports
+		 * must come first in the ranges, skip the pins without GPIO
+		 * ports by stopping at the first range that contains such a
+		 * pin.
+		 */
+		for (i = 0; i < pfc->nr_ranges; ++i) {
+			const struct sh_pfc_pin_range *range = &pfc->ranges[i];
+
+			if (range->start >= pfc->nr_gpio_pins)
+				break;
+
+			ret = gpiochip_add_pin_range(&chip->gpio_chip,
+				dev_name(pfc->dev), range->start, range->start,
+				range->end - range->start + 1);
+			if (ret < 0)
+				return ret;
+		}
 	}
 
 	/* Register the function GPIOs chip. */
-- 
1.9.1

[PATCH 5/7] pinctrl: sh-pfc: Remove empty gpio_function_free()

[Geert Uytterhoeven]

gpio_chip.free() is optional, and can just be left unimplemented.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Untested due to lack of hardware (used on sh only).
---
 drivers/pinctrl/sh-pfc/gpio.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index 1be118e4865fd3f8..b8929b5e2fa26c53 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -286,17 +286,12 @@ static int gpio_function_request(struct gpio_chip *gc, unsigned offset)
 	return ret;
 }
 
-static void gpio_function_free(struct gpio_chip *gc, unsigned offset)
-{
-}
-
 static int gpio_function_setup(struct sh_pfc_chip *chip)
 {
 	struct sh_pfc *pfc = chip->pfc;
 	struct gpio_chip *gc = &chip->gpio_chip;
 
 	gc->request = gpio_function_request;
-	gc->free = gpio_function_free;
 
 	gc->label = pfc->info->name;
 	gc->owner = THIS_MODULE;
-- 
1.9.1

[PATCH 6/7] pinctrl: sh-pfc: Move sh_pfc_add_gpiochip() up

[Geert Uytterhoeven]

Move sh_pfc_add_gpiochip() to the generic helper section at the top of
the file, so it can be called from the "Function GPIOs" section later.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 drivers/pinctrl/sh-pfc/gpio.c | 58 +++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 29 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index b8929b5e2fa26c53..30654e800580524d 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -131,6 +131,35 @@ static int gpio_setup_data_regs(struct sh_pfc_chip *chip)
 	return 0;
 }
 
+static struct sh_pfc_chip *
+sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *),
+		    struct sh_pfc_window *mem)
+{
+	struct sh_pfc_chip *chip;
+	int ret;
+
+	chip = devm_kzalloc(pfc->dev, sizeof(*chip), GFP_KERNEL);
+	if (unlikely(!chip))
+		return ERR_PTR(-ENOMEM);
+
+	chip->mem = mem;
+	chip->pfc = pfc;
+
+	ret = setup(chip);
+	if (ret < 0)
+		return ERR_PTR(ret);
+
+	ret = gpiochip_add(&chip->gpio_chip);
+	if (unlikely(ret < 0))
+		return ERR_PTR(ret);
+
+	dev_info(pfc->dev, "%s handling gpio %u -> %u\n",
+		 chip->gpio_chip.label, chip->gpio_chip.base,
+		 chip->gpio_chip.base + chip->gpio_chip.ngpio - 1);
+
+	return chip;
+}
+
 /* -----------------------------------------------------------------------------
  * Pin GPIOs
  */
@@ -305,35 +334,6 @@ static int gpio_function_setup(struct sh_pfc_chip *chip)
  * Register/unregister
  */
 
-static struct sh_pfc_chip *
-sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *),
-		    struct sh_pfc_window *mem)
-{
-	struct sh_pfc_chip *chip;
-	int ret;
-
-	chip = devm_kzalloc(pfc->dev, sizeof(*chip), GFP_KERNEL);
-	if (unlikely(!chip))
-		return ERR_PTR(-ENOMEM);
-
-	chip->mem = mem;
-	chip->pfc = pfc;
-
-	ret = setup(chip);
-	if (ret < 0)
-		return ERR_PTR(ret);
-
-	ret = gpiochip_add(&chip->gpio_chip);
-	if (unlikely(ret < 0))
-		return ERR_PTR(ret);
-
-	dev_info(pfc->dev, "%s handling gpio %u -> %u\n",
-		 chip->gpio_chip.label, chip->gpio_chip.base,
-		 chip->gpio_chip.base + chip->gpio_chip.ngpio - 1);
-
-	return chip;
-}
-
 int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
 {
 	struct sh_pfc_chip *chip;
-- 
1.9.1

[PATCH 7/7] pinctrl: sh-pfc: Confine legacy function GPIOs to SH

[Geert Uytterhoeven]

Legacy function GPIOs are no longer used on ARM since commit
a27c5cd1a08cc95c ("sh-pfc: sh73a0: Remove function GPIOs").
Extract its setup code into a separate function, and make all function
GPIO related code and data depend on CONFIG_SUPERH.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
Compile-tested on sh using se7724_defconfig.
---
 drivers/pinctrl/sh-pfc/core.h   |  2 ++
 drivers/pinctrl/sh-pfc/gpio.c   | 39 ++++++++++++++++++++++++++++-----------
 drivers/pinctrl/sh-pfc/sh_pfc.h |  2 ++
 3 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
index 4c3c37bf7161804d..c38ace46d7111b0d 100644
--- a/drivers/pinctrl/sh-pfc/core.h
+++ b/drivers/pinctrl/sh-pfc/core.h
@@ -46,7 +46,9 @@ struct sh_pfc {
 	unsigned int nr_gpio_pins;
 
 	struct sh_pfc_chip *gpio;
+#ifdef CONFIG_SUPERH
 	struct sh_pfc_chip *func;
+#endif
 
 	struct sh_pfc_pinctrl *pinctrl;
 };
diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
index 30654e800580524d..15f4eea186e2ba56 100644
--- a/drivers/pinctrl/sh-pfc/gpio.c
+++ b/drivers/pinctrl/sh-pfc/gpio.c
@@ -290,6 +290,7 @@ static int gpio_pin_setup(struct sh_pfc_chip *chip)
  * Function GPIOs
  */
 
+#ifdef CONFIG_SUPERH
 static int gpio_function_request(struct gpio_chip *gc, unsigned offset)
 {
 	static bool __print_once;
@@ -330,6 +331,31 @@ static int gpio_function_setup(struct sh_pfc_chip *chip)
 	return 0;
 }
 
+static int sh_pfc_add_function_gpios(struct sh_pfc *pfc)
+{
+	struct sh_pfc_chip *chip;
+
+	if (!pfc->info->nr_func_gpios)
+		return 0;
+
+	chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	pfc->func = chip;
+
+	return 0;
+}
+
+static void sh_pfc_remove_function_gpios(struct sh_pfc *pfc)
+{
+	gpiochip_remove(&pfc->func->gpio_chip);
+}
+#else
+static inline int sh_pfc_add_function_gpios(struct sh_pfc *pfc) { return 0; }
+static inline int sh_pfc_remove_function_gpios(struct sh_pfc *pfc) { return 0; }
+#endif
+
 /* -----------------------------------------------------------------------------
  * Register/unregister
  */
@@ -397,22 +423,13 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
 	}
 
 	/* Register the function GPIOs chip. */
-	if (pfc->info->nr_func_gpios == 0)
-		return 0;
-
-	chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL);
-	if (IS_ERR(chip))
-		return PTR_ERR(chip);
-
-	pfc->func = chip;
-
-	return 0;
+	return sh_pfc_add_function_gpios(pfc);
 }
 
 int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc)
 {
 	gpiochip_remove(&pfc->gpio->gpio_chip);
-	gpiochip_remove(&pfc->func->gpio_chip);
+	sh_pfc_remove_function_gpios(pfc);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h b/drivers/pinctrl/sh-pfc/sh_pfc.h
index c7508d5f688613b2..c6f9163b3e23041f 100644
--- a/drivers/pinctrl/sh-pfc/sh_pfc.h
+++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
@@ -138,8 +138,10 @@ struct sh_pfc_soc_info {
 	const struct sh_pfc_function *functions;
 	unsigned int nr_functions;
 
+#ifdef CONFIG_SUPERH
 	const struct pinmux_func *func_gpios;
 	unsigned int nr_func_gpios;
+#endif
 
 	const struct pinmux_cfg_reg *cfg_regs;
 	const struct pinmux_data_reg *data_regs;
-- 
1.9.1

Re: [PATCH 4/7] pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:24 Geert Uytterhoeven wrote:
> On platforms where the PFC/GPIO controller is instantiated from DT, the
> mapping between GPIOs and pins is set up using the "gpio-ranges"
> property in DT.
> 
> Hence stop setting up the mapping from C code on DT platforms.
> This code is still used for SH or ARM-legacy platforms.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
>  drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
>  1 file changed, 20 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index ba353735ecf2be9a..1be118e4865fd3f8 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
> 
>  	pfc->gpio = chip;
> 
> -	/* Register the GPIO to pin mappings. As pins with GPIO ports must come
> -	 * first in the ranges, skip the pins without GPIO ports by stopping at
> -	 * the first range that contains such a pin.
> -	 */
> -	for (i = 0; i < pfc->nr_ranges; ++i) {
> -		const struct sh_pfc_pin_range *range = &pfc->ranges[i];
> -
> -		if (range->start >= pfc->nr_gpio_pins)
> -			break;
> -
> -		ret = gpiochip_add_pin_range(&chip->gpio_chip,
> -					     dev_name(pfc->dev),
> -					     range->start, range->start,
> -					     range->end - range->start + 1);
> -		if (ret < 0)
> -			return ret;
> +	if (IS_ENABLED(CONFIG_SUPERH) ||
> +	    IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {

I'd prefer checking IS_ENABLED(CONFIG_OF) && pfc->dev->of_node as that doesn't 
explicitly depend on the platform type.

As the code after this if block also don't need to run on non-DT platforms, 
how about just using

	if (IS_ENABLED(CONFIG_OF) && pfc->dev->of_node)
		return 0;

?

> +		/*
> +		 * Register the GPIO to pin mappings. As pins with GPIO ports
> +		 * must come first in the ranges, skip the pins without GPIO
> +		 * ports by stopping at the first range that contains such a
> +		 * pin.
> +		 */
> +		for (i = 0; i < pfc->nr_ranges; ++i) {
> +			const struct sh_pfc_pin_range *range = &pfc->ranges[i];
> +
> +			if (range->start >= pfc->nr_gpio_pins)
> +				break;
> +
> +			ret = gpiochip_add_pin_range(&chip->gpio_chip,
> +				dev_name(pfc->dev), range->start, range->start,
> +				range->end - range->start + 1);
> +			if (ret < 0)
> +				return ret;
> +		}
>  	}
> 
>  	/* Register the function GPIOs chip. */

-- 
Regards,

Laurent Pinchart

Re: [PATCH 5/7] pinctrl: sh-pfc: Remove empty gpio_function_free()

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:25 Geert Uytterhoeven wrote:
> gpio_chip.free() is optional, and can just be left unimplemented.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
> Untested due to lack of hardware (used on sh only).
> ---
>  drivers/pinctrl/sh-pfc/gpio.c | 5 -----
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index 1be118e4865fd3f8..b8929b5e2fa26c53 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -286,17 +286,12 @@ static int gpio_function_request(struct gpio_chip *gc,
> unsigned offset) return ret;
>  }
> 
> -static void gpio_function_free(struct gpio_chip *gc, unsigned offset)
> -{
> -}
> -
>  static int gpio_function_setup(struct sh_pfc_chip *chip)
>  {
>  	struct sh_pfc *pfc = chip->pfc;
>  	struct gpio_chip *gc = &chip->gpio_chip;
> 
>  	gc->request = gpio_function_request;
> -	gc->free = gpio_function_free;
> 
>  	gc->label = pfc->info->name;
>  	gc->owner = THIS_MODULE;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 6/7] pinctrl: sh-pfc: Move sh_pfc_add_gpiochip() up

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:26 Geert Uytterhoeven wrote:
> Move sh_pfc_add_gpiochip() to the generic helper section at the top of
> the file, so it can be called from the "Function GPIOs" section later.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/pinctrl/sh-pfc/gpio.c | 58  +++++++++++++++++++--------------------
>  1 file changed, 29 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index b8929b5e2fa26c53..30654e800580524d 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -131,6 +131,35 @@ static int gpio_setup_data_regs(struct sh_pfc_chip
> *chip) return 0;
>  }
> 
> +static struct sh_pfc_chip *
> +sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *),
> +		    struct sh_pfc_window *mem)
> +{
> +	struct sh_pfc_chip *chip;
> +	int ret;
> +
> +	chip = devm_kzalloc(pfc->dev, sizeof(*chip), GFP_KERNEL);
> +	if (unlikely(!chip))
> +		return ERR_PTR(-ENOMEM);
> +
> +	chip->mem = mem;
> +	chip->pfc = pfc;
> +
> +	ret = setup(chip);
> +	if (ret < 0)
> +		return ERR_PTR(ret);
> +
> +	ret = gpiochip_add(&chip->gpio_chip);
> +	if (unlikely(ret < 0))
> +		return ERR_PTR(ret);
> +
> +	dev_info(pfc->dev, "%s handling gpio %u -> %u\n",
> +		 chip->gpio_chip.label, chip->gpio_chip.base,
> +		 chip->gpio_chip.base + chip->gpio_chip.ngpio - 1);
> +
> +	return chip;
> +}
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Pin GPIOs
>   */
> @@ -305,35 +334,6 @@ static int gpio_function_setup(struct sh_pfc_chip
> *chip) * Register/unregister
>   */
> 
> -static struct sh_pfc_chip *
> -sh_pfc_add_gpiochip(struct sh_pfc *pfc, int(*setup)(struct sh_pfc_chip *),
> -		    struct sh_pfc_window *mem)
> -{
> -	struct sh_pfc_chip *chip;
> -	int ret;
> -
> -	chip = devm_kzalloc(pfc->dev, sizeof(*chip), GFP_KERNEL);
> -	if (unlikely(!chip))
> -		return ERR_PTR(-ENOMEM);
> -
> -	chip->mem = mem;
> -	chip->pfc = pfc;
> -
> -	ret = setup(chip);
> -	if (ret < 0)
> -		return ERR_PTR(ret);
> -
> -	ret = gpiochip_add(&chip->gpio_chip);
> -	if (unlikely(ret < 0))
> -		return ERR_PTR(ret);
> -
> -	dev_info(pfc->dev, "%s handling gpio %u -> %u\n",
> -		 chip->gpio_chip.label, chip->gpio_chip.base,
> -		 chip->gpio_chip.base + chip->gpio_chip.ngpio - 1);
> -
> -	return chip;
> -}
> -
>  int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
>  {
>  	struct sh_pfc_chip *chip;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 7/7] pinctrl: sh-pfc: Confine legacy function GPIOs to SH

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:27 Geert Uytterhoeven wrote:
> Legacy function GPIOs are no longer used on ARM since commit
> a27c5cd1a08cc95c ("sh-pfc: sh73a0: Remove function GPIOs").
> Extract its setup code into a separate function, and make all function
> GPIO related code and data depend on CONFIG_SUPERH.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> ---
> Compile-tested on sh using se7724_defconfig.
> ---
>  drivers/pinctrl/sh-pfc/core.h   |  2 ++
>  drivers/pinctrl/sh-pfc/gpio.c   | 39 ++++++++++++++++++++++++++----------
>  drivers/pinctrl/sh-pfc/sh_pfc.h |  2 ++
>  3 files changed, 32 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/pinctrl/sh-pfc/core.h b/drivers/pinctrl/sh-pfc/core.h
> index 4c3c37bf7161804d..c38ace46d7111b0d 100644
> --- a/drivers/pinctrl/sh-pfc/core.h
> +++ b/drivers/pinctrl/sh-pfc/core.h
> @@ -46,7 +46,9 @@ struct sh_pfc {
>  	unsigned int nr_gpio_pins;
> 
>  	struct sh_pfc_chip *gpio;
> +#ifdef CONFIG_SUPERH
>  	struct sh_pfc_chip *func;
> +#endif
> 
>  	struct sh_pfc_pinctrl *pinctrl;
>  };
> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
> index 30654e800580524d..15f4eea186e2ba56 100644
> --- a/drivers/pinctrl/sh-pfc/gpio.c
> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> @@ -290,6 +290,7 @@ static int gpio_pin_setup(struct sh_pfc_chip *chip)
>   * Function GPIOs
>   */
> 
> +#ifdef CONFIG_SUPERH
>  static int gpio_function_request(struct gpio_chip *gc, unsigned offset)
>  {
>  	static bool __print_once;
> @@ -330,6 +331,31 @@ static int gpio_function_setup(struct sh_pfc_chip
> *chip) return 0;
>  }
> 
> +static int sh_pfc_add_function_gpios(struct sh_pfc *pfc)
> +{
> +	struct sh_pfc_chip *chip;
> +
> +	if (!pfc->info->nr_func_gpios)
> +		return 0;
> +
> +	chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	pfc->func = chip;
> +
> +	return 0;
> +}
> +
> +static void sh_pfc_remove_function_gpios(struct sh_pfc *pfc)
> +{
> +	gpiochip_remove(&pfc->func->gpio_chip);
> +}
> +#else
> +static inline int sh_pfc_add_function_gpios(struct sh_pfc *pfc) { return 0;
> }
> +static inline int sh_pfc_remove_function_gpios(struct sh_pfc *pfc) {
> return 0; }

Given that those functions are only called from a single location, would it 
make sense to just compile the calls conditionally instead ? I would find that 
slightly more readable as it would be more explicit.

> +#endif
> +
>  /*
> ---------------------------------------------------------------------------
> -- * Register/unregister
>   */
> @@ -397,22 +423,13 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
>  	}
> 
>  	/* Register the function GPIOs chip. */
> -	if (pfc->info->nr_func_gpios == 0)
> -		return 0;
> -
> -	chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL);
> -	if (IS_ERR(chip))
> -		return PTR_ERR(chip);
> -
> -	pfc->func = chip;
> -
> -	return 0;
> +	return sh_pfc_add_function_gpios(pfc);
>  }
> 
>  int sh_pfc_unregister_gpiochip(struct sh_pfc *pfc)
>  {
>  	gpiochip_remove(&pfc->gpio->gpio_chip);
> -	gpiochip_remove(&pfc->func->gpio_chip);
> +	sh_pfc_remove_function_gpios(pfc);
> 
>  	return 0;
>  }
> diff --git a/drivers/pinctrl/sh-pfc/sh_pfc.h
> b/drivers/pinctrl/sh-pfc/sh_pfc.h index c7508d5f688613b2..c6f9163b3e23041f
> 100644
> --- a/drivers/pinctrl/sh-pfc/sh_pfc.h
> +++ b/drivers/pinctrl/sh-pfc/sh_pfc.h
> @@ -138,8 +138,10 @@ struct sh_pfc_soc_info {
>  	const struct sh_pfc_function *functions;
>  	unsigned int nr_functions;
> 
> +#ifdef CONFIG_SUPERH
>  	const struct pinmux_func *func_gpios;
>  	unsigned int nr_func_gpios;
> +#endif
> 
>  	const struct pinmux_cfg_reg *cfg_regs;
>  	const struct pinmux_data_reg *data_regs;

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/7] ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges" to gpio node

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:22 Geert Uytterhoeven wrote:
> If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
> case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
> doesn't work:
> 
>     requesting hog GPIO lcd0 (chip r8a7740_pfc, offset 176) failed
> 
> The actual error code is -517 == -EPROBE_DEFER.
> 
> The problem is that PFC+GPIO registration is handled in multiple steps:
>   1. pinctrl_register(),
>   2. gpiochip_add(),
>   3. gpiochip_add_pin_range().
> 
> Configuration of the hogs is handled in gpiochip_add():
> 
>     gpiochip_add
>         of_gpiochip_add
>             of_gpiochip_scan_hogs
>                 gpiod_hog
>                     gpiochip_request_own_desc
>                         __gpiod_request
>                             chip->request
>                                 pinctrl_request_gpio
>                                     pinctrl_get_device_gpio_range
> 
> However, at this point the GPIO controller hasn't been added to
> pinctrldev_list yet, so the range can't be found, and the operation fails
> with -EPROBE_DEFER.
> 
> To fix this, add a "gpio-ranges" property to the gpio device node, so
> the range is added by of_gpiochip_add_pin_range(), which is called by
> of_gpiochip_add() before the call to of_gpiochip_scan_hogs().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This looks sane to me, even though referencing the same DT node seems a bit 
dodgy. I'll let Linus comment on that, but for the implementation itself,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm/boot/dts/r8a7740.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/boot/dts/r8a7740.dtsi b/arch/arm/boot/dts/r8a7740.dtsi
> index 3aaab195132bfc2c..15977b5834de9579 100644
> --- a/arch/arm/boot/dts/r8a7740.dtsi
> +++ b/arch/arm/boot/dts/r8a7740.dtsi
> @@ -316,6 +316,7 @@
>  		      <0xe605800c 0x20>;
>  		gpio-controller;
>  		#gpio-cells = <2>;
> +		gpio-ranges = <&pfc 0 0 212>;
>  		interrupts-extended =
>  			<&irqpin0 0 0>, <&irqpin0 1 0>, <&irqpin0 2 0>, <&irqpin0 3 0>,
>  			<&irqpin0 4 0>, <&irqpin0 5 0>, <&irqpin0 6 0>, <&irqpin0 7 0>,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jun 30, 2015 at 11:30 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 30 June 2015 09:45:24 Geert Uytterhoeven wrote:
>> On platforms where the PFC/GPIO controller is instantiated from DT, the
>> mapping between GPIOs and pins is set up using the "gpio-ranges"
>> property in DT.
>>
>> Hence stop setting up the mapping from C code on DT platforms.
>> This code is still used for SH or ARM-legacy platforms.
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>> ---
>>  drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
>>  1 file changed, 20 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/pinctrl/sh-pfc/gpio.c b/drivers/pinctrl/sh-pfc/gpio.c
>> index ba353735ecf2be9a..1be118e4865fd3f8 100644
>> --- a/drivers/pinctrl/sh-pfc/gpio.c
>> +++ b/drivers/pinctrl/sh-pfc/gpio.c
>> @@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
>>
>>       pfc->gpio = chip;
>>
>> -     /* Register the GPIO to pin mappings. As pins with GPIO ports must come
>> -      * first in the ranges, skip the pins without GPIO ports by stopping at
>> -      * the first range that contains such a pin.
>> -      */
>> -     for (i = 0; i < pfc->nr_ranges; ++i) {
>> -             const struct sh_pfc_pin_range *range = &pfc->ranges[i];
>> -
>> -             if (range->start >= pfc->nr_gpio_pins)
>> -                     break;
>> -
>> -             ret = gpiochip_add_pin_range(&chip->gpio_chip,
>> -                                          dev_name(pfc->dev),
>> -                                          range->start, range->start,
>> -                                          range->end - range->start + 1);
>> -             if (ret < 0)
>> -                     return ret;
>> +     if (IS_ENABLED(CONFIG_SUPERH) ||
>> +         IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {
>
> I'd prefer checking IS_ENABLED(CONFIG_OF) && pfc->dev->of_node as that doesn't
> explicitly depend on the platform type.

Note that CONFIG_OF is also set for ARM-legacy (and soon for SH). But that's
a minor problem. But due to the runtime check on pfc->dev->of_node, the
unused code below won't be left out by the compiler, and I want to get rid
of that code.

The platform check make it clear when the code can be removed.

> As the code after this if block also don't need to run on non-DT platforms,
> how about just using
>
>         if (IS_ENABLED(CONFIG_OF) && pfc->dev->of_node)
>                 return 0;
>
> ?

Early return is indeed an option, as we don't need the function GPIOs on
DT platforms.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 7/7] pinctrl: sh-pfc: Confine legacy function GPIOs to SH

[Geert Uytterhoeven]

Hi Laurent,

On Tue, Jun 30, 2015 at 11:38 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> --- a/drivers/pinctrl/sh-pfc/gpio.c
>> +++ b/drivers/pinctrl/sh-pfc/gpio.c
>> @@ -290,6 +290,7 @@ static int gpio_pin_setup(struct sh_pfc_chip *chip)
>>   * Function GPIOs
>>   */
>>
>> +#ifdef CONFIG_SUPERH
>>  static int gpio_function_request(struct gpio_chip *gc, unsigned offset)
>>  {
>>       static bool __print_once;
>> @@ -330,6 +331,31 @@ static int gpio_function_setup(struct sh_pfc_chip
>> *chip) return 0;
>>  }
>>
>> +static int sh_pfc_add_function_gpios(struct sh_pfc *pfc)
>> +{
>> +     struct sh_pfc_chip *chip;
>> +
>> +     if (!pfc->info->nr_func_gpios)
>> +             return 0;
>> +
>> +     chip = sh_pfc_add_gpiochip(pfc, gpio_function_setup, NULL);
>> +     if (IS_ERR(chip))
>> +             return PTR_ERR(chip);
>> +
>> +     pfc->func = chip;
>> +
>> +     return 0;
>> +}
>> +
>> +static void sh_pfc_remove_function_gpios(struct sh_pfc *pfc)
>> +{
>> +     gpiochip_remove(&pfc->func->gpio_chip);
>> +}
>> +#else
>> +static inline int sh_pfc_add_function_gpios(struct sh_pfc *pfc) { return 0;
>> }
>> +static inline int sh_pfc_remove_function_gpios(struct sh_pfc *pfc) {
>> return 0; }
>
> Given that those functions are only called from a single location, would it
> make sense to just compile the calls conditionally instead ? I would find that
> slightly more readable as it would be more explicit.

If that's acceptable for you, that would be fine for me, too. One (or two) more
visual cues for future removal of legacy code.

And then I can drop "[PATCH 6/7] pinctrl: sh-pfc: Move sh_pfc_add_gpiochip()
up".

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 3/7] ARM: shmobile: sh73a0 dtsi: Add missing "gpio-ranges" to gpio node

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:23 Geert Uytterhoeven wrote:
> If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
> case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
> doesn't work:
> 
>     requesting hog GPIO led1-high (chip sh73a0_pfc, offset 20) failed
> 
> The actual error code is -517 == -EPROBE_DEFER.
> 
> The problem is that PFC+GPIO registration is handled in multiple steps:
>   1. pinctrl_register(),
>   2. gpiochip_add(),
>   3. gpiochip_add_pin_range().
> 
> Configuration of the hogs is handled in gpiochip_add():
> 
>     gpiochip_add
>         of_gpiochip_add
>             of_gpiochip_scan_hogs
>                 gpiod_hog
>                     gpiochip_request_own_desc
>                         __gpiod_request
>                             chip->request
>                                 pinctrl_request_gpio
>                                     pinctrl_get_device_gpio_range
> 
> However, at this point the GPIO controller hasn't been added to
> pinctrldev_list yet, so the range can't be found, and the operation fails
> with -EPROBE_DEFER.
> 
> To fix this, add a "gpio-ranges" property to the gpio device node, so
> the ranges are added by of_gpiochip_add_pin_range(), which is called by
> of_gpiochip_add() before the call to of_gpiochip_scan_hogs().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This looks sane to me, even though referencing the same DT node seems a bit 
dodgy. I'll let Linus comment on that, but for the implementation itself,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm/boot/dts/sh73a0.dtsi | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sh73a0.dtsi b/arch/arm/boot/dts/sh73a0.dtsi
> index 11e17c5f26e2cae2..ff7c8f298f30a58d 100644
> --- a/arch/arm/boot/dts/sh73a0.dtsi
> +++ b/arch/arm/boot/dts/sh73a0.dtsi
> @@ -392,6 +392,9 @@
>  		      <0xe605801c 0x1c>;
>  		gpio-controller;
>  		#gpio-cells = <2>;
> +		gpio-ranges =
> +			<&pfc 0 0 119>, <&pfc 128 128 37>, <&pfc 192 192 91>,
> +			<&pfc 288 288 22>;
>  		interrupts-extended =
>  			<&irqpin0 0 0>, <&irqpin0 1 0>, <&irqpin0 2 0>, <&irqpin0 3 0>,
>  			<&irqpin0 4 0>, <&irqpin0 5 0>, <&irqpin0 6 0>, <&irqpin0 7 0>,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/7] ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node

[Laurent Pinchart]

Hi Geert,

Thank you for the patch.

On Tuesday 30 June 2015 09:45:21 Geert Uytterhoeven wrote:
> If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
> case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
> doesn't work:
> 
>     requesting hog GPIO led1-high (chip r8a73a4_pfc, offset 28) failed
> 
> The actual error code is -517 == -EPROBE_DEFER.
> 
> The problem is that PFC+GPIO registration is handled in multiple steps:
>   1. pinctrl_register(),
>   2. gpiochip_add(),
>   3. gpiochip_add_pin_range().
> 
> Configuration of the hogs is handled in gpiochip_add():
> 
>     gpiochip_add
>         of_gpiochip_add
>             of_gpiochip_scan_hogs
>                 gpiod_hog
>                     gpiochip_request_own_desc
>                         __gpiod_request
>                             chip->request
>                                 pinctrl_request_gpio
>                                     pinctrl_get_device_gpio_range
> 
> However, at this point the GPIO controller hasn't been added to
> pinctrldev_list yet, so the range can't be found, and the operation fails
> with -EPROBE_DEFER.
> 
> To fix this, add a "gpio-ranges" property to the gpio device node, so
> the ranges are added by of_gpiochip_add_pin_range(), which is called by
> of_gpiochip_add() before the call to of_gpiochip_scan_hogs().
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

This looks sane to me, even though referencing the same DT node seems a bit 
dodgy. I'll let Linus comment on that, but for the implementation itself,

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  arch/arm/boot/dts/r8a73a4.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi
> index 5090d1a8f652e8be..cb4f7b2798fe23be 100644
> --- a/arch/arm/boot/dts/r8a73a4.dtsi
> +++ b/arch/arm/boot/dts/r8a73a4.dtsi
> @@ -207,6 +207,13 @@
>  		reg = <0 0xe6050000 0 0x9000>;
>  		gpio-controller;
>  		#gpio-cells = <2>;
> +		gpio-ranges =
> +			<&pfc 0 0 31>, <&pfc 32 32 9>,
> +			<&pfc 64 64 22>, <&pfc 96 96 31>,
> +			<&pfc 128 128 7>, <&pfc 160 160 19>,
> +			<&pfc 192 192 31>, <&pfc 224 224 27>,
> +			<&pfc 256 256 28>, <&pfc 288 288 21>,
> +			<&pfc 320 320 10>;
>  		interrupts-extended =
>  			<&irqc0  0 0>, <&irqc0  1 0>, <&irqc0  2 0>, <&irqc0  3 0>,
>  			<&irqc0  4 0>, <&irqc0  5 0>, <&irqc0  6 0>, <&irqc0  7 0>,

-- 
Regards,

Laurent Pinchart

Re: [PATCH 4/7] pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms

[Laurent Pinchart]

Hi Geert,

On Tuesday 30 June 2015 11:44:24 Geert Uytterhoeven wrote:
> On Tue, Jun 30, 2015 at 11:30 AM, Laurent Pinchart wrote:
> > On Tuesday 30 June 2015 09:45:24 Geert Uytterhoeven wrote:
> >> On platforms where the PFC/GPIO controller is instantiated from DT, the
> >> mapping between GPIOs and pins is set up using the "gpio-ranges"
> >> property in DT.
> >> 
> >> Hence stop setting up the mapping from C code on DT platforms.
> >> This code is still used for SH or ARM-legacy platforms.
> >> 
> >> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >> ---
> >> 
> >>  drivers/pinctrl/sh-pfc/gpio.c | 36 ++++++++++++++++++++----------------
> >>  1 file changed, 20 insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/drivers/pinctrl/sh-pfc/gpio.c
> >> b/drivers/pinctrl/sh-pfc/gpio.c
> >> index ba353735ecf2be9a..1be118e4865fd3f8 100644
> >> --- a/drivers/pinctrl/sh-pfc/gpio.c
> >> +++ b/drivers/pinctrl/sh-pfc/gpio.c
> >> @@ -379,22 +379,26 @@ int sh_pfc_register_gpiochip(struct sh_pfc *pfc)
> >> 
> >>       pfc->gpio = chip;
> >> 
> >> -     /* Register the GPIO to pin mappings. As pins with GPIO ports must
> >> come
> >> -      * first in the ranges, skip the pins without GPIO ports by
> >> stopping at
> >> -      * the first range that contains such a pin.
> >> -      */
> >> -     for (i = 0; i < pfc->nr_ranges; ++i) {
> >> -             const struct sh_pfc_pin_range *range = &pfc->ranges[i];
> >> -
> >> -             if (range->start >= pfc->nr_gpio_pins)
> >> -                     break;
> >> -
> >> -             ret = gpiochip_add_pin_range(&chip->gpio_chip,
> >> -                                          dev_name(pfc->dev),
> >> -                                          range->start, range->start,
> >> -                                          range->end - range->start +
> >> 1);
> >> -             if (ret < 0)
> >> -                     return ret;
> >> +     if (IS_ENABLED(CONFIG_SUPERH) ||
> >> +         IS_ENABLED(CONFIG_ARCH_SHMOBILE_LEGACY)) {
> > 
> > I'd prefer checking IS_ENABLED(CONFIG_OF) && pfc->dev->of_node as that
> > doesn't explicitly depend on the platform type.
> 
> Note that CONFIG_OF is also set for ARM-legacy (and soon for SH). But that's
> a minor problem. But due to the runtime check on pfc->dev->of_node, the
> unused code below won't be left out by the compiler, and I want to get rid
> of that code.
> 
> The platform check make it clear when the code can be removed.

Not any time soon I'm afraid as arch/sh won't fully move to DT in the near 
future, but ARCH_SHMOBILE_LEGACY should go away soon, so I'm fine with 
compile-time optimization.

> > As the code after this if block also don't need to run on non-DT
> > platforms,
> > how about just using
> > 
> >         if (IS_ENABLED(CONFIG_OF) && pfc->dev->of_node)
> >         
> >                 return 0;
> > 
> > ?
> 
> Early return is indeed an option, as we don't need the function GPIOs on
> DT platforms.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/7] ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node

[Geert Uytterhoeven]

Hi Laurent, Linus,

On Tue, Jun 30, 2015 at 11:49 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 30 June 2015 09:45:21 Geert Uytterhoeven wrote:
>> If a GPIO driver uses gpiochip_add_pin_range() (which is usually the
>> case for GPIO/PFC combos), the GPIO hogging mechanism configured from DT
>> doesn't work:
>>
>>     requesting hog GPIO led1-high (chip r8a73a4_pfc, offset 28) failed
>>
>> The actual error code is -517 == -EPROBE_DEFER.
>>
>> The problem is that PFC+GPIO registration is handled in multiple steps:
>>   1. pinctrl_register(),
>>   2. gpiochip_add(),
>>   3. gpiochip_add_pin_range().
>>
>> Configuration of the hogs is handled in gpiochip_add():
>>
>>     gpiochip_add
>>         of_gpiochip_add
>>             of_gpiochip_scan_hogs
>>                 gpiod_hog
>>                     gpiochip_request_own_desc
>>                         __gpiod_request
>>                             chip->request
>>                                 pinctrl_request_gpio
>>                                     pinctrl_get_device_gpio_range
>>
>> However, at this point the GPIO controller hasn't been added to
>> pinctrldev_list yet, so the range can't be found, and the operation fails
>> with -EPROBE_DEFER.
>>
>> To fix this, add a "gpio-ranges" property to the gpio device node, so
>> the ranges are added by of_gpiochip_add_pin_range(), which is called by
>> of_gpiochip_add() before the call to of_gpiochip_scan_hogs().
>>
>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> This looks sane to me, even though referencing the same DT node seems a bit
> dodgy. I'll let Linus comment on that, but for the implementation itself,
>
> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Thank you!

Any wise words from Linus?

Thanks again!

>
>> ---
>>  arch/arm/boot/dts/r8a73a4.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/r8a73a4.dtsi b/arch/arm/boot/dts/r8a73a4.dtsi
>> index 5090d1a8f652e8be..cb4f7b2798fe23be 100644
>> --- a/arch/arm/boot/dts/r8a73a4.dtsi
>> +++ b/arch/arm/boot/dts/r8a73a4.dtsi
>> @@ -207,6 +207,13 @@
>>               reg = <0 0xe6050000 0 0x9000>;
>>               gpio-controller;
>>               #gpio-cells = <2>;
>> +             gpio-ranges =
>> +                     <&pfc 0 0 31>, <&pfc 32 32 9>,
>> +                     <&pfc 64 64 22>, <&pfc 96 96 31>,
>> +                     <&pfc 128 128 7>, <&pfc 160 160 19>,
>> +                     <&pfc 192 192 31>, <&pfc 224 224 27>,
>> +                     <&pfc 256 256 28>, <&pfc 288 288 21>,
>> +                     <&pfc 320 320 10>;
>>               interrupts-extended =
>>                       <&irqc0  0 0>, <&irqc0  1 0>, <&irqc0  2 0>, <&irqc0  3 0>,
>>                       <&irqc0  4 0>, <&irqc0  5 0>, <&irqc0  6 0>, <&irqc0  7 0>,

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 0/7] ARM: shmobile: Move gpio ranges from C code to DT

[Linus Walleij]

On Tue, Jun 30, 2015 at 9:45 AM, Geert Uytterhoeven
<geert+renesas@glider.be> wrote:

>         Hi Linus, Alex, Laurent, Simon, Magnus,
>
> This patch series moves the setup of the GPIO-PFC pin mapping for
> Renesas PFC/GPIO combos from C code to DT, and does some cleanups.
> The move to DT is needed to make the GPIO hogging mechanism work, cfr.
> the discussion following "[PATCH] [RFC] gpio: Retry deferred GPIO
> hogging on pin range change" (https://lkml.org/lkml/2015/6/16/455).
>
> The series consists of 3 parts:
>   a. Patches 1-3 add the missing "gpio-ranges" properties to the dtsi
>      files for all affected SoCs,
>   b. Patch 4 disables the C code to set up the mapping on DT platforms
>      (it's still needed on SH or ARM-legacy),
>   c. Patches 5-7 do a few more cleanups in the sh-pfc gpio code.
>
> Dependencies:
>   - This series applies against next-next-20150630.
>   - Part a must go in first, to avoid regressions.
>     In addition, it's a prerequisite for "ARM: shmobile: armadillo800eva
>     dts: Add pinctrl and gpio-hog for lcdc0", which has been queued up
>     already in Simon's dt-for-v4.3 branch. Perhaps it can still be
>     reordered?
>   - While I didn't notice any bad behavior by having part a only, part b
>     should go in immediately after part a. Hence I think it's best if
>     Simon can take this one, too.
>   - Part c is independent (it doesn't touch the same code), so it can go
>     in before or after the other parts, or in parallel.
>
> Thanks for applying!
>
> Geert Uytterhoeven (7):
>   ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node
>   ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges" to gpio node
>   ARM: shmobile: sh73a0 dtsi: Add missing "gpio-ranges" to gpio node
>   pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms
>   pinctrl: sh-pfc: Remove empty gpio_function_free()
>   pinctrl: sh-pfc: Move sh_pfc_add_gpiochip() up
>   pinctrl: sh-pfc: Confine legacy function GPIOs to SH

I'm happy of shmobile people are happy.

Acked-by: Linus Walleij <linus.walleij@linaro.org>

For all.

Yours,
Linus Walleij

Re: [PATCH 0/7] ARM: shmobile: Move gpio ranges from C code to DT

[Geert Uytterhoeven]

Hi Linus,

On Thu, Jul 16, 2015 at 11:00 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Tue, Jun 30, 2015 at 9:45 AM, Geert Uytterhoeven
> <geert+renesas@glider.be> wrote:
>> This patch series moves the setup of the GPIO-PFC pin mapping for
>> Renesas PFC/GPIO combos from C code to DT, and does some cleanups.
>> The move to DT is needed to make the GPIO hogging mechanism work, cfr.
>> the discussion following "[PATCH] [RFC] gpio: Retry deferred GPIO
>> hogging on pin range change" (https://lkml.org/lkml/2015/6/16/455).
>>
>> The series consists of 3 parts:
>>   a. Patches 1-3 add the missing "gpio-ranges" properties to the dtsi
>>      files for all affected SoCs,
>>   b. Patch 4 disables the C code to set up the mapping on DT platforms
>>      (it's still needed on SH or ARM-legacy),
>>   c. Patches 5-7 do a few more cleanups in the sh-pfc gpio code.
>>
>> Dependencies:
>>   - This series applies against next-next-20150630.
>>   - Part a must go in first, to avoid regressions.
>>     In addition, it's a prerequisite for "ARM: shmobile: armadillo800eva
>>     dts: Add pinctrl and gpio-hog for lcdc0", which has been queued up
>>     already in Simon's dt-for-v4.3 branch. Perhaps it can still be
>>     reordered?
>>   - While I didn't notice any bad behavior by having part a only, part b
>>     should go in immediately after part a. Hence I think it's best if
>>     Simon can take this one, too.
>>   - Part c is independent (it doesn't touch the same code), so it can go
>>     in before or after the other parts, or in parallel.
>>
>> Thanks for applying!
>>
>> Geert Uytterhoeven (7):
>>   ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node
>>   ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges" to gpio node
>>   ARM: shmobile: sh73a0 dtsi: Add missing "gpio-ranges" to gpio node
>>   pinctrl: sh-pfc: Stop calling gpiochip_add_pin_range() on DT platforms
>>   pinctrl: sh-pfc: Remove empty gpio_function_free()
>>   pinctrl: sh-pfc: Move sh_pfc_add_gpiochip() up
>>   pinctrl: sh-pfc: Confine legacy function GPIOs to SH
>
> I'm happy of shmobile people are happy.

Laurent was mostly worried about the phandle in the "gpio-ranges" property
pointing to the gpio/pfc combo node itself. Do you see any issues with that?

As there were comments on some of the patches, I have to resend this
series anyway...

> Acked-by: Linus Walleij <linus.walleij@linaro.org>

Thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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

Re: [PATCH 1/7] ARM: shmobile: r8a73a4 dtsi: Add missing "gpio-ranges" to gpio node

[Linus Walleij]

On Tue, Jul 14, 2015 at 2:05 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> On Tue, Jun 30, 2015 at 11:49 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:

>>> To fix this, add a "gpio-ranges" property to the gpio device node, so
>>> the ranges are added by of_gpiochip_add_pin_range(), which is called by
>>> of_gpiochip_add() before the call to of_gpiochip_scan_hogs().
>>>
>>> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
>>
>> This looks sane to me, even though referencing the same DT node seems a bit
>> dodgy. I'll let Linus comment on that, but for the implementation itself,
>>
>> Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Thank you!
>
> Any wise words from Linus?

I don't know if I'm wise but I'm OK with this:
Acked-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij