[PATCH 0/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[PATCH 0/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Keerthy]

The Davinci GPIO driver is implemented to work with one monolithic
Davinci GPIO platform device which may have up to Y(144) gpios.
The Davinci GPIO driver instantiates number of GPIO chips with
max 32 gpio pins per each during initialization and one IRQ domain.
So, the current GPIO's  opjects structure is:

<platform device> Davinci GPIO controller
 |- <gpio0_chip0> ------|
 ...                    |--- irq_domain (hwirq [0..143])
 |- <gpio0_chipN> ------|

Current driver creates one chip for every 32 GPIOs in a controller.
This was a limitation earlier now there is no need for that. Hence
redesigning the driver to create one gpio chip for all the ngpio
in the controller.

|- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).

The previous discussion on this can be found here:
https://www.spinics.net/lists/linux-omap/msg132869.html

The series is posted on top of coouple of clean up patches:

https://lkml.org/lkml/2017/1/4/94
https://patchwork.ozlabs.org/patch/710855/

Keerthy (3):
  gpio: davinci: Remove unwanted blank line
  gpio: davinci: Redesign driver to accomodate ngpio in one gpio chip
  gpio: davinci: Add support for multiple GPIO controllers

 drivers/gpio/gpio-davinci.c                | 129 +++++++++++++++++------------
 include/linux/platform_data/gpio-davinci.h |  13 ++-
 2 files changed, 86 insertions(+), 56 deletions(-)

-- 
1.9.1

[PATCH 1/3] gpio: davinci: Remove unwanted blank line

[Keerthy]

Remove redundant blank line.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 include/linux/platform_data/gpio-davinci.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index 44ca530..18127c4 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -26,7 +26,6 @@ struct davinci_gpio_platform_data {
 	u32	gpio_unbanked;
 };
 
-
 struct davinci_gpio_controller {
 	struct gpio_chip	chip;
 	struct irq_domain	*irq_domain;
-- 
1.9.1

[PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Keerthy]

The Davinci GPIO driver is implemented to work with one monolithic
Davinci GPIO platform device which may have up to Y(144) gpios.
The Davinci GPIO driver instantiates number of GPIO chips with
max 32 gpio pins per each during initialization and one IRQ domain.
So, the current GPIO's  opjects structure is:

<platform device> Davinci GPIO controller
 |- <gpio0_chip0> ------|
 ...                    |--- irq_domain (hwirq [0..143])
 |- <gpio0_chipN> ------|

Current driver creates one chip for every 32 GPIOs in a controller.
This was a limitation earlier now there is no need for that. Hence
redesigning the driver to create one gpio chip for all the ngpio
in the controller.

|- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).

The previous discussion on this can be found here:
https://www.spinics.net/lists/linux-omap/msg132869.html

Signed-off-by: Keerthy <j-keerthy@ti.com>
---

Boot tested on Davinci platform.

 drivers/gpio/gpio-davinci.c                | 127 +++++++++++++++++------------
 include/linux/platform_data/gpio-davinci.h |  13 ++-
 2 files changed, 84 insertions(+), 56 deletions(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 26b874a..6c1c00a 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -63,11 +63,13 @@ static inline int __davinci_direction(struct gpio_chip *chip,
 			unsigned offset, bool out, int value)
 {
 	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
-	struct davinci_gpio_regs __iomem *g = d->regs;
+	struct davinci_gpio_regs __iomem *g;
 	unsigned long flags;
 	u32 temp;
-	u32 mask = 1 << offset;
+	int bank = offset / 32;
+	u32 mask = 1 << (offset % 32);
 
+	g = d->regs[bank];
 	spin_lock_irqsave(&d->lock, flags);
 	temp = readl_relaxed(&g->dir);
 	if (out) {
@@ -103,9 +105,12 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
 static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
-	struct davinci_gpio_regs __iomem *g = d->regs;
+	struct davinci_gpio_regs __iomem *g;
+	int bank = offset / 32;
 
-	return !!((1 << offset) & readl_relaxed(&g->in_data));
+	g = d->regs[bank];
+
+	return !!((1 << (offset % 32)) & readl_relaxed(&g->in_data));
 }
 
 /*
@@ -115,9 +120,13 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
 davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 {
 	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
-	struct davinci_gpio_regs __iomem *g = d->regs;
+	struct davinci_gpio_regs __iomem *g;
+	int bank = offset / 32;
 
-	writel_relaxed((1 << offset), value ? &g->set_data : &g->clr_data);
+	g = d->regs[bank];
+
+	writel_relaxed((1 << (offset % 32)),
+		       value ? &g->set_data : &g->clr_data);
 }
 
 static struct davinci_gpio_platform_data *
@@ -165,7 +174,7 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
 	if (gpiospec->args[0] > pdata->ngpio)
 		return -EINVAL;
 
-	if (gc != &chips[gpiospec->args[0] / 32].chip)
+	if (gc != &chips->chip)
 		return -EINVAL;
 
 	if (flags)
@@ -177,11 +186,11 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
 
 static int davinci_gpio_probe(struct platform_device *pdev)
 {
-	int i, base;
+	static int ctrl_num;
+	int gpio, bank;
 	unsigned ngpio, nbank;
 	struct davinci_gpio_controller *chips;
 	struct davinci_gpio_platform_data *pdata;
-	struct davinci_gpio_regs __iomem *regs;
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	char label[MAX_LABEL_SIZE];
@@ -220,38 +229,30 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(gpio_base))
 		return PTR_ERR(gpio_base);
 
-	for (i = 0, base = 0; base < ngpio; i++, base += 32) {
-		snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", i);
-		chips[i].chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
-		if (!chips[i].chip.label)
+	snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
+	chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
+		if (!chips->chip.label)
 			return -ENOMEM;
 
-		chips[i].chip.direction_input = davinci_direction_in;
-		chips[i].chip.get = davinci_gpio_get;
-		chips[i].chip.direction_output = davinci_direction_out;
-		chips[i].chip.set = davinci_gpio_set;
+	chips->chip.direction_input = davinci_direction_in;
+	chips->chip.get = davinci_gpio_get;
+	chips->chip.direction_output = davinci_direction_out;
+	chips->chip.set = davinci_gpio_set;
 
-		chips[i].chip.base = base;
-		chips[i].chip.ngpio = ngpio - base;
-		if (chips[i].chip.ngpio > 32)
-			chips[i].chip.ngpio = 32;
+	chips->chip.ngpio = ngpio;
 
 #ifdef CONFIG_OF_GPIO
-		chips[i].chip.of_gpio_n_cells = 2;
-		chips[i].chip.of_xlate = davinci_gpio_of_xlate;
-		chips[i].chip.parent = dev;
-		chips[i].chip.of_node = dev->of_node;
+	chips->chip.of_gpio_n_cells = 2;
+	chips->chip.of_xlate = davinci_gpio_of_xlate;
+	chips->chip.parent = dev;
+	chips->chip.of_node = dev->of_node;
 #endif
-		spin_lock_init(&chips[i].lock);
-
-		regs = gpio_base + offset_array[i];
-		if (!regs)
-			return -ENXIO;
-		chips[i].regs = regs;
+	spin_lock_init(&chips->lock);
 
-		gpiochip_add_data(&chips[i].chip, &chips[i]);
-	}
+	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
+		chips->regs[bank] = gpio_base + offset_array[bank];
 
+	gpiochip_add_data(&chips->chip, chips);
 	platform_set_drvdata(pdev, chips);
 	davinci_gpio_irq_setup(pdev);
 	return 0;
@@ -312,16 +313,19 @@ static int gpio_irq_type(struct irq_data *d, unsigned trigger)
 
 static void gpio_irq_handler(struct irq_desc *desc)
 {
-	unsigned int irq = irq_desc_get_irq(desc);
 	struct davinci_gpio_regs __iomem *g;
 	u32 mask = 0xffff;
+	int bank_num;
 	struct davinci_gpio_controller *d;
+	struct davinci_gpio_irq_data *irqdata;
 
-	d = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
-	g = (struct davinci_gpio_regs __iomem *)d->regs;
+	irqdata = (struct davinci_gpio_irq_data *)irq_desc_get_handler_data(desc);
+	bank_num = irqdata->bank_num;
+	g = irqdata->regs;
+	d = irqdata->chip;
 
 	/* we only care about one bank */
-	if (irq & 1)
+	if ((bank_num % 2) == 1)
 		mask <<= 16;
 
 	/* temporarily mask (level sensitive) parent IRQ */
@@ -329,6 +333,7 @@ static void gpio_irq_handler(struct irq_desc *desc)
 	while (1) {
 		u32		status;
 		int		bit;
+		irq_hw_number_t hw_irq;
 
 		/* ack any irqs */
 		status = readl_relaxed(&g->intstat) & mask;
@@ -341,9 +346,13 @@ static void gpio_irq_handler(struct irq_desc *desc)
 		while (status) {
 			bit = __ffs(status);
 			status &= ~BIT(bit);
+			/* Max number of gpios per controller is 144 so
+			 * hw_irq will be in [0..143]
+			 */
+			hw_irq = (bank_num / 2) * 32 + bit;
+
 			generic_handle_irq(
-				irq_find_mapping(d->irq_domain,
-						 d->chip.base + bit));
+				irq_find_mapping(d->irq_domain, hw_irq));
 		}
 	}
 	chained_irq_exit(irq_desc_get_chip(desc), desc);
@@ -355,7 +364,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
 	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
 
 	if (d->irq_domain)
-		return irq_create_mapping(d->irq_domain, d->chip.base + offset);
+		return irq_create_mapping(d->irq_domain, offset);
 	else
 		return -ENXIO;
 }
@@ -369,7 +378,7 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
 	 * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
 	 */
 	if (offset < d->gpio_unbanked)
-		return d->gpio_irq + offset;
+		return d->base_irq + offset;
 	else
 		return -ENODEV;
 }
@@ -382,7 +391,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
 
 	d = (struct davinci_gpio_controller *)irq_data_get_irq_handler_data(data);
 	g = (struct davinci_gpio_regs __iomem *)d->regs;
-	mask = __gpio_mask(data->irq - d->gpio_irq);
+	mask = __gpio_mask(data->irq - d->base_irq);
 
 	if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
 		return -EINVAL;
@@ -401,7 +410,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
 {
 	struct davinci_gpio_controller *chips =
 				(struct davinci_gpio_controller *)d->host_data;
-	struct davinci_gpio_regs __iomem *g = chips[hw / 32].regs;
+	struct davinci_gpio_regs __iomem *g = chips->regs[hw / 32];
 
 	irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
 				"davinci_gpio");
@@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 	struct irq_domain	*irq_domain = NULL;
 	const struct of_device_id *match;
 	struct irq_chip *irq_chip;
+	struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS];
 	gpio_get_irq_chip_cb_t gpio_get_irq_chip;
 
 	/*
@@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 	 * IRQs, while the others use banked IRQs, would need some setup
 	 * tweaks to recognize hardware which can do that.
 	 */
-	for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
-		chips[bank].chip.to_irq = gpio_to_irq_banked;
-		chips[bank].irq_domain = irq_domain;
-	}
+	chips->chip.to_irq = gpio_to_irq_banked;
+	chips->irq_domain = irq_domain;
 
 	/*
 	 * AINTC can handle direct/unbanked IRQs for GPIOs, with the GPIO
@@ -526,9 +534,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 	 */
 	if (pdata->gpio_unbanked) {
 		/* pass "bank 0" GPIO IRQs to AINTC */
-		chips[0].chip.to_irq = gpio_to_irq_unbanked;
-		chips[0].gpio_irq = bank_irq;
-		chips[0].gpio_unbanked = pdata->gpio_unbanked;
+		chips->chip.to_irq = gpio_to_irq_unbanked;
+		chips->base_irq = bank_irq;
+		chips->gpio_unbanked = pdata->gpio_unbanked;
 		binten = GENMASK(pdata->gpio_unbanked / 16, 0);
 
 		/* AINTC handles mask/unmask; GPIO handles triggering */
@@ -538,14 +546,14 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 		irq_chip->irq_set_type = gpio_irq_type_unbanked;
 
 		/* default trigger: both edges */
-		g = chips[0].regs;
+		g = chips->regs[0];
 		writel_relaxed(~0, &g->set_falling);
 		writel_relaxed(~0, &g->set_rising);
 
 		/* set the direct IRQs up to use that irqchip */
 		for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
 			irq_set_chip(irq, irq_chip);
-			irq_set_handler_data(irq, &chips[gpio / 32]);
+			irq_set_handler_data(irq, chips);
 			irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
 		}
 
@@ -558,7 +566,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 	 */
 	for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
 		/* disabled by default, enabled only as needed */
-		g = chips[bank / 2].regs;
+		g = chips->regs[bank / 2];
 		writel_relaxed(~0, &g->clr_falling);
 		writel_relaxed(~0, &g->clr_rising);
 
@@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
 		 * gpio irqs. Pass the irq bank's corresponding controller to
 		 * the chained irq handler.
 		 */
+		irqdata[bank] = devm_kzalloc(&pdev->dev,
+					     sizeof(struct
+						    davinci_gpio_irq_data),
+					     GFP_KERNEL);
+		if (!irqdata[bank])
+			return -ENOMEM;
+
+		irqdata[bank]->regs = g;
+		irqdata[bank]->bank_num = bank;
+		irqdata[bank]->chip = chips;
+
 		irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
-						 &chips[gpio / 32]);
+						 irqdata[bank]);
 
 		binten |= BIT(bank);
 	}
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index 18127c4..ca09686 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -21,19 +21,28 @@
 
 #include <asm-generic/gpio.h>
 
+#define MAX_BANKS		5
+#define MAX_BANKED_IRQS		9
+
 struct davinci_gpio_platform_data {
 	u32	ngpio;
 	u32	gpio_unbanked;
 };
 
+struct davinci_gpio_irq_data {
+	void __iomem			*regs;
+	struct davinci_gpio_controller	*chip;
+	int				bank_num;
+};
+
 struct davinci_gpio_controller {
 	struct gpio_chip	chip;
 	struct irq_domain	*irq_domain;
 	/* Serialize access to GPIO registers */
 	spinlock_t		lock;
-	void __iomem		*regs;
+	void __iomem		*regs[MAX_BANKS];
 	int			gpio_unbanked;
-	unsigned		gpio_irq;
+	unsigned int		base_irq;
 };
 
 /*
-- 
1.9.1

[PATCH 3/3] gpio: davinci: Add support for multiple GPIO controllers

[Keerthy]

Update GPIO driver to support Multiple GPIO controllers by updating
the base of subsequent GPIO chips with total of previous chips
gpio count so that gpio_add_chip gets unique numbers.

Signed-off-by: Keerthy <j-keerthy@ti.com>
---
 drivers/gpio/gpio-davinci.c                | 4 +++-
 include/linux/platform_data/gpio-davinci.h | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 6c1c00a..1928de3 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -186,7 +186,7 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
 
 static int davinci_gpio_probe(struct platform_device *pdev)
 {
-	static int ctrl_num;
+	static int ctrl_num, bank_base;
 	int gpio, bank;
 	unsigned ngpio, nbank;
 	struct davinci_gpio_controller *chips;
@@ -240,6 +240,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 	chips->chip.set = davinci_gpio_set;
 
 	chips->chip.ngpio = ngpio;
+	chip->chip.base = bank_base;
 
 #ifdef CONFIG_OF_GPIO
 	chips->chip.of_gpio_n_cells = 2;
@@ -248,6 +249,7 @@ static int davinci_gpio_probe(struct platform_device *pdev)
 	chips->chip.of_node = dev->of_node;
 #endif
 	spin_lock_init(&chips->lock);
+	bank_base += ngpio;
 
 	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
 		chips->regs[bank] = gpio_base + offset_array[bank];
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index ca09686..fd10f47 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -43,6 +43,7 @@ struct davinci_gpio_controller {
 	void __iomem		*regs[MAX_BANKS];
 	int			gpio_unbanked;
 	unsigned int		base_irq;
+	unsigned int		base;
 };
 
 /*
-- 
1.9.1

Re: [PATCH 1/3] gpio: davinci: Remove unwanted blank line

[Grygorii Strashko]

On 01/10/2017 11:00 PM, Keerthy wrote:
> Remove redundant blank line.

i think it's better to drop this patch ;)

>
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
>  include/linux/platform_data/gpio-davinci.h | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
> index 44ca530..18127c4 100644
> --- a/include/linux/platform_data/gpio-davinci.h
> +++ b/include/linux/platform_data/gpio-davinci.h
> @@ -26,7 +26,6 @@ struct davinci_gpio_platform_data {
>  	u32	gpio_unbanked;
>  };
>
> -
>  struct davinci_gpio_controller {
>  	struct gpio_chip	chip;
>  	struct irq_domain	*irq_domain;
>

-- 
regards,
-grygorii

Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Grygorii Strashko]

On 01/10/2017 11:00 PM, Keerthy wrote:
> The Davinci GPIO driver is implemented to work with one monolithic
> Davinci GPIO platform device which may have up to Y(144) gpios.
> The Davinci GPIO driver instantiates number of GPIO chips with
> max 32 gpio pins per each during initialization and one IRQ domain.
> So, the current GPIO's  opjects structure is:
> 
> <platform device> Davinci GPIO controller
>  |- <gpio0_chip0> ------|
>  ...                    |--- irq_domain (hwirq [0..143])
>  |- <gpio0_chipN> ------|
> 
> Current driver creates one chip for every 32 GPIOs in a controller.
> This was a limitation earlier now there is no need for that. Hence
> redesigning the driver to create one gpio chip for all the ngpio
> in the controller.
> 
> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).
> 
> The previous discussion on this can be found here:
> https://www.spinics.net/lists/linux-omap/msg132869.html

nice rework.

> 
> Signed-off-by: Keerthy <j-keerthy@ti.com>
> ---
> 
> Boot tested on Davinci platform.
> 
>  drivers/gpio/gpio-davinci.c                | 127 +++++++++++++++++------------
>  include/linux/platform_data/gpio-davinci.h |  13 ++-
>  2 files changed, 84 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 26b874a..6c1c00a 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -63,11 +63,13 @@ static inline int __davinci_direction(struct gpio_chip *chip,
>  			unsigned offset, bool out, int value)
>  {
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
> -	struct davinci_gpio_regs __iomem *g = d->regs;
> +	struct davinci_gpio_regs __iomem *g;
>  	unsigned long flags;
>  	u32 temp;
> -	u32 mask = 1 << offset;
> +	int bank = offset / 32;
> +	u32 mask = 1 << (offset % 32);
>  
> +	g = d->regs[bank];
>  	spin_lock_irqsave(&d->lock, flags);
>  	temp = readl_relaxed(&g->dir);
>  	if (out) {
> @@ -103,9 +105,12 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
>  static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
> -	struct davinci_gpio_regs __iomem *g = d->regs;
> +	struct davinci_gpio_regs __iomem *g;
> +	int bank = offset / 32;
>  
> -	return !!((1 << offset) & readl_relaxed(&g->in_data));
> +	g = d->regs[bank];
> +
> +	return !!((1 << (offset % 32)) & readl_relaxed(&g->in_data));

(1 << (offset % 32) is __gpio_mask()

>  }
>  
>  /*
> @@ -115,9 +120,13 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>  davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>  {
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
> -	struct davinci_gpio_regs __iomem *g = d->regs;
> +	struct davinci_gpio_regs __iomem *g;
> +	int bank = offset / 32;
>  
> -	writel_relaxed((1 << offset), value ? &g->set_data : &g->clr_data);
> +	g = d->regs[bank];
> +
> +	writel_relaxed((1 << (offset % 32)),
> +		       value ? &g->set_data : &g->clr_data);
>  }
>  
>  static struct davinci_gpio_platform_data *
> @@ -165,7 +174,7 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
>  	if (gpiospec->args[0] > pdata->ngpio)
>  		return -EINVAL;
>  
> -	if (gc != &chips[gpiospec->args[0] / 32].chip)
> +	if (gc != &chips->chip)
>  		return -EINVAL;
>  
>  	if (flags)
> @@ -177,11 +186,11 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
>  
>  static int davinci_gpio_probe(struct platform_device *pdev)
>  {
> -	int i, base;
> +	static int ctrl_num;
> +	int gpio, bank;
>  	unsigned ngpio, nbank;
>  	struct davinci_gpio_controller *chips;
>  	struct davinci_gpio_platform_data *pdata;
> -	struct davinci_gpio_regs __iomem *regs;
>  	struct device *dev = &pdev->dev;
>  	struct resource *res;
>  	char label[MAX_LABEL_SIZE];
> @@ -220,38 +229,30 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>  	if (IS_ERR(gpio_base))
>  		return PTR_ERR(gpio_base);
>  
> -	for (i = 0, base = 0; base < ngpio; i++, base += 32) {
> -		snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", i);
> -		chips[i].chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
> -		if (!chips[i].chip.label)
> +	snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
> +	chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
> +		if (!chips->chip.label)
>  			return -ENOMEM;
>  
> -		chips[i].chip.direction_input = davinci_direction_in;
> -		chips[i].chip.get = davinci_gpio_get;
> -		chips[i].chip.direction_output = davinci_direction_out;
> -		chips[i].chip.set = davinci_gpio_set;
> +	chips->chip.direction_input = davinci_direction_in;
> +	chips->chip.get = davinci_gpio_get;
> +	chips->chip.direction_output = davinci_direction_out;
> +	chips->chip.set = davinci_gpio_set;
>  
> -		chips[i].chip.base = base;
> -		chips[i].chip.ngpio = ngpio - base;
> -		if (chips[i].chip.ngpio > 32)
> -			chips[i].chip.ngpio = 32;
> +	chips->chip.ngpio = ngpio;
>  
>  #ifdef CONFIG_OF_GPIO
> -		chips[i].chip.of_gpio_n_cells = 2;
> -		chips[i].chip.of_xlate = davinci_gpio_of_xlate;
> -		chips[i].chip.parent = dev;
> -		chips[i].chip.of_node = dev->of_node;
> +	chips->chip.of_gpio_n_cells = 2;
> +	chips->chip.of_xlate = davinci_gpio_of_xlate;

I think It's not necessary to have custom .xlate() and
it can be removed, then gpiolib will assign default one of_gpio_simple_xlate().

> +	chips->chip.parent = dev;
> +	chips->chip.of_node = dev->of_node;
>  #endif
> -		spin_lock_init(&chips[i].lock);
> -
> -		regs = gpio_base + offset_array[i];
> -		if (!regs)
> -			return -ENXIO;
> -		chips[i].regs = regs;
> +	spin_lock_init(&chips->lock);
>  
> -		gpiochip_add_data(&chips[i].chip, &chips[i]);
> -	}
> +	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
> +		chips->regs[bank] = gpio_base + offset_array[bank];
>  
> +	gpiochip_add_data(&chips->chip, chips);
>  	platform_set_drvdata(pdev, chips);
>  	davinci_gpio_irq_setup(pdev);
>  	return 0;
> @@ -312,16 +313,19 @@ static int gpio_irq_type(struct irq_data *d, unsigned trigger)
>  
>  static void gpio_irq_handler(struct irq_desc *desc)
>  {
> -	unsigned int irq = irq_desc_get_irq(desc);
>  	struct davinci_gpio_regs __iomem *g;
>  	u32 mask = 0xffff;
> +	int bank_num;
>  	struct davinci_gpio_controller *d;
> +	struct davinci_gpio_irq_data *irqdata;
>  
> -	d = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
> -	g = (struct davinci_gpio_regs __iomem *)d->regs;
> +	irqdata = (struct davinci_gpio_irq_data *)irq_desc_get_handler_data(desc);
> +	bank_num = irqdata->bank_num;
> +	g = irqdata->regs;
> +	d = irqdata->chip;
>  
>  	/* we only care about one bank */
> -	if (irq & 1)
> +	if ((bank_num % 2) == 1)
>  		mask <<= 16;
>  
>  	/* temporarily mask (level sensitive) parent IRQ */
> @@ -329,6 +333,7 @@ static void gpio_irq_handler(struct irq_desc *desc)
>  	while (1) {
>  		u32		status;
>  		int		bit;
> +		irq_hw_number_t hw_irq;
>  
>  		/* ack any irqs */
>  		status = readl_relaxed(&g->intstat) & mask;
> @@ -341,9 +346,13 @@ static void gpio_irq_handler(struct irq_desc *desc)
>  		while (status) {
>  			bit = __ffs(status);
>  			status &= ~BIT(bit);
> +			/* Max number of gpios per controller is 144 so
> +			 * hw_irq will be in [0..143]
> +			 */
> +			hw_irq = (bank_num / 2) * 32 + bit;
> +
>  			generic_handle_irq(
> -				irq_find_mapping(d->irq_domain,
> -						 d->chip.base + bit));
> +				irq_find_mapping(d->irq_domain, hw_irq));
>  		}
>  	}
>  	chained_irq_exit(irq_desc_get_chip(desc), desc);
> @@ -355,7 +364,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
>  
>  	if (d->irq_domain)
> -		return irq_create_mapping(d->irq_domain, d->chip.base + offset);
> +		return irq_create_mapping(d->irq_domain, offset);
>  	else
>  		return -ENXIO;
>  }
> @@ -369,7 +378,7 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
>  	 * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
>  	 */
>  	if (offset < d->gpio_unbanked)
> -		return d->gpio_irq + offset;
> +		return d->base_irq + offset;
>  	else
>  		return -ENODEV;
>  }
> @@ -382,7 +391,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
>  
>  	d = (struct davinci_gpio_controller *)irq_data_get_irq_handler_data(data);
>  	g = (struct davinci_gpio_regs __iomem *)d->regs;
> -	mask = __gpio_mask(data->irq - d->gpio_irq);
> +	mask = __gpio_mask(data->irq - d->base_irq);
>  
>  	if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>  		return -EINVAL;
> @@ -401,7 +410,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
>  {
>  	struct davinci_gpio_controller *chips =
>  				(struct davinci_gpio_controller *)d->host_data;
> -	struct davinci_gpio_regs __iomem *g = chips[hw / 32].regs;
> +	struct davinci_gpio_regs __iomem *g = chips->regs[hw / 32];
>  
>  	irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
>  				"davinci_gpio");
> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	struct irq_domain	*irq_domain = NULL;
>  	const struct of_device_id *match;
>  	struct irq_chip *irq_chip;
> +	struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS];

You declare irqdata as array here but it has not been used anywhere
except for assignment. Could you remove this array and MAX_BANKED_IRQS define?

Seems you can just use struct davinci_gpio_irq_data *irqdata;

>  	gpio_get_irq_chip_cb_t gpio_get_irq_chip;
>  
>  	/*
> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	 * IRQs, while the others use banked IRQs, would need some setup
>  	 * tweaks to recognize hardware which can do that.
>  	 */
> -	for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
> -		chips[bank].chip.to_irq = gpio_to_irq_banked;
> -		chips[bank].irq_domain = irq_domain;
> -	}
> +	chips->chip.to_irq = gpio_to_irq_banked;
> +	chips->irq_domain = irq_domain;
>  
>  	/*
>  	 * AINTC can handle direct/unbanked IRQs for GPIOs, with the GPIO
> @@ -526,9 +534,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	 */
>  	if (pdata->gpio_unbanked) {
>  		/* pass "bank 0" GPIO IRQs to AINTC */
> -		chips[0].chip.to_irq = gpio_to_irq_unbanked;
> -		chips[0].gpio_irq = bank_irq;
> -		chips[0].gpio_unbanked = pdata->gpio_unbanked;
> +		chips->chip.to_irq = gpio_to_irq_unbanked;
> +		chips->base_irq = bank_irq;
> +		chips->gpio_unbanked = pdata->gpio_unbanked;
>  		binten = GENMASK(pdata->gpio_unbanked / 16, 0);
>  
>  		/* AINTC handles mask/unmask; GPIO handles triggering */
> @@ -538,14 +546,14 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  		irq_chip->irq_set_type = gpio_irq_type_unbanked;
>  
>  		/* default trigger: both edges */
> -		g = chips[0].regs;
> +		g = chips->regs[0];
>  		writel_relaxed(~0, &g->set_falling);
>  		writel_relaxed(~0, &g->set_rising);
>  
>  		/* set the direct IRQs up to use that irqchip */
>  		for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
>  			irq_set_chip(irq, irq_chip);
> -			irq_set_handler_data(irq, &chips[gpio / 32]);
> +			irq_set_handler_data(irq, chips);
>  			irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
>  		}
>  
> @@ -558,7 +566,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  	 */
>  	for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
>  		/* disabled by default, enabled only as needed */
> -		g = chips[bank / 2].regs;
> +		g = chips->regs[bank / 2];
>  		writel_relaxed(~0, &g->clr_falling);
>  		writel_relaxed(~0, &g->clr_rising);
>  
> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>  		 * gpio irqs. Pass the irq bank's corresponding controller to
>  		 * the chained irq handler.
>  		 */
> +		irqdata[bank] = devm_kzalloc(&pdev->dev,
> +					     sizeof(struct
> +						    davinci_gpio_irq_data),
> +					     GFP_KERNEL);
> +		if (!irqdata[bank])
> +			return -ENOMEM;
> +
> +		irqdata[bank]->regs = g;
> +		irqdata[bank]->bank_num = bank;
> +		irqdata[bank]->chip = chips;
> +
>  		irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
> -						 &chips[gpio / 32]);
> +						 irqdata[bank]);
>  
>  		binten |= BIT(bank);
>  	}
> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
> index 18127c4..ca09686 100644
> --- a/include/linux/platform_data/gpio-davinci.h
> +++ b/include/linux/platform_data/gpio-davinci.h
> @@ -21,19 +21,28 @@
>  
>  #include <asm-generic/gpio.h>
>  
> +#define MAX_BANKS		5

probably MAX_REGS_BANKS will be better, as it defines
max number of idnetical registers sets and not number of gpio banks.

> +#define MAX_BANKED_IRQS		9
> +
>  struct davinci_gpio_platform_data {
>  	u32	ngpio;
>  	u32	gpio_unbanked;
>  };
>  
> +struct davinci_gpio_irq_data {
> +	void __iomem			*regs;
> +	struct davinci_gpio_controller	*chip;
> +	int				bank_num;
> +};
> +
>  struct davinci_gpio_controller {
>  	struct gpio_chip	chip;
>  	struct irq_domain	*irq_domain;
>  	/* Serialize access to GPIO registers */
>  	spinlock_t		lock;
> -	void __iomem		*regs;
> +	void __iomem		*regs[MAX_BANKS];
>  	int			gpio_unbanked;
> -	unsigned		gpio_irq;
> +	unsigned int		base_irq;
>  };
>  
>  /*
> 

-- 
regards,
-grygorii

Re: [PATCH 1/3] gpio: davinci: Remove unwanted blank line

[Keerthy]

On Wednesday 11 January 2017 11:07 PM, Grygorii Strashko wrote:
>
>
> On 01/10/2017 11:00 PM, Keerthy wrote:
>> Remove redundant blank line.
>
> i think it's better to drop this patch ;)

There were already too many things in the second patch so just did not 
want to add more lines of code change :-P.

>
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>  include/linux/platform_data/gpio-davinci.h | 1 -
>>  1 file changed, 1 deletion(-)
>>
>> diff --git a/include/linux/platform_data/gpio-davinci.h
>> b/include/linux/platform_data/gpio-davinci.h
>> index 44ca530..18127c4 100644
>> --- a/include/linux/platform_data/gpio-davinci.h
>> +++ b/include/linux/platform_data/gpio-davinci.h
>> @@ -26,7 +26,6 @@ struct davinci_gpio_platform_data {
>>      u32    gpio_unbanked;
>>  };
>>
>> -
>>  struct davinci_gpio_controller {
>>      struct gpio_chip    chip;
>>      struct irq_domain    *irq_domain;
>>
>

Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Keerthy]

On Wednesday 11 January 2017 11:23 PM, Grygorii Strashko wrote:
>
>
> On 01/10/2017 11:00 PM, Keerthy wrote:
>> The Davinci GPIO driver is implemented to work with one monolithic
>> Davinci GPIO platform device which may have up to Y(144) gpios.
>> The Davinci GPIO driver instantiates number of GPIO chips with
>> max 32 gpio pins per each during initialization and one IRQ domain.
>> So, the current GPIO's  opjects structure is:
>>
>> <platform device> Davinci GPIO controller
>>  |- <gpio0_chip0> ------|
>>  ...                    |--- irq_domain (hwirq [0..143])
>>  |- <gpio0_chipN> ------|
>>
>> Current driver creates one chip for every 32 GPIOs in a controller.
>> This was a limitation earlier now there is no need for that. Hence
>> redesigning the driver to create one gpio chip for all the ngpio
>> in the controller.
>>
>> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).
>>
>> The previous discussion on this can be found here:
>> https://www.spinics.net/lists/linux-omap/msg132869.html
>
> nice rework.

Thanks!

>
>>
>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>> ---
>>
>> Boot tested on Davinci platform.
>>
>>  drivers/gpio/gpio-davinci.c                | 127 +++++++++++++++++------------
>>  include/linux/platform_data/gpio-davinci.h |  13 ++-
>>  2 files changed, 84 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
>> index 26b874a..6c1c00a 100644
>> --- a/drivers/gpio/gpio-davinci.c
>> +++ b/drivers/gpio/gpio-davinci.c
>> @@ -63,11 +63,13 @@ static inline int __davinci_direction(struct gpio_chip *chip,
>>  			unsigned offset, bool out, int value)
>>  {
>>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
>> -	struct davinci_gpio_regs __iomem *g = d->regs;
>> +	struct davinci_gpio_regs __iomem *g;
>>  	unsigned long flags;
>>  	u32 temp;
>> -	u32 mask = 1 << offset;
>> +	int bank = offset / 32;
>> +	u32 mask = 1 << (offset % 32);
>>
>> +	g = d->regs[bank];
>>  	spin_lock_irqsave(&d->lock, flags);
>>  	temp = readl_relaxed(&g->dir);
>>  	if (out) {
>> @@ -103,9 +105,12 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
>>  static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>>  {
>>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
>> -	struct davinci_gpio_regs __iomem *g = d->regs;
>> +	struct davinci_gpio_regs __iomem *g;
>> +	int bank = offset / 32;
>>
>> -	return !!((1 << offset) & readl_relaxed(&g->in_data));
>> +	g = d->regs[bank];
>> +
>> +	return !!((1 << (offset % 32)) & readl_relaxed(&g->in_data));
>
> (1 << (offset % 32) is __gpio_mask()

Okay. I can try and use that here.

>
>>  }
>>
>>  /*
>> @@ -115,9 +120,13 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
>>  davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>>  {
>>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
>> -	struct davinci_gpio_regs __iomem *g = d->regs;
>> +	struct davinci_gpio_regs __iomem *g;
>> +	int bank = offset / 32;
>>
>> -	writel_relaxed((1 << offset), value ? &g->set_data : &g->clr_data);
>> +	g = d->regs[bank];
>> +
>> +	writel_relaxed((1 << (offset % 32)),
>> +		       value ? &g->set_data : &g->clr_data);
>>  }
>>
>>  static struct davinci_gpio_platform_data *
>> @@ -165,7 +174,7 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
>>  	if (gpiospec->args[0] > pdata->ngpio)
>>  		return -EINVAL;
>>
>> -	if (gc != &chips[gpiospec->args[0] / 32].chip)
>> +	if (gc != &chips->chip)
>>  		return -EINVAL;
>>
>>  	if (flags)
>> @@ -177,11 +186,11 @@ static int davinci_gpio_of_xlate(struct gpio_chip *gc,
>>
>>  static int davinci_gpio_probe(struct platform_device *pdev)
>>  {
>> -	int i, base;
>> +	static int ctrl_num;
>> +	int gpio, bank;
>>  	unsigned ngpio, nbank;
>>  	struct davinci_gpio_controller *chips;
>>  	struct davinci_gpio_platform_data *pdata;
>> -	struct davinci_gpio_regs __iomem *regs;
>>  	struct device *dev = &pdev->dev;
>>  	struct resource *res;
>>  	char label[MAX_LABEL_SIZE];
>> @@ -220,38 +229,30 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>  	if (IS_ERR(gpio_base))
>>  		return PTR_ERR(gpio_base);
>>
>> -	for (i = 0, base = 0; base < ngpio; i++, base += 32) {
>> -		snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", i);
>> -		chips[i].chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
>> -		if (!chips[i].chip.label)
>> +	snprintf(label, MAX_LABEL_SIZE, "davinci_gpio.%d", ctrl_num++);
>> +	chips->chip.label = devm_kstrdup(dev, label, GFP_KERNEL);
>> +		if (!chips->chip.label)
>>  			return -ENOMEM;
>>
>> -		chips[i].chip.direction_input = davinci_direction_in;
>> -		chips[i].chip.get = davinci_gpio_get;
>> -		chips[i].chip.direction_output = davinci_direction_out;
>> -		chips[i].chip.set = davinci_gpio_set;
>> +	chips->chip.direction_input = davinci_direction_in;
>> +	chips->chip.get = davinci_gpio_get;
>> +	chips->chip.direction_output = davinci_direction_out;
>> +	chips->chip.set = davinci_gpio_set;
>>
>> -		chips[i].chip.base = base;
>> -		chips[i].chip.ngpio = ngpio - base;
>> -		if (chips[i].chip.ngpio > 32)
>> -			chips[i].chip.ngpio = 32;
>> +	chips->chip.ngpio = ngpio;
>>
>>  #ifdef CONFIG_OF_GPIO
>> -		chips[i].chip.of_gpio_n_cells = 2;
>> -		chips[i].chip.of_xlate = davinci_gpio_of_xlate;
>> -		chips[i].chip.parent = dev;
>> -		chips[i].chip.of_node = dev->of_node;
>> +	chips->chip.of_gpio_n_cells = 2;
>> +	chips->chip.of_xlate = davinci_gpio_of_xlate;
>
> I think It's not necessary to have custom .xlate() and
> it can be removed, then gpiolib will assign default one of_gpio_simple_xlate().

Okay. Can i do that as a separate patch?

>
>> +	chips->chip.parent = dev;
>> +	chips->chip.of_node = dev->of_node;
>>  #endif
>> -		spin_lock_init(&chips[i].lock);
>> -
>> -		regs = gpio_base + offset_array[i];
>> -		if (!regs)
>> -			return -ENXIO;
>> -		chips[i].regs = regs;
>> +	spin_lock_init(&chips->lock);
>>
>> -		gpiochip_add_data(&chips[i].chip, &chips[i]);
>> -	}
>> +	for (gpio = 0, bank = 0; gpio < ngpio; gpio += 32, bank++)
>> +		chips->regs[bank] = gpio_base + offset_array[bank];
>>
>> +	gpiochip_add_data(&chips->chip, chips);
>>  	platform_set_drvdata(pdev, chips);
>>  	davinci_gpio_irq_setup(pdev);
>>  	return 0;
>> @@ -312,16 +313,19 @@ static int gpio_irq_type(struct irq_data *d, unsigned trigger)
>>
>>  static void gpio_irq_handler(struct irq_desc *desc)
>>  {
>> -	unsigned int irq = irq_desc_get_irq(desc);
>>  	struct davinci_gpio_regs __iomem *g;
>>  	u32 mask = 0xffff;
>> +	int bank_num;
>>  	struct davinci_gpio_controller *d;
>> +	struct davinci_gpio_irq_data *irqdata;
>>
>> -	d = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
>> -	g = (struct davinci_gpio_regs __iomem *)d->regs;
>> +	irqdata = (struct davinci_gpio_irq_data *)irq_desc_get_handler_data(desc);
>> +	bank_num = irqdata->bank_num;
>> +	g = irqdata->regs;
>> +	d = irqdata->chip;
>>
>>  	/* we only care about one bank */
>> -	if (irq & 1)
>> +	if ((bank_num % 2) == 1)
>>  		mask <<= 16;
>>
>>  	/* temporarily mask (level sensitive) parent IRQ */
>> @@ -329,6 +333,7 @@ static void gpio_irq_handler(struct irq_desc *desc)
>>  	while (1) {
>>  		u32		status;
>>  		int		bit;
>> +		irq_hw_number_t hw_irq;
>>
>>  		/* ack any irqs */
>>  		status = readl_relaxed(&g->intstat) & mask;
>> @@ -341,9 +346,13 @@ static void gpio_irq_handler(struct irq_desc *desc)
>>  		while (status) {
>>  			bit = __ffs(status);
>>  			status &= ~BIT(bit);
>> +			/* Max number of gpios per controller is 144 so
>> +			 * hw_irq will be in [0..143]
>> +			 */
>> +			hw_irq = (bank_num / 2) * 32 + bit;
>> +
>>  			generic_handle_irq(
>> -				irq_find_mapping(d->irq_domain,
>> -						 d->chip.base + bit));
>> +				irq_find_mapping(d->irq_domain, hw_irq));
>>  		}
>>  	}
>>  	chained_irq_exit(irq_desc_get_chip(desc), desc);
>> @@ -355,7 +364,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
>>  	struct davinci_gpio_controller *d = gpiochip_get_data(chip);
>>
>>  	if (d->irq_domain)
>> -		return irq_create_mapping(d->irq_domain, d->chip.base + offset);
>> +		return irq_create_mapping(d->irq_domain, offset);
>>  	else
>>  		return -ENXIO;
>>  }
>> @@ -369,7 +378,7 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
>>  	 * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
>>  	 */
>>  	if (offset < d->gpio_unbanked)
>> -		return d->gpio_irq + offset;
>> +		return d->base_irq + offset;
>>  	else
>>  		return -ENODEV;
>>  }
>> @@ -382,7 +391,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
>>
>>  	d = (struct davinci_gpio_controller *)irq_data_get_irq_handler_data(data);
>>  	g = (struct davinci_gpio_regs __iomem *)d->regs;
>> -	mask = __gpio_mask(data->irq - d->gpio_irq);
>> +	mask = __gpio_mask(data->irq - d->base_irq);
>>
>>  	if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
>>  		return -EINVAL;
>> @@ -401,7 +410,7 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
>>  {
>>  	struct davinci_gpio_controller *chips =
>>  				(struct davinci_gpio_controller *)d->host_data;
>> -	struct davinci_gpio_regs __iomem *g = chips[hw / 32].regs;
>> +	struct davinci_gpio_regs __iomem *g = chips->regs[hw / 32];
>>
>>  	irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
>>  				"davinci_gpio");
>> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  	struct irq_domain	*irq_domain = NULL;
>>  	const struct of_device_id *match;
>>  	struct irq_chip *irq_chip;
>> +	struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS];
>
> You declare irqdata as array here but it has not been used anywhere
> except for assignment. Could you remove this array and MAX_BANKED_IRQS define?

irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
						 &chips[gpio / 32]);
						 irqdata[bank]);

That is hooked as data for each bank. As there is only one controller 
now and the differentiating parameters per bank is the irqdata data 
structure with the registers pointer and the bank number.
This structure makes it very easy in the irq handler to identify the 
register sets that need to be modified and the bank irqs.

>
> Seems you can just use struct davinci_gpio_irq_data *irqdata;
>
>>  	gpio_get_irq_chip_cb_t gpio_get_irq_chip;
>>
>>  	/*
>> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  	 * IRQs, while the others use banked IRQs, would need some setup
>>  	 * tweaks to recognize hardware which can do that.
>>  	 */
>> -	for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
>> -		chips[bank].chip.to_irq = gpio_to_irq_banked;
>> -		chips[bank].irq_domain = irq_domain;
>> -	}
>> +	chips->chip.to_irq = gpio_to_irq_banked;
>> +	chips->irq_domain = irq_domain;
>>
>>  	/*
>>  	 * AINTC can handle direct/unbanked IRQs for GPIOs, with the GPIO
>> @@ -526,9 +534,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  	 */
>>  	if (pdata->gpio_unbanked) {
>>  		/* pass "bank 0" GPIO IRQs to AINTC */
>> -		chips[0].chip.to_irq = gpio_to_irq_unbanked;
>> -		chips[0].gpio_irq = bank_irq;
>> -		chips[0].gpio_unbanked = pdata->gpio_unbanked;
>> +		chips->chip.to_irq = gpio_to_irq_unbanked;
>> +		chips->base_irq = bank_irq;
>> +		chips->gpio_unbanked = pdata->gpio_unbanked;
>>  		binten = GENMASK(pdata->gpio_unbanked / 16, 0);
>>
>>  		/* AINTC handles mask/unmask; GPIO handles triggering */
>> @@ -538,14 +546,14 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  		irq_chip->irq_set_type = gpio_irq_type_unbanked;
>>
>>  		/* default trigger: both edges */
>> -		g = chips[0].regs;
>> +		g = chips->regs[0];
>>  		writel_relaxed(~0, &g->set_falling);
>>  		writel_relaxed(~0, &g->set_rising);
>>
>>  		/* set the direct IRQs up to use that irqchip */
>>  		for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
>>  			irq_set_chip(irq, irq_chip);
>> -			irq_set_handler_data(irq, &chips[gpio / 32]);
>> +			irq_set_handler_data(irq, chips);
>>  			irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
>>  		}
>>
>> @@ -558,7 +566,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  	 */
>>  	for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
>>  		/* disabled by default, enabled only as needed */
>> -		g = chips[bank / 2].regs;
>> +		g = chips->regs[bank / 2];
>>  		writel_relaxed(~0, &g->clr_falling);
>>  		writel_relaxed(~0, &g->clr_rising);
>>
>> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>>  		 * gpio irqs. Pass the irq bank's corresponding controller to
>>  		 * the chained irq handler.
>>  		 */
>> +		irqdata[bank] = devm_kzalloc(&pdev->dev,
>> +					     sizeof(struct
>> +						    davinci_gpio_irq_data),
>> +					     GFP_KERNEL);
>> +		if (!irqdata[bank])
>> +			return -ENOMEM;
>> +
>> +		irqdata[bank]->regs = g;
>> +		irqdata[bank]->bank_num = bank;
>> +		irqdata[bank]->chip = chips;
>> +
>>  		irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
>> -						 &chips[gpio / 32]);
>> +						 irqdata[bank]);
>>
>>  		binten |= BIT(bank);
>>  	}
>> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
>> index 18127c4..ca09686 100644
>> --- a/include/linux/platform_data/gpio-davinci.h
>> +++ b/include/linux/platform_data/gpio-davinci.h
>> @@ -21,19 +21,28 @@
>>
>>  #include <asm-generic/gpio.h>
>>
>> +#define MAX_BANKS		5
>
> probably MAX_REGS_BANKS will be better, as it defines
> max number of idnetical registers sets and not number of gpio banks.

Yes that makes it more meaningful. I will do that.

>
>> +#define MAX_BANKED_IRQS		9
>> +
>>  struct davinci_gpio_platform_data {
>>  	u32	ngpio;
>>  	u32	gpio_unbanked;
>>  };
>>
>> +struct davinci_gpio_irq_data {
>> +	void __iomem			*regs;
>> +	struct davinci_gpio_controller	*chip;
>> +	int				bank_num;
>> +};
>> +
>>  struct davinci_gpio_controller {
>>  	struct gpio_chip	chip;
>>  	struct irq_domain	*irq_domain;
>>  	/* Serialize access to GPIO registers */
>>  	spinlock_t		lock;
>> -	void __iomem		*regs;
>> +	void __iomem		*regs[MAX_BANKS];
>>  	int			gpio_unbanked;
>> -	unsigned		gpio_irq;
>> +	unsigned int		base_irq;
>>  };
>>
>>  /*
>>
>

Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Grygorii Strashko]

On 01/11/2017 08:00 PM, Keerthy wrote:
>
>
> On Wednesday 11 January 2017 11:23 PM, Grygorii Strashko wrote:
>>
>>
>> On 01/10/2017 11:00 PM, Keerthy wrote:
>>> The Davinci GPIO driver is implemented to work with one monolithic
>>> Davinci GPIO platform device which may have up to Y(144) gpios.
>>> The Davinci GPIO driver instantiates number of GPIO chips with
>>> max 32 gpio pins per each during initialization and one IRQ domain.
>>> So, the current GPIO's  opjects structure is:
>>>
>>> <platform device> Davinci GPIO controller
>>>  |- <gpio0_chip0> ------|
>>>  ...                    |--- irq_domain (hwirq [0..143])
>>>  |- <gpio0_chipN> ------|
>>>
>>> Current driver creates one chip for every 32 GPIOs in a controller.
>>> This was a limitation earlier now there is no need for that. Hence
>>> redesigning the driver to create one gpio chip for all the ngpio
>>> in the controller.
>>>
>>> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).
>>>
>>> The previous discussion on this can be found here:
>>> https://www.spinics.net/lists/linux-omap/msg132869.html
>>
>> nice rework.
>
> Thanks!
>
>>
>>>
>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>> ---
>>>
>>> Boot tested on Davinci platform.
>>>
>>>  drivers/gpio/gpio-davinci.c                | 127
>>> +++++++++++++++++------------

[...]

>>>
>>>  #ifdef CONFIG_OF_GPIO
>>> -        chips[i].chip.of_gpio_n_cells = 2;
>>> -        chips[i].chip.of_xlate = davinci_gpio_of_xlate;
>>> -        chips[i].chip.parent = dev;
>>> -        chips[i].chip.of_node = dev->of_node;
>>> +    chips->chip.of_gpio_n_cells = 2;
>>> +    chips->chip.of_xlate = davinci_gpio_of_xlate;
>>
>> I think It's not necessary to have custom .xlate() and
>> it can be removed, then gpiolib will assign default one
>> of_gpio_simple_xlate().
>
> Okay. Can i do that as a separate patch?

I think it's ok.

>
>>
>>> +    chips->chip.parent = dev;
>>> +    chips->chip.of_node = dev->of_node;
>>>  #endif
>>> -        spin_lock_init(&chips[i].lock);
>>> -

[...]

>>>
>>>      irq_set_chip_and_handler_name(irq, &gpio_irqchip,
>>> handle_simple_irq,
>>>                  "davinci_gpio");
>>> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct
>>> platform_device *pdev)
>>>      struct irq_domain    *irq_domain = NULL;
>>>      const struct of_device_id *match;
>>>      struct irq_chip *irq_chip;
>>> +    struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS];
>>
>> You declare irqdata as array here but it has not been used anywhere
>> except for assignment. Could you remove this array and MAX_BANKED_IRQS
>> define?
>
> irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
>                          &chips[gpio / 32]);
>                          irqdata[bank]);
>
> That is hooked as data for each bank. As there is only one controller
> now and the differentiating parameters per bank is the irqdata data
> structure with the registers pointer and the bank number.
> This structure makes it very easy in the irq handler to identify the
> register sets that need to be modified and the bank irqs.

That I understood, but why do you need array here?

>
>>
>> Seems you can just use struct davinci_gpio_irq_data *irqdata;

why can't you use (see below):
	struct davinci_gpio_irq_data *irqdata;
>>
>>>      gpio_get_irq_chip_cb_t gpio_get_irq_chip;
>>>
>>>      /*
>>> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct
>>> platform_device *pdev)
>>>       * IRQs, while the others use banked IRQs, would need some setup
>>>       * tweaks to recognize hardware which can do that.
>>>       */

[...]

>>>
>>> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct
>>> platform_device *pdev)
>>>           * gpio irqs. Pass the irq bank's corresponding controller to
>>>           * the chained irq handler.
>>>           */
>>> +        irqdata[bank] = devm_kzalloc(&pdev->dev,
>>> +                         sizeof(struct
>>> +                            davinci_gpio_irq_data),
>>> +                         GFP_KERNEL);

irqdata = devm_kzalloc(&pdev->dev,
                         sizeof(struct
                             davinci_gpio_irq_data),
                          GFP_KERNEL);

>>> +        if (!irqdata[bank])
>>> +            return -ENOMEM;
>>> +
>>> +        irqdata[bank]->regs = g;
>>> +        irqdata[bank]->bank_num = bank;
>>> +        irqdata[bank]->chip = chips;
>>> +
>>>          irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
>>> -                         &chips[gpio / 32]);
>>> +                         irqdata[bank]);

          irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
                          irqdata);


[...]

-- 
regards,
-grygorii

Re: [PATCH 2/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Keerthy]

On Friday 13 January 2017 12:36 AM, Grygorii Strashko wrote:
>
>
> On 01/11/2017 08:00 PM, Keerthy wrote:
>>
>>
>> On Wednesday 11 January 2017 11:23 PM, Grygorii Strashko wrote:
>>>
>>>
>>> On 01/10/2017 11:00 PM, Keerthy wrote:
>>>> The Davinci GPIO driver is implemented to work with one monolithic
>>>> Davinci GPIO platform device which may have up to Y(144) gpios.
>>>> The Davinci GPIO driver instantiates number of GPIO chips with
>>>> max 32 gpio pins per each during initialization and one IRQ domain.
>>>> So, the current GPIO's  opjects structure is:
>>>>
>>>> <platform device> Davinci GPIO controller
>>>>  |- <gpio0_chip0> ------|
>>>>  ...                    |--- irq_domain (hwirq [0..143])
>>>>  |- <gpio0_chipN> ------|
>>>>
>>>> Current driver creates one chip for every 32 GPIOs in a controller.
>>>> This was a limitation earlier now there is no need for that. Hence
>>>> redesigning the driver to create one gpio chip for all the ngpio
>>>> in the controller.
>>>>
>>>> |- <gpio0_chip0> ------|--- irq_domain (hwirq [0..143]).
>>>>
>>>> The previous discussion on this can be found here:
>>>> https://www.spinics.net/lists/linux-omap/msg132869.html
>>>
>>> nice rework.
>>
>> Thanks!
>>
>>>
>>>>
>>>> Signed-off-by: Keerthy <j-keerthy@ti.com>
>>>> ---
>>>>
>>>> Boot tested on Davinci platform.
>>>>
>>>>  drivers/gpio/gpio-davinci.c                | 127
>>>> +++++++++++++++++------------
>
> [...]
>
>>>>
>>>>  #ifdef CONFIG_OF_GPIO
>>>> -        chips[i].chip.of_gpio_n_cells = 2;
>>>> -        chips[i].chip.of_xlate = davinci_gpio_of_xlate;
>>>> -        chips[i].chip.parent = dev;
>>>> -        chips[i].chip.of_node = dev->of_node;
>>>> +    chips->chip.of_gpio_n_cells = 2;
>>>> +    chips->chip.of_xlate = davinci_gpio_of_xlate;
>>>
>>> I think It's not necessary to have custom .xlate() and
>>> it can be removed, then gpiolib will assign default one
>>> of_gpio_simple_xlate().
>>
>> Okay. Can i do that as a separate patch?
>
> I think it's ok.
>
>>
>>>
>>>> +    chips->chip.parent = dev;
>>>> +    chips->chip.of_node = dev->of_node;
>>>>  #endif
>>>> -        spin_lock_init(&chips[i].lock);
>>>> -
>
> [...]
>
>>>>
>>>>      irq_set_chip_and_handler_name(irq, &gpio_irqchip,
>>>> handle_simple_irq,
>>>>                  "davinci_gpio");
>>>> @@ -459,6 +468,7 @@ static int davinci_gpio_irq_setup(struct
>>>> platform_device *pdev)
>>>>      struct irq_domain    *irq_domain = NULL;
>>>>      const struct of_device_id *match;
>>>>      struct irq_chip *irq_chip;
>>>> +    struct davinci_gpio_irq_data *irqdata[MAX_BANKED_IRQS];
>>>
>>> You declare irqdata as array here but it has not been used anywhere
>>> except for assignment. Could you remove this array and MAX_BANKED_IRQS
>>> define?
>>
>> irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
>>                          &chips[gpio / 32]);
>>                          irqdata[bank]);
>>
>> That is hooked as data for each bank. As there is only one controller
>> now and the differentiating parameters per bank is the irqdata data
>> structure with the registers pointer and the bank number.
>> This structure makes it very easy in the irq handler to identify the
>> register sets that need to be modified and the bank irqs.
>
> That I understood, but why do you need array here?
>
>>
>>>
>>> Seems you can just use struct davinci_gpio_irq_data *irqdata;
>
> why can't you use (see below):
>     struct davinci_gpio_irq_data *irqdata;

Understood your comment now thanks for clarifying. I will do like you 
have suggested.

>>>
>>>>      gpio_get_irq_chip_cb_t gpio_get_irq_chip;
>>>>
>>>>      /*
>>>> @@ -514,10 +524,8 @@ static int davinci_gpio_irq_setup(struct
>>>> platform_device *pdev)
>>>>       * IRQs, while the others use banked IRQs, would need some setup
>>>>       * tweaks to recognize hardware which can do that.
>>>>       */
>
> [...]
>
>>>>
>>>> @@ -567,8 +575,19 @@ static int davinci_gpio_irq_setup(struct
>>>> platform_device *pdev)
>>>>           * gpio irqs. Pass the irq bank's corresponding controller to
>>>>           * the chained irq handler.
>>>>           */
>>>> +        irqdata[bank] = devm_kzalloc(&pdev->dev,
>>>> +                         sizeof(struct
>>>> +                            davinci_gpio_irq_data),
>>>> +                         GFP_KERNEL);
>
> irqdata = devm_kzalloc(&pdev->dev,
>                         sizeof(struct
>                             davinci_gpio_irq_data),
>                          GFP_KERNEL);
>

Okay.

>>>> +        if (!irqdata[bank])
>>>> +            return -ENOMEM;
>>>> +
>>>> +        irqdata[bank]->regs = g;
>>>> +        irqdata[bank]->bank_num = bank;
>>>> +        irqdata[bank]->chip = chips;
>>>> +
>>>>          irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
>>>> -                         &chips[gpio / 32]);
>>>> +                         irqdata[bank]);
>
>          irq_set_chained_handler_and_data(bank_irq, gpio_irq_handler,
>                          irqdata);
>

I will post the new set with the above changes.

>
> [...]
>

Re: [PATCH 0/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Linus Walleij]

On Wed, Jan 11, 2017 at 6:00 AM, Keerthy <j-keerthy@ti.com> wrote:

> The Davinci GPIO driver is implemented to work with one monolithic
> Davinci GPIO platform device which may have up to Y(144) gpios.

I'm a bit confused by the different DaVinci series since there seem
to be different series floating and I don't know what order to apply them,
it fails for me.

Can you:

- Rebase *ALL* your DaVinci work on top of the "devel" branch in the
  GPIO tree.

- Repost *ALL* patches that should be applied, in one batch.

- Include any accumulated ACKs etc.

Yours,
Linus Walleij

Re: [PATCH 0/3] gpio: davinci: Redesign driver to accommodate ngpios in one gpio chip

[Keerthy]

On Tuesday 17 January 2017 08:22 PM, Linus Walleij wrote:
> On Wed, Jan 11, 2017 at 6:00 AM, Keerthy <j-keerthy@ti.com> wrote:
>
>> The Davinci GPIO driver is implemented to work with one monolithic
>> Davinci GPIO platform device which may have up to Y(144) gpios.
>
> I'm a bit confused by the different DaVinci series since there seem
> to be different series floating and I don't know what order to apply them,
> it fails for me.
>
> Can you:
>
> - Rebase *ALL* your DaVinci work on top of the "devel" branch in the
>   GPIO tree.
>
> - Repost *ALL* patches that should be applied, in one batch.
>
> - Include any accumulated ACKs etc.

:-)

I have already posted a consolidated v2:
http://www.spinics.net/lists/linux-gpio/msg19109.html

Then seems like patch 6/6 is not required. So i will be posting the 
final set with Grygorii's reviewed-by. You can directly pick the v3 set.

Thanks,
Keerthy


>
> Yours,
> Linus Walleij
>