[1/2] gpio: xgene: add support to configure GPIO line as input, output or external IRQ pin
diff mbox

Message ID 1441963332-13329-2-git-send-email-yvo@apm.com
State New
Headers show

Commit Message

Y Vo Sept. 11, 2015, 9:22 a.m. UTC
Add support to configure GPIO line as input, output or external IRQ pin.

Signed-off-by: Y Vo <yvo@apm.com>
---
 drivers/gpio/gpio-xgene-sb.c |   33 ++++++++++++++++++++++++++-------
 1 files changed, 26 insertions(+), 7 deletions(-)

Comments

Linus Walleij Oct. 2, 2015, 9:51 a.m. UTC | #1
On Fri, Sep 11, 2015 at 2:22 AM, Y Vo <yvo@apm.com> wrote:

> Add support to configure GPIO line as input, output or external IRQ pin.
>
> Signed-off-by: Y Vo <yvo@apm.com>

Mostly OK but...

>  #define XGENE_MAX_GPIO_DS              22
>  #define XGENE_MAX_GPIO_DS_IRQ          6
> +#define XGENE_GPIO8_HWIRQ              0x48
> +#define XGENE_GPIO8_IDX                        8
(...)
> +static int xgene_irq_to_line(u32 irq)
> +{
> +       u32 offset = irq_get_irq_data(irq)->hwirq - XGENE_GPIO8_HWIRQ;
> +
> +       return (offset < XGENE_MAX_GPIO_DS_IRQ) ?
> +                       (offset + XGENE_GPIO8_IDX) : -EINVAL;
> +}

What is this hardcoded IRQ 0x48 business?

This patch needs kerneldoc for this xgene_irq_to_line() to explain
what exactly is going on, or noone will understand it enough to
debug or refactor the code.

I hope this 0x48 is not some way of compensating for Linux internal
IRQ offsets, that is what irqdomain is for.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Y Vo Oct. 2, 2015, 2:03 p.m. UTC | #2
On Fri, Oct 2, 2015 at 4:51 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 11, 2015 at 2:22 AM, Y Vo <yvo@apm.com> wrote:
>
>> Add support to configure GPIO line as input, output or external IRQ pin.
>>
>> Signed-off-by: Y Vo <yvo@apm.com>
>
> Mostly OK but...
>
>>  #define XGENE_MAX_GPIO_DS              22
>>  #define XGENE_MAX_GPIO_DS_IRQ          6
>> +#define XGENE_GPIO8_HWIRQ              0x48
>> +#define XGENE_GPIO8_IDX                        8
> (...)
>> +static int xgene_irq_to_line(u32 irq)
>> +{
>> +       u32 offset = irq_get_irq_data(irq)->hwirq - XGENE_GPIO8_HWIRQ;
>> +
>> +       return (offset < XGENE_MAX_GPIO_DS_IRQ) ?
>> +                       (offset + XGENE_GPIO8_IDX) : -EINVAL;
>> +}
>
> What is this hardcoded IRQ 0x48 business?

This is hardware irq number ~ GPIO_DS8. So we use it to get the gpio
number from hwirq. But we are preparing another version which is
better to configure the GPIO lines as interrupt or direction(in/out).

>
> This patch needs kerneldoc for this xgene_irq_to_line() to explain
> what exactly is going on, or noone will understand it enough to
> debug or refactor the code.
>
> I hope this 0x48 is not some way of compensating for Linux internal
> IRQ offsets, that is what irqdomain is for.
>
> Yours,
> Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/gpio/gpio-xgene-sb.c b/drivers/gpio/gpio-xgene-sb.c
index d57068b..76b15e0 100644
--- a/drivers/gpio/gpio-xgene-sb.c
+++ b/drivers/gpio/gpio-xgene-sb.c
@@ -32,6 +32,8 @@ 
 
 #define XGENE_MAX_GPIO_DS		22
 #define XGENE_MAX_GPIO_DS_IRQ		6
+#define XGENE_GPIO8_HWIRQ		0x48
+#define XGENE_GPIO8_IDX			8
 
 #define GPIO_MASK(x)			(1U << ((x) % 32))
 
@@ -82,11 +84,19 @@  static int apm_gpio_sb_to_irq(struct gpio_chip *gc, u32 gpio)
 	return -ENXIO;
 }
 
+static int xgene_irq_to_line(u32 irq)
+{
+	u32 offset = irq_get_irq_data(irq)->hwirq - XGENE_GPIO8_HWIRQ;
+
+	return (offset < XGENE_MAX_GPIO_DS_IRQ) ?
+			(offset + XGENE_GPIO8_IDX) : -EINVAL;
+}
+
 static int xgene_gpio_sb_probe(struct platform_device *pdev)
 {
 	struct xgene_gpio_sb *priv;
 	u32 ret, i;
-	u32 default_lines[] = {0x08, 0x09, 0x0A, 0x0B, 0x0C, 0x0D};
+	int virq, line;
 	struct resource *res;
 	void __iomem *regs;
 
@@ -109,20 +119,29 @@  static int xgene_gpio_sb_probe(struct platform_device *pdev)
 	priv->bgc.gc.to_irq = apm_gpio_sb_to_irq;
 	priv->bgc.gc.ngpio = XGENE_MAX_GPIO_DS;
 
-	priv->nirq = XGENE_MAX_GPIO_DS_IRQ;
-
 	priv->irq = devm_kzalloc(&pdev->dev, sizeof(u32) * XGENE_MAX_GPIO_DS,
 				   GFP_KERNEL);
 	if (!priv->irq)
 		return -ENOMEM;
 
-	for (i = 0; i < priv->nirq; i++) {
-		priv->irq[default_lines[i]] = platform_get_irq(pdev, i);
+	for (i = 0; i < XGENE_MAX_GPIO_DS_IRQ; i++) {
+		virq = platform_get_irq(pdev, i);
+		if (virq < 0)
+			break;
+		line = xgene_irq_to_line(virq);
+		if (line < XGENE_GPIO8_IDX)
+			break;
+		priv->irq[line] = virq;
 		xgene_gpio_set_bit(&priv->bgc, regs + MPA_GPIO_SEL_LO,
-                                   default_lines[i] * 2, 1);
-		xgene_gpio_set_bit(&priv->bgc, regs + MPA_GPIO_INT_LVL, i, 1);
+						line * 2, 1);
+		xgene_gpio_set_bit(&priv->bgc, regs + MPA_GPIO_INT_LVL,
+						(line - XGENE_GPIO8_IDX), 1);
+		dev_dbg(&pdev->dev,
+			"gpio line %d mapped as external irq pin\n", line);
 	}
 
+	priv->nirq = i;
+
 	platform_set_drvdata(pdev, priv);
 
 	ret = gpiochip_add(&priv->bgc.gc);