* [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement
@ 2013-11-08 10:11 Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 1/7] gpio: davinci: remove unnecessary printk Prabhakar Lad
` (7 more replies)
0 siblings, 8 replies; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: KV Sujith <sujithkv@ti.com>
This patch series does the following
1> Ports the driver to use irqdomain.
2> Adds dt binding support for gpio-davinci.
3> Adds DA850 dt support goio.
Changes for v5:
1: Fixed review comments pointed by Grygorii.
2: Dropped the fixup patch
This patch series is based on following fixup series
http://us.generation-nt.com/answer/patch-0-2-davinci-gpio-fixes-help-212978272.html
KV Sujith (3):
gpio: davinci: add OF support
ARM: davinci: da850: add GPIO DT node
ARM: davinci: da850 evm: add GPIO pinumux entries DT node
Lad, Prabhakar (4):
gpio: davinci: remove unnecessary printk
gpio: davinci: use readl/writel instead of __raw_*
gpio: davinci: use irqdomain
gpio: davinci: remove unused variable intc_irq_num
.../devicetree/bindings/gpio/gpio-davinci.txt | 39 +++++
arch/arm/boot/dts/da850-evm.dts | 20 +++
arch/arm/boot/dts/da850.dtsi | 14 ++
arch/arm/mach-davinci/da830.c | 1 -
arch/arm/mach-davinci/da850.c | 1 -
arch/arm/mach-davinci/dm355.c | 1 -
arch/arm/mach-davinci/dm365.c | 1 -
arch/arm/mach-davinci/dm644x.c | 1 -
arch/arm/mach-davinci/dm646x.c | 1 -
drivers/gpio/gpio-davinci.c | 170 +++++++++++++-------
include/linux/platform_data/gpio-davinci.h | 3 +-
11 files changed, 185 insertions(+), 67 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
--
1.7.9.5
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v5 1/7] gpio: davinci: remove unnecessary printk
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-08 15:39 ` Grygorii Strashko
2013-11-08 10:11 ` [PATCH v5 2/7] gpio: davinci: use readl/writel instead of __raw_* Prabhakar Lad
` (6 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
the devm_*() helper prints error messages in case of
errors no need to do the same in the driver.
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
drivers/gpio/gpio-davinci.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 84be701..7230c43 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -389,11 +389,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
}
clk = devm_clk_get(dev, "gpio");
- if (IS_ERR(clk)) {
- printk(KERN_ERR "Error %ld getting gpio clock?\n",
- PTR_ERR(clk));
+ if (IS_ERR(clk))
return PTR_ERR(clk);
- }
+
clk_prepare_enable(clk);
/*
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 2/7] gpio: davinci: use readl/writel instead of __raw_*
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 1/7] gpio: davinci: remove unnecessary printk Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-18 13:01 ` Linus Walleij
2013-11-08 10:11 ` [PATCH v5 3/7] gpio: davinci: use irqdomain Prabhakar Lad
` (5 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
This patch replaces the __raw_readl/writel with
readl and writel, Although the code runs on ARMv5
based SOCs, changing this will help copying the code
for other uses.
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
drivers/gpio/gpio-davinci.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 7230c43..ca3d7fd 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -82,14 +82,14 @@ static inline int __davinci_direction(struct gpio_chip *chip,
u32 mask = 1 << offset;
spin_lock_irqsave(&d->lock, flags);
- temp = __raw_readl(&g->dir);
+ temp = readl(&g->dir);
if (out) {
temp &= ~mask;
- __raw_writel(mask, value ? &g->set_data : &g->clr_data);
+ writel(mask, value ? &g->set_data : &g->clr_data);
} else {
temp |= mask;
}
- __raw_writel(temp, &g->dir);
+ writel(temp, &g->dir);
spin_unlock_irqrestore(&d->lock, flags);
return 0;
@@ -118,7 +118,7 @@ static int davinci_gpio_get(struct gpio_chip *chip, unsigned offset)
struct davinci_gpio_controller *d = chip2controller(chip);
struct davinci_gpio_regs __iomem *g = d->regs;
- return (1 << offset) & __raw_readl(&g->in_data);
+ return (1 << offset) & readl(&g->in_data);
}
/*
@@ -130,7 +130,7 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
struct davinci_gpio_controller *d = chip2controller(chip);
struct davinci_gpio_regs __iomem *g = d->regs;
- __raw_writel((1 << offset), value ? &g->set_data : &g->clr_data);
+ writel((1 << offset), value ? &g->set_data : &g->clr_data);
}
static int davinci_gpio_probe(struct platform_device *pdev)
@@ -227,8 +227,8 @@ static void gpio_irq_disable(struct irq_data *d)
struct davinci_gpio_regs __iomem *g = irq2regs(d->irq);
u32 mask = (u32) irq_data_get_irq_handler_data(d);
- __raw_writel(mask, &g->clr_falling);
- __raw_writel(mask, &g->clr_rising);
+ writel(mask, &g->clr_falling);
+ writel(mask, &g->clr_rising);
}
static void gpio_irq_enable(struct irq_data *d)
@@ -242,9 +242,9 @@ static void gpio_irq_enable(struct irq_data *d)
status = IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING;
if (status & IRQ_TYPE_EDGE_FALLING)
- __raw_writel(mask, &g->set_falling);
+ writel(mask, &g->set_falling);
if (status & IRQ_TYPE_EDGE_RISING)
- __raw_writel(mask, &g->set_rising);
+ writel(mask, &g->set_rising);
}
static int gpio_irq_type(struct irq_data *d, unsigned trigger)
@@ -286,10 +286,10 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
int res;
/* ack any irqs */
- status = __raw_readl(&g->intstat) & mask;
+ status = readl(&g->intstat) & mask;
if (!status)
break;
- __raw_writel(status, &g->intstat);
+ writel(status, &g->intstat);
/* now demux them to the right lowlevel handler */
n = d->irq_base;
@@ -346,9 +346,9 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
- __raw_writel(mask, (trigger & IRQ_TYPE_EDGE_FALLING)
+ writel(mask, (trigger & IRQ_TYPE_EDGE_FALLING)
? &g->set_falling : &g->clr_falling);
- __raw_writel(mask, (trigger & IRQ_TYPE_EDGE_RISING)
+ writel(mask, (trigger & IRQ_TYPE_EDGE_RISING)
? &g->set_rising : &g->clr_rising);
return 0;
@@ -430,8 +430,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
/* default trigger: both edges */
g = gpio2regs(0);
- __raw_writel(~0, &g->set_falling);
- __raw_writel(~0, &g->set_rising);
+ writel(~0, &g->set_falling);
+ writel(~0, &g->set_rising);
/* set the direct IRQs up to use that irqchip */
for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
@@ -454,8 +454,8 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
/* disabled by default, enabled only as needed */
g = gpio2regs(gpio);
- __raw_writel(~0, &g->clr_falling);
- __raw_writel(~0, &g->clr_rising);
+ writel(~0, &g->clr_falling);
+ writel(~0, &g->clr_rising);
/* set up all irqs in this bank */
irq_set_chained_handler(bank_irq, gpio_irq_handler);
@@ -483,7 +483,7 @@ done:
* BINTEN -- per-bank interrupt enable. genirq would also let these
* bits be set/cleared dynamically.
*/
- __raw_writel(binten, gpio_base + BINTEN);
+ writel(binten, gpio_base + BINTEN);
printk(KERN_INFO "DaVinci: %d gpio irqs\n", irq - gpio_to_irq(0));
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 1/7] gpio: davinci: remove unnecessary printk Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 2/7] gpio: davinci: use readl/writel instead of __raw_* Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-18 13:11 ` Linus Walleij
2013-11-08 10:11 ` [PATCH v5 4/7] gpio: davinci: remove unused variable intc_irq_num Prabhakar Lad
` (4 subsequent siblings)
7 siblings, 1 reply; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
This patch converts the davinci gpio driver to use irqdomain
support.
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
drivers/gpio/gpio-davinci.c | 74 +++++++++++++++-------------
include/linux/platform_data/gpio-davinci.h | 2 +-
2 files changed, 41 insertions(+), 35 deletions(-)
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index ca3d7fd..b149239 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -16,6 +16,7 @@
#include <linux/err.h>
#include <linux/io.h>
#include <linux/irq.h>
+#include <linux/irqdomain.h>
#include <linux/platform_device.h>
#include <linux/platform_data/gpio-davinci.h>
@@ -282,8 +283,7 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
desc->irq_data.chip->irq_ack(&desc->irq_data);
while (1) {
u32 status;
- int n;
- int res;
+ int bit;
/* ack any irqs */
status = readl(&g->intstat) & mask;
@@ -292,17 +292,11 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
writel(status, &g->intstat);
/* now demux them to the right lowlevel handler */
- n = d->irq_base;
- if (irq & 1) {
- n += 16;
- status >>= 16;
- }
-
while (status) {
- res = ffs(status);
- n += res;
- generic_handle_irq(n - 1);
- status >>= res;
+ bit = __ffs(status);
+ status &= ~(1 << bit);
+ generic_handle_irq(irq_find_mapping(d->irq_domain,
+ bit));
}
}
desc->irq_data.chip->irq_unmask(&desc->irq_data);
@@ -313,10 +307,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
{
struct davinci_gpio_controller *d = chip2controller(chip);
- if (d->irq_base >= 0)
- return d->irq_base + offset;
- else
- return -ENODEV;
+ return irq_create_mapping(d->irq_domain, offset);
}
static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
@@ -354,6 +345,28 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
return 0;
}
+static int davinci_gpio_irq_map(struct irq_domain *d, unsigned int irq,
+ irq_hw_number_t hw)
+{
+ struct davinci_gpio_controller *chip = d->host_data;
+ unsigned gpio = chip->chip.base + hw;
+ struct davinci_gpio_regs __iomem *g = gpio2regs(gpio);
+
+ irq_set_chip_and_handler_name(irq, &gpio_irqchip, handle_simple_irq,
+ "davinci_gpio");
+ irq_set_irq_type(irq, IRQ_TYPE_NONE);
+ irq_set_chip_data(irq, (__force void *)g);
+ irq_set_handler_data(irq, (void *)__gpio_mask(gpio));
+ set_irq_flags(irq, IRQF_VALID);
+
+ return 0;
+}
+
+static const struct irq_domain_ops davinci_gpio_irq_ops = {
+ .map = davinci_gpio_irq_map,
+ .xlate = irq_domain_xlate_onetwocell,
+};
+
/*
* NOTE: for suspend/resume, probably best to make a platform_device with
* suspend_late/resume_resume calls hooking into results of the set_wake()
@@ -402,9 +415,16 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
*/
for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
chips[bank].chip.to_irq = gpio_to_irq_banked;
- chips[bank].irq_base = pdata->gpio_unbanked
- ? -EINVAL
- : (pdata->intc_irq_num + gpio);
+ if (!pdata->gpio_unbanked) {
+ chips[bank].irq_domain =
+ irq_domain_add_linear(NULL,
+ chips[bank].chip.ngpio,
+ &davinci_gpio_irq_ops,
+ &chips[bank]);
+
+ if (!chips[bank].irq_domain)
+ return -ENOMEM;
+ }
}
/*
@@ -447,11 +467,7 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
* Or, AINTC can handle IRQs for banks of 16 GPIO IRQs, which we
* then chain through our own handler.
*/
- for (gpio = 0, irq = gpio_to_irq(0), bank = 0;
- gpio < ngpio;
- bank++, bank_irq++) {
- unsigned i;
-
+ for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
/* disabled by default, enabled only as needed */
g = gpio2regs(gpio);
writel(~0, &g->clr_falling);
@@ -467,14 +483,6 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
*/
irq_set_handler_data(bank_irq, &chips[gpio / 32]);
- for (i = 0; i < 16 && gpio < ngpio; i++, irq++, gpio++) {
- irq_set_chip(irq, &gpio_irqchip);
- irq_set_chip_data(irq, (__force void *)g);
- irq_set_handler_data(irq, (void *)__gpio_mask(gpio));
- irq_set_handler(irq, handle_simple_irq);
- set_irq_flags(irq, IRQF_VALID);
- }
-
binten |= BIT(bank);
}
@@ -485,8 +493,6 @@ done:
*/
writel(binten, gpio_base + BINTEN);
- printk(KERN_INFO "DaVinci: %d gpio irqs\n", irq - gpio_to_irq(0));
-
return 0;
}
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index 6efd202..0c3551b 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -34,7 +34,7 @@ struct davinci_gpio_platform_data {
struct davinci_gpio_controller {
struct gpio_chip chip;
- int irq_base;
+ struct irq_domain *irq_domain;
/* Serialize access to GPIO registers */
spinlock_t lock;
void __iomem *regs;
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 4/7] gpio: davinci: remove unused variable intc_irq_num
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
` (2 preceding siblings ...)
2013-11-08 10:11 ` [PATCH v5 3/7] gpio: davinci: use irqdomain Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 5/7] gpio: davinci: add OF support Prabhakar Lad
` (3 subsequent siblings)
7 siblings, 0 replies; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
As the davinci-gpio driver is migrated to use irqdomain
there is no need to pass the irq base for the gpio driver.
This patch removes this variable from davinci_gpio_platform_data
and also the references from the machine file.
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
arch/arm/mach-davinci/da830.c | 1 -
arch/arm/mach-davinci/da850.c | 1 -
arch/arm/mach-davinci/dm355.c | 1 -
arch/arm/mach-davinci/dm365.c | 1 -
arch/arm/mach-davinci/dm644x.c | 1 -
arch/arm/mach-davinci/dm646x.c | 1 -
include/linux/platform_data/gpio-davinci.h | 1 -
7 files changed, 7 deletions(-)
diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
index 0813b51..fb72035 100644
--- a/arch/arm/mach-davinci/da830.c
+++ b/arch/arm/mach-davinci/da830.c
@@ -1153,7 +1153,6 @@ static struct davinci_id da830_ids[] = {
static struct davinci_gpio_platform_data da830_gpio_platform_data = {
.ngpio = 128,
- .intc_irq_num = DA830_N_CP_INTC_IRQ,
};
int __init da830_register_gpio(void)
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 352984e..4379317 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -1283,7 +1283,6 @@ int __init da850_register_vpif_capture(struct vpif_capture_config
static struct davinci_gpio_platform_data da850_gpio_platform_data = {
.ngpio = 144,
- .intc_irq_num = DA850_N_CP_INTC_IRQ,
};
int __init da850_register_gpio(void)
diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
index 536ce52..a1713cc 100644
--- a/arch/arm/mach-davinci/dm355.c
+++ b/arch/arm/mach-davinci/dm355.c
@@ -900,7 +900,6 @@ static struct resource dm355_gpio_resources[] = {
static struct davinci_gpio_platform_data dm355_gpio_platform_data = {
.ngpio = 104,
- .intc_irq_num = DAVINCI_N_AINTC_IRQ,
};
int __init dm355_gpio_register(void)
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index 9c96520..2595097 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
@@ -713,7 +713,6 @@ static struct resource dm365_gpio_resources[] = {
static struct davinci_gpio_platform_data dm365_gpio_platform_data = {
.ngpio = 104,
- .intc_irq_num = DAVINCI_N_AINTC_IRQ,
.gpio_unbanked = 8,
};
diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
index 72a3aa7..d83436d 100644
--- a/arch/arm/mach-davinci/dm644x.c
+++ b/arch/arm/mach-davinci/dm644x.c
@@ -786,7 +786,6 @@ static struct resource dm644_gpio_resources[] = {
static struct davinci_gpio_platform_data dm644_gpio_platform_data = {
.ngpio = 71,
- .intc_irq_num = DAVINCI_N_AINTC_IRQ,
};
int __init dm644x_gpio_register(void)
diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index d1b646c..5d289f4 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -763,7 +763,6 @@ static struct resource dm646x_gpio_resources[] = {
static struct davinci_gpio_platform_data dm646x_gpio_platform_data = {
.ngpio = 43,
- .intc_irq_num = DAVINCI_N_AINTC_IRQ,
};
int __init dm646x_gpio_register(void)
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index 0c3551b..fbe2f75 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -28,7 +28,6 @@ enum davinci_gpio_type {
struct davinci_gpio_platform_data {
u32 ngpio;
u32 gpio_unbanked;
- u32 intc_irq_num;
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 5/7] gpio: davinci: add OF support
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
` (3 preceding siblings ...)
2013-11-08 10:11 ` [PATCH v5 4/7] gpio: davinci: remove unused variable intc_irq_num Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-11 15:18 ` Grygorii Strashko
` (2 more replies)
2013-11-08 10:11 ` [PATCH v5 6/7] ARM: davinci: da850: add GPIO DT node Prabhakar Lad
` (2 subsequent siblings)
7 siblings, 3 replies; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: KV Sujith <sujithkv@ti.com>
This patch adds OF parser support for davinci gpio
driver and also appropriate documentation in gpio-davinci.txt
located at Documentation/devicetree/bindings/gpio/.
Signed-off-by: KV Sujith <sujithkv@ti.com>
Signed-off-by: Philip Avinash <avinashphilip@ti.com>
[prabhakar.csengg@gmail.com: simplified the OF code, removed
unnecessary DT property and also simplified
the commit message]
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
.../devicetree/bindings/gpio/gpio-davinci.txt | 39 ++++++++++++++
drivers/gpio/gpio-davinci.c | 54 ++++++++++++++++++--
2 files changed, 90 insertions(+), 3 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
new file mode 100644
index 0000000..d677bbe
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
@@ -0,0 +1,39 @@
+Davinci GPIO controller bindings
+
+Required Properties:
+- compatible: should be "ti,dm6441-gpio"
+
+- reg: Physical base address of the controller and the size of memory mapped
+ registers.
+
+- gpio-controller : Marks the device node as a gpio controller.
+
+- interrupt-parent: phandle of the parent interrupt controller.
+
+- interrupts: Array of GPIO interrupt number. Only banked or unbanked IRQs are
+ supported at a time.
+
+- ti,ngpio: The number of GPIO pins supported.
+
+- ti,davinci-gpio-unbanked: The number of GPIOs that have an individual interrupt
+ line to processor.
+
+The GPIO controller also acts as an interrupt controller. It uses the default
+two cells specifier as described in Documentation/devicetree/bindings/
+interrupt-controller/interrupts.txt.
+
+Example:
+
+gpio: gpio@1e26000 {
+ compatible = "ti,dm6441-gpio";
+ gpio-controller;
+ reg = <0x226000 0x1000>;
+ interrupt-parent = <&intc>;
+ interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
+ 44 IRQ_TYPE_EDGE_BOTH 45 IRQ_TYPE_EDGE_BOTH
+ 46 IRQ_TYPE_EDGE_BOTH 47 IRQ_TYPE_EDGE_BOTH
+ 48 IRQ_TYPE_EDGE_BOTH 49 IRQ_TYPE_EDGE_BOTH
+ 50 IRQ_TYPE_EDGE_BOTH>;
+ ti,ngpio = <144>;
+ ti,davinci-gpio-unbanked = <0>;
+};
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index b149239..ed04835 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -17,6 +17,9 @@
#include <linux/io.h>
#include <linux/irq.h>
#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/platform_data/gpio-davinci.h>
@@ -134,6 +137,40 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
writel((1 << offset), value ? &g->set_data : &g->clr_data);
}
+static struct davinci_gpio_platform_data *
+davinci_gpio_get_pdata(struct platform_device *pdev)
+{
+ struct device_node *dn = pdev->dev.of_node;
+ struct davinci_gpio_platform_data *pdata;
+ int ret;
+ u32 val;
+
+ if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
+ return pdev->dev.platform_data;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (!pdata)
+ return NULL;
+
+ ret = of_property_read_u32(dn, "ti,ngpio", &val);
+ if (ret)
+ goto of_err;
+
+ pdata->ngpio = val;
+
+ ret = of_property_read_u32(dn, "ti,davinci-gpio-unbanked", &val);
+ if (ret)
+ goto of_err;
+
+ pdata->gpio_unbanked = val;
+
+ return pdata;
+
+of_err:
+ dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
+ return NULL;
+}
+
static int davinci_gpio_probe(struct platform_device *pdev)
{
int i, base;
@@ -144,12 +181,14 @@ static int davinci_gpio_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct resource *res;
- pdata = dev->platform_data;
+ pdata = davinci_gpio_get_pdata(pdev);
if (!pdata) {
dev_err(dev, "No platform data found\n");
return -EINVAL;
}
+ dev->platform_data = pdata;
+
/*
* The gpio banks conceptually expose a segmented bitmap,
* and "ngpio" is one more than the largest zero-based
@@ -496,11 +535,20 @@ done:
return 0;
}
+#if IS_ENABLED(CONFIG_OF)
+static const struct of_device_id davinci_gpio_ids[] = {
+ { .compatible = "ti,dm6441-gpio", },
+ { /* sentinel */ },
+};
+MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
+#endif
+
static struct platform_driver davinci_gpio_driver = {
.probe = davinci_gpio_probe,
.driver = {
- .name = "davinci_gpio",
- .owner = THIS_MODULE,
+ .name = "davinci_gpio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(davinci_gpio_ids),
},
};
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 6/7] ARM: davinci: da850: add GPIO DT node
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
` (4 preceding siblings ...)
2013-11-08 10:11 ` [PATCH v5 5/7] gpio: davinci: add OF support Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 7/7] ARM: davinci: da850 evm: add GPIO pinumux entries " Prabhakar Lad
2013-11-11 15:52 ` [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Grygorii Strashko
7 siblings, 0 replies; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: KV Sujith <sujithkv@ti.com>
Add DT node for Davinci GPIO driver.
Signed-off-by: KV Sujith <sujithkv@ti.com>
Signed-off-by: Philip Avinash <avinashphilip@ti.com>
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
arch/arm/boot/dts/da850.dtsi | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 8d17346..b695548 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -8,6 +8,7 @@
* option) any later version.
*/
#include "skeleton.dtsi"
+#include <dt-bindings/interrupt-controller/irq.h>
/ {
arm {
@@ -256,6 +257,19 @@
36
>;
};
+ gpio: gpio@1e26000 {
+ compatible = "ti,dm6441-gpio";
+ gpio-controller;
+ reg = <0x226000 0x1000>;
+ interrupts = <42 IRQ_TYPE_EDGE_BOTH
+ 43 IRQ_TYPE_EDGE_BOTH 44 IRQ_TYPE_EDGE_BOTH
+ 45 IRQ_TYPE_EDGE_BOTH 46 IRQ_TYPE_EDGE_BOTH
+ 47 IRQ_TYPE_EDGE_BOTH 48 IRQ_TYPE_EDGE_BOTH
+ 49 IRQ_TYPE_EDGE_BOTH 50 IRQ_TYPE_EDGE_BOTH>;
+ ti,ngpio = <144>;
+ ti,davinci-gpio-unbanked = <0>;
+ status = "disabled";
+ };
};
nand_cs3@62000000 {
compatible = "ti,davinci-nand";
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v5 7/7] ARM: davinci: da850 evm: add GPIO pinumux entries DT node
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
` (5 preceding siblings ...)
2013-11-08 10:11 ` [PATCH v5 6/7] ARM: davinci: da850: add GPIO DT node Prabhakar Lad
@ 2013-11-08 10:11 ` Prabhakar Lad
2013-11-11 15:52 ` [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Grygorii Strashko
7 siblings, 0 replies; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-08 10:11 UTC (permalink / raw)
To: Sekhar Nori, Linus Walleij, Grygorii Strashko
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Prabhakar Lad, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
From: KV Sujith <sujithkv@ti.com>
Add GPIO DT node and pinmux entries for DA850 EVM. GPIO is
configurable differently on different boards. So add GPIO
pinmuxing in dts file.
Signed-off-by: KV Sujith <sujithkv@ti.com>
Signed-off-by: Philip Avinash <avinashphilip@ti.com>
Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
---
arch/arm/boot/dts/da850-evm.dts | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index 588ce58..f82c129 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -17,6 +17,21 @@
soc {
pmx_core: pinmux@1c14120 {
status = "okay";
+
+ gpio_pins: pinmux_gpio_pins {
+ pinctrl-single,bits = <
+ /* GPIO2_4 GPIO2_6 */
+ 0x18 0x00008080 0x0000f0f0
+ /* GPIO2_8 GPIO2_15 */
+ 0x14 0x80000008 0xf000000f
+ /* GPIO3_12 GPIO3_13 */
+ 0x1C 0x00008800 0x0000ff00
+ /* GPIO4_0 GPIO4_1 */
+ 0x28 0x88000000 0xff000000
+ /* GPIO6_9 GPIO6_10 GPIO6_13 */
+ 0x34 0x08800800 0x0ff00f00
+ >;
+ };
};
serial0: serial@1c42000 {
status = "okay";
@@ -101,6 +116,11 @@
pinctrl-names = "default";
pinctrl-0 = <&mii_pins>;
};
+ gpio: gpio@1e26000 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&gpio_pins>;
+ };
};
nand_cs3@62000000 {
status = "okay";
--
1.7.9.5
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/7] gpio: davinci: remove unnecessary printk
2013-11-08 10:11 ` [PATCH v5 1/7] gpio: davinci: remove unnecessary printk Prabhakar Lad
@ 2013-11-08 15:39 ` Grygorii Strashko
2013-11-11 15:39 ` Prabhakar Lad
0 siblings, 1 reply; 23+ messages in thread
From: Grygorii Strashko @ 2013-11-08 15:39 UTC (permalink / raw)
To: Prabhakar Lad, Sekhar Nori, Linus Walleij
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
Rob Landley, Grant Likely
On 11/08/2013 12:11 PM, Prabhakar Lad wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>
> the devm_*() helper prints error messages in case of
> errors no need to do the same in the driver.
Pls, drop it - devm_clk_get() doesn't always print error messages
>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
> drivers/gpio/gpio-davinci.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 84be701..7230c43 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -389,11 +389,9 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
> }
>
> clk = devm_clk_get(dev, "gpio");
> - if (IS_ERR(clk)) {
> - printk(KERN_ERR "Error %ld getting gpio clock?\n",
> - PTR_ERR(clk));
> + if (IS_ERR(clk))
> return PTR_ERR(clk);
> - }
> +
> clk_prepare_enable(clk);
>
> /*
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/7] gpio: davinci: add OF support
2013-11-08 10:11 ` [PATCH v5 5/7] gpio: davinci: add OF support Prabhakar Lad
@ 2013-11-11 15:18 ` Grygorii Strashko
2013-11-11 15:37 ` Prabhakar Lad
2013-11-18 13:13 ` Linus Walleij
2013-11-18 13:42 ` Rob Herring
2 siblings, 1 reply; 23+ messages in thread
From: Grygorii Strashko @ 2013-11-11 15:18 UTC (permalink / raw)
To: Prabhakar Lad, Sekhar Nori, Linus Walleij
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
Rob Landley, Grant Likely
On 11/08/2013 12:11 PM, Prabhakar Lad wrote:
> From: KV Sujith <sujithkv@ti.com>
>
> This patch adds OF parser support for davinci gpio
> driver and also appropriate documentation in gpio-davinci.txt
> located at Documentation/devicetree/bindings/gpio/.
>
> Signed-off-by: KV Sujith <sujithkv@ti.com>
> Signed-off-by: Philip Avinash <avinashphilip@ti.com>
> [prabhakar.csengg@gmail.com: simplified the OF code, removed
> unnecessary DT property and also simplified
> the commit message]
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
> ---
> .../devicetree/bindings/gpio/gpio-davinci.txt | 39 ++++++++++++++
> drivers/gpio/gpio-davinci.c | 54 ++++++++++++++++++--
> 2 files changed, 90 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> new file mode 100644
> index 0000000..d677bbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> @@ -0,0 +1,39 @@
> +Davinci GPIO controller bindings
> +
> +Required Properties:
> +- compatible: should be "ti,dm6441-gpio"
> +
> +- reg: Physical base address of the controller and the size of memory mapped
> + registers.
> +
> +- gpio-controller : Marks the device node as a gpio controller.
> +
> +- interrupt-parent: phandle of the parent interrupt controller.
> +
> +- interrupts: Array of GPIO interrupt number. Only banked or unbanked IRQs are
> + supported at a time.
> +
> +- ti,ngpio: The number of GPIO pins supported.
> +
> +- ti,davinci-gpio-unbanked: The number of GPIOs that have an individual interrupt
> + line to processor.
> +
> +The GPIO controller also acts as an interrupt controller. It uses the default
> +two cells specifier as described in Documentation/devicetree/bindings/
> +interrupt-controller/interrupts.txt.
> +
> +Example:
> +
> +gpio: gpio@1e26000 {
> + compatible = "ti,dm6441-gpio";
> + gpio-controller;
> + reg = <0x226000 0x1000>;
> + interrupt-parent = <&intc>;
> + interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
> + 44 IRQ_TYPE_EDGE_BOTH 45 IRQ_TYPE_EDGE_BOTH
> + 46 IRQ_TYPE_EDGE_BOTH 47 IRQ_TYPE_EDGE_BOTH
> + 48 IRQ_TYPE_EDGE_BOTH 49 IRQ_TYPE_EDGE_BOTH
> + 50 IRQ_TYPE_EDGE_BOTH>;
> + ti,ngpio = <144>;
> + ti,davinci-gpio-unbanked = <0>;
> +};
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index b149239..ed04835 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -17,6 +17,9 @@
> #include <linux/io.h>
> #include <linux/irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/platform_data/gpio-davinci.h>
>
> @@ -134,6 +137,40 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> writel((1 << offset), value ? &g->set_data : &g->clr_data);
> }
>
> +static struct davinci_gpio_platform_data *
> +davinci_gpio_get_pdata(struct platform_device *pdev)
Minor: usually such functions have "_of" in their names:
davinci_gpio_of_get_pdata()
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct davinci_gpio_platform_data *pdata;
> + int ret;
> + u32 val;
> +
> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> + return pdev->dev.platform_data;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return NULL;
> +
> + ret = of_property_read_u32(dn, "ti,ngpio", &val);
> + if (ret)
> + goto of_err;
> +
> + pdata->ngpio = val;
> +
> + ret = of_property_read_u32(dn, "ti,davinci-gpio-unbanked", &val);
> + if (ret)
> + goto of_err;
> +
> + pdata->gpio_unbanked = val;
> +
> + return pdata;
> +
> +of_err:
> + dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
> + return NULL;
> +}
> +
> static int davinci_gpio_probe(struct platform_device *pdev)
> {
> int i, base;
> @@ -144,12 +181,14 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct resource *res;
>
> - pdata = dev->platform_data;
> + pdata = davinci_gpio_get_pdata(pdev);
> if (!pdata) {
> dev_err(dev, "No platform data found\n");
> return -EINVAL;
> }
>
> + dev->platform_data = pdata;
> +
Pls, add following code to GPIO chip initialization:
@@ -233,6 +245,9 @@ static int davinci_gpio_probe(struct platform_device *pdev)
chips[i].chip.ngpio = ngpio - base;
if (chips[i].chip.ngpio > 32)
chips[i].chip.ngpio = 32;
+#ifdef CONFIG_OF_GPIO
+ chips[i].chip.of_node = dev->of_node;
+#endif
> /*
> * The gpio banks conceptually expose a segmented bitmap,
> * and "ngpio" is one more than the largest zero-based
> @@ -496,11 +535,20 @@ done:
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id davinci_gpio_ids[] = {
> + { .compatible = "ti,dm6441-gpio", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
> +#endif
> +
> static struct platform_driver davinci_gpio_driver = {
> .probe = davinci_gpio_probe,
> .driver = {
> - .name = "davinci_gpio",
> - .owner = THIS_MODULE,
> + .name = "davinci_gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(davinci_gpio_ids),
> },
> };
>
>
Regards,
- grygorii
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/7] gpio: davinci: add OF support
2013-11-11 15:18 ` Grygorii Strashko
@ 2013-11-11 15:37 ` Prabhakar Lad
2013-11-11 15:44 ` Grygorii Strashko
0 siblings, 1 reply; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-11 15:37 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Sekhar Nori, Linus Walleij, LKML, DLOS, LAK, linux-gpio,
devicetree, LDOC, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
Hi Grygorii,
Thanks for the review.
On Mon, Nov 11, 2013 at 8:48 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 11/08/2013 12:11 PM, Prabhakar Lad wrote:
[Snip]
>> @@ -134,6 +137,40 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>> writel((1 << offset), value ? &g->set_data : &g->clr_data);
>> }
>>
>> +static struct davinci_gpio_platform_data *
>> +davinci_gpio_get_pdata(struct platform_device *pdev)
>
> Minor: usually such functions have "_of" in their names:
> davinci_gpio_of_get_pdata()
>
Ahh but actual this function is intended to get pdata in both the
cases DT and NON-DT, so kept it generic :)
>> +{
>> + struct device_node *dn = pdev->dev.of_node;
>> + struct davinci_gpio_platform_data *pdata;
>> + int ret;
>> + u32 val;
>> +
>> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>> + return pdev->dev.platform_data;
>> +
>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>> + if (!pdata)
>> + return NULL;
>> +
>> + ret = of_property_read_u32(dn, "ti,ngpio", &val);
>> + if (ret)
>> + goto of_err;
>> +
>> + pdata->ngpio = val;
>> +
>> + ret = of_property_read_u32(dn, "ti,davinci-gpio-unbanked", &val);
>> + if (ret)
>> + goto of_err;
>> +
>> + pdata->gpio_unbanked = val;
>> +
>> + return pdata;
>> +
>> +of_err:
>> + dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
>> + return NULL;
>> +}
>> +
>> static int davinci_gpio_probe(struct platform_device *pdev)
>> {
>> int i, base;
>> @@ -144,12 +181,14 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>> struct device *dev = &pdev->dev;
>> struct resource *res;
>>
>> - pdata = dev->platform_data;
>> + pdata = davinci_gpio_get_pdata(pdev);
>> if (!pdata) {
>> dev_err(dev, "No platform data found\n");
>> return -EINVAL;
>> }
>>
>> + dev->platform_data = pdata;
>> +
>
> Pls, add following code to GPIO chip initialization:
>
> @@ -233,6 +245,9 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> chips[i].chip.ngpio = ngpio - base;
> if (chips[i].chip.ngpio > 32)
> chips[i].chip.ngpio = 32;
> +#ifdef CONFIG_OF_GPIO
> + chips[i].chip.of_node = dev->of_node;
> +#endif
>
>
OK
Regards,
--Prabhakar Lad
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 1/7] gpio: davinci: remove unnecessary printk
2013-11-08 15:39 ` Grygorii Strashko
@ 2013-11-11 15:39 ` Prabhakar Lad
0 siblings, 0 replies; 23+ messages in thread
From: Prabhakar Lad @ 2013-11-11 15:39 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Sekhar Nori, Linus Walleij, LKML, DLOS, LAK, linux-gpio,
devicetree, LDOC, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
Hi Grygorii,
Thanks for the review.
On Fri, Nov 8, 2013 at 9:09 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 11/08/2013 12:11 PM, Prabhakar Lad wrote:
>>
>> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>>
>> the devm_*() helper prints error messages in case of
>> errors no need to do the same in the driver.
>
>
> Pls, drop it - devm_clk_get() doesn't always print error messages
>
OK will drop in the next version.
Regards,
--Prabhakar Lad
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/7] gpio: davinci: add OF support
2013-11-11 15:37 ` Prabhakar Lad
@ 2013-11-11 15:44 ` Grygorii Strashko
0 siblings, 0 replies; 23+ messages in thread
From: Grygorii Strashko @ 2013-11-11 15:44 UTC (permalink / raw)
To: Prabhakar Lad
Cc: Sekhar Nori, Linus Walleij, LKML, DLOS, LAK, linux-gpio,
devicetree, LDOC, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On 11/11/2013 05:37 PM, Prabhakar Lad wrote:
> Hi Grygorii,
>
> Thanks for the review.
>
> On Mon, Nov 11, 2013 at 8:48 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 11/08/2013 12:11 PM, Prabhakar Lad wrote:
> [Snip]
>>> @@ -134,6 +137,40 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>>> writel((1 << offset), value ? &g->set_data : &g->clr_data);
>>> }
>>>
>>> +static struct davinci_gpio_platform_data *
>>> +davinci_gpio_get_pdata(struct platform_device *pdev)
>>
>> Minor: usually such functions have "_of" in their names:
>> davinci_gpio_of_get_pdata()
>>
> Ahh but actual this function is intended to get pdata in both the
> cases DT and NON-DT, so kept it generic :)
np
>
>>> +{
>>> + struct device_node *dn = pdev->dev.of_node;
>>> + struct davinci_gpio_platform_data *pdata;
>>> + int ret;
>>> + u32 val;
>>> +
>>> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
>>> + return pdev->dev.platform_data;
>>> +
>>> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
>>> + if (!pdata)
>>> + return NULL;
>>> +
>>> + ret = of_property_read_u32(dn, "ti,ngpio", &val);
>>> + if (ret)
>>> + goto of_err;
>>> +
>>> + pdata->ngpio = val;
>>> +
>>> + ret = of_property_read_u32(dn, "ti,davinci-gpio-unbanked", &val);
>>> + if (ret)
>>> + goto of_err;
>>> +
>>> + pdata->gpio_unbanked = val;
>>> +
>>> + return pdata;
>>> +
>>> +of_err:
>>> + dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
>>> + return NULL;
>>> +}
>>> +
>>> static int davinci_gpio_probe(struct platform_device *pdev)
>>> {
>>> int i, base;
>>> @@ -144,12 +181,14 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>>> struct device *dev = &pdev->dev;
>>> struct resource *res;
>>>
>>> - pdata = dev->platform_data;
>>> + pdata = davinci_gpio_get_pdata(pdev);
>>> if (!pdata) {
>>> dev_err(dev, "No platform data found\n");
>>> return -EINVAL;
>>> }
>>>
>>> + dev->platform_data = pdata;
>>> +
>>
>> Pls, add following code to GPIO chip initialization:
>>
>> @@ -233,6 +245,9 @@ static int davinci_gpio_probe(struct platform_device *pdev)
>> chips[i].chip.ngpio = ngpio - base;
>> if (chips[i].chip.ngpio > 32)
>> chips[i].chip.ngpio = 32;
>> +#ifdef CONFIG_OF_GPIO
>> + chips[i].chip.of_node = dev->of_node;
>> +#endif
>>
>>
> OK
>
> Regards,
> --Prabhakar Lad
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
` (6 preceding siblings ...)
2013-11-08 10:11 ` [PATCH v5 7/7] ARM: davinci: da850 evm: add GPIO pinumux entries " Prabhakar Lad
@ 2013-11-11 15:52 ` Grygorii Strashko
7 siblings, 0 replies; 23+ messages in thread
From: Grygorii Strashko @ 2013-11-11 15:52 UTC (permalink / raw)
To: Prabhakar Lad, Sekhar Nori, Linus Walleij
Cc: LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc, Rob Herring,
Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
Rob Landley, Grant Likely
Hi Prabhakar Lad,
On 11/08/2013 12:11 PM, Prabhakar Lad wrote:
> From: KV Sujith <sujithkv@ti.com>
>
> This patch series does the following
> 1> Ports the driver to use irqdomain.
> 2> Adds dt binding support for gpio-davinci.
> 3> Adds DA850 dt support goio.
>
> Changes for v5:
> 1: Fixed review comments pointed by Grygorii.
> 2: Dropped the fixup patch
>
> This patch series is based on following fixup series
> http://us.generation-nt.com/answer/patch-0-2-davinci-gpio-fixes-help-212978272.html
Few comments to this series - the rest looks ok to me.
Hope it will pass review of bindings :P
>
> KV Sujith (3):
> gpio: davinci: add OF support
> ARM: davinci: da850: add GPIO DT node
> ARM: davinci: da850 evm: add GPIO pinumux entries DT node
>
> Lad, Prabhakar (4):
> gpio: davinci: remove unnecessary printk
> gpio: davinci: use readl/writel instead of __raw_*
> gpio: davinci: use irqdomain
> gpio: davinci: remove unused variable intc_irq_num
>
> .../devicetree/bindings/gpio/gpio-davinci.txt | 39 +++++
> arch/arm/boot/dts/da850-evm.dts | 20 +++
> arch/arm/boot/dts/da850.dtsi | 14 ++
> arch/arm/mach-davinci/da830.c | 1 -
> arch/arm/mach-davinci/da850.c | 1 -
> arch/arm/mach-davinci/dm355.c | 1 -
> arch/arm/mach-davinci/dm365.c | 1 -
> arch/arm/mach-davinci/dm644x.c | 1 -
> arch/arm/mach-davinci/dm646x.c | 1 -
> drivers/gpio/gpio-davinci.c | 170 +++++++++++++-------
> include/linux/platform_data/gpio-davinci.h | 3 +-
> 11 files changed, 185 insertions(+), 67 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>
Regards,
- grygorii
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 2/7] gpio: davinci: use readl/writel instead of __raw_*
2013-11-08 10:11 ` [PATCH v5 2/7] gpio: davinci: use readl/writel instead of __raw_* Prabhakar Lad
@ 2013-11-18 13:01 ` Linus Walleij
0 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2013-11-18 13:01 UTC (permalink / raw)
To: Prabhakar Lad
Cc: Sekhar Nori, Grygorii Strashko, LKML, DLOS, LAK, linux-gpio,
devicetree, linux-doc, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On Fri, Nov 8, 2013 at 11:11 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>
> This patch replaces the __raw_readl/writel with
> readl and writel, Although the code runs on ARMv5
> based SOCs, changing this will help copying the code
> for other uses.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-08 10:11 ` [PATCH v5 3/7] gpio: davinci: use irqdomain Prabhakar Lad
@ 2013-11-18 13:11 ` Linus Walleij
2013-11-18 13:56 ` Russell King - ARM Linux
2013-11-18 14:34 ` Grygorii Strashko
0 siblings, 2 replies; 23+ messages in thread
From: Linus Walleij @ 2013-11-18 13:11 UTC (permalink / raw)
To: Prabhakar Lad
Cc: Sekhar Nori, Grygorii Strashko, LKML, DLOS, LAK, linux-gpio,
devicetree, linux-doc, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On Fri, Nov 8, 2013 at 11:11 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>
> This patch converts the davinci gpio driver to use irqdomain
> support.
>
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
(...)
> @@ -282,8 +283,7 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
> desc->irq_data.chip->irq_ack(&desc->irq_data);
> while (1) {
> u32 status;
> - int n;
> - int res;
> + int bit;
Why is this an int? I think u8 would suffice.
> /* now demux them to the right lowlevel handler */
> - n = d->irq_base;
> - if (irq & 1) {
> - n += 16;
> - status >>= 16;
> - }
> -
> while (status) {
> - res = ffs(status);
> - n += res;
> - generic_handle_irq(n - 1);
> - status >>= res;
> + bit = __ffs(status);
> + status &= ~(1 << bit);
> + generic_handle_irq(irq_find_mapping(d->irq_domain,
> + bit));
This is a nice hunk of the patch.
I would use <linux/bitops.h> and do:
status &= ~BIT(bit);
> @@ -313,10 +307,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
> {
> struct davinci_gpio_controller *d = chip2controller(chip);
>
> - if (d->irq_base >= 0)
> - return d->irq_base + offset;
> - else
> - return -ENODEV;
> + return irq_create_mapping(d->irq_domain, offset);
> }
I think we recently established that map creating cannot be done
in gpio_to_irq* functions as that will not work properly if you request
an IRQ from device tree without first obtaining the IRQ from the GPIO
number with this function.
Instead call irq_create_mapping() on *all* used IRQ lines in the
probe function and use irq_find_mapping() here too.
> + for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
> /* disabled by default, enabled only as needed */
> g = gpio2regs(gpio);
> writel(~0, &g->clr_falling);
> @@ -467,14 +483,6 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
> */
> irq_set_handler_data(bank_irq, &chips[gpio / 32]);
So for example here you could call iurq_create_mapping();
Also: please write a patch that marks the IRQ lines.
Call gpio_lock_as_irq(*gpio_chip, offset); in the
irqchip startup/shutdown functions. (Can be a separate
patch.)
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/7] gpio: davinci: add OF support
2013-11-08 10:11 ` [PATCH v5 5/7] gpio: davinci: add OF support Prabhakar Lad
2013-11-11 15:18 ` Grygorii Strashko
@ 2013-11-18 13:13 ` Linus Walleij
2013-11-18 13:42 ` Rob Herring
2 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2013-11-18 13:13 UTC (permalink / raw)
To: Prabhakar Lad
Cc: Sekhar Nori, Grygorii Strashko, LKML, DLOS, LAK, linux-gpio,
devicetree, linux-doc, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On Fri, Nov 8, 2013 at 11:11 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> From: KV Sujith <sujithkv@ti.com>
>
> This patch adds OF parser support for davinci gpio
> driver and also appropriate documentation in gpio-davinci.txt
> located at Documentation/devicetree/bindings/gpio/.
>
> Signed-off-by: KV Sujith <sujithkv@ti.com>
> Signed-off-by: Philip Avinash <avinashphilip@ti.com>
> [prabhakar.csengg@gmail.com: simplified the OF code, removed
> unnecessary DT property and also simplified
> the commit message]
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
I like this patch now!
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 5/7] gpio: davinci: add OF support
2013-11-08 10:11 ` [PATCH v5 5/7] gpio: davinci: add OF support Prabhakar Lad
2013-11-11 15:18 ` Grygorii Strashko
2013-11-18 13:13 ` Linus Walleij
@ 2013-11-18 13:42 ` Rob Herring
2 siblings, 0 replies; 23+ messages in thread
From: Rob Herring @ 2013-11-18 13:42 UTC (permalink / raw)
To: Prabhakar Lad
Cc: Sekhar Nori, Linus Walleij, Grygorii Strashko, LKML, DLOS, LAK,
linux-gpio, devicetree, linux-doc, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
Grant Likely
On Fri, Nov 8, 2013 at 4:11 AM, Prabhakar Lad
<prabhakar.csengg@gmail.com> wrote:
> From: KV Sujith <sujithkv@ti.com>
>
> This patch adds OF parser support for davinci gpio
> driver and also appropriate documentation in gpio-davinci.txt
> located at Documentation/devicetree/bindings/gpio/.
>
> Signed-off-by: KV Sujith <sujithkv@ti.com>
> Signed-off-by: Philip Avinash <avinashphilip@ti.com>
> [prabhakar.csengg@gmail.com: simplified the OF code, removed
> unnecessary DT property and also simplified
> the commit message]
> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
You should add the interrupt-controller and #interrupt-cells
properties to the example. Otherwise:
Acked-by: Rob Herring <rob.herring@calxeda.com>
> ---
> .../devicetree/bindings/gpio/gpio-davinci.txt | 39 ++++++++++++++
> drivers/gpio/gpio-davinci.c | 54 ++++++++++++++++++--
> 2 files changed, 90 insertions(+), 3 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-davinci.txt b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> new file mode 100644
> index 0000000..d677bbe
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
> @@ -0,0 +1,39 @@
> +Davinci GPIO controller bindings
> +
> +Required Properties:
> +- compatible: should be "ti,dm6441-gpio"
> +
> +- reg: Physical base address of the controller and the size of memory mapped
> + registers.
> +
> +- gpio-controller : Marks the device node as a gpio controller.
> +
> +- interrupt-parent: phandle of the parent interrupt controller.
> +
> +- interrupts: Array of GPIO interrupt number. Only banked or unbanked IRQs are
> + supported at a time.
> +
> +- ti,ngpio: The number of GPIO pins supported.
> +
> +- ti,davinci-gpio-unbanked: The number of GPIOs that have an individual interrupt
> + line to processor.
> +
> +The GPIO controller also acts as an interrupt controller. It uses the default
> +two cells specifier as described in Documentation/devicetree/bindings/
> +interrupt-controller/interrupts.txt.
> +
> +Example:
> +
> +gpio: gpio@1e26000 {
> + compatible = "ti,dm6441-gpio";
> + gpio-controller;
> + reg = <0x226000 0x1000>;
> + interrupt-parent = <&intc>;
> + interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
> + 44 IRQ_TYPE_EDGE_BOTH 45 IRQ_TYPE_EDGE_BOTH
> + 46 IRQ_TYPE_EDGE_BOTH 47 IRQ_TYPE_EDGE_BOTH
> + 48 IRQ_TYPE_EDGE_BOTH 49 IRQ_TYPE_EDGE_BOTH
> + 50 IRQ_TYPE_EDGE_BOTH>;
> + ti,ngpio = <144>;
> + ti,davinci-gpio-unbanked = <0>;
> +};
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index b149239..ed04835 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -17,6 +17,9 @@
> #include <linux/io.h>
> #include <linux/irq.h>
> #include <linux/irqdomain.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
> #include <linux/platform_data/gpio-davinci.h>
>
> @@ -134,6 +137,40 @@ davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> writel((1 << offset), value ? &g->set_data : &g->clr_data);
> }
>
> +static struct davinci_gpio_platform_data *
> +davinci_gpio_get_pdata(struct platform_device *pdev)
> +{
> + struct device_node *dn = pdev->dev.of_node;
> + struct davinci_gpio_platform_data *pdata;
> + int ret;
> + u32 val;
> +
> + if (!IS_ENABLED(CONFIG_OF) || !pdev->dev.of_node)
> + return pdev->dev.platform_data;
> +
> + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
> + if (!pdata)
> + return NULL;
> +
> + ret = of_property_read_u32(dn, "ti,ngpio", &val);
> + if (ret)
> + goto of_err;
> +
> + pdata->ngpio = val;
> +
> + ret = of_property_read_u32(dn, "ti,davinci-gpio-unbanked", &val);
> + if (ret)
> + goto of_err;
> +
> + pdata->gpio_unbanked = val;
> +
> + return pdata;
> +
> +of_err:
> + dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
> + return NULL;
> +}
> +
> static int davinci_gpio_probe(struct platform_device *pdev)
> {
> int i, base;
> @@ -144,12 +181,14 @@ static int davinci_gpio_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct resource *res;
>
> - pdata = dev->platform_data;
> + pdata = davinci_gpio_get_pdata(pdev);
> if (!pdata) {
> dev_err(dev, "No platform data found\n");
> return -EINVAL;
> }
>
> + dev->platform_data = pdata;
> +
> /*
> * The gpio banks conceptually expose a segmented bitmap,
> * and "ngpio" is one more than the largest zero-based
> @@ -496,11 +535,20 @@ done:
> return 0;
> }
>
> +#if IS_ENABLED(CONFIG_OF)
> +static const struct of_device_id davinci_gpio_ids[] = {
> + { .compatible = "ti,dm6441-gpio", },
> + { /* sentinel */ },
> +};
> +MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
> +#endif
> +
> static struct platform_driver davinci_gpio_driver = {
> .probe = davinci_gpio_probe,
> .driver = {
> - .name = "davinci_gpio",
> - .owner = THIS_MODULE,
> + .name = "davinci_gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = of_match_ptr(davinci_gpio_ids),
> },
> };
>
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-18 13:11 ` Linus Walleij
@ 2013-11-18 13:56 ` Russell King - ARM Linux
2013-11-18 14:34 ` Grygorii Strashko
1 sibling, 0 replies; 23+ messages in thread
From: Russell King - ARM Linux @ 2013-11-18 13:56 UTC (permalink / raw)
To: Linus Walleij
Cc: Prabhakar Lad, Mark Rutland, devicetree, DLOS, Grygorii Strashko,
Pawel Moll, linux-doc, Stephen Warren, Sekhar Nori, LKML,
Rob Herring, linux-gpio, Rob Landley, Grant Likely, Ian Campbell,
LAK
On Mon, Nov 18, 2013 at 02:11:32PM +0100, Linus Walleij wrote:
> On Fri, Nov 8, 2013 at 11:11 AM, Prabhakar Lad
> <prabhakar.csengg@gmail.com> wrote:
>
> > From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
> >
> > This patch converts the davinci gpio driver to use irqdomain
> > support.
> >
> > Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>
> (...)
> > @@ -282,8 +283,7 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
> > desc->irq_data.chip->irq_ack(&desc->irq_data);
> > while (1) {
> > u32 status;
> > - int n;
> > - int res;
> > + int bit;
>
> Why is this an int? I think u8 would suffice.
For local variables, it's better to use "int" or even "unsigned" here
rather than u8. u8 tells the compiler that you want the variable to be
8-bit, which seems fine, except the compiler is then at liberty to keep
on masking the register back down to 8-bit.
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-18 13:11 ` Linus Walleij
2013-11-18 13:56 ` Russell King - ARM Linux
@ 2013-11-18 14:34 ` Grygorii Strashko
2013-11-18 23:06 ` Linus Walleij
1 sibling, 1 reply; 23+ messages in thread
From: Grygorii Strashko @ 2013-11-18 14:34 UTC (permalink / raw)
To: Linus Walleij, Prabhakar Lad
Cc: Sekhar Nori, LKML, DLOS, LAK, linux-gpio, devicetree, linux-doc,
Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren,
Ian Campbell, Rob Landley, Grant Likely
Hi Linus,
On 11/18/2013 03:11 PM, Linus Walleij wrote:
> On Fri, Nov 8, 2013 at 11:11 AM, Prabhakar Lad
> <prabhakar.csengg@gmail.com> wrote:
>
>> From: "Lad, Prabhakar" <prabhakar.csengg@gmail.com>
>>
>> This patch converts the davinci gpio driver to use irqdomain
>> support.
>>
>> Signed-off-by: Lad, Prabhakar <prabhakar.csengg@gmail.com>
>
> (...)
>> @@ -282,8 +283,7 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
>> desc->irq_data.chip->irq_ack(&desc->irq_data);
>> while (1) {
>> u32 status;
>> - int n;
>> - int res;
>> + int bit;
>
> Why is this an int? I think u8 would suffice.
>
>> /* now demux them to the right lowlevel handler */
>> - n = d->irq_base;
>> - if (irq & 1) {
>> - n += 16;
>> - status >>= 16;
>> - }
>> -
>> while (status) {
>> - res = ffs(status);
>> - n += res;
>> - generic_handle_irq(n - 1);
>> - status >>= res;
>> + bit = __ffs(status);
>> + status &= ~(1 << bit);
>> + generic_handle_irq(irq_find_mapping(d->irq_domain,
>> + bit));
>
> This is a nice hunk of the patch.
>
> I would use <linux/bitops.h> and do:
> status &= ~BIT(bit);
>
>
>> @@ -313,10 +307,7 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
>> {
>> struct davinci_gpio_controller *d = chip2controller(chip);
>>
>> - if (d->irq_base >= 0)
>> - return d->irq_base + offset;
>> - else
>> - return -ENODEV;
>> + return irq_create_mapping(d->irq_domain, offset);
>> }
>
> I think we recently established that map creating cannot be done
> in gpio_to_irq* functions as that will not work properly if you request
> an IRQ from device tree without first obtaining the IRQ from the GPIO
> number with this function.
Why? Could you point on corresponding thread, pls?
Current call tree is:
irq_create_of_mapping()
|-hwirq = omain->ops->xlate()
|-irq_create_mapping(domain, hwirq)
>
> Instead call irq_create_mapping() on *all* used IRQ lines in the
> probe function and use irq_find_mapping() here too.
>
>> + for (gpio = 0, bank = 0; gpio < ngpio; bank++, bank_irq++, gpio += 16) {
>> /* disabled by default, enabled only as needed */
>> g = gpio2regs(gpio);
>> writel(~0, &g->clr_falling);
>> @@ -467,14 +483,6 @@ static int davinci_gpio_irq_setup(struct platform_device *pdev)
>> */
>> irq_set_handler_data(bank_irq, &chips[gpio / 32]);
>
> So for example here you could call iurq_create_mapping();
>
> Also: please write a patch that marks the IRQ lines.
> Call gpio_lock_as_irq(*gpio_chip, offset); in the
> irqchip startup/shutdown functions. (Can be a separate
> patch.)
It seems, some misunderstanding is here. So I'd like to clarify few
points and would be very appreciate for your comments:
1) This patch itself will work unless we switch to DT (as in the
following patch)
2) After this patch the following object structure will be created
during Davinci GPIO driver initialization (DA850 has 144 IRQ lines):
- gpio_chip0(0..31)
|- irq_domain1
- gpio_chip1(32..63)
|- irq_domain2
- gpio_chip2(64..95)
|- irq_domain3
- gpio_chip3(96..127)
|- irq_domain4
- gpio_chip4(128..143)
|- irq_domain5
3) But in case of DT only one GPIO controller node will be created
Example:
gpio: gpio@1e26000 {
compatible = "ti,dm6441-gpio";
gpio-controller;
reg = <0x226000 0x1000>;
interrupt-parent = <&intc>;
interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
44 IRQ_TYPE_EDGE_BOTH 45 IRQ_TYPE_EDGE_BOTH
46 IRQ_TYPE_EDGE_BOTH 47 IRQ_TYPE_EDGE_BOTH
48 IRQ_TYPE_EDGE_BOTH 49 IRQ_TYPE_EDGE_BOTH
50 IRQ_TYPE_EDGE_BOTH>;
interrupt-controller;
#interrupt-cells = <2>;
ti,ngpio = <144>;
ti,davinci-gpio-unbanked = <0>;
}
4) As result, to make GPIO bindings/mappings work - we'll need to
implement .of_xlate() callback per GPIO chip, which will provide us with
valid valid gpio_chip and offset of gpio inside chip
(It was discussed before and supposed to be done as next step).
For example, gpio_chip3 and offset=15 should be selected:
devA {
gpios = <&gpio 111 GPIO_ACTIVE_HIGH>;
}
5) What should be done to make GPIO IRQ bindings/mappings work?
Example:
devB {
interrupt-parent = <&gpio>;
interrupts = <111 IRQ_TYPE_EDGE_BOTH>;
}
Should we implement one IRQ domain per all GPIO chips (per GPIO controller)?
Thanks.
Regards,
-Grygorii
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-18 14:34 ` Grygorii Strashko
@ 2013-11-18 23:06 ` Linus Walleij
2013-11-19 16:22 ` Grygorii Strashko
0 siblings, 1 reply; 23+ messages in thread
From: Linus Walleij @ 2013-11-18 23:06 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Prabhakar Lad, Sekhar Nori, LKML, DLOS, LAK, linux-gpio,
devicetree, linux-doc, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On Mon, Nov 18, 2013 at 3:34 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 11/18/2013 03:11 PM, Linus Walleij wrote:
>> I think we recently established that map creating cannot be done
>> in gpio_to_irq* functions as that will not work properly if you request
>> an IRQ from device tree without first obtaining the IRQ from the GPIO
>> number with this function.
>
> Why? Could you point on corresponding thread, pls?
All that contain this:
"gpio: interrupt consistency check for OF GPIO IRQs"
http://marc.info/?l=linux-kernel&w=2&r=1&s=interrupt+consistency&q=b
> Current call tree is:
> irq_create_of_mapping()
> |-hwirq = omain->ops->xlate()
> |-irq_create_mapping(domain, hwirq)
OK that works for the pure device-tree scenario so mostly
this is OK.
The problem that appear is if someone is using platform data-provided
IRQs off the irq_chip without calling gpio_to_irq() on the GPIO line
first. This has been determined to be legal to do, so preferably
create the map when registering the lines.
>> Also: please write a patch that marks the IRQ lines.
>> Call gpio_lock_as_irq(*gpio_chip, offset); in the
>> irqchip startup/shutdown functions. (Can be a separate
>> patch.)
>
> It seems, some misunderstanding is here. So I'd like to clarify few points
> and would be very appreciate for your comments:
> 1) This patch itself will work unless we switch to DT (as in the following
> patch)
Sure.... but this does not seem to have much to do
with my request to use gpio_lock_as_irq().
> 2) After this patch the following object structure will be created during
> Davinci GPIO driver initialization (DA850 has 144 IRQ lines):
> - gpio_chip0(0..31)
> |- irq_domain1
> - gpio_chip1(32..63)
> |- irq_domain2
> - gpio_chip2(64..95)
> |- irq_domain3
> - gpio_chip3(96..127)
> |- irq_domain4
> - gpio_chip4(128..143)
> |- irq_domain5
OK that's nice.
> 3) But in case of DT only one GPIO controller node will be created
> Example:
> gpio: gpio@1e26000 {
> compatible = "ti,dm6441-gpio";
> gpio-controller;
> reg = <0x226000 0x1000>;
> interrupt-parent = <&intc>;
> interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
> 44 IRQ_TYPE_EDGE_BOTH 45 IRQ_TYPE_EDGE_BOTH
> 46 IRQ_TYPE_EDGE_BOTH 47 IRQ_TYPE_EDGE_BOTH
> 48 IRQ_TYPE_EDGE_BOTH 49 IRQ_TYPE_EDGE_BOTH
> 50 IRQ_TYPE_EDGE_BOTH>;
> interrupt-controller;
> #interrupt-cells = <2>;
> ti,ngpio = <144>;
> ti,davinci-gpio-unbanked = <0>;
> }
Yep...
> 4) As result, to make GPIO bindings/mappings work - we'll need to implement
> .of_xlate() callback per GPIO chip, which will provide us with valid valid
> gpio_chip and offset of gpio inside chip
> (It was discussed before and supposed to be done as next step).
Yeah.. this is usually a bit tricky.
> For example, gpio_chip3 and offset=15 should be selected:
> devA {
> gpios = <&gpio 111 GPIO_ACTIVE_HIGH>;
> }
>
> 5) What should be done to make GPIO IRQ bindings/mappings work?
>
> Example:
> devB {
> interrupt-parent = <&gpio>;
> interrupts = <111 IRQ_TYPE_EDGE_BOTH>;
> }
>
> Should we implement one IRQ domain per all GPIO chips (per GPIO controller)?
I don't know, I cannot really see where the problem is.
The IRQ domain is for translationg hardware numbers such as bit offsets
into Linux IRQ numbers and nothing else, so I'd suggest that as long as
the thing you are translating/mapping is something simple like a bit
index you're doing the right thing.
If it becomes something complex where that index is not just a bit
but an index into an array of registers at various locations it is
abusing the irqdomain.
So I think you should create one irqdomain per GPIO instance
or bank or whatever you want to call it: like if there is this one
register with 32 bits, then it gets its own IRQdomain.
I think you should try to keep the 5 irqdomains, but whatever
gives the simplest code is usually the right answer, and
divide-and-conquer (split down the problem) is usually a good
idea.
How the GPIO block(s) are represented in the device tree is
another totally separate issue about hardware description,
do not let the device tree model your driver structure.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-18 23:06 ` Linus Walleij
@ 2013-11-19 16:22 ` Grygorii Strashko
2013-11-19 20:34 ` Linus Walleij
0 siblings, 1 reply; 23+ messages in thread
From: Grygorii Strashko @ 2013-11-19 16:22 UTC (permalink / raw)
To: Linus Walleij
Cc: Prabhakar Lad, Sekhar Nori, LKML, DLOS, LAK, linux-gpio,
devicetree, linux-doc, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On 11/19/2013 01:06 AM, Linus Walleij wrote:
> On Mon, Nov 18, 2013 at 3:34 PM, Grygorii Strashko
> <grygorii.strashko@ti.com> wrote:
>> On 11/18/2013 03:11 PM, Linus Walleij wrote:
>
>>> I think we recently established that map creating cannot be done
>>> in gpio_to_irq* functions as that will not work properly if you request
>>> an IRQ from device tree without first obtaining the IRQ from the GPIO
>>> number with this function.
>>
>> Why? Could you point on corresponding thread, pls?
>
> All that contain this:
> "gpio: interrupt consistency check for OF GPIO IRQs"
> http://marc.info/?l=linux-kernel&w=2&r=1&s=interrupt+consistency&q=b
Thanks.
>
>> Current call tree is:
>> irq_create_of_mapping()
>> |-hwirq = omain->ops->xlate()
>> |-irq_create_mapping(domain, hwirq)
>
> OK that works for the pure device-tree scenario so mostly
> this is OK.
>
> The problem that appear is if someone is using platform data-provided
> IRQs off the irq_chip without calling gpio_to_irq() on the GPIO line
> first. This has been determined to be legal to do, so preferably
> create the map when registering the lines.
Ok, I understand. It may fail in case if someone will define/calculate
IRQ number for device manually in board code, like:
dev_irq = (DA850_N_CP_INTC_IRQ + gpioN)
Also, looks like It is possible to fail if Main/arch IRQ controller
code doesn't make call of irq_alloc_descs() during init, so
allocated_irqs will be empty.
Actually, the irq_find_mapping() was called before from
gpio_to_irq_banked() in v4 of this series - than It was changed
according to my comment, to get maximum benefits of using linear IRQ
domain.
Before recommending that, I've checked Davinci platform code and didn't
find any risk places - BUT, Seems my findings need to be confirmed by
Sekhar.
I purpose to move forward as is if above will be confirmed. Otherwise,
we can switch to use irq_domain_add_legacy().
>
>>> Also: please write a patch that marks the IRQ lines.
>>> Call gpio_lock_as_irq(*gpio_chip, offset); in the
>>> irqchip startup/shutdown functions. (Can be a separate
>>> patch.)
>>
>> It seems, some misunderstanding is here. So I'd like to clarify few points
>> and would be very appreciate for your comments:
>> 1) This patch itself will work unless we switch to DT (as in the following
>> patch)
>
> Sure.... but this does not seem to have much to do
> with my request to use gpio_lock_as_irq().
Oh no. Your request about gpio_lock_as_irq() is absolutely valid :)
And it has to be added
>
>> 2) After this patch the following object structure will be created during
>> Davinci GPIO driver initialization (DA850 has 144 IRQ lines):
>> - gpio_chip0(0..31)
>> |- irq_domain1
>> - gpio_chip1(32..63)
>> |- irq_domain2
>> - gpio_chip2(64..95)
>> |- irq_domain3
>> - gpio_chip3(96..127)
>> |- irq_domain4
>> - gpio_chip4(128..143)
>> |- irq_domain5
>
> OK that's nice.
>
>> 3) But in case of DT only one GPIO controller node will be created
>> Example:
>> gpio: gpio@1e26000 {
>> compatible = "ti,dm6441-gpio";
>> gpio-controller;
>> reg = <0x226000 0x1000>;
>> interrupt-parent = <&intc>;
>> interrupts = <42 IRQ_TYPE_EDGE_BOTH 43 IRQ_TYPE_EDGE_BOTH
>> 44 IRQ_TYPE_EDGE_BOTH 45 IRQ_TYPE_EDGE_BOTH
>> 46 IRQ_TYPE_EDGE_BOTH 47 IRQ_TYPE_EDGE_BOTH
>> 48 IRQ_TYPE_EDGE_BOTH 49 IRQ_TYPE_EDGE_BOTH
>> 50 IRQ_TYPE_EDGE_BOTH>;
>> interrupt-controller;
>> #interrupt-cells = <2>;
>> ti,ngpio = <144>;
>> ti,davinci-gpio-unbanked = <0>;
>> }
>
> Yep...
>
>> 4) As result, to make GPIO bindings/mappings work - we'll need to implement
>> .of_xlate() callback per GPIO chip, which will provide us with valid valid
>> gpio_chip and offset of gpio inside chip
>> (It was discussed before and supposed to be done as next step).
>
> Yeah.. this is usually a bit tricky.
>
>> For example, gpio_chip3 and offset=15 should be selected:
>> devA {
>> gpios = <&gpio 111 GPIO_ACTIVE_HIGH>;
>> }
>>
>> 5) What should be done to make GPIO IRQ bindings/mappings work?
>>
>> Example:
>> devB {
>> interrupt-parent = <&gpio>;
>> interrupts = <111 IRQ_TYPE_EDGE_BOTH>;
>> }
>>
>> Should we implement one IRQ domain per all GPIO chips (per GPIO controller)?
>
> I don't know, I cannot really see where the problem is.
>
> The IRQ domain is for translationg hardware numbers such as bit offsets
> into Linux IRQ numbers and nothing else, so I'd suggest that as long as
> the thing you are translating/mapping is something simple like a bit
> index you're doing the right thing.
>
> If it becomes something complex where that index is not just a bit
> but an index into an array of registers at various locations it is
> abusing the irqdomain.
>
> So I think you should create one irqdomain per GPIO instance
> or bank or whatever you want to call it: like if there is this one
> register with 32 bits, then it gets its own IRQdomain.
>
> I think you should try to keep the 5 irqdomains, but whatever
> gives the simplest code is usually the right answer, and
> divide-and-conquer (split down the problem) is usually a good
> idea.
>
> How the GPIO block(s) are represented in the device tree is
> another totally separate issue about hardware description,
> do not let the device tree model your driver structure.
Thanks for you comments, but looks like I have to be more specific :)
How irq_find_host() will look for proper IRQ domain in our case?
And will it work at all, taking into account that all (5) IRQ domains
will have the same value of "of_node" property?
of_irq_to_resource()
|-irq_of_parse_and_map()
|-irq_create_of_mapping()
|-irq_find_host(irq_data->np)
where np will point on GPIO node.
As result my question is about what do DT core frameworks allow or not
allow to do? And according to that implementation of driver can be
changed.
Regards,
-grygorii
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v5 3/7] gpio: davinci: use irqdomain
2013-11-19 16:22 ` Grygorii Strashko
@ 2013-11-19 20:34 ` Linus Walleij
0 siblings, 0 replies; 23+ messages in thread
From: Linus Walleij @ 2013-11-19 20:34 UTC (permalink / raw)
To: Grygorii Strashko
Cc: Prabhakar Lad, Sekhar Nori, LKML, DLOS, LAK, linux-gpio,
devicetree, linux-doc, Rob Herring, Pawel Moll, Mark Rutland,
Stephen Warren, Ian Campbell, Rob Landley, Grant Likely
On Tue, Nov 19, 2013 at 5:22 PM, Grygorii Strashko
<grygorii.strashko@ti.com> wrote:
> On 11/19/2013 01:06 AM, Linus Walleij wrote:
>> The problem that appear is if someone is using platform data-provided
>> IRQs off the irq_chip without calling gpio_to_irq() on the GPIO line
>> first. This has been determined to be legal to do, so preferably
>> create the map when registering the lines.
>
> Ok, I understand. It may fail in case if someone will define/calculate
> IRQ number for device manually in board code, like:
> dev_irq = (DA850_N_CP_INTC_IRQ + gpioN)
Usually people just hardcode the IRQ value or use some
#define, like they have often also done with GPIO numbers ...
> Also, looks like It is possible to fail if Main/arch IRQ controller
> code doesn't make call of irq_alloc_descs() during init, so
> allocated_irqs will be empty.
If you use irq_domain_add_simple() the irqdomain core
will actually attempt to do this.
But if you use a linear domain, you have to create these
irqdescs as you go. The simplest way to do this is to call
irq_create_mapping() on all IRQs, because that call will
also allocate some descriptor.
> Before recommending that, I've checked Davinci platform code and didn't
> find any risk places - BUT, Seems my findings need to be confirmed by
> Sekhar.
It's no big deal, but I just want you to be aware that this
may cause a problem at some point.
>> I think you should try to keep the 5 irqdomains, but whatever
>> gives the simplest code is usually the right answer, and
>> divide-and-conquer (split down the problem) is usually a good
>> idea.
>>
>> How the GPIO block(s) are represented in the device tree is
>> another totally separate issue about hardware description,
>> do not let the device tree model your driver structure.
>
>
> Thanks for you comments, but looks like I have to be more specific :)
>
> How irq_find_host() will look for proper IRQ domain in our case?
> And will it work at all, taking into account that all (5) IRQ domains
> will have the same value of "of_node" property?
> of_irq_to_resource()
> |-irq_of_parse_and_map()
> |-irq_create_of_mapping()
> |-irq_find_host(irq_data->np)
> where np will point on GPIO node.
>
> As result my question is about what do DT core frameworks allow or not
> allow to do? And according to that implementation of driver can be
> changed.
Hm, I'm not like a super-expert on the interrupt core, but maybe
you need to create subnodes inside the top node to represent this,
then use of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
to populate them from the main node.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2013-11-19 20:34 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-11-08 10:11 [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 1/7] gpio: davinci: remove unnecessary printk Prabhakar Lad
2013-11-08 15:39 ` Grygorii Strashko
2013-11-11 15:39 ` Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 2/7] gpio: davinci: use readl/writel instead of __raw_* Prabhakar Lad
2013-11-18 13:01 ` Linus Walleij
2013-11-08 10:11 ` [PATCH v5 3/7] gpio: davinci: use irqdomain Prabhakar Lad
2013-11-18 13:11 ` Linus Walleij
2013-11-18 13:56 ` Russell King - ARM Linux
2013-11-18 14:34 ` Grygorii Strashko
2013-11-18 23:06 ` Linus Walleij
2013-11-19 16:22 ` Grygorii Strashko
2013-11-19 20:34 ` Linus Walleij
2013-11-08 10:11 ` [PATCH v5 4/7] gpio: davinci: remove unused variable intc_irq_num Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 5/7] gpio: davinci: add OF support Prabhakar Lad
2013-11-11 15:18 ` Grygorii Strashko
2013-11-11 15:37 ` Prabhakar Lad
2013-11-11 15:44 ` Grygorii Strashko
2013-11-18 13:13 ` Linus Walleij
2013-11-18 13:42 ` Rob Herring
2013-11-08 10:11 ` [PATCH v5 6/7] ARM: davinci: da850: add GPIO DT node Prabhakar Lad
2013-11-08 10:11 ` [PATCH v5 7/7] ARM: davinci: da850 evm: add GPIO pinumux entries " Prabhakar Lad
2013-11-11 15:52 ` [PATCH v5 0/7] gpio: daVinci: cleanup and feature enhancement Grygorii Strashko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).