[PATCH] pinctrl: amd: Add support for additional GPIO

[PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

This Provide IRQ sharing for AMD's GPIO devices.And set IRQCHIP_SKIP_SET_WAKE flag.

Also, fix the build issue for devm_request_irq()

Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 50 ++++++++++++++++++++++++++-----------------
 drivers/pinctrl/pinctrl-amd.h |  8 ++++---
 2 files changed, 35 insertions(+), 23 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index aea310a..29ad8e1 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -186,7 +186,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *output_value;
 	char *output_enable;
 
-	for (bank = 0; bank < AMD_GPIO_TOTAL_BANKS; bank++) {
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
 		seq_printf(s, "GPIO bank%d\t", bank);
 
 		switch (bank) {
@@ -202,6 +202,10 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			i = 128;
 			pin_num = AMD_GPIO_PINS_BANK2 + i;
 			break;
+		case 3:
+			i = 192;
+			pin_num = AMD_GPIO_PINS_BANK3 + i;
+			break;
 		}
 
 		for (; i < pin_num; i++) {
@@ -244,17 +248,17 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 				interrupt_mask =
 					"interrupt is masked|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
 				wake_cntrl0 = "enable wakeup in S0i3 state|";
 			else
 				wake_cntrl0 = "disable wakeup in S0i3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S3))
 				wake_cntrl1 = "enable wakeup in S3 state|";
 			else
 				wake_cntrl1 = "disable wakeup in S3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S4))
 				wake_cntrl2 = "enable wakeup in S4/S5 state|";
 			else
 				wake_cntrl2 = "disable wakeup in S4/S5 state|";
@@ -479,9 +483,10 @@ static struct irq_chip amd_gpio_irqchip = {
 	.irq_unmask   = amd_gpio_irq_unmask,
 	.irq_eoi      = amd_gpio_irq_eoi,
 	.irq_set_type = amd_gpio_irq_set_type,
+	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
-static void amd_gpio_irq_handler(struct irq_desc *desc)
+static irqreturn_t amd_gpio_irq_handler(int irq, void *data)
 {
 	u32 i;
 	u32 off;
@@ -489,13 +494,9 @@ static void amd_gpio_irq_handler(struct irq_desc *desc)
 	u32 pin_reg;
 	u64 reg64;
 	int handled = 0;
-	unsigned int irq;
 	unsigned long flags;
-	struct irq_chip *chip = irq_desc_get_chip(desc);
-	struct gpio_chip *gc = irq_desc_get_handler_data(desc);
-	struct amd_gpio *gpio_dev = gpiochip_get_data(gc);
+	struct amd_gpio *gpio_dev = data;
 
-	chained_irq_enter(chip, desc);
 	/*enable GPIO interrupt again*/
 	spin_lock_irqsave(&gpio_dev->lock, flags);
 	reg = readl(gpio_dev->base + WAKE_INT_STATUS_REG1);
@@ -517,8 +518,9 @@ static void amd_gpio_irq_handler(struct irq_desc *desc)
 						(off * 4 + i) * 4);
 				if ((pin_reg & BIT(INTERRUPT_STS_OFF)) ||
 					(pin_reg & BIT(WAKE_STS_OFF))) {
-					irq = irq_find_mapping(gc->irqdomain,
-								off * 4 + i);
+					irq = irq_find_mapping(
+						gpio_dev->gc.irqdomain,
+						off * 4 + i);
 					generic_handle_irq(irq);
 					writel(pin_reg,
 						gpio_dev->base
@@ -529,16 +531,13 @@ static void amd_gpio_irq_handler(struct irq_desc *desc)
 		}
 	}
 
-	if (handled == 0)
-		handle_bad_irq(desc);
-
 	spin_lock_irqsave(&gpio_dev->lock, flags);
 	reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
 	reg |= EOI_MASK;
 	writel(reg, gpio_dev->base + WAKE_INT_MASTER_REG);
 	spin_unlock_irqrestore(&gpio_dev->lock, flags);
 
-	chained_irq_exit(chip, desc);
+	return 0;
 }
 
 static int amd_get_groups_count(struct pinctrl_dev *pctldev)
@@ -727,10 +726,14 @@ static struct pinctrl_desc amd_pinctrl_desc = {
 
 static int amd_gpio_probe(struct platform_device *pdev)
 {
+	int hwnum;
 	int ret = 0;
 	int irq_base;
 	struct resource *res;
 	struct amd_gpio *gpio_dev;
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+	ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
 
 	gpio_dev = devm_kzalloc(&pdev->dev,
 				sizeof(struct amd_gpio), GFP_KERNEL);
@@ -763,16 +766,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpio_dev->gc.set			= amd_gpio_set_value;
 	gpio_dev->gc.set_debounce	= amd_gpio_set_debounce;
 	gpio_dev->gc.dbg_show		= amd_gpio_dbg_show;
+	gpio_dev->gc.base               = gpio_dev->gc.ngpio * hwnum;
 
-	gpio_dev->gc.base			= 0;
 	gpio_dev->gc.label			= pdev->name;
 	gpio_dev->gc.owner			= THIS_MODULE;
 	gpio_dev->gc.parent			= &pdev->dev;
-	gpio_dev->gc.ngpio			= TOTAL_NUMBER_OF_PINS;
+	gpio_dev->gc.ngpio			= resource_size(res) / 4;
 #if defined(CONFIG_OF_GPIO)
 	gpio_dev->gc.of_node			= pdev->dev.of_node;
 #endif
 
+	gpio_dev->hwbank_num = gpio_dev->gc.ngpio / 64;
 	gpio_dev->groups = kerncz_groups;
 	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
 
@@ -789,7 +793,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
-				0, 0, TOTAL_NUMBER_OF_PINS);
+				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
@@ -809,7 +813,12 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpiochip_set_chained_irqchip(&gpio_dev->gc,
 				 &amd_gpio_irqchip,
 				 irq_base,
-				 amd_gpio_irq_handler);
+				 NULL);
+
+	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
+			       IRQF_SHARED, dev_name(&pdev->dev), gpio_dev);
+	if (ret)
+		goto out2;
 
 	platform_set_drvdata(pdev, gpio_dev);
 
@@ -829,6 +838,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	pinctrl_unregister(gpio_dev->pctrl);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 7bfea47..c03f778 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -13,13 +13,12 @@
 #ifndef _PINCTRL_AMD_H
 #define _PINCTRL_AMD_H
 
-#define TOTAL_NUMBER_OF_PINS	192
 #define AMD_GPIO_PINS_PER_BANK  64
-#define AMD_GPIO_TOTAL_BANKS    3
 
 #define AMD_GPIO_PINS_BANK0     63
 #define AMD_GPIO_PINS_BANK1     64
 #define AMD_GPIO_PINS_BANK2     56
+#define AMD_GPIO_PINS_BANK3     32
 
 #define WAKE_INT_MASTER_REG 0xfc
 #define EOI_MASK (1 << 29)
@@ -35,7 +34,9 @@
 #define ACTIVE_LEVEL_OFF		9
 #define INTERRUPT_ENABLE_OFF		11
 #define INTERRUPT_MASK_OFF		12
-#define WAKE_CNTRL_OFF			13
+#define WAKE_CNTRL_OFF_S0I3             13
+#define WAKE_CNTRL_OFF_S3               14
+#define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
 #define PULL_UP_SEL_OFF			19
@@ -93,6 +94,7 @@ struct amd_gpio {
 	u32 ngroups;
 	struct pinctrl_dev *pctrl;
 	struct gpio_chip        gc;
+	unsigned int            hwbank_num;
 	struct resource         *res;
 	struct platform_device  *pdev;
 };
-- 
2.7.4

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Wed, Nov 9, 2016 at 11:28 AM, Shah, Nehal-bakulchandra
<Nehal-bakulchandra.Shah@amd.com> wrote:

> This Provide IRQ sharing for AMD's GPIO devices.And set IRQCHIP_SKIP_SET_WAKE flag.
>
> Also, fix the build issue for devm_request_irq()
>
> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>

This is not looking correct at all.

> @@ -529,16 +531,13 @@ static void amd_gpio_irq_handler(struct irq_desc *desc)
>                 }
>         }
>
> -       if (handled == 0)
> -               handle_bad_irq(desc);
> -
>         spin_lock_irqsave(&gpio_dev->lock, flags);
>         reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
>         reg |= EOI_MASK;
>         writel(reg, gpio_dev->base + WAKE_INT_MASTER_REG);
>         spin_unlock_irqrestore(&gpio_dev->lock, flags);
>
> -       chained_irq_exit(chip, desc);
> +       return 0;

You are removing the chained interrupt handling for no good reason.
The commit message does not say why this is being done.

It's especially erroneous to remove chained_irq_exit() but not
chained_irq_enter(), but generally, NONE of them should be
removed this *is* a chained interrupt handler.

>         gpiochip_set_chained_irqchip(&gpio_dev->gc,
>                                  &amd_gpio_irqchip,
>                                  irq_base,
> -                                amd_gpio_irq_handler);
> +                                NULL);
> +
> +       ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
> +                              IRQF_SHARED, dev_name(&pdev->dev), gpio_dev);
> +       if (ret)
> +               goto out2;

This is just wrong. The gpiochip_set_chained_irqchip() should not
be called in combination with devm_request_irq() like this.

devm_request_irq() should not be used at all. Keep the chained
handler.

I'm worried that this patch has a "trial-and-error" quality, it seems
you don't really know what is going on.

Please investigate and send the patch with a commitlog that describes
exactly why you are doing what you are doing.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

On 11/14/2016 6:48 PM, Linus Walleij wrote:
> On Wed, Nov 9, 2016 at 11:28 AM, Shah, Nehal-bakulchandra
> <Nehal-bakulchandra.Shah@amd.com> wrote:
> 
>> This Provide IRQ sharing for AMD's GPIO devices.And set IRQCHIP_SKIP_SET_WAKE flag.
>>
>> Also, fix the build issue for devm_request_irq()
>>
>> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
> 
> This is not looking correct at all.
> 
>> @@ -529,16 +531,13 @@ static void amd_gpio_irq_handler(struct irq_desc *desc)
>>                 }
>>         }
>>
>> -       if (handled == 0)
>> -               handle_bad_irq(desc);
>> -
>>         spin_lock_irqsave(&gpio_dev->lock, flags);
>>         reg = readl(gpio_dev->base + WAKE_INT_MASTER_REG);
>>         reg |= EOI_MASK;
>>         writel(reg, gpio_dev->base + WAKE_INT_MASTER_REG);
>>         spin_unlock_irqrestore(&gpio_dev->lock, flags);
>>
>> -       chained_irq_exit(chip, desc);
>> +       return 0;
> 
> You are removing the chained interrupt handling for no good reason.
> The commit message does not say why this is being done.
> 
> It's especially erroneous to remove chained_irq_exit() but not
> chained_irq_enter(), but generally, NONE of them should be
> removed this *is* a chained interrupt handler.
> 
>>         gpiochip_set_chained_irqchip(&gpio_dev->gc,
>>                                  &amd_gpio_irqchip,
>>                                  irq_base,
>> -                                amd_gpio_irq_handler);
>> +                                NULL);
>> +
>> +       ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
>> +                              IRQF_SHARED, dev_name(&pdev->dev), gpio_dev);
>> +       if (ret)
>> +               goto out2;
> 
> This is just wrong. The gpiochip_set_chained_irqchip() should not
> be called in combination with devm_request_irq() like this.
> 
> devm_request_irq() should not be used at all. Keep the chained
> handler.
> 
> I'm worried that this patch has a "trial-and-error" quality, it seems
> you don't really know what is going on.
> 
> Please investigate and send the patch with a commitlog that describes
> exactly why you are doing what you are doing.
> 
> Yours,
> Linus Walleij
> 
Hi Linus,
 
I understood that chained irq and devm_irq can't be used together. Surely, 
I will rework on the patch. The reason behind using devm_irq is that in our 
upcoming platform bios may report two gpio controllers. Both will have same IRQs. 
In my understanding if I use chained irq handler, it will not be clear interrupt 
is due to which gpio controller.It may lead to serve irq twice. 
Looking at this constrain we changed the logic to devm irq.

Please let me know if anything is incorrect in my understanding or if there is 
a better way to handle this situation.
 
Your valuable inputs will help us a lot.
 
Thanks and Regards
Nehal

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Tue, Nov 22, 2016 at 11:19 AM, Shah, Nehal-bakulchandra
<Nehal-Bakulchandra.shah@amd.com> wrote:

> I understood that chained irq and devm_irq can't be used together. Surely,
> I will rework on the patch.

OK! :)

We have a case where devm_request_threaded_irq() is used like this,
that is called *nested* irqs, when an interrupt-handling thread is called
from within another interrupt-handling thread.

But since this handler is not threaded, but rather a hardirq, it is not
a viable option.

> The reason behind using devm_irq is that in our
> upcoming platform bios may report two gpio controllers. Both will have same IRQs.
> In my understanding if I use chained irq handler, it will not be clear interrupt
> is due to which gpio controller.It may lead to serve irq twice.

This is a shared IRQ line.

How stressful that it is shared between two cascaded irqchips!

Shared IRQs should nominally be handled by the leafs of the interrupt
handlers returning IRQ_HANDLED and the ones that did not
trigger an IRQ returning IRQ_NONE.

With a chained handler the assumption is that the flow handler
is not really returning anything, just dispatching down to the
consumer (device etc) to return IRQ_HANDLED if it was
their IRQ.

If you want to "bail out" of the chained handler (in your
case what you pass to gpiochip_set_chained_irqchip())
just check if the IRQ status register happens to be zero,
and in that case return; before even calling
chained_irq_enter().

I do not know if there is a way to mark a line as shared
in a chained flow handler!

The current assumption is that gpiochip_set_chained_irqchip()
is called for one or many *different* IRQs. That will work.
But if it is called twice for the *same* IRQ, we might need
some special work inside gpiochip_set_chained_irqchip()
to make this work properly with shared IRQs...

> Please let me know if anything is incorrect in my understanding or if there is
> a better way to handle this situation.

I think reading a bit in kernel/irq/chip.c to see what is really
happening with chained IRQs help a lot. If there are unclarities,
it needs to be discussed with the IRQchip maintainers
(Marc & Thomas).

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

On 11/22/2016 4:14 PM, Linus Walleij wrote:
> On Tue, Nov 22, 2016 at 11:19 AM, Shah, Nehal-bakulchandra
> <Nehal-Bakulchandra.shah@amd.com> wrote:
> 
>> I understood that chained irq and devm_irq can't be used together. Surely,
>> I will rework on the patch.
> 
> OK! :)
> 
> We have a case where devm_request_threaded_irq() is used like this,
> that is called *nested* irqs, when an interrupt-handling thread is called
> from within another interrupt-handling thread.
> 
> But since this handler is not threaded, but rather a hardirq, it is not
> a viable option.
> 
>> The reason behind using devm_irq is that in our
>> upcoming platform bios may report two gpio controllers. Both will have same IRQs.
>> In my understanding if I use chained irq handler, it will not be clear interrupt
>> is due to which gpio controller.It may lead to serve irq twice.
> 
> This is a shared IRQ line.
> 
> How stressful that it is shared between two cascaded irqchips!
> 
> Shared IRQs should nominally be handled by the leafs of the interrupt
> handlers returning IRQ_HANDLED and the ones that did not
> trigger an IRQ returning IRQ_NONE.
> 
> With a chained handler the assumption is that the flow handler
> is not really returning anything, just dispatching down to the
> consumer (device etc) to return IRQ_HANDLED if it was
> their IRQ.
> 
> If you want to "bail out" of the chained handler (in your
> case what you pass to gpiochip_set_chained_irqchip())
> just check if the IRQ status register happens to be zero,
> and in that case return; before even calling
> chained_irq_enter().
> 
> I do not know if there is a way to mark a line as shared
> in a chained flow handler!
> 
> The current assumption is that gpiochip_set_chained_irqchip()
> is called for one or many *different* IRQs. That will work.
> But if it is called twice for the *same* IRQ, we might need
> some special work inside gpiochip_set_chained_irqchip()
> to make this work properly with shared IRQs...
> 
>> Please let me know if anything is incorrect in my understanding or if there is
>> a better way to handle this situation.
> 
> I think reading a bit in kernel/irq/chip.c to see what is really
> happening with chained IRQs help a lot. If there are unclarities,
> it needs to be discussed with the IRQchip maintainers
> (Marc & Thomas).
> 
> Yours,
> Linus Walleij
> 
Hi Linus,

Thanks for your valuable input. Currently due to some limitations in IP we are restricting number of GPIO controller to 1. Hence Bios also will report 1 GPIO controller. So now to make it simple i will submit patch for Addtional GPIO Bank and adding IRQCHIP_SKIP_SET_WAKE flag. I will submit that patch in separate mail. Now regarding the another GPIO controller once limitation and behavior will be cleared i will make a separate patch.

Regards
Nehal Shah

[PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
IRQCHIP_SKIP_SET_WAKE flag.

Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 30 ++++++++++++++++++++----------
 drivers/pinctrl/pinctrl-amd.h |  8 +++++---
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index aea310a..93cc1ca 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -186,7 +186,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *output_value;
 	char *output_enable;
 
-	for (bank = 0; bank < AMD_GPIO_TOTAL_BANKS; bank++) {
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
 		seq_printf(s, "GPIO bank%d\t", bank);
 
 		switch (bank) {
@@ -202,8 +202,11 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			i = 128;
 			pin_num = AMD_GPIO_PINS_BANK2 + i;
 			break;
+		case 3:
+			i = 192;
+			pin_num = AMD_GPIO_PINS_BANK3 + i;
+			break;
 		}
-
 		for (; i < pin_num; i++) {
 			seq_printf(s, "pin%d\t", i);
 			spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -213,7 +216,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			if (pin_reg & BIT(INTERRUPT_ENABLE_OFF)) {
 				interrupt_enable = "interrupt is enabled|";
 
-				if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
+		if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
 				&& !(pin_reg & BIT(ACTIVE_LEVEL_OFF+1)))
 					active_level = "Active low|";
 				else if (pin_reg & BIT(ACTIVE_LEVEL_OFF)
@@ -244,17 +247,17 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 				interrupt_mask =
 					"interrupt is masked|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
 				wake_cntrl0 = "enable wakeup in S0i3 state|";
 			else
 				wake_cntrl0 = "disable wakeup in S0i3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S3))
 				wake_cntrl1 = "enable wakeup in S3 state|";
 			else
 				wake_cntrl1 = "disable wakeup in S3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S4))
 				wake_cntrl2 = "enable wakeup in S4/S5 state|";
 			else
 				wake_cntrl2 = "disable wakeup in S4/S5 state|";
@@ -479,6 +482,7 @@ static void amd_irq_ack(struct irq_data *d)
 	.irq_unmask   = amd_gpio_irq_unmask,
 	.irq_eoi      = amd_gpio_irq_eoi,
 	.irq_set_type = amd_gpio_irq_set_type,
+	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
 static void amd_gpio_irq_handler(struct irq_desc *desc)
@@ -727,10 +731,14 @@ static int amd_pinconf_group_set(struct pinctrl_dev *pctldev,
 
 static int amd_gpio_probe(struct platform_device *pdev)
 {
+	int hwnum;
 	int ret = 0;
 	int irq_base;
 	struct resource *res;
 	struct amd_gpio *gpio_dev;
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+	ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
 
 	gpio_dev = devm_kzalloc(&pdev->dev,
 				sizeof(struct amd_gpio), GFP_KERNEL);
@@ -763,16 +771,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpio_dev->gc.set			= amd_gpio_set_value;
 	gpio_dev->gc.set_debounce	= amd_gpio_set_debounce;
 	gpio_dev->gc.dbg_show		= amd_gpio_dbg_show;
+	gpio_dev->gc.base               = gpio_dev->gc.ngpio * hwnum;
 
-	gpio_dev->gc.base			= 0;
 	gpio_dev->gc.label			= pdev->name;
 	gpio_dev->gc.owner			= THIS_MODULE;
 	gpio_dev->gc.parent			= &pdev->dev;
-	gpio_dev->gc.ngpio			= TOTAL_NUMBER_OF_PINS;
+	gpio_dev->gc.ngpio			= resource_size(res) / 4;
 #if defined(CONFIG_OF_GPIO)
 	gpio_dev->gc.of_node			= pdev->dev.of_node;
 #endif
 
+	gpio_dev->hwbank_num = gpio_dev->gc.ngpio / 64;
 	gpio_dev->groups = kerncz_groups;
 	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
 
@@ -789,7 +798,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
-				0, 0, TOTAL_NUMBER_OF_PINS);
+				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
@@ -810,7 +819,6 @@ static int amd_gpio_probe(struct platform_device *pdev)
 				 &amd_gpio_irqchip,
 				 irq_base,
 				 amd_gpio_irq_handler);
-
 	platform_set_drvdata(pdev, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
@@ -829,6 +837,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	pinctrl_unregister(gpio_dev->pctrl);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 7bfea47..c03f778 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -13,13 +13,12 @@
 #ifndef _PINCTRL_AMD_H
 #define _PINCTRL_AMD_H
 
-#define TOTAL_NUMBER_OF_PINS	192
 #define AMD_GPIO_PINS_PER_BANK  64
-#define AMD_GPIO_TOTAL_BANKS    3
 
 #define AMD_GPIO_PINS_BANK0     63
 #define AMD_GPIO_PINS_BANK1     64
 #define AMD_GPIO_PINS_BANK2     56
+#define AMD_GPIO_PINS_BANK3     32
 
 #define WAKE_INT_MASTER_REG 0xfc
 #define EOI_MASK (1 << 29)
@@ -35,7 +34,9 @@
 #define ACTIVE_LEVEL_OFF		9
 #define INTERRUPT_ENABLE_OFF		11
 #define INTERRUPT_MASK_OFF		12
-#define WAKE_CNTRL_OFF			13
+#define WAKE_CNTRL_OFF_S0I3             13
+#define WAKE_CNTRL_OFF_S3               14
+#define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
 #define PULL_UP_SEL_OFF			19
@@ -93,6 +94,7 @@ struct amd_gpio {
 	u32 ngroups;
 	struct pinctrl_dev *pctrl;
 	struct gpio_chip        gc;
+	unsigned int            hwbank_num;
 	struct resource         *res;
 	struct platform_device  *pdev;
 };
-- 
1.9.1

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Tue, Nov 29, 2016 at 7:32 AM, Shah, Nehal-bakulchandra
<Nehal-Bakulchandra.shah@amd.com> wrote:

> This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
> IRQCHIP_SKIP_SET_WAKE flag.
>
> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>

(...)
>         struct amd_gpio *gpio_dev;
> +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +
> +       ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);

I guess this is the unique hardware instance number?

>         gpio_dev = devm_kzalloc(&pdev->dev,
>                                 sizeof(struct amd_gpio), GFP_KERNEL);
> @@ -763,16 +771,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
>         gpio_dev->gc.set                        = amd_gpio_set_value;
>         gpio_dev->gc.set_debounce       = amd_gpio_set_debounce;
>         gpio_dev->gc.dbg_show           = amd_gpio_dbg_show;
> +       gpio_dev->gc.base               = gpio_dev->gc.ngpio * hwnum;
>
> -       gpio_dev->gc.base                       = 0;

Please don't do this. Instead set .base to -1 so you get dynamic assignment
of GPIO numbers.

Suggestion: maybe you could augment gc.label to reflect instance number
instead?

That way the stuff from "lsgpio" (tools/gpio/lsgpio.c) will look nice and
identifiable.

Apart from that it looks good.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

On 11/30/2016 6:40 PM, Linus Walleij wrote:
> On Tue, Nov 29, 2016 at 7:32 AM, Shah, Nehal-bakulchandra
> <Nehal-Bakulchandra.shah@amd.com> wrote:
> 
>> This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
>> IRQCHIP_SKIP_SET_WAKE flag.
>>
>> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
>> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
> 
> (...)
>>         struct amd_gpio *gpio_dev;
>> +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
>> +
>> +       ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
> 
> I guess this is the unique hardware instance number?
> 
>>         gpio_dev = devm_kzalloc(&pdev->dev,
>>                                 sizeof(struct amd_gpio), GFP_KERNEL);
>> @@ -763,16 +771,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
>>         gpio_dev->gc.set                        = amd_gpio_set_value;
>>         gpio_dev->gc.set_debounce       = amd_gpio_set_debounce;
>>         gpio_dev->gc.dbg_show           = amd_gpio_dbg_show;
>> +       gpio_dev->gc.base               = gpio_dev->gc.ngpio * hwnum;
>>
>> -       gpio_dev->gc.base                       = 0;
> 
> Please don't do this. Instead set .base to -1 so you get dynamic assignment
> of GPIO numbers.
> 
> Suggestion: maybe you could augment gc.label to reflect instance number
> instead?
> 
> That way the stuff from "lsgpio" (tools/gpio/lsgpio.c) will look nice and
> identifiable.
> 
> Apart from that it looks good.
> 
> Yours,
> Linus Walleij
> 



Hi Linus

Thanks for the review. Will resubmit the patch with making .base to -1.

Regarding suggestion for gc.label will take care when submitting patch for more than one gpio controller.

Thanks 
Nehal

[PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

This patch adds support for new Bank and add IRQCHIP_SKIP_SET_WAKE flag.

Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 30 ++++++++++++++++++++----------
 drivers/pinctrl/pinctrl-amd.h |  8 +++++---
 2 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index aea310a..93cc1ca 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -186,7 +186,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *output_value;
 	char *output_enable;
 
-	for (bank = 0; bank < AMD_GPIO_TOTAL_BANKS; bank++) {
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
 		seq_printf(s, "GPIO bank%d\t", bank);
 
 		switch (bank) {
@@ -202,8 +202,11 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			i = 128;
 			pin_num = AMD_GPIO_PINS_BANK2 + i;
 			break;
+		case 3:
+			i = 192;
+			pin_num = AMD_GPIO_PINS_BANK3 + i;
+			break;
 		}
-
 		for (; i < pin_num; i++) {
 			seq_printf(s, "pin%d\t", i);
 			spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -213,7 +216,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			if (pin_reg & BIT(INTERRUPT_ENABLE_OFF)) {
 				interrupt_enable = "interrupt is enabled|";
 
-				if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
+		if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
 				&& !(pin_reg & BIT(ACTIVE_LEVEL_OFF+1)))
 					active_level = "Active low|";
 				else if (pin_reg & BIT(ACTIVE_LEVEL_OFF)
@@ -244,17 +247,17 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 				interrupt_mask =
 					"interrupt is masked|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
 				wake_cntrl0 = "enable wakeup in S0i3 state|";
 			else
 				wake_cntrl0 = "disable wakeup in S0i3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S3))
 				wake_cntrl1 = "enable wakeup in S3 state|";
 			else
 				wake_cntrl1 = "disable wakeup in S3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S4))
 				wake_cntrl2 = "enable wakeup in S4/S5 state|";
 			else
 				wake_cntrl2 = "disable wakeup in S4/S5 state|";
@@ -479,6 +482,7 @@ static void amd_irq_ack(struct irq_data *d)
 	.irq_unmask   = amd_gpio_irq_unmask,
 	.irq_eoi      = amd_gpio_irq_eoi,
 	.irq_set_type = amd_gpio_irq_set_type,
+	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
 static void amd_gpio_irq_handler(struct irq_desc *desc)
@@ -727,10 +731,14 @@ static int amd_pinconf_group_set(struct pinctrl_dev *pctldev,
 
 static int amd_gpio_probe(struct platform_device *pdev)
 {
+	int hwnum;
 	int ret = 0;
 	int irq_base;
 	struct resource *res;
 	struct amd_gpio *gpio_dev;
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+	ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
 
 	gpio_dev = devm_kzalloc(&pdev->dev,
 				sizeof(struct amd_gpio), GFP_KERNEL);
@@ -763,16 +771,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpio_dev->gc.set			= amd_gpio_set_value;
 	gpio_dev->gc.set_debounce	= amd_gpio_set_debounce;
 	gpio_dev->gc.dbg_show		= amd_gpio_dbg_show;
+	gpio_dev->gc.base               = -1;
 
-	gpio_dev->gc.base			= 0;
 	gpio_dev->gc.label			= pdev->name;
 	gpio_dev->gc.owner			= THIS_MODULE;
 	gpio_dev->gc.parent			= &pdev->dev;
-	gpio_dev->gc.ngpio			= TOTAL_NUMBER_OF_PINS;
+	gpio_dev->gc.ngpio			= resource_size(res) / 4;
 #if defined(CONFIG_OF_GPIO)
 	gpio_dev->gc.of_node			= pdev->dev.of_node;
 #endif
 
+	gpio_dev->hwbank_num = gpio_dev->gc.ngpio / 64;
 	gpio_dev->groups = kerncz_groups;
 	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
 
@@ -789,7 +798,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
-				0, 0, TOTAL_NUMBER_OF_PINS);
+				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
@@ -810,7 +819,6 @@ static int amd_gpio_probe(struct platform_device *pdev)
 				 &amd_gpio_irqchip,
 				 irq_base,
 				 amd_gpio_irq_handler);
-
 	platform_set_drvdata(pdev, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
@@ -829,6 +837,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	pinctrl_unregister(gpio_dev->pctrl);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 7bfea47..c03f778 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -13,13 +13,12 @@
 #ifndef _PINCTRL_AMD_H
 #define _PINCTRL_AMD_H
 
-#define TOTAL_NUMBER_OF_PINS	192
 #define AMD_GPIO_PINS_PER_BANK  64
-#define AMD_GPIO_TOTAL_BANKS    3
 
 #define AMD_GPIO_PINS_BANK0     63
 #define AMD_GPIO_PINS_BANK1     64
 #define AMD_GPIO_PINS_BANK2     56
+#define AMD_GPIO_PINS_BANK3     32
 
 #define WAKE_INT_MASTER_REG 0xfc
 #define EOI_MASK (1 << 29)
@@ -35,7 +34,9 @@
 #define ACTIVE_LEVEL_OFF		9
 #define INTERRUPT_ENABLE_OFF		11
 #define INTERRUPT_MASK_OFF		12
-#define WAKE_CNTRL_OFF			13
+#define WAKE_CNTRL_OFF_S0I3             13
+#define WAKE_CNTRL_OFF_S3               14
+#define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
 #define PULL_UP_SEL_OFF			19
@@ -93,6 +94,7 @@ struct amd_gpio {
 	u32 ngroups;
 	struct pinctrl_dev *pctrl;
 	struct gpio_chip        gc;
+	unsigned int            hwbank_num;
 	struct resource         *res;
 	struct platform_device  *pdev;
 };
-- 
1.9.1

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Thu, Dec 1, 2016 at 9:13 AM, Shah, Nehal-bakulchandra
<Nehal-Bakulchandra.shah@amd.com> wrote:

> This patch adds support for new Bank and add IRQCHIP_SKIP_SET_WAKE flag.
>
> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>

(...)
>  static int amd_gpio_probe(struct platform_device *pdev)
>  {
> +       int hwnum;
>         int ret = 0;
>         int irq_base;
>         struct resource *res;
>         struct amd_gpio *gpio_dev;
> +       struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
> +
> +       ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
>
>         gpio_dev = devm_kzalloc(&pdev->dev,
>                                 sizeof(struct amd_gpio), GFP_KERNEL);
> @@ -763,16 +771,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
>         gpio_dev->gc.set                        = amd_gpio_set_value;
>         gpio_dev->gc.set_debounce       = amd_gpio_set_debounce;
>         gpio_dev->gc.dbg_show           = amd_gpio_dbg_show;
> +       gpio_dev->gc.base               = -1;

Good!

But now hwnum is an unused variable.

Yours,
Linus Walleij

[PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

This patch adds support for new Bank and adds IRQCHIP_SKIP_SET_WAKE flag.

Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 25 +++++++++++++++----------
 drivers/pinctrl/pinctrl-amd.h |  8 +++++---
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index aea310a..1899b90 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -186,7 +186,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *output_value;
 	char *output_enable;
 
-	for (bank = 0; bank < AMD_GPIO_TOTAL_BANKS; bank++) {
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
 		seq_printf(s, "GPIO bank%d\t", bank);
 
 		switch (bank) {
@@ -202,8 +202,11 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			i = 128;
 			pin_num = AMD_GPIO_PINS_BANK2 + i;
 			break;
+		case 3:
+			i = 192;
+			pin_num = AMD_GPIO_PINS_BANK3 + i;
+			break;
 		}
-
 		for (; i < pin_num; i++) {
 			seq_printf(s, "pin%d\t", i);
 			spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -213,7 +216,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			if (pin_reg & BIT(INTERRUPT_ENABLE_OFF)) {
 				interrupt_enable = "interrupt is enabled|";
 
-				if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
+		if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
 				&& !(pin_reg & BIT(ACTIVE_LEVEL_OFF+1)))
 					active_level = "Active low|";
 				else if (pin_reg & BIT(ACTIVE_LEVEL_OFF)
@@ -244,17 +247,17 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 				interrupt_mask =
 					"interrupt is masked|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
 				wake_cntrl0 = "enable wakeup in S0i3 state|";
 			else
 				wake_cntrl0 = "disable wakeup in S0i3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S3))
 				wake_cntrl1 = "enable wakeup in S3 state|";
 			else
 				wake_cntrl1 = "disable wakeup in S3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S4))
 				wake_cntrl2 = "enable wakeup in S4/S5 state|";
 			else
 				wake_cntrl2 = "disable wakeup in S4/S5 state|";
@@ -479,6 +482,7 @@ static void amd_irq_ack(struct irq_data *d)
 	.irq_unmask   = amd_gpio_irq_unmask,
 	.irq_eoi      = amd_gpio_irq_eoi,
 	.irq_set_type = amd_gpio_irq_set_type,
+	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
 static void amd_gpio_irq_handler(struct irq_desc *desc)
@@ -764,15 +768,16 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpio_dev->gc.set_debounce	= amd_gpio_set_debounce;
 	gpio_dev->gc.dbg_show		= amd_gpio_dbg_show;
 
-	gpio_dev->gc.base			= 0;
+	gpio_dev->gc.base		= -1;
 	gpio_dev->gc.label			= pdev->name;
 	gpio_dev->gc.owner			= THIS_MODULE;
 	gpio_dev->gc.parent			= &pdev->dev;
-	gpio_dev->gc.ngpio			= TOTAL_NUMBER_OF_PINS;
+	gpio_dev->gc.ngpio			= resource_size(res) / 4;
 #if defined(CONFIG_OF_GPIO)
 	gpio_dev->gc.of_node			= pdev->dev.of_node;
 #endif
 
+	gpio_dev->hwbank_num = gpio_dev->gc.ngpio / 64;
 	gpio_dev->groups = kerncz_groups;
 	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
 
@@ -789,7 +794,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
-				0, 0, TOTAL_NUMBER_OF_PINS);
+				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
@@ -810,7 +815,6 @@ static int amd_gpio_probe(struct platform_device *pdev)
 				 &amd_gpio_irqchip,
 				 irq_base,
 				 amd_gpio_irq_handler);
-
 	platform_set_drvdata(pdev, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
@@ -829,6 +833,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	pinctrl_unregister(gpio_dev->pctrl);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 7bfea47..c03f778 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -13,13 +13,12 @@
 #ifndef _PINCTRL_AMD_H
 #define _PINCTRL_AMD_H
 
-#define TOTAL_NUMBER_OF_PINS	192
 #define AMD_GPIO_PINS_PER_BANK  64
-#define AMD_GPIO_TOTAL_BANKS    3
 
 #define AMD_GPIO_PINS_BANK0     63
 #define AMD_GPIO_PINS_BANK1     64
 #define AMD_GPIO_PINS_BANK2     56
+#define AMD_GPIO_PINS_BANK3     32
 
 #define WAKE_INT_MASTER_REG 0xfc
 #define EOI_MASK (1 << 29)
@@ -35,7 +34,9 @@
 #define ACTIVE_LEVEL_OFF		9
 #define INTERRUPT_ENABLE_OFF		11
 #define INTERRUPT_MASK_OFF		12
-#define WAKE_CNTRL_OFF			13
+#define WAKE_CNTRL_OFF_S0I3             13
+#define WAKE_CNTRL_OFF_S3               14
+#define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
 #define PULL_UP_SEL_OFF			19
@@ -93,6 +94,7 @@ struct amd_gpio {
 	u32 ngroups;
 	struct pinctrl_dev *pctrl;
 	struct gpio_chip        gc;
+	unsigned int            hwbank_num;
 	struct resource         *res;
 	struct platform_device  *pdev;
 };
-- 
1.9.1

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

Resubmitting this patch. Found one alignment issue.

Sorry for inconvenience.


This patch adds support for new Bank and adds IRQCHIP_SKIP_SET_WAKE flag.

Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 25 +++++++++++++++----------
 drivers/pinctrl/pinctrl-amd.h |  8 +++++---
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index aea310a..1899b90 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -186,7 +186,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *output_value;
 	char *output_enable;
 
-	for (bank = 0; bank < AMD_GPIO_TOTAL_BANKS; bank++) {
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
 		seq_printf(s, "GPIO bank%d\t", bank);
 
 		switch (bank) {
@@ -202,8 +202,11 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			i = 128;
 			pin_num = AMD_GPIO_PINS_BANK2 + i;
 			break;
+		case 3:
+			i = 192;
+			pin_num = AMD_GPIO_PINS_BANK3 + i;
+			break;
 		}
-
 		for (; i < pin_num; i++) {
 			seq_printf(s, "pin%d\t", i);
 			spin_lock_irqsave(&gpio_dev->lock, flags);
@@ -213,7 +216,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 			if (pin_reg & BIT(INTERRUPT_ENABLE_OFF)) {
 				interrupt_enable = "interrupt is enabled|";
 
-				if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
+		if (!(pin_reg & BIT(ACTIVE_LEVEL_OFF))
 				&& !(pin_reg & BIT(ACTIVE_LEVEL_OFF+1)))
 					active_level = "Active low|";
 				else if (pin_reg & BIT(ACTIVE_LEVEL_OFF)
@@ -244,17 +247,17 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 				interrupt_mask =
 					"interrupt is masked|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
 				wake_cntrl0 = "enable wakeup in S0i3 state|";
 			else
 				wake_cntrl0 = "disable wakeup in S0i3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S3))
 				wake_cntrl1 = "enable wakeup in S3 state|";
 			else
 				wake_cntrl1 = "disable wakeup in S3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S4))
 				wake_cntrl2 = "enable wakeup in S4/S5 state|";
 			else
 				wake_cntrl2 = "disable wakeup in S4/S5 state|";
@@ -479,6 +482,7 @@ static void amd_irq_ack(struct irq_data *d)
 	.irq_unmask   = amd_gpio_irq_unmask,
 	.irq_eoi      = amd_gpio_irq_eoi,
 	.irq_set_type = amd_gpio_irq_set_type,
+	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
 static void amd_gpio_irq_handler(struct irq_desc *desc)
@@ -764,15 +768,16 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpio_dev->gc.set_debounce	= amd_gpio_set_debounce;
 	gpio_dev->gc.dbg_show		= amd_gpio_dbg_show;
 
-	gpio_dev->gc.base			= 0;
+	gpio_dev->gc.base			= -1;
 	gpio_dev->gc.label			= pdev->name;
 	gpio_dev->gc.owner			= THIS_MODULE;
 	gpio_dev->gc.parent			= &pdev->dev;
-	gpio_dev->gc.ngpio			= TOTAL_NUMBER_OF_PINS;
+	gpio_dev->gc.ngpio			= resource_size(res) / 4;
 #if defined(CONFIG_OF_GPIO)
 	gpio_dev->gc.of_node			= pdev->dev.of_node;
 #endif
 
+	gpio_dev->hwbank_num = gpio_dev->gc.ngpio / 64;
 	gpio_dev->groups = kerncz_groups;
 	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
 
@@ -789,7 +794,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
-				0, 0, TOTAL_NUMBER_OF_PINS);
+				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
@@ -810,7 +815,6 @@ static int amd_gpio_probe(struct platform_device *pdev)
 				 &amd_gpio_irqchip,
 				 irq_base,
 				 amd_gpio_irq_handler);
-
 	platform_set_drvdata(pdev, gpio_dev);
 
 	dev_dbg(&pdev->dev, "amd gpio driver loaded\n");
@@ -829,6 +833,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	pinctrl_unregister(gpio_dev->pctrl);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 7bfea47..c03f778 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -13,13 +13,12 @@
 #ifndef _PINCTRL_AMD_H
 #define _PINCTRL_AMD_H
 
-#define TOTAL_NUMBER_OF_PINS	192
 #define AMD_GPIO_PINS_PER_BANK  64
-#define AMD_GPIO_TOTAL_BANKS    3
 
 #define AMD_GPIO_PINS_BANK0     63
 #define AMD_GPIO_PINS_BANK1     64
 #define AMD_GPIO_PINS_BANK2     56
+#define AMD_GPIO_PINS_BANK3     32
 
 #define WAKE_INT_MASTER_REG 0xfc
 #define EOI_MASK (1 << 29)
@@ -35,7 +34,9 @@
 #define ACTIVE_LEVEL_OFF		9
 #define INTERRUPT_ENABLE_OFF		11
 #define INTERRUPT_MASK_OFF		12
-#define WAKE_CNTRL_OFF			13
+#define WAKE_CNTRL_OFF_S0I3             13
+#define WAKE_CNTRL_OFF_S3               14
+#define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
 #define PULL_UP_SEL_OFF			19
@@ -93,6 +94,7 @@ struct amd_gpio {
 	u32 ngroups;
 	struct pinctrl_dev *pctrl;
 	struct gpio_chip        gc;
+	unsigned int            hwbank_num;
 	struct resource         *res;
 	struct platform_device  *pdev;
 };
-- 
1.9.1

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Tue, Dec 6, 2016 at 7:47 AM, Shah, Nehal-bakulchandra
<Nehal-Bakulchandra.shah@amd.com> wrote:

> This patch adds support for new Bank and adds IRQCHIP_SKIP_SET_WAKE flag.
>
> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>

Patch applied.

Yours,
Linus Walleij

[PATCH] pinctrl: amd: Add support for additional GPIO

[Shah, Nehal-bakulchandra]

This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
IRQCHIP_SKIP_SET_WAKE flag.

Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>
---
 drivers/pinctrl/pinctrl-amd.c | 28 ++++++++++++++++++++--------
 drivers/pinctrl/pinctrl-amd.h |  7 ++++---
 2 files changed, 24 insertions(+), 11 deletions(-)

diff --git a/drivers/pinctrl/pinctrl-amd.c b/drivers/pinctrl/pinctrl-amd.c
index aea310a..e3189ef4 100644
--- a/drivers/pinctrl/pinctrl-amd.c
+++ b/drivers/pinctrl/pinctrl-amd.c
@@ -186,7 +186,7 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 	char *output_value;
 	char *output_enable;
 
-	for (bank = 0; bank < AMD_GPIO_TOTAL_BANKS; bank++) {
+	for (bank = 0; bank < gpio_dev->hwbank_num; bank++) {
 		seq_printf(s, "GPIO bank%d\t", bank);
 
 		switch (bank) {
@@ -244,17 +244,17 @@ static void amd_gpio_dbg_show(struct seq_file *s, struct gpio_chip *gc)
 				interrupt_mask =
 					"interrupt is masked|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S0I3))
 				wake_cntrl0 = "enable wakeup in S0i3 state|";
 			else
 				wake_cntrl0 = "disable wakeup in S0i3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S3))
 				wake_cntrl1 = "enable wakeup in S3 state|";
 			else
 				wake_cntrl1 = "disable wakeup in S3 state|";
 
-			if (pin_reg & BIT(WAKE_CNTRL_OFF))
+			if (pin_reg & BIT(WAKE_CNTRL_OFF_S4))
 				wake_cntrl2 = "enable wakeup in S4/S5 state|";
 			else
 				wake_cntrl2 = "disable wakeup in S4/S5 state|";
@@ -479,6 +479,7 @@ static void amd_irq_ack(struct irq_data *d)
 	.irq_unmask   = amd_gpio_irq_unmask,
 	.irq_eoi      = amd_gpio_irq_eoi,
 	.irq_set_type = amd_gpio_irq_set_type,
+	.flags        = IRQCHIP_SKIP_SET_WAKE,
 };
 
 static void amd_gpio_irq_handler(struct irq_desc *desc)
@@ -727,10 +728,14 @@ static int amd_pinconf_group_set(struct pinctrl_dev *pctldev,
 
 static int amd_gpio_probe(struct platform_device *pdev)
 {
+	int hwnum;
 	int ret = 0;
 	int irq_base;
 	struct resource *res;
 	struct amd_gpio *gpio_dev;
+	struct acpi_device *adev = ACPI_COMPANION(&pdev->dev);
+
+	ret = kstrtoint(acpi_device_uid(adev), 2, &hwnum);
 
 	gpio_dev = devm_kzalloc(&pdev->dev,
 				sizeof(struct amd_gpio), GFP_KERNEL);
@@ -763,16 +768,17 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpio_dev->gc.set			= amd_gpio_set_value;
 	gpio_dev->gc.set_debounce	= amd_gpio_set_debounce;
 	gpio_dev->gc.dbg_show		= amd_gpio_dbg_show;
+	gpio_dev->gc.base               = gpio_dev->gc.ngpio * hwnum;
 
-	gpio_dev->gc.base			= 0;
 	gpio_dev->gc.label			= pdev->name;
 	gpio_dev->gc.owner			= THIS_MODULE;
 	gpio_dev->gc.parent			= &pdev->dev;
-	gpio_dev->gc.ngpio			= TOTAL_NUMBER_OF_PINS;
+	gpio_dev->gc.ngpio			= resource_size(res) / 4;
 #if defined(CONFIG_OF_GPIO)
 	gpio_dev->gc.of_node			= pdev->dev.of_node;
 #endif
 
+	gpio_dev->hwbank_num = gpio_dev->gc.ngpio / 64;
 	gpio_dev->groups = kerncz_groups;
 	gpio_dev->ngroups = ARRAY_SIZE(kerncz_groups);
 
@@ -789,7 +795,7 @@ static int amd_gpio_probe(struct platform_device *pdev)
 		return ret;
 
 	ret = gpiochip_add_pin_range(&gpio_dev->gc, dev_name(&pdev->dev),
-				0, 0, TOTAL_NUMBER_OF_PINS);
+				0, 0, gpio_dev->gc.ngpio);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to add pin range\n");
 		goto out2;
@@ -809,7 +815,12 @@ static int amd_gpio_probe(struct platform_device *pdev)
 	gpiochip_set_chained_irqchip(&gpio_dev->gc,
 				 &amd_gpio_irqchip,
 				 irq_base,
-				 amd_gpio_irq_handler);
+				 NULL);
+
+	ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
+			       IRQF_SHARED, dev_name(&pdev->dev), gpio_dev);
+	if (ret)
+		goto out2;
 
 	platform_set_drvdata(pdev, gpio_dev);
 
@@ -829,6 +840,7 @@ static int amd_gpio_remove(struct platform_device *pdev)
 	gpio_dev = platform_get_drvdata(pdev);
 
 	gpiochip_remove(&gpio_dev->gc);
+	pinctrl_unregister(gpio_dev->pctrl);
 
 	return 0;
 }
diff --git a/drivers/pinctrl/pinctrl-amd.h b/drivers/pinctrl/pinctrl-amd.h
index 7bfea47..0ed47c6 100644
--- a/drivers/pinctrl/pinctrl-amd.h
+++ b/drivers/pinctrl/pinctrl-amd.h
@@ -13,9 +13,7 @@
 #ifndef _PINCTRL_AMD_H
 #define _PINCTRL_AMD_H
 
-#define TOTAL_NUMBER_OF_PINS	192
 #define AMD_GPIO_PINS_PER_BANK  64
-#define AMD_GPIO_TOTAL_BANKS    3
 
 #define AMD_GPIO_PINS_BANK0     63
 #define AMD_GPIO_PINS_BANK1     64
@@ -35,7 +33,9 @@
 #define ACTIVE_LEVEL_OFF		9
 #define INTERRUPT_ENABLE_OFF		11
 #define INTERRUPT_MASK_OFF		12
-#define WAKE_CNTRL_OFF			13
+#define WAKE_CNTRL_OFF_S0I3             13
+#define WAKE_CNTRL_OFF_S3               14
+#define WAKE_CNTRL_OFF_S4               15
 #define PIN_STS_OFF			16
 #define DRV_STRENGTH_SEL_OFF		17
 #define PULL_UP_SEL_OFF			19
@@ -93,6 +93,7 @@ struct amd_gpio {
 	u32 ngroups;
 	struct pinctrl_dev *pctrl;
 	struct gpio_chip        gc;
+	unsigned int            hwbank_num;
 	struct resource         *res;
 	struct platform_device  *pdev;
 };
-- 
1.9.1

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Fri, Nov 4, 2016 at 4:46 AM, Shah, Nehal-bakulchandra
<Nehal-bakulchandra.Shah@amd.com> wrote:

> This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
> IRQCHIP_SKIP_SET_WAKE flag.
>
> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>

Patch applied.

Every time there is a patch to the AMD driver it comes from a new
person inside AMD, how do you actually manage this? It is better
for building trust in the long run if a person steps up as maintainer
and take a bit of long term responsibility.

Yours,
Linus Walleij

Re: [PATCH] pinctrl: amd: Add support for additional GPIO

[Linus Walleij]

On Fri, Nov 4, 2016 at 4:46 AM, Shah, Nehal-bakulchandra
<Nehal-bakulchandra.Shah@amd.com> wrote:

> This patch provides support for additional GPIO devices and also provides IRQ sharing for AMD's GPIO devices and set
> IRQCHIP_SKIP_SET_WAKE flag.
>
> Reviewed-by: S-k, Shyam-sundar <Shyam-sundar.S-k@amd.com>
> Signed-off-by: Nehal Shah <Nehal-bakulchandra.Shah@amd.com>

So the build servers are protesting against this patch because:

> +       ret = devm_request_irq(&pdev->dev, irq_base, amd_gpio_irq_handler,
> +                              IRQF_SHARED, dev_name(&pdev->dev), gpio_dev);

This is assigning amd_gpio_irq_handler() as IRQ handler, but that
one returns void and a proper irq handler needs to return
irqreturn_t.

Please look at this, I have backed out the patch.

Yours,
Linus Walleij