All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/3] Have Tegra's GPIO chip depend explicitly on the pinctrl device
@ 2015-07-01 12:45 ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-gpio, devicetree, linux-tegra, Tomeu Vizoso,
	Thierry Reding, Russell King, Linus Walleij, Kumar Gala,
	Stephen Warren, Grant Likely, Ian Campbell, Rob Herring,
	Pawel Moll, Mark Rutland, Alexandre Courbot, linux-arm-kernel

Hello,

these three patches make sure that there's an explicit dependency from
the GPIO chip in Tegra SoCs to the corresponding pinctrl device, without
having duplicated gpio ranges.

By having an explicit dependency, we can do things such as probing the
pinctrl device before the GPIO chip device to avoid deferred probes.

Thanks,

Tomeu


Tomeu Vizoso (3):
  gpio: defer probe if pinctrl cannot be found
  pinctrl: tegra: Only set the gpio range if needed
  ARM: tegra: Add gpio-ranges property

 arch/arm/boot/dts/tegra114.dtsi |  1 +
 arch/arm/boot/dts/tegra124.dtsi |  1 +
 arch/arm/boot/dts/tegra20.dtsi  |  1 +
 arch/arm/boot/dts/tegra30.dtsi  |  1 +
 drivers/gpio/gpiolib-of.c       | 27 ++++++++++++++++++---------
 drivers/gpio/gpiolib.c          |  5 ++++-
 drivers/pinctrl/pinctrl-tegra.c | 19 ++++++++++++++++++-
 include/linux/of_gpio.h         |  4 ++--
 8 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.4.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v1 0/3] Have Tegra's GPIO chip depend explicitly on the pinctrl device
@ 2015-07-01 12:45 ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-arm-kernel

Hello,

these three patches make sure that there's an explicit dependency from
the GPIO chip in Tegra SoCs to the corresponding pinctrl device, without
having duplicated gpio ranges.

By having an explicit dependency, we can do things such as probing the
pinctrl device before the GPIO chip device to avoid deferred probes.

Thanks,

Tomeu


Tomeu Vizoso (3):
  gpio: defer probe if pinctrl cannot be found
  pinctrl: tegra: Only set the gpio range if needed
  ARM: tegra: Add gpio-ranges property

 arch/arm/boot/dts/tegra114.dtsi |  1 +
 arch/arm/boot/dts/tegra124.dtsi |  1 +
 arch/arm/boot/dts/tegra20.dtsi  |  1 +
 arch/arm/boot/dts/tegra30.dtsi  |  1 +
 drivers/gpio/gpiolib-of.c       | 27 ++++++++++++++++++---------
 drivers/gpio/gpiolib.c          |  5 ++++-
 drivers/pinctrl/pinctrl-tegra.c | 19 ++++++++++++++++++-
 include/linux/of_gpio.h         |  4 ++--
 8 files changed, 46 insertions(+), 13 deletions(-)

-- 
2.4.1

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-01 12:45 ` Tomeu Vizoso
  (?)
@ 2015-07-01 12:45 ` Tomeu Vizoso
  2015-07-01 17:36   ` Rob Herring
       [not found]   ` <1435754753-31307-2-git-send-email-tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
  -1 siblings, 2 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-gpio, devicetree, linux-tegra, Tomeu Vizoso, Linus Walleij,
	Grant Likely, Rob Herring, Alexandre Courbot

When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
the pin controller isn't available.

Otherwise, the GPIO range wouldn't be set at all unless the pin
controller probed always before the GPIO chip.

With this change, the probe of the GPIO chip will be deferred and will
be retried at a later point, hopefully once the pin controller has been
registered and probed already.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 drivers/gpio/gpiolib-of.c | 27 ++++++++++++++++++---------
 drivers/gpio/gpiolib.c    |  5 ++++-
 include/linux/of_gpio.h   |  4 ++--
 3 files changed, 24 insertions(+), 12 deletions(-)

diff --git a/drivers/gpio/gpiolib-of.c b/drivers/gpio/gpiolib-of.c
index 9a0ec48..4e1f73b 100644
--- a/drivers/gpio/gpiolib-of.c
+++ b/drivers/gpio/gpiolib-of.c
@@ -338,7 +338,7 @@ void of_mm_gpiochip_remove(struct of_mm_gpio_chip *mm_gc)
 EXPORT_SYMBOL(of_mm_gpiochip_remove);
 
 #ifdef CONFIG_PINCTRL
-static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
+static int of_gpiochip_add_pin_range(struct gpio_chip *chip)
 {
 	struct device_node *np = chip->of_node;
 	struct of_phandle_args pinspec;
@@ -349,7 +349,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
 	struct property *group_names;
 
 	if (!np)
-		return;
+		return 0;
 
 	group_names = of_find_property(np, group_names_propname, NULL);
 
@@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
 
 		pctldev = of_pinctrl_get(pinspec.np);
 		if (!pctldev)
-			break;
+			return -EPROBE_DEFER;
 
 		if (pinspec.args[2]) {
 			if (group_names) {
@@ -381,7 +381,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
 					pinspec.args[1],
 					pinspec.args[2]);
 			if (ret)
-				break;
+				return ret;
 		} else {
 			/* npins == 0: special range */
 			if (pinspec.args[1]) {
@@ -411,32 +411,41 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
 			ret = gpiochip_add_pingroup_range(chip, pctldev,
 						pinspec.args[0], name);
 			if (ret)
-				break;
+				return ret;
 		}
 	}
+
+	return 0;
 }
 
 #else
-static void of_gpiochip_add_pin_range(struct gpio_chip *chip) {}
+static int of_gpiochip_add_pin_range(struct gpio_chip *chip) { return 0; }
 #endif
 
-void of_gpiochip_add(struct gpio_chip *chip)
+int of_gpiochip_add(struct gpio_chip *chip)
 {
+	int status;
+
 	if ((!chip->of_node) && (chip->dev))
 		chip->of_node = chip->dev->of_node;
 
 	if (!chip->of_node)
-		return;
+		return 0;
 
 	if (!chip->of_xlate) {
 		chip->of_gpio_n_cells = 2;
 		chip->of_xlate = of_gpio_simple_xlate;
 	}
 
-	of_gpiochip_add_pin_range(chip);
+	status = of_gpiochip_add_pin_range(chip);
+	if (status)
+		return status;
+
 	of_node_get(chip->of_node);
 
 	of_gpiochip_scan_hogs(chip);
+
+	return 0;
 }
 
 void of_gpiochip_remove(struct gpio_chip *chip)
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index bf4bd1d..a8cab33 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -287,7 +287,10 @@ int gpiochip_add(struct gpio_chip *chip)
 	INIT_LIST_HEAD(&chip->pin_ranges);
 #endif
 
-	of_gpiochip_add(chip);
+	status = of_gpiochip_add(chip);
+	if (status)
+		goto err_remove_chip;
+
 	acpi_gpiochip_add(chip);
 
 	status = gpiochip_sysfs_register(chip);
diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 69dbe31..f319182 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -54,7 +54,7 @@ extern int of_mm_gpiochip_add(struct device_node *np,
 			      struct of_mm_gpio_chip *mm_gc);
 extern void of_mm_gpiochip_remove(struct of_mm_gpio_chip *mm_gc);
 
-extern void of_gpiochip_add(struct gpio_chip *gc);
+extern int of_gpiochip_add(struct gpio_chip *gc);
 extern void of_gpiochip_remove(struct gpio_chip *gc);
 extern int of_gpio_simple_xlate(struct gpio_chip *gc,
 				const struct of_phandle_args *gpiospec,
@@ -76,7 +76,7 @@ static inline int of_gpio_simple_xlate(struct gpio_chip *gc,
 	return -ENOSYS;
 }
 
-static inline void of_gpiochip_add(struct gpio_chip *gc) { }
+static inline int of_gpiochip_add(struct gpio_chip *gc) { return 0; }
 static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
 
 #endif /* CONFIG_OF_GPIO */
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 2/3] pinctrl: tegra: Only set the gpio range if needed
  2015-07-01 12:45 ` Tomeu Vizoso
  (?)
  (?)
@ 2015-07-01 12:45 ` Tomeu Vizoso
  -1 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-gpio, devicetree, linux-tegra, Tomeu Vizoso,
	Thierry Reding, Linus Walleij, Stephen Warren, Alexandre Courbot

If the gpio DT node has the gpio-ranges property, the range will be
added by the gpio core and doesn't need to be added by the pinctrl
driver.

By having the gpio-ranges property, we have an explicit dependency from
the gpio node to the pinctrl node and we can stop using the deprecated
pinctrl_add_gpio_range() function.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 drivers/pinctrl/pinctrl-tegra.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/pinctrl-tegra.c b/drivers/pinctrl/pinctrl-tegra.c
index 0f982b8..0fd7fd2 100644
--- a/drivers/pinctrl/pinctrl-tegra.c
+++ b/drivers/pinctrl/pinctrl-tegra.c
@@ -624,6 +624,22 @@ static struct pinctrl_desc tegra_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
+static bool gpio_node_has_range(void)
+{
+	struct device_node *np;
+	bool has_prop = false;
+
+	np = of_find_compatible_node(NULL, NULL, "nvidia,tegra30-gpio");
+	if (!np)
+		return has_prop;
+
+	has_prop = of_find_property(np, "gpio-ranges", NULL);
+
+	of_node_put(np);
+
+	return has_prop;
+}
+
 int tegra_pinctrl_probe(struct platform_device *pdev,
 			const struct tegra_pinctrl_soc_data *soc_data)
 {
@@ -708,7 +724,8 @@ int tegra_pinctrl_probe(struct platform_device *pdev,
 		return PTR_ERR(pmx->pctl);
 	}
 
-	pinctrl_add_gpio_range(pmx->pctl, &tegra_pinctrl_gpio_range);
+	if (!gpio_node_has_range())
+		pinctrl_add_gpio_range(pmx->pctl, &tegra_pinctrl_gpio_range);
 
 	platform_set_drvdata(pdev, pmx);
 
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property
  2015-07-01 12:45 ` Tomeu Vizoso
  (?)
@ 2015-07-01 12:45   ` Tomeu Vizoso
  -1 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: Mark Rutland, devicetree, Russell King, Pawel Moll, Tomeu Vizoso,
	Stephen Warren, Ian Campbell, Rob Herring, linux-gpio,
	Thierry Reding, Kumar Gala, linux-tegra, Alexandre Courbot,
	linux-arm-kernel

Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
explicit.

This currently will add a duplicated entry in the map from pins to gpios
in the pinmux controller but it should be harmless and will be fixed in a
later commit.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 arch/arm/boot/dts/tegra114.dtsi | 1 +
 arch/arm/boot/dts/tegra124.dtsi | 1 +
 arch/arm/boot/dts/tegra20.dtsi  | 1 +
 arch/arm/boot/dts/tegra30.dtsi  | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index f58a3d9..7e1e171 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -234,6 +234,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 246>;
 	};
 
 	apbmisc@70000800 {
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 87318a7..4779df3 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -255,6 +255,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 251>;
 	};
 
 	apbdma: dma@0,60020000 {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index f444b67..b44277c 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -244,6 +244,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 224>;
 	};
 
 	apbmisc@70000800 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 782b11b..28c547f 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -349,6 +349,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 248>;
 	};
 
 	apbmisc@70000800 {
-- 
2.4.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property
@ 2015-07-01 12:45   ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-kernel
  Cc: linux-gpio, devicetree, linux-tegra, Tomeu Vizoso, Russell King,
	Thierry Reding, Kumar Gala, Stephen Warren, Ian Campbell,
	Rob Herring, Pawel Moll, Mark Rutland, Alexandre Courbot,
	linux-arm-kernel

Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
explicit.

This currently will add a duplicated entry in the map from pins to gpios
in the pinmux controller but it should be harmless and will be fixed in a
later commit.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 arch/arm/boot/dts/tegra114.dtsi | 1 +
 arch/arm/boot/dts/tegra124.dtsi | 1 +
 arch/arm/boot/dts/tegra20.dtsi  | 1 +
 arch/arm/boot/dts/tegra30.dtsi  | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index f58a3d9..7e1e171 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -234,6 +234,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 246>;
 	};
 
 	apbmisc@70000800 {
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 87318a7..4779df3 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -255,6 +255,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 251>;
 	};
 
 	apbdma: dma@0,60020000 {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index f444b67..b44277c 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -244,6 +244,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 224>;
 	};
 
 	apbmisc@70000800 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 782b11b..28c547f 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -349,6 +349,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 248>;
 	};
 
 	apbmisc@70000800 {
-- 
2.4.1


^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property
@ 2015-07-01 12:45   ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-01 12:45 UTC (permalink / raw)
  To: linux-arm-kernel

Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
explicit.

This currently will add a duplicated entry in the map from pins to gpios
in the pinmux controller but it should be harmless and will be fixed in a
later commit.

Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---

 arch/arm/boot/dts/tegra114.dtsi | 1 +
 arch/arm/boot/dts/tegra124.dtsi | 1 +
 arch/arm/boot/dts/tegra20.dtsi  | 1 +
 arch/arm/boot/dts/tegra30.dtsi  | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/boot/dts/tegra114.dtsi b/arch/arm/boot/dts/tegra114.dtsi
index f58a3d9..7e1e171 100644
--- a/arch/arm/boot/dts/tegra114.dtsi
+++ b/arch/arm/boot/dts/tegra114.dtsi
@@ -234,6 +234,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 246>;
 	};
 
 	apbmisc at 70000800 {
diff --git a/arch/arm/boot/dts/tegra124.dtsi b/arch/arm/boot/dts/tegra124.dtsi
index 87318a7..4779df3 100644
--- a/arch/arm/boot/dts/tegra124.dtsi
+++ b/arch/arm/boot/dts/tegra124.dtsi
@@ -255,6 +255,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 251>;
 	};
 
 	apbdma: dma at 0,60020000 {
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index f444b67..b44277c 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -244,6 +244,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 224>;
 	};
 
 	apbmisc at 70000800 {
diff --git a/arch/arm/boot/dts/tegra30.dtsi b/arch/arm/boot/dts/tegra30.dtsi
index 782b11b..28c547f 100644
--- a/arch/arm/boot/dts/tegra30.dtsi
+++ b/arch/arm/boot/dts/tegra30.dtsi
@@ -349,6 +349,7 @@
 		gpio-controller;
 		#interrupt-cells = <2>;
 		interrupt-controller;
+		gpio-ranges = <&pinmux 0 0 248>;
 	};
 
 	apbmisc at 70000800 {
-- 
2.4.1

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-01 12:45 ` [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found Tomeu Vizoso
@ 2015-07-01 17:36   ` Rob Herring
       [not found]     ` <CAL_JsqKXODBk4FVB-kiWFHLGFLCwJDjXvpVqOTvuEJubApbVSg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2015-07-10  9:29     ` Tomeu Vizoso
       [not found]   ` <1435754753-31307-2-git-send-email-tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
  1 sibling, 2 replies; 24+ messages in thread
From: Rob Herring @ 2015-07-01 17:36 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: linux-kernel, linux-gpio, devicetree, linux-tegra, Linus Walleij,
	Grant Likely, Rob Herring, Alexandre Courbot

On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
> the pin controller isn't available.
>
> Otherwise, the GPIO range wouldn't be set at all unless the pin
> controller probed always before the GPIO chip.
>
> With this change, the probe of the GPIO chip will be deferred and will
> be retried at a later point, hopefully once the pin controller has been
> registered and probed already.

This will break cases where the pinctrl driver does not exist, but the
DT contains pinctrl bindings. We can have similar problems already
with clocks though. However, IMO this problem is a bit different in
that pinctrl is more likely entirely optional while clocks are often
required. You may do all pin setup in bootloader/firmware on some
boards and not others. Of course then why put pinctrl in the DT in
that case? They could be present just due to how chip vs. board dts
files are structured.

We could address this by simply marking the pin controller node
disabled. However, ...

> @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
>
>                 pctldev = of_pinctrl_get(pinspec.np);
>                 if (!pctldev)
> -                       break;
> +                       return -EPROBE_DEFER;

But you cannot distinguish that case here. I think of_pinctrl_get
needs to set the error code appropriately.

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-01 12:45 ` [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found Tomeu Vizoso
@ 2015-07-02  8:49       ` Geert Uytterhoeven
       [not found]   ` <1435754753-31307-2-git-send-email-tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
  1 sibling, 0 replies; 24+ messages in thread
From: Geert Uytterhoeven @ 2015-07-02  8:49 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Grant Likely,
	Rob Herring, Alexandre Courbot

Hi Tomeu,

On Wed, Jul 1, 2015 at 2:45 PM, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
> the pin controller isn't available.
>
> Otherwise, the GPIO range wouldn't be set at all unless the pin
> controller probed always before the GPIO chip.
>
> With this change, the probe of the GPIO chip will be deferred and will
> be retried at a later point, hopefully once the pin controller has been
> registered and probed already.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>

I was a bit afraid this would break the case of gpio controllers that are
also pin controllers, i.e. where "gpio-ranges" points to the gpio controller
itself[*], but it doesn't.

Tested-by: Geert Uytterhoeven <geert+renesas-gXvu3+zWzMSzQB+pC5nmwQ@public.gmane.org>

[*] E.g. "[PATCH 2/7] ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges"
    to gpio node" (http://www.spinics.net/lists/linux-sh/msg43077.html)

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
@ 2015-07-02  8:49       ` Geert Uytterhoeven
  0 siblings, 0 replies; 24+ messages in thread
From: Geert Uytterhoeven @ 2015-07-02  8:49 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: linux-kernel, linux-gpio, devicetree, linux-tegra, Linus Walleij,
	Grant Likely, Rob Herring, Alexandre Courbot

Hi Tomeu,

On Wed, Jul 1, 2015 at 2:45 PM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
> the pin controller isn't available.
>
> Otherwise, the GPIO range wouldn't be set at all unless the pin
> controller probed always before the GPIO chip.
>
> With this change, the probe of the GPIO chip will be deferred and will
> be retried at a later point, hopefully once the pin controller has been
> registered and probed already.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

I was a bit afraid this would break the case of gpio controllers that are
also pin controllers, i.e. where "gpio-ranges" points to the gpio controller
itself[*], but it doesn't.

Tested-by: Geert Uytterhoeven <geert+renesas@glider.be>

[*] E.g. "[PATCH 2/7] ARM: shmobile: r8a7740 dtsi: Add missing "gpio-ranges"
    to gpio node" (http://www.spinics.net/lists/linux-sh/msg43077.html)

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-01 17:36   ` Rob Herring
@ 2015-07-02  8:59         ` Geert Uytterhoeven
  2015-07-10  9:29     ` Tomeu Vizoso
  1 sibling, 0 replies; 24+ messages in thread
From: Geert Uytterhoeven @ 2015-07-02  8:59 UTC (permalink / raw)
  To: Rob Herring
  Cc: Tomeu Vizoso, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Grant Likely,
	Rob Herring, Alexandre Courbot

Hi Rob,

On Wed, Jul 1, 2015 at 7:36 PM, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>> the pin controller isn't available.
>>
>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>> controller probed always before the GPIO chip.
>>
>> With this change, the probe of the GPIO chip will be deferred and will
>> be retried at a later point, hopefully once the pin controller has been
>> registered and probed already.
>
> This will break cases where the pinctrl driver does not exist, but the
> DT contains pinctrl bindings. We can have similar problems already
> with clocks though. However, IMO this problem is a bit different in
> that pinctrl is more likely entirely optional while clocks are often
> required. You may do all pin setup in bootloader/firmware on some
> boards and not others. Of course then why put pinctrl in the DT in
> that case? They could be present just due to how chip vs. board dts
> files are structured.

Isn't that already the case?
If I change the compatible value of a pinctrl node to an invalid value, I get:

    sh-sci e6c50000.serial: could not find pctldev for node
/pfc@e6050000/serial1, deferring probe

> We could address this by simply marking the pin controller node
> disabled. However, ...

Doesn't seem to make any difference.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
@ 2015-07-02  8:59         ` Geert Uytterhoeven
  0 siblings, 0 replies; 24+ messages in thread
From: Geert Uytterhoeven @ 2015-07-02  8:59 UTC (permalink / raw)
  To: Rob Herring
  Cc: Tomeu Vizoso, linux-kernel, linux-gpio, devicetree, linux-tegra,
	Linus Walleij, Grant Likely, Rob Herring, Alexandre Courbot

Hi Rob,

On Wed, Jul 1, 2015 at 7:36 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>> the pin controller isn't available.
>>
>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>> controller probed always before the GPIO chip.
>>
>> With this change, the probe of the GPIO chip will be deferred and will
>> be retried at a later point, hopefully once the pin controller has been
>> registered and probed already.
>
> This will break cases where the pinctrl driver does not exist, but the
> DT contains pinctrl bindings. We can have similar problems already
> with clocks though. However, IMO this problem is a bit different in
> that pinctrl is more likely entirely optional while clocks are often
> required. You may do all pin setup in bootloader/firmware on some
> boards and not others. Of course then why put pinctrl in the DT in
> that case? They could be present just due to how chip vs. board dts
> files are structured.

Isn't that already the case?
If I change the compatible value of a pinctrl node to an invalid value, I get:

    sh-sci e6c50000.serial: could not find pctldev for node
/pfc@e6050000/serial1, deferring probe

> We could address this by simply marking the pin controller node
> disabled. However, ...

Doesn't seem to make any difference.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-02  8:59         ` Geert Uytterhoeven
  (?)
@ 2015-07-02 15:38         ` Rob Herring
  -1 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2015-07-02 15:38 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Tomeu Vizoso, linux-kernel, linux-gpio, devicetree, linux-tegra,
	Linus Walleij, Grant Likely, Rob Herring, Alexandre Courbot

On Thu, Jul 2, 2015 at 3:59 AM, Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Rob,
>
> On Wed, Jul 1, 2015 at 7:36 PM, Rob Herring <robherring2@gmail.com> wrote:
>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>> the pin controller isn't available.
>>>
>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>> controller probed always before the GPIO chip.
>>>
>>> With this change, the probe of the GPIO chip will be deferred and will
>>> be retried at a later point, hopefully once the pin controller has been
>>> registered and probed already.
>>
>> This will break cases where the pinctrl driver does not exist, but the
>> DT contains pinctrl bindings. We can have similar problems already
>> with clocks though. However, IMO this problem is a bit different in
>> that pinctrl is more likely entirely optional while clocks are often
>> required. You may do all pin setup in bootloader/firmware on some
>> boards and not others. Of course then why put pinctrl in the DT in
>> that case? They could be present just due to how chip vs. board dts
>> files are structured.
>
> Isn't that already the case?
> If I change the compatible value of a pinctrl node to an invalid value, I get:
>
>     sh-sci e6c50000.serial: could not find pctldev for node
> /pfc@e6050000/serial1, deferring probe

I guess so.

>> We could address this by simply marking the pin controller node
>> disabled. However, ...
>
> Doesn't seem to make any difference.

No doubt. I'm proposing that it should, not that it does already. Of
course, the callers will also have to test for -ENODEV and ignore
those errors.

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property
  2015-07-01 12:45   ` Tomeu Vizoso
@ 2015-07-08 20:55     ` Stephen Warren
  -1 siblings, 0 replies; 24+ messages in thread
From: Stephen Warren @ 2015-07-08 20:55 UTC (permalink / raw)
  To: Tomeu Vizoso, linux-kernel
  Cc: linux-gpio, devicetree, linux-tegra, Russell King,
	Thierry Reding, Kumar Gala, Ian Campbell, Rob Herring,
	Pawel Moll, Mark Rutland, Alexandre Courbot, linux-arm-kernel

On 07/01/2015 06:45 AM, Tomeu Vizoso wrote:
> Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
> explicit.
>
> This currently will add a duplicated entry in the map from pins to gpios
> in the pinmux controller but it should be harmless and will be fixed in a
> later commit.

Isn't it in an earlier commit now (patch 2/3)? :-)

At a quick glance, I think this approach will be fine, so the series,
Acked-by: Stephen Warren <swarren@nvidia.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property
@ 2015-07-08 20:55     ` Stephen Warren
  0 siblings, 0 replies; 24+ messages in thread
From: Stephen Warren @ 2015-07-08 20:55 UTC (permalink / raw)
  To: linux-arm-kernel

On 07/01/2015 06:45 AM, Tomeu Vizoso wrote:
> Specify how the GPIOs map to the pins in Tegra SoCs, so the dependency is
> explicit.
>
> This currently will add a duplicated entry in the map from pins to gpios
> in the pinmux controller but it should be harmless and will be fixed in a
> later commit.

Isn't it in an earlier commit now (patch 2/3)? :-)

At a quick glance, I think this approach will be fine, so the series,
Acked-by: Stephen Warren <swarren@nvidia.com>

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-01 17:36   ` Rob Herring
       [not found]     ` <CAL_JsqKXODBk4FVB-kiWFHLGFLCwJDjXvpVqOTvuEJubApbVSg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2015-07-10  9:29     ` Tomeu Vizoso
  2015-07-10 15:27       ` Stephen Warren
       [not found]       ` <CAAObsKBS0OOhPNEAEV5Bh_+WMVWYhuGtGLqaVdCbzxPuT8a_gA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 2 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-10  9:29 UTC (permalink / raw)
  To: Rob Herring
  Cc: linux-kernel, linux-gpio, devicetree, linux-tegra, Linus Walleij,
	Grant Likely, Rob Herring, Alexandre Courbot

On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote:
> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>> the pin controller isn't available.
>>
>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>> controller probed always before the GPIO chip.
>>
>> With this change, the probe of the GPIO chip will be deferred and will
>> be retried at a later point, hopefully once the pin controller has been
>> registered and probed already.
>
> This will break cases where the pinctrl driver does not exist, but the
> DT contains pinctrl bindings. We can have similar problems already
> with clocks though. However, IMO this problem is a bit different in
> that pinctrl is more likely entirely optional while clocks are often
> required. You may do all pin setup in bootloader/firmware on some
> boards and not others. Of course then why put pinctrl in the DT in
> that case? They could be present just due to how chip vs. board dts
> files are structured.

I see. My instinct tells me that it would be better if the gpio-ranges
property was set in the board dts, but I don't really know what each
mach does with its DTSs.

> We could address this by simply marking the pin controller node
> disabled. However, ...
>
>> @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
>>
>>                 pctldev = of_pinctrl_get(pinspec.np);
>>                 if (!pctldev)
>> -                       break;
>> +                       return -EPROBE_DEFER;
>
> But you cannot distinguish that case here. I think of_pinctrl_get
> needs to set the error code appropriately.

Why not? I was thinking of just doing this before we call of_pinctrl_get():

        if (!of_device_is_available(pinspec.np))
            continue;

Thanks,

Tomeu

> Rob
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-10  9:29     ` Tomeu Vizoso
@ 2015-07-10 15:27       ` Stephen Warren
       [not found]         ` <559FE471.1030408-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
       [not found]       ` <CAAObsKBS0OOhPNEAEV5Bh_+WMVWYhuGtGLqaVdCbzxPuT8a_gA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 24+ messages in thread
From: Stephen Warren @ 2015-07-10 15:27 UTC (permalink / raw)
  To: Tomeu Vizoso, Rob Herring
  Cc: linux-kernel, linux-gpio, devicetree, linux-tegra, Linus Walleij,
	Grant Likely, Rob Herring, Alexandre Courbot

On 07/10/2015 03:29 AM, Tomeu Vizoso wrote:
> On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote:
>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>> the pin controller isn't available.
>>>
>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>> controller probed always before the GPIO chip.
>>>
>>> With this change, the probe of the GPIO chip will be deferred and will
>>> be retried at a later point, hopefully once the pin controller has been
>>> registered and probed already.
>>
>> This will break cases where the pinctrl driver does not exist, but the
>> DT contains pinctrl bindings. We can have similar problems already
>> with clocks though. However, IMO this problem is a bit different in
>> that pinctrl is more likely entirely optional while clocks are often
>> required. You may do all pin setup in bootloader/firmware on some
>> boards and not others. Of course then why put pinctrl in the DT in
>> that case? They could be present just due to how chip vs. board dts
>> files are structured.
>
> I see. My instinct tells me that it would be better if the gpio-ranges
> property was set in the board dts, but I don't really know what each
> mach does with its DTSs.

That doesn't make sense; the mapping between GPIO controller pins and 
pin controller pins is a property of the SoC not the board.



^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
       [not found]         ` <559FE471.1030408-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
  2015-07-10 16:21             ` Tomeu Vizoso
@ 2015-07-10 16:21             ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-10 16:21 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Rob Herring, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Grant Likely,
	Rob Herring, Alexandre Courbot

On 10 July 2015 at 17:27, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/10/2015 03:29 AM, Tomeu Vizoso wrote:
>>
>> On 1 July 2015 at 19:36, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> wrote:
>>>>
>>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>>> the pin controller isn't available.
>>>>
>>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>>> controller probed always before the GPIO chip.
>>>>
>>>> With this change, the probe of the GPIO chip will be deferred and will
>>>> be retried at a later point, hopefully once the pin controller has been
>>>> registered and probed already.
>>>
>>>
>>> This will break cases where the pinctrl driver does not exist, but the
>>> DT contains pinctrl bindings. We can have similar problems already
>>> with clocks though. However, IMO this problem is a bit different in
>>> that pinctrl is more likely entirely optional while clocks are often
>>> required. You may do all pin setup in bootloader/firmware on some
>>> boards and not others. Of course then why put pinctrl in the DT in
>>> that case? They could be present just due to how chip vs. board dts
>>> files are structured.
>>
>>
>> I see. My instinct tells me that it would be better if the gpio-ranges
>> property was set in the board dts, but I don't really know what each
>> mach does with its DTSs.
>
>
> That doesn't make sense; the mapping between GPIO controller pins and pin
> controller pins is a property of the SoC not the board.

From what Rob said above, apparently some boards will rely on the pin
setup done by the bootloader, and some other boards with the same soc
will want to do it in the kernel. So it's not really a difference in
the hw itself, but what expectations exist about the firmware on a
specific board.

Regards,

Tomeu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
@ 2015-07-10 16:21             ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-10 16:21 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Rob Herring, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Grant Likely,
	Rob Herring, Alexandre Courbot

On 10 July 2015 at 17:27, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/10/2015 03:29 AM, Tomeu Vizoso wrote:
>>
>> On 1 July 2015 at 19:36, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>
>>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>> wrote:
>>>>
>>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>>> the pin controller isn't available.
>>>>
>>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>>> controller probed always before the GPIO chip.
>>>>
>>>> With this change, the probe of the GPIO chip will be deferred and will
>>>> be retried at a later point, hopefully once the pin controller has been
>>>> registered and probed already.
>>>
>>>
>>> This will break cases where the pinctrl driver does not exist, but the
>>> DT contains pinctrl bindings. We can have similar problems already
>>> with clocks though. However, IMO this problem is a bit different in
>>> that pinctrl is more likely entirely optional while clocks are often
>>> required. You may do all pin setup in bootloader/firmware on some
>>> boards and not others. Of course then why put pinctrl in the DT in
>>> that case? They could be present just due to how chip vs. board dts
>>> files are structured.
>>
>>
>> I see. My instinct tells me that it would be better if the gpio-ranges
>> property was set in the board dts, but I don't really know what each
>> mach does with its DTSs.
>
>
> That doesn't make sense; the mapping between GPIO controller pins and pin
> controller pins is a property of the SoC not the board.

>From what Rob said above, apparently some boards will rely on the pin
setup done by the bootloader, and some other boards with the same soc
will want to do it in the kernel. So it's not really a difference in
the hw itself, but what expectations exist about the firmware on a
specific board.

Regards,

Tomeu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
@ 2015-07-10 16:21             ` Tomeu Vizoso
  0 siblings, 0 replies; 24+ messages in thread
From: Tomeu Vizoso @ 2015-07-10 16:21 UTC (permalink / raw)
  To: Stephen Warren
  Cc: Rob Herring, linux-kernel, linux-gpio, devicetree, linux-tegra,
	Linus Walleij, Grant Likely, Rob Herring, Alexandre Courbot

On 10 July 2015 at 17:27, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/10/2015 03:29 AM, Tomeu Vizoso wrote:
>>
>> On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote:
>>>
>>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>> wrote:
>>>>
>>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>>> the pin controller isn't available.
>>>>
>>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>>> controller probed always before the GPIO chip.
>>>>
>>>> With this change, the probe of the GPIO chip will be deferred and will
>>>> be retried at a later point, hopefully once the pin controller has been
>>>> registered and probed already.
>>>
>>>
>>> This will break cases where the pinctrl driver does not exist, but the
>>> DT contains pinctrl bindings. We can have similar problems already
>>> with clocks though. However, IMO this problem is a bit different in
>>> that pinctrl is more likely entirely optional while clocks are often
>>> required. You may do all pin setup in bootloader/firmware on some
>>> boards and not others. Of course then why put pinctrl in the DT in
>>> that case? They could be present just due to how chip vs. board dts
>>> files are structured.
>>
>>
>> I see. My instinct tells me that it would be better if the gpio-ranges
>> property was set in the board dts, but I don't really know what each
>> mach does with its DTSs.
>
>
> That doesn't make sense; the mapping between GPIO controller pins and pin
> controller pins is a property of the SoC not the board.

>From what Rob said above, apparently some boards will rely on the pin
setup done by the bootloader, and some other boards with the same soc
will want to do it in the kernel. So it's not really a difference in
the hw itself, but what expectations exist about the firmware on a
specific board.

Regards,

Tomeu

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-10 16:21             ` Tomeu Vizoso
@ 2015-07-10 17:07                 ` Stephen Warren
  -1 siblings, 0 replies; 24+ messages in thread
From: Stephen Warren @ 2015-07-10 17:07 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: Rob Herring, linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Grant Likely,
	Rob Herring, Alexandre Courbot

On 07/10/2015 10:21 AM, Tomeu Vizoso wrote:
> On 10 July 2015 at 17:27, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 07/10/2015 03:29 AM, Tomeu Vizoso wrote:
>>>
>>> On 1 July 2015 at 19:36, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>
>>>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
>>>> wrote:
>>>>>
>>>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>>>> the pin controller isn't available.
>>>>>
>>>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>>>> controller probed always before the GPIO chip.
>>>>>
>>>>> With this change, the probe of the GPIO chip will be deferred and will
>>>>> be retried at a later point, hopefully once the pin controller has been
>>>>> registered and probed already.
>>>>
>>>>
>>>> This will break cases where the pinctrl driver does not exist, but the
>>>> DT contains pinctrl bindings. We can have similar problems already
>>>> with clocks though. However, IMO this problem is a bit different in
>>>> that pinctrl is more likely entirely optional while clocks are often
>>>> required. You may do all pin setup in bootloader/firmware on some
>>>> boards and not others. Of course then why put pinctrl in the DT in
>>>> that case? They could be present just due to how chip vs. board dts
>>>> files are structured.
>>>
>>>
>>> I see. My instinct tells me that it would be better if the gpio-ranges
>>> property was set in the board dts, but I don't really know what each
>>> mach does with its DTSs.
>>
>>
>> That doesn't make sense; the mapping between GPIO controller pins and pin
>> controller pins is a property of the SoC not the board.
>
>  From what Rob said above, apparently some boards will rely on the pin
> setup done by the bootloader, and some other boards with the same soc
> will want to do it in the kernel. So it's not really a difference in
> the hw itself, but what expectations exist about the firmware on a
> specific board.

Sure, but none of that changes the mapping between the GPIO and pin 
controller pins.

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
@ 2015-07-10 17:07                 ` Stephen Warren
  0 siblings, 0 replies; 24+ messages in thread
From: Stephen Warren @ 2015-07-10 17:07 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: Rob Herring, linux-kernel, linux-gpio, devicetree, linux-tegra,
	Linus Walleij, Grant Likely, Rob Herring, Alexandre Courbot

On 07/10/2015 10:21 AM, Tomeu Vizoso wrote:
> On 10 July 2015 at 17:27, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 07/10/2015 03:29 AM, Tomeu Vizoso wrote:
>>>
>>> On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote:
>>>>
>>>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com>
>>>> wrote:
>>>>>
>>>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>>>> the pin controller isn't available.
>>>>>
>>>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>>>> controller probed always before the GPIO chip.
>>>>>
>>>>> With this change, the probe of the GPIO chip will be deferred and will
>>>>> be retried at a later point, hopefully once the pin controller has been
>>>>> registered and probed already.
>>>>
>>>>
>>>> This will break cases where the pinctrl driver does not exist, but the
>>>> DT contains pinctrl bindings. We can have similar problems already
>>>> with clocks though. However, IMO this problem is a bit different in
>>>> that pinctrl is more likely entirely optional while clocks are often
>>>> required. You may do all pin setup in bootloader/firmware on some
>>>> boards and not others. Of course then why put pinctrl in the DT in
>>>> that case? They could be present just due to how chip vs. board dts
>>>> files are structured.
>>>
>>>
>>> I see. My instinct tells me that it would be better if the gpio-ranges
>>> property was set in the board dts, but I don't really know what each
>>> mach does with its DTSs.
>>
>>
>> That doesn't make sense; the mapping between GPIO controller pins and pin
>> controller pins is a property of the SoC not the board.
>
>  From what Rob said above, apparently some boards will rely on the pin
> setup done by the bootloader, and some other boards with the same soc
> will want to do it in the kernel. So it's not really a difference in
> the hw itself, but what expectations exist about the firmware on a
> specific board.

Sure, but none of that changes the mapping between the GPIO and pin 
controller pins.


^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
  2015-07-10  9:29     ` Tomeu Vizoso
@ 2015-07-10 18:40           ` Rob Herring
       [not found]       ` <CAAObsKBS0OOhPNEAEV5Bh_+WMVWYhuGtGLqaVdCbzxPuT8a_gA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 0 replies; 24+ messages in thread
From: Rob Herring @ 2015-07-10 18:40 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: linux-kernel-u79uwXL29TY76Z2rM5mHXA,
	linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA,
	linux-tegra-u79uwXL29TY76Z2rM5mHXA, Linus Walleij, Grant Likely,
	Rob Herring, Alexandre Courbot

On Fri, Jul 10, 2015 at 4:29 AM, Tomeu Vizoso
<tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
> On 1 July 2015 at 19:36, Rob Herring <robherring2-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org> wrote:
>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>> the pin controller isn't available.
>>>
>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>> controller probed always before the GPIO chip.
>>>
>>> With this change, the probe of the GPIO chip will be deferred and will
>>> be retried at a later point, hopefully once the pin controller has been
>>> registered and probed already.
>>
>> This will break cases where the pinctrl driver does not exist, but the
>> DT contains pinctrl bindings. We can have similar problems already
>> with clocks though. However, IMO this problem is a bit different in
>> that pinctrl is more likely entirely optional while clocks are often
>> required. You may do all pin setup in bootloader/firmware on some
>> boards and not others. Of course then why put pinctrl in the DT in
>> that case? They could be present just due to how chip vs. board dts
>> files are structured.
>
> I see. My instinct tells me that it would be better if the gpio-ranges
> property was set in the board dts, but I don't really know what each
> mach does with its DTSs.
>
>> We could address this by simply marking the pin controller node
>> disabled. However, ...
>>
>>> @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
>>>
>>>                 pctldev = of_pinctrl_get(pinspec.np);
>>>                 if (!pctldev)
>>> -                       break;
>>> +                       return -EPROBE_DEFER;
>>
>> But you cannot distinguish that case here. I think of_pinctrl_get
>> needs to set the error code appropriately.
>
> Why not? I was thinking of just doing this before we call of_pinctrl_get():
>
>         if (!of_device_is_available(pinspec.np))
>             continue;

That is exactly what you need, but that should be of_pinctrl_get's
responsibility to check, not the caller's. IIRC, this is the only user
of of_pinctrl_get, so it should be just as easy to change.

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found
@ 2015-07-10 18:40           ` Rob Herring
  0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2015-07-10 18:40 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: linux-kernel, linux-gpio, devicetree, linux-tegra, Linus Walleij,
	Grant Likely, Rob Herring, Alexandre Courbot

On Fri, Jul 10, 2015 at 4:29 AM, Tomeu Vizoso
<tomeu.vizoso@collabora.com> wrote:
> On 1 July 2015 at 19:36, Rob Herring <robherring2@gmail.com> wrote:
>> On Wed, Jul 1, 2015 at 7:45 AM, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> When an OF node has a pin range for its GPIOs, return -EPROBE_DEFER if
>>> the pin controller isn't available.
>>>
>>> Otherwise, the GPIO range wouldn't be set at all unless the pin
>>> controller probed always before the GPIO chip.
>>>
>>> With this change, the probe of the GPIO chip will be deferred and will
>>> be retried at a later point, hopefully once the pin controller has been
>>> registered and probed already.
>>
>> This will break cases where the pinctrl driver does not exist, but the
>> DT contains pinctrl bindings. We can have similar problems already
>> with clocks though. However, IMO this problem is a bit different in
>> that pinctrl is more likely entirely optional while clocks are often
>> required. You may do all pin setup in bootloader/firmware on some
>> boards and not others. Of course then why put pinctrl in the DT in
>> that case? They could be present just due to how chip vs. board dts
>> files are structured.
>
> I see. My instinct tells me that it would be better if the gpio-ranges
> property was set in the board dts, but I don't really know what each
> mach does with its DTSs.
>
>> We could address this by simply marking the pin controller node
>> disabled. However, ...
>>
>>> @@ -361,7 +361,7 @@ static void of_gpiochip_add_pin_range(struct gpio_chip *chip)
>>>
>>>                 pctldev = of_pinctrl_get(pinspec.np);
>>>                 if (!pctldev)
>>> -                       break;
>>> +                       return -EPROBE_DEFER;
>>
>> But you cannot distinguish that case here. I think of_pinctrl_get
>> needs to set the error code appropriately.
>
> Why not? I was thinking of just doing this before we call of_pinctrl_get():
>
>         if (!of_device_is_available(pinspec.np))
>             continue;

That is exactly what you need, but that should be of_pinctrl_get's
responsibility to check, not the caller's. IIRC, this is the only user
of of_pinctrl_get, so it should be just as easy to change.

Rob

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2015-07-10 18:41 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-01 12:45 [PATCH v1 0/3] Have Tegra's GPIO chip depend explicitly on the pinctrl device Tomeu Vizoso
2015-07-01 12:45 ` Tomeu Vizoso
2015-07-01 12:45 ` [PATCH v1 1/3] gpio: defer probe if pinctrl cannot be found Tomeu Vizoso
2015-07-01 17:36   ` Rob Herring
     [not found]     ` <CAL_JsqKXODBk4FVB-kiWFHLGFLCwJDjXvpVqOTvuEJubApbVSg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-07-02  8:59       ` Geert Uytterhoeven
2015-07-02  8:59         ` Geert Uytterhoeven
2015-07-02 15:38         ` Rob Herring
2015-07-10  9:29     ` Tomeu Vizoso
2015-07-10 15:27       ` Stephen Warren
     [not found]         ` <559FE471.1030408-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
2015-07-10 16:21           ` Tomeu Vizoso
2015-07-10 16:21             ` Tomeu Vizoso
2015-07-10 16:21             ` Tomeu Vizoso
     [not found]             ` <CAAObsKAav-5-0bZA2XYW4m7eUd9F7yc+9kGvmt_hS-Z0ip=zKw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-07-10 17:07               ` Stephen Warren
2015-07-10 17:07                 ` Stephen Warren
     [not found]       ` <CAAObsKBS0OOhPNEAEV5Bh_+WMVWYhuGtGLqaVdCbzxPuT8a_gA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-07-10 18:40         ` Rob Herring
2015-07-10 18:40           ` Rob Herring
     [not found]   ` <1435754753-31307-2-git-send-email-tomeu.vizoso-ZGY8ohtN/8qB+jHODAdFcQ@public.gmane.org>
2015-07-02  8:49     ` Geert Uytterhoeven
2015-07-02  8:49       ` Geert Uytterhoeven
2015-07-01 12:45 ` [PATCH v1 2/3] pinctrl: tegra: Only set the gpio range if needed Tomeu Vizoso
2015-07-01 12:45 ` [PATCH v1 3/3] ARM: tegra: Add gpio-ranges property Tomeu Vizoso
2015-07-01 12:45   ` Tomeu Vizoso
2015-07-01 12:45   ` Tomeu Vizoso
2015-07-08 20:55   ` Stephen Warren
2015-07-08 20:55     ` Stephen Warren

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.