pinctrl-msm cleanups

PATCH In review Stephen Boyd 3 participants 10 messages Mar 7, 2014 – Mar 12, 2014

Subsystems (2)

Arm Qualcomm mailing list Pin control

lore

Share & tools

lore

Stephen Boyd

author

2014-03-07 06:44 UTC #0 lore raw

cover pinctrl-msm cleanups

Here are some small fixes and cleanups for the msm pinctrl driver.

Stephen Boyd (7):
pinctrl: msm: Silence recursive lockdep warning
pinctrl: msm: Check for ngpios > MAX_NR_GPIO
pinctrl: msm: Drop unused includes
pinctrl: msm: Drop OF_IRQ dependency
pinctrl: msm: Replace lookup tables with math
pinctrl: msm: Remove impossible WARN_ON()s
pinctrl: msm: Simplify msm_config_reg() and callers

 drivers/pinctrl/Kconfig           |  2 +-
 drivers/pinctrl/pinctrl-msm.c     | 73 ++++++++++++++-------------------------
 drivers/pinctrl/pinctrl-msm.h     |  5 +--
 drivers/pinctrl/pinctrl-msm8x74.c |  1 -
 4 files changed, 27 insertions(+), 54 deletions(-)

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #1 lore raw

PATCH 1/7 pinctrl: msm: Silence recursive lockdep warning

If a driver calls enable_irq_wake() on a gpio turned interrupt from the msm pinctrl driver we'll get a lockdep warning like so:

=============================================
[ INFO: possible recursive locking detected ]
3.14.0-rc3 #2 Not tainted
---------------------------------------------
modprobe/52 is trying to acquire lock:
(&irq_desc_lock_class){-.....}, at: [<c026aea0>] __irq_get_desc_lock+0x48/0x88

but task is already holding lock:
(&irq_desc_lock_class){-.....}, at: [<c026aea0>] __irq_get_desc_lock+0x48/0x88

other info that might help us debug this:

 Possible unsafe locking scenario:

       CPU0
       ----

lock(&irq_desc_lock_class);
lock(&irq_desc_lock_class);

*** DEADLOCK ***

May be due to missing lock nesting notation

4 locks held by modprobe/52:

 #0:  (&__lockdep_no_validate__){......}, at: [<c04f2864>] __driver_attach+0x48/0x98
 #1:  (&__lockdep_no_validate__){......}, at: [<c04f2874>] __driver_attach+0x58/0x98
 #2:  (&irq_desc_lock_class){-.....}, at: [<c026aea0>] __irq_get_desc_lock+0x48/0x88
 #3:  (&(&pctrl->lock)->rlock){......}, at: [<c04bb4b8>] msm_gpio_irq_set_wake+0x20/0xa8

Silence it by putting the gpios into their own lock class.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/pinctrl-msm.c | 7 +++++++
 1 file changed, 7 insertions(+)

drivers/pinctrl/pinctrl-msm.c +7 -0

index ef2bf3126da6..ac7223dab4a3 100644
@@ -869,6 +869,12 @@ static void msm_gpio_irq_handler(unsigned int irq, struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+/*
+ * This lock class tells lockdep that GPIO irqs are in a different
+ * category than their parents, so it won't report false recursion.
+ */
+static struct lock_class_key gpio_lock_class;
+
 static int msm_gpio_init(struct msm_pinctrl *pctrl)
 {
 	struct gpio_chip *chip;

@@ -907,6 +913,7 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 
 	for (i = 0; i < chip->ngpio; i++) {
 		irq = irq_create_mapping(pctrl->domain, i);
+		irq_set_lockdep_class(irq, &gpio_lock_class);
 		irq_set_chip_and_handler(irq, &msm_gpio_irq_chip, handle_edge_irq);
 		irq_set_chip_data(irq, pctrl);
 	}

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #2 lore raw

PATCH 2/7 pinctrl: msm: Check for ngpios > MAX_NR_GPIO

In reply to Stephen Boyd

Fail the probe and print a warning if SoC specific drivers have more GPIOs than there can be accounted for in the static bitmaps. This should avoid silent corruption/failures in the future.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/pinctrl-msm.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

drivers/pinctrl/pinctrl-msm.c +5 -1

index ac7223dab4a3..19b0de89dc07 100644
@@ -882,10 +882,14 @@ static int msm_gpio_init(struct msm_pinctrl *pctrl)
 	int ret;
 	int i;
 	int r;
+	unsigned ngpio = pctrl->soc->ngpios;
+
+	if (WARN_ON(ngpio > MAX_NR_GPIO))
+		return -EINVAL;
 
 	chip = &pctrl->chip;
 	chip->base = 0;
-	chip->ngpio = pctrl->soc->ngpios;
+	chip->ngpio = ngpio;
 	chip->label = dev_name(pctrl->dev);
 	chip->dev = pctrl->dev;
 	chip->owner = THIS_MODULE;

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #3 lore raw

PATCH 3/7 pinctrl: msm: Drop unused includes

In reply to Stephen Boyd

These includes are unused or can be handled via forward declarations. Remove them.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/pinctrl-msm.c     | 1 -
 drivers/pinctrl/pinctrl-msm.h     | 5 +----
 drivers/pinctrl/pinctrl-msm8x74.c | 1 -
 3 files changed, 1 insertion(+), 6 deletions(-)

drivers/pinctrl/pinctrl-msm.c +0 -1

index 19b0de89dc07..5daad720ecbb 100644
@@ -28,7 +28,6 @@
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqchip/chained_irq.h>
-#include <linux/of_irq.h>
 #include <linux/spinlock.h>
 
 #include "core.h"

drivers/pinctrl/pinctrl-msm.h +1 -4

index 206e782e2daa..8fbe9fb19f36 100644
@@ -13,10 +13,7 @@
 #ifndef __PINCTRL_MSM_H__
 #define __PINCTRL_MSM_H__
 
-#include <linux/pinctrl/pinctrl.h>
-#include <linux/pinctrl/pinmux.h>
-#include <linux/pinctrl/pinconf.h>
-#include <linux/pinctrl/machine.h>
+struct pinctrl_pin_desc;
 
 /**
  * struct msm_function - a pinmux function

drivers/pinctrl/pinctrl-msm8x74.c +0 -1

index f944bf2172ef..bb5ded69f963 100644
@@ -15,7 +15,6 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pinctrl/pinctrl.h>
-#include <linux/pinctrl/pinmux.h>
 
 #include "pinctrl-msm.h"

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #4 lore raw

PATCH 4/7 pinctrl: msm: Drop OF_IRQ dependency

In reply to Stephen Boyd

This driver doesn't rely on any functionality living in drivers/of/irq.c to compile. Drop this dependency.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/pinctrl/Kconfig +1 -1

index be361b7cd30f..32b6b49f28ec 100644
@@ -224,7 +224,7 @@ config PINCTRL_MSM
 
 config PINCTRL_MSM8X74
 	tristate "Qualcomm 8x74 pin controller driver"
-	depends on GPIOLIB && OF && OF_IRQ
+	depends on GPIOLIB && OF
 	select PINCTRL_MSM
 	help
 	  This is the pinctrl, pinmux, pinconf and gpiolib driver for the

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #5 lore raw

PATCH 5/7 pinctrl: msm: Replace lookup tables with math

In reply to Stephen Boyd

We don't need to waste space with these lookup tables, just do the math directly.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/pinctrl-msm.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

drivers/pinctrl/pinctrl-msm.c +8 -6

index 5daad720ecbb..58c23f138d7a 100644
@@ -260,8 +260,10 @@ static int msm_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 #define MSM_PULL_DOWN	1
 #define MSM_PULL_UP	3
 
-static const unsigned msm_regval_to_drive[] = { 2, 4, 6, 8, 10, 12, 14, 16 };
-static const unsigned msm_drive_to_regval[] = { -1, -1, 0, -1, 1, -1, 2, -1, 3, -1, 4, -1, 5, -1, 6, -1, 7 };
+static unsigned msm_regval_to_drive(u32 val)
+{
+	return (val + 1) * 2;
+}
 
 static int msm_config_group_get(struct pinctrl_dev *pctldev,
 				unsigned int group,

@@ -298,7 +300,7 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
 		arg = arg == MSM_PULL_UP;
 		break;
 	case PIN_CONFIG_DRIVE_STRENGTH:
-		arg = msm_regval_to_drive[arg];
+		arg = msm_regval_to_drive(arg);
 		break;
 	default:
 		dev_err(pctrl->dev, "Unsupported config parameter: %x\n",

@@ -351,10 +353,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 			break;
 		case PIN_CONFIG_DRIVE_STRENGTH:
 			/* Check for invalid values */
-			if (arg >= ARRAY_SIZE(msm_drive_to_regval))
+			if (arg > 16 || arg < 2 || (arg % 2) != 0)
 				arg = -1;
 			else
-				arg = msm_drive_to_regval[arg];
+				arg = (arg / 2) - 1;
 			break;
 		default:
 			dev_err(pctrl->dev, "Unsupported config parameter: %x\n",

@@ -533,7 +535,7 @@ static void msm_gpio_dbg_show_one(struct seq_file *s,
 	pull = (ctl_reg >> g->pull_bit) & 3;
 
 	seq_printf(s, " %-8s: %-3s %d", g->name, is_out ? "out" : "in", func);
-	seq_printf(s, " %dmA", msm_regval_to_drive[drive]);
+	seq_printf(s, " %dmA", msm_regval_to_drive(drive));
 	seq_printf(s, " %s", pulls[pull]);
 }

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #6 lore raw

PATCH 6/7 pinctrl: msm: Remove impossible WARN_ON()s

In reply to Stephen Boyd

All these functions are limited in what they can pass as the gpio or irq number to whatever is setup during probe. Remove the checks.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/pinctrl-msm.c | 16 ----------------
 1 file changed, 16 deletions(-)

drivers/pinctrl/pinctrl-msm.c +0 ~~-16~~

index 58c23f138d7a..bdfb697194f6 100644
@@ -403,8 +403,6 @@ static int msm_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 	u32 val;
 
 	g = &pctrl->soc->groups[offset];
-	if (WARN_ON(g->io_reg < 0))
-		return -EINVAL;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

@@ -425,8 +423,6 @@ static int msm_gpio_direction_output(struct gpio_chip *chip, unsigned offset, in
 	u32 val;
 
 	g = &pctrl->soc->groups[offset];
-	if (WARN_ON(g->io_reg < 0))
-		return -EINVAL;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

@@ -453,8 +449,6 @@ static int msm_gpio_get(struct gpio_chip *chip, unsigned offset)
 	u32 val;
 
 	g = &pctrl->soc->groups[offset];
-	if (WARN_ON(g->io_reg < 0))
-		return -EINVAL;
 
 	val = readl(pctrl->regs + g->io_reg);
 	return !!(val & BIT(g->in_bit));

@@ -468,8 +462,6 @@ static void msm_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 	u32 val;
 
 	g = &pctrl->soc->groups[offset];
-	if (WARN_ON(g->io_reg < 0))
-		return;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

@@ -618,8 +610,6 @@ static void msm_gpio_irq_mask(struct irq_data *d)
 
 	pctrl = irq_data_get_irq_chip_data(d);
 	g = &pctrl->soc->groups[d->hwirq];
-	if (WARN_ON(g->intr_cfg_reg < 0))
-		return;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

@@ -641,8 +631,6 @@ static void msm_gpio_irq_unmask(struct irq_data *d)
 
 	pctrl = irq_data_get_irq_chip_data(d);
 	g = &pctrl->soc->groups[d->hwirq];
-	if (WARN_ON(g->intr_status_reg < 0))
-		return;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

@@ -668,8 +656,6 @@ static void msm_gpio_irq_ack(struct irq_data *d)
 
 	pctrl = irq_data_get_irq_chip_data(d);
 	g = &pctrl->soc->groups[d->hwirq];
-	if (WARN_ON(g->intr_status_reg < 0))
-		return;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

@@ -694,8 +680,6 @@ static int msm_gpio_irq_set_type(struct irq_data *d, unsigned int type)
 
 	pctrl = irq_data_get_irq_chip_data(d);
 	g = &pctrl->soc->groups[d->hwirq];
-	if (WARN_ON(g->intr_cfg_reg < 0))
-		return -EINVAL;
 
 	spin_lock_irqsave(&pctrl->lock, flags);

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Stephen Boyd

author

2014-03-07 06:44 UTC #7 lore raw

PATCH 7/7 pinctrl: msm: Simplify msm_config_reg() and callers

In reply to Stephen Boyd

We don't need to check for a negative reg here because reg is always the same and is always non-negative. Also, collapse the switch statement down for the duplicate cases.

Cc Bjorn Andersson <[email protected]>

Signed-off-by Stephen Boyd <[email protected]>

---
 drivers/pinctrl/pinctrl-msm.c | 29 +++++------------------------
 1 file changed, 5 insertions(+), 24 deletions(-)

drivers/pinctrl/pinctrl-msm.c +5 ~~-24~~

index bdfb697194f6..534dd117dab4 100644
@@ -202,28 +202,17 @@ static const struct pinmux_ops msm_pinmux_ops = {
 static int msm_config_reg(struct msm_pinctrl *pctrl,
 			  const struct msm_pingroup *g,
 			  unsigned param,
-			  s16 *reg,
 			  unsigned *mask,
 			  unsigned *bit)
 {
 	switch (param) {
 	case PIN_CONFIG_BIAS_DISABLE:
-		*reg = g->ctl_reg;
-		*bit = g->pull_bit;
-		*mask = 3;
-		break;
 	case PIN_CONFIG_BIAS_PULL_DOWN:
-		*reg = g->ctl_reg;
-		*bit = g->pull_bit;
-		*mask = 3;
-		break;
 	case PIN_CONFIG_BIAS_PULL_UP:
-		*reg = g->ctl_reg;
 		*bit = g->pull_bit;
 		*mask = 3;
 		break;
 	case PIN_CONFIG_DRIVE_STRENGTH:
-		*reg = g->ctl_reg;
 		*bit = g->drv_bit;
 		*mask = 7;
 		break;

@@ -232,12 +221,6 @@ static int msm_config_reg(struct msm_pinctrl *pctrl,
 		return -ENOTSUPP;
 	}
 
-	if (*reg < 0) {
-		dev_err(pctrl->dev, "Config param %04x not supported on group %s\n",
-			param, g->name);
-		return -ENOTSUPP;
-	}
-
 	return 0;
 }

@@ -275,17 +258,16 @@ static int msm_config_group_get(struct pinctrl_dev *pctldev,
 	unsigned mask;
 	unsigned arg;
 	unsigned bit;
-	s16 reg;
 	int ret;
 	u32 val;
 
 	g = &pctrl->soc->groups[group];
 
-	ret = msm_config_reg(pctrl, g, param, &reg, &mask, &bit);
+	ret = msm_config_reg(pctrl, g, param, &mask, &bit);
 	if (ret < 0)
 		return ret;
 
-	val = readl(pctrl->regs + reg);
+	val = readl(pctrl->regs + g->ctl_reg);
 	arg = (val >> bit) & mask;
 
 	/* Convert register value to pinconf value */

@@ -325,7 +307,6 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 	unsigned mask;
 	unsigned arg;
 	unsigned bit;
-	s16 reg;
 	int ret;
 	u32 val;
 	int i;

@@ -336,7 +317,7 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 		param = pinconf_to_config_param(configs[i]);
 		arg = pinconf_to_config_argument(configs[i]);
 
-		ret = msm_config_reg(pctrl, g, param, &reg, &mask, &bit);
+		ret = msm_config_reg(pctrl, g, param, &mask, &bit);
 		if (ret < 0)
 			return ret;

@@ -371,10 +352,10 @@ static int msm_config_group_set(struct pinctrl_dev *pctldev,
 		}
 
 		spin_lock_irqsave(&pctrl->lock, flags);
-		val = readl(pctrl->regs + reg);
+		val = readl(pctrl->regs + g->ctl_reg);
 		val &= ~(mask << bit);
 		val |= arg << bit;
-		writel(val, pctrl->regs + reg);
+		writel(val, pctrl->regs + g->ctl_reg);
 		spin_unlock_irqrestore(&pctrl->lock, flags);
 	}

Signature · 2 lines

The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Bjorn Andersson

Acked-by on the v1 series

2014-03-07 16:05 UTC #8 lore raw

In reply to Stephen Boyd

Quoted 12 lines · Stephen Boyd

Here are some small fixes and cleanups for the msm pinctrl driver.
Stephen Boyd (7):
pinctrl: msm: Silence recursive lockdep warning
pinctrl: msm: Check for ngpios > MAX_NR_GPIO
pinctrl: msm: Drop unused includes
pinctrl: msm: Drop OF_IRQ dependency
pinctrl: msm: Replace lookup tables with math
pinctrl: msm: Remove impossible WARN_ON()s
pinctrl: msm: Simplify msm_config_reg() and callers

Quoted 5 lines

 drivers/pinctrl/Kconfig           |  2 +-
 drivers/pinctrl/pinctrl-msm.c     | 73 ++++++++++++++-------------------------
 drivers/pinctrl/pinctrl-msm.h     |  5 +--
 drivers/pinctrl/pinctrl-msm8x74.c |  1 -
 4 files changed, 27 insertions(+), 54 deletions(-)

Looks good

Acked-by Bjorn Andersson <[email protected]>

Linus Walleij

2014-03-12 14:10 UTC #9 lore raw

In reply to Stephen Boyd

Quoted 2 lines · Stephen Boyd

Here are some small fixes and cleanups for the msm pinctrl driver.

Okay, applied all 7 patches with Bj?rn's ACK.

I had to use some fuzzing on patch 5 so please check the end result in linux-next.

Yours,
Linus Walleij