All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/3] pinctrl: lpc18xx: support pint setup
@ 2016-02-20 22:51 Joachim Eastwood
  2016-02-20 22:51 ` [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin Joachim Eastwood
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Joachim Eastwood @ 2016-02-20 22:51 UTC (permalink / raw)
  To: linus.walleij; +Cc: Joachim Eastwood, linux-gpio, devicetree

Some background here:
The LPC1850 has no less than 3 GPIO interrupt blocks. One of these
blocks is called 'gpio pin interrupt' or just PINT. LPC1850 PINT can
handle up to 8 interrupts and these have a one-to-one relationship with
the main interrupt controller (NVIC).

Selecting which GPIOs that are associated with PINT irq lines is done in
pinctrl hw block (SCU). The pinctrl device usually deals with the pin
namespace but PINT selecction is done in the GPIO namespace. Fortunatly
there is a function that can translate from the pin namespace to the
GPIO namespace.

Selection is done in DT with the "nxp,gpio-pin-interrupt" property.
Example usage;
&pinctrl {
	gpio_joystick_1_cfg {
		pins =  "p9_0";
		function = "gpio";
		nxp,gpio-pin-interrupt = <0>;
		input-enable;
		bias-disable;
	};
};

The reason for not doing this irq line selection on the fly in the
irqchip driver is that the registers lie in the SCU hw block not in the
PINT block and DT gives you more control since you can select a specific
irq lines easily.

The irq chip driver is ready but will posted separetly.

Linus; Take a careful look at patch 1 as this one deals with a pinctrl
locking issue I encountered when trying to use the pinctrl to gpio
function. I am not that familiar with pinctrl locking so please advice.


Joachim Eastwood (3):
  pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  pinctrl: lpc18xx: add nxp,gpio-pin-interrupt property
  pinctrl: lpc1850-scu: document nxp,gpio-pin-interrupt

 .../bindings/pinctrl/nxp,lpc1850-scu.txt           |  14 ++
 drivers/pinctrl/core.c                             |  25 +++-
 drivers/pinctrl/core.h                             |   4 +
 drivers/pinctrl/pinctrl-lpc18xx.c                  | 144 ++++++++++++++++++++-
 4 files changed, 174 insertions(+), 13 deletions(-)

-- 
1.8.0


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

* [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-20 22:51 [PATCH 0/3] pinctrl: lpc18xx: support pint setup Joachim Eastwood
@ 2016-02-20 22:51 ` Joachim Eastwood
  2016-02-25  9:21   ` Linus Walleij
  2016-02-20 22:51 ` [PATCH 2/3] pinctrl: lpc18xx: add nxp,gpio-pin-interrupt property Joachim Eastwood
       [not found] ` <1456008716-6236-1-git-send-email-manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2 siblings, 1 reply; 11+ messages in thread
From: Joachim Eastwood @ 2016-02-20 22:51 UTC (permalink / raw)
  To: linus.walleij; +Cc: Joachim Eastwood, linux-gpio

pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so
does pinconf_pins_show and this will cause a deadlock if
pinctrl_find_gpio_range_from_pin is used in .pin_config_get
callback.

Create an unlocked version of pinctrl_find_gpio_range_from_pin to
allow pin to gpio lookup to be used from pinconf_pins_show.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
Hi Linus,

Here is traceback I got before I added this:
[  161.370000] cat             D 281b5275     0    23      1 0x00000000
[  161.370000] [<281b5275>] (__schedule) from [<281b5479>] (schedule+0x29/0x5c)
[  161.370000] [<281b5479>] (schedule) from [<281b54bb>] (schedule_preempt_disabled+0xf/0x18)
[  161.370000] [<281b54bb>] (schedule_preempt_disabled) from [<281b6401>] (__mutex_lock_slowpath+0x45/0xf8)
[  161.370000] [<281b6401>] (__mutex_lock_slowpath) from [<280b5ddb>] (pinctrl_find_gpio_range_from_pin+0x13/0x68)
[  161.370000] [<280b5ddb>] (pinctrl_find_gpio_range_from_pin) from [<280b8da9>] (lpc18xx_pconf_get+0x95/0x204)
[  161.370000] [<280b8da9>] (lpc18xx_pconf_get) from [<280b827b>] (pinconf_generic_dump_one+0xbb/0xcc)
[  161.370000] [<280b827b>] (pinconf_generic_dump_one) from [<280b8337>] (pinconf_generic_dump_pins+0x4f/0x54)
[  161.370000] [<280b8337>] (pinconf_generic_dump_pins) from [<280b7b01>] (pinconf_pins_show+0x8d/0xc0)
[  161.370000] [<280b7b01>] (pinconf_pins_show) from [<28068fb7>] (seq_read+0x7b/0x2bc)
[  161.370000] [<28068fb7>] (seq_read) from [<280547f3>] (SyS_read+0x2b/0x68)
[  161.370000] [<280547f3>] (SyS_read) from [<28008c61>] (ret_fast_syscall+0x1/0x4a)

 drivers/pinctrl/core.c | 25 ++++++++++++++++++-------
 drivers/pinctrl/core.h |  4 ++++
 2 files changed, 22 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 2686a4450dfc..0c2cb883b18a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -487,12 +487,11 @@ EXPORT_SYMBOL_GPL(pinctrl_get_group_pins);
  * @pin: a controller-local number to find the range for
  */
 struct pinctrl_gpio_range *
-pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
-				 unsigned int pin)
+__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
+				   unsigned int pin)
 {
 	struct pinctrl_gpio_range *range;
 
-	mutex_lock(&pctldev->mutex);
 	/* Loop over the ranges */
 	list_for_each_entry(range, &pctldev->gpio_ranges, node) {
 		/* Check if we're in the valid range */
@@ -500,15 +499,27 @@ pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
 			int a;
 			for (a = 0; a < range->npins; a++) {
 				if (range->pins[a] == pin)
-					goto out;
+					return range;
 			}
 		} else if (pin >= range->pin_base &&
 			   pin < range->pin_base + range->npins)
-			goto out;
+			return range;
 	}
-	range = NULL;
-out:
+
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin);
+
+struct pinctrl_gpio_range *
+pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
+				 unsigned int pin)
+{
+	struct pinctrl_gpio_range *range;
+
+	mutex_lock(&pctldev->mutex);
+	range = __pinctrl_find_gpio_range_from_pin(pctldev, pin);
 	mutex_unlock(&pctldev->mutex);
+
 	return range;
 }
 EXPORT_SYMBOL_GPL(pinctrl_find_gpio_range_from_pin);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index b24ea846c867..99bd8428542a 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -182,6 +182,10 @@ static inline struct pin_desc *pin_desc_get(struct pinctrl_dev *pctldev,
 	return radix_tree_lookup(&pctldev->pin_desc_tree, pin);
 }
 
+extern struct pinctrl_gpio_range *
+__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
+				   unsigned int pin);
+
 int pinctrl_register_map(struct pinctrl_map const *maps, unsigned num_maps,
 			 bool dup);
 void pinctrl_unregister_map(struct pinctrl_map const *map);
-- 
1.8.0


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

* [PATCH 2/3] pinctrl: lpc18xx: add nxp,gpio-pin-interrupt property
  2016-02-20 22:51 [PATCH 0/3] pinctrl: lpc18xx: support pint setup Joachim Eastwood
  2016-02-20 22:51 ` [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin Joachim Eastwood
@ 2016-02-20 22:51 ` Joachim Eastwood
       [not found] ` <1456008716-6236-1-git-send-email-manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
  2 siblings, 0 replies; 11+ messages in thread
From: Joachim Eastwood @ 2016-02-20 22:51 UTC (permalink / raw)
  To: linus.walleij; +Cc: Joachim Eastwood, linux-gpio

Add support for setting up GPIO pin interrupts in the lpc18xx pinctrl
driver. The LPC18xx SCU contain two registers that sets up the signal
routing to the GPIO pin interrupt (PINT) block. The routing uses the
GPIO namespace and not the pin namespace so a lookup is preformed on
the pin.

Routing configuration is done in the device tree by using the new
nxp,gpio-pin-interrupt property. This property takes single parameter
which sets the PINT hwirq for the GPIO.

Signed-off-by: Joachim Eastwood <manabian@gmail.com>
---
 drivers/pinctrl/pinctrl-lpc18xx.c | 144 ++++++++++++++++++++++++++++++++++++--
 1 file changed, 138 insertions(+), 6 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-lpc18xx.c b/drivers/pinctrl/pinctrl-lpc18xx.c
index f0bebbe0682b..c95af68d5925 100644
--- a/drivers/pinctrl/pinctrl-lpc18xx.c
+++ b/drivers/pinctrl/pinctrl-lpc18xx.c
@@ -49,6 +49,19 @@
 
 #define LPC18XX_SCU_FUNC_PER_PIN	8
 
+/* LPC18XX SCU pin interrupt select registers */
+#define LPC18XX_SCU_PINTSEL0		0xe00
+#define LPC18XX_SCU_PINTSEL1		0xe04
+#define LPC18XX_SCU_PINTSEL_VAL_MASK	0xff
+#define LPC18XX_SCU_PINTSEL_PORT_SHIFT	5
+#define LPC18XX_SCU_IRQ_PER_PINTSEL	4
+#define LPC18XX_GPIO_PINS_PER_PORT	32
+#define LPC18XX_GPIO_PIN_INT_MAX	8
+
+#define LPC18XX_SCU_PINTSEL_VAL(val, n) \
+	((val) << (((n) % LPC18XX_SCU_IRQ_PER_PINTSEL) * 8))
+
+
 /* LPC18xx pin types */
 enum {
 	TYPE_ND,	/* Normal-drive */
@@ -618,6 +631,25 @@ static const struct pinctrl_pin_desc lpc18xx_pins[] = {
 	LPC18XX_PIN(i2c0_sda, PIN_I2C0_SDA),
 };
 
+/**
+ * enum lpc18xx_pin_config_param - possible pin configuration parameters
+ * @PIN_CONFIG_GPIO_PIN_INT: route gpio to the gpio pin interrupt
+ * 	controller.
+ */
+enum lpc18xx_pin_config_param {
+	PIN_CONFIG_GPIO_PIN_INT = PIN_CONFIG_END + 1,
+};
+
+static const struct pinconf_generic_params lpc18xx_params[] = {
+	{"nxp,gpio-pin-interrupt", PIN_CONFIG_GPIO_PIN_INT, 0},
+};
+
+#ifdef CONFIG_DEBUG_FS
+static const struct pin_config_item lpc18xx_conf_items[ARRAY_SIZE(lpc18xx_params)] = {
+	PCONFDUMP(PIN_CONFIG_GPIO_PIN_INT, "gpio pin int", NULL, true),
+};
+#endif
+
 static int lpc18xx_pconf_get_usb1(enum pin_config_param param, int *arg, u32 reg)
 {
 	switch (param) {
@@ -693,7 +725,71 @@ static int lpc18xx_pconf_get_i2c0(enum pin_config_param param, int *arg, u32 reg
 	return 0;
 }
 
-static int lpc18xx_pconf_get_pin(enum pin_config_param param, int *arg, u32 reg,
+static int lpc18xx_pin_to_gpio(struct pinctrl_dev *pctldev, unsigned pin)
+{
+	struct pinctrl_gpio_range *range;
+
+	range = __pinctrl_find_gpio_range_from_pin(pctldev, pin);
+	if (!range)
+		return -EINVAL;
+
+	return pin - range->pin_base + range->base;
+}
+
+static int lpc18xx_get_pintsel(void __iomem *addr, u32 val, int *arg)
+{
+	u32 reg_val;
+	int i;
+
+	reg_val = readl(addr);
+	for (i = 0; i < LPC18XX_SCU_IRQ_PER_PINTSEL; i++) {
+		if ((reg_val & LPC18XX_SCU_PINTSEL_VAL_MASK) == val)
+			return 0;
+
+		reg_val >>= BITS_PER_BYTE;
+		*arg += 1;
+	}
+
+	return -EINVAL;
+}
+
+static u32 lpc18xx_gpio_to_pintsel_val(int gpio)
+{
+	unsigned int gpio_port, gpio_pin;
+
+	gpio_port = gpio / LPC18XX_GPIO_PINS_PER_PORT;
+	gpio_pin  = gpio % LPC18XX_GPIO_PINS_PER_PORT;
+
+	return gpio_pin | (gpio_port << LPC18XX_SCU_PINTSEL_PORT_SHIFT);
+}
+
+static int lpc18xx_pconf_get_gpio_pin_int(struct pinctrl_dev *pctldev,
+					  int *arg, unsigned pin)
+{
+	struct lpc18xx_scu_data *scu = pinctrl_dev_get_drvdata(pctldev);
+	int gpio, ret;
+	u32 val;
+
+	gpio = lpc18xx_pin_to_gpio(pctldev, pin);
+	if (gpio < 0)
+		return -ENOTSUPP;
+
+	val = lpc18xx_gpio_to_pintsel_val(gpio);
+
+	/*
+	 * Check if this pin has been enabled as a interrupt in any of the two
+	 * PINTSEL registers. *arg indicates which interrupt number (0-7).
+	 */
+	*arg = 0;
+	ret = lpc18xx_get_pintsel(scu->base + LPC18XX_SCU_PINTSEL0, val, arg);
+	if (ret == 0)
+		return ret;
+
+	return lpc18xx_get_pintsel(scu->base + LPC18XX_SCU_PINTSEL1, val, arg);
+}
+
+static int lpc18xx_pconf_get_pin(struct pinctrl_dev *pctldev, unsigned param,
+				 int *arg, u32 reg, unsigned pin,
 				 struct lpc18xx_pin_caps *pin_cap)
 {
 	switch (param) {
@@ -755,6 +851,9 @@ static int lpc18xx_pconf_get_pin(enum pin_config_param param, int *arg, u32 reg,
 		}
 		break;
 
+	case PIN_CONFIG_GPIO_PIN_INT:
+		return lpc18xx_pconf_get_gpio_pin_int(pctldev, arg, pin);
+
 	default:
 		return -ENOTSUPP;
 	}
@@ -794,7 +893,7 @@ static int lpc18xx_pconf_get(struct pinctrl_dev *pctldev, unsigned pin,
 	else if (pin_cap->type == TYPE_USB1)
 		ret = lpc18xx_pconf_get_usb1(param, &arg, reg);
 	else
-		ret = lpc18xx_pconf_get_pin(param, &arg, reg, pin_cap);
+		ret = lpc18xx_pconf_get_pin(pctldev, param, &arg, reg, pin, pin_cap);
 
 	if (ret < 0)
 		return ret;
@@ -883,9 +982,34 @@ static int lpc18xx_pconf_set_i2c0(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static int lpc18xx_pconf_set_pin(struct pinctrl_dev *pctldev,
-				 enum pin_config_param param,
-				 u16 param_val, u32 *reg,
+static int lpc18xx_pconf_set_gpio_pin_int(struct pinctrl_dev *pctldev,
+					  u16 param_val, unsigned pin)
+{
+	struct lpc18xx_scu_data *scu = pinctrl_dev_get_drvdata(pctldev);
+	u32 val, reg_val, reg_offset = LPC18XX_SCU_PINTSEL0;
+	int gpio;
+
+	if (param_val >= LPC18XX_GPIO_PIN_INT_MAX)
+		return -EINVAL;
+
+	gpio = lpc18xx_pin_to_gpio(pctldev, pin);
+	if (gpio < 0)
+		return -ENOTSUPP;
+
+	val = lpc18xx_gpio_to_pintsel_val(gpio);
+
+	reg_offset += (param_val / LPC18XX_SCU_IRQ_PER_PINTSEL) * sizeof(u32);
+
+	reg_val = readl(scu->base + reg_offset);
+	reg_val &= ~LPC18XX_SCU_PINTSEL_VAL(LPC18XX_SCU_PINTSEL_VAL_MASK, param_val);
+	reg_val |= LPC18XX_SCU_PINTSEL_VAL(val, param_val);
+	writel(reg_val, scu->base + reg_offset);
+
+	return 0;
+}
+
+static int lpc18xx_pconf_set_pin(struct pinctrl_dev *pctldev, unsigned param,
+				 u16 param_val, u32 *reg, unsigned pin,
 				 struct lpc18xx_pin_caps *pin_cap)
 {
 	switch (param) {
@@ -948,6 +1072,9 @@ static int lpc18xx_pconf_set_pin(struct pinctrl_dev *pctldev,
 		*reg |= param_val << LPC18XX_SCU_PIN_EHD_POS;
 		break;
 
+	case PIN_CONFIG_GPIO_PIN_INT:
+		return lpc18xx_pconf_set_gpio_pin_int(pctldev, param_val, pin);
+
 	default:
 		dev_err(pctldev->dev, "Property not supported\n");
 		return -ENOTSUPP;
@@ -982,7 +1109,7 @@ static int lpc18xx_pconf_set(struct pinctrl_dev *pctldev, unsigned pin,
 		else if (pin_cap->type == TYPE_USB1)
 			ret = lpc18xx_pconf_set_usb1(pctldev, param, param_val, &reg);
 		else
-			ret = lpc18xx_pconf_set_pin(pctldev, param, param_val, &reg, pin_cap);
+			ret = lpc18xx_pconf_set_pin(pctldev, param, param_val, &reg, pin, pin_cap);
 
 		if (ret)
 			return ret;
@@ -1136,6 +1263,11 @@ static struct pinctrl_desc lpc18xx_scu_desc = {
 	.pctlops = &lpc18xx_pctl_ops,
 	.pmxops = &lpc18xx_pmx_ops,
 	.confops = &lpc18xx_pconf_ops,
+	.num_custom_params = ARRAY_SIZE(lpc18xx_params),
+	.custom_params = lpc18xx_params,
+#ifdef CONFIG_DEBUG_FS
+	.custom_conf_items = lpc18xx_conf_items,
+#endif
 	.owner = THIS_MODULE,
 };
 
-- 
1.8.0


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

* [PATCH 3/3] pinctrl: lpc1850-scu: document nxp,gpio-pin-interrupt
       [not found] ` <1456008716-6236-1-git-send-email-manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
@ 2016-02-20 22:51   ` Joachim Eastwood
  2016-02-22 19:11     ` Rob Herring
  0 siblings, 1 reply; 11+ messages in thread
From: Joachim Eastwood @ 2016-02-20 22:51 UTC (permalink / raw)
  To: linus.walleij-QSEj5FYQhm4dnm+yROfE0A
  Cc: Joachim Eastwood, linux-gpio-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Update devicetree documention for lpc1850-scu with the new
nxp,gpio-pin-interrupt property.

Signed-off-by: Joachim Eastwood <manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
 .../devicetree/bindings/pinctrl/nxp,lpc1850-scu.txt        | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/nxp,lpc1850-scu.txt b/Documentation/devicetree/bindings/pinctrl/nxp,lpc1850-scu.txt
index df0309c57505..bd8b0c69fa44 100644
--- a/Documentation/devicetree/bindings/pinctrl/nxp,lpc1850-scu.txt
+++ b/Documentation/devicetree/bindings/pinctrl/nxp,lpc1850-scu.txt
@@ -22,6 +22,10 @@ The following generic nodes are supported:
  - input-schmitt-disable
  - slew-rate
 
+NXP specific properties:
+ - nxp,gpio-pin-interrupt : Assign pin to gpio pin interrupt controller
+			    irq number 0 to 7. See example below.
+
 Not all pins support all properties so either refer to the NXP 1850/4350
 user manual or the pin table in the pinctrl-lpc18xx driver for supported
 pin properties.
@@ -54,4 +58,14 @@ pinctrl: pinctrl@40086000 {
 			bias-disable;
 		};
 	};
+
+	gpio_joystick_pins: gpio-joystick-pins {
+		gpio_joystick_1_cfg {
+			pins =  "p9_0";
+			function = "gpio";
+			nxp,gpio-pin-interrupt = <0>;
+			input-enable;
+			bias-disable;
+		};
+	};
 };
-- 
1.8.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

* Re: [PATCH 3/3] pinctrl: lpc1850-scu: document nxp,gpio-pin-interrupt
  2016-02-20 22:51   ` [PATCH 3/3] pinctrl: lpc1850-scu: document nxp,gpio-pin-interrupt Joachim Eastwood
@ 2016-02-22 19:11     ` Rob Herring
  0 siblings, 0 replies; 11+ messages in thread
From: Rob Herring @ 2016-02-22 19:11 UTC (permalink / raw)
  To: Joachim Eastwood; +Cc: linus.walleij, linux-gpio, devicetree

On Sat, Feb 20, 2016 at 11:51:56PM +0100, Joachim Eastwood wrote:
> Update devicetree documention for lpc1850-scu with the new
> nxp,gpio-pin-interrupt property.
> 
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> ---
>  .../devicetree/bindings/pinctrl/nxp,lpc1850-scu.txt        | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

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

* Re: [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-20 22:51 ` [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin Joachim Eastwood
@ 2016-02-25  9:21   ` Linus Walleij
  2016-02-25  9:23     ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2016-02-25  9:21 UTC (permalink / raw)
  To: Joachim Eastwood; +Cc: linux-gpio

On Sat, Feb 20, 2016 at 11:51 PM, Joachim Eastwood <manabian@gmail.com> wrote:

> pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so
> does pinconf_pins_show and this will cause a deadlock if
> pinctrl_find_gpio_range_from_pin is used in .pin_config_get
> callback.
>
> Create an unlocked version of pinctrl_find_gpio_range_from_pin to
> allow pin to gpio lookup to be used from pinconf_pins_show.
>
> Signed-off-by: Joachim Eastwood <manabian@gmail.com>

I understand that the function is needed and it's semantically
OK.

> +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin);
(...)
> +extern struct pinctrl_gpio_range *
> +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
> +                                  unsigned int pin);
> +

This function name is NOT OK.

Rename it pinctrl_fund_gpio_range_from_pin_unlocked(),
The arbitrary uses of the __-prefix is one of my biggest confusions
when trying to understand code.

Yours,
Linus Walleij

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

* Re: [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-25  9:21   ` Linus Walleij
@ 2016-02-25  9:23     ` Linus Walleij
  2016-02-25 11:19       ` Joachim Eastwood
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2016-02-25  9:23 UTC (permalink / raw)
  To: Joachim Eastwood; +Cc: linux-gpio

On Thu, Feb 25, 2016 at 10:21 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Sat, Feb 20, 2016 at 11:51 PM, Joachim Eastwood <manabian@gmail.com> wrote:
>
>> pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so
>> does pinconf_pins_show and this will cause a deadlock if
>> pinctrl_find_gpio_range_from_pin is used in .pin_config_get
>> callback.
>>
>> Create an unlocked version of pinctrl_find_gpio_range_from_pin to
>> allow pin to gpio lookup to be used from pinconf_pins_show.
>>
>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>
> I understand that the function is needed and it's semantically
> OK.
>
>> +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin);
> (...)
>> +extern struct pinctrl_gpio_range *
>> +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
>> +                                  unsigned int pin);
>> +
>
> This function name is NOT OK.
>
> Rename it pinctrl_fund_gpio_range_from_pin_unlocked(),

Or rather, pinctrl_fund_gpio_range_from_pin_locked(),
indicating that you're already holding the necessary lock
when calling the function. Now I'm even confusing myself,
sorry :(

Yours,
Linus Walleij

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

* Re: [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-25  9:23     ` Linus Walleij
@ 2016-02-25 11:19       ` Joachim Eastwood
  2016-02-25 14:55         ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Joachim Eastwood @ 2016-02-25 11:19 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-gpio

On 25 February 2016 at 10:23, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 25, 2016 at 10:21 AM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
>> On Sat, Feb 20, 2016 at 11:51 PM, Joachim Eastwood <manabian@gmail.com> wrote:
>>
>>> pinctrl_find_gpio_range_from_pin takes the pctldev->mutex but so
>>> does pinconf_pins_show and this will cause a deadlock if
>>> pinctrl_find_gpio_range_from_pin is used in .pin_config_get
>>> callback.
>>>
>>> Create an unlocked version of pinctrl_find_gpio_range_from_pin to
>>> allow pin to gpio lookup to be used from pinconf_pins_show.
>>>
>>> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
>>
>> I understand that the function is needed and it's semantically
>> OK.
>>
>>> +EXPORT_SYMBOL_GPL(__pinctrl_find_gpio_range_from_pin);
>> (...)
>>> +extern struct pinctrl_gpio_range *
>>> +__pinctrl_find_gpio_range_from_pin(struct pinctrl_dev *pctldev,
>>> +                                  unsigned int pin);
>>> +
>>
>> This function name is NOT OK.
>>
>> Rename it pinctrl_fund_gpio_range_from_pin_unlocked(),
>
> Or rather, pinctrl_fund_gpio_range_from_pin_locked(),
> indicating that you're already holding the necessary lock
> when calling the function. Now I'm even confusing myself,
> sorry :(

Shouldn't the function name indicate what the function does with the lock?

pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that
it does not acquire a lock and it is your responsibility as a caller
to ensure that the correct lock is held before calling.

But I am fine with either pinctrl_fund_gpio_range_from_pin_unlocked()
or pinctrl_fund_gpio_range_from_pin_locked(). It's all up to you.


regards,
Joachim Eastwood

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

* Re: [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-25 11:19       ` Joachim Eastwood
@ 2016-02-25 14:55         ` Linus Walleij
  2016-02-25 15:15           ` Joachim Eastwood
  0 siblings, 1 reply; 11+ messages in thread
From: Linus Walleij @ 2016-02-25 14:55 UTC (permalink / raw)
  To: Joachim Eastwood; +Cc: linux-gpio

On Thu, Feb 25, 2016 at 12:19 PM, Joachim  Eastwood <manabian@gmail.com> wrote:

>> Or rather, pinctrl_fund_gpio_range_from_pin_locked(),
>> indicating that you're already holding the necessary lock
>> when calling the function. Now I'm even confusing myself,
>> sorry :(
>
> Shouldn't the function name indicate what the function does with the lock?
>
> pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that
> it does not acquire a lock and it is your responsibility as a caller
> to ensure that the correct lock is held before calling.

OK hm maybe you're right, grep the kernel for precedents.

Yours,
Linus Walleij

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

* Re: [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-25 14:55         ` Linus Walleij
@ 2016-02-25 15:15           ` Joachim Eastwood
  2016-02-25 15:23             ` Linus Walleij
  0 siblings, 1 reply; 11+ messages in thread
From: Joachim Eastwood @ 2016-02-25 15:15 UTC (permalink / raw)
  To: Linus Walleij; +Cc: linux-gpio

On 25 February 2016 at 15:55, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Feb 25, 2016 at 12:19 PM, Joachim  Eastwood <manabian@gmail.com> wrote:
>
>>> Or rather, pinctrl_fund_gpio_range_from_pin_locked(),
>>> indicating that you're already holding the necessary lock
>>> when calling the function. Now I'm even confusing myself,
>>> sorry :(
>>
>> Shouldn't the function name indicate what the function does with the lock?
>>
>> pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that
>> it does not acquire a lock and it is your responsibility as a caller
>> to ensure that the correct lock is held before calling.
>
> OK hm maybe you're right, grep the kernel for precedents.

hmm, I not sure anymore.

What do you think about pinctrl_find_gpio_range_from_pin_nolock()?

The _nolock() prefix is also used in the kernel and might convey what
we want better. Thoughts?


regards,
Joachim Eastwood

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

* Re: [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin
  2016-02-25 15:15           ` Joachim Eastwood
@ 2016-02-25 15:23             ` Linus Walleij
  0 siblings, 0 replies; 11+ messages in thread
From: Linus Walleij @ 2016-02-25 15:23 UTC (permalink / raw)
  To: Joachim Eastwood; +Cc: linux-gpio

On Thu, Feb 25, 2016 at 4:15 PM, Joachim  Eastwood <manabian@gmail.com> wrote:
> On 25 February 2016 at 15:55, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Thu, Feb 25, 2016 at 12:19 PM, Joachim  Eastwood <manabian@gmail.com> wrote:
>>
>>>> Or rather, pinctrl_fund_gpio_range_from_pin_locked(),
>>>> indicating that you're already holding the necessary lock
>>>> when calling the function. Now I'm even confusing myself,
>>>> sorry :(
>>>
>>> Shouldn't the function name indicate what the function does with the lock?
>>>
>>> pinctrl_fund_gpio_range_from_pin_unlocked() would indicate to me that
>>> it does not acquire a lock and it is your responsibility as a caller
>>> to ensure that the correct lock is held before calling.
>>
>> OK hm maybe you're right, grep the kernel for precedents.
>
> hmm, I not sure anymore.
>
> What do you think about pinctrl_find_gpio_range_from_pin_nolock()?
>
> The _nolock() prefix is also used in the kernel and might convey what
> we want better. Thoughts?

Go for it.

Yours,
Linus Walleij

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

end of thread, other threads:[~2016-02-25 15:23 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-20 22:51 [PATCH 0/3] pinctrl: lpc18xx: support pint setup Joachim Eastwood
2016-02-20 22:51 ` [PATCH 1/3] pinctrl: core: create unlocked version of pinctrl_find_gpio_range_from_pin Joachim Eastwood
2016-02-25  9:21   ` Linus Walleij
2016-02-25  9:23     ` Linus Walleij
2016-02-25 11:19       ` Joachim Eastwood
2016-02-25 14:55         ` Linus Walleij
2016-02-25 15:15           ` Joachim Eastwood
2016-02-25 15:23             ` Linus Walleij
2016-02-20 22:51 ` [PATCH 2/3] pinctrl: lpc18xx: add nxp,gpio-pin-interrupt property Joachim Eastwood
     [not found] ` <1456008716-6236-1-git-send-email-manabian-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2016-02-20 22:51   ` [PATCH 3/3] pinctrl: lpc1850-scu: document nxp,gpio-pin-interrupt Joachim Eastwood
2016-02-22 19:11     ` Rob Herring

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.