GPIO Davinci driver converted to platform driver to support DT booting.
In this patch series
- Cleaned gpio Davinci driver code with proper commenting style and appropriate
variable names.
- Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
gpio related member updation in davinci_soc_info structure.
- DT support added for da850 board and tested on da850 EVM.
- Remove soc_info reference in the gpio davinci driver and start uses
gpiolib interface.
This sereise based on [1] and is avilable at [2].
1. http://gitorious.org/linux-davinci/linux-davinci/trees/davinci-for-v3.10/soc
2. https://github.com/avinashphilip/am335x_linux/commits/linux_davinci_v3.10_soc_gpio
KV Sujith (6):
ARM: davinci: GPIO: Add platform data structure
gpio: davinci: Modify to platform driver
ARM: davinci: da8xx: creation of gpio platform device
gpio: davinci: DT changes for driver
ARM: davinci: da850: add GPIO DT entries
ARM: davinci: da850 evm: add GPIO DT data
Philip Avinash (5):
gpio: davinci: coding style correction
ARM: davinci: creation of gpio platform device for dm platforms
ARM: davinci: da8xx: gpio device creation
ARM: davinci: create davinci gpio device for dm platforms
ARM: davinci: start using gpiolib support
.../devicetree/bindings/gpio/gpio-davinci.txt | 26 ++
arch/arm/Kconfig | 1 -
arch/arm/boot/dts/da850-evm.dts | 19 ++
arch/arm/boot/dts/da850.dtsi | 9 +
arch/arm/mach-davinci/board-da830-evm.c | 19 +-
arch/arm/mach-davinci/board-da850-evm.c | 11 +
arch/arm/mach-davinci/board-dm355-evm.c | 27 ++
arch/arm/mach-davinci/board-dm355-leopard.c | 1 +
arch/arm/mach-davinci/board-dm365-evm.c | 28 ++
arch/arm/mach-davinci/board-dm644x-evm.c | 26 ++
arch/arm/mach-davinci/board-dm646x-evm.c | 27 ++
arch/arm/mach-davinci/board-neuros-osd2.c | 1 +
arch/arm/mach-davinci/board-omapl138-hawk.c | 2 +
arch/arm/mach-davinci/da830.c | 4 -
arch/arm/mach-davinci/da850.c | 4 -
arch/arm/mach-davinci/devices-da8xx.c | 26 ++
arch/arm/mach-davinci/devices.c | 14 +
arch/arm/mach-davinci/dm355.c | 4 -
arch/arm/mach-davinci/dm365.c | 5 -
arch/arm/mach-davinci/dm644x.c | 4 -
arch/arm/mach-davinci/dm646x.c | 4 -
arch/arm/mach-davinci/include/mach/common.h | 4 +
arch/arm/mach-davinci/include/mach/da8xx.h | 1 +
arch/arm/mach-davinci/include/mach/gpio-davinci.h | 8 +-
drivers/gpio/gpio-davinci.c | 346 +++++++++++++-------
include/linux/platform_data/gpio-davinci.h | 79 +++++
26 files changed, 543 insertions(+), 157 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-davinci.txt
create mode 100644 include/linux/platform_data/gpio-davinci.h
--
1.7.9.5
From: KV Sujith <[email protected]>
Add struct davinci_gpio_platform_data davinci gpio module.
Signed-off-by: KV Sujith <[email protected]>
Signed-off-by: Philip Avinash <[email protected]>
---
include/linux/platform_data/gpio-davinci.h | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
create mode 100644 include/linux/platform_data/gpio-davinci.h
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
new file mode 100644
index 0000000..f1c8277
--- /dev/null
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -0,0 +1,27 @@
+/*
+ * gpio-davinci.h
+ *
+ * DaVinci GPIO Platform Related Defines
+ *
+ * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation version 2.
+ *
+ * This program is distributed "as is" WITHOUT ANY WARRANTY of any
+ * kind, whether express or implied; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __ASM_ARCH_DAVINCI_GPIO_H
+#define __ASM_ARCH_DAVINCI_GPIO_H
+
+struct davinci_gpio_platform_data {
+ u32 ngpio;
+ u32 gpio_unbanked;
+ u32 intc_irq_num;
+};
+
+#endif
--
1.7.9.5
1. Corrects coding and commenting styles
2. Variables name change to meaningful name
3. Remove unnecessary variable usage
4. Add BINTEN macro definition
Signed-off-by: Philip Avinash <[email protected]>
---
drivers/gpio/gpio-davinci.c | 182 +++++++++++++++++++++----------------------
1 file changed, 89 insertions(+), 93 deletions(-)
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 17df6db..d308955 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -9,12 +9,12 @@
* the Free Software Foundation; either version 2 of the License, or
* (at your option) any later version.
*/
-#include <linux/gpio.h>
-#include <linux/errno.h>
-#include <linux/kernel.h>
+
#include <linux/clk.h>
-#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/gpio.h>
#include <linux/io.h>
+#include <linux/kernel.h>
#include <asm/mach/irq.h>
@@ -31,10 +31,11 @@ struct davinci_gpio_regs {
u32 intstat;
};
+#define BINTEN 0x08 /* GPIO Interrupt Per-Bank Enable Register */
#define chip2controller(chip) \
container_of(chip, struct davinci_gpio_controller, chip)
-static struct davinci_gpio_controller chips[DIV_ROUND_UP(DAVINCI_N_GPIO, 32)];
+static struct davinci_gpio_controller ctlrs[DIV_ROUND_UP(DAVINCI_N_GPIO, 32)];
static void __iomem *gpio_base;
static struct davinci_gpio_regs __iomem __init *gpio2regs(unsigned gpio)
@@ -53,42 +54,37 @@ static struct davinci_gpio_regs __iomem __init *gpio2regs(unsigned gpio)
ptr = gpio_base + 0xb0;
else
ptr = NULL;
+
return ptr;
}
static inline struct davinci_gpio_regs __iomem *irq2regs(int irq)
{
- struct davinci_gpio_regs __iomem *g;
-
- g = (__force struct davinci_gpio_regs __iomem *)irq_get_chip_data(irq);
-
- return g;
+ return (__force struct davinci_gpio_regs __iomem *)
+ irq_get_chip_data(irq);
}
static int __init davinci_gpio_irq_setup(void);
-/*--------------------------------------------------------------------------*/
-
-/* board setup code *MUST* setup pinmux and enable the GPIO clock. */
static inline int __davinci_direction(struct gpio_chip *chip,
unsigned offset, bool out, int value)
{
- struct davinci_gpio_controller *d = chip2controller(chip);
- struct davinci_gpio_regs __iomem *g = d->regs;
+ struct davinci_gpio_controller *ctlr = chip2controller(chip);
+ struct davinci_gpio_regs __iomem *regs = ctlr->regs;
unsigned long flags;
u32 temp;
u32 mask = 1 << offset;
- spin_lock_irqsave(&d->lock, flags);
- temp = __raw_readl(&g->dir);
+ spin_lock_irqsave(&ctlr->lock, flags);
+ temp = __raw_readl(®s->dir);
if (out) {
temp &= ~mask;
- __raw_writel(mask, value ? &g->set_data : &g->clr_data);
+ __raw_writel(mask, value ? ®s->set_data : ®s->clr_data);
} else {
temp |= mask;
}
- __raw_writel(temp, &g->dir);
- spin_unlock_irqrestore(&d->lock, flags);
+ __raw_writel(temp, ®s->dir);
+ spin_unlock_irqrestore(&ctlr->lock, flags);
return 0;
}
@@ -98,8 +94,8 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
return __davinci_direction(chip, offset, false, 0);
}
-static int
-davinci_direction_out(struct gpio_chip *chip, unsigned offset, int value)
+static int davinci_direction_out(struct gpio_chip *chip, unsigned offset,
+ int value)
{
return __davinci_direction(chip, offset, true, value);
}
@@ -113,22 +109,22 @@ davinci_direction_out(struct gpio_chip *chip, unsigned offset, int value)
*/
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;
+ struct davinci_gpio_controller *ctlr = chip2controller(chip);
+ struct davinci_gpio_regs __iomem *regs = ctlr->regs;
- return (1 << offset) & __raw_readl(&g->in_data);
+ return (1 << offset) & __raw_readl(®s->in_data);
}
/*
* Assuming the pin is muxed as a gpio output, set its output value.
*/
-static void
-davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+static void 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;
+ struct davinci_gpio_controller *ctlr = chip2controller(chip);
+ struct davinci_gpio_regs __iomem *regs = ctlr->regs;
- __raw_writel((1 << offset), value ? &g->set_data : &g->clr_data);
+ __raw_writel((1 << offset), value ? ®s->set_data : ®s->clr_data);
}
static int __init davinci_gpio_setup(void)
@@ -160,30 +156,30 @@ static int __init davinci_gpio_setup(void)
return -ENOMEM;
for (i = 0, base = 0; base < ngpio; i++, base += 32) {
- chips[i].chip.label = "DaVinci";
+ ctlrs[i].chip.label = "DaVinci";
- 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;
+ ctlrs[i].chip.direction_input = davinci_direction_in;
+ ctlrs[i].chip.direction_output = davinci_direction_out;
+ ctlrs[i].chip.get = davinci_gpio_get;
+ ctlrs[i].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;
+ ctlrs[i].chip.base = base;
+ ctlrs[i].chip.ngpio = ngpio - base;
+ if (ctlrs[i].chip.ngpio > 32)
+ ctlrs[i].chip.ngpio = 32;
- spin_lock_init(&chips[i].lock);
+ spin_lock_init(&ctlrs[i].lock);
regs = gpio2regs(base);
- chips[i].regs = regs;
- chips[i].set_data = ®s->set_data;
- chips[i].clr_data = ®s->clr_data;
- chips[i].in_data = ®s->in_data;
+ ctlrs[i].regs = regs;
+ ctlrs[i].set_data = ®s->set_data;
+ ctlrs[i].clr_data = ®s->clr_data;
+ ctlrs[i].in_data = ®s->in_data;
- gpiochip_add(&chips[i].chip);
+ gpiochip_add(&ctlrs[i].chip);
}
- soc_info->gpio_ctlrs = chips;
+ soc_info->gpio_ctlrs = ctlrs;
soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
davinci_gpio_irq_setup();
@@ -191,7 +187,6 @@ static int __init davinci_gpio_setup(void)
}
pure_initcall(davinci_gpio_setup);
-/*--------------------------------------------------------------------------*/
/*
* We expect irqs will normally be set up as input pins, but they can also be
* used as output pins ... which is convenient for testing.
@@ -205,16 +200,16 @@ pure_initcall(davinci_gpio_setup);
static void gpio_irq_disable(struct irq_data *d)
{
- struct davinci_gpio_regs __iomem *g = irq2regs(d->irq);
+ struct davinci_gpio_regs __iomem *regs = 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);
+ __raw_writel(mask, ®s->clr_falling);
+ __raw_writel(mask, ®s->clr_rising);
}
static void gpio_irq_enable(struct irq_data *d)
{
- struct davinci_gpio_regs __iomem *g = irq2regs(d->irq);
+ struct davinci_gpio_regs __iomem *regs = irq2regs(d->irq);
u32 mask = (u32) irq_data_get_irq_handler_data(d);
unsigned status = irqd_get_trigger_type(d);
@@ -223,9 +218,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);
+ __raw_writel(mask, ®s->set_falling);
if (status & IRQ_TYPE_EDGE_RISING)
- __raw_writel(mask, &g->set_rising);
+ __raw_writel(mask, ®s->set_rising);
}
static int gpio_irq_type(struct irq_data *d, unsigned trigger)
@@ -244,15 +239,15 @@ static struct irq_chip gpio_irqchip = {
.flags = IRQCHIP_SET_TYPE_MASKED,
};
-static void
-gpio_irq_handler(unsigned irq, struct irq_desc *desc)
+static void gpio_irq_handler(unsigned irq, struct irq_desc *desc)
{
- struct davinci_gpio_regs __iomem *g;
u32 mask = 0xffff;
- struct davinci_gpio_controller *d;
+ struct davinci_gpio_controller *ctlr;
+ struct davinci_gpio_regs __iomem *regs;
- d = (struct davinci_gpio_controller *)irq_desc_get_handler_data(desc);
- g = (struct davinci_gpio_regs __iomem *)d->regs;
+ ctlr = (struct davinci_gpio_controller *)
+ irq_desc_get_handler_data(desc);
+ regs = ctlr->regs;
/* we only care about one bank */
if (irq & 1)
@@ -267,13 +262,13 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
int res;
/* ack any irqs */
- status = __raw_readl(&g->intstat) & mask;
+ status = __raw_readl(®s->intstat) & mask;
if (!status)
break;
- __raw_writel(status, &g->intstat);
+ __raw_writel(status, ®s->intstat);
/* now demux them to the right lowlevel handler */
- n = d->irq_base;
+ n = ctlr->irq_base;
if (irq & 1) {
n += 16;
status >>= 16;
@@ -292,10 +287,10 @@ gpio_irq_handler(unsigned irq, struct irq_desc *desc)
static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
{
- struct davinci_gpio_controller *d = chip2controller(chip);
+ struct davinci_gpio_controller *ctlr = chip2controller(chip);
- if (d->irq_base >= 0)
- return d->irq_base + offset;
+ if (ctlr->irq_base >= 0)
+ return ctlr->irq_base + offset;
else
return -ENODEV;
}
@@ -304,7 +299,8 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
{
struct davinci_soc_info *soc_info = &davinci_soc_info;
- /* NOTE: we assume for now that only irqs in the first gpio_chip
+ /*
+ * NOTE: we assume for now that only irqs in the first gpio_chip
* can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
*/
if (offset < soc_info->gpio_unbanked)
@@ -315,22 +311,22 @@ static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
{
- struct davinci_gpio_controller *d;
- struct davinci_gpio_regs __iomem *g;
+ struct davinci_gpio_controller *ctlr;
+ struct davinci_gpio_regs __iomem *regs;
struct davinci_soc_info *soc_info = &davinci_soc_info;
u32 mask;
- d = (struct davinci_gpio_controller *)data->handler_data;
- g = (struct davinci_gpio_regs __iomem *)d->regs;
+ ctlr = (struct davinci_gpio_controller *)data->handler_data;
+ regs = (struct davinci_gpio_regs __iomem *)ctlr->regs;
mask = __gpio_mask(data->irq - soc_info->gpio_irq);
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
__raw_writel(mask, (trigger & IRQ_TYPE_EDGE_FALLING)
- ? &g->set_falling : &g->clr_falling);
+ ? ®s->set_falling : ®s->clr_falling);
__raw_writel(mask, (trigger & IRQ_TYPE_EDGE_RISING)
- ? &g->set_rising : &g->clr_rising);
+ ? ®s->set_rising : ®s->clr_rising);
return 0;
}
@@ -345,12 +341,11 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
static int __init davinci_gpio_irq_setup(void)
{
- unsigned gpio, irq, bank;
- struct clk *clk;
u32 binten = 0;
- unsigned ngpio, bank_irq;
+ unsigned gpio, irq, bank, ngpio, bank_irq;
+ struct clk *clk;
+ struct davinci_gpio_regs __iomem *regs;
struct davinci_soc_info *soc_info = &davinci_soc_info;
- struct davinci_gpio_regs __iomem *g;
ngpio = soc_info->gpio_num;
@@ -368,16 +363,16 @@ static int __init davinci_gpio_irq_setup(void)
}
clk_prepare_enable(clk);
- /* Arrange gpio_to_irq() support, handling either direct IRQs or
+ /*
+ * Arrange gpio_to_irq() support, handling either direct IRQs or
* banked IRQs. Having GPIOs in the first GPIO bank use direct
* 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_base = soc_info->gpio_unbanked
- ? -EINVAL
- : (soc_info->intc_irq_num + gpio);
+ ctlrs[bank].chip.to_irq = gpio_to_irq_banked;
+ ctlrs[bank].irq_base = soc_info->gpio_unbanked ?
+ -EINVAL : (soc_info->intc_irq_num + gpio);
}
/*
@@ -389,7 +384,7 @@ static int __init davinci_gpio_irq_setup(void)
static struct irq_chip_type gpio_unbanked;
/* pass "bank 0" GPIO IRQs to AINTC */
- chips[0].chip.to_irq = gpio_to_irq_unbanked;
+ ctlrs[0].chip.to_irq = gpio_to_irq_unbanked;
binten = BIT(0);
/* AINTC handles mask/unmask; GPIO handles triggering */
@@ -400,14 +395,14 @@ static int __init davinci_gpio_irq_setup(void)
gpio_unbanked.chip.irq_set_type = gpio_irq_type_unbanked;
/* default trigger: both edges */
- g = gpio2regs(0);
- __raw_writel(~0, &g->set_falling);
- __raw_writel(~0, &g->set_rising);
+ regs = gpio2regs(0);
+ __raw_writel(~0, ®s->set_falling);
+ __raw_writel(~0, ®s->set_rising);
/* set the direct IRQs up to use that irqchip */
for (gpio = 0; gpio < soc_info->gpio_unbanked; gpio++, irq++) {
irq_set_chip(irq, &gpio_unbanked.chip);
- irq_set_handler_data(irq, &chips[gpio / 32]);
+ irq_set_handler_data(irq, &ctlrs[gpio / 32]);
irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
}
@@ -424,9 +419,9 @@ static int __init davinci_gpio_irq_setup(void)
unsigned i;
/* disabled by default, enabled only as needed */
- g = gpio2regs(gpio);
- __raw_writel(~0, &g->clr_falling);
- __raw_writel(~0, &g->clr_rising);
+ regs = gpio2regs(gpio);
+ __raw_writel(~0, ®s->clr_falling);
+ __raw_writel(~0, ®s->clr_rising);
/* set up all irqs in this bank */
irq_set_chained_handler(bank_irq, gpio_irq_handler);
@@ -436,11 +431,11 @@ static int __init davinci_gpio_irq_setup(void)
* gpio irqs. Pass the irq bank's corresponding controller to
* the chained irq handler.
*/
- irq_set_handler_data(bank_irq, &chips[gpio / 32]);
+ irq_set_handler_data(bank_irq, &ctlrs[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_chip_data(irq, (__force void *)regs);
irq_set_handler_data(irq, (void *)__gpio_mask(gpio));
irq_set_handler(irq, handle_simple_irq);
set_irq_flags(irq, IRQF_VALID);
@@ -450,10 +445,11 @@ static int __init davinci_gpio_irq_setup(void)
}
done:
- /* BINTEN -- per-bank interrupt enable. genirq would also let these
+ /*
+ * BINTEN -- per-bank interrupt enable. genirq would also let these
* bits be set/cleared dynamically.
*/
- __raw_writel(binten, gpio_base + 0x08);
+ __raw_writel(binten, gpio_base + BINTEN);
printk(KERN_INFO "DaVinci: %d gpio irqs\n", irq - gpio_to_irq(0));
--
1.7.9.5
From: KV Sujith <[email protected]>
Modify GPIO Davinci driver to be compliant to standard platform drivers.
The driver did not have platform driver structure or a probe. Instead,
had a davinci_gpio_setup() function which is called in the pure_init
sequence. The function also had dependency on davinci_soc_info structure
of the corresponding platform. For Device Tree(DT) implementation, we
need to get rid of the dependency on the davinci_soc_info structure.
Hence as a first stage of DT conversion, we implement a probe. Future
commits shall modify the probe to read platform related data from DT.
- Add platform_driver structure and driver register function for davinci
GPIO driver. The driver registration is made to happen in
postcore_initcall. This is required since machine init functions like
da850_lcd_hw_init() make use of GPIO.
- Convert the davinci_gpio_setup() to davinci_gpio_probe().
- Remove access of members in soc_info structure. Instead, relevant data
are taken from davinci_gpio_platform_data structure pointed by
pdev->dev.platform_data.
- Change clk_get() to devm_clk_get() as devm_clk_get() is a device
managed function and makes error handling simpler.
- Change pr_err to dev_err for ngpio error reporting.
- Arrange include files and variables in alphabetical order
Signed-off-by: KV Sujith <[email protected]>
[[email protected]: Move global definition for "struct
davinci_gpio_controller" variable to local in probe and set it as driver
data.]
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/mach-davinci/include/mach/gpio-davinci.h | 2 +
drivers/gpio/gpio-davinci.c | 121 +++++++++++++++------
2 files changed, 87 insertions(+), 36 deletions(-)
diff --git a/arch/arm/mach-davinci/include/mach/gpio-davinci.h b/arch/arm/mach-davinci/include/mach/gpio-davinci.h
index 1fdd1fd..b325a1d 100644
--- a/arch/arm/mach-davinci/include/mach/gpio-davinci.h
+++ b/arch/arm/mach-davinci/include/mach/gpio-davinci.h
@@ -60,6 +60,8 @@ struct davinci_gpio_controller {
void __iomem *set_data;
void __iomem *clr_data;
void __iomem *in_data;
+ int gpio_unbanked;
+ unsigned gpio_irq;
};
/* The __gpio_to_controller() and __gpio_mask() functions inline to constants
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index d308955..08830aa 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -11,10 +11,17 @@
*/
#include <linux/clk.h>
+#include <linux/device.h>
#include <linux/errno.h>
#include <linux/gpio.h>
#include <linux/io.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/platform_data/gpio-davinci.h>
+#include <mach/gpio-davinci.h>
#include <asm/mach/irq.h>
@@ -35,10 +42,9 @@ struct davinci_gpio_regs {
#define chip2controller(chip) \
container_of(chip, struct davinci_gpio_controller, chip)
-static struct davinci_gpio_controller ctlrs[DIV_ROUND_UP(DAVINCI_N_GPIO, 32)];
static void __iomem *gpio_base;
-static struct davinci_gpio_regs __iomem __init *gpio2regs(unsigned gpio)
+static struct davinci_gpio_regs __iomem *gpio2regs(unsigned gpio)
{
void __iomem *ptr;
@@ -64,7 +70,7 @@ static inline struct davinci_gpio_regs __iomem *irq2regs(int irq)
irq_get_chip_data(irq);
}
-static int __init davinci_gpio_irq_setup(void);
+static int davinci_gpio_irq_setup(struct platform_device *pdev);
static inline int __davinci_direction(struct gpio_chip *chip,
unsigned offset, bool out, int value)
@@ -127,33 +133,52 @@ static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
__raw_writel((1 << offset), value ? ®s->set_data : ®s->clr_data);
}
-static int __init davinci_gpio_setup(void)
+static int davinci_gpio_probe(struct platform_device *pdev)
{
int i, base;
unsigned ngpio;
- struct davinci_soc_info *soc_info = &davinci_soc_info;
+ struct davinci_gpio_controller *ctlrs;
+ struct davinci_gpio_platform_data *pdata;
struct davinci_gpio_regs *regs;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
- if (soc_info->gpio_type != GPIO_TYPE_DAVINCI)
- return 0;
+ pdata = dev->platform_data;
+ if (!pdata) {
+ dev_err(dev, "GPIO: No Platform Data Supplied\n");
+ return -EINVAL;
+ }
/*
* The gpio banks conceptually expose a segmented bitmap,
* and "ngpio" is one more than the largest zero-based
* bit index that's valid.
*/
- ngpio = soc_info->gpio_num;
+ ngpio = pdata->ngpio;
if (ngpio == 0) {
- pr_err("GPIO setup: how many GPIOs?\n");
+ dev_err(dev, "GPIO Probe: how many GPIOs?\n");
return -EINVAL;
}
if (WARN_ON(DAVINCI_N_GPIO < ngpio))
ngpio = DAVINCI_N_GPIO;
- gpio_base = ioremap(soc_info->gpio_base, SZ_4K);
- if (WARN_ON(!gpio_base))
+ ctlrs = devm_kzalloc(dev,
+ ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL);
+ if (!ctlrs) {
+ dev_err(dev, "Memory alloc failed\n");
return -ENOMEM;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (unlikely(!res)) {
+ dev_err(dev, "Invalid mem resource\n");
+ return -ENODEV;
+ }
+
+ gpio_base = devm_ioremap_resource(dev, res);
+ if (!gpio_base)
+ return -EADDRNOTAVAIL;
for (i = 0, base = 0; base < ngpio; i++, base += 32) {
ctlrs[i].chip.label = "DaVinci";
@@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
gpiochip_add(&ctlrs[i].chip);
}
- soc_info->gpio_ctlrs = ctlrs;
- soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
-
- davinci_gpio_irq_setup();
+ platform_set_drvdata(pdev, ctlrs);
+ davinci_gpio_irq_setup(pdev);
return 0;
}
-pure_initcall(davinci_gpio_setup);
/*
* We expect irqs will normally be set up as input pins, but they can also be
@@ -297,14 +319,14 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
{
- struct davinci_soc_info *soc_info = &davinci_soc_info;
+ struct davinci_gpio_controller *ctlr = chip2controller(chip);
/*
* NOTE: we assume for now that only irqs in the first gpio_chip
* can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
*/
- if (offset < soc_info->gpio_unbanked)
- return soc_info->gpio_irq + offset;
+ if (offset < ctlr->irq_base)
+ return ctlr->gpio_irq + offset;
else
return -ENODEV;
}
@@ -313,12 +335,11 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
{
struct davinci_gpio_controller *ctlr;
struct davinci_gpio_regs __iomem *regs;
- struct davinci_soc_info *soc_info = &davinci_soc_info;
u32 mask;
ctlr = (struct davinci_gpio_controller *)data->handler_data;
regs = (struct davinci_gpio_regs __iomem *)ctlr->regs;
- mask = __gpio_mask(data->irq - soc_info->gpio_irq);
+ mask = __gpio_mask(data->irq - ctlr->gpio_irq);
if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
return -EINVAL;
@@ -339,23 +360,33 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
* (dm6446) can be set appropriately for GPIOV33 pins.
*/
-static int __init davinci_gpio_irq_setup(void)
+static int davinci_gpio_irq_setup(struct platform_device *pdev)
{
- u32 binten = 0;
- unsigned gpio, irq, bank, ngpio, bank_irq;
- struct clk *clk;
+ u32 binten = 0;
+ unsigned gpio, irq, bank, ngpio, bank_irq;
+ struct clk *clk;
+ struct davinci_gpio_controller *ctlrs = platform_get_drvdata(pdev);
+ struct davinci_gpio_platform_data *pdata;
struct davinci_gpio_regs __iomem *regs;
- struct davinci_soc_info *soc_info = &davinci_soc_info;
+ struct device *dev = &pdev->dev;
+ struct resource *res;
+
+ pdata = dev->platform_data;
+ ngpio = pdata->ngpio;
+ res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ if (unlikely(!res)) {
+ dev_err(dev, "Invalid IRQ resource\n");
+ return -ENODEV;
+ }
- ngpio = soc_info->gpio_num;
+ bank_irq = res->start;
- bank_irq = soc_info->gpio_irq;
- if (bank_irq == 0) {
- printk(KERN_ERR "Don't know first GPIO bank IRQ.\n");
- return -EINVAL;
+ if (unlikely(!bank_irq)) {
+ dev_err(dev, "Invalid IRQ resource\n");
+ return -ENODEV;
}
- clk = clk_get(NULL, "gpio");
+ clk = devm_clk_get(dev, "gpio");
if (IS_ERR(clk)) {
printk(KERN_ERR "Error %ld getting gpio clock?\n",
PTR_ERR(clk));
@@ -371,8 +402,8 @@ static int __init davinci_gpio_irq_setup(void)
*/
for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
ctlrs[bank].chip.to_irq = gpio_to_irq_banked;
- ctlrs[bank].irq_base = soc_info->gpio_unbanked ?
- -EINVAL : (soc_info->intc_irq_num + gpio);
+ ctlrs[bank].irq_base = pdata->gpio_unbanked ?
+ -EINVAL : (pdata->intc_irq_num + gpio);
}
/*
@@ -380,7 +411,7 @@ static int __init davinci_gpio_irq_setup(void)
* controller only handling trigger modes. We currently assume no
* IRQ mux conflicts; gpio_irq_type_unbanked() is only for GPIOs.
*/
- if (soc_info->gpio_unbanked) {
+ if (pdata->gpio_unbanked) {
static struct irq_chip_type gpio_unbanked;
/* pass "bank 0" GPIO IRQs to AINTC */
@@ -400,7 +431,7 @@ static int __init davinci_gpio_irq_setup(void)
__raw_writel(~0, ®s->set_rising);
/* set the direct IRQs up to use that irqchip */
- for (gpio = 0; gpio < soc_info->gpio_unbanked; gpio++, irq++) {
+ for (gpio = 0; gpio < pdata->gpio_unbanked; gpio++, irq++) {
irq_set_chip(irq, &gpio_unbanked.chip);
irq_set_handler_data(irq, &ctlrs[gpio / 32]);
irq_set_status_flags(irq, IRQ_TYPE_EDGE_BOTH);
@@ -455,3 +486,21 @@ done:
return 0;
}
+
+static struct platform_driver davinci_gpio_driver = {
+ .probe = davinci_gpio_probe,
+ .driver = {
+ .name = "davinci_gpio",
+ .owner = THIS_MODULE,
+ },
+};
+
+/**
+ * gpio driver register needs to be done before machine_init functions
+ * access gpio APIs. Hence davinci_gpio_drv_reg() is a postcore_initcall.
+ */
+static int __init davinci_gpio_drv_reg(void)
+{
+ return platform_driver_register(&davinci_gpio_driver);
+}
+postcore_initcall(davinci_gpio_drv_reg);
--
1.7.9.5
From: KV Sujith <[email protected]>
gpio controller resource information being associated with
davinci_soc_info structure and not created any device. Hence davinci
gpio didn't fall under proper device model. This patch creates gpio
davinci as a platform device for da8xx platforms.
- Add Memory and IRQ resources for DA8xx.
- Register GPIO platform driver for DA8xx.
- Add da8xx_register_gpio API to create platform device for da8xx
platforms.
Signed-off-by: KV Sujith <[email protected]>
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/mach-davinci/devices-da8xx.c | 26 ++++++++++++++++++++++++++
arch/arm/mach-davinci/include/mach/da8xx.h | 1 +
2 files changed, 27 insertions(+)
diff --git a/arch/arm/mach-davinci/devices-da8xx.c b/arch/arm/mach-davinci/devices-da8xx.c
index bf57252..892ad86 100644
--- a/arch/arm/mach-davinci/devices-da8xx.c
+++ b/arch/arm/mach-davinci/devices-da8xx.c
@@ -640,6 +640,32 @@ int __init da8xx_register_lcdc(struct da8xx_lcdc_platform_data *pdata)
return platform_device_register(&da8xx_lcdc_device);
}
+static struct resource da8xx_gpio_resources[] = {
+ { /* registers */
+ .start = DA8XX_GPIO_BASE,
+ .end = DA8XX_GPIO_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ { /* interrupt */
+ .start = IRQ_DA8XX_GPIO0,
+ .end = IRQ_DA8XX_GPIO8,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct platform_device da8xx_gpio_device = {
+ .name = "davinci_gpio",
+ .id = -1,
+ .num_resources = ARRAY_SIZE(da8xx_gpio_resources),
+ .resource = da8xx_gpio_resources,
+};
+
+int __init da8xx_register_gpio(void *pdata)
+{
+ da8xx_gpio_device.dev.platform_data = pdata;
+ return platform_device_register(&da8xx_gpio_device);
+}
+
static struct resource da8xx_mmcsd0_resources[] = {
{ /* registers */
.start = DA8XX_MMCSD0_BASE,
diff --git a/arch/arm/mach-davinci/include/mach/da8xx.h b/arch/arm/mach-davinci/include/mach/da8xx.h
index 2e1c9ea..aa66690 100644
--- a/arch/arm/mach-davinci/include/mach/da8xx.h
+++ b/arch/arm/mach-davinci/include/mach/da8xx.h
@@ -96,6 +96,7 @@ int da8xx_register_mmcsd0(struct davinci_mmc_config *config);
int da850_register_mmcsd1(struct davinci_mmc_config *config);
void __init da8xx_register_mcasp(int id, struct snd_platform_data *pdata);
int da8xx_register_rtc(void);
+int da8xx_register_gpio(void *pdata);
int da850_register_cpufreq(char *async_clk);
int da8xx_register_cpuidle(void);
void __iomem * __init da8xx_get_mem_ctlr(void);
--
1.7.9.5
gpio controller resource information being associated with
davinci_soc_info structure and not created any device. Hence davinci
gpio didn't fall under proper device model. This patch creates gpio
davinci as a platform device for dm platforms.
Also add daivinci_register_gpio API to create platform device for dm*
platforms.
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/mach-davinci/devices.c | 13 +++++++++++++
arch/arm/mach-davinci/include/mach/common.h | 2 ++
2 files changed, 15 insertions(+)
diff --git a/arch/arm/mach-davinci/devices.c b/arch/arm/mach-davinci/devices.c
index a7068a3..b4f345b 100644
--- a/arch/arm/mach-davinci/devices.c
+++ b/arch/arm/mach-davinci/devices.c
@@ -313,6 +313,19 @@ static void davinci_init_wdt(void)
platform_device_register(&davinci_wdt_device);
}
+static struct platform_device davinci_gpio_device = {
+ .name = "davinci_gpio",
+ .id = -1,
+};
+
+int davinci_gpio_register(struct resource *res, int size, void *pdata)
+{
+ davinci_gpio_device.resource = res;
+ davinci_gpio_device.num_resources = size;
+ davinci_gpio_device.dev.platform_data = pdata;
+ return platform_device_register(&davinci_gpio_device);
+}
+
/*-------------------------------------------------------------------------*/
/*-------------------------------------------------------------------------*/
diff --git a/arch/arm/mach-davinci/include/mach/common.h b/arch/arm/mach-davinci/include/mach/common.h
index b124b77..bd389ba 100644
--- a/arch/arm/mach-davinci/include/mach/common.h
+++ b/arch/arm/mach-davinci/include/mach/common.h
@@ -14,6 +14,7 @@
#include <linux/compiler.h>
#include <linux/types.h>
+#include <linux/ioport.h>
extern void davinci_timer_init(void);
@@ -83,6 +84,7 @@ extern void davinci_common_init(struct davinci_soc_info *soc_info);
extern void davinci_init_ide(void);
void davinci_restart(char mode, const char *cmd);
void davinci_init_late(void);
+int davinci_gpio_register(struct resource *res, int size, void *pdata);
#ifdef CONFIG_DAVINCI_RESET_CLOCKS
int davinci_clk_disable_unused(void);
--
1.7.9.5
Create davinci gpio device and remove references in davinci_soc_info
structure. Also rearrange header file inclusion in group basis.
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/mach-davinci/board-da830-evm.c | 19 +++++++++++++++----
arch/arm/mach-davinci/board-da850-evm.c | 11 +++++++++++
arch/arm/mach-davinci/board-omapl138-hawk.c | 2 ++
arch/arm/mach-davinci/da830.c | 4 ----
arch/arm/mach-davinci/da850.c | 4 ----
5 files changed, 28 insertions(+), 12 deletions(-)
diff --git a/arch/arm/mach-davinci/board-da830-evm.c b/arch/arm/mach-davinci/board-da830-evm.c
index 1332de8..4e8bcc1 100644
--- a/arch/arm/mach-davinci/board-da830-evm.c
+++ b/arch/arm/mach-davinci/board-da830-evm.c
@@ -22,17 +22,19 @@
#include <linux/mtd/partitions.h>
#include <linux/spi/spi.h>
#include <linux/spi/flash.h>
+#include <linux/platform_data/mtd-davinci.h>
+#include <linux/platform_data/gpio-davinci.h>
+#include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_data/mtd-davinci-aemif.h>
+#include <linux/platform_data/spi-davinci.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
#include <mach/cp_intc.h>
#include <mach/mux.h>
-#include <linux/platform_data/mtd-davinci.h>
+#include <mach/common.h>
#include <mach/da8xx.h>
-#include <linux/platform_data/usb-davinci.h>
-#include <linux/platform_data/mtd-davinci-aemif.h>
-#include <linux/platform_data/spi-davinci.h>
#define DA830_EVM_PHY_ID ""
/*
@@ -590,11 +592,20 @@ static struct spi_board_info da830evm_spi_info[] = {
},
};
+static struct davinci_gpio_platform_data da830_gpio_platform_data = {
+ .ngpio = 128,
+ .intc_irq_num = DA830_N_CP_INTC_IRQ,
+};
+
static __init void da830_evm_init(void)
{
struct davinci_soc_info *soc_info = &davinci_soc_info;
int ret;
+ ret = da8xx_register_gpio(&da830_gpio_platform_data);
+ if (ret)
+ pr_warn("da830_evm_init: GPIO init failed: %d\n", ret);
+
ret = da830_register_edma(da830_edma_rsv);
if (ret)
pr_warning("da830_evm_init: edma registration failed: %d\n",
diff --git a/arch/arm/mach-davinci/board-da850-evm.c b/arch/arm/mach-davinci/board-da850-evm.c
index 8a24b6c..d5dd010 100644
--- a/arch/arm/mach-davinci/board-da850-evm.c
+++ b/arch/arm/mach-davinci/board-da850-evm.c
@@ -28,6 +28,7 @@
#include <linux/mtd/partitions.h>
#include <linux/mtd/physmap.h>
#include <linux/platform_device.h>
+#include <linux/platform_data/gpio-davinci.h>
#include <linux/platform_data/mtd-davinci.h>
#include <linux/platform_data/mtd-davinci-aemif.h>
#include <linux/platform_data/spi-davinci.h>
@@ -42,6 +43,7 @@
#include <mach/da8xx.h>
#include <mach/mux.h>
#include <mach/sram.h>
+#include <mach/common.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -1138,6 +1140,11 @@ static struct edma_rsv_info *da850_edma_rsv[2] = {
&da850_edma_cc1_rsv,
};
+static struct davinci_gpio_platform_data da850_gpio_platform_data = {
+ .ngpio = 144,
+ .intc_irq_num = DA850_N_CP_INTC_IRQ,
+};
+
#ifdef CONFIG_CPU_FREQ
static __init int da850_evm_init_cpufreq(void)
{
@@ -1444,6 +1451,10 @@ static __init void da850_evm_init(void)
{
int ret;
+ ret = da8xx_register_gpio(&da850_gpio_platform_data);
+ if (ret)
+ pr_warn("da850_evm_init: GPIO init failed: %d\n", ret);
+
ret = pmic_tps65070_init();
if (ret)
pr_warn("%s: TPS65070 PMIC init failed: %d\n", __func__, ret);
diff --git a/arch/arm/mach-davinci/board-omapl138-hawk.c b/arch/arm/mach-davinci/board-omapl138-hawk.c
index b8c20de..1f44a1b 100644
--- a/arch/arm/mach-davinci/board-omapl138-hawk.c
+++ b/arch/arm/mach-davinci/board-omapl138-hawk.c
@@ -13,6 +13,7 @@
#include <linux/init.h>
#include <linux/console.h>
#include <linux/gpio.h>
+#include <linux/platform_data/gpio-davinci.h>
#include <asm/mach-types.h>
#include <asm/mach/arch.h>
@@ -20,6 +21,7 @@
#include <mach/cp_intc.h>
#include <mach/da8xx.h>
#include <mach/mux.h>
+#include <mach/common.h>
#define HAWKBOARD_PHY_ID "davinci_mdio-0:07"
#define DA850_HAWK_MMCSD_CD_PIN GPIO_TO_PIN(3, 12)
diff --git a/arch/arm/mach-davinci/da830.c b/arch/arm/mach-davinci/da830.c
index abbaf02..e7b79ee 100644
--- a/arch/arm/mach-davinci/da830.c
+++ b/arch/arm/mach-davinci/da830.c
@@ -1195,10 +1195,6 @@ static struct davinci_soc_info davinci_soc_info_da830 = {
.intc_irq_prios = da830_default_priorities,
.intc_irq_num = DA830_N_CP_INTC_IRQ,
.timer_info = &da830_timer_info,
- .gpio_type = GPIO_TYPE_DAVINCI,
- .gpio_base = DA8XX_GPIO_BASE,
- .gpio_num = 128,
- .gpio_irq = IRQ_DA8XX_GPIO0,
.serial_dev = &da8xx_serial_device,
.emac_pdata = &da8xx_emac_pdata,
};
diff --git a/arch/arm/mach-davinci/da850.c b/arch/arm/mach-davinci/da850.c
index 4d69338..5f7cfa4 100644
--- a/arch/arm/mach-davinci/da850.c
+++ b/arch/arm/mach-davinci/da850.c
@@ -1297,10 +1297,6 @@ static struct davinci_soc_info davinci_soc_info_da850 = {
.intc_irq_prios = da850_default_priorities,
.intc_irq_num = DA850_N_CP_INTC_IRQ,
.timer_info = &da850_timer_info,
- .gpio_type = GPIO_TYPE_DAVINCI,
- .gpio_base = DA8XX_GPIO_BASE,
- .gpio_num = 144,
- .gpio_irq = IRQ_DA8XX_GPIO0,
.serial_dev = &da8xx_serial_device,
.emac_pdata = &da8xx_emac_pdata,
.sram_dma = DA8XX_SHARED_RAM_BASE,
--
1.7.9.5
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/mach-davinci/board-dm355-evm.c | 27 ++++++++++++++++++++++++++
arch/arm/mach-davinci/board-dm355-leopard.c | 1 +
arch/arm/mach-davinci/board-dm365-evm.c | 28 +++++++++++++++++++++++++++
arch/arm/mach-davinci/board-dm644x-evm.c | 26 +++++++++++++++++++++++++
arch/arm/mach-davinci/board-dm646x-evm.c | 27 ++++++++++++++++++++++++++
arch/arm/mach-davinci/board-neuros-osd2.c | 1 +
arch/arm/mach-davinci/dm355.c | 4 ----
arch/arm/mach-davinci/dm365.c | 5 -----
arch/arm/mach-davinci/dm644x.c | 4 ----
arch/arm/mach-davinci/dm646x.c | 4 ----
arch/arm/mach-davinci/include/mach/common.h | 2 ++
11 files changed, 112 insertions(+), 17 deletions(-)
diff --git a/arch/arm/mach-davinci/board-dm355-evm.c b/arch/arm/mach-davinci/board-dm355-evm.c
index bfdf8b9..785c7b8 100644
--- a/arch/arm/mach-davinci/board-dm355-evm.c
+++ b/arch/arm/mach-davinci/board-dm355-evm.c
@@ -28,9 +28,11 @@
#include <linux/platform_data/i2c-davinci.h>
#include <mach/serial.h>
+#include <mach/common.h>
#include <linux/platform_data/mtd-davinci.h>
#include <linux/platform_data/mmc-davinci.h>
#include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_data/gpio-davinci.h>
#include "davinci.h"
@@ -311,9 +313,34 @@ static struct spi_board_info dm355_evm_spi_info[] __initconst = {
},
};
+static struct resource dm355_gpio_resources[] = {
+ { /* registers */
+ .start = DAVINCI_GPIO_BASE,
+ .end = DAVINCI_GPIO_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ { /* interrupt */
+ .start = IRQ_DM355_GPIOBNK0,
+ .end = IRQ_DM355_GPIOBNK6,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct davinci_gpio_platform_data dm355_gpio_platform_data = {
+ .ngpio = 104,
+ .intc_irq_num = DAVINCI_N_AINTC_IRQ,
+};
+
static __init void dm355_evm_init(void)
{
struct clk *aemif;
+ int ret;
+
+ ret = davinci_gpio_register(dm355_gpio_resources,
+ sizeof(dm355_gpio_resources),
+ &dm355_gpio_platform_data);
+ if (ret)
+ pr_warn("dm355_evm_init: GPIO init failed: %d\n", ret);
gpio_request(1, "dm9000");
gpio_direction_input(1);
diff --git a/arch/arm/mach-davinci/board-dm355-leopard.c b/arch/arm/mach-davinci/board-dm355-leopard.c
index dff4ddc..34a2b64 100644
--- a/arch/arm/mach-davinci/board-dm355-leopard.c
+++ b/arch/arm/mach-davinci/board-dm355-leopard.c
@@ -25,6 +25,7 @@
#include <linux/platform_data/i2c-davinci.h>
#include <mach/serial.h>
+#include <mach/common.h>
#include <linux/platform_data/mtd-davinci.h>
#include <linux/platform_data/mmc-davinci.h>
#include <linux/platform_data/usb-davinci.h>
diff --git a/arch/arm/mach-davinci/board-dm365-evm.c b/arch/arm/mach-davinci/board-dm365-evm.c
index 4cfdd91..623263e 100644
--- a/arch/arm/mach-davinci/board-dm365-evm.c
+++ b/arch/arm/mach-davinci/board-dm365-evm.c
@@ -38,6 +38,7 @@
#include <linux/platform_data/mmc-davinci.h>
#include <linux/platform_data/mtd-davinci.h>
#include <linux/platform_data/keyscan-davinci.h>
+#include <linux/platform_data/gpio-davinci.h>
#include <media/tvp514x.h>
@@ -586,8 +587,35 @@ static struct spi_board_info dm365_evm_spi_info[] __initconst = {
},
};
+static struct resource dm365_gpio_resources[] = {
+ { /* registers */
+ .start = DAVINCI_GPIO_BASE,
+ .end = DAVINCI_GPIO_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ { /* interrupt */
+ .start = IRQ_DM365_GPIO0,
+ .end = IRQ_DM365_GPIO7,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct davinci_gpio_platform_data dm365_gpio_platform_data = {
+ .ngpio = 104,
+ .intc_irq_num = DAVINCI_N_AINTC_IRQ,
+ .gpio_unbanked = 8,
+};
+
static __init void dm365_evm_init(void)
{
+ int ret;
+
+ ret = davinci_gpio_register(dm365_gpio_resources,
+ sizeof(dm365_gpio_resources),
+ &dm365_gpio_platform_data);
+ if (ret)
+ pr_warn("dm365_evm_init: GPIO init failed: %d\n", ret);
+
evm_init_i2c();
davinci_serial_init(&uart_config);
diff --git a/arch/arm/mach-davinci/board-dm644x-evm.c b/arch/arm/mach-davinci/board-dm644x-evm.c
index fc8e38e..76d9f6b 100644
--- a/arch/arm/mach-davinci/board-dm644x-evm.c
+++ b/arch/arm/mach-davinci/board-dm644x-evm.c
@@ -39,6 +39,7 @@
#include <linux/platform_data/mmc-davinci.h>
#include <linux/platform_data/usb-davinci.h>
#include <linux/platform_data/mtd-davinci-aemif.h>
+#include <linux/platform_data/gpio-davinci.h>
#include "davinci.h"
@@ -755,11 +756,36 @@ static int davinci_phy_fixup(struct phy_device *phydev)
#define HAS_NAND IS_ENABLED(CONFIG_MTD_NAND_DAVINCI)
+static struct resource dm644_gpio_resources[] = {
+ { /* registers */
+ .start = DAVINCI_GPIO_BASE,
+ .end = DAVINCI_GPIO_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ { /* interrupt */
+ .start = IRQ_GPIOBNK0,
+ .end = IRQ_GPIOBNK4,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct davinci_gpio_platform_data dm644_gpio_platform_data = {
+ .ngpio = 71,
+ .intc_irq_num = DAVINCI_N_AINTC_IRQ,
+};
+
static __init void davinci_evm_init(void)
{
+ int ret;
struct clk *aemif_clk;
struct davinci_soc_info *soc_info = &davinci_soc_info;
+ ret = davinci_gpio_register(dm644_gpio_resources,
+ sizeof(dm644_gpio_resources),
+ &dm644_gpio_platform_data);
+ if (ret)
+ pr_warn("davinci_evm_init: GPIO init failed: %d\n", ret);
+
aemif_clk = clk_get(NULL, "aemif");
clk_prepare_enable(aemif_clk);
diff --git a/arch/arm/mach-davinci/board-dm646x-evm.c b/arch/arm/mach-davinci/board-dm646x-evm.c
index 03785e0..e419d9e 100644
--- a/arch/arm/mach-davinci/board-dm646x-evm.c
+++ b/arch/arm/mach-davinci/board-dm646x-evm.c
@@ -39,11 +39,13 @@
#include <mach/common.h>
#include <mach/serial.h>
+#include <mach/irqs.h>
#include <linux/platform_data/i2c-davinci.h>
#include <linux/platform_data/mtd-davinci.h>
#include <mach/clock.h>
#include <mach/cdce949.h>
#include <linux/platform_data/mtd-davinci-aemif.h>
+#include <linux/platform_data/gpio-davinci.h>
#include "davinci.h"
#include "clock.h"
@@ -787,10 +789,35 @@ static struct edma_rsv_info dm646x_edma_rsv[] = {
},
};
+static struct resource dm646_gpio_resources[] = {
+ { /* registers */
+ .start = DAVINCI_GPIO_BASE,
+ .end = DAVINCI_GPIO_BASE + SZ_4K - 1,
+ .flags = IORESOURCE_MEM,
+ },
+ { /* interrupt */
+ .start = IRQ_DM646X_GPIOBNK0,
+ .end = IRQ_DM646X_GPIOBNK2,
+ .flags = IORESOURCE_IRQ,
+ },
+};
+
+static struct davinci_gpio_platform_data dm646_gpio_platform_data = {
+ .ngpio = 43,
+ .intc_irq_num = DAVINCI_N_AINTC_IRQ,
+};
+
static __init void evm_init(void)
{
+ int ret;
struct davinci_soc_info *soc_info = &davinci_soc_info;
+ ret = davinci_gpio_register(dm646_gpio_resources,
+ sizeof(dm646_gpio_resources),
+ &dm646_gpio_platform_data);
+ if (ret)
+ pr_warn("evm_init: GPIO init failed: %d\n", ret);
+
evm_init_i2c();
davinci_serial_init(&uart_config);
dm646x_init_mcasp0(&dm646x_evm_snd_data[0]);
diff --git a/arch/arm/mach-davinci/board-neuros-osd2.c b/arch/arm/mach-davinci/board-neuros-osd2.c
index 2bc112a..c2b96ec 100644
--- a/arch/arm/mach-davinci/board-neuros-osd2.c
+++ b/arch/arm/mach-davinci/board-neuros-osd2.c
@@ -37,6 +37,7 @@
#include <linux/platform_data/mtd-davinci.h>
#include <linux/platform_data/mmc-davinci.h>
#include <linux/platform_data/usb-davinci.h>
+#include <linux/platform_data/gpio-davinci.h>
#include "davinci.h"
diff --git a/arch/arm/mach-davinci/dm355.c b/arch/arm/mach-davinci/dm355.c
index 87e6104..2ecbb71 100644
--- a/arch/arm/mach-davinci/dm355.c
+++ b/arch/arm/mach-davinci/dm355.c
@@ -840,10 +840,6 @@ static struct davinci_soc_info davinci_soc_info_dm355 = {
.intc_irq_prios = dm355_default_priorities,
.intc_irq_num = DAVINCI_N_AINTC_IRQ,
.timer_info = &dm355_timer_info,
- .gpio_type = GPIO_TYPE_DAVINCI,
- .gpio_base = DAVINCI_GPIO_BASE,
- .gpio_num = 104,
- .gpio_irq = IRQ_DM355_GPIOBNK0,
.serial_dev = &dm355_serial_device,
.sram_dma = 0x00010000,
.sram_len = SZ_32K,
diff --git a/arch/arm/mach-davinci/dm365.c b/arch/arm/mach-davinci/dm365.c
index 2791df9..861dec9 100644
--- a/arch/arm/mach-davinci/dm365.c
+++ b/arch/arm/mach-davinci/dm365.c
@@ -1084,11 +1084,6 @@ static struct davinci_soc_info davinci_soc_info_dm365 = {
.intc_irq_prios = dm365_default_priorities,
.intc_irq_num = DAVINCI_N_AINTC_IRQ,
.timer_info = &dm365_timer_info,
- .gpio_type = GPIO_TYPE_DAVINCI,
- .gpio_base = DAVINCI_GPIO_BASE,
- .gpio_num = 104,
- .gpio_irq = IRQ_DM365_GPIO0,
- .gpio_unbanked = 8, /* really 16 ... skip muxed GPIOs */
.serial_dev = &dm365_serial_device,
.emac_pdata = &dm365_emac_pdata,
.sram_dma = 0x00010000,
diff --git a/arch/arm/mach-davinci/dm644x.c b/arch/arm/mach-davinci/dm644x.c
index ab6bf54..d1d7670 100644
--- a/arch/arm/mach-davinci/dm644x.c
+++ b/arch/arm/mach-davinci/dm644x.c
@@ -868,10 +868,6 @@ static struct davinci_soc_info davinci_soc_info_dm644x = {
.intc_irq_prios = dm644x_default_priorities,
.intc_irq_num = DAVINCI_N_AINTC_IRQ,
.timer_info = &dm644x_timer_info,
- .gpio_type = GPIO_TYPE_DAVINCI,
- .gpio_base = DAVINCI_GPIO_BASE,
- .gpio_num = 71,
- .gpio_irq = IRQ_GPIOBNK0,
.serial_dev = &dm644x_serial_device,
.emac_pdata = &dm644x_emac_pdata,
.sram_dma = 0x00008000,
diff --git a/arch/arm/mach-davinci/dm646x.c b/arch/arm/mach-davinci/dm646x.c
index ac7b431..1058e7c 100644
--- a/arch/arm/mach-davinci/dm646x.c
+++ b/arch/arm/mach-davinci/dm646x.c
@@ -845,10 +845,6 @@ static struct davinci_soc_info davinci_soc_info_dm646x = {
.intc_irq_prios = dm646x_default_priorities,
.intc_irq_num = DAVINCI_N_AINTC_IRQ,
.timer_info = &dm646x_timer_info,
- .gpio_type = GPIO_TYPE_DAVINCI,
- .gpio_base = DAVINCI_GPIO_BASE,
- .gpio_num = 43, /* Only 33 usable */
- .gpio_irq = IRQ_DM646X_GPIOBNK0,
.serial_dev = &dm646x_serial_device,
.emac_pdata = &dm646x_emac_pdata,
.sram_dma = 0x10010000,
diff --git a/arch/arm/mach-davinci/include/mach/common.h b/arch/arm/mach-davinci/include/mach/common.h
index bd389ba..f9d81fb 100644
--- a/arch/arm/mach-davinci/include/mach/common.h
+++ b/arch/arm/mach-davinci/include/mach/common.h
@@ -38,6 +38,8 @@ struct davinci_timer_info {
struct davinci_gpio_controller;
+#define DAVINCI_GPIO_BASE 0x01C67000
+
/*
* SoC info passed into common davinci modules.
*
--
1.7.9.5
- Remove NEED_MACH_GPIO_H config option for Davinci platforms to start
using common gpio library interface.
- Added struct davinci_gpio_controller definitions in platform_data
directory for Davinci gpio devices.
- Removed GPIO_TYPE_DAVINCI enum definition as GPIO Davinci is converted
to Linux device driver model.
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/Kconfig | 1 -
arch/arm/mach-davinci/include/mach/gpio-davinci.h | 6 +++--
include/linux/platform_data/gpio-davinci.h | 27 +++++++++++++++++++++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 13b7394..74d3e85 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -955,7 +955,6 @@ config ARCH_DAVINCI
select GENERIC_CLOCKEVENTS
select GENERIC_IRQ_CHIP
select HAVE_IDE
- select NEED_MACH_GPIO_H
select USE_OF
select ZONE_DMA
help
diff --git a/arch/arm/mach-davinci/include/mach/gpio-davinci.h b/arch/arm/mach-davinci/include/mach/gpio-davinci.h
index b325a1d..18140e0 100644
--- a/arch/arm/mach-davinci/include/mach/gpio-davinci.h
+++ b/arch/arm/mach-davinci/include/mach/gpio-davinci.h
@@ -23,9 +23,10 @@
#define DAVINCI_GPIO_BASE 0x01C67000
+#ifdef CONFIG_ARCH_DAVINCI_TNETV107X
+
enum davinci_gpio_type {
- GPIO_TYPE_DAVINCI = 0,
- GPIO_TYPE_TNETV107X,
+ GPIO_TYPE_TNETV107X = 0,
};
/*
@@ -90,4 +91,5 @@ static inline u32 __gpio_mask(unsigned gpio)
return 1 << (gpio % 32);
}
+#endif /* CONFIG_ARCH_DAVINCI_TNETV107X */
#endif /* __DAVINCI_DAVINCI_GPIO_H */
diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
index f1c8277..75805d4 100644
--- a/include/linux/platform_data/gpio-davinci.h
+++ b/include/linux/platform_data/gpio-davinci.h
@@ -18,10 +18,37 @@
#ifndef __ASM_ARCH_DAVINCI_GPIO_H
#define __ASM_ARCH_DAVINCI_GPIO_H
+#include <asm-generic/gpio.h>
+
struct davinci_gpio_platform_data {
u32 ngpio;
u32 gpio_unbanked;
u32 intc_irq_num;
};
+
+struct davinci_gpio_controller {
+ struct gpio_chip chip;
+ int irq_base;
+ spinlock_t lock;
+ void __iomem *regs;
+ void __iomem *set_data;
+ void __iomem *clr_data;
+ void __iomem *in_data;
+ int gpio_unbanked;
+ unsigned gpio_irq;
+};
+
+/*
+ * basic gpio routines
+ */
+#define GPIO(X) (X) /* 0 <= X <= (DAVINCI_N_GPIO - 1) */
+
+/* Convert GPIO signal to GPIO pin number */
+#define GPIO_TO_PIN(bank, gpio) (16 * (bank) + (gpio))
+
+static inline u32 __gpio_mask(unsigned gpio)
+{
+ return 1 << (gpio % 32);
+}
#endif
--
1.7.9.5
From: KV Sujith <[email protected]>
- Add of_device_id for Davinci GPIO driver.
- Add function to populate data from DT.
- Modify the probe to read from DT if DT match is found.
- Add DT binding documentation for Davinci GPIO properties in a new file
gpio-davinci.txt located at Documentation/devicetree/bindings/gpio/.
Cc: Grant Likely <[email protected]>
Cc: Rob Herring <[email protected]>
Cc: Rob Landley <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: KV Sujith <[email protected]>
Signed-off-by: Philip Avinash <[email protected]>
---
.../devicetree/bindings/gpio/gpio-davinci.txt | 26 +++++++++
drivers/gpio/gpio-davinci.c | 57 ++++++++++++++++++--
2 files changed, 80 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..0d599d9
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-davinci.txt
@@ -0,0 +1,26 @@
+Davinci GPIO controller bindings
+
+Required Properties:
+- compatible:"ti,da830-gpio"
+
+- reg: Physical base address of the controller and length of memory mapped
+ region.
+
+- interrupts: The Starting IRQ number for GPIO
+
+- ngpio: The number of GPIO pins supported
+
+- intc_irq_num: The number of IRQs supported by the Interrupt Controller
+
+- gpio_unbanked: The number of GPIOs that have an individual interrupt
+ line to processor.
+
+Example:
+gpio: gpio@1e26000 {
+ compatible = "ti,da830-gpio";
+ reg = <0x226000 0x1000>;
+ interrupts = <42>;
+ ngpio = <144>;
+ intc_irq_num = <101>;
+ gpio_unbanked = <0>;
+};
diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
index 08830aa..dbe3b83 100644
--- a/drivers/gpio/gpio-davinci.c
+++ b/drivers/gpio/gpio-davinci.c
@@ -19,6 +19,8 @@
#include <linux/irqdomain.h>
#include <linux/kernel.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>
#include <mach/gpio-davinci.h>
@@ -133,6 +135,50 @@ static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
__raw_writel((1 << offset), value ? ®s->set_data : ®s->clr_data);
}
+static struct davinci_gpio_platform_data *davinci_gpio_set_pdata_of(
+ struct platform_device *pdev)
+{
+ struct device_node *dn = pdev->dev.of_node;
+ struct davinci_gpio_platform_data *pdata;
+ u32 val, ret;
+
+ pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL);
+ if (pdata) {
+ ret = of_property_read_u32(dn, "ngpio", &val);
+ if (ret)
+ goto of_err;
+
+ pdata->ngpio = val;
+
+ ret = of_property_read_u32(dn, "gpio_unbanked", &val);
+ if (ret)
+ goto of_err;
+
+ pdata->gpio_unbanked = val;
+
+ ret = of_property_read_u32(dn, "intc_irq_num", &val);
+ if (ret)
+ goto of_err;
+
+ pdata->intc_irq_num = val;
+ }
+
+ return pdata;
+
+of_err:
+ dev_err(&pdev->dev, "Populating pdata from DT failed: err %d\n", ret);
+ return NULL;
+}
+
+static const struct of_device_id davinci_gpio_ids[] = {
+ {
+ .compatible = "ti,da830-gpio",
+ },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, davinci_gpio_ids);
+
static int davinci_gpio_probe(struct platform_device *pdev)
{
int i, base;
@@ -142,13 +188,17 @@ static int davinci_gpio_probe(struct platform_device *pdev)
struct davinci_gpio_regs *regs;
struct device *dev = &pdev->dev;
struct resource *res;
+ const struct of_device_id *match =
+ of_match_device(of_match_ptr(davinci_gpio_ids), &pdev->dev);
- pdata = dev->platform_data;
+ pdata = match ? davinci_gpio_set_pdata_of(pdev) : dev->platform_data;
if (!pdata) {
dev_err(dev, "GPIO: No Platform Data Supplied\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
@@ -490,8 +540,9 @@ done:
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
From: KV Sujith <[email protected]>
Add DT entries for Davinci GPIO.
Signed-off-by: KV Sujith <[email protected]>
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/boot/dts/da850.dtsi | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/arch/arm/boot/dts/da850.dtsi b/arch/arm/boot/dts/da850.dtsi
index 452bdc6..9014eba 100644
--- a/arch/arm/boot/dts/da850.dtsi
+++ b/arch/arm/boot/dts/da850.dtsi
@@ -126,6 +126,15 @@
>;
};
};
+ gpio: gpio@1e26000 {
+ compatible = "ti,da830-gpio";
+ reg = <0x226000 0x1000>;
+ interrupts = <42>;
+ ngpio = <144>;
+ intc_irq_num = <101>;
+ gpio_unbanked = <0>;
+ status = "disabled";
+ };
serial0: serial@1c42000 {
compatible = "ns16550a";
reg = <0x42000 0x100>;
--
1.7.9.5
From: KV Sujith <[email protected]>
- Add GPIO DT Data and pinmux for DA850 EVM. GPIO is configurable differently
on different boards. So add GPIO pinmuxing in dts file.
- Dependency: This patch is dependent on Grab-pin-control patch;
https://patchwork.kernel.org/patch/2013751/
Signed-off-by: KV Sujith <[email protected]>
Signed-off-by: Philip Avinash <[email protected]>
---
arch/arm/boot/dts/da850-evm.dts | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)
diff --git a/arch/arm/boot/dts/da850-evm.dts b/arch/arm/boot/dts/da850-evm.dts
index c914357..ab59e60 100644
--- a/arch/arm/boot/dts/da850-evm.dts
+++ b/arch/arm/boot/dts/da850-evm.dts
@@ -17,6 +17,20 @@
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";
@@ -90,6 +104,11 @@
};
};
};
+ gpio: gpio@1e26000 {
+ status = "okay";
+ pinctrl-names = "default";
+ pinctrl-0 = <&gpio_pins>;
+ };
};
nand_cs3@62000000 {
status = "okay";
--
1.7.9.5
Hello.
On 22-05-2013 11:10, Philip Avinash wrote:
> 1. Corrects coding and commenting styles
> 2. Variables name change to meaningful name
> 3. Remove unnecessary variable usage
> 4. Add BINTEN macro definition
>
> Signed-off-by: Philip Avinash <[email protected]>
> ---
> drivers/gpio/gpio-davinci.c | 182 +++++++++++++++++++++----------------------
> 1 file changed, 89 insertions(+), 93 deletions(-)
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index 17df6db..d308955 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
[...]
> @@ -31,10 +31,11 @@ struct davinci_gpio_regs {
> u32 intstat;
> };
>
> +#define BINTEN 0x08 /* GPIO Interrupt Per-Bank Enable Register */
Empty line needed here.
> #define chip2controller(chip) \
> container_of(chip, struct davinci_gpio_controller, chip)
>
[...]
> @@ -98,8 +94,8 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
> return __davinci_direction(chip, offset, false, 0);
> }
>
> -static int
> -davinci_direction_out(struct gpio_chip *chip, unsigned offset, int value)
> +static int davinci_direction_out(struct gpio_chip *chip, unsigned offset,
> + int value)
This line should be aligned under the next character after (.
[...]
> @@ -113,22 +109,22 @@ davinci_direction_out(struct gpio_chip *chip, unsigned offset, int value)
[...]
> /*
> * Assuming the pin is muxed as a gpio output, set its output value.
> */
> -static void
> -davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
> + int value)
Same here.
[...]
> @@ -368,16 +363,16 @@ static int __init davinci_gpio_irq_setup(void)
[...]
> for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
> - chips[bank].chip.to_irq = gpio_to_irq_banked;
> - chips[bank].irq_base = soc_info->gpio_unbanked
> - ? -EINVAL
> - : (soc_info->intc_irq_num + gpio);
> + ctlrs[bank].chip.to_irq = gpio_to_irq_banked;
> + ctlrs[bank].irq_base = soc_info->gpio_unbanked ?
> + -EINVAL : (soc_info->intc_irq_num + gpio);
() not needed here.
WBR, Sergei
On Wed, May 22, 2013 at 12:40:25PM +0530, Philip Avinash wrote:
> /*
> * Assuming the pin is muxed as a gpio output, set its output value.
> */
> -static void
> -davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
> + int value)
This kind of stuff is just churn. If you read Documentation/CodingStyle:
Statements longer than 80 columns will be broken into sensible chunks, unless
exceeding 80 columns significantly increases readability and does not hide
information. Descendants are always substantially shorter than the parent and
are placed substantially to the right. The same applies to function headers
with a long argument list. However, never break user-visible strings such as
printk messages, because that breaks the ability to grep for them.
"broken into sensible chunks". Here's the question: is the former a
sensible format? Arguably it is because it results in all the arguments
fitting on one line at the expense of missing the return value.
The latter is also a sensible format - but breaks the arguments instead
of the return value.
Both formats can be found in their entirety by grep by function name alone:
grep -1 davinci_gpio_set
or if you prefer to type some more then you end up with more specific
grep -A1 davinci_gpio_set
or
grep -B1 davinci_gpio_set
depending on the version.
Where there's no clear advantage one way or the other, let the authors
preference stand.
On Wed, May 22, 2013 at 18:29:46, Sergei Shtylyov wrote:
> Hello.
>
> On 22-05-2013 11:10, Philip Avinash wrote:
>
> > 1. Corrects coding and commenting styles
> > 2. Variables name change to meaningful name
> > 3. Remove unnecessary variable usage
> > 4. Add BINTEN macro definition
> >
> > Signed-off-by: Philip Avinash <[email protected]>
> > ---
> > drivers/gpio/gpio-davinci.c | 182 +++++++++++++++++++++----------------------
> > 1 file changed, 89 insertions(+), 93 deletions(-)
>
> > diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> > index 17df6db..d308955 100644
> > --- a/drivers/gpio/gpio-davinci.c
> > +++ b/drivers/gpio/gpio-davinci.c
> [...]
> > @@ -31,10 +31,11 @@ struct davinci_gpio_regs {
> > u32 intstat;
> > };
> >
> > +#define BINTEN 0x08 /* GPIO Interrupt Per-Bank Enable Register */
>
> Empty line needed here.
Ok, I will add.
>
> > #define chip2controller(chip) \
> > container_of(chip, struct davinci_gpio_controller, chip)
> >
> [...]
> > @@ -98,8 +94,8 @@ static int davinci_direction_in(struct gpio_chip *chip, unsigned offset)
> > return __davinci_direction(chip, offset, false, 0);
> > }
> >
> > -static int
> > -davinci_direction_out(struct gpio_chip *chip, unsigned offset, int value)
> > +static int davinci_direction_out(struct gpio_chip *chip, unsigned offset,
> > + int value)
>
> This line should be aligned under the next character after (.
Will remove this change as Russel pointed out.
>
> [...]
> > @@ -113,22 +109,22 @@ davinci_direction_out(struct gpio_chip *chip, unsigned offset, int value)
> [...]
> > /*
> > * Assuming the pin is muxed as a gpio output, set its output value.
> > */
> > -static void
> > -davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> > +static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
> > + int value)
>
> Same here.
I will remove this change as Russel pointed out.
>
> [...]
> > @@ -368,16 +363,16 @@ static int __init davinci_gpio_irq_setup(void)
> [...]
> > for (gpio = 0, bank = 0; gpio < ngpio; bank++, gpio += 32) {
> > - chips[bank].chip.to_irq = gpio_to_irq_banked;
> > - chips[bank].irq_base = soc_info->gpio_unbanked
> > - ? -EINVAL
> > - : (soc_info->intc_irq_num + gpio);
> > + ctlrs[bank].chip.to_irq = gpio_to_irq_banked;
> > + ctlrs[bank].irq_base = soc_info->gpio_unbanked ?
> > + -EINVAL : (soc_info->intc_irq_num + gpio);
>
> () not needed here.
Ok will remove in next version ().
Thanks
Avinash
>
> WBR, Sergei
>
>
On Wed, May 22, 2013 at 20:10:42, Russell King - ARM Linux wrote:
> On Wed, May 22, 2013 at 12:40:25PM +0530, Philip Avinash wrote:
> > /*
> > * Assuming the pin is muxed as a gpio output, set its output value.
> > */
> > -static void
> > -davinci_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> > +static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
> > + int value)
>
> This kind of stuff is just churn. If you read Documentation/CodingStyle:
>
> Statements longer than 80 columns will be broken into sensible chunks, unless
> exceeding 80 columns significantly increases readability and does not hide
> information. Descendants are always substantially shorter than the parent and
> are placed substantially to the right. The same applies to function headers
> with a long argument list. However, never break user-visible strings such as
> printk messages, because that breaks the ability to grep for them.
>
> "broken into sensible chunks". Here's the question: is the former a
> sensible format? Arguably it is because it results in all the arguments
> fitting on one line at the expense of missing the return value.
>
> The latter is also a sensible format - but breaks the arguments instead
> of the return value.
>
> Both formats can be found in their entirety by grep by function name alone:
>
> grep -1 davinci_gpio_set
>
> or if you prefer to type some more then you end up with more specific
>
> grep -A1 davinci_gpio_set
> or
> grep -B1 davinci_gpio_set
>
> depending on the version.
>
> Where there's no clear advantage one way or the other, let the authors
> preference stand.
Ok I understood and I will remove these changes in next version.
Thanks
Avinash
>
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> GPIO Davinci driver converted to platform driver to support DT booting.
> In this patch series
> - Cleaned gpio Davinci driver code with proper commenting style and appropriate
> variable names.
> - Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
> gpio related member updation in davinci_soc_info structure.
> - DT support added for da850 board and tested on da850 EVM.
> - Remove soc_info reference in the gpio davinci driver and start uses
> gpiolib interface.
I'll go over and review this. I assume this will then go through
some DaVinci tree and up into ARM SoC when/if I ACK it.
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> From: KV Sujith <[email protected]>
>
> Add struct davinci_gpio_platform_data davinci gpio module.
>
> Signed-off-by: KV Sujith <[email protected]>
> Signed-off-by: Philip Avinash <[email protected]>
Usually I squash such patches into the first patch making use of it,
but the spirit is good so:
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
Why not squash patch 1 into this patch?
Anyway, this bring us closer to the model of other drivers so:
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> From: KV Sujith <[email protected]>
>
> gpio controller resource information being associated with
> davinci_soc_info structure and not created any device. Hence davinci
> gpio didn't fall under proper device model. This patch creates gpio
> davinci as a platform device for da8xx platforms.
>
> - Add Memory and IRQ resources for DA8xx.
> - Register GPIO platform driver for DA8xx.
> - Add da8xx_register_gpio API to create platform device for da8xx
> platforms.
>
> Signed-off-by: KV Sujith <[email protected]>
> Signed-off-by: Philip Avinash <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> gpio controller resource information being associated with
> davinci_soc_info structure and not created any device. Hence davinci
> gpio didn't fall under proper device model. This patch creates gpio
> davinci as a platform device for dm platforms.
> Also add daivinci_register_gpio API to create platform device for dm*
> platforms.
>
> Signed-off-by: Philip Avinash <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> Create davinci gpio device and remove references in davinci_soc_info
> structure. Also rearrange header file inclusion in group basis.
>
> Signed-off-by: Philip Avinash <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> Signed-off-by: Philip Avinash <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> - Remove NEED_MACH_GPIO_H config option for Davinci platforms to start
> using common gpio library interface.
> - Added struct davinci_gpio_controller definitions in platform_data
> directory for Davinci gpio devices.
> - Removed GPIO_TYPE_DAVINCI enum definition as GPIO Davinci is converted
> to Linux device driver model.
>
> Signed-off-by: Philip Avinash <[email protected]>
Acked-by: Linus Walleij <[email protected]>
> +/* Convert GPIO signal to GPIO pin number */
> +#define GPIO_TO_PIN(bank, gpio) (16 * (bank) + (gpio))
Hm not synonymous with pinctrl pins, well I guess I have to live
with this...
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
(...)
> +- interrupts: The Starting IRQ number for GPIO
> +- intc_irq_num: The number of IRQs supported by the Interrupt Controller
(...)
No this is not how you pass a number of IRQs in the device tree.
"interrupts" is an array. Pass every interrupt here for a full
resolution of the IRQs.
Further this looks fishy:
+ interrupts = <42>;
Usually you pass flags with the IRQs, I would rather have expected
an array like this:
interrupts = < 90 0x4 96 0x4 14 0x4 15 0x4 79 0x4>;
0x4 is IRQ_TYPE_LEVEL_HIGH, you can use the dts
#include <dt-bindings/interrupt-controller/irq.h> and
define that symbolically.
Doesn't the DaVinci IRQ controller support *any* IRQ flags?
Since the driver code is not reading out the interrupts but
(I guess?) falling back to platform data IRQ assignment,
this seems wrong.
Yours,
Linus Walleij
On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
> From: KV Sujith <[email protected]>
>
> - Add GPIO DT Data and pinmux for DA850 EVM. GPIO is configurable differently
> on different boards. So add GPIO pinmuxing in dts file.
> - Dependency: This patch is dependent on Grab-pin-control patch;
> https://patchwork.kernel.org/patch/2013751/
>
> Signed-off-by: KV Sujith <[email protected]>
> Signed-off-by: Philip Avinash <[email protected]>
Looks fine to me, but Tony should review this.
Yours,
Linus Walleij
Hi Avinash,
On 5/22/2013 12:40 PM, Philip Avinash wrote:
> GPIO Davinci driver converted to platform driver to support DT booting.
> In this patch series
> - Cleaned gpio Davinci driver code with proper commenting style and appropriate
> variable names.
> - Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
> gpio related member updation in davinci_soc_info structure.
> - DT support added for da850 board and tested on da850 EVM.
> - Remove soc_info reference in the gpio davinci driver and start uses
> gpiolib interface.
Can you please document which platforms this series was tested on and how?
Thanks,
Sekhar
On Fri, Jun 07, 2013 at 13:40:52, Nori, Sekhar wrote:
> Hi Avinash,
>
> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> > GPIO Davinci driver converted to platform driver to support DT booting.
> > In this patch series
> > - Cleaned gpio Davinci driver code with proper commenting style and appropriate
> > variable names.
> > - Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
> > gpio related member updation in davinci_soc_info structure.
> > - DT support added for da850 board and tested on da850 EVM.
> > - Remove soc_info reference in the gpio davinci driver and start uses
> > gpiolib interface.
>
> Can you please document which platforms this series was tested on and how?
This series being tested on da850 EVM. Tested by setting VPIF_DOUT[12] as GPIO
pin [2] and reading GPIO status from GPIO[7,4]
GPIO[7,4] will reflect the status switch number 8 of the S7 dip switch in DA850 EVM.
Testing Procedure
Requirement GPIO SYSFS support [1]
#echo 116 > /sys/class/gpio/export
setting GPIO[7,4] as input GPIO
#echo "in" > /sys/class/gpio/gpio116/direction
#mount -t debugfs debugfs /sys/kernel/debug
Reading GPIO pin status
#cat /sys/kernel/debug/gpio
#echo 116 > /sys/class/gpio/unexport
1. Enable GPIO SYSFS support through menconfig
--- GPIO Support
[ ] Debug GPIO calls
[ ] /sys/class/gpio/... (sysfs interface)
2. Patch for pinmux support for GPIO[7,4] is available at
https://github.com/avinashphilip/am335x_linux/commits/linux_davinci_v3.10_soc_gpio
Thanks
Avinash
>
> Thanks,
> Sekhar
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Thu, May 30, 2013 at 23:55:22, Linus Walleij wrote:
> On Wed, May 22, 2013 at 9:10 AM, Philip Avinash <[email protected]> wrote:
>
> (...)
> > +- interrupts: The Starting IRQ number for GPIO
> > +- intc_irq_num: The number of IRQs supported by the Interrupt Controller
> (...)
>
> No this is not how you pass a number of IRQs in the device tree.
>
> "interrupts" is an array. Pass every interrupt here for a full
> resolution of the IRQs.
Correct. I will change.
>
> Further this looks fishy:
>
> + interrupts = <42>;
>
> Usually you pass flags with the IRQs, I would rather have expected
> an array like this:
>
> interrupts = < 90 0x4 96 0x4 14 0x4 15 0x4 79 0x4>;
>
> 0x4 is IRQ_TYPE_LEVEL_HIGH, you can use the dts
> #include <dt-bindings/interrupt-controller/irq.h> and
> define that symbolically.
>
> Doesn't the DaVinci IRQ controller support *any* IRQ flags?
I wasn't sure about it.
But from davinci GPIO driver perspective, GPIO pins are
configured as edge sensitive. So IRQ_TYPE_EDGE_BOTH can be used.
So I will correct Documentation and update DT nodes in next version.
>
> Since the driver code is not reading out the interrupts but
> (I guess?) falling back to platform data IRQ assignment,
> this seems wrong.
Driver code reads "Starting IRQ number for GPIO" from platform resource
See [PATCH 03/11] gpio: davinci: Modify to platform driver.
Driver requires only starting offset of gpio irq number. GPIO interrupt
Number expected in sequential order for davinci GPIO.
Thanks
Avinash
> Yours,
> Linus Walleij
>
On 6/10/2013 2:32 PM, Philip, Avinash wrote:
> On Fri, Jun 07, 2013 at 13:40:52, Nori, Sekhar wrote:
>> Hi Avinash,
>>
>> On 5/22/2013 12:40 PM, Philip Avinash wrote:
>>> GPIO Davinci driver converted to platform driver to support DT booting.
>>> In this patch series
>>> - Cleaned gpio Davinci driver code with proper commenting style and appropriate
>>> variable names.
>>> - Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
>>> gpio related member updation in davinci_soc_info structure.
>>> - DT support added for da850 board and tested on da850 EVM.
>>> - Remove soc_info reference in the gpio davinci driver and start uses
>>> gpiolib interface.
>>
>> Can you please document which platforms this series was tested on and how?
>
> This series being tested on da850 EVM. Tested by setting VPIF_DOUT[12] as GPIO
> pin [2] and reading GPIO status from GPIO[7,4]
>
> GPIO[7,4] will reflect the status switch number 8 of the S7 dip switch in DA850 EVM.
>
> Testing Procedure
>
> Requirement GPIO SYSFS support [1]
>
> #echo 116 > /sys/class/gpio/export
> setting GPIO[7,4] as input GPIO
> #echo "in" > /sys/class/gpio/gpio116/direction
> #mount -t debugfs debugfs /sys/kernel/debug
> Reading GPIO pin status
> #cat /sys/kernel/debug/gpio
> #echo 116 > /sys/class/gpio/unexport
>
>
> 1. Enable GPIO SYSFS support through menconfig
> --- GPIO Support
> [ ] Debug GPIO calls
> [ ] /sys/class/gpio/... (sysfs interface)
>
> 2. Patch for pinmux support for GPIO[7,4] is available at
> https://github.com/avinashphilip/am335x_linux/commits/linux_davinci_v3.10_soc_gpio
Can you check interrupt generation too since there are significant
changes in that area? You can generate an interrupt using the MMC/SD
card detect pin (inserting an MMC/SD card into the slot should generate
an interrupt). You can also write a new value to any GPIO pin. That will
also trigger an interrupt.
It will be nice if you can test this series on DM365 as well (read/write
as well as interrupt).
Thanks,
Sekhar
On Tue, Jun 11, 2013 at 10:09:17, Nori, Sekhar wrote:
>
> On 6/10/2013 2:32 PM, Philip, Avinash wrote:
> > On Fri, Jun 07, 2013 at 13:40:52, Nori, Sekhar wrote:
> >> Hi Avinash,
> >>
> >> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> >>> GPIO Davinci driver converted to platform driver to support DT booting.
> >>> In this patch series
> >>> - Cleaned gpio Davinci driver code with proper commenting style and appropriate
> >>> variable names.
> >>> - Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
> >>> gpio related member updation in davinci_soc_info structure.
> >>> - DT support added for da850 board and tested on da850 EVM.
> >>> - Remove soc_info reference in the gpio davinci driver and start uses
> >>> gpiolib interface.
> >>
> >> Can you please document which platforms this series was tested on and how?
> >
> > This series being tested on da850 EVM. Tested by setting VPIF_DOUT[12] as GPIO
> > pin [2] and reading GPIO status from GPIO[7,4]
> >
> > GPIO[7,4] will reflect the status switch number 8 of the S7 dip switch in DA850 EVM.
> >
> > Testing Procedure
> >
> > Requirement GPIO SYSFS support [1]
> >
> > #echo 116 > /sys/class/gpio/export
> > setting GPIO[7,4] as input GPIO
> > #echo "in" > /sys/class/gpio/gpio116/direction
> > #mount -t debugfs debugfs /sys/kernel/debug
> > Reading GPIO pin status
> > #cat /sys/kernel/debug/gpio
> > #echo 116 > /sys/class/gpio/unexport
> >
> >
> > 1. Enable GPIO SYSFS support through menconfig
> > --- GPIO Support
> > [ ] Debug GPIO calls
> > [ ] /sys/class/gpio/... (sysfs interface)
> >
> > 2. Patch for pinmux support for GPIO[7,4] is available at
> > https://github.com/avinashphilip/am335x_linux/commits/linux_davinci_v3.10_soc_gpio
>
> Can you check interrupt generation too since there are significant
> changes in that area? You can generate an interrupt using the MMC/SD
> card detect pin (inserting an MMC/SD card into the slot should generate
> an interrupt). You can also write a new value to any GPIO pin. That will
> also trigger an interrupt.
I have tested GPIO interrupt with GPIO[7,4] by inserting a kernel module in DA850 EVM.
For GPIO interrupt generation DIP switch position changed.
Source code for GPIO test kernel module is shared at
https://github.com/avinashphilip/am335x_linux/commit/affc0c1841beacd8430af689f7f12dcab0cbaf28
>
> It will be nice if you can test this series on DM365 as well (read/write
> as well as interrupt).
Don't have any DM365 board for testing.
Thanks
Avinash
>
> Thanks,
> Sekhar
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 5/22/2013 12:40 PM, Philip Avinash wrote:
> From: KV Sujith <[email protected]>
>
> Add struct davinci_gpio_platform_data davinci gpio module.
>
> Signed-off-by: KV Sujith <[email protected]>
> Signed-off-by: Philip Avinash <[email protected]>
As Linus commented before, this should be merged with 03/11.
> ---
> include/linux/platform_data/gpio-davinci.h | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
> create mode 100644 include/linux/platform_data/gpio-davinci.h
>
> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
> new file mode 100644
> index 0000000..f1c8277
> --- /dev/null
> +++ b/include/linux/platform_data/gpio-davinci.h
> @@ -0,0 +1,27 @@
> +/*
> + * gpio-davinci.h
I would drop this unnecessary filename mention as well.
> + *
> + * DaVinci GPIO Platform Related Defines
> + *
> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation version 2.
> + *
> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> + * kind, whether express or implied; without even the implied warranty
> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef __ASM_ARCH_DAVINCI_GPIO_H
> +#define __ASM_ARCH_DAVINCI_GPIO_H
This should now be __PLATFORM_DATA_DAVINCI_GPIO_H__ or some such since
the file as been moved out of arch specific folder.
Thanks,
Sekhar
Hello.
On 11-06-2013 14:36, Sekhar Nori wrote:
>> From: KV Sujith <[email protected]>
>> Add struct davinci_gpio_platform_data davinci gpio module.
>> Signed-off-by: KV Sujith <[email protected]>
>> Signed-off-by: Philip Avinash <[email protected]>
> As Linus commented before, this should be merged with 03/11.
>> ---
>> include/linux/platform_data/gpio-davinci.h | 27 +++++++++++++++++++++++++++
>> 1 file changed, 27 insertions(+)
>> create mode 100644 include/linux/platform_data/gpio-davinci.h
>> diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
>> new file mode 100644
>> index 0000000..f1c8277
>> --- /dev/null
>> +++ b/include/linux/platform_data/gpio-davinci.h
>> @@ -0,0 +1,27 @@
>> +/*
>> + * gpio-davinci.h
>
> I would drop this unnecessary filename mention as well.
>
>> + *
>> + * DaVinci GPIO Platform Related Defines
>> + *
>> + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation version 2.
>> + *
>> + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
>> + * kind, whether express or implied; without even the implied warranty
>> + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#ifndef __ASM_ARCH_DAVINCI_GPIO_H
>> +#define __ASM_ARCH_DAVINCI_GPIO_H
> This should now be __PLATFORM_DATA_DAVINCI_GPIO_H__ or some such since
> the file as been moved out of arch specific folder.
I think __GPIO_DAVINCI_H would be enough...
> Thanks,
> Sekhar
WBR, Sergei
On 5/22/2013 12:40 PM, Philip Avinash wrote:
> 1. Corrects coding and commenting styles
> 2. Variables name change to meaningful name
> 3. Remove unnecessary variable usage
> 4. Add BINTEN macro definition
>
> Signed-off-by: Philip Avinash <[email protected]>
As Russell mentioned, the 80 char limit changes are churn. I looked at
the variable name renaming and that's also churn with little benefit.
The only changes that are probably worth doing are alphabetical ordering
of include files, fixes for mult-line commenting style and usage of
macro for register offset.
As I started reviewing these patches, I also started fixing some of the
comments I had and I have pushed the resulting work here:
https://git.kernel.org/cgit/linux/kernel/git/nsekhar/linux-davinci.git/log/?h=devel-gpio
It is only compile tested, but I hope this will serve as good starting
point for your next submission.
Thanks,
Sekhar
On 6/11/2013 12:19 PM, Philip, Avinash wrote:
> On Tue, Jun 11, 2013 at 10:09:17, Nori, Sekhar wrote:
>>
>> On 6/10/2013 2:32 PM, Philip, Avinash wrote:
>>> On Fri, Jun 07, 2013 at 13:40:52, Nori, Sekhar wrote:
>>>> Hi Avinash,
>>>>
>>>> On 5/22/2013 12:40 PM, Philip Avinash wrote:
>>>>> GPIO Davinci driver converted to platform driver to support DT booting.
>>>>> In this patch series
>>>>> - Cleaned gpio Davinci driver code with proper commenting style and appropriate
>>>>> variable names.
>>>>> - Create platform driver for GPIO Davinci in da8xx and dm* platforms and removed
>>>>> gpio related member updation in davinci_soc_info structure.
>>>>> - DT support added for da850 board and tested on da850 EVM.
>>>>> - Remove soc_info reference in the gpio davinci driver and start uses
>>>>> gpiolib interface.
>>>>
>>>> Can you please document which platforms this series was tested on and how?
>>>
>>> This series being tested on da850 EVM. Tested by setting VPIF_DOUT[12] as GPIO
>>> pin [2] and reading GPIO status from GPIO[7,4]
>>>
>>> GPIO[7,4] will reflect the status switch number 8 of the S7 dip switch in DA850 EVM.
>>>
>>> Testing Procedure
>>>
>>> Requirement GPIO SYSFS support [1]
>>>
>>> #echo 116 > /sys/class/gpio/export
>>> setting GPIO[7,4] as input GPIO
>>> #echo "in" > /sys/class/gpio/gpio116/direction
>>> #mount -t debugfs debugfs /sys/kernel/debug
>>> Reading GPIO pin status
>>> #cat /sys/kernel/debug/gpio
>>> #echo 116 > /sys/class/gpio/unexport
>>>
>>>
>>> 1. Enable GPIO SYSFS support through menconfig
>>> --- GPIO Support
>>> [ ] Debug GPIO calls
>>> [ ] /sys/class/gpio/... (sysfs interface)
>>>
>>> 2. Patch for pinmux support for GPIO[7,4] is available at
>>> https://github.com/avinashphilip/am335x_linux/commits/linux_davinci_v3.10_soc_gpio
>>
>> Can you check interrupt generation too since there are significant
>> changes in that area? You can generate an interrupt using the MMC/SD
>> card detect pin (inserting an MMC/SD card into the slot should generate
>> an interrupt). You can also write a new value to any GPIO pin. That will
>> also trigger an interrupt.
>
> I have tested GPIO interrupt with GPIO[7,4] by inserting a kernel module in DA850 EVM.
> For GPIO interrupt generation DIP switch position changed.
>
> Source code for GPIO test kernel module is shared at
> https://github.com/avinashphilip/am335x_linux/commit/affc0c1841beacd8430af689f7f12dcab0cbaf28
Thanks for the test report.
Regards,
Sekhar
On 5/22/2013 12:40 PM, Philip Avinash wrote:
> From: KV Sujith <[email protected]>
>
> Modify GPIO Davinci driver to be compliant to standard platform drivers.
> The driver did not have platform driver structure or a probe. Instead,
> had a davinci_gpio_setup() function which is called in the pure_init
> sequence. The function also had dependency on davinci_soc_info structure
> of the corresponding platform. For Device Tree(DT) implementation, we
> need to get rid of the dependency on the davinci_soc_info structure.
> Hence as a first stage of DT conversion, we implement a probe. Future
> commits shall modify the probe to read platform related data from DT.
>
> - Add platform_driver structure and driver register function for davinci
> GPIO driver. The driver registration is made to happen in
> postcore_initcall. This is required since machine init functions like
> da850_lcd_hw_init() make use of GPIO.
> - Convert the davinci_gpio_setup() to davinci_gpio_probe().
> - Remove access of members in soc_info structure. Instead, relevant data
> are taken from davinci_gpio_platform_data structure pointed by
> pdev->dev.platform_data.
> - Change clk_get() to devm_clk_get() as devm_clk_get() is a device
> managed function and makes error handling simpler.
> - Change pr_err to dev_err for ngpio error reporting.
> - Arrange include files and variables in alphabetical order
>
> Signed-off-by: KV Sujith <[email protected]>
> [[email protected]: Move global definition for "struct
> davinci_gpio_controller" variable to local in probe and set it as driver
> data.]
> Signed-off-by: Philip Avinash <[email protected]>
> ---
> arch/arm/mach-davinci/include/mach/gpio-davinci.h | 2 +
> drivers/gpio/gpio-davinci.c | 121 +++++++++++++++------
> 2 files changed, 87 insertions(+), 36 deletions(-)
>
> diff --git a/arch/arm/mach-davinci/include/mach/gpio-davinci.h b/arch/arm/mach-davinci/include/mach/gpio-davinci.h
> index 1fdd1fd..b325a1d 100644
> --- a/arch/arm/mach-davinci/include/mach/gpio-davinci.h
> +++ b/arch/arm/mach-davinci/include/mach/gpio-davinci.h
> @@ -60,6 +60,8 @@ struct davinci_gpio_controller {
> void __iomem *set_data;
> void __iomem *clr_data;
> void __iomem *in_data;
> + int gpio_unbanked;
> + unsigned gpio_irq;
> };
>
> /* The __gpio_to_controller() and __gpio_mask() functions inline to constants
> diff --git a/drivers/gpio/gpio-davinci.c b/drivers/gpio/gpio-davinci.c
> index d308955..08830aa 100644
> --- a/drivers/gpio/gpio-davinci.c
> +++ b/drivers/gpio/gpio-davinci.c
> @@ -11,10 +11,17 @@
> */
>
> #include <linux/clk.h>
> +#include <linux/device.h>
> #include <linux/errno.h>
> #include <linux/gpio.h>
> #include <linux/io.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
> #include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/platform_data/gpio-davinci.h>
> +#include <mach/gpio-davinci.h>
This include seems unnecessary.
>
> #include <asm/mach/irq.h>
While at it, you can get rid of this include and use <linux/irq.h> instead?
>
> @@ -35,10 +42,9 @@ struct davinci_gpio_regs {
> #define chip2controller(chip) \
> container_of(chip, struct davinci_gpio_controller, chip)
>
> -static struct davinci_gpio_controller ctlrs[DIV_ROUND_UP(DAVINCI_N_GPIO, 32)];
> static void __iomem *gpio_base;
>
> -static struct davinci_gpio_regs __iomem __init *gpio2regs(unsigned gpio)
> +static struct davinci_gpio_regs __iomem *gpio2regs(unsigned gpio)
> {
> void __iomem *ptr;
>
> @@ -64,7 +70,7 @@ static inline struct davinci_gpio_regs __iomem *irq2regs(int irq)
> irq_get_chip_data(irq);
> }
>
> -static int __init davinci_gpio_irq_setup(void);
> +static int davinci_gpio_irq_setup(struct platform_device *pdev);
>
> static inline int __davinci_direction(struct gpio_chip *chip,
> unsigned offset, bool out, int value)
> @@ -127,33 +133,52 @@ static void davinci_gpio_set(struct gpio_chip *chip, unsigned offset,
> __raw_writel((1 << offset), value ? ®s->set_data : ®s->clr_data);
> }
>
> -static int __init davinci_gpio_setup(void)
> +static int davinci_gpio_probe(struct platform_device *pdev)
> {
> int i, base;
> unsigned ngpio;
> - struct davinci_soc_info *soc_info = &davinci_soc_info;
> + struct davinci_gpio_controller *ctlrs;
> + struct davinci_gpio_platform_data *pdata;
> struct davinci_gpio_regs *regs;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
>
> - if (soc_info->gpio_type != GPIO_TYPE_DAVINCI)
> - return 0;
> + pdata = dev->platform_data;
> + if (!pdata) {
> + dev_err(dev, "GPIO: No Platform Data Supplied\n");
dev_err should already tell that the error is coming from davinci-gpio
so no need to prefix GPIO: again.
> + return -EINVAL;
> + }
>
> /*
> * The gpio banks conceptually expose a segmented bitmap,
> * and "ngpio" is one more than the largest zero-based
> * bit index that's valid.
> */
> - ngpio = soc_info->gpio_num;
> + ngpio = pdata->ngpio;
> if (ngpio == 0) {
> - pr_err("GPIO setup: how many GPIOs?\n");
> + dev_err(dev, "GPIO Probe: how many GPIOs?\n");
> return -EINVAL;
> }
>
> if (WARN_ON(DAVINCI_N_GPIO < ngpio))
> ngpio = DAVINCI_N_GPIO;
>
> - gpio_base = ioremap(soc_info->gpio_base, SZ_4K);
> - if (WARN_ON(!gpio_base))
> + ctlrs = devm_kzalloc(dev,
> + ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL);
Line break alignment needs fixing.
> + if (!ctlrs) {
> + dev_err(dev, "Memory alloc failed\n");
> return -ENOMEM;
> + }
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (unlikely(!res)) {
> + dev_err(dev, "Invalid mem resource\n");
> + return -ENODEV;
-EBUSY is better if you cannot get the resource.
> + }
> +
> + gpio_base = devm_ioremap_resource(dev, res);
> + if (!gpio_base)
> + return -EADDRNOTAVAIL;
devm_ioremap_resource gives an error encoder pointer if it fails so
please use that instead of masking it.
>
> for (i = 0, base = 0; base < ngpio; i++, base += 32) {
> ctlrs[i].chip.label = "DaVinci";
> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> gpiochip_add(&ctlrs[i].chip);
> }
>
> - soc_info->gpio_ctlrs = ctlrs;
> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
You drop setting gpio_ctlrs_num here and don't introduce it anywhere
else in the patchset so in effect you render the inline gpio get/set API
useless. Looks like this initialization should be moved to platform code?
> -
> - davinci_gpio_irq_setup();
> + platform_set_drvdata(pdev, ctlrs);
> + davinci_gpio_irq_setup(pdev);
> return 0;
> }
> -pure_initcall(davinci_gpio_setup);
>
> /*
> * We expect irqs will normally be set up as input pins, but they can also be
> @@ -297,14 +319,14 @@ static int gpio_to_irq_banked(struct gpio_chip *chip, unsigned offset)
>
> static int gpio_to_irq_unbanked(struct gpio_chip *chip, unsigned offset)
> {
> - struct davinci_soc_info *soc_info = &davinci_soc_info;
> + struct davinci_gpio_controller *ctlr = chip2controller(chip);
>
> /*
> * NOTE: we assume for now that only irqs in the first gpio_chip
> * can provide direct-mapped IRQs to AINTC (up to 32 GPIOs).
> */
> - if (offset < soc_info->gpio_unbanked)
> - return soc_info->gpio_irq + offset;
> + if (offset < ctlr->irq_base)
> + return ctlr->gpio_irq + offset;
> else
> return -ENODEV;
> }
> @@ -313,12 +335,11 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
> {
> struct davinci_gpio_controller *ctlr;
> struct davinci_gpio_regs __iomem *regs;
> - struct davinci_soc_info *soc_info = &davinci_soc_info;
> u32 mask;
>
> ctlr = (struct davinci_gpio_controller *)data->handler_data;
> regs = (struct davinci_gpio_regs __iomem *)ctlr->regs;
> - mask = __gpio_mask(data->irq - soc_info->gpio_irq);
> + mask = __gpio_mask(data->irq - ctlr->gpio_irq);
>
> if (trigger & ~(IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_EDGE_RISING))
> return -EINVAL;
> @@ -339,23 +360,33 @@ static int gpio_irq_type_unbanked(struct irq_data *data, unsigned trigger)
> * (dm6446) can be set appropriately for GPIOV33 pins.
> */
>
> -static int __init davinci_gpio_irq_setup(void)
> +static int davinci_gpio_irq_setup(struct platform_device *pdev)
> {
> - u32 binten = 0;
> - unsigned gpio, irq, bank, ngpio, bank_irq;
> - struct clk *clk;
> + u32 binten = 0;
> + unsigned gpio, irq, bank, ngpio, bank_irq;
> + struct clk *clk;
> + struct davinci_gpio_controller *ctlrs = platform_get_drvdata(pdev);
> + struct davinci_gpio_platform_data *pdata;
> struct davinci_gpio_regs __iomem *regs;
> - struct davinci_soc_info *soc_info = &davinci_soc_info;
> + struct device *dev = &pdev->dev;
> + struct resource *res;
> +
> + pdata = dev->platform_data;
> + ngpio = pdata->ngpio;
> + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> + if (unlikely(!res)) {
> + dev_err(dev, "Invalid IRQ resource\n");
> + return -ENODEV;
-EBUSY again?
Thanks,
Sekhar
On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
>
>
> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> > From: KV Sujith <[email protected]>
> >
> > Modify GPIO Davinci driver to be compliant to standard platform drivers.
> > The driver did not have platform driver structure or a probe. Instead,
> > had a davinci_gpio_setup() function which is called in the pure_init
> > sequence. The function also had dependency on davinci_soc_info structure
> > of the corresponding platform. For Device Tree(DT) implementation, we
> > need to get rid of the dependency on the davinci_soc_info structure.
> > Hence as a first stage of DT conversion, we implement a probe. Future
> > commits shall modify the probe to read platform related data from DT.
> >
> > - Add platform_driver structure and driver register function for davinci
> > GPIO driver. The driver registration is made to happen in
> > postcore_initcall. This is required since machine init functions like
> > da850_lcd_hw_init() make use of GPIO.
> > - Convert the davinci_gpio_setup() to davinci_gpio_probe().
> > - Remove access of members in soc_info structure. Instead, relevant data
> > are taken from davinci_gpio_platform_data structure pointed by
> > pdev->dev.platform_data.
> > - Change clk_get() to devm_clk_get() as devm_clk_get() is a device
> > managed function and makes error handling simpler.
> > - Change pr_err to dev_err for ngpio error reporting.
> > - Arrange include files and variables in alphabetical order
> >
> > Signed-off-by: KV Sujith <[email protected]>
> > [[email protected]: Move global definition for "struct
> > davinci_gpio_controller" variable to local in probe and set it as driver
> > data.]
> > Signed-off-by: Philip Avinash <[email protected]>
> > ---
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/platform_data/gpio-davinci.h>
>
> > +#include <mach/gpio-davinci.h>
>
> This include seems unnecessary.
This include is not required.
>
> >
> > #include <asm/mach/irq.h>
>
> While at it, you can get rid of this include and use <linux/irq.h> instead?
Ok
>
> >
> > + pdata = dev->platform_data;
> > + if (!pdata) {
> > + dev_err(dev, "GPIO: No Platform Data Supplied\n");
>
> dev_err should already tell that the error is coming from davinci-gpio
> so no need to prefix GPIO: again.
Ok
>
> > + return -EINVAL;
> > + }
> > - if (WARN_ON(!gpio_base))
> > + ctlrs = devm_kzalloc(dev,
> > + ngpio * sizeof(struct davinci_gpio_controller), GFP_KERNEL);
>
> Line break alignment needs fixing.
Ok
>
> > + if (!ctlrs) {
> > + dev_err(dev, "Memory alloc failed\n");
> > return -ENOMEM;
> > + }
> > +
> > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > + if (unlikely(!res)) {
> > + dev_err(dev, "Invalid mem resource\n");
> > + return -ENODEV;
>
> -EBUSY is better if you cannot get the resource.
Ok
>
> > + }
> > +
> > + gpio_base = devm_ioremap_resource(dev, res);
> > + if (!gpio_base)
> > + return -EADDRNOTAVAIL;
>
> devm_ioremap_resource gives an error encoder pointer if it fails so
> please use that instead of masking it.
Ok
>
> >
> > for (i = 0, base = 0; base < ngpio; i++, base += 32) {
> > ctlrs[i].chip.label = "DaVinci";
> > @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> > gpiochip_add(&ctlrs[i].chip);
> > }
> >
> > - soc_info->gpio_ctlrs = ctlrs;
>
> > - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
>
> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
> else in the patchset so in effect you render the inline gpio get/set API
> useless. Looks like this initialization should be moved to platform code?
With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
Has no more dependency on soc_info->gpio_ctlrs_num.
I can merge [PATCH 08/11] ARM: davinci: start using gpiolib support to
[PATCH 03/11] gpio: davinci: Modify to platform driver
>
> > -
> > + pdata = dev->platform_data;
> > + ngpio = pdata->ngpio;
> > + res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> > + if (unlikely(!res)) {
> > + dev_err(dev, "Invalid IRQ resource\n");
> > + return -ENODEV;
>
> -EBUSY again?
Ok
Thanks
Avinash
>
> Thanks,
> Sekhar
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On Tue, Jun 11, 2013 at 16:06:18, Nori, Sekhar wrote:
> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> > From: KV Sujith <[email protected]>
> >
> > Add struct davinci_gpio_platform_data davinci gpio module.
> >
> > Signed-off-by: KV Sujith <[email protected]>
> > Signed-off-by: Philip Avinash <[email protected]>
>
> As Linus commented before, this should be merged with 03/11.
Ok
>
> > ---
> > include/linux/platform_data/gpio-davinci.h | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> > create mode 100644 include/linux/platform_data/gpio-davinci.h
> >
> > diff --git a/include/linux/platform_data/gpio-davinci.h b/include/linux/platform_data/gpio-davinci.h
> > new file mode 100644
> > index 0000000..f1c8277
> > --- /dev/null
> > +++ b/include/linux/platform_data/gpio-davinci.h
> > @@ -0,0 +1,27 @@
> > +/*
> > + * gpio-davinci.h
>
> I would drop this unnecessary filename mention as well.
Ok
>
> > + *
> > + * DaVinci GPIO Platform Related Defines
> > + *
> > + * Copyright (C) 2013 Texas Instruments Incorporated - http://www.ti.com/
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of the GNU General Public License as
> > + * published by the Free Software Foundation version 2.
> > + *
> > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any
> > + * kind, whether express or implied; without even the implied warranty
> > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#ifndef __ASM_ARCH_DAVINCI_GPIO_H
> > +#define __ASM_ARCH_DAVINCI_GPIO_H
>
> This should now be __PLATFORM_DATA_DAVINCI_GPIO_H__ or some such since
> the file as been moved out of arch specific folder.
As Sergei pointed out, macro name will change to __GPIO_DAVINCI_H in next revision.
Thanks
Avinash
>
> Thanks,
> Sekhar
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 6/11/2013 6:25 PM, Philip, Avinash wrote:
> On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
>> On 5/22/2013 12:40 PM, Philip Avinash wrote:
>>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
>>> gpiochip_add(&ctlrs[i].chip);
>>> }
>>>
>>> - soc_info->gpio_ctlrs = ctlrs;
>>
>>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
>>
>> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
>> else in the patchset so in effect you render the inline gpio get/set API
>> useless. Looks like this initialization should be moved to platform code?
>
> With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
> Has no more dependency on soc_info->gpio_ctlrs_num.
With this series, you have removed support for inline gpio get/set API.
I see that the inline functions are still available for use on
tnetv107x. I wonder why it is important to keep these for tnetv107x when
not necessary for other DaVinci devices?
When you are removing this feature, please note it prominently in your
cover letter and patch description. Also, please provide some data on
how the latency now compares to that of inline access earlier. This is
important especially for the read. For the writes, gpio clock will
mostly determine how soon the value changes on the pin.
I am okay with removing the inline access feature. It helps simplify
code and most arm machines don't use them. I would just like to see some
data for justification as this can be seen as feature regression. Also,
if we are removing it, its better to also remove it completely and get
the LOC savings instead of just stopping its usage.
Thanks,
Sekhar
On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote:
> On 6/11/2013 6:25 PM, Philip, Avinash wrote:
>
> > On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
>
> >> On 5/22/2013 12:40 PM, Philip Avinash wrote:
>
> >>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> >>> gpiochip_add(&ctlrs[i].chip);
> >>> }
> >>>
> >>> - soc_info->gpio_ctlrs = ctlrs;
> >>
> >>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
> >>
> >> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
> >> else in the patchset so in effect you render the inline gpio get/set API
> >> useless. Looks like this initialization should be moved to platform code?
> >
> > With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
> > Has no more dependency on soc_info->gpio_ctlrs_num.
>
> With this series, you have removed support for inline gpio get/set API.
> I see that the inline functions are still available for use on
> tnetv107x. I wonder why it is important to keep these for tnetv107x when
> not necessary for other DaVinci devices?
To support DT boot in da850, gpio davinci has to be converted to a driver and
remove references to davinci_soc_info from driver. But tnetv107x has
separate GPIO driver and reference to davinci_soc_info can also be removed.
But I didn't found defconfig support for tnetv107x platforms and left untouched.
As I will not be able to build and test on tnetv107x, I prefer to not touch it.
>
> When you are removing this feature, please note it prominently in your
> cover letter and patch description.
Ok
> Also, please provide some data on
> how the latency now compares to that of inline access earlier. This is
> important especially for the read.
I am not sure whether I understood correctly or not? Meanwhile I had done
an experiment by reading printk timing before and after gpio_get_value from
a test module. I think this will help in software latency for inline access over
gpiolib specific access.
gpio_get_value latency testing code
+
+ local_irq_disable();
+ pr_emerg("%d %x\n", __LINE__, jiffies);
+ gpio_get_value(gpio_num);
+ pr_emerg("%d %x\n", __LINE__, jiffies);
+ local_irq_enable();
inline gpio functions with interrupt disabled
[ 29.734337] 81 ffff966c
[ 29.736847] 83 ffff966c
Time diff = 0.00251
gpio library with interrupt disabled
[ 272.876763] 81 fffff567
[ 272.879291] 83 fffff567
Time diff = 0.002528
Inline function takes less time as expected.
> For the writes, gpio clock will
> mostly determine how soon the value changes on the pin.
>
> I am okay with removing the inline access feature. It helps simplify
> code and most arm machines don't use them. I would just like to see some
> data for justification as this can be seen as feature regression. Also,
> if we are removing it, its better to also remove it completely and get
> the LOC savings instead of just stopping its usage.
I see build failure with below patch for tnetv107x
[v6,6/6] Davinci: tnetv107x default configuration
So I prefer to leave tnetv107x platform for now.
Thanks
Avinash
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 6/12/2013 5:40 PM, Philip, Avinash wrote:
> On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote:
>> On 6/11/2013 6:25 PM, Philip, Avinash wrote:
>>
>>> On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
>>
>>>> On 5/22/2013 12:40 PM, Philip Avinash wrote:
>>
>>>>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
>>>>> gpiochip_add(&ctlrs[i].chip);
>>>>> }
>>>>>
>>>>> - soc_info->gpio_ctlrs = ctlrs;
>>>>
>>>>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
>>>>
>>>> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
>>>> else in the patchset so in effect you render the inline gpio get/set API
>>>> useless. Looks like this initialization should be moved to platform code?
>>>
>>> With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
>>> Has no more dependency on soc_info->gpio_ctlrs_num.
>>
>> With this series, you have removed support for inline gpio get/set API.
>> I see that the inline functions are still available for use on
>> tnetv107x. I wonder why it is important to keep these for tnetv107x when
>> not necessary for other DaVinci devices?
>
> To support DT boot in da850, gpio davinci has to be converted to a driver and
> remove references to davinci_soc_info from driver. But tnetv107x has
> separate GPIO driver and reference to davinci_soc_info can also be removed.
> But I didn't found defconfig support for tnetv107x platforms and left untouched.
> As I will not be able to build and test on tnetv107x, I prefer to not touch it.
You can always build it by enabling it in menuconfig. Its an ARMv6
platform so you will have to disable other ARMv5 based platforms from
while enabling it. ARMv5 and ARMv6 cannot co-exist in the same image.
>
>>
>> When you are removing this feature, please note it prominently in your
>> cover letter and patch description.
>
> Ok
>
>> Also, please provide some data on
>> how the latency now compares to that of inline access earlier. This is
>> important especially for the read.
>
> I am not sure whether I understood correctly or not? Meanwhile I had done
> an experiment by reading printk timing before and after gpio_get_value from
> a test module. I think this will help in software latency for inline access over
> gpiolib specific access.
>
> gpio_get_value latency testing code
>
> +
> + local_irq_disable();
> + pr_emerg("%d %x\n", __LINE__, jiffies);
> + gpio_get_value(gpio_num);
> + pr_emerg("%d %x\n", __LINE__, jiffies);
> + local_irq_enable();
>
> inline gpio functions with interrupt disabled
> [ 29.734337] 81 ffff966c
> [ 29.736847] 83 ffff966c
>
> Time diff = 0.00251
>
> gpio library with interrupt disabled
>
> [ 272.876763] 81 fffff567
> [ 272.879291] 83 fffff567
>
> Time diff = 0.002528
>
> Inline function takes less time as expected.
Okay, please note these experiments in your cover letter. Its an 18usec
difference. I have no reference to say if that will affect any
application, but it will at least serve as information for someone who
may get affected by it.
>
>> For the writes, gpio clock will
>> mostly determine how soon the value changes on the pin.
>>
>> I am okay with removing the inline access feature. It helps simplify
>> code and most arm machines don't use them. I would just like to see some
>> data for justification as this can be seen as feature regression. Also,
>> if we are removing it, its better to also remove it completely and get
>> the LOC savings instead of just stopping its usage.
>
> I see build failure with below patch for tnetv107x
> [v6,6/6] Davinci: tnetv107x default configuration
Where is this patch? What is the commit-id if it is in mainline? Where
is the failure log?
>
> So I prefer to leave tnetv107x platform for now.
I don't think that's acceptable. At least by me.
Thanks,
Sekhar
On Thu, Jun 13, 2013 at 11:47:52, Nori, Sekhar wrote:
> On 6/12/2013 5:40 PM, Philip, Avinash wrote:
> > On Wed, Jun 12, 2013 at 13:13:59, Nori, Sekhar wrote:
> >> On 6/11/2013 6:25 PM, Philip, Avinash wrote:
> >>
> >>> On Tue, Jun 11, 2013 at 17:26:06, Nori, Sekhar wrote:
> >>
> >>>> On 5/22/2013 12:40 PM, Philip Avinash wrote:
> >>
> >>>>> @@ -179,13 +204,10 @@ static int __init davinci_gpio_setup(void)
> >>>>> gpiochip_add(&ctlrs[i].chip);
> >>>>> }
> >>>>>
> >>>>> - soc_info->gpio_ctlrs = ctlrs;
> >>>>
> >>>>> - soc_info->gpio_ctlrs_num = DIV_ROUND_UP(ngpio, 32);
> >>>>
> >>>> You drop setting gpio_ctlrs_num here and don't introduce it anywhere
> >>>> else in the patchset so in effect you render the inline gpio get/set API
> >>>> useless. Looks like this initialization should be moved to platform code?
> >>>
> >>> With [PATCH 08/11] ARM: davinci: start using gpiolib support gpio get/set API
> >>> Has no more dependency on soc_info->gpio_ctlrs_num.
> >>
> >> With this series, you have removed support for inline gpio get/set API.
> >> I see that the inline functions are still available for use on
> >> tnetv107x. I wonder why it is important to keep these for tnetv107x when
> >> not necessary for other DaVinci devices?
> >
> > To support DT boot in da850, gpio davinci has to be converted to a driver and
> > remove references to davinci_soc_info from driver. But tnetv107x has
> > separate GPIO driver and reference to davinci_soc_info can also be removed.
> > But I didn't found defconfig support for tnetv107x platforms and left untouched.
> > As I will not be able to build and test on tnetv107x, I prefer to not touch it.
>
> You can always build it by enabling it in menuconfig. Its an ARMv6
> platform so you will have to disable other ARMv5 based platforms from
> while enabling it. ARMv5 and ARMv6 cannot co-exist in the same image.
I will try and update.
>
> >
> >>
> >> When you are removing this feature, please note it prominently in your
> >> cover letter and patch description.
> >
> > Ok
> >
> >> Also, please provide some data on
> >> how the latency now compares to that of inline access earlier. This is
> >> important especially for the read.
> >
> > I am not sure whether I understood correctly or not? Meanwhile I had done
> > an experiment by reading printk timing before and after gpio_get_value from
> > a test module. I think this will help in software latency for inline access over
> > gpiolib specific access.
> >
> > gpio_get_value latency testing code
> >
> > +
> > + local_irq_disable();
> > + pr_emerg("%d %x\n", __LINE__, jiffies);
> > + gpio_get_value(gpio_num);
> > + pr_emerg("%d %x\n", __LINE__, jiffies);
> > + local_irq_enable();
> >
> > inline gpio functions with interrupt disabled
> > [ 29.734337] 81 ffff966c
> > [ 29.736847] 83 ffff966c
> >
> > Time diff = 0.00251
> >
> > gpio library with interrupt disabled
> >
> > [ 272.876763] 81 fffff567
> > [ 272.879291] 83 fffff567
> >
> > Time diff = 0.002528
> >
> > Inline function takes less time as expected.
>
> Okay, please note these experiments in your cover letter. Its an 18usec
> difference. I have no reference to say if that will affect any
> application, but it will at least serve as information for someone who
> may get affected by it.
Ok I will give above details in commit message.
>
> >
> >> For the writes, gpio clock will
> >> mostly determine how soon the value changes on the pin.
> >>
> >> I am okay with removing the inline access feature. It helps simplify
> >> code and most arm machines don't use them. I would just like to see some
> >> data for justification as this can be seen as feature regression. Also,
> >> if we are removing it, its better to also remove it completely and get
> >> the LOC savings instead of just stopping its usage.
> >
> > I see build failure with below patch for tnetv107x
> > [v6,6/6] Davinci: tnetv107x default configuration
>
> Where is this patch?
This patch is not in mainline and got it from patchwork
https://patchwork.kernel.org/patch/97853/
> What is the commit-id if it is in mainline? Where
> is the failure log?
With tnetv107x_defconfig build is failing
arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error:
'davinci_timer_init' undeclared here (not in a function)
arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error:
'davinci_init_late' undeclared here (not in a function)
make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1
Following patch fixes the build above breakage
diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c
index ba79837..4a9c320 100644
--- a/arch/arm/mach-davinci/board-tnetv107x-evm.c
+++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c
@@ -30,6 +30,7 @@
#include <asm/mach/arch.h>
#include <asm/mach-types.h>
+#include <mach/common.h>
#include <mach/irqs.h>
#include <mach/edma.h>
#include <mach/mux.h>
@@ -147,7 +148,7 @@ static struct davinci_nand_pdata nand_config = {
.ecc_bits = 1,
};
-static struct davinci_uart_config serial_config __initconst = {
+static struct davinci_uart_config serial_config = {
.enabled_uarts = BIT(1),
};
@@ -245,7 +246,7 @@ static struct ti_ssp_data ssp_config = {
},
};
-static struct tnetv107x_device_info evm_device_info __initconst = {
+static struct tnetv107x_device_info evm_device_info = {
.serial_config = &serial_config,
.mmc_config[1] = &mmc_config, /* controller 1 */
.nand_config[0] = &nand_config, /* chip select 0 */
>
> >
> > So I prefer to leave tnetv107x platform for now.
>
> I don't think that's acceptable. At least by me.
I think 2 options are available
1. Convert gpio-tnetv107x.c to platform driver. This will help in
removing gpio references in davinci_soc_info structure.
2. Remove inline gpio api support and start use gpiolib support.
I prefer first option. It will help in removing
<arch/arm/mach-davinci/include/mach/gpio-davinci.h>.
Thanks
Avinash
>
> Thanks,
> Sekhar
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 6/13/2013 1:02 PM, Philip, Avinash wrote:
> With tnetv107x_defconfig build is failing
>
> arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error:
> 'davinci_timer_init' undeclared here (not in a function)
> arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error:
> 'davinci_init_late' undeclared here (not in a function)
> make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1
>
> Following patch fixes the build above breakage
The error you are seeing and the patch you provided below have no
correlation.
>
> diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> index ba79837..4a9c320 100644
> --- a/arch/arm/mach-davinci/board-tnetv107x-evm.c
> +++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> @@ -30,6 +30,7 @@
> #include <asm/mach/arch.h>
> #include <asm/mach-types.h>
>
> +#include <mach/common.h>
> #include <mach/irqs.h>
> #include <mach/edma.h>
> #include <mach/mux.h>
> @@ -147,7 +148,7 @@ static struct davinci_nand_pdata nand_config = {
> .ecc_bits = 1,
> };
>
> -static struct davinci_uart_config serial_config __initconst = {
> +static struct davinci_uart_config serial_config = {
> .enabled_uarts = BIT(1),
> };
You can make this __initdata instead - assuming its okay to have this
memory discarded at init.
>
> @@ -245,7 +246,7 @@ static struct ti_ssp_data ssp_config = {
> },
> };
>
> -static struct tnetv107x_device_info evm_device_info __initconst = {
> +static struct tnetv107x_device_info evm_device_info = {
Same here. You can make this __initdata.
Please send a formal patch for the errors you have seen.
> .serial_config = &serial_config,
> .mmc_config[1] = &mmc_config, /* controller 1 */
> .nand_config[0] = &nand_config, /* chip select 0 */
>
>
>
>>
>>>
>>> So I prefer to leave tnetv107x platform for now.
>>
>> I don't think that's acceptable. At least by me.
>
> I think 2 options are available
> 1. Convert gpio-tnetv107x.c to platform driver. This will help in
> removing gpio references in davinci_soc_info structure.
> 2. Remove inline gpio api support and start use gpiolib support.
>
> I prefer first option. It will help in removing
> <arch/arm/mach-davinci/include/mach/gpio-davinci.h>.
Okay. Can you take this up in this series? I understand you may not have
an tnetv107x board, but at least you can get to a series that builds.
Even if you choose to do just option #2, I am OK. What I really want to
see is inline API gone completely (not just remain largely unused). This
will also help you avoid exposing internal data structures like
davinci_gpio_controller exposed to the whole kernel. The worse part
right now is you have two copies of the same structure exposed globally
from two different include folders.
Thanks,
Sekhar
On Thu, Jun 13, 2013 at 13:59:53, Nori, Sekhar wrote:
> On 6/13/2013 1:02 PM, Philip, Avinash wrote:
>
> > With tnetv107x_defconfig build is failing
> >
> > arch/arm/mach-davinci/board-tnetv107x-evm.c:282:15: error:
> > 'davinci_timer_init' undeclared here (not in a function)
> > arch/arm/mach-davinci/board-tnetv107x-evm.c:284:15: error:
> > 'davinci_init_late' undeclared here (not in a function)
> > make[1]: *** [arch/arm/mach-davinci/board-tnetv107x-evm.o] Error 1
> >
> > Following patch fixes the build above breakage
>
> The error you are seeing and the patch you provided below have no
> correlation.
No. Above build error fixed by
+#include <mach/common.h>
Other changes are not related to above error.
>
> >
> > diff --git a/arch/arm/mach-davinci/board-tnetv107x-evm.c b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> > index ba79837..4a9c320 100644
> > --- a/arch/arm/mach-davinci/board-tnetv107x-evm.c
> > +++ b/arch/arm/mach-davinci/board-tnetv107x-evm.c
> > @@ -30,6 +30,7 @@
> > #include <asm/mach/arch.h>
> > #include <asm/mach-types.h>
> >
> > +#include <mach/common.h>
> > #include <mach/irqs.h>
> > #include <mach/edma.h>
> > #include <mach/mux.h>
> > @@ -147,7 +148,7 @@ static struct davinci_nand_pdata nand_config = {
> > .ecc_bits = 1,
> > };
> >
> > -static struct davinci_uart_config serial_config __initconst = {
> > +static struct davinci_uart_config serial_config = {
> > .enabled_uarts = BIT(1),
> > };
>
> You can make this __initdata instead - assuming its okay to have this
> memory discarded at init.
I will check.
>
> >
> > @@ -245,7 +246,7 @@ static struct ti_ssp_data ssp_config = {
> > },
> > };
> >
> > -static struct tnetv107x_device_info evm_device_info __initconst = {
> > +static struct tnetv107x_device_info evm_device_info = {
>
> Same here. You can make this __initdata.
>
> Please send a formal patch for the errors you have seen.
Ok
>
> > .serial_config = &serial_config,
> > .mmc_config[1] = &mmc_config, /* controller 1 */
> > .nand_config[0] = &nand_config, /* chip select 0 */
> >
> >
> >
> >>
> >>>
> >>> So I prefer to leave tnetv107x platform for now.
> >>
> >> I don't think that's acceptable. At least by me.
> >
> > I think 2 options are available
> > 1. Convert gpio-tnetv107x.c to platform driver. This will help in
> > removing gpio references in davinci_soc_info structure.
> > 2. Remove inline gpio api support and start use gpiolib support.
> >
> > I prefer first option. It will help in removing
> > <arch/arm/mach-davinci/include/mach/gpio-davinci.h>.
>
> Okay. Can you take this up in this series? I understand you may not have
> an tnetv107x board, but at least you can get to a series that builds.
>
> Even if you choose to do just option #2, I am OK. What I really want to
> see is inline API gone completely (not just remain largely unused). This
> will also help you avoid exposing internal data structures like
> davinci_gpio_controller exposed to the whole kernel. The worse part
> right now is you have two copies of the same structure exposed globally
> from two different include folders.
I understood. I will take option 2.
Thanks
Avinash
>
> Thanks,
> Sekhar
>
????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?