[PATCH v2 0/6] pinctrl: intel: High impedance impl. and cleanups

[PATCH v2 0/6] pinctrl: intel: High impedance impl. and cleanups

[Andy Shevchenko]

We would need a high impedance implementation for a quirk, so here it
is. While doing this series I also noticed a couple of opportunities
to clean up, hence a few more patches (1st, 5th, and 6th).

Series has been tested on Intel Meteor Lake-P.

v2:
- fixed a bug in patch 1 when applying debounce value
- updated enum style (Mika)
- made intel_gpio_set_high_impedance return void (Mika)
- new patch to constify intel_get_community()
- resplit "absolutely grotesque" macro to four (Mika)
- ...and update more users, this shrinks binary a lot

Andy Shevchenko (6):
  pinctrl: intel: Move debounce validation out of the lock
  pinctrl: intel: Refactor __intel_gpio_set_direction() to be more
    useful
  pinctrl: intel: Add __intel_gpio_get_direction() helper
  pinctrl: intel: Implement high impedance support
  pinctrl: intel: Constify intel_get_community() returned object
  pinctrl: intel: Introduce for_each_intel_gpio_group() helper et al.

 drivers/pinctrl/intel/pinctrl-baytrail.c  |   3 +-
 drivers/pinctrl/intel/pinctrl-intel.c     | 280 +++++++++++++---------
 drivers/pinctrl/intel/pinctrl-intel.h     |   2 +-
 drivers/pinctrl/intel/pinctrl-lynxpoint.c |   2 +-
 4 files changed, 170 insertions(+), 117 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 1/6] pinctrl: intel: Move debounce validation out of the lock

[Andy Shevchenko]

There is no need to validate debounce value under the lock.
Move it outside.

It also results in a smaller binary:

  add/remove: 0/0 grow/shrink: 0/1 up/down: 0/-11 (-11)
  Total: Before=15374, After=15363, chg -0.07%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 89bd7ce6711a..959413c8f4c9 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -758,6 +758,15 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 {
 	void __iomem *padcfg0, *padcfg2;
 	u32 value0, value2;
+	unsigned long v;
+
+	if (debounce) {
+		v = order_base_2(debounce * NSEC_PER_USEC / DEBOUNCE_PERIOD_NSEC);
+		if (v < 3 || v > 15)
+			return -EINVAL;
+	} else {
+		v = 0;
+	}
 
 	padcfg2 = intel_get_padcfg(pctrl, pin, PADCFG2);
 	if (!padcfg2)
@@ -770,21 +779,15 @@ static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 	value0 = readl(padcfg0);
 	value2 = readl(padcfg2);
 
-	/* Disable glitch filter and debouncer */
-	value0 &= ~PADCFG0_PREGFRXSEL;
-	value2 &= ~(PADCFG2_DEBEN | PADCFG2_DEBOUNCE_MASK);
-
-	if (debounce) {
-		unsigned long v;
-
-		v = order_base_2(debounce * NSEC_PER_USEC / DEBOUNCE_PERIOD_NSEC);
-		if (v < 3 || v > 15)
-			return -EINVAL;
-
+	value2 = (value2 & ~PADCFG2_DEBOUNCE_MASK) | (v << PADCFG2_DEBOUNCE_SHIFT);
+	if (v) {
 		/* Enable glitch filter and debouncer */
 		value0 |= PADCFG0_PREGFRXSEL;
-		value2 |= v << PADCFG2_DEBOUNCE_SHIFT;
 		value2 |= PADCFG2_DEBEN;
+	} else {
+		/* Disable glitch filter and debouncer */
+		value0 &= ~PADCFG0_PREGFRXSEL;
+		value2 &= ~PADCFG2_DEBEN;
 	}
 
 	writel(value0, padcfg0);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 2/6] pinctrl: intel: Refactor __intel_gpio_set_direction() to be more useful

[Andy Shevchenko]

Refactor __intel_gpio_set_direction() to be more useful, i.e.
1) use one parameter per each direction to support all combinatios;
2) move IO to the only user that needs it.

With that done, update current users and deduplicate existing code.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 959413c8f4c9..b9f3d94a6256 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -429,19 +429,19 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
-static void __intel_gpio_set_direction(void __iomem *padcfg0, bool input)
+static u32 __intel_gpio_set_direction(u32 value, bool input, bool output)
 {
-	u32 value;
-
-	value = readl(padcfg0);
-	if (input) {
+	if (input)
 		value &= ~PADCFG0_GPIORXDIS;
-		value |= PADCFG0_GPIOTXDIS;
-	} else {
-		value &= ~PADCFG0_GPIOTXDIS;
+	else
 		value |= PADCFG0_GPIORXDIS;
-	}
-	writel(value, padcfg0);
+
+	if (output)
+		value &= ~PADCFG0_GPIOTXDIS;
+	else
+		value |= PADCFG0_GPIOTXDIS;
+
+	return value;
 }
 
 static int __intel_gpio_get_gpio_mode(u32 value)
@@ -465,8 +465,7 @@ static void intel_gpio_set_gpio_mode(void __iomem *padcfg0)
 	value |= PADCFG0_PMODE_GPIO;
 
 	/* Disable TX buffer and enable RX (this will be input) */
-	value &= ~PADCFG0_GPIORXDIS;
-	value |= PADCFG0_GPIOTXDIS;
+	value = __intel_gpio_set_direction(value, true, false);
 
 	/* Disable SCI/SMI/NMI generation */
 	value &= ~(PADCFG0_GPIROUTIOXAPIC | PADCFG0_GPIROUTSCI);
@@ -512,12 +511,18 @@ static int intel_gpio_set_direction(struct pinctrl_dev *pctldev,
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	void __iomem *padcfg0;
+	u32 value;
 
 	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
 
 	guard(raw_spinlock_irqsave)(&pctrl->lock);
 
-	__intel_gpio_set_direction(padcfg0, input);
+	value = readl(padcfg0);
+	if (input)
+		value = __intel_gpio_set_direction(value, true, false);
+	else
+		value = __intel_gpio_set_direction(value, false, true);
+	writel(value, padcfg0);
 
 	return 0;
 }
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 3/6] pinctrl: intel: Add __intel_gpio_get_direction() helper

[Andy Shevchenko]

Add __intel_gpio_get_direction() helper which provides all possible
physical states of the pad.

With that done, update current users and make the respective checks
consistent.

While at it, make the style of anonymous enum kernel documentation
consistent.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index b9f3d94a6256..c6013d967fa6 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -70,6 +70,12 @@
 #define PADCFG0_PMODE_SHIFT		10
 #define PADCFG0_PMODE_MASK		GENMASK(13, 10)
 #define PADCFG0_PMODE_GPIO		0
+#define PADCFG0_GPIODIS_SHIFT		8
+#define PADCFG0_GPIODIS_MASK		GENMASK(9, 8)
+#define PADCFG0_GPIODIS_NONE		0
+#define PADCFG0_GPIODIS_OUTPUT		1
+#define PADCFG0_GPIODIS_INPUT		2
+#define PADCFG0_GPIODIS_FULL		3
 #define PADCFG0_GPIORXDIS		BIT(9)
 #define PADCFG0_GPIOTXDIS		BIT(8)
 #define PADCFG0_GPIORXSTATE		BIT(1)
@@ -212,7 +218,6 @@ static bool intel_pad_acpi_mode(struct intel_pinctrl *pctrl, unsigned int pin)
 
 /**
  * enum - Locking variants of the pad configuration
- *
  * @PAD_UNLOCKED:	pad is fully controlled by the configuration registers
  * @PAD_LOCKED:		pad configuration registers, except TX state, are locked
  * @PAD_LOCKED_TX:	pad configuration TX state is locked
@@ -429,6 +434,36 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+/**
+ * enum - Possible pad physical connections
+ * @PAD_CONNECT_NONE:	pad is fully disconnected
+ * @PAD_CONNECT_INPUT:	pad is in input only mode
+ * @PAD_CONNECT_OUTPUT:	pad is in output only mode
+ * @PAD_CONNECT_FULL:	pad is fully connected
+ */
+enum {
+	PAD_CONNECT_NONE	= 0,
+	PAD_CONNECT_INPUT	= 1,
+	PAD_CONNECT_OUTPUT	= 2,
+	PAD_CONNECT_FULL	= PAD_CONNECT_INPUT | PAD_CONNECT_OUTPUT,
+};
+
+static int __intel_gpio_get_direction(u32 value)
+{
+	switch ((value & PADCFG0_GPIODIS_MASK) >> PADCFG0_GPIODIS_SHIFT) {
+	case PADCFG0_GPIODIS_FULL:
+		return PAD_CONNECT_NONE;
+	case PADCFG0_GPIODIS_OUTPUT:
+		return PAD_CONNECT_INPUT;
+	case PADCFG0_GPIODIS_INPUT:
+		return PAD_CONNECT_OUTPUT;
+	case PADCFG0_GPIODIS_NONE:
+		return PAD_CONNECT_FULL;
+	default:
+		return -ENOTSUPP;
+	};
+}
+
 static u32 __intel_gpio_set_direction(u32 value, bool input, bool output)
 {
 	if (input)
@@ -937,7 +972,7 @@ static int intel_gpio_get(struct gpio_chip *chip, unsigned int offset)
 		return -EINVAL;
 
 	padcfg0 = readl(reg);
-	if (!(padcfg0 & PADCFG0_GPIOTXDIS))
+	if (__intel_gpio_get_direction(padcfg0) & PAD_CONNECT_OUTPUT)
 		return !!(padcfg0 & PADCFG0_GPIOTXSTATE);
 
 	return !!(padcfg0 & PADCFG0_GPIORXSTATE);
@@ -990,10 +1025,10 @@ static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
 	if (padcfg0 & PADCFG0_PMODE_MASK)
 		return -EINVAL;
 
-	if (padcfg0 & PADCFG0_GPIOTXDIS)
-		return GPIO_LINE_DIRECTION_IN;
+	if (__intel_gpio_get_direction(padcfg0) & PAD_CONNECT_OUTPUT)
+		return GPIO_LINE_DIRECTION_OUT;
 
-	return GPIO_LINE_DIRECTION_OUT;
+	return GPIO_LINE_DIRECTION_IN;
 }
 
 static int intel_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
@@ -1690,7 +1725,8 @@ EXPORT_SYMBOL_NS_GPL(intel_pinctrl_get_soc_data, PINCTRL_INTEL);
 
 static bool __intel_gpio_is_direct_irq(u32 value)
 {
-	return (value & PADCFG0_GPIROUTIOXAPIC) && (value & PADCFG0_GPIOTXDIS) &&
+	return (value & PADCFG0_GPIROUTIOXAPIC) &&
+	       (__intel_gpio_get_direction(value) == PAD_CONNECT_INPUT) &&
 	       (__intel_gpio_get_gpio_mode(value) == PADCFG0_PMODE_GPIO);
 }
 
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 4/6] pinctrl: intel: Implement high impedance support

[Andy Shevchenko]

Implement high impedance support for Intel pin control hardware.
It allows to set high impedance and check it.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index c6013d967fa6..46530ee6e92d 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -652,6 +652,23 @@ static int intel_config_get_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 	return 0;
 }
 
+static int intel_config_get_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin,
+					   enum pin_config_param param, u32 *arg)
+{
+	void __iomem *padcfg0;
+	u32 value;
+
+	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
+
+	scoped_guard(raw_spinlock_irqsave, &pctrl->lock)
+		value = readl(padcfg0);
+
+	if (__intel_gpio_get_direction(value) != PAD_CONNECT_NONE)
+		return -EINVAL;
+
+	return 0;
+}
+
 static int intel_config_get_debounce(struct intel_pinctrl *pctrl, unsigned int pin,
 				     enum pin_config_param param, u32 *arg)
 {
@@ -695,6 +712,12 @@ static int intel_config_get(struct pinctrl_dev *pctldev, unsigned int pin,
 			return ret;
 		break;
 
+	case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+		ret = intel_config_get_high_impedance(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)
@@ -793,6 +816,20 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 	return 0;
 }
 
+static void intel_gpio_set_high_impedance(struct intel_pinctrl *pctrl, unsigned int pin)
+{
+	void __iomem *padcfg0;
+	u32 value;
+
+	padcfg0 = intel_get_padcfg(pctrl, pin, PADCFG0);
+
+	guard(raw_spinlock_irqsave)(&pctrl->lock);
+
+	value = readl(padcfg0);
+	value = __intel_gpio_set_direction(value, false, false);
+	writel(value, padcfg0);
+}
+
 static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 				     unsigned int pin, unsigned int debounce)
 {
@@ -855,6 +892,10 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 				return ret;
 			break;
 
+		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+			intel_gpio_set_high_impedance(pctrl, pin);
+			break;
+
 		case PIN_CONFIG_INPUT_DEBOUNCE:
 			ret = intel_config_set_debounce(pctrl, pin,
 				pinconf_to_config_argument(configs[i]));
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 5/6] pinctrl: intel: Constify intel_get_community() returned object

[Andy Shevchenko]

There is nothing prevents us from constifying intel_get_community()
returned object. Do it to make code more robust.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/pinctrl/intel/pinctrl-baytrail.c  | 3 ++-
 drivers/pinctrl/intel/pinctrl-intel.c     | 8 ++++----
 drivers/pinctrl/intel/pinctrl-intel.h     | 2 +-
 drivers/pinctrl/intel/pinctrl-lynxpoint.c | 2 +-
 4 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-baytrail.c b/drivers/pinctrl/intel/pinctrl-baytrail.c
index 4e87f5b875c0..d39e666a62df 100644
--- a/drivers/pinctrl/intel/pinctrl-baytrail.c
+++ b/drivers/pinctrl/intel/pinctrl-baytrail.c
@@ -560,9 +560,10 @@ static DEFINE_RAW_SPINLOCK(byt_lock);
 static void __iomem *byt_gpio_reg(struct intel_pinctrl *vg, unsigned int offset,
 				  int reg)
 {
-	struct intel_community *comm = intel_get_community(vg, offset);
+	const struct intel_community *comm;
 	u32 reg_offset;
 
+	comm = intel_get_community(vg, offset);
 	if (!comm)
 		return NULL;
 
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 46530ee6e92d..e8dbaf3964dc 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -114,9 +114,9 @@ struct intel_community_context {
 #define pin_to_padno(c, p)	((p) - (c)->pin_base)
 #define padgroup_offset(g, p)	((p) - (g)->base)
 
-struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin)
+const struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin)
 {
-	struct intel_community *community;
+	const struct intel_community *community;
 	int i;
 
 	for (i = 0; i < pctrl->ncommunities; i++) {
@@ -236,7 +236,7 @@ enum {
 
 static int intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
 {
-	struct intel_community *community;
+	const struct intel_community *community;
 	const struct intel_padgroup *padgrp;
 	unsigned int offset, gpp_offset;
 	u32 value;
@@ -1368,7 +1368,7 @@ static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
 	int ret, i;
 
 	for (i = 0; i < pctrl->ncommunities; i++) {
-		struct intel_community *community = &pctrl->communities[i];
+		const struct intel_community *community = &pctrl->communities[i];
 
 		ret = intel_gpio_add_community_ranges(pctrl, community);
 		if (ret) {
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index 6981e2fab93f..c7b025ea989a 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -264,7 +264,7 @@ int intel_pinctrl_probe_by_uid(struct platform_device *pdev);
 
 extern const struct dev_pm_ops intel_pinctrl_pm_ops;
 
-struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin);
+const struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin);
 
 int intel_get_groups_count(struct pinctrl_dev *pctldev);
 const char *intel_get_group_name(struct pinctrl_dev *pctldev, unsigned int group);
diff --git a/drivers/pinctrl/intel/pinctrl-lynxpoint.c b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
index 1fb0bba8b386..bcce97f3b897 100644
--- a/drivers/pinctrl/intel/pinctrl-lynxpoint.c
+++ b/drivers/pinctrl/intel/pinctrl-lynxpoint.c
@@ -211,7 +211,7 @@ static void __iomem *lp_gpio_reg(struct gpio_chip *chip, unsigned int offset,
 				 int reg)
 {
 	struct intel_pinctrl *lg = gpiochip_get_data(chip);
-	struct intel_community *comm;
+	const struct intel_community *comm;
 	int reg_offset;
 
 	comm = intel_get_community(lg, offset);
-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v2 6/6] pinctrl: intel: Introduce for_each_intel_gpio_group() helper et al.

[Andy Shevchenko]

Introduce a helper macro for_each_intel_gpio_group() et al.
With those in place, update the users.

It reduces the C code base as well as shrinks the binary:

  add/remove: 0/0 grow/shrink: 4/21 up/down: 39/-621 (-582)
  Total: Before=15942, After=15360, chg -3.65%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index e8dbaf3964dc..75201d5c52a1 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -114,13 +114,16 @@ struct intel_community_context {
 #define pin_to_padno(c, p)	((p) - (c)->pin_base)
 #define padgroup_offset(g, p)	((p) - (g)->base)
 
+#define for_each_intel_pin_community(pctrl, community)					\
+	for (unsigned int __ci = 0;							\
+	     __ci < pctrl->ncommunities && (community = &pctrl->communities[__ci]);	\
+	     __ci++)									\
+
 const struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin)
 {
 	const struct intel_community *community;
-	int i;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		community = &pctrl->communities[i];
+	for_each_intel_pin_community(pctrl, community) {
 		if (pin >= community->pin_base &&
 		    pin < community->pin_base + community->npins)
 			return community;
@@ -131,15 +134,18 @@ const struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, u
 }
 EXPORT_SYMBOL_NS_GPL(intel_get_community, PINCTRL_INTEL);
 
+#define for_each_intel_community_pad_group(community, grp)			\
+	for (unsigned int __gi = 0;						\
+	     __gi < community->ngpps && (grp = &community->gpps[__gi]);		\
+	     __gi++)								\
+
 static const struct intel_padgroup *
 intel_community_get_padgroup(const struct intel_community *community,
 			     unsigned int pin)
 {
-	int i;
-
-	for (i = 0; i < community->ngpps; i++) {
-		const struct intel_padgroup *padgrp = &community->gpps[i];
+	const struct intel_padgroup *padgrp;
 
+	for_each_intel_community_pad_group(community, padgrp) {
 		if (pin >= padgrp->base && pin < padgrp->base + padgrp->size)
 			return padgrp;
 	}
@@ -924,6 +930,14 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
+#define for_each_intel_pad_group(pctrl, community, grp)			\
+	for_each_intel_pin_community(pctrl, community)			\
+		for_each_intel_community_pad_group(community, grp)
+
+#define for_each_intel_gpio_group(pctrl, community, grp)		\
+	for_each_intel_pad_group(pctrl, community, grp)			\
+		if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else
+
 /**
  * intel_gpio_to_pin() - Translate from GPIO offset to pin number
  * @pctrl: Pinctrl structure
@@ -942,30 +956,17 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset,
 			     const struct intel_community **community,
 			     const struct intel_padgroup **padgrp)
 {
-	int i;
+	const struct intel_community *comm;
+	const struct intel_padgroup *grp;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		const struct intel_community *comm = &pctrl->communities[i];
-		int j;
+	for_each_intel_gpio_group(pctrl, comm, grp) {
+		if (offset >= grp->gpio_base && offset < grp->gpio_base + grp->size) {
+			if (community)
+				*community = comm;
+			if (padgrp)
+				*padgrp = grp;
 
-		for (j = 0; j < comm->ngpps; j++) {
-			const struct intel_padgroup *pgrp = &comm->gpps[j];
-
-			if (pgrp->gpio_base == INTEL_GPIO_BASE_NOMAP)
-				continue;
-
-			if (offset >= pgrp->gpio_base &&
-			    offset < pgrp->gpio_base + pgrp->size) {
-				int pin;
-
-				pin = pgrp->base + offset - pgrp->gpio_base;
-				if (community)
-					*community = comm;
-				if (padgrp)
-					*padgrp = pgrp;
-
-				return pin;
-			}
+			return grp->base + offset - grp->gpio_base;
 		}
 	}
 
@@ -1258,12 +1259,11 @@ static const struct irq_chip intel_gpio_irq_chip = {
 static int intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
 					    const struct intel_community *community)
 {
+	const struct intel_padgroup *padgrp;
 	struct gpio_chip *gc = &pctrl->chip;
-	unsigned int gpp;
 	int ret = 0;
 
-	for (gpp = 0; gpp < community->ngpps; gpp++) {
-		const struct intel_padgroup *padgrp = &community->gpps[gpp];
+	for_each_intel_community_pad_group(community, padgrp) {
 		unsigned long pending, enabled;
 		unsigned int gpp, gpp_offset;
 		void __iomem *reg, *is;
@@ -1294,29 +1294,23 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
 {
 	const struct intel_community *community;
 	struct intel_pinctrl *pctrl = data;
-	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];
+	for_each_intel_pin_community(pctrl, community)
 		ret += intel_gpio_community_irq_handler(pctrl, community);
-	}
 
 	return IRQ_RETVAL(ret);
 }
 
 static void intel_gpio_irq_init(struct intel_pinctrl *pctrl)
 {
-	int i;
+	const struct intel_community *community;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		const struct intel_community *community;
+	for_each_intel_pin_community(pctrl, community) {
 		void __iomem *reg, *is;
 		unsigned int gpp;
 
-		community = &pctrl->communities[i];
-
 		for (gpp = 0; gpp < community->ngpps; gpp++) {
 			reg = community->regs + community->ie_offset + gpp * 4;
 			is = community->regs + community->is_offset + gpp * 4;
@@ -1341,36 +1335,17 @@ static int intel_gpio_irq_init_hw(struct gpio_chip *gc)
 	return 0;
 }
 
-static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
-				const struct intel_community *community)
-{
-	int ret = 0, i;
-
-	for (i = 0; i < community->ngpps; i++) {
-		const struct intel_padgroup *gpp = &community->gpps[i];
-
-		if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
-			continue;
-
-		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
-					     gpp->gpio_base, gpp->base,
-					     gpp->size);
-		if (ret)
-			return ret;
-	}
-
-	return ret;
-}
-
 static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
-	int ret, i;
+	const struct intel_community *community;
+	const struct intel_padgroup *grp;
+	int ret;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		const struct intel_community *community = &pctrl->communities[i];
-
-		ret = intel_gpio_add_community_ranges(pctrl, community);
+	for_each_intel_gpio_group(pctrl, community, grp) {
+		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
+					     grp->gpio_base, grp->base,
+					     grp->size);
 		if (ret) {
 			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
 			return ret;
@@ -1383,20 +1358,12 @@ static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
 static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
 {
 	const struct intel_community *community;
+	const struct intel_padgroup *grp;
 	unsigned int ngpio = 0;
-	int i, j;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		community = &pctrl->communities[i];
-		for (j = 0; j < community->ngpps; j++) {
-			const struct intel_padgroup *gpp = &community->gpps[j];
-
-			if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
-				continue;
-
-			if (gpp->gpio_base + gpp->size > ngpio)
-				ngpio = gpp->gpio_base + gpp->size;
-		}
+	for_each_intel_gpio_group(pctrl, community, grp) {
+		if (grp->gpio_base + grp->size > ngpio)
+			ngpio = grp->gpio_base + grp->size;
 	}
 
 	return ngpio;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 6/6] pinctrl: intel: Introduce for_each_intel_gpio_group() helper et al.

[Mika Westerberg]

On Thu, Aug 29, 2024 at 04:59:20PM +0300, Andy Shevchenko wrote:
> Introduce a helper macro for_each_intel_gpio_group() et al.
> With those in place, update the users.
> 
> It reduces the C code base as well as shrinks the binary:
> 
>   add/remove: 0/0 grow/shrink: 4/21 up/down: 39/-621 (-582)
>   Total: Before=15942, After=15360, chg -3.65%
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 125 ++++++++++----------------
>  1 file changed, 46 insertions(+), 79 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index e8dbaf3964dc..75201d5c52a1 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -114,13 +114,16 @@ struct intel_community_context {
>  #define pin_to_padno(c, p)	((p) - (c)->pin_base)
>  #define padgroup_offset(g, p)	((p) - (g)->base)
>  
> +#define for_each_intel_pin_community(pctrl, community)					\
> +	for (unsigned int __ci = 0;							\
> +	     __ci < pctrl->ncommunities && (community = &pctrl->communities[__ci]);	\
> +	     __ci++)									\
> +

These look more readable, thanks. Just one comment. Can you move all
them here at the top of the file so all the variants are close to each
other? You can do that while applying.

>  const struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, unsigned int pin)
>  {
>  	const struct intel_community *community;
> -	int i;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		community = &pctrl->communities[i];
> +	for_each_intel_pin_community(pctrl, community) {
>  		if (pin >= community->pin_base &&
>  		    pin < community->pin_base + community->npins)
>  			return community;
> @@ -131,15 +134,18 @@ const struct intel_community *intel_get_community(struct intel_pinctrl *pctrl, u
>  }
>  EXPORT_SYMBOL_NS_GPL(intel_get_community, PINCTRL_INTEL);
>  
> +#define for_each_intel_community_pad_group(community, grp)			\
> +	for (unsigned int __gi = 0;						\
> +	     __gi < community->ngpps && (grp = &community->gpps[__gi]);		\
> +	     __gi++)								\
> +
>  static const struct intel_padgroup *
>  intel_community_get_padgroup(const struct intel_community *community,
>  			     unsigned int pin)
>  {
> -	int i;
> -
> -	for (i = 0; i < community->ngpps; i++) {
> -		const struct intel_padgroup *padgrp = &community->gpps[i];
> +	const struct intel_padgroup *padgrp;
>  
> +	for_each_intel_community_pad_group(community, padgrp) {
>  		if (pin >= padgrp->base && pin < padgrp->base + padgrp->size)
>  			return padgrp;
>  	}
> @@ -924,6 +930,14 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
>  	.owner = THIS_MODULE,
>  };
>  
> +#define for_each_intel_pad_group(pctrl, community, grp)			\
> +	for_each_intel_pin_community(pctrl, community)			\
> +		for_each_intel_community_pad_group(community, grp)
> +
> +#define for_each_intel_gpio_group(pctrl, community, grp)		\
> +	for_each_intel_pad_group(pctrl, community, grp)			\
> +		if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else
> +
>  /**
>   * intel_gpio_to_pin() - Translate from GPIO offset to pin number
>   * @pctrl: Pinctrl structure
> @@ -942,30 +956,17 @@ static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned int offset,
>  			     const struct intel_community **community,
>  			     const struct intel_padgroup **padgrp)
>  {
> -	int i;
> +	const struct intel_community *comm;
> +	const struct intel_padgroup *grp;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		const struct intel_community *comm = &pctrl->communities[i];
> -		int j;
> +	for_each_intel_gpio_group(pctrl, comm, grp) {
> +		if (offset >= grp->gpio_base && offset < grp->gpio_base + grp->size) {
> +			if (community)
> +				*community = comm;
> +			if (padgrp)
> +				*padgrp = grp;
>  
> -		for (j = 0; j < comm->ngpps; j++) {
> -			const struct intel_padgroup *pgrp = &comm->gpps[j];
> -
> -			if (pgrp->gpio_base == INTEL_GPIO_BASE_NOMAP)
> -				continue;
> -
> -			if (offset >= pgrp->gpio_base &&
> -			    offset < pgrp->gpio_base + pgrp->size) {
> -				int pin;
> -
> -				pin = pgrp->base + offset - pgrp->gpio_base;
> -				if (community)
> -					*community = comm;
> -				if (padgrp)
> -					*padgrp = pgrp;
> -
> -				return pin;
> -			}
> +			return grp->base + offset - grp->gpio_base;
>  		}
>  	}
>  
> @@ -1258,12 +1259,11 @@ static const struct irq_chip intel_gpio_irq_chip = {
>  static int intel_gpio_community_irq_handler(struct intel_pinctrl *pctrl,
>  					    const struct intel_community *community)
>  {
> +	const struct intel_padgroup *padgrp;
>  	struct gpio_chip *gc = &pctrl->chip;
> -	unsigned int gpp;
>  	int ret = 0;
>  
> -	for (gpp = 0; gpp < community->ngpps; gpp++) {
> -		const struct intel_padgroup *padgrp = &community->gpps[gpp];
> +	for_each_intel_community_pad_group(community, padgrp) {
>  		unsigned long pending, enabled;
>  		unsigned int gpp, gpp_offset;
>  		void __iomem *reg, *is;
> @@ -1294,29 +1294,23 @@ static irqreturn_t intel_gpio_irq(int irq, void *data)
>  {
>  	const struct intel_community *community;
>  	struct intel_pinctrl *pctrl = data;
> -	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];
> +	for_each_intel_pin_community(pctrl, community)
>  		ret += intel_gpio_community_irq_handler(pctrl, community);
> -	}
>  
>  	return IRQ_RETVAL(ret);
>  }
>  
>  static void intel_gpio_irq_init(struct intel_pinctrl *pctrl)
>  {
> -	int i;
> +	const struct intel_community *community;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		const struct intel_community *community;
> +	for_each_intel_pin_community(pctrl, community) {
>  		void __iomem *reg, *is;
>  		unsigned int gpp;
>  
> -		community = &pctrl->communities[i];
> -
>  		for (gpp = 0; gpp < community->ngpps; gpp++) {
>  			reg = community->regs + community->ie_offset + gpp * 4;
>  			is = community->regs + community->is_offset + gpp * 4;
> @@ -1341,36 +1335,17 @@ static int intel_gpio_irq_init_hw(struct gpio_chip *gc)
>  	return 0;
>  }
>  
> -static int intel_gpio_add_community_ranges(struct intel_pinctrl *pctrl,
> -				const struct intel_community *community)
> -{
> -	int ret = 0, i;
> -
> -	for (i = 0; i < community->ngpps; i++) {
> -		const struct intel_padgroup *gpp = &community->gpps[i];
> -
> -		if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
> -			continue;
> -
> -		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> -					     gpp->gpio_base, gpp->base,
> -					     gpp->size);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	return ret;
> -}
> -
>  static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
>  {
>  	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> -	int ret, i;
> +	const struct intel_community *community;
> +	const struct intel_padgroup *grp;
> +	int ret;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		const struct intel_community *community = &pctrl->communities[i];
> -
> -		ret = intel_gpio_add_community_ranges(pctrl, community);
> +	for_each_intel_gpio_group(pctrl, community, grp) {
> +		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> +					     grp->gpio_base, grp->base,
> +					     grp->size);
>  		if (ret) {
>  			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
>  			return ret;
> @@ -1383,20 +1358,12 @@ static int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
>  static unsigned int intel_gpio_ngpio(const struct intel_pinctrl *pctrl)
>  {
>  	const struct intel_community *community;
> +	const struct intel_padgroup *grp;
>  	unsigned int ngpio = 0;
> -	int i, j;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		community = &pctrl->communities[i];
> -		for (j = 0; j < community->ngpps; j++) {
> -			const struct intel_padgroup *gpp = &community->gpps[j];
> -
> -			if (gpp->gpio_base == INTEL_GPIO_BASE_NOMAP)
> -				continue;
> -
> -			if (gpp->gpio_base + gpp->size > ngpio)
> -				ngpio = gpp->gpio_base + gpp->size;
> -		}
> +	for_each_intel_gpio_group(pctrl, community, grp) {
> +		if (grp->gpio_base + grp->size > ngpio)
> +			ngpio = grp->gpio_base + grp->size;
>  	}
>  
>  	return ngpio;
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v2 0/6] pinctrl: intel: High impedance impl. and cleanups

[Mika Westerberg]

On Thu, Aug 29, 2024 at 04:59:14PM +0300, Andy Shevchenko wrote:
> We would need a high impedance implementation for a quirk, so here it
> is. While doing this series I also noticed a couple of opportunities
> to clean up, hence a few more patches (1st, 5th, and 6th).
> 
> Series has been tested on Intel Meteor Lake-P.
> 
> v2:
> - fixed a bug in patch 1 when applying debounce value
> - updated enum style (Mika)
> - made intel_gpio_set_high_impedance return void (Mika)
> - new patch to constify intel_get_community()
> - resplit "absolutely grotesque" macro to four (Mika)
> - ...and update more users, this shrinks binary a lot
> 
> Andy Shevchenko (6):
>   pinctrl: intel: Move debounce validation out of the lock
>   pinctrl: intel: Refactor __intel_gpio_set_direction() to be more
>     useful
>   pinctrl: intel: Add __intel_gpio_get_direction() helper
>   pinctrl: intel: Implement high impedance support
>   pinctrl: intel: Constify intel_get_community() returned object
>   pinctrl: intel: Introduce for_each_intel_gpio_group() helper et al.

I sent one minor comment for the last patch. For the series,

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

Re: [PATCH v2 6/6] pinctrl: intel: Introduce for_each_intel_gpio_group() helper et al.

[Andy Shevchenko]

On Fri, Aug 30, 2024 at 07:50:39AM +0300, Mika Westerberg wrote:
> On Thu, Aug 29, 2024 at 04:59:20PM +0300, Andy Shevchenko wrote:

...

> > +#define for_each_intel_pin_community(pctrl, community)					\
> > +	for (unsigned int __ci = 0;							\
> > +	     __ci < pctrl->ncommunities && (community = &pctrl->communities[__ci]);	\
> > +	     __ci++)									\
> > +
> 
> These look more readable, thanks. Just one comment. Can you move all
> them here at the top of the file so all the variants are close to each
> other? You can do that while applying.

The rest three moved here.

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko