[PATCH v2 01/13] pinctrl: intel: Disable input and output buffer when switching to GPIO

[PATCH v2 01/13] pinctrl: intel: Disable input and output buffer when switching to GPIO

[Andy Shevchenko]

It's possible scenario that pin has been in different mode, while
the respective GPIO register has a leftover output buffer enabled.
In such case when we request GPIO it will switch to GPIO mode, and
thus to output with unknown value, followed by switching to input
mode. This can produce a glitch on the pin.

Disable input and output buffer when switching to GPIO to avoid
potential glitches.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 6a274e20d926..9df5a0c0d416 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -435,11 +435,20 @@ static void intel_gpio_set_gpio_mode(void __iomem *padcfg0)
 {
 	u32 value;
 
+	value = readl(padcfg0);
+
 	/* Put the pad into GPIO mode */
-	value = readl(padcfg0) & ~PADCFG0_PMODE_MASK;
+	value &= ~PADCFG0_PMODE_MASK;
+	value |= PADCFG0_PMODE_GPIO;
+
+	/* Disable input and output buffers */
+	value &= ~PADCFG0_GPIORXDIS;
+	value &= ~PADCFG0_GPIOTXDIS;
+
 	/* Disable SCI/SMI/NMI generation */
 	value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
 	value &= ~(PADCFG0_GPIROUTSMI | PADCFG0_GPIROUTNMI);
+
 	writel(value, padcfg0);
 }
 
@@ -1036,6 +1045,9 @@ static int intel_gpio_irq_type(struct irq_data *d, unsigned int type)
 
 	intel_gpio_set_gpio_mode(reg);
 
+	/* Disable TX buffer and enable RX (this will be input) */
+	__intel_gpio_set_direction(reg, true);
+
 	value = readl(reg);
 
 	value &= ~(PADCFG0_RXEVCFG_MASK | PADCFG0_RXINV);
-- 
2.27.0.rc2

[PATCH v2 02/13] pinctrl: intel: Reduce scope of the lock

[Andy Shevchenko]

In some cases lock covers unneeded calls and operations.
Reduce scope of the lock in such cases.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 9df5a0c0d416..d0b658ba2136 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -460,6 +460,8 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 	void __iomem *padcfg0;
 	unsigned long flags;
 
+	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
+
 	raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 	if (!intel_pad_owned_by_host(pctrl, pin)) {
@@ -472,8 +474,6 @@ static int intel_gpio_request_enable(struct pinctrl_dev *pctldev,
 		return 0;
 	}
 
-	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
-
 	/*
 	 * If pin is already configured in GPIO mode, we assume that
 	 * firmware provides correct settings. In such case we avoid
@@ -503,11 +503,10 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
 	void __iomem *padcfg0;
 	unsigned long flags;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
-
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
-	__intel_gpio_set_direction(padcfg0, input);
 
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+	__intel_gpio_set_direction(padcfg0, input);
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	return 0;
@@ -622,10 +621,11 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 	int ret = 0;
 	u32 value;
 
-	raw_spin_lock_irqsave(&pctrl->lock, flags);
-
 	community = intel_get_community(pctrl, pin);
 	padcfg1 = intel_get_padcfg(pctrl, pin, PADCFG1);
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
+
 	value = readl(padcfg1);
 
 	switch (param) {
-- 
2.27.0.rc2

[PATCH v2 03/13] pinctrl: intel: Make use of IRQ_RETVAL()

[Andy Shevchenko]

Instead of using bitwise operations against returned values,
which is a bit fragile, convert IRQ handler to count amount of
GPIO groups, where at least one interrupt happened, and convert
it to returned value by IRQ_RETVAL() macro.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index d0b658ba2136..e05273a00ff2 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1093,12 +1093,12 @@ static int intel_gpio_irq_wake(struct irq_data *d, unsigned int on)
 	return 0;
 }
 
-static irqreturn_t intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
-	const struct intel_community *community)
+static int intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
+					    const struct intel_community *community)
 {
 	struct gpio_chip *gc = &pctrl->chip;
-	irqreturn_t ret = IRQ_NONE;
-	int gpp;
+	unsigned int gpp;
+	int ret = 0;
 
 	for (gpp = 0; gpp < community->ngpps; gpp++) {
 		const struct intel_padgroup *padgrp = &community->gpps[gpp];
@@ -1118,9 +1118,9 @@ static irqreturn_t intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
 			irq = irq_find_mapping(gc->irq.domain,
 					       padgrp->gpio_base + gpp_offset);
 			generic_handle_irq(irq);
-
-			ret |= IRQ_HANDLED;
 		}
+
+		ret += pending ? 1 : 0;
 	}
 
 	return ret;
@@ -1130,16 +1130,16 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
 {
 	const struct intel_community *community;
 	struct intel_pinctrl *pctrl = data;
-	irqreturn_t ret = IRQ_NONE;
-	int i;
+	unsigned int i;
+	int ret = 0;
 
 	/* Need to check all communities for pending interrupts */
 	for (i = 0; i < pctrl->ncommunities; i++) {
 		community = &pctrl->communities[i];
-		ret |= intel_gpio_community_irq_handler(pctrl, community);
+		ret += intel_gpio_community_irq_handler(pctrl, community);
 	}
 
-	return ret;
+	return IRQ_RETVAL(ret);
 }
 
 static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
-- 
2.27.0.rc2

[PATCH v2 04/13] pinctrl: intel: Get rid of redundant 'else' in intel_config_set_debounce()

[Andy Shevchenko]

In a code like
	if (...) {
		...
		goto label;
	} else {
		...
	}
the 'else' keyword is redundant. Get rid of it for better readability.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e05273a00ff2..76b1b899a389 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -719,12 +719,12 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 		if (v < 3 || v > 15) {
 			ret = -EINVAL;
 			goto exit_unlock;
-		} else {
-			/* Enable glitch filter and debouncer */
-			value0 |= PADCFG0_PREGFRXSEL;
-			value2 |= v << PADCFG2_DEBOUNCE_SHIFT;
-			value2 |= PADCFG2_DEBEN;
 		}
+
+		/* Enable glitch filter and debouncer */
+		value0 |= PADCFG0_PREGFRXSEL;
+		value2 |= v << PADCFG2_DEBOUNCE_SHIFT;
+		value2 |= PADCFG2_DEBEN;
 	}
 
 	writel(value0, padcfg0);
-- 
2.27.0.rc2

[PATCH v2 05/13] pinctrl: intel: Drop the only label in the code for consistency

[Andy Shevchenko]

Drop the only label in the code, i.e. in intel_config_set_debounce(),
for consistency with the rest. In entire driver we use multipoint
return.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 76b1b899a389..2bcda48ea29a 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -695,7 +695,6 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 	void __iomem *padcfg0, *padcfg2;
 	unsigned long flags;
 	u32 value0, value2;
-	int ret = 0;
 
 	padcfg2 = intel_get_padcfg(pctrl, pin, PADCFG2);
 	if (!padcfg2)
@@ -717,8 +716,8 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 
 		v = order_base_2(debounce * NSEC_PER_USEC / DEBOUNCE_PERIOD_NSEC);
 		if (v < 3 || v > 15) {
-			ret = -EINVAL;
-			goto exit_unlock;
+			raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+			return -EINVAL;
 		}
 
 		/* Enable glitch filter and debouncer */
@@ -730,10 +729,9 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 	writel(value0, padcfg0);
 	writel(value2, padcfg2);
 
-exit_unlock:
 	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
-	return ret;
+	return 0;
 }
 
 static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
-- 
2.27.0.rc2

[PATCH v2 06/13] pinctrl: intel: Split intel_config_get() to three functions

[Andy Shevchenko]

Split intel_config_get() to three functions, i.e. intel_config_get() and
two helpers intel_config_get_pull() and intel_config_get_debounce() to be
symmetrical with intel_config_set*().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 89 ++++++++++++++++++---------
 1 file changed, 61 insertions(+), 28 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 2bcda48ea29a..d6ef012f2cc4 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -521,20 +521,17 @@ static const struct pinmux_ops intel_pinmux_ops = {
 	.gpio_set_direction = intel_gpio_set_direction,
 };
 
-static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
-			    unsigned long *config)
+static int intel_config_get_pull(struct intel_pinctrl *pctrl, unsigned int pin,
+				 enum pin_config_param param, u32 *arg)
 {
-	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	enum pin_config_param param = pinconf_to_config_param(*config);
 	const struct intel_community *community;
+	void __iomem *padcfg1;
 	u32 value, term;
-	u32 arg = 0;
-
-	if (!intel_pad_owned_by_host(pctrl, pin))
-		return -ENOTSUPP;
 
 	community = intel_get_community(pctrl, pin);
-	value = readl(intel_get_padcfg(pctrl, pin, PADCFG1));
+	padcfg1 = intel_get_padcfg(pctrl, pin, PADCFG1);
+	value = readl(padcfg1);
+
 	term = (value & PADCFG1_TERM_MASK) >> PADCFG1_TERM_SHIFT;
 
 	switch (param) {
@@ -549,16 +546,16 @@ static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 
 		switch (term) {
 		case PADCFG1_TERM_1K:
-			arg = 1000;
+			*arg = 1000;
 			break;
 		case PADCFG1_TERM_2K:
-			arg = 2000;
+			*arg = 2000;
 			break;
 		case PADCFG1_TERM_5K:
-			arg = 5000;
+			*arg = 5000;
 			break;
 		case PADCFG1_TERM_20K:
-			arg = 20000;
+			*arg = 20000;
 			break;
 		}
 
@@ -572,35 +569,71 @@ static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 		case PADCFG1_TERM_1K:
 			if (!(community->features & PINCTRL_FEATURE_1K_PD))
 				return -EINVAL;
-			arg = 1000;
+			*arg = 1000;
 			break;
 		case PADCFG1_TERM_5K:
-			arg = 5000;
+			*arg = 5000;
 			break;
 		case PADCFG1_TERM_20K:
-			arg = 20000;
+			*arg = 20000;
 			break;
 		}
 
 		break;
 
-	case PIN_CONFIG_INPUT_DEBOUNCE: {
-		void __iomem *padcfg2;
-		u32 v;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
 
-		padcfg2 = intel_get_padcfg(pctrl, pin, PADCFG2);
-		if (!padcfg2)
-			return -ENOTSUPP;
+static int intel_config_get_debounce(struct intel_pinctrl *pctrl, unsigned int pin,
+				     enum pin_config_param param, u32 *arg)
+{
+	void __iomem *padcfg2;
+	unsigned long v;
+	u32 value2;
 
-		v = readl(padcfg2);
-		if (!(v & PADCFG2_DEBEN))
-			return -EINVAL;
+	padcfg2 = intel_get_padcfg(pctrl, pin, PADCFG2);
+	if (!padcfg2)
+		return -ENOTSUPP;
+
+	value2 = readl(padcfg2);
+	if (!(value2 & PADCFG2_DEBEN))
+		return -EINVAL;
+
+	v = (value2 & PADCFG2_DEBOUNCE_MASK) >> PADCFG2_DEBOUNCE_SHIFT;
+	*arg = BIT(v) * DEBOUNCE_PERIOD_NSEC / NSEC_PER_USEC;
+
+	return 0;
+}
+
+static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
+			    unsigned long *config)
+{
+	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
+	enum pin_config_param param = pinconf_to_config_param(*config);
+	u32 arg = 0;
+	int ret;
 
-		v = (v & PADCFG2_DEBOUNCE_MASK) >> PADCFG2_DEBOUNCE_SHIFT;
-		arg = BIT(v) * DEBOUNCE_PERIOD_NSEC / NSEC_PER_USEC;
+	if (!intel_pad_owned_by_host(pctrl, pin))
+		return -ENOTSUPP;
 
+	switch (param) {
+	case PIN_CONFIG_BIAS_DISABLE:
+	case PIN_CONFIG_BIAS_PULL_UP:
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		ret = intel_config_get_pull(pctrl, pin, param, &arg);
+		if (ret)
+			return ret;
+		break;
+
+	case PIN_CONFIG_INPUT_DEBOUNCE:
+		ret = intel_config_get_debounce(pctrl, pin, param, &arg);
+		if (ret)
+			return ret;
 		break;
-	}
 
 	default:
 		return -ENOTSUPP;
-- 
2.27.0.rc2

[PATCH v2 07/13] pinctrl: intel: Protect IO in few call backs by lock

[Andy Shevchenko]

Protect IO in intel_gpio_get_direction(), intel_gpio_community_irq_handler(),
intel_config_get_debounce() and intel_config_get_pull() by lock. Even for
simple readl() we better serialize IO to avoid potential problems.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index d6ef012f2cc4..35c88fcb75a2 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -526,11 +526,15 @@ static int intel_config_get_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 {
 	const struct intel_community *community;
 	void __iomem *padcfg1;
+	unsigned long flags;
 	u32 value, term;
 
 	community = intel_get_community(pctrl, pin);
 	padcfg1 = intel_get_padcfg(pctrl, pin, PADCFG1);
+
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 	value = readl(padcfg1);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 
 	term = (value & PADCFG1_TERM_MASK) >> PADCFG1_TERM_SHIFT;
 
@@ -592,6 +596,7 @@ static int intel_config_get_debounce(struct intel_pinctrl *pctrl, unsigned int p
 				     enum pin_config_param param, u32 *arg)
 {
 	void __iomem *padcfg2;
+	unsigned long flags;
 	unsigned long v;
 	u32 value2;
 
@@ -599,7 +604,9 @@ static int intel_config_get_debounce(struct intel_pinctrl *pctrl, unsigned int p
 	if (!padcfg2)
 		return -ENOTSUPP;
 
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 	value2 = readl(padcfg2);
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 	if (!(value2 & PADCFG2_DEBEN))
 		return -EINVAL;
 
@@ -934,6 +941,7 @@ static void intel_gpio_set(struct gpio_chip *chip, unsigned int offset,
 static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
+	unsigned long flags;
 	void __iomem *reg;
 	u32 padcfg0;
 	int pin;
@@ -946,8 +954,9 @@ static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 	if (!reg)
 		return -EINVAL;
 
+	raw_spin_lock_irqsave(&pctrl->lock, flags);
 	padcfg0 = readl(reg);
-
+	raw_spin_unlock_irqrestore(&pctrl->lock, flags);
 	if (padcfg0 & PADCFG0_PMODE_MASK)
 		return -EINVAL;
 
@@ -1134,12 +1143,17 @@ static int intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
 	for (gpp = 0; gpp < community->ngpps; gpp++) {
 		const struct intel_padgroup *padgrp = &community->gpps[gpp];
 		unsigned long pending, enabled, gpp_offset;
+		unsigned long flags;
+
+		raw_spin_lock_irqsave(&pctrl->lock, flags);
 
 		pending = readl(community->regs + community->is_offset +
 				padgrp->reg_num * 4);
 		enabled = readl(community->regs + community->ie_offset +
 				padgrp->reg_num * 4);
 
+		raw_spin_unlock_irqrestore(&pctrl->lock, flags);
+
 		/* Only interrupts that are enabled */
 		pending &= enabled;
 
-- 
2.27.0.rc2

[PATCH v2 08/13] pinctrl: intel: Introduce for_each_requested_gpio() macro

[Andy Shevchenko]

Introduce for_each_requested_gpio() macro which helps to iterate
over requested GPIO in a range.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.h | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index 4e17308d33e9..c1f312bc28eb 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -249,4 +249,8 @@ const struct dev_pm_ops _name = {					\
 				      intel_pinctrl_resume_noirq)	\
 }
 
+#define for_each_requested_gpio(chip, i, base, size)			\
+	for (i = 0; i < size; i++)					\
+		if (!gpiochip_is_requested(chip, base + i)) {} else
+
 #endif /* PINCTRL_INTEL_H */
-- 
2.27.0.rc2

Re: [PATCH v2 08/13] pinctrl: intel: Introduce for_each_requested_gpio() macro

[Mika Westerberg]

On Fri, Jun 12, 2020 at 05:50:01PM +0300, Andy Shevchenko wrote:
> Introduce for_each_requested_gpio() macro which helps to iterate
> over requested GPIO in a range.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.h | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
> index 4e17308d33e9..c1f312bc28eb 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.h
> +++ b/drivers/pinctrl/intel/pinctrl-intel.h
> @@ -249,4 +249,8 @@ const struct dev_pm_ops _name = {					\
>  				      intel_pinctrl_resume_noirq)	\
>  }
>  

kernel-doc would be good to have here.

> +#define for_each_requested_gpio(chip, i, base, size)			\
> +	for (i = 0; i < size; i++)					\
> +		if (!gpiochip_is_requested(chip, base + i)) {} else
> +
>  #endif /* PINCTRL_INTEL_H */
> -- 
> 2.27.0.rc2

Re: [PATCH v2 08/13] pinctrl: intel: Introduce for_each_requested_gpio() macro

[Andy Shevchenko]

On Mon, Jun 15, 2020 at 02:59:27PM +0300, Mika Westerberg wrote:
> On Fri, Jun 12, 2020 at 05:50:01PM +0300, Andy Shevchenko wrote:
> > Introduce for_each_requested_gpio() macro which helps to iterate
> > over requested GPIO in a range.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> >  drivers/pinctrl/intel/pinctrl-intel.h | 4 ++++
> >  1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
> > index 4e17308d33e9..c1f312bc28eb 100644
> > --- a/drivers/pinctrl/intel/pinctrl-intel.h
> > +++ b/drivers/pinctrl/intel/pinctrl-intel.h
> > @@ -249,4 +249,8 @@ const struct dev_pm_ops _name = {					\
> >  				      intel_pinctrl_resume_noirq)	\
> >  }
> >  
> 
> kernel-doc would be good to have here.

Okay, actually I considered to have this in the gpio/driver.h or so.

> > +#define for_each_requested_gpio(chip, i, base, size)			\
> > +	for (i = 0; i < size; i++)					\
> > +		if (!gpiochip_is_requested(chip, base + i)) {} else
> > +
> >  #endif /* PINCTRL_INTEL_H */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/13] pinctrl: intel: Introduce for_each_requested_gpio() macro

[Linus Walleij]

On Mon, Jun 15, 2020 at 2:01 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
> On Mon, Jun 15, 2020 at 02:59:27PM +0300, Mika Westerberg wrote:
> > On Fri, Jun 12, 2020 at 05:50:01PM +0300, Andy Shevchenko wrote:
> > > Introduce for_each_requested_gpio() macro which helps to iterate
> > > over requested GPIO in a range.
> > >
> > > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > > ---
> > >  drivers/pinctrl/intel/pinctrl-intel.h | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
> > > index 4e17308d33e9..c1f312bc28eb 100644
> > > --- a/drivers/pinctrl/intel/pinctrl-intel.h
> > > +++ b/drivers/pinctrl/intel/pinctrl-intel.h
> > > @@ -249,4 +249,8 @@ const struct dev_pm_ops _name = {                                       \
> > >                                   intel_pinctrl_resume_noirq)       \
> > >  }
> > >
> >
> > kernel-doc would be good to have here.
>
> Okay, actually I considered to have this in the gpio/driver.h or so.

Please put it there! It looks generally useful.

Yours,
Linus Walleij

[PATCH v2 09/13] pinctrl: intel: Make use of for_each_requested_gpio()

[Andy Shevchenko]

Make use of for_each_requested_gpio() instead of home grown analogue.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 35c88fcb75a2..74f0157c99fb 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1628,19 +1628,6 @@ static void intel_gpio_irq_init(struct intel_pinctrl *pctrl)
 	}
 }
 
-static u32
-intel_gpio_is_requested(struct gpio_chip *chip, int base, unsigned int size)
-{
-	u32 requested = 0;
-	unsigned int i;
-
-	for (i = 0; i < size; i++)
-		if (gpiochip_is_requested(chip, base + i))
-			requested |= BIT(i);
-
-	return requested;
-}
-
 static bool intel_gpio_update_reg(void __iomem *reg, u32 mask, u32 value)
 {
 	u32 curr, updated;
@@ -1661,12 +1648,15 @@ static void intel_restore_hostown(struct intel_pinctrl *pctrl, unsigned int c,
 	const struct intel_community *community = &pctrl->communities[c];
 	const struct intel_padgroup *padgrp = &community->gpps[gpp];
 	struct device *dev = pctrl->dev;
-	u32 requested;
+	u32 requested = 0;
+	unsigned int i;
 
 	if (padgrp->gpio_base == INTEL_GPIO_BASE_NOMAP)
 		return;
 
-	requested = intel_gpio_is_requested(&pctrl->chip, padgrp->gpio_base, padgrp->size);
+	for_each_requested_gpio(&pctrl->chip, i, padgrp->gpio_base, padgrp->size)
+		requested |= BIT(i);
+
 	if (!intel_gpio_update_reg(base + gpp * 4, requested, saved))
 		return;
 
-- 
2.27.0.rc2

[PATCH v2 10/13] pinctrl: lynxpoint: Make use of for_each_requested_gpio()

[Andy Shevchenko]

Make use of for_each_requested_gpio() instead of home grown analogue.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-lynxpoint.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-lynxpoint.c b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
index a45b8f2182fd..36cc45c013fb 100644
--- a/drivers/pinctrl/intel/pinctrl-lynxpoint.c
+++ b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
@@ -919,16 +919,16 @@ static int lp_gpio_runtime_resume(struct device *dev)
 static int lp_gpio_resume(struct device *dev)
 {
 	struct intel_pinctrl *lg = dev_get_drvdata(dev);
+	struct gpio_chip *chip = &lg->chip;
 	void __iomem *reg;
 	int i;
 
 	/* on some hardware suspend clears input sensing, re-enable it here */
-	for (i = 0; i < lg->chip.ngpio; i++) {
-		if (gpiochip_is_requested(&lg->chip, i) != NULL) {
-			reg = lp_gpio_reg(&lg->chip, i, LP_CONFIG2);
-			iowrite32(ioread32(reg) & ~GPINDIS_BIT, reg);
-		}
+	for_each_requested_gpio(chip, i, 0, chip->ngpio) {
+		reg = lp_gpio_reg(chip, i, LP_CONFIG2);
+		iowrite32(ioread32(reg) & ~GPINDIS_BIT, reg);
 	}
+
 	return 0;
 }
 
-- 
2.27.0.rc2

[PATCH v2 11/13] pinctrl: lynxpoint: Introduce helpers to enable or disable input

[Andy Shevchenko]

Introduce couple of helpers to enable or disable input. i.e.
lp_gpio_enable_input() and lp_gpio_disable_input().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-lynxpoint.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-lynxpoint.c b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
index 36cc45c013fb..6f1641833665 100644
--- a/drivers/pinctrl/intel/pinctrl-lynxpoint.c
+++ b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
@@ -386,6 +386,16 @@ static int lp_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+static void lp_gpio_enable_input(void __iomem *reg)
+{
+	iowrite32(ioread32(reg) & ~GPINDIS_BIT, reg);
+}
+
+static void lp_gpio_disable_input(void __iomem *reg)
+{
+	iowrite32(ioread32(reg) | GPINDIS_BIT, reg);
+}
+
 static int lp_gpio_request_enable(struct pinctrl_dev *pctldev,
 				  struct pinctrl_gpio_range *range,
 				  unsigned int pin)
@@ -411,7 +421,7 @@ static int lp_gpio_request_enable(struct pinctrl_dev *pctldev,
 	}
 
 	/* Enable input sensing */
-	iowrite32(ioread32(conf2) & ~GPINDIS_BIT, conf2);
+	lp_gpio_enable_input(conf2);
 
 	raw_spin_unlock_irqrestore(&lg->lock, flags);
 
@@ -429,7 +439,7 @@ static void lp_gpio_disable_free(struct pinctrl_dev *pctldev,
 	raw_spin_lock_irqsave(&lg->lock, flags);
 
 	/* Disable input sensing */
-	iowrite32(ioread32(conf2) | GPINDIS_BIT, conf2);
+	lp_gpio_disable_input(conf2);
 
 	raw_spin_unlock_irqrestore(&lg->lock, flags);
 
@@ -920,14 +930,11 @@ static int lp_gpio_resume(struct device *dev)
 {
 	struct intel_pinctrl *lg = dev_get_drvdata(dev);
 	struct gpio_chip *chip = &lg->chip;
-	void __iomem *reg;
 	int i;
 
 	/* on some hardware suspend clears input sensing, re-enable it here */
-	for_each_requested_gpio(chip, i, 0, chip->ngpio) {
-		reg = lp_gpio_reg(chip, i, LP_CONFIG2);
-		iowrite32(ioread32(reg) & ~GPINDIS_BIT, reg);
-	}
+	for_each_requested_gpio(chip, i, 0, chip->ngpio)
+		lp_gpio_enable_input(lp_gpio_reg(&lg->chip, i, LP_CONFIG2));
 
 	return 0;
 }
-- 
2.27.0.rc2

[PATCH v2 12/13] pinctrl: lynxpoint: Drop no-op ACPI_PTR() call

[Andy Shevchenko]

Since we dependent on ACPI, there is no need to use ACPI_PTR()
which is a no-op in this case.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-lynxpoint.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-lynxpoint.c b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
index 6f1641833665..53c6da52fa2a 100644
--- a/drivers/pinctrl/intel/pinctrl-lynxpoint.c
+++ b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
@@ -958,7 +958,7 @@ static struct platform_driver lp_gpio_driver = {
 	.driver         = {
 		.name   = "lp_gpio",
 		.pm	= &lp_gpio_pm_ops,
-		.acpi_match_table = ACPI_PTR(lynxpoint_gpio_acpi_match),
+		.acpi_match_table = lynxpoint_gpio_acpi_match,
 	},
 };
 
-- 
2.27.0.rc2

[PATCH v2 13/13] pinctrl: baytrail: Drop no-op ACPI_PTR() call

[Andy Shevchenko]

Since we dependent on ACPI, there is no need to use ACPI_PTR()
which is a no-op in this case.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 0ff7c55173da..e3ceb3dfeabe 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -1757,9 +1757,8 @@ static struct platform_driver byt_gpio_driver = {
 	.driver         = {
 		.name			= "byt_gpio",
 		.pm			= &byt_gpio_pm_ops,
+		.acpi_match_table	= byt_gpio_acpi_match,
 		.suppress_bind_attrs	= true,
-
-		.acpi_match_table = ACPI_PTR(byt_gpio_acpi_match),
 	},
 };
 
-- 
2.27.0.rc2

Re: [PATCH v2 01/13] pinctrl: intel: Disable input and output buffer when switching to GPIO

[Mika Westerberg]

On Fri, Jun 12, 2020 at 05:49:54PM +0300, Andy Shevchenko wrote:
> It's possible scenario that pin has been in different mode, while
> the respective GPIO register has a leftover output buffer enabled.
> In such case when we request GPIO it will switch to GPIO mode, and
> thus to output with unknown value, followed by switching to input
> mode. This can produce a glitch on the pin.
> 
> Disable input and output buffer when switching to GPIO to avoid
> potential glitches.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

There was a minor comment on one patch but other than that this looks
good.

For the whole series,

Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH v2 01/13] pinctrl: intel: Disable input and output buffer when switching to GPIO

[Andy Shevchenko]

On Mon, Jun 15, 2020 at 03:01:03PM +0300, Mika Westerberg wrote:
> On Fri, Jun 12, 2020 at 05:49:54PM +0300, Andy Shevchenko wrote:
> > It's possible scenario that pin has been in different mode, while
> > the respective GPIO register has a leftover output buffer enabled.
> > In such case when we request GPIO it will switch to GPIO mode, and
> > thus to output with unknown value, followed by switching to input
> > mode. This can produce a glitch on the pin.
> > 
> > Disable input and output buffer when switching to GPIO to avoid
> > potential glitches.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> There was a minor comment on one patch but other than that this looks
> good.

I will apply patches to my review and testing queue soon, but I will reconsider
few of them (for_each_requested_gpio() helper and its use) for v2.

> For the whole series,
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!

-- 
With Best Regards,
Andy Shevchenko