[PATCH 1/3] pinctrl: cherryview: Don't use pin/offset 0 to mark an interrupt-line as unused

[PATCH 1/3] pinctrl: cherryview: Don't use pin/offset 0 to mark an interrupt-line as unused

[Hans de Goede]

offset/pin 0 is a perfectly valid offset, so stop using it to have
the special meaning of interrupt-line not used in the intr_lines.

Instead introduce a new special INTR_LINE_UNUSED value which is never
a valid offset and use that to indicate unused interrupt-lines.

Cc: Yauhen Kharuzhy <jekhor@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 980099028cf8..a46f9e5a4748 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -73,6 +73,8 @@ struct intel_pad_context {
 	u32 padctrl1;
 };
 
+#define INTR_LINE_UNUSED		U32_MAX
+
 /**
  * struct intel_community_context - community context for Cherryview
  * @intr_lines: Mapping between 16 HW interrupt wires and GPIO offset (in GPIO number space)
@@ -812,7 +814,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
 		/* Reset the interrupt mapping */
 		for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++) {
 			if (cctx->intr_lines[i] == offset) {
-				cctx->intr_lines[i] = 0;
+				cctx->intr_lines[i] = INTR_LINE_UNUSED;
 				break;
 			}
 		}
@@ -1319,7 +1321,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 		else
 			handler = handle_edge_irq;
 
-		if (!cctx->intr_lines[intsel]) {
+		if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
 			irq_set_handler_locked(d, handler);
 			cctx->intr_lines[intsel] = pin;
 		}
@@ -1412,6 +1414,12 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
 		unsigned int offset;
 
 		offset = cctx->intr_lines[intr_line];
+		if (offset == INTR_LINE_UNUSED) {
+			dev_err(pctrl->dev, "Interrupt on unused interrupt line %u\n",
+				intr_line);
+			continue;
+		}
+
 		generic_handle_domain_irq(gc->irq.domain, offset);
 	}
 
@@ -1620,9 +1628,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	struct intel_community *community;
 	struct device *dev = &pdev->dev;
 	struct acpi_device *adev = ACPI_COMPANION(dev);
+	struct intel_community_context *cctx;
 	struct intel_pinctrl *pctrl;
 	acpi_status status;
-	int ret, irq;
+	int i, ret, irq;
 
 	soc_data = intel_pinctrl_get_soc_data(pdev);
 	if (IS_ERR(soc_data))
@@ -1663,6 +1672,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	if (!pctrl->context.communities)
 		return -ENOMEM;
 
+	cctx = &pctrl->context.communities[0];
+	for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++)
+		cctx->intr_lines[i] = INTR_LINE_UNUSED;
+
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0)
 		return irq;
-- 
2.31.1

[PATCH 2/3] pinctrl: cherryview: Do not allow the same intr-line to be used by 2 pins

[Hans de Goede]

It is impossible to use the same intr-line for 2 pins, this will result
in the interrupts only being delivered to the irq-handler for the pin for
which chv_gpio_irq_type() was called last.

The pinctrl-cherryview.c code relies on the BIOS to correctly setup the
interrupt-line, but there is a BIOS bug on at least the Medion Akoya E1239T
and the GPD win models where both INT33FF:02 pin 8, used by the powerbutton
and INT33FF:02 pin 21 used as IRQ input for the accelerometer are mapped to
interrupt-line 0.

This causes 2 problems:
1. The accelerometer IRQ does not work, since the power-button is probed
later taking over the intr_lines[0] slot.

2. Since the accelerometer IRQ is not marked as wakeup, interrupt-line 0
gets masked on suspend, causing the power-button to not work to wake
the system from suspend.

Likewise on the Lenovo Yogabook which has a touchscreen as keyboard
and the keyboard half of the tablet also has a drawing tablet, the
BIOS by default assigns the same interrupt-line to the GPIOs used
for their interrupts.

Fix these problems by adding a check for this and assigning a new
interrupt-line to the 2nd pin for which chv_gpio_irq_type() gets called.

With this fix in place the following 2 messages show up in dmesg on
the Medion Akoya E1239T and the GPD win:

 cherryview-pinctrl INT33FF:02: [Firmware Bug]: interrupt-line 0 is used by both pin 21 and pin 8
 cherryview-pinctrl INT33FF:02: Changing the interrupt-line for pin 8 to 15

And the following gets logged on the Lenovo Yogabook:

 cherryview-pinctrl INT33FF:01: Interrupt-line 0 is used by both pin 49 and pin
 cherryview-pinctrl INT33FF:01: Changing the interrupt-line for pin 56 to 7

Note commit 9747070c11d6 ("Input: axp20x-pek - always register interrupt
handlers") was added as a work around for the power-button not being able
to wakeup the system. This relies on using the PMIC's connection to the
powerbutton but that only works on systems with the AXP288 PMIC.
Once this fix has been merged that workaround can be removed.

Cc: Yauhen Kharuzhy <jekhor@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 69 +++++++++++++++++++---
 1 file changed, 61 insertions(+), 8 deletions(-)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index a46f9e5a4748..491b234812cd 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1323,6 +1323,8 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 
 		if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
 			irq_set_handler_locked(d, handler);
+			dev_dbg(pctrl->dev, "Using interrupt-line %u for IRQ_TYPE_NONE IRQ on pin %u\n",
+				intsel, pin);
 			cctx->intr_lines[intsel] = pin;
 		}
 		raw_spin_unlock_irqrestore(&chv_lock, flags);
@@ -1332,17 +1334,73 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
 	return 0;
 }
 
+static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin)
+{
+	struct intel_community_context *cctx = &pctrl->context.communities[0];
+	const struct intel_community *community = &pctrl->communities[0];
+	u32 value, intsel;
+	int i;
+
+	value = chv_readl(pctrl, pin, CHV_PADCTRL0);
+	intsel = (value & CHV_PADCTRL0_INTSEL_MASK) >> CHV_PADCTRL0_INTSEL_SHIFT;
+
+	if (cctx->intr_lines[intsel] == pin)
+		return 0;
+
+	if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
+		dev_dbg(pctrl->dev, "Using interrupt-line %u for pin %u\n", intsel, pin);
+		cctx->intr_lines[intsel] = pin;
+		return 0;
+	}
+
+	/*
+	 * The interrupt-line selected by the BIOS is already in use by
+	 * another pin, this is a known BIOS bug found on several models.
+	 * But this may also be caused by Linux deciding to use a pin as
+	 * IRQ which was not expected to be used as such by the BIOS authors,
+	 * so log this at info level only.
+	 */
+	dev_info(pctrl->dev, "Interrupt-line %u is used by both pin %u and pin %u\n",
+		 intsel, cctx->intr_lines[intsel], pin);
+
+	if (chv_pad_locked(pctrl, pin))
+		return -EBUSY;
+
+	/*
+	 * The BIOS fills the interrupt lines from 0 counting up, start at
+	 * the other end to find a free interrupt-line to workaround this.
+	 */
+	for (i = community->nirqs - 1; i >= 0; i--) {
+		if (cctx->intr_lines[i] == INTR_LINE_UNUSED)
+			break;
+	}
+	if (i < 0)
+		return -EBUSY;
+
+	dev_info(pctrl->dev, "Changing the interrupt-line for pin %u to %d\n", pin, i);
+
+	value = (value & ~CHV_PADCTRL0_INTSEL_MASK) | (i << CHV_PADCTRL0_INTSEL_SHIFT);
+	chv_writel(pctrl, pin, CHV_PADCTRL0, value);
+	cctx->intr_lines[i] = pin;
+
+	return 0;
+}
+
 static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
 	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
-	struct intel_community_context *cctx = &pctrl->context.communities[0];
 	unsigned int pin = irqd_to_hwirq(d);
 	unsigned long flags;
 	u32 value;
+	int ret;
 
 	raw_spin_lock_irqsave(&chv_lock, flags);
 
+	ret = chv_gpio_set_intr_line(pctrl, pin);
+	if (ret)
+		goto out_unlock;
+
 	/*
 	 * Pins which can be used as shared interrupt are configured in
 	 * BIOS. Driver trusts BIOS configurations and assigns different
@@ -1377,20 +1435,15 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
 		chv_writel(pctrl, pin, CHV_PADCTRL1, value);
 	}
 
-	value = chv_readl(pctrl, pin, CHV_PADCTRL0);
-	value &= CHV_PADCTRL0_INTSEL_MASK;
-	value >>= CHV_PADCTRL0_INTSEL_SHIFT;
-
-	cctx->intr_lines[value] = pin;
-
 	if (type & IRQ_TYPE_EDGE_BOTH)
 		irq_set_handler_locked(d, handle_edge_irq);
 	else if (type & IRQ_TYPE_LEVEL_MASK)
 		irq_set_handler_locked(d, handle_level_irq);
 
+out_unlock:
 	raw_spin_unlock_irqrestore(&chv_lock, flags);
 
-	return 0;
+	return ret;
 }
 
 static void chv_gpio_irq_handler(struct irq_desc *desc)
-- 
2.31.1

Re: [PATCH 2/3] pinctrl: cherryview: Do not allow the same intr-line to be used by 2 pins

[Mika Westerberg]

On Thu, Nov 18, 2021 at 12:16:13AM +0100, Hans de Goede wrote:
> It is impossible to use the same intr-line for 2 pins, this will result
> in the interrupts only being delivered to the irq-handler for the pin for

IRQ handler

> which chv_gpio_irq_type() was called last.
> 
> The pinctrl-cherryview.c code relies on the BIOS to correctly setup the
> interrupt-line, but there is a BIOS bug on at least the Medion Akoya E1239T

interrupt line

(Ditto everywhere).

> and the GPD win models where both INT33FF:02 pin 8, used by the powerbutton

power button

> and INT33FF:02 pin 21 used as IRQ input for the accelerometer are mapped to
> interrupt-line 0.
> 
> This causes 2 problems:
> 1. The accelerometer IRQ does not work, since the power-button is probed

power button

(Ditto everywhere)

> later taking over the intr_lines[0] slot.
> 
> 2. Since the accelerometer IRQ is not marked as wakeup, interrupt-line 0
> gets masked on suspend, causing the power-button to not work to wake
> the system from suspend.
> 
> Likewise on the Lenovo Yogabook which has a touchscreen as keyboard
> and the keyboard half of the tablet also has a drawing tablet, the
> BIOS by default assigns the same interrupt-line to the GPIOs used
> for their interrupts.
> 
> Fix these problems by adding a check for this and assigning a new
> interrupt-line to the 2nd pin for which chv_gpio_irq_type() gets called.
> 
> With this fix in place the following 2 messages show up in dmesg on
> the Medion Akoya E1239T and the GPD win:
> 
>  cherryview-pinctrl INT33FF:02: [Firmware Bug]: interrupt-line 0 is used by both pin 21 and pin 8
>  cherryview-pinctrl INT33FF:02: Changing the interrupt-line for pin 8 to 15
> 
> And the following gets logged on the Lenovo Yogabook:
> 
>  cherryview-pinctrl INT33FF:01: Interrupt-line 0 is used by both pin 49 and pin
>  cherryview-pinctrl INT33FF:01: Changing the interrupt-line for pin 56 to 7
> 
> Note commit 9747070c11d6 ("Input: axp20x-pek - always register interrupt
> handlers") was added as a work around for the power-button not being able
> to wakeup the system. This relies on using the PMIC's connection to the
> powerbutton but that only works on systems with the AXP288 PMIC.
> Once this fix has been merged that workaround can be removed.
> 
> Cc: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 69 +++++++++++++++++++---
>  1 file changed, 61 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index a46f9e5a4748..491b234812cd 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -1323,6 +1323,8 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  
>  		if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
>  			irq_set_handler_locked(d, handler);
> +			dev_dbg(pctrl->dev, "Using interrupt-line %u for IRQ_TYPE_NONE IRQ on pin %u\n",
> +				intsel, pin);

"using interrupt line ..."

>  			cctx->intr_lines[intsel] = pin;
>  		}
>  		raw_spin_unlock_irqrestore(&chv_lock, flags);
> @@ -1332,17 +1334,73 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  	return 0;
>  }
>  
> +static int chv_gpio_set_intr_line(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> +	struct intel_community_context *cctx = &pctrl->context.communities[0];
> +	const struct intel_community *community = &pctrl->communities[0];
> +	u32 value, intsel;
> +	int i;
> +
> +	value = chv_readl(pctrl, pin, CHV_PADCTRL0);
> +	intsel = (value & CHV_PADCTRL0_INTSEL_MASK) >> CHV_PADCTRL0_INTSEL_SHIFT;
> +
> +	if (cctx->intr_lines[intsel] == pin)
> +		return 0;
> +
> +	if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
> +		dev_dbg(pctrl->dev, "Using interrupt-line %u for pin %u\n", intsel, pin);

"using interrupt line ..."

> +		cctx->intr_lines[intsel] = pin;
> +		return 0;
> +	}
> +
> +	/*
> +	 * The interrupt-line selected by the BIOS is already in use by
> +	 * another pin, this is a known BIOS bug found on several models.
> +	 * But this may also be caused by Linux deciding to use a pin as
> +	 * IRQ which was not expected to be used as such by the BIOS authors,
> +	 * so log this at info level only.
> +	 */
> +	dev_info(pctrl->dev, "Interrupt-line %u is used by both pin %u and pin %u\n",
> +		 intsel, cctx->intr_lines[intsel], pin);

"interrupt line %u ..."

> +
> +	if (chv_pad_locked(pctrl, pin))
> +		return -EBUSY;
> +
> +	/*
> +	 * The BIOS fills the interrupt lines from 0 counting up, start at
> +	 * the other end to find a free interrupt-line to workaround this.
> +	 */
> +	for (i = community->nirqs - 1; i >= 0; i--) {
> +		if (cctx->intr_lines[i] == INTR_LINE_UNUSED)
> +			break;
> +	}
> +	if (i < 0)
> +		return -EBUSY;
> +
> +	dev_info(pctrl->dev, "Changing the interrupt-line for pin %u to %d\n", pin, i);

"changing the ..."

> +
> +	value = (value & ~CHV_PADCTRL0_INTSEL_MASK) | (i << CHV_PADCTRL0_INTSEL_SHIFT);
> +	chv_writel(pctrl, pin, CHV_PADCTRL0, value);
> +	cctx->intr_lines[i] = pin;
> +
> +	return 0;
> +}
> +
>  static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
>  {
>  	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
>  	struct intel_pinctrl *pctrl = gpiochip_get_data(gc);
> -	struct intel_community_context *cctx = &pctrl->context.communities[0];
>  	unsigned int pin = irqd_to_hwirq(d);
>  	unsigned long flags;
>  	u32 value;
> +	int ret;
>  
>  	raw_spin_lock_irqsave(&chv_lock, flags);
>  
> +	ret = chv_gpio_set_intr_line(pctrl, pin);
> +	if (ret)
> +		goto out_unlock;
> +
>  	/*
>  	 * Pins which can be used as shared interrupt are configured in
>  	 * BIOS. Driver trusts BIOS configurations and assigns different
> @@ -1377,20 +1435,15 @@ static int chv_gpio_irq_type(struct irq_data *d, unsigned int type)
>  		chv_writel(pctrl, pin, CHV_PADCTRL1, value);
>  	}
>  
> -	value = chv_readl(pctrl, pin, CHV_PADCTRL0);
> -	value &= CHV_PADCTRL0_INTSEL_MASK;
> -	value >>= CHV_PADCTRL0_INTSEL_SHIFT;
> -
> -	cctx->intr_lines[value] = pin;
> -
>  	if (type & IRQ_TYPE_EDGE_BOTH)
>  		irq_set_handler_locked(d, handle_edge_irq);
>  	else if (type & IRQ_TYPE_LEVEL_MASK)
>  		irq_set_handler_locked(d, handle_level_irq);
>  
> +out_unlock:
>  	raw_spin_unlock_irqrestore(&chv_lock, flags);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  static void chv_gpio_irq_handler(struct irq_desc *desc)
> -- 
> 2.31.1

[PATCH 3/3] pinctrl: cherryview: Ignore INT33FF UID 5 ACPI device

[Hans de Goede]

Many Cherry Trail DSDTs have an extra INT33FF device with UID 5,
the intel_pinctrl_get_soc_data() call will fail for this extra
unknown UID, leading to the following error in dmesg:

 cherryview-pinctrl: probe of INT33FF:04 failed with error -61

Add a check for this extra UID and return -ENODEV for it to
silence this false-positive error message.

Cc: Yauhen Kharuzhy <jekhor@gmail.com>
Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/pinctrl/intel/pinctrl-cherryview.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
index 491b234812cd..ca6fe6115ecb 100644
--- a/drivers/pinctrl/intel/pinctrl-cherryview.c
+++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
@@ -1686,6 +1686,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
 	acpi_status status;
 	int i, ret, irq;
 
+	/* Cherry Trail DSDTs have an extra INT33FF device with UID 5, ignore */
+	if (!strcmp(adev->pnp.unique_id, "5"))
+		return -ENODEV;
+
 	soc_data = intel_pinctrl_get_soc_data(pdev);
 	if (IS_ERR(soc_data))
 		return PTR_ERR(soc_data);
-- 
2.31.1

Re: [PATCH 3/3] pinctrl: cherryview: Ignore INT33FF UID 5 ACPI device

[Mika Westerberg]

On Thu, Nov 18, 2021 at 12:16:14AM +0100, Hans de Goede wrote:
> Many Cherry Trail DSDTs have an extra INT33FF device with UID 5,
> the intel_pinctrl_get_soc_data() call will fail for this extra
> unknown UID, leading to the following error in dmesg:

Yeah, this may be the "DSW" or Deep Sleep Well community that is
typically not available for software to use at all. No idea why they
expose it on some systems.

>  cherryview-pinctrl: probe of INT33FF:04 failed with error -61
> 
> Add a check for this extra UID and return -ENODEV for it to
> silence this false-positive error message.
> 
> Cc: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>

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

Re: [PATCH 1/3] pinctrl: cherryview: Don't use pin/offset 0 to mark an interrupt-line as unused

[Mika Westerberg]

Hi Hans,

Few minor comments below.

On Thu, Nov 18, 2021 at 12:16:12AM +0100, Hans de Goede wrote:
> offset/pin 0 is a perfectly valid offset, so stop using it to have

Offset/pin (with capital O).

> the special meaning of interrupt-line not used in the intr_lines.

interrupt line

> Instead introduce a new special INTR_LINE_UNUSED value which is never
> a valid offset and use that to indicate unused interrupt-lines.

interrupt lines

> Cc: Yauhen Kharuzhy <jekhor@gmail.com>
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/pinctrl/intel/pinctrl-cherryview.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
> index 980099028cf8..a46f9e5a4748 100644
> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
> @@ -73,6 +73,8 @@ struct intel_pad_context {
>  	u32 padctrl1;
>  };
>  
> +#define INTR_LINE_UNUSED		U32_MAX
> +
>  /**
>   * struct intel_community_context - community context for Cherryview
>   * @intr_lines: Mapping between 16 HW interrupt wires and GPIO offset (in GPIO number space)
> @@ -812,7 +814,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
>  		/* Reset the interrupt mapping */
>  		for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++) {
>  			if (cctx->intr_lines[i] == offset) {
> -				cctx->intr_lines[i] = 0;
> +				cctx->intr_lines[i] = INTR_LINE_UNUSED;
>  				break;
>  			}
>  		}
> @@ -1319,7 +1321,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>  		else
>  			handler = handle_edge_irq;
>  
> -		if (!cctx->intr_lines[intsel]) {
> +		if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
>  			irq_set_handler_locked(d, handler);
>  			cctx->intr_lines[intsel] = pin;
>  		}
> @@ -1412,6 +1414,12 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
>  		unsigned int offset;
>  
>  		offset = cctx->intr_lines[intr_line];
> +		if (offset == INTR_LINE_UNUSED) {
> +			dev_err(pctrl->dev, "Interrupt on unused interrupt line %u\n",

Let's be consistent with the logging so no capital letter here =>
"interrupt on .."

> +				intr_line);
> +			continue;
> +		}
> +
>  		generic_handle_domain_irq(gc->irq.domain, offset);
>  	}
>  
> @@ -1620,9 +1628,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>  	struct intel_community *community;
>  	struct device *dev = &pdev->dev;
>  	struct acpi_device *adev = ACPI_COMPANION(dev);
> +	struct intel_community_context *cctx;
>  	struct intel_pinctrl *pctrl;
>  	acpi_status status;
> -	int ret, irq;
> +	int i, ret, irq;
>  
>  	soc_data = intel_pinctrl_get_soc_data(pdev);
>  	if (IS_ERR(soc_data))
> @@ -1663,6 +1672,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>  	if (!pctrl->context.communities)
>  		return -ENOMEM;
>  
> +	cctx = &pctrl->context.communities[0];
> +	for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++)
> +		cctx->intr_lines[i] = INTR_LINE_UNUSED;
> +
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
> -- 
> 2.31.1

Re: [PATCH 1/3] pinctrl: cherryview: Don't use pin/offset 0 to mark an interrupt-line as unused

[Hans de Goede]

Hi Mika,

On 11/18/21 11:13, Mika Westerberg wrote:
> Hi Hans,
> 
> Few minor comments below.

Thank you for the review. I'll send a new version addressing
all your remarks.

Regards,

Hans


> 
> On Thu, Nov 18, 2021 at 12:16:12AM +0100, Hans de Goede wrote:
>> offset/pin 0 is a perfectly valid offset, so stop using it to have
> 
> Offset/pin (with capital O).
> 
>> the special meaning of interrupt-line not used in the intr_lines.
> 
> interrupt line
> 
>> Instead introduce a new special INTR_LINE_UNUSED value which is never
>> a valid offset and use that to indicate unused interrupt-lines.
> 
> interrupt lines
> 
>> Cc: Yauhen Kharuzhy <jekhor@gmail.com>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>  drivers/pinctrl/intel/pinctrl-cherryview.c | 19 ++++++++++++++++---
>>  1 file changed, 16 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/pinctrl/intel/pinctrl-cherryview.c b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> index 980099028cf8..a46f9e5a4748 100644
>> --- a/drivers/pinctrl/intel/pinctrl-cherryview.c
>> +++ b/drivers/pinctrl/intel/pinctrl-cherryview.c
>> @@ -73,6 +73,8 @@ struct intel_pad_context {
>>  	u32 padctrl1;
>>  };
>>  
>> +#define INTR_LINE_UNUSED		U32_MAX
>> +
>>  /**
>>   * struct intel_community_context - community context for Cherryview
>>   * @intr_lines: Mapping between 16 HW interrupt wires and GPIO offset (in GPIO number space)
>> @@ -812,7 +814,7 @@ static int chv_gpio_request_enable(struct pinctrl_dev *pctldev,
>>  		/* Reset the interrupt mapping */
>>  		for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++) {
>>  			if (cctx->intr_lines[i] == offset) {
>> -				cctx->intr_lines[i] = 0;
>> +				cctx->intr_lines[i] = INTR_LINE_UNUSED;
>>  				break;
>>  			}
>>  		}
>> @@ -1319,7 +1321,7 @@ static unsigned chv_gpio_irq_startup(struct irq_data *d)
>>  		else
>>  			handler = handle_edge_irq;
>>  
>> -		if (!cctx->intr_lines[intsel]) {
>> +		if (cctx->intr_lines[intsel] == INTR_LINE_UNUSED) {
>>  			irq_set_handler_locked(d, handler);
>>  			cctx->intr_lines[intsel] = pin;
>>  		}
>> @@ -1412,6 +1414,12 @@ static void chv_gpio_irq_handler(struct irq_desc *desc)
>>  		unsigned int offset;
>>  
>>  		offset = cctx->intr_lines[intr_line];
>> +		if (offset == INTR_LINE_UNUSED) {
>> +			dev_err(pctrl->dev, "Interrupt on unused interrupt line %u\n",
> 
> Let's be consistent with the logging so no capital letter here =>
> "interrupt on .."
> 
>> +				intr_line);
>> +			continue;
>> +		}
>> +
>>  		generic_handle_domain_irq(gc->irq.domain, offset);
>>  	}
>>  
>> @@ -1620,9 +1628,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>>  	struct intel_community *community;
>>  	struct device *dev = &pdev->dev;
>>  	struct acpi_device *adev = ACPI_COMPANION(dev);
>> +	struct intel_community_context *cctx;
>>  	struct intel_pinctrl *pctrl;
>>  	acpi_status status;
>> -	int ret, irq;
>> +	int i, ret, irq;
>>  
>>  	soc_data = intel_pinctrl_get_soc_data(pdev);
>>  	if (IS_ERR(soc_data))
>> @@ -1663,6 +1672,10 @@ static int chv_pinctrl_probe(struct platform_device *pdev)
>>  	if (!pctrl->context.communities)
>>  		return -ENOMEM;
>>  
>> +	cctx = &pctrl->context.communities[0];
>> +	for (i = 0; i < ARRAY_SIZE(cctx->intr_lines); i++)
>> +		cctx->intr_lines[i] = INTR_LINE_UNUSED;
>> +
>>  	irq = platform_get_irq(pdev, 0);
>>  	if (irq < 0)
>>  		return irq;
>> -- 
>> 2.31.1
>