[PATCH v1 0/5] pinctrl: intel: High impedance impl. and cleanups

[PATCH v1 0/5] 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 two more patches (1st and 5th).

Andy Shevchenko (5):
  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: Introduce for_each_intel_gpio_group() helper

 drivers/pinctrl/intel/pinctrl-intel.c | 239 ++++++++++++++++----------
 1 file changed, 150 insertions(+), 89 deletions(-)

-- 
2.43.0.rc1.1336.g36b5255a03ac

[PATCH v1 1/5] 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.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 89bd7ce6711a..371b21d8f90b 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;
-
+	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 | PADCFG2_DEBOUNCE_MASK);
 	}
 
 	writel(value0, padcfg0);
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 1/5] pinctrl: intel: Move debounce validation out of the lock

[Andy Shevchenko]

On Wed, Aug 28, 2024 at 09:38:34PM +0300, Andy Shevchenko wrote:
> There is no need to validate debounce value under the lock.
> Move it outside.

...

> +	if (v) {
>  		/* Enable glitch filter and debouncer */
>  		value0 |= PADCFG0_PREGFRXSEL;
>  		value2 |= v << PADCFG2_DEBOUNCE_SHIFT;
>  		value2 |= PADCFG2_DEBEN;

This has a bug, I fix it locally for now and will wait for the comments for
the rest of the series.

> +	} else {
> +		/* Disable glitch filter and debouncer */
> +		value0 &= ~PADCFG0_PREGFRXSEL;
> +		value2 &= ~(PADCFG2_DEBEN | PADCFG2_DEBOUNCE_MASK);
>  	}

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 2/5] 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 371b21d8f90b..2a3d44f35348 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 v1 3/5] 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.

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 2a3d44f35348..3a135cfe435f 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)
@@ -429,6 +435,37 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
 	return 0;
 }
 
+/**
+ * enum - possible pad physical configurations
+ *
+ * @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 PAD_CONNECT_FULL;
+	};
+}
+
 static u32 __intel_gpio_set_direction(u32 value, bool input, bool output)
 {
 	if (input)
@@ -937,7 +974,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 +1027,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 +1727,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

Re: [PATCH v1 3/5] pinctrl: intel: Add __intel_gpio_get_direction() helper

[Mika Westerberg]

On Wed, Aug 28, 2024 at 09:38:36PM +0300, Andy Shevchenko wrote:
> 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.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 48 ++++++++++++++++++++++++---
>  1 file changed, 43 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 2a3d44f35348..3a135cfe435f 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)
> @@ -429,6 +435,37 @@ static int intel_pinmux_set_mux(struct pinctrl_dev *pctldev,
>  	return 0;
>  }
>  
> +/**
> + * enum - possible pad physical configurations
> + *

Start with capital letter as others:

enum - Possible..

Also I think we should follow the structs and drop the empty line here
(well and for other enums, I don't know how they got there ;-) but it
looks better without.

Other than this, looks good to me.

> + * @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 PAD_CONNECT_FULL;
> +	};
> +}
> +
>  static u32 __intel_gpio_set_direction(u32 value, bool input, bool output)
>  {
>  	if (input)
> @@ -937,7 +974,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 +1027,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 +1727,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

Re: [PATCH v1 3/5] pinctrl: intel: Add __intel_gpio_get_direction() helper

[Andy Shevchenko]

On Thu, Aug 29, 2024 at 07:46:53AM +0300, Mika Westerberg wrote:
> On Wed, Aug 28, 2024 at 09:38:36PM +0300, Andy Shevchenko wrote:

...

> > + * enum - possible pad physical configurations
> > + *
> 
> Start with capital letter as others:
> 
> enum - Possible..

Fixed!

> Also I think we should follow the structs and drop the empty line here
> (well and for other enums, I don't know how they got there ;-) but it
> looks better without.

We have the other enum with a blank line. Perhaps removing them in a separate
patch?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 3/5] pinctrl: intel: Add __intel_gpio_get_direction() helper

[Andy Shevchenko]

On Thu, Aug 29, 2024 at 01:49:38PM +0300, Andy Shevchenko wrote:
> On Thu, Aug 29, 2024 at 07:46:53AM +0300, Mika Westerberg wrote:
> > On Wed, Aug 28, 2024 at 09:38:36PM +0300, Andy Shevchenko wrote:

...

> > > + * enum - possible pad physical configurations
> > > + *
> > 
> > Start with capital letter as others:
> > 
> > enum - Possible..
> 
> Fixed!
> 
> > Also I think we should follow the structs and drop the empty line here
> > (well and for other enums, I don't know how they got there ;-) but it
> > looks better without.
> 
> We have the other enum with a blank line. Perhaps removing them in a separate
> patch?

Or I can do it "while at it", it seems to be quite small and likely never
conflicting change...

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 4/5] 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 | 46 +++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 3a135cfe435f..ae30969b2dee 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -78,6 +78,7 @@
 #define PADCFG0_GPIODIS_FULL		3
 #define PADCFG0_GPIORXDIS		BIT(9)
 #define PADCFG0_GPIOTXDIS		BIT(8)
+#define PADCFG0_GPIODIS			(BIT(9) | BIT(8))
 #define PADCFG0_GPIORXSTATE		BIT(1)
 #define PADCFG0_GPIOTXSTATE		BIT(0)
 
@@ -654,6 +655,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)
 {
@@ -697,6 +715,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)
@@ -795,6 +819,22 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
 	return 0;
 }
 
+static int 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);
+
+	return 0;
+}
+
 static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
 				     unsigned int pin, unsigned int debounce)
 {
@@ -857,6 +897,12 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
 				return ret;
 			break;
 
+		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
+			ret = intel_gpio_set_high_impedance(pctrl, pin);
+			if (ret)
+				return ret;
+			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

Re: [PATCH v1 4/5] pinctrl: intel: Implement high impedance support

[Mika Westerberg]

On Wed, Aug 28, 2024 at 09:38:37PM +0300, Andy Shevchenko wrote:
> 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 | 46 +++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 3a135cfe435f..ae30969b2dee 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -78,6 +78,7 @@
>  #define PADCFG0_GPIODIS_FULL		3
>  #define PADCFG0_GPIORXDIS		BIT(9)
>  #define PADCFG0_GPIOTXDIS		BIT(8)
> +#define PADCFG0_GPIODIS			(BIT(9) | BIT(8))
>  #define PADCFG0_GPIORXSTATE		BIT(1)
>  #define PADCFG0_GPIOTXSTATE		BIT(0)
>  
> @@ -654,6 +655,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)
>  {
> @@ -697,6 +715,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)
> @@ -795,6 +819,22 @@ static int intel_config_set_pull(struct intel_pinctrl *pctrl, unsigned int pin,
>  	return 0;
>  }
>  
> +static int 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);
> +
> +	return 0;

Why not make this return void?

> +}
> +
>  static int intel_config_set_debounce(struct intel_pinctrl *pctrl,
>  				     unsigned int pin, unsigned int debounce)
>  {
> @@ -857,6 +897,12 @@ static int intel_config_set(struct pinctrl_dev *pctldev, unsigned int pin,
>  				return ret;
>  			break;
>  
> +		case PIN_CONFIG_BIAS_HIGH_IMPEDANCE:
> +			ret = intel_gpio_set_high_impedance(pctrl, pin);
> +			if (ret)
> +				return ret;

Then this becomes simpler too.

> +			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

Re: [PATCH v1 4/5] pinctrl: intel: Implement high impedance support

[Andy Shevchenko]

On Thu, Aug 29, 2024 at 07:48:19AM +0300, Mika Westerberg wrote:
> On Wed, Aug 28, 2024 at 09:38:37PM +0300, Andy Shevchenko wrote:

...

> > +static int 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);
> > +
> > +	return 0;
> 
> Why not make this return void?

No special needs, can be void.

> > +}

...

Thank you for the review! I'll send a v2 soon.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v1 5/5] pinctrl: intel: Introduce for_each_intel_gpio_group() helper

[Andy Shevchenko]

Introduce a helper macro for_each_intel_gpio_group().
With that in place, update users.

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

  add/remove: 0/0 grow/shrink: 1/8 up/down: 37/-106 (-69)
  Total: Before=15611, After=15542, chg -0.44%

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

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index ae30969b2dee..143174abda32 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -931,6 +931,15 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
 	.owner = THIS_MODULE,
 };
 
+#define for_each_intel_gpio_group(pctrl, community, grp)				\
+	for (unsigned int __i = 0;							\
+	     __i < pctrl->ncommunities && (community = &pctrl->communities[__i]);	\
+	     __i++)									\
+		for (unsigned int __j = 0;						\
+		     __j < community->ngpps && (grp = &community->gpps[__j]);		\
+		     __j++)								\
+			if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else
+
 /**
  * intel_gpio_to_pin() - Translate from GPIO offset to pin number
  * @pctrl: Pinctrl structure
@@ -949,30 +958,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 *c;
+	const struct intel_padgroup *gpp;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		const struct intel_community *comm = &pctrl->communities[i];
-		int j;
+	for_each_intel_gpio_group(pctrl, c, gpp) {
+		if (offset >= gpp->gpio_base && offset < gpp->gpio_base + gpp->size) {
+			if (community)
+				*community = c;
+			if (padgrp)
+				*padgrp = gpp;
 
-		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 gpp->base + offset - gpp->gpio_base;
 		}
 	}
 
@@ -1348,36 +1344,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;
+	struct intel_community *community;
+	const struct intel_padgroup *gpp;
+	int ret;
 
-	for (i = 0; i < pctrl->ncommunities; i++) {
-		struct intel_community *community = &pctrl->communities[i];
-
-		ret = intel_gpio_add_community_ranges(pctrl, community);
+	for_each_intel_gpio_group(pctrl, community, gpp) {
+		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
+					     gpp->gpio_base, gpp->base,
+					     gpp->size);
 		if (ret) {
 			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
 			return ret;
@@ -1390,20 +1367,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 *gpp;
 	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, gpp) {
+		if (gpp->gpio_base + gpp->size > ngpio)
+			ngpio = gpp->gpio_base + gpp->size;
 	}
 
 	return ngpio;
-- 
2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 5/5] pinctrl: intel: Introduce for_each_intel_gpio_group() helper

[Mika Westerberg]

On Wed, Aug 28, 2024 at 09:38:38PM +0300, Andy Shevchenko wrote:
> Introduce a helper macro for_each_intel_gpio_group().
> With that in place, update users.
> 
> It reduces the C code base as well as shrinks the binary:
> 
>   add/remove: 0/0 grow/shrink: 1/8 up/down: 37/-106 (-69)
>   Total: Before=15611, After=15542, chg -0.44%
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 89 +++++++++------------------
>  1 file changed, 29 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index ae30969b2dee..143174abda32 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -931,6 +931,15 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
>  	.owner = THIS_MODULE,
>  };
>  
> +#define for_each_intel_gpio_group(pctrl, community, grp)				\
> +	for (unsigned int __i = 0;							\
> +	     __i < pctrl->ncommunities && (community = &pctrl->communities[__i]);	\
> +	     __i++)									\
> +		for (unsigned int __j = 0;						\
> +		     __j < community->ngpps && (grp = &community->gpps[__j]);		\
> +		     __j++)								\
> +			if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else
> +

This looks absolutely grotesque. I hope that you can debug this still
after couple of months has passed because I cannot ;-)

I wonder if there is a way to make it more readable by adding some sort
of helpers? Or perhaps we don't need to make the whole thing as macro
and just provide helpers we can use in the otherwise open-coded callers.

>  /**
>   * intel_gpio_to_pin() - Translate from GPIO offset to pin number
>   * @pctrl: Pinctrl structure
> @@ -949,30 +958,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 *c;
> +	const struct intel_padgroup *gpp;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		const struct intel_community *comm = &pctrl->communities[i];
> -		int j;
> +	for_each_intel_gpio_group(pctrl, c, gpp) {
> +		if (offset >= gpp->gpio_base && offset < gpp->gpio_base + gpp->size) {
> +			if (community)
> +				*community = c;
> +			if (padgrp)
> +				*padgrp = gpp;
>  
> -		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;
> -			}

Because I think this open-coded one is still at least readable. Of
course if there is duplication we should try to get rid of it but not in
expense of readability IMHO.

> +			return gpp->base + offset - gpp->gpio_base;
>  		}
>  	}
>  
> @@ -1348,36 +1344,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;
> +	struct intel_community *community;
> +	const struct intel_padgroup *gpp;
> +	int ret;
>  
> -	for (i = 0; i < pctrl->ncommunities; i++) {
> -		struct intel_community *community = &pctrl->communities[i];
> -
> -		ret = intel_gpio_add_community_ranges(pctrl, community);
> +	for_each_intel_gpio_group(pctrl, community, gpp) {
> +		ret = gpiochip_add_pin_range(&pctrl->chip, dev_name(pctrl->dev),
> +					     gpp->gpio_base, gpp->base,
> +					     gpp->size);
>  		if (ret) {
>  			dev_err(pctrl->dev, "failed to add GPIO pin range\n");
>  			return ret;
> @@ -1390,20 +1367,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 *gpp;
>  	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, gpp) {
> +		if (gpp->gpio_base + gpp->size > ngpio)
> +			ngpio = gpp->gpio_base + gpp->size;
>  	}
>  
>  	return ngpio;
> -- 
> 2.43.0.rc1.1336.g36b5255a03ac

Re: [PATCH v1 5/5] pinctrl: intel: Introduce for_each_intel_gpio_group() helper

[Andy Shevchenko]

On Thu, Aug 29, 2024 at 7:53 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Aug 28, 2024 at 09:38:38PM +0300, Andy Shevchenko wrote:
> > Introduce a helper macro for_each_intel_gpio_group().
> > With that in place, update users.
> >
> > It reduces the C code base as well as shrinks the binary:
> >
> >   add/remove: 0/0 grow/shrink: 1/8 up/down: 37/-106 (-69)
> >   Total: Before=15611, After=15542, chg -0.44%

...

> > +#define for_each_intel_gpio_group(pctrl, community, grp)                             \
> > +     for (unsigned int __i = 0;                                                      \
> > +          __i < pctrl->ncommunities && (community = &pctrl->communities[__i]);       \
> > +          __i++)                                                                     \
> > +             for (unsigned int __j = 0;                                              \
> > +                  __j < community->ngpps && (grp = &community->gpps[__j]);           \
> > +                  __j++)                                                             \
> > +                     if (grp->gpio_base == INTEL_GPIO_BASE_NOMAP) {} else
> > +
>
> This looks absolutely grotesque. I hope that you can debug this still
> after couple of months has passed because I cannot ;-)

Yes, I can.

> I wonder if there is a way to make it more readable by adding some sort
> of helpers? Or perhaps we don't need to make the whole thing as macro
> and just provide helpers we can use in the otherwise open-coded callers.

Yes, I can split it into two for-loops. But note, each of them a quite
standard how we define for_each macro with and without conditional,
see the jernel full of them (PCI, GPIOLIB, i915, ...).

...

> > -     for (i = 0; i < pctrl->ncommunities; i++) {
> > -             const struct intel_community *comm = &pctrl->communities[i];
> > -             int j;
> > +     for_each_intel_gpio_group(pctrl, c, gpp) {
> > +             if (offset >= gpp->gpio_base && offset < gpp->gpio_base + gpp->size) {
> > +                     if (community)
> > +                             *community = c;
> > +                     if (padgrp)
> > +                             *padgrp = gpp;
> >
> > -             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;
> > -                     }
>
> Because I think this open-coded one is still at least readable. Of
> course if there is duplication we should try to get rid of it but not in
> expense of readability IMHO.

The result I think is more readable as it's pretty clear from the
macro name what is iterating over. It also hides unneeded detail, i.e.
iterator variable.

>
> > +                     return gpp->base + offset - gpp->gpio_base;
> >               }
> >       }


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/5] pinctrl: intel: High impedance impl. and cleanups

[Andy Shevchenko]

+Cc: Heiner

On Wed, Aug 28, 2024 at 09:38:33PM +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 two more patches (1st and 5th).

Sorry it took a while to actually start implementing the quirk for your case.
Here I'm asking for the following things:

1) what is the marketing name of the device you have problems with?
(I believe it's available on the free market, correct?);

2) does it have any BIOS updates and, if it has, does it fix the issue?

3) can you apply patches 2,3,4,5 from this series (the first one is buggy and
not needed for you) and replace the hack I mentioned earlier with

	ret = intel_gpio_set_high_impedance(pctrl, 3);
	if (ret)
		return ret;

somewhere at the end of intel_pinctrl_probe()?

Does it still work as expected?


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/5] pinctrl: intel: High impedance impl. and cleanups

[Heiner Kallweit]

On 28.08.2024 22:00, Andy Shevchenko wrote:
> +Cc: Heiner
> 
> On Wed, Aug 28, 2024 at 09:38:33PM +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 two more patches (1st and 5th).
> 
> Sorry it took a while to actually start implementing the quirk for your case.
> Here I'm asking for the following things:
> 
> 1) what is the marketing name of the device you have problems with?
> (I believe it's available on the free market, correct?);
> 

Device is a dirt-cheap mini pc, marketed as Chatreey T9. It's available
on the free market, right. Dmesg says:
DMI: Default string Default string/Default string, BIOS ADLN.M6.SODIMM.ZB.CY.015 08/08/2023

> 2) does it have any BIOS updates and, if it has, does it fix the issue?
> 
No BIOS updates.

> 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and
> not needed for you) and replace the hack I mentioned earlier with
> 
> 	ret = intel_gpio_set_high_impedance(pctrl, 3);
> 	if (ret)
> 		return ret;
> 
> somewhere at the end of intel_pinctrl_probe()?
> 
> Does it still work as expected?
> 
> 
I will check.

Re: [PATCH v1 0/5] pinctrl: intel: High impedance impl. and cleanups

[Andy Shevchenko]

On Fri, Aug 30, 2024 at 07:56:11AM +0200, Heiner Kallweit wrote:
> On 28.08.2024 22:00, Andy Shevchenko wrote:
> > On Wed, Aug 28, 2024 at 09:38:33PM +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 two more patches (1st and 5th).
> > 
> > Sorry it took a while to actually start implementing the quirk for your case.
> > Here I'm asking for the following things:
> > 
> > 1) what is the marketing name of the device you have problems with?
> > (I believe it's available on the free market, correct?);
> 
> Device is a dirt-cheap mini pc, marketed as Chatreey T9. It's available
> on the free market, right. Dmesg says:
> DMI: Default string Default string/Default string, BIOS ADLN.M6.SODIMM.ZB.CY.015 08/08/2023
> 
> > 2) does it have any BIOS updates and, if it has, does it fix the issue?
> > 
> No BIOS updates.
> 
> > 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and
> > not needed for you) and replace the hack I mentioned earlier with
> > 
> > 	ret = intel_gpio_set_high_impedance(pctrl, 3);
> > 	if (ret)
> > 		return ret;
> > 
> > somewhere at the end of intel_pinctrl_probe()?
> > 
> > Does it still work as expected?
> > 
> I will check.

There is a v2 to test, you can take entire series, or for-next branch of Intel
pin control tree

https://git.kernel.org/pub/scm/linux/kernel/git/pinctrl/intel.git/log/?h=for-next

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 0/5] pinctrl: intel: High impedance impl. and cleanups

[Heiner Kallweit]

On 28.08.2024 22:00, Andy Shevchenko wrote:
> +Cc: Heiner
> 
> On Wed, Aug 28, 2024 at 09:38:33PM +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 two more patches (1st and 5th).
> 
> Sorry it took a while to actually start implementing the quirk for your case.
> Here I'm asking for the following things:
> 
> 1) what is the marketing name of the device you have problems with?
> (I believe it's available on the free market, correct?);
> 
> 2) does it have any BIOS updates and, if it has, does it fix the issue?
> 
> 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and
> not needed for you) and replace the hack I mentioned earlier with
> 
> 	ret = intel_gpio_set_high_impedance(pctrl, 3);
> 	if (ret)
> 		return ret;
> 
> somewhere at the end of intel_pinctrl_probe()?
> 
> Does it still work as expected?
> 
> 
In latest series the return value of intel_gpio_set_high_impedance()
has been removed. With the call to intel_gpio_set_high_impedance()
changed accordingly this fixes the problem on my system.
Thanks a lot for your efforts!

Re: [PATCH v1 0/5] pinctrl: intel: High impedance impl. and cleanups

[Andy Shevchenko]

On Fri, Aug 30, 2024 at 11:24:06PM +0200, Heiner Kallweit wrote:
> On 28.08.2024 22:00, Andy Shevchenko wrote:
> > On Wed, Aug 28, 2024 at 09:38:33PM +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 two more patches (1st and 5th).
> > 
> > Sorry it took a while to actually start implementing the quirk for your case.
> > Here I'm asking for the following things:
> > 
> > 1) what is the marketing name of the device you have problems with?
> > (I believe it's available on the free market, correct?);
> > 
> > 2) does it have any BIOS updates and, if it has, does it fix the issue?
> > 
> > 3) can you apply patches 2,3,4,5 from this series (the first one is buggy and
> > not needed for you) and replace the hack I mentioned earlier with
> > 
> > 	ret = intel_gpio_set_high_impedance(pctrl, 3);
> > 	if (ret)
> > 		return ret;
> > 
> > somewhere at the end of intel_pinctrl_probe()?
> > 
> > Does it still work as expected?
> > 
> > 
> In latest series the return value of intel_gpio_set_high_impedance()
> has been removed. With the call to intel_gpio_set_high_impedance()
> changed accordingly this fixes the problem on my system.
> Thanks a lot for your efforts!

Thank you! I will think now, how to make the real quirk (which I may ask you to
test in the future, when it will be ready) that looks and feels not as an ugly
hack.

-- 
With Best Regards,
Andy Shevchenko