linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] Add fixes to STM32 pintrl
@ 2017-04-07 15:10 Alexandre TORGUE
  2017-04-07 15:10 ` [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt Alexandre TORGUE
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Alexandre TORGUE @ 2017-04-07 15:10 UTC (permalink / raw)
  To: Linus Walleij, Maxime Coquelin, Patrice Chotard, Paul Gortmaker,
	Rob Herring
  Cc: linux-kernel, linux-gpio, linux-arm-kernel, devicetree

This series add several fixes to STM32 pinctrl:
 - Set input mode when PIN is used as interrupt
 - Implement .get_direction() gpio_chip callback  
 - In DT: set each gpio controller as a interrupt controller. User who
   wants to use gpio as interrupt will have choice to use either "gpiolib"
   interface or "common" interrupt interface.

Regards
Alex

Alexandre TORGUE (4):
  pinctrl: stm32: set pin to gpio input when used as interrupt
  pinctrl: stm32: replace device_initcall() with arch_initcall()
  pinctrl: stm32: Implement .get_direction gpio_chip callback
  ARM: dts: stm32: Set gpio controller also as interrupt controller

 arch/arm/boot/dts/stm32f429.dtsi          | 22 +++++++++++++++
 arch/arm/boot/dts/stm32f746.dtsi          | 22 +++++++++++++++
 drivers/pinctrl/stm32/pinctrl-stm32.c     | 47 +++++++++++++++++++++++++++++--
 drivers/pinctrl/stm32/pinctrl-stm32.h     |  5 +++-
 drivers/pinctrl/stm32/pinctrl-stm32f429.c |  6 +++-
 drivers/pinctrl/stm32/pinctrl-stm32f746.c |  7 ++++-
 drivers/pinctrl/stm32/pinctrl-stm32h743.c |  6 +++-
 7 files changed, 109 insertions(+), 6 deletions(-)

-- 
1.9.1

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

* [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt
  2017-04-07 15:10 [PATCH 0/4] Add fixes to STM32 pintrl Alexandre TORGUE
@ 2017-04-07 15:10 ` Alexandre TORGUE
  2017-04-24 12:36   ` Linus Walleij
  2017-04-07 15:10 ` [PATCH 2/4] pinctrl: stm32: replace device_initcall() with arch_initcall() Alexandre TORGUE
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Alexandre TORGUE @ 2017-04-07 15:10 UTC (permalink / raw)
  To: Linus Walleij, Maxime Coquelin, Patrice Chotard, Paul Gortmaker,
	Rob Herring
  Cc: linux-kernel, linux-gpio, linux-arm-kernel, devicetree

This patch ensures that pin is correctly set as gpio input when it is used
as an interrupt.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index abc405b..c8825e5 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -207,12 +207,35 @@ static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
 	.to_irq			= stm32_gpio_to_irq,
 };
 
+static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
+{
+	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
+	u32 ret;
+
+	if (!gpiochip_is_requested(&bank->gpio_chip, irq_data->hwirq)) {
+		ret = stm32_gpio_request(&bank->gpio_chip, irq_data->hwirq);
+		if (ret)
+			return ret;
+	}
+
+	return stm32_gpio_direction_input(&bank->gpio_chip, irq_data->hwirq);
+}
+
+static void stm32_gpio_irq_release_resources(struct irq_data *irq_data)
+{
+	struct stm32_gpio_bank *bank = irq_data->domain->host_data;
+
+	stm32_gpio_free(&bank->gpio_chip, irq_data->hwirq);
+}
+
 static struct irq_chip stm32_gpio_irq_chip = {
 	.name           = "stm32gpio",
 	.irq_eoi	= irq_chip_eoi_parent,
 	.irq_mask       = irq_chip_mask_parent,
 	.irq_unmask     = irq_chip_unmask_parent,
 	.irq_set_type   = irq_chip_set_type_parent,
+	.irq_request_resources = stm32_gpio_irq_request_resources,
+	.irq_release_resources = stm32_gpio_irq_release_resources,
 };
 
 static int stm32_gpio_domain_translate(struct irq_domain *d,
-- 
1.9.1

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

* [PATCH 2/4] pinctrl: stm32: replace device_initcall() with arch_initcall()
  2017-04-07 15:10 [PATCH 0/4] Add fixes to STM32 pintrl Alexandre TORGUE
  2017-04-07 15:10 ` [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt Alexandre TORGUE
@ 2017-04-07 15:10 ` Alexandre TORGUE
  2017-04-24 12:22   ` Linus Walleij
  2017-04-07 15:10 ` [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback Alexandre TORGUE
  2017-04-07 15:10 ` [PATCH 4/4] ARM: dts: stm32: Set gpio controller also as interrupt controller Alexandre TORGUE
  3 siblings, 1 reply; 11+ messages in thread
From: Alexandre TORGUE @ 2017-04-07 15:10 UTC (permalink / raw)
  To: Linus Walleij, Maxime Coquelin, Patrice Chotard, Paul Gortmaker,
	Rob Herring
  Cc: linux-kernel, linux-gpio, linux-arm-kernel, devicetree

Pinctrl has to be registered earlier. Mainly to register bank irqdomain
earlier as other devices could use interrupts from those irqdomain.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32f429.c b/drivers/pinctrl/stm32/pinctrl-stm32f429.c
index 990b867..4bbade2 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32f429.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32f429.c
@@ -1584,4 +1584,8 @@
 	},
 };
 
-builtin_platform_driver(stm32f429_pinctrl_driver);
+static int __init stm32f429_pinctrl_init(void)
+{
+	return platform_driver_register(&stm32f429_pinctrl_driver);
+}
+arch_initcall(stm32f429_pinctrl_init);
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32f746.c b/drivers/pinctrl/stm32/pinctrl-stm32f746.c
index c0b4462..a2fae73 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32f746.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32f746.c
@@ -1678,4 +1678,9 @@
 		.of_match_table = stm32f746_pctrl_match,
 	},
 };
-builtin_platform_driver(stm32f746_pinctrl_driver);
+
+static int __init stm32f746_pinctrl_init(void)
+{
+	return platform_driver_register(&stm32f746_pinctrl_driver);
+}
+arch_initcall(stm32f746_pinctrl_init);
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32h743.c b/drivers/pinctrl/stm32/pinctrl-stm32h743.c
index f7f9eac..e34b2b9 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32h743.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32h743.c
@@ -1977,4 +1977,8 @@
 	},
 };
 
-builtin_platform_driver(stm32h743_pinctrl_driver);
+static int __init stm32h743_pinctrl_init(void)
+{
+	return platform_driver_register(&stm32h743_pinctrl_driver);
+}
+arch_initcall(stm32h743_pinctrl_init);
-- 
1.9.1

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

* [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback
  2017-04-07 15:10 [PATCH 0/4] Add fixes to STM32 pintrl Alexandre TORGUE
  2017-04-07 15:10 ` [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt Alexandre TORGUE
  2017-04-07 15:10 ` [PATCH 2/4] pinctrl: stm32: replace device_initcall() with arch_initcall() Alexandre TORGUE
@ 2017-04-07 15:10 ` Alexandre TORGUE
  2017-04-24 12:37   ` Linus Walleij
  2017-04-07 15:10 ` [PATCH 4/4] ARM: dts: stm32: Set gpio controller also as interrupt controller Alexandre TORGUE
  3 siblings, 1 reply; 11+ messages in thread
From: Alexandre TORGUE @ 2017-04-07 15:10 UTC (permalink / raw)
  To: Linus Walleij, Maxime Coquelin, Patrice Chotard, Paul Gortmaker,
	Rob Herring
  Cc: linux-kernel, linux-gpio, linux-arm-kernel, devicetree

Add .get_direction() gpiochip callback in STM32 pinctrl driver.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.c b/drivers/pinctrl/stm32/pinctrl-stm32.c
index c8825e5..fdde60f 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.c
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.c
@@ -25,6 +25,7 @@
 #include <linux/regmap.h>
 #include <linux/reset.h>
 #include <linux/slab.h>
+#include <linux/gpio.h>
 
 #include "../core.h"
 #include "../pinconf.h"
@@ -197,6 +198,24 @@ static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
 	return irq_create_fwspec_mapping(&fwspec);
 }
 
+static int stm32_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct stm32_gpio_bank *bank = gpiochip_get_data(chip);
+	int pin = stm32_gpio_pin(offset);
+	int ret;
+	u32 mode, alt;
+
+	stm32_pmx_get_mode(bank, pin, &mode, &alt);
+	if ((alt == 0) && (mode == 0))
+		ret = GPIOF_DIR_IN;
+	else if ((alt == 0) && (mode == 1))
+		ret = GPIOF_DIR_OUT;
+	else
+		ret = -EINVAL;
+
+	return ret;
+}
+
 static const struct gpio_chip stm32_gpio_template = {
 	.request		= stm32_gpio_request,
 	.free			= stm32_gpio_free,
@@ -205,6 +224,7 @@ static int stm32_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
 	.direction_input	= stm32_gpio_direction_input,
 	.direction_output	= stm32_gpio_direction_output,
 	.to_irq			= stm32_gpio_to_irq,
+	.get_direction		= stm32_gpio_get_direction,
 };
 
 static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
@@ -569,8 +589,8 @@ static void stm32_pmx_set_mode(struct stm32_gpio_bank *bank,
 	clk_disable(bank->clk);
 }
 
-static void stm32_pmx_get_mode(struct stm32_gpio_bank *bank,
-		int pin, u32 *mode, u32 *alt)
+void stm32_pmx_get_mode(struct stm32_gpio_bank *bank, int pin, u32 *mode,
+			u32 *alt)
 {
 	u32 val;
 	int alt_shift = (pin % 8) * 4;
diff --git a/drivers/pinctrl/stm32/pinctrl-stm32.h b/drivers/pinctrl/stm32/pinctrl-stm32.h
index 35ebc94..8702a99 100644
--- a/drivers/pinctrl/stm32/pinctrl-stm32.h
+++ b/drivers/pinctrl/stm32/pinctrl-stm32.h
@@ -45,7 +45,10 @@ struct stm32_pinctrl_match_data {
 	const unsigned int npins;
 };
 
-int stm32_pctl_probe(struct platform_device *pdev);
+struct stm32_gpio_bank;
 
+int stm32_pctl_probe(struct platform_device *pdev);
+void stm32_pmx_get_mode(struct stm32_gpio_bank *bank,
+			int pin, u32 *mode, u32 *alt);
 #endif /* __PINCTRL_STM32_H */
 
-- 
1.9.1

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

* [PATCH 4/4] ARM: dts: stm32: Set gpio controller also as interrupt controller
  2017-04-07 15:10 [PATCH 0/4] Add fixes to STM32 pintrl Alexandre TORGUE
                   ` (2 preceding siblings ...)
  2017-04-07 15:10 ` [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback Alexandre TORGUE
@ 2017-04-07 15:10 ` Alexandre TORGUE
  2017-04-24 12:38   ` Linus Walleij
  3 siblings, 1 reply; 11+ messages in thread
From: Alexandre TORGUE @ 2017-04-07 15:10 UTC (permalink / raw)
  To: Linus Walleij, Maxime Coquelin, Patrice Chotard, Paul Gortmaker,
	Rob Herring
  Cc: linux-kernel, linux-gpio, linux-arm-kernel, devicetree

This patch set each gpio controller as a interrupt controller. User who
wants to use gpio as interrupt will have choice to use either "gpiolib"
interface or "common" interrupt interface.

Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

diff --git a/arch/arm/boot/dts/stm32f429.dtsi b/arch/arm/boot/dts/stm32f429.dtsi
index ee0da97..12c6b70 100644
--- a/arch/arm/boot/dts/stm32f429.dtsi
+++ b/arch/arm/boot/dts/stm32f429.dtsi
@@ -555,6 +555,8 @@
 			gpioa: gpio@40020000 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x0 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOA)>;
 				st,bank-name = "GPIOA";
@@ -563,6 +565,8 @@
 			gpiob: gpio@40020400 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x400 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOB)>;
 				st,bank-name = "GPIOB";
@@ -571,6 +575,8 @@
 			gpioc: gpio@40020800 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x800 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOC)>;
 				st,bank-name = "GPIOC";
@@ -579,6 +585,8 @@
 			gpiod: gpio@40020c00 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0xc00 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOD)>;
 				st,bank-name = "GPIOD";
@@ -587,6 +595,8 @@
 			gpioe: gpio@40021000 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1000 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOE)>;
 				st,bank-name = "GPIOE";
@@ -595,6 +605,8 @@
 			gpiof: gpio@40021400 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1400 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOF)>;
 				st,bank-name = "GPIOF";
@@ -603,6 +615,8 @@
 			gpiog: gpio@40021800 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1800 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOG)>;
 				st,bank-name = "GPIOG";
@@ -611,6 +625,8 @@
 			gpioh: gpio@40021c00 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1c00 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOH)>;
 				st,bank-name = "GPIOH";
@@ -619,6 +635,8 @@
 			gpioi: gpio@40022000 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x2000 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOI)>;
 				st,bank-name = "GPIOI";
@@ -627,6 +645,8 @@
 			gpioj: gpio@40022400 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x2400 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOJ)>;
 				st,bank-name = "GPIOJ";
@@ -635,6 +655,8 @@
 			gpiok: gpio@40022800 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x2800 0x400>;
 				clocks = <&rcc 0 STM32F4_AHB1_CLOCK(GPIOK)>;
 				st,bank-name = "GPIOK";
diff --git a/arch/arm/boot/dts/stm32f746.dtsi b/arch/arm/boot/dts/stm32f746.dtsi
index f321ffe..8f2525f 100644
--- a/arch/arm/boot/dts/stm32f746.dtsi
+++ b/arch/arm/boot/dts/stm32f746.dtsi
@@ -190,6 +190,8 @@
 			gpioa: gpio@40020000 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x0 0x400>;
 				clocks = <&rcc 0 256>;
 				st,bank-name = "GPIOA";
@@ -198,6 +200,8 @@
 			gpiob: gpio@40020400 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x400 0x400>;
 				clocks = <&rcc 0 257>;
 				st,bank-name = "GPIOB";
@@ -206,6 +210,8 @@
 			gpioc: gpio@40020800 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x800 0x400>;
 				clocks = <&rcc 0 258>;
 				st,bank-name = "GPIOC";
@@ -214,6 +220,8 @@
 			gpiod: gpio@40020c00 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0xc00 0x400>;
 				clocks = <&rcc 0 259>;
 				st,bank-name = "GPIOD";
@@ -222,6 +230,8 @@
 			gpioe: gpio@40021000 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1000 0x400>;
 				clocks = <&rcc 0 260>;
 				st,bank-name = "GPIOE";
@@ -230,6 +240,8 @@
 			gpiof: gpio@40021400 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1400 0x400>;
 				clocks = <&rcc 0 261>;
 				st,bank-name = "GPIOF";
@@ -238,6 +250,8 @@
 			gpiog: gpio@40021800 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1800 0x400>;
 				clocks = <&rcc 0 262>;
 				st,bank-name = "GPIOG";
@@ -246,6 +260,8 @@
 			gpioh: gpio@40021c00 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x1c00 0x400>;
 				clocks = <&rcc 0 263>;
 				st,bank-name = "GPIOH";
@@ -254,6 +270,8 @@
 			gpioi: gpio@40022000 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x2000 0x400>;
 				clocks = <&rcc 0 264>;
 				st,bank-name = "GPIOI";
@@ -262,6 +280,8 @@
 			gpioj: gpio@40022400 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x2400 0x400>;
 				clocks = <&rcc 0 265>;
 				st,bank-name = "GPIOJ";
@@ -270,6 +290,8 @@
 			gpiok: gpio@40022800 {
 				gpio-controller;
 				#gpio-cells = <2>;
+				interrupt-controller;
+				#interrupt-cells = <2>;
 				reg = <0x2800 0x400>;
 				clocks = <&rcc 0 266>;
 				st,bank-name = "GPIOK";
-- 
1.9.1

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

* Re: [PATCH 2/4] pinctrl: stm32: replace device_initcall() with arch_initcall()
  2017-04-07 15:10 ` [PATCH 2/4] pinctrl: stm32: replace device_initcall() with arch_initcall() Alexandre TORGUE
@ 2017-04-24 12:22   ` Linus Walleij
  0 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2017-04-24 12:22 UTC (permalink / raw)
  To: Alexandre TORGUE
  Cc: Maxime Coquelin, Patrice Chotard, Paul Gortmaker, Rob Herring,
	linux-kernel, linux-gpio, linux-arm-kernel, devicetree

On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE
<alexandre.torgue@st.com> wrote:

> Pinctrl has to be registered earlier. Mainly to register bank irqdomain
> earlier as other devices could use interrupts from those irqdomain.
>
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

Yeah I also see that a lot. I guess workarounds like this is
necessary.

Patch applied.

Yours,
Linus Walleij

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

* Re: [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt
  2017-04-07 15:10 ` [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt Alexandre TORGUE
@ 2017-04-24 12:36   ` Linus Walleij
  2017-04-24 16:40     ` Alexandre Torgue
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2017-04-24 12:36 UTC (permalink / raw)
  To: Alexandre TORGUE
  Cc: Maxime Coquelin, Patrice Chotard, Paul Gortmaker, Rob Herring,
	linux-kernel, linux-gpio, linux-arm-kernel, devicetree

On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE
<alexandre.torgue@st.com> wrote:

> This patch ensures that pin is correctly set as gpio input when it is used
> as an interrupt.
>
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
(...)

> +static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
> +{
> +       struct stm32_gpio_bank *bank = irq_data->domain->host_data;
> +       u32 ret;
> +
> +       if (!gpiochip_is_requested(&bank->gpio_chip, irq_data->hwirq)) {
> +               ret = stm32_gpio_request(&bank->gpio_chip, irq_data->hwirq);
> +               if (ret)
> +                       return ret;
> +       }

This is wrong. You should only use gpiochip_lock_as_irq(), because of the
following in Documentation/gpio/driver.txt:

---------------
It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
irq_chip are orthogonal, and offering their services independent of each
other.
(...)
So always prepare the hardware and make it ready for action in respective
callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
been called first.

This orthogonality leads to ambiguities that we need to solve: if there is
competition inside the subsystem which side is using the resource (a certain
GPIO line and register for example) it needs to deny certain operations and
keep track of usage inside of the gpiolib subsystem. This is why the API
below exists.

Locking IRQ usage
-----------------
Input GPIOs can be used as IRQ signals. When this happens, a driver is requested
to mark the GPIO as being used as an IRQ:

        int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)

This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock
is released:

        void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)

When implementing an irqchip inside a GPIO driver, these two functions should
typically be called in the .startup() and .shutdown() callbacks from the
irqchip.

When using the gpiolib irqchip helpers, these callback are automatically
assigned.
--------------

It is because of easy to make errors like this that I prefer that people try
to use GPIOLIB_IRQCHIP helpers insteaf of rolling their own irqchip code.

Yours,
Linus Walleij

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

* Re: [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback
  2017-04-07 15:10 ` [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback Alexandre TORGUE
@ 2017-04-24 12:37   ` Linus Walleij
  2017-04-24 16:07     ` Alexandre Torgue
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2017-04-24 12:37 UTC (permalink / raw)
  To: Alexandre TORGUE
  Cc: Maxime Coquelin, Patrice Chotard, Paul Gortmaker, Rob Herring,
	linux-kernel, linux-gpio, linux-arm-kernel, devicetree

On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE
<alexandre.torgue@st.com> wrote:

> Add .get_direction() gpiochip callback in STM32 pinctrl driver.
>
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

(...)
> +#include <linux/gpio.h>

No this is wrong, drivers should never include this file.
It is a deprecated consumer header.

> +       if ((alt == 0) && (mode == 0))
> +               ret = GPIOF_DIR_IN;
> +       else if ((alt == 0) && (mode == 1))
> +               ret = GPIOF_DIR_OUT;

Just return 0 or 1, that is the driver-internal API.

Yours,
Linus Walleij

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

* Re: [PATCH 4/4] ARM: dts: stm32: Set gpio controller also as interrupt controller
  2017-04-07 15:10 ` [PATCH 4/4] ARM: dts: stm32: Set gpio controller also as interrupt controller Alexandre TORGUE
@ 2017-04-24 12:38   ` Linus Walleij
  0 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2017-04-24 12:38 UTC (permalink / raw)
  To: Alexandre TORGUE
  Cc: Maxime Coquelin, Patrice Chotard, Paul Gortmaker, Rob Herring,
	linux-kernel, linux-gpio, linux-arm-kernel, devicetree

On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE
<alexandre.torgue@st.com> wrote:

> This patch set each gpio controller as a interrupt controller. User who
> wants to use gpio as interrupt will have choice to use either "gpiolib"
> interface or "common" interrupt interface.
>
> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>

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

Yours,
Linus Walleij

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

* Re: [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback
  2017-04-24 12:37   ` Linus Walleij
@ 2017-04-24 16:07     ` Alexandre Torgue
  0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Torgue @ 2017-04-24 16:07 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Maxime Coquelin, Patrice Chotard, Paul Gortmaker, Rob Herring,
	linux-kernel, linux-gpio, linux-arm-kernel, devicetree

Hi Linus,

On 04/24/2017 02:37 PM, Linus Walleij wrote:
> On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE
> <alexandre.torgue@st.com> wrote:
>
>> Add .get_direction() gpiochip callback in STM32 pinctrl driver.
>>
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
>
> (...)
>> +#include <linux/gpio.h>
>
> No this is wrong, drivers should never include this file.
> It is a deprecated consumer header.
>
>> +       if ((alt == 0) && (mode == 0))
>> +               ret = GPIOF_DIR_IN;
>> +       else if ((alt == 0) && (mode == 1))
>> +               ret = GPIOF_DIR_OUT;
>
> Just return 0 or 1, that is the driver-internal API.

Ok. I will fix it in V2.

Thanks
Alex

>
> Yours,
> Linus Walleij
>

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

* Re: [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt
  2017-04-24 12:36   ` Linus Walleij
@ 2017-04-24 16:40     ` Alexandre Torgue
  0 siblings, 0 replies; 11+ messages in thread
From: Alexandre Torgue @ 2017-04-24 16:40 UTC (permalink / raw)
  To: Linus Walleij
  Cc: Maxime Coquelin, Patrice Chotard, Paul Gortmaker, Rob Herring,
	linux-kernel, linux-gpio, linux-arm-kernel, devicetree

Hi Linus,

On 04/24/2017 02:36 PM, Linus Walleij wrote:
> On Fri, Apr 7, 2017 at 5:10 PM, Alexandre TORGUE
> <alexandre.torgue@st.com> wrote:
>
>> This patch ensures that pin is correctly set as gpio input when it is used
>> as an interrupt.
>>
>> Signed-off-by: Alexandre TORGUE <alexandre.torgue@st.com>
> (...)
>
>> +static int stm32_gpio_irq_request_resources(struct irq_data *irq_data)
>> +{
>> +       struct stm32_gpio_bank *bank = irq_data->domain->host_data;
>> +       u32 ret;
>> +
>> +       if (!gpiochip_is_requested(&bank->gpio_chip, irq_data->hwirq)) {
>> +               ret = stm32_gpio_request(&bank->gpio_chip, irq_data->hwirq);
>> +               if (ret)
>> +                       return ret;
>> +       }
>
> This is wrong. You should only use gpiochip_lock_as_irq(), because of the
> following in Documentation/gpio/driver.txt:
>
> ---------------
> It is legal for any IRQ consumer to request an IRQ from any irqchip no matter
> if that is a combined GPIO+IRQ driver. The basic premise is that gpio_chip and
> irq_chip are orthogonal, and offering their services independent of each
> other.
> (...)
> So always prepare the hardware and make it ready for action in respective
> callbacks from the GPIO and irqchip APIs. Do not rely on gpiod_to_irq() having
> been called first.

Ok, so actually the action to set pin in input mode is necessary in 
stm32_gpio_irq_request_resources.

I'll fix it in V2.


>
> This orthogonality leads to ambiguities that we need to solve: if there is
> competition inside the subsystem which side is using the resource (a certain
> GPIO line and register for example) it needs to deny certain operations and
> keep track of usage inside of the gpiolib subsystem. This is why the API
> below exists.
>
> Locking IRQ usage
> -----------------
> Input GPIOs can be used as IRQ signals. When this happens, a driver is requested
> to mark the GPIO as being used as an IRQ:
>
>         int gpiochip_lock_as_irq(struct gpio_chip *chip, unsigned int offset)
>
> This will prevent the use of non-irq related GPIO APIs until the GPIO IRQ lock
> is released:
>
>         void gpiochip_unlock_as_irq(struct gpio_chip *chip, unsigned int offset)
>
> When implementing an irqchip inside a GPIO driver, these two functions should
> typically be called in the .startup() and .shutdown() callbacks from the
> irqchip.
>
> When using the gpiolib irqchip helpers, these callback are automatically
> assigned.
> --------------
>
> It is because of easy to make errors like this that I prefer that people try
> to use GPIOLIB_IRQCHIP helpers insteaf of rolling their own irqchip code.

I understand. It was difficult in our case due to design.

Thanks for review.
Alex

>
> Yours,
> Linus Walleij
>

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

end of thread, other threads:[~2017-04-24 16:41 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-07 15:10 [PATCH 0/4] Add fixes to STM32 pintrl Alexandre TORGUE
2017-04-07 15:10 ` [PATCH 1/4] pinctrl: stm32: set pin to gpio input when used as interrupt Alexandre TORGUE
2017-04-24 12:36   ` Linus Walleij
2017-04-24 16:40     ` Alexandre Torgue
2017-04-07 15:10 ` [PATCH 2/4] pinctrl: stm32: replace device_initcall() with arch_initcall() Alexandre TORGUE
2017-04-24 12:22   ` Linus Walleij
2017-04-07 15:10 ` [PATCH 3/4] pinctrl: stm32: Implement .get_direction gpio_chip callback Alexandre TORGUE
2017-04-24 12:37   ` Linus Walleij
2017-04-24 16:07     ` Alexandre Torgue
2017-04-07 15:10 ` [PATCH 4/4] ARM: dts: stm32: Set gpio controller also as interrupt controller Alexandre TORGUE
2017-04-24 12:38   ` Linus Walleij

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).