[PATCH v1 0/5] pinctrl: intel: capability handling rework

[PATCH v1 0/5] pinctrl: intel: capability handling rework

[Andy Shevchenko]

During testing a new feature (see patch 3). I discovered that the
handling of capabilities is currently limited and in some cases may
even lead to not-working but announced feature (PWM). Hence there
2 fixes for the existing support, 1 enabling of a new feature, and
a couple of cleanup changes.

Andy Shevchenko (5):
  pinctrl: intel: Improve capability support
  pinctrl: intel: Fix the revision for new features (1kOhm PD, HW
    debouncer)
  pinctrl: intel: Enable 3-bit PAD_OWN feature
  pinctrl: intel: Refactor intel_gpio_add_pin_ranges() to make it
    shorter
  pinctrl: intel: define iterator variables inside for-loop

 drivers/pinctrl/intel/pinctrl-intel.c | 87 +++++++++++++++------------
 drivers/pinctrl/intel/pinctrl-intel.h |  1 +
 2 files changed, 49 insertions(+), 39 deletions(-)

-- 
2.50.1

[PATCH v1 1/5] pinctrl: intel: Improve capability support

[Andy Shevchenko]

The register space of a certain capability starts at the offset just after
the respective node in the capability list. It means that there are no fixed
offsets for them from SoC to SoC generation and they have to be calculated
at run-time. Improve capability support by adding the respective calculation
algorithm and in the result enable PWM on more platforms that currently may
use the wrong register.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 9d32bb8bc13a..adaa37a42754 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -53,8 +53,6 @@
 #define PADOWN_MASK(p)			(GENMASK(3, 0) << PADOWN_SHIFT(p))
 #define PADOWN_GPP(p)			((p) / 8)
 
-#define PWMC				0x204
-
 /* Offset from pad_regs */
 #define PADCFG0				0x000
 #define PADCFG0_RXEVCFG_MASK		GENMASK(26, 25)
@@ -1549,8 +1547,10 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
 }
 
 static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl,
-				   struct intel_community *community)
+				   struct intel_community *community,
+				   unsigned short capability_offset)
 {
+	void __iomem *base = community->regs + capability_offset + 4;
 	static const struct pwm_lpss_boardinfo info = {
 		.clk_rate = 19200000,
 		.npwm = 1,
@@ -1564,7 +1564,7 @@ static int intel_pinctrl_probe_pwm(struct intel_pinctrl *pctrl,
 	if (!IS_REACHABLE(CONFIG_PWM_LPSS))
 		return 0;
 
-	chip = devm_pwm_lpss_probe(pctrl->dev, community->regs + PWMC, &info);
+	chip = devm_pwm_lpss_probe(pctrl->dev, base, &info);
 	return PTR_ERR_OR_ZERO(chip);
 }
 
@@ -1595,6 +1595,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 
 	for (i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
+		unsigned short capability_offset[6];
 		void __iomem *regs;
 		u32 offset;
 		u32 value;
@@ -1622,15 +1623,19 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 			switch ((value & CAPLIST_ID_MASK) >> CAPLIST_ID_SHIFT) {
 			case CAPLIST_ID_GPIO_HW_INFO:
 				community->features |= PINCTRL_FEATURE_GPIO_HW_INFO;
+				capability_offset[CAPLIST_ID_GPIO_HW_INFO] = offset;
 				break;
 			case CAPLIST_ID_PWM:
 				community->features |= PINCTRL_FEATURE_PWM;
+				capability_offset[CAPLIST_ID_PWM] = offset;
 				break;
 			case CAPLIST_ID_BLINK:
 				community->features |= PINCTRL_FEATURE_BLINK;
+				capability_offset[CAPLIST_ID_BLINK] = offset;
 				break;
 			case CAPLIST_ID_EXP:
 				community->features |= PINCTRL_FEATURE_EXP;
+				capability_offset[CAPLIST_ID_EXP] = offset;
 				break;
 			default:
 				break;
@@ -1653,7 +1658,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 		if (ret)
 			return ret;
 
-		ret = intel_pinctrl_probe_pwm(pctrl, community);
+		ret = intel_pinctrl_probe_pwm(pctrl, community, capability_offset[CAPLIST_ID_PWM]);
 		if (ret)
 			return ret;
 	}
-- 
2.50.1

[PATCH v1 2/5] pinctrl: intel: Fix the revision for new features (1kOhm PD, HW debouncer)

[Andy Shevchenko]

The 1kOhm pull down and hardware debouncer are features of the revision 0.92
of the Chassis specification. Fix that in the code accordingly.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index adaa37a42754..a5a264ba6fbb 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1611,7 +1611,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 		value = readl(regs + REVID);
 		if (value == ~0u)
 			return -ENODEV;
-		if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x94) {
+		if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x92) {
 			community->features |= PINCTRL_FEATURE_DEBOUNCE;
 			community->features |= PINCTRL_FEATURE_1K_PD;
 		}
-- 
2.50.1

[PATCH v1 3/5] pinctrl: intel: Enable 3-bit PAD_OWN feature

[Andy Shevchenko]

Starting from revision 1.1 of the Chassis specification the PAD_OWN
is represented by 3 bits instead of 2 bits in the previous revisions.
Update the driver to support this feature.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index a5a264ba6fbb..97bf5ec78db4 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -203,19 +203,25 @@ static bool intel_pad_owned_by_host(const struct intel_pinctrl *pctrl, unsigned
 	community = intel_get_community(pctrl, pin);
 	if (!community)
 		return false;
-	if (!community->padown_offset)
+
+	/* If padown_offset is not provided, assume host ownership */
+	padown = community->regs + community->padown_offset;
+	if (padown == community->regs)
 		return true;
 
+	/* New HW generations have extended PAD_OWN registers */
+	if (community->features & PINCTRL_FEATURE_3BIT_PAD_OWN)
+		return !(readl(padown + pin_to_padno(community, pin) * 4) & 7);
+
 	padgrp = intel_community_get_padgroup(community, pin);
 	if (!padgrp)
 		return false;
 
 	gpp_offset = padgroup_offset(padgrp, pin);
 	gpp = PADOWN_GPP(gpp_offset);
-	offset = community->padown_offset + padgrp->padown_num * 4 + gpp * 4;
-	padown = community->regs + offset;
+	offset = padgrp->padown_num * 4 + gpp * 4;
 
-	return !(readl(padown) & PADOWN_MASK(gpp_offset));
+	return !(readl(padown + offset) & PADOWN_MASK(gpp_offset));
 }
 
 static bool intel_pad_acpi_mode(const struct intel_pinctrl *pctrl, unsigned int pin)
@@ -1597,6 +1603,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 		struct intel_community *community = &pctrl->communities[i];
 		unsigned short capability_offset[6];
 		void __iomem *regs;
+		u32 revision;
 		u32 offset;
 		u32 value;
 
@@ -1611,10 +1618,14 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 		value = readl(regs + REVID);
 		if (value == ~0u)
 			return -ENODEV;
-		if (((value & REVID_MASK) >> REVID_SHIFT) >= 0x92) {
+
+		revision = (value & REVID_MASK) >> REVID_SHIFT;
+		if (revision >= 0x092) {
 			community->features |= PINCTRL_FEATURE_DEBOUNCE;
 			community->features |= PINCTRL_FEATURE_1K_PD;
 		}
+		if (revision >= 0x110)
+			community->features |= PINCTRL_FEATURE_3BIT_PAD_OWN;
 
 		/* Determine community features based on the capabilities */
 		offset = CAPLIST;
diff --git a/drivers/pinctrl/intel/pinctrl-intel.h b/drivers/pinctrl/intel/pinctrl-intel.h
index 2f37109d5860..b5476b9de0db 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.h
+++ b/drivers/pinctrl/intel/pinctrl-intel.h
@@ -150,6 +150,7 @@ struct intel_community {
 #define PINCTRL_FEATURE_PWM		BIT(3)
 #define PINCTRL_FEATURE_BLINK		BIT(4)
 #define PINCTRL_FEATURE_EXP		BIT(5)
+#define PINCTRL_FEATURE_3BIT_PAD_OWN	BIT(6)
 
 #define __INTEL_COMMUNITY(b, s, e, g, n, gs, gn, soc)		\
 	{							\
-- 
2.50.1

[PATCH v1 4/5] pinctrl: intel: Refactor intel_gpio_add_pin_ranges() to make it shorter

[Andy Shevchenko]

Refactor intel_gpio_add_pin_ranges() to make it shorter in binary
and source formats.

Function                                     old     new   delta
intel_gpio_add_pin_ranges                    219     215      -4
Total: Before=15522, After=15518, chg -0.03%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 97bf5ec78db4..7311b787dfc6 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -1361,16 +1361,15 @@ static int intel_gpio_irq_init_hw(struct gpio_chip *gc)
 int intel_gpio_add_pin_ranges(struct gpio_chip *gc)
 {
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
+	const struct device *dev = pctrl->dev;
 	const struct intel_community *community;
 	const struct intel_padgroup *grp;
 	int ret;
 
 	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);
+		ret = gpiochip_add_pin_range(gc, dev_name(dev), grp->gpio_base, grp->base, grp->size);
 		if (ret)
-			return dev_err_probe(pctrl->dev, ret, "failed to add GPIO pin range\n");
+			return dev_err_probe(dev, ret, "failed to add GPIO pin range\n");
 	}
 
 	return 0;
-- 
2.50.1

[PATCH v1 5/5] pinctrl: intel: define iterator variables inside for-loop

[Andy Shevchenko]

Reduce the scope of the iterator variables by defining them inside
the respective for-loops. This makes code more robust against reuse
of the same variable in the future, which might lead to some mistakes.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 7311b787dfc6..c506f9f343c3 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -431,7 +431,6 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
 	const struct intel_pingroup *grp = &pctrl->soc->groups[group];
-	int i;
 
 	guard(raw_spinlock_irqsave)(&pctrl->lock);
 
@@ -439,13 +438,13 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	 * All pins in the groups needs to be accessible and writable
 	 * before we can enable the mux for this group.
 	 */
-	for (i = 0; i < grp->grp.npins; i++) {
+	for (unsigned int i = 0; i < grp->grp.npins; i++) {
 		if (!intel_pad_usable(pctrl, grp->grp.pins[i]))
 			return -EBUSY;
 	}
 
 	/* Now enable the mux setting for each pin in the group */
-	for (i = 0; i < grp->grp.npins; i++) {
+	for (unsigned int i = 0; i < grp->grp.npins; i++) {
 		void __iomem *padcfg0;
 		u32 value, pmode;
 
@@ -909,12 +908,12 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 			  unsigned long *configs, unsigned int nconfigs)
 {
 	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-	int i, ret;
+	int ret;
 
 	if (!intel_pad_usable(pctrl, pin))
 		return -ENOTSUPP;
 
-	for (i = 0; i < nconfigs; i++) {
+	for (unsigned int i = 0; i < nconfigs; i++) {
 		switch (pinconf_to_config_param(configs[i])) {
 		case PIN_CONFIG_BIAS_DISABLE:
 		case PIN_CONFIG_BIAS_PULL_UP:
@@ -1323,9 +1322,8 @@ static void intel_gpio_irq_init(struct intel_pinctrl *pctrl)
 
 	for_each_intel_pin_community(pctrl, community) {
 		void __iomem *reg, *is;
-		unsigned int gpp;
 
-		for (gpp = 0; gpp < community->ngpps; gpp++) {
+		for (unsigned int gpp = 0; gpp < community->ngpps; gpp++) {
 			reg = community->regs + community->ie_offset + gpp * 4;
 			is = community->regs + community->is_offset + gpp * 4;
 
@@ -1436,14 +1434,14 @@ static int intel_pinctrl_add_padgroups_by_gpps(struct intel_pinctrl *pctrl,
 					       struct intel_community *community)
 {
 	struct intel_padgroup *gpps;
+	unsigned int ngpps = community->ngpps;
 	unsigned int padown_num = 0;
-	size_t i, ngpps = community->ngpps;
 
 	gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL);
 	if (!gpps)
 		return -ENOMEM;
 
-	for (i = 0; i < ngpps; i++) {
+	for (unsigned int i = 0; i < ngpps; i++) {
 		gpps[i] = community->gpps[i];
 
 		if (gpps[i].size > INTEL_PINCTRL_MAX_GPP_SIZE)
@@ -1476,18 +1474,18 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl,
 					       struct intel_community *community)
 {
 	struct intel_padgroup *gpps;
-	unsigned int npins = community->npins;
+	unsigned int npins = community->npins, ngpps;
 	unsigned int padown_num = 0;
-	size_t i, ngpps = DIV_ROUND_UP(npins, community->gpp_size);
 
 	if (community->gpp_size > INTEL_PINCTRL_MAX_GPP_SIZE)
 		return -EINVAL;
 
+	ngpps = DIV_ROUND_UP(npins, community->gpp_size);
 	gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL);
 	if (!gpps)
 		return -ENOMEM;
 
-	for (i = 0; i < ngpps; i++) {
+	for (unsigned int i = 0; i < ngpps; i++) {
 		unsigned int gpp_size = community->gpp_size;
 
 		gpps[i].reg_num = i;
@@ -1513,7 +1511,6 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
 	const struct intel_pinctrl_soc_data *soc = pctrl->soc;
 	struct intel_community_context *communities;
 	struct intel_pad_context *pads;
-	int i;
 
 	pads = devm_kcalloc(pctrl->dev, soc->npins, sizeof(*pads), GFP_KERNEL);
 	if (!pads)
@@ -1525,7 +1522,7 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
 		return -ENOMEM;
 
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
+	for (unsigned int i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 		u32 *intmask, *hostown;
 
@@ -1578,7 +1575,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 {
 	struct device *dev = &pdev->dev;
 	struct intel_pinctrl *pctrl;
-	int i, ret, irq;
+	int ret, irq;
 
 	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
 	if (!pctrl)
@@ -1598,7 +1595,7 @@ int intel_pinctrl_probe(struct platform_device *pdev,
 	if (!pctrl->communities)
 		return -ENOMEM;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
+	for (unsigned int i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 		unsigned short capability_offset[6];
 		void __iomem *regs;
@@ -1806,10 +1803,9 @@ static int intel_pinctrl_suspend_noirq(struct device *dev)
 	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
 	struct intel_community_context *communities;
 	struct intel_pad_context *pads;
-	int i;
 
 	pads = pctrl->context.pads;
-	for (i = 0; i < pctrl->soc->npins; i++) {
+	for (unsigned int i = 0; i < pctrl->soc->npins; i++) {
 		const struct pinctrl_pin_desc *desc = &pctrl->soc->pins[i];
 		void __iomem *padcfg;
 		u32 val;
@@ -1828,7 +1824,7 @@ static int intel_pinctrl_suspend_noirq(struct device *dev)
 	}
 
 	communities = pctrl->context.communities;
-	for (i = 0; i < pctrl->ncommunities; i++) {
+	for (unsigned int i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 		void __iomem *base;
 		unsigned int gpp;
@@ -1915,13 +1911,12 @@ static int intel_pinctrl_resume_noirq(struct device *dev)
 	struct intel_pinctrl *pctrl = dev_get_drvdata(dev);
 	const struct intel_community_context *communities;
 	const struct intel_pad_context *pads;
-	int i;
 
 	/* Mask all interrupts */
 	intel_gpio_irq_init(pctrl);
 
 	pads = pctrl->context.pads;
-	for (i = 0; i < pctrl->soc->npins; i++) {
+	for (unsigned int i = 0; i < pctrl->soc->npins; i++) {
 		const struct pinctrl_pin_desc *desc = &pctrl->soc->pins[i];
 
 		if (!(intel_pinctrl_should_save(pctrl, desc->number) ||
@@ -1938,17 +1933,16 @@ static int intel_pinctrl_resume_noirq(struct device *dev)
 	}
 
 	communities = pctrl->context.communities;
-	for (i = 0; i < pctrl->ncommunities; i++) {
+	for (unsigned int i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 		void __iomem *base;
-		unsigned int gpp;
 
 		base = community->regs + community->ie_offset;
-		for (gpp = 0; gpp < community->ngpps; gpp++)
+		for (unsigned int gpp = 0; gpp < community->ngpps; gpp++)
 			intel_restore_intmask(pctrl, i, base, gpp, communities[i].intmask[gpp]);
 
 		base = community->regs + community->hostown_offset;
-		for (gpp = 0; gpp < community->ngpps; gpp++)
+		for (unsigned int gpp = 0; gpp < community->ngpps; gpp++)
 			intel_restore_hostown(pctrl, i, base, gpp, communities[i].hostown[gpp]);
 	}
 
-- 
2.50.1

Re: [PATCH v1 1/5] pinctrl: intel: Improve capability support

[Mika Westerberg]

On Wed, Mar 18, 2026 at 04:10:15PM +0100, Andy Shevchenko wrote:
> The register space of a certain capability starts at the offset just after
> the respective node in the capability list. It means that there are no fixed
> offsets for them from SoC to SoC generation and they have to be calculated
> at run-time. Improve capability support by adding the respective calculation
> algorithm and in the result enable PWM on more platforms that currently may
> use the wrong register.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Re: [PATCH v1 2/5] pinctrl: intel: Fix the revision for new features (1kOhm PD, HW debouncer)

[Mika Westerberg]

On Wed, Mar 18, 2026 at 04:10:16PM +0100, Andy Shevchenko wrote:
> The 1kOhm pull down and hardware debouncer are features of the revision 0.92
> of the Chassis specification. Fix that in the code accordingly.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Re: [PATCH v1 3/5] pinctrl: intel: Enable 3-bit PAD_OWN feature

[Mika Westerberg]

On Wed, Mar 18, 2026 at 04:10:17PM +0100, Andy Shevchenko wrote:
> Starting from revision 1.1 of the Chassis specification the PAD_OWN
> is represented by 3 bits instead of 2 bits in the previous revisions.
> Update the driver to support this feature.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

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

Re: [PATCH v1 5/5] pinctrl: intel: define iterator variables inside for-loop

[Mika Westerberg]

On Wed, Mar 18, 2026 at 04:10:19PM +0100, Andy Shevchenko wrote:
> Reduce the scope of the iterator variables by defining them inside
> the respective for-loops. This makes code more robust against reuse
> of the same variable in the future, which might lead to some mistakes.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 44 ++++++++++++---------------
>  1 file changed, 19 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 7311b787dfc6..c506f9f343c3 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -431,7 +431,6 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  {
>  	struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
>  	const struct intel_pingroup *grp = &pctrl->soc->groups[group];
> -	int i;

If there are multiple loops, I prefer to declare the variable outside of
them.

If it is just a single loop then for (int i = 0, ..) is fine.

>  
>  	guard(raw_spinlock_irqsave)(&pctrl->lock);
>  
> @@ -439,13 +438,13 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	 * All pins in the groups needs to be accessible and writable
>  	 * before we can enable the mux for this group.
>  	 */
> -	for (i = 0; i < grp->grp.npins; i++) {
> +	for (unsigned int i = 0; i < grp->grp.npins; i++) {

also why you use "unsigned int". int i is fine here.

>  		if (!intel_pad_usable(pctrl, grp->grp.pins[i]))
>  			return -EBUSY;
>  	}

Re: [PATCH v1 4/5] pinctrl: intel: Refactor intel_gpio_add_pin_ranges() to make it shorter

[Mika Westerberg]

On Wed, Mar 18, 2026 at 04:10:18PM +0100, Andy Shevchenko wrote:
> Refactor intel_gpio_add_pin_ranges() to make it shorter in binary
> and source formats.
> 
> Function                                     old     new   delta
> intel_gpio_add_pin_ranges                    219     215      -4
> Total: Before=15522, After=15518, chg -0.03%

Well if you enable optimization the compiler probably puts this into
register and you don't see any changes here. Also code-wise this is
neglible IMHO.

Re: [PATCH v1 4/5] pinctrl: intel: Refactor intel_gpio_add_pin_ranges() to make it shorter

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 07:03:45AM +0100, Mika Westerberg wrote:
> On Wed, Mar 18, 2026 at 04:10:18PM +0100, Andy Shevchenko wrote:
> > Refactor intel_gpio_add_pin_ranges() to make it shorter in binary
> > and source formats.
> > 
> > Function                                     old     new   delta
> > intel_gpio_add_pin_ranges                    219     215      -4
> > Total: Before=15522, After=15518, chg -0.03%
> 
> Well if you enable optimization the compiler probably puts this into
> register and you don't see any changes here.

What do you mean? I do not alter what kernel uses.

> Also code-wise this is neglible IMHO.

Maybe, but still good to have also shorter in source as well — we have
-1 LoC at the end of the day.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] pinctrl: intel: define iterator variables inside for-loop

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 07:02:21AM +0100, Mika Westerberg wrote:
> On Wed, Mar 18, 2026 at 04:10:19PM +0100, Andy Shevchenko wrote:
> > Reduce the scope of the iterator variables by defining them inside
> > the respective for-loops. This makes code more robust against reuse
> > of the same variable in the future, which might lead to some mistakes.

...

> > -	int i;
> 
> If there are multiple loops, I prefer to declare the variable outside of
> them.

Why?! It's exactly where it make even more sense to hide.

> If it is just a single loop then for (int i = 0, ..) is fine.

...

> > -	for (i = 0; i < grp->grp.npins; i++) {
> > +	for (unsigned int i = 0; i < grp->grp.npins; i++) {
> 
> also why you use "unsigned int". int i is fine here.

Because grp.npins is unsigned. This is the common sense to use the same
variable type that's used for the (upper) limit.

> >  	}

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 1/5] pinctrl: intel: Improve capability support

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 06:57:54AM +0100, Mika Westerberg wrote:
> On Wed, Mar 18, 2026 at 04:10:15PM +0100, Andy Shevchenko wrote:
> > The register space of a certain capability starts at the offset just after
> > the respective node in the capability list. It means that there are no fixed
> > offsets for them from SoC to SoC generation and they have to be calculated
> > at run-time. Improve capability support by adding the respective calculation
> > algorithm and in the result enable PWM on more platforms that currently may
> > use the wrong register.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 2/5] pinctrl: intel: Fix the revision for new features (1kOhm PD, HW debouncer)

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 06:58:12AM +0100, Mika Westerberg wrote:
> On Wed, Mar 18, 2026 at 04:10:16PM +0100, Andy Shevchenko wrote:
> > The 1kOhm pull down and hardware debouncer are features of the revision 0.92
> > of the Chassis specification. Fix that in the code accordingly.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] pinctrl: intel: Enable 3-bit PAD_OWN feature

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 06:58:37AM +0100, Mika Westerberg wrote:
> On Wed, Mar 18, 2026 at 04:10:17PM +0100, Andy Shevchenko wrote:
> > Starting from revision 1.1 of the Chassis specification the PAD_OWN
> > is represented by 3 bits instead of 2 bits in the previous revisions.
> > Update the driver to support this feature.
> > 
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> 
> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Pushed to my review and testing queue, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 4/5] pinctrl: intel: Refactor intel_gpio_add_pin_ranges() to make it shorter

[Mika Westerberg]

On Thu, Mar 19, 2026 at 08:56:04AM +0200, Andy Shevchenko wrote:
> On Thu, Mar 19, 2026 at 07:03:45AM +0100, Mika Westerberg wrote:
> > On Wed, Mar 18, 2026 at 04:10:18PM +0100, Andy Shevchenko wrote:
> > > Refactor intel_gpio_add_pin_ranges() to make it shorter in binary
> > > and source formats.
> > > 
> > > Function                                     old     new   delta
> > > intel_gpio_add_pin_ranges                    219     215      -4
> > > Total: Before=15522, After=15518, chg -0.03%
> > 
> > Well if you enable optimization the compiler probably puts this into
> > register and you don't see any changes here.
> 
> What do you mean? I do not alter what kernel uses.

Enable -Ox and compare then. I think there is Kconfig option for that too.

> > Also code-wise this is neglible IMHO.
> 
> Maybe, but still good to have also shorter in source as well — we have
> -1 LoC at the end of the day.

Well this is silly. Your other patch adds "unsigned " to every other place
growing the driver "horizonally" but that's okay.

Re: [PATCH v1 5/5] pinctrl: intel: define iterator variables inside for-loop

[Mika Westerberg]

On Thu, Mar 19, 2026 at 08:57:58AM +0200, Andy Shevchenko wrote:
> On Thu, Mar 19, 2026 at 07:02:21AM +0100, Mika Westerberg wrote:
> > On Wed, Mar 18, 2026 at 04:10:19PM +0100, Andy Shevchenko wrote:
> > > Reduce the scope of the iterator variables by defining them inside
> > > the respective for-loops. This makes code more robust against reuse
> > > of the same variable in the future, which might lead to some mistakes.
> 
> ...
> 
> > > -	int i;
> > 
> > If there are multiple loops, I prefer to declare the variable outside of
> > them.
> 
> Why?! It's exactly where it make even more sense to hide.

I disagree.

> 
> > If it is just a single loop then for (int i = 0, ..) is fine.
> 
> ...
> 
> > > -	for (i = 0; i < grp->grp.npins; i++) {
> > > +	for (unsigned int i = 0; i < grp->grp.npins; i++) {
> > 
> > also why you use "unsigned int". int i is fine here.
> 
> Because grp.npins is unsigned. This is the common sense to use the same
> variable type that's used for the (upper) limit.

No, just use "int i" there. Compiler is fine and this is more idiomatic C
anyways.

Re: [PATCH v1 4/5] pinctrl: intel: Refactor intel_gpio_add_pin_ranges() to make it shorter

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 08:07:16AM +0100, Mika Westerberg wrote:
> On Thu, Mar 19, 2026 at 08:56:04AM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 19, 2026 at 07:03:45AM +0100, Mika Westerberg wrote:
> > > On Wed, Mar 18, 2026 at 04:10:18PM +0100, Andy Shevchenko wrote:
> > > > Refactor intel_gpio_add_pin_ranges() to make it shorter in binary
> > > > and source formats.
> > > > 
> > > > Function                                     old     new   delta
> > > > intel_gpio_add_pin_ranges                    219     215      -4
> > > > Total: Before=15522, After=15518, chg -0.03%
> > > 
> > > Well if you enable optimization the compiler probably puts this into
> > > register and you don't see any changes here.
> > 
> > What do you mean? I do not alter what kernel uses.
> 
> Enable -Ox and compare then. I think there is Kconfig option for that too.

I don't see it. Can you elaborate?

> > > Also code-wise this is neglible IMHO.
> > 
> > Maybe, but still good to have also shorter in source as well — we have
> > -1 LoC at the end of the day.
> 
> Well this is silly.

You usually likes - statistics. So, here we are.

> Your other patch adds "unsigned " to every other place
> growing the driver "horizonally" but that's okay.

That's unrelated to this patch and to this certain improvement.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] pinctrl: intel: define iterator variables inside for-loop

[Andy Shevchenko]

On Thu, Mar 19, 2026 at 08:09:25AM +0100, Mika Westerberg wrote:
> On Thu, Mar 19, 2026 at 08:57:58AM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 19, 2026 at 07:02:21AM +0100, Mika Westerberg wrote:
> > > On Wed, Mar 18, 2026 at 04:10:19PM +0100, Andy Shevchenko wrote:

...

> > > > -	int i;
> > > 
> > > If there are multiple loops, I prefer to declare the variable outside of
> > > them.
> > 
> > Why?! It's exactly where it make even more sense to hide.
> 
> I disagree.

Why? Can you give a constructive feedback, please?

> > > If it is just a single loop then for (int i = 0, ..) is fine.

...

> > > > -	for (i = 0; i < grp->grp.npins; i++) {
> > > > +	for (unsigned int i = 0; i < grp->grp.npins; i++) {
> > > 
> > > also why you use "unsigned int". int i is fine here.
> > 
> > Because grp.npins is unsigned. This is the common sense to use the same
> > variable type that's used for the (upper) limit.
> 
> No, just use "int i" there. Compiler is fine and this is more idiomatic C
> anyways.

I can agree on this, but it (in this case theoretically) might lead to subtle
signdness issues if compiler is not capable to see the upper limit and predict
no overflow or wrap-around.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 5/5] pinctrl: intel: define iterator variables inside for-loop

[Mika Westerberg]

On Thu, Mar 19, 2026 at 09:20:51AM +0200, Andy Shevchenko wrote:
> On Thu, Mar 19, 2026 at 08:09:25AM +0100, Mika Westerberg wrote:
> > On Thu, Mar 19, 2026 at 08:57:58AM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 19, 2026 at 07:02:21AM +0100, Mika Westerberg wrote:
> > > > On Wed, Mar 18, 2026 at 04:10:19PM +0100, Andy Shevchenko wrote:
> 
> ...
> 
> > > > > -	int i;
> > > > 
> > > > If there are multiple loops, I prefer to declare the variable outside of
> > > > them.
> > > 
> > > Why?! It's exactly where it make even more sense to hide.
> > 
> > I disagree.
> 
> Why? Can you give a constructive feedback, please?

I think I did already.

If you have just a single loop it's okay. If multiple then it makes more
sense to declare the variable in the outer scope. This is at least what I'm
used to do and prefer here as well.