* [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-11 9:41 ` Magnus Damm
0 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-11 9:41 UTC (permalink / raw)
To: linux-kernel
Cc: rjw, linus.walleij, arnd, linux-sh, horms, grant.likely, lethal,
olof, Magnus Damm
From: Magnus Damm <damm@opensource.se>
This patch adds a GPIO driver for the Emma Mobile line
of SoCs. The driver is designed to be reusable between
multiple SoCs that share the same basic building block,
but so far it has only been used on Emma Mobile EV2.
Each driver instance handles 32 GPIOs with individually
maskable IRQs. The driver operates on two I/O memory
ranges and the 32 GPIOs are hooked up to two interrupts.
In the case of Emma Mobile EV2 this GPIO building block
is used as main external interrupt controller hooking up
159 GPIOS as 159 interrupts via 5 driver instances and
10 interrupts to the GIC and the Cortex-A9 Dual.
This version of the driver gets all configuration via
struct resource and platform data, but DT bindings are
currently in development and such support will be sent
as a individual feature patch in the near future.
Signed-off-by: Magnus Damm <damm@opensource.se>
---
Built on top of linux-next 20120511. There are no known
merge or runtime dependencies on queued up patches.
drivers/gpio/Kconfig | 6
drivers/gpio/Makefile | 1
drivers/gpio/gpio-em.c | 385 +++++++++++++++++++++++++++++++++
include/linux/platform_data/gpio-em.h | 10
4 files changed, 402 insertions(+)
--- 0001/drivers/gpio/Kconfig
+++ work/drivers/gpio/Kconfig 2012-05-11 18:04:53.000000000 +0900
@@ -95,6 +95,12 @@ config GPIO_IT8761E
help
Say yes here to support GPIO functionality of IT8761E super I/O chip.
+config GPIO_EM
+ tristate "Emma Mobile GPIO"
+ depends on ARM
+ help
+ Say yes here to support GPIO on Renesas Emma Mobile SoCs.
+
config GPIO_EP93XX
def_bool y
depends on ARCH_EP93XX
--- 0001/drivers/gpio/Makefile
+++ work/drivers/gpio/Makefile 2012-05-11 18:04:53.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_ARCH_DAVINCI) += gpio-davinci.o
+obj-$(CONFIG_GPIO_EM) += gpio-em.o
obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
--- /dev/null
+++ work/drivers/gpio/gpio-em.c 2012-05-11 18:06:47.000000000 +0900
@@ -0,0 +1,385 @@
+/*
+ * Emma Mobile GPIO Support - GIO
+ *
+ * Copyright (C) 2012 Magnus Damm
+ *
+ * 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; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_data/gpio-em.h>
+
+struct em_gio_priv {
+ void __iomem *base0;
+ void __iomem *base1;
+ unsigned int irq_base;
+ spinlock_t sense_lock;
+ struct gpio_chip gpio_chip;
+ struct irq_chip irq_chip;
+};
+
+#define GIO_E1 0x00
+#define GIO_E0 0x04
+#define GIO_EM 0x04
+#define GIO_OL 0x08
+#define GIO_OH 0x0c
+#define GIO_I 0x10
+#define GIO_IIA 0x14
+#define GIO_IEN 0x18
+#define GIO_IDS 0x1c
+#define GIO_IIM 0x1c
+#define GIO_RAW 0x20
+#define GIO_MST 0x24
+#define GIO_IIR 0x28
+
+#define GIO_IDT0 0x40
+#define GIO_IDT1 0x44
+#define GIO_IDT2 0x48
+#define GIO_IDT3 0x4c
+#define GIO_RAWBL 0x50
+#define GIO_RAWBH 0x54
+#define GIO_IRBL 0x58
+#define GIO_IRBH 0x5c
+
+#define GIO_IDT(n) GIO_IDT0 + ((n) * 4)
+
+static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
+{
+ if (offs < GIO_IDT0)
+ return ioread32(p->base0 + offs);
+ else
+ return ioread32(p->base1 + (offs - GIO_IDT0));
+}
+
+static inline void em_gio_write(struct em_gio_priv *p, int offs,
+ unsigned long value)
+{
+ if (offs < GIO_IDT0)
+ iowrite32(value, p->base0 + offs);
+ else
+ iowrite32(value, p->base1 + (offs - GIO_IDT0));
+}
+
+static struct em_gio_priv *irq_to_priv(unsigned int irq)
+{
+ struct irq_chip *chip = irq_get_chip(irq);
+ return container_of(chip, struct em_gio_priv, irq_chip);
+}
+
+static void em_gio_irq_disable(struct irq_data *data)
+{
+ struct em_gio_priv *p = irq_to_priv(data->irq);
+ int offset = data->irq - p->irq_base;
+
+ em_gio_write(p, GIO_IDS, 1 << offset);
+}
+
+static void em_gio_irq_enable(struct irq_data *data)
+{
+ struct em_gio_priv *p = irq_to_priv(data->irq);
+ int offset = data->irq - p->irq_base;
+
+ em_gio_write(p, GIO_IEN, 1 << offset);
+}
+
+#define GIO_ASYNC(x) (x + 8)
+
+static unsigned char em_gio_sense_table[IRQ_TYPE_SENSE_MASK + 1] = {
+ [IRQ_TYPE_EDGE_RISING] = GIO_ASYNC(0x00),
+ [IRQ_TYPE_EDGE_FALLING] = GIO_ASYNC(0x01),
+ [IRQ_TYPE_LEVEL_HIGH] = GIO_ASYNC(0x02),
+ [IRQ_TYPE_LEVEL_LOW] = GIO_ASYNC(0x03),
+ [IRQ_TYPE_EDGE_BOTH] = GIO_ASYNC(0x04),
+};
+
+static int em_gio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ unsigned char value = em_gio_sense_table[type & IRQ_TYPE_SENSE_MASK];
+ struct em_gio_priv *p = irq_to_priv(data->irq);
+ unsigned int reg, offset, shift;
+ unsigned long flags;
+ unsigned long tmp;
+
+ if (!value)
+ return -EINVAL;
+
+ offset = data->irq - p->irq_base;
+
+ pr_debug("gio: sense irq = %d, irq = %d, mode = %d, sw base = %d\n",
+ offset, data->irq, value, p->irq_base);
+
+ /* 8 x 4 bit fields in 4 IDT registers */
+ reg = GIO_IDT(offset >> 3);
+ shift = (offset & 0x07) << 4;
+
+ spin_lock_irqsave(&p->sense_lock, flags);
+
+ /* disable the interrupt in IIA */
+ tmp = em_gio_read(p, GIO_IIA);
+ tmp &= ~(1 << offset);
+ em_gio_write(p, GIO_IIA, tmp);
+
+ /* change the sense setting in IDT */
+ tmp = em_gio_read(p, reg);
+ tmp &= ~(0xf << shift);
+ tmp |= value << shift;
+ em_gio_write(p, reg, tmp);
+
+ /* clear pending interrupts */
+ em_gio_write(p, GIO_IIR, 1 << offset);
+
+ /* enable the interrupt in IIA */
+ tmp = em_gio_read(p, GIO_IIA);
+ tmp |= 1 << offset;
+ em_gio_write(p, GIO_IIA, tmp);
+
+ spin_unlock_irqrestore(&p->sense_lock, flags);
+
+ return 0;
+}
+
+static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
+{
+ struct em_gio_priv *p = dev_id;
+ unsigned long tmp, status;
+ unsigned int offset;
+
+ status = tmp = em_gio_read(p, GIO_MST);
+ if (!status)
+ return IRQ_NONE;
+
+ while (status) {
+ offset = __ffs(status);
+ generic_handle_irq(p->irq_base + offset);
+ status &= ~(1 << offset);
+ }
+
+ em_gio_write(p, GIO_IIR, tmp);
+
+ return IRQ_HANDLED;
+}
+
+static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+ return container_of(chip, struct em_gio_priv, gpio_chip);
+}
+
+static int em_gio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ em_gio_write(gpio_to_priv(chip), GIO_E0, 1 << offset);
+ return 0;
+}
+
+static int em_gio_get(struct gpio_chip *chip, unsigned offset)
+{
+ return (int)(em_gio_read(gpio_to_priv(chip), GIO_I) & (1 << offset));
+}
+
+static void __em_gio_set(struct gpio_chip *chip, unsigned int reg,
+ unsigned shift, int value)
+{
+ /* upper 16 bits contains mask and lower 16 actual value */
+ em_gio_write(gpio_to_priv(chip), reg,
+ (1 << (shift + 16)) | (value << shift));
+}
+
+static void em_gio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ /* output is split into two registers */
+ if (offset < 16)
+ __em_gio_set(chip, GIO_OL, offset, value);
+ else
+ __em_gio_set(chip, GIO_OH, offset - 16, value);
+}
+
+static int em_gio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ /* write GPIO value to output before selecting output mode of pin */
+ em_gio_set(chip, offset, value);
+ em_gio_write(gpio_to_priv(chip), GIO_E1, 1 << offset);
+ return 0;
+}
+
+static int em_gio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ return gpio_to_priv(chip)->irq_base + offset;
+}
+
+static int __devinit em_gio_probe(struct platform_device *pdev)
+{
+ struct gpio_em_config *pdata = pdev->dev.platform_data;
+ struct em_gio_priv *p;
+ struct resource *io[2], *irq[2];
+ struct gpio_chip *gpio_chip;
+ struct irq_chip *irq_chip;
+ const char *name = dev_name(&pdev->dev);
+ int k, ret;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ dev_err(&pdev->dev, "failed to allocate driver data\n");
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ gpio_chip = &p->gpio_chip;
+ irq_chip = &p->irq_chip;
+ platform_set_drvdata(pdev, p);
+ spin_lock_init(&p->sense_lock);
+
+ io[0] = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ io[1] = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ irq[0] = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+ if (!io[0] || !io[1] || !irq[0] || !irq[1] || !pdata) {
+ dev_err(&pdev->dev, "missing IRQ, IOMEM or configuration\n");
+ ret = -EINVAL;
+ goto err1;
+ }
+
+ p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
+ if (!p->base0) {
+ dev_err(&pdev->dev, "failed to remap low I/O memory\n");
+ ret = -ENXIO;
+ goto err1;
+ }
+
+ p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
+ if (!p->base1) {
+ dev_err(&pdev->dev, "failed to remap high I/O memory\n");
+ ret = -ENXIO;
+ goto err2;
+ }
+
+ p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
+ pdata->number_of_pins, numa_node_id());
+ if (IS_ERR_VALUE(p->irq_base)) {
+ dev_err(&pdev->dev, "cannot get irq_desc\n");
+ ret = -ENXIO;
+ goto err3;
+ }
+
+ pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
+ pdata->gpio_base, pdata->number_of_pins, p->irq_base);
+
+ irq_chip->name = name;
+ irq_chip->irq_mask = em_gio_irq_disable;
+ irq_chip->irq_unmask = em_gio_irq_enable;
+ irq_chip->irq_enable = em_gio_irq_enable;
+ irq_chip->irq_disable = em_gio_irq_disable;
+ irq_chip->irq_set_type = em_gio_irq_set_type;
+ irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
+
+ for (k = p->irq_base; k < (p->irq_base + pdata->number_of_pins); k++) {
+ irq_set_chip_and_handler_name(k, irq_chip, handle_level_irq,
+ "level");
+ set_irq_flags(k, IRQF_VALID); /* kill me now */
+ }
+
+ if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
+ dev_err(&pdev->dev, "failed to request low IRQ\n");
+ ret = -ENOENT;
+ goto err4;
+ }
+
+ if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
+ dev_err(&pdev->dev, "failed to request high IRQ\n");
+ ret = -ENOENT;
+ goto err5;
+ }
+
+ gpio_chip->direction_input = em_gio_direction_input;
+ gpio_chip->get = em_gio_get;
+ gpio_chip->direction_output = em_gio_direction_output;
+ gpio_chip->set = em_gio_set;
+
+ gpio_chip->to_irq = em_gio_to_irq;
+
+ gpio_chip->label = name;
+ gpio_chip->owner = THIS_MODULE;
+
+ gpio_chip->base = pdata->gpio_base;
+ gpio_chip->ngpio = pdata->number_of_pins;
+
+ ret = gpiochip_add(gpio_chip);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to add GPIO cntroller\n");
+ goto err6;
+ }
+ return 0;
+
+err6:
+ free_irq(irq[1]->start, pdev);
+err5:
+ free_irq(irq[0]->start, pdev);
+err4:
+ irq_free_descs(p->irq_base, pdata->number_of_pins);
+err3:
+ iounmap(p->base1);
+err2:
+ iounmap(p->base0);
+err1:
+ kfree(p);
+err0:
+ return ret;
+}
+
+static int __devexit em_gio_remove(struct platform_device *pdev)
+{
+ struct gpio_em_config *pdata = pdev->dev.platform_data;
+ struct em_gio_priv *p = platform_get_drvdata(pdev);
+ struct resource *irq[2];
+ int ret;
+
+ ret = gpiochip_remove(&p->gpio_chip);
+ if (ret)
+ return ret;
+
+ irq[0] = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+ free_irq(irq[1]->start, pdev);
+ free_irq(irq[0]->start, pdev);
+ irq_free_descs(p->irq_base, pdata->number_of_pins);
+ iounmap(p->base1);
+ iounmap(p->base0);
+ kfree(p);
+ return 0;
+}
+
+static struct platform_driver em_gio_device_driver = {
+ .probe = em_gio_probe,
+ .remove = __devexit_p(em_gio_remove),
+ .driver = {
+ .name = "em_gio",
+ }
+};
+
+module_platform_driver(em_gio_device_driver);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Renesas Emma Mobile GIO Driver");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/linux/platform_data/gpio-em.h 2012-05-11 18:04:54.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __GPIO_EM_H__
+#define __GPIO_EM_H__
+
+struct gpio_em_config {
+ unsigned int gpio_base;
+ unsigned int irq_base;
+ unsigned int number_of_pins;
+};
+
+#endif /* __GPIO_EM_H__ */
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-11 9:41 ` Magnus Damm
0 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-11 9:41 UTC (permalink / raw)
To: linux-kernel
Cc: rjw, linus.walleij, arnd, linux-sh, horms, grant.likely, lethal,
olof, Magnus Damm
From: Magnus Damm <damm@opensource.se>
This patch adds a GPIO driver for the Emma Mobile line
of SoCs. The driver is designed to be reusable between
multiple SoCs that share the same basic building block,
but so far it has only been used on Emma Mobile EV2.
Each driver instance handles 32 GPIOs with individually
maskable IRQs. The driver operates on two I/O memory
ranges and the 32 GPIOs are hooked up to two interrupts.
In the case of Emma Mobile EV2 this GPIO building block
is used as main external interrupt controller hooking up
159 GPIOS as 159 interrupts via 5 driver instances and
10 interrupts to the GIC and the Cortex-A9 Dual.
This version of the driver gets all configuration via
struct resource and platform data, but DT bindings are
currently in development and such support will be sent
as a individual feature patch in the near future.
Signed-off-by: Magnus Damm <damm@opensource.se>
---
Built on top of linux-next 20120511. There are no known
merge or runtime dependencies on queued up patches.
drivers/gpio/Kconfig | 6
drivers/gpio/Makefile | 1
drivers/gpio/gpio-em.c | 385 +++++++++++++++++++++++++++++++++
include/linux/platform_data/gpio-em.h | 10
4 files changed, 402 insertions(+)
--- 0001/drivers/gpio/Kconfig
+++ work/drivers/gpio/Kconfig 2012-05-11 18:04:53.000000000 +0900
@@ -95,6 +95,12 @@ config GPIO_IT8761E
help
Say yes here to support GPIO functionality of IT8761E super I/O chip.
+config GPIO_EM
+ tristate "Emma Mobile GPIO"
+ depends on ARM
+ help
+ Say yes here to support GPIO on Renesas Emma Mobile SoCs.
+
config GPIO_EP93XX
def_bool y
depends on ARCH_EP93XX
--- 0001/drivers/gpio/Makefile
+++ work/drivers/gpio/Makefile 2012-05-11 18:04:53.000000000 +0900
@@ -16,6 +16,7 @@ obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
obj-$(CONFIG_GPIO_DA9052) += gpio-da9052.o
obj-$(CONFIG_ARCH_DAVINCI) += gpio-davinci.o
+obj-$(CONFIG_GPIO_EM) += gpio-em.o
obj-$(CONFIG_GPIO_EP93XX) += gpio-ep93xx.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
--- /dev/null
+++ work/drivers/gpio/gpio-em.c 2012-05-11 18:06:47.000000000 +0900
@@ -0,0 +1,385 @@
+/*
+ * Emma Mobile GPIO Support - GIO
+ *
+ * Copyright (C) 2012 Magnus Damm
+ *
+ * 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; either version 2 of the License
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+
+#include <linux/init.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/ioport.h>
+#include <linux/io.h>
+#include <linux/irq.h>
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/platform_data/gpio-em.h>
+
+struct em_gio_priv {
+ void __iomem *base0;
+ void __iomem *base1;
+ unsigned int irq_base;
+ spinlock_t sense_lock;
+ struct gpio_chip gpio_chip;
+ struct irq_chip irq_chip;
+};
+
+#define GIO_E1 0x00
+#define GIO_E0 0x04
+#define GIO_EM 0x04
+#define GIO_OL 0x08
+#define GIO_OH 0x0c
+#define GIO_I 0x10
+#define GIO_IIA 0x14
+#define GIO_IEN 0x18
+#define GIO_IDS 0x1c
+#define GIO_IIM 0x1c
+#define GIO_RAW 0x20
+#define GIO_MST 0x24
+#define GIO_IIR 0x28
+
+#define GIO_IDT0 0x40
+#define GIO_IDT1 0x44
+#define GIO_IDT2 0x48
+#define GIO_IDT3 0x4c
+#define GIO_RAWBL 0x50
+#define GIO_RAWBH 0x54
+#define GIO_IRBL 0x58
+#define GIO_IRBH 0x5c
+
+#define GIO_IDT(n) GIO_IDT0 + ((n) * 4)
+
+static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
+{
+ if (offs < GIO_IDT0)
+ return ioread32(p->base0 + offs);
+ else
+ return ioread32(p->base1 + (offs - GIO_IDT0));
+}
+
+static inline void em_gio_write(struct em_gio_priv *p, int offs,
+ unsigned long value)
+{
+ if (offs < GIO_IDT0)
+ iowrite32(value, p->base0 + offs);
+ else
+ iowrite32(value, p->base1 + (offs - GIO_IDT0));
+}
+
+static struct em_gio_priv *irq_to_priv(unsigned int irq)
+{
+ struct irq_chip *chip = irq_get_chip(irq);
+ return container_of(chip, struct em_gio_priv, irq_chip);
+}
+
+static void em_gio_irq_disable(struct irq_data *data)
+{
+ struct em_gio_priv *p = irq_to_priv(data->irq);
+ int offset = data->irq - p->irq_base;
+
+ em_gio_write(p, GIO_IDS, 1 << offset);
+}
+
+static void em_gio_irq_enable(struct irq_data *data)
+{
+ struct em_gio_priv *p = irq_to_priv(data->irq);
+ int offset = data->irq - p->irq_base;
+
+ em_gio_write(p, GIO_IEN, 1 << offset);
+}
+
+#define GIO_ASYNC(x) (x + 8)
+
+static unsigned char em_gio_sense_table[IRQ_TYPE_SENSE_MASK + 1] = {
+ [IRQ_TYPE_EDGE_RISING] = GIO_ASYNC(0x00),
+ [IRQ_TYPE_EDGE_FALLING] = GIO_ASYNC(0x01),
+ [IRQ_TYPE_LEVEL_HIGH] = GIO_ASYNC(0x02),
+ [IRQ_TYPE_LEVEL_LOW] = GIO_ASYNC(0x03),
+ [IRQ_TYPE_EDGE_BOTH] = GIO_ASYNC(0x04),
+};
+
+static int em_gio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ unsigned char value = em_gio_sense_table[type & IRQ_TYPE_SENSE_MASK];
+ struct em_gio_priv *p = irq_to_priv(data->irq);
+ unsigned int reg, offset, shift;
+ unsigned long flags;
+ unsigned long tmp;
+
+ if (!value)
+ return -EINVAL;
+
+ offset = data->irq - p->irq_base;
+
+ pr_debug("gio: sense irq = %d, irq = %d, mode = %d, sw base = %d\n",
+ offset, data->irq, value, p->irq_base);
+
+ /* 8 x 4 bit fields in 4 IDT registers */
+ reg = GIO_IDT(offset >> 3);
+ shift = (offset & 0x07) << 4;
+
+ spin_lock_irqsave(&p->sense_lock, flags);
+
+ /* disable the interrupt in IIA */
+ tmp = em_gio_read(p, GIO_IIA);
+ tmp &= ~(1 << offset);
+ em_gio_write(p, GIO_IIA, tmp);
+
+ /* change the sense setting in IDT */
+ tmp = em_gio_read(p, reg);
+ tmp &= ~(0xf << shift);
+ tmp |= value << shift;
+ em_gio_write(p, reg, tmp);
+
+ /* clear pending interrupts */
+ em_gio_write(p, GIO_IIR, 1 << offset);
+
+ /* enable the interrupt in IIA */
+ tmp = em_gio_read(p, GIO_IIA);
+ tmp |= 1 << offset;
+ em_gio_write(p, GIO_IIA, tmp);
+
+ spin_unlock_irqrestore(&p->sense_lock, flags);
+
+ return 0;
+}
+
+static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
+{
+ struct em_gio_priv *p = dev_id;
+ unsigned long tmp, status;
+ unsigned int offset;
+
+ status = tmp = em_gio_read(p, GIO_MST);
+ if (!status)
+ return IRQ_NONE;
+
+ while (status) {
+ offset = __ffs(status);
+ generic_handle_irq(p->irq_base + offset);
+ status &= ~(1 << offset);
+ }
+
+ em_gio_write(p, GIO_IIR, tmp);
+
+ return IRQ_HANDLED;
+}
+
+static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
+{
+ return container_of(chip, struct em_gio_priv, gpio_chip);
+}
+
+static int em_gio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ em_gio_write(gpio_to_priv(chip), GIO_E0, 1 << offset);
+ return 0;
+}
+
+static int em_gio_get(struct gpio_chip *chip, unsigned offset)
+{
+ return (int)(em_gio_read(gpio_to_priv(chip), GIO_I) & (1 << offset));
+}
+
+static void __em_gio_set(struct gpio_chip *chip, unsigned int reg,
+ unsigned shift, int value)
+{
+ /* upper 16 bits contains mask and lower 16 actual value */
+ em_gio_write(gpio_to_priv(chip), reg,
+ (1 << (shift + 16)) | (value << shift));
+}
+
+static void em_gio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ /* output is split into two registers */
+ if (offset < 16)
+ __em_gio_set(chip, GIO_OL, offset, value);
+ else
+ __em_gio_set(chip, GIO_OH, offset - 16, value);
+}
+
+static int em_gio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ /* write GPIO value to output before selecting output mode of pin */
+ em_gio_set(chip, offset, value);
+ em_gio_write(gpio_to_priv(chip), GIO_E1, 1 << offset);
+ return 0;
+}
+
+static int em_gio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ return gpio_to_priv(chip)->irq_base + offset;
+}
+
+static int __devinit em_gio_probe(struct platform_device *pdev)
+{
+ struct gpio_em_config *pdata = pdev->dev.platform_data;
+ struct em_gio_priv *p;
+ struct resource *io[2], *irq[2];
+ struct gpio_chip *gpio_chip;
+ struct irq_chip *irq_chip;
+ const char *name = dev_name(&pdev->dev);
+ int k, ret;
+
+ p = kzalloc(sizeof(*p), GFP_KERNEL);
+ if (!p) {
+ dev_err(&pdev->dev, "failed to allocate driver data\n");
+ ret = -ENOMEM;
+ goto err0;
+ }
+
+ gpio_chip = &p->gpio_chip;
+ irq_chip = &p->irq_chip;
+ platform_set_drvdata(pdev, p);
+ spin_lock_init(&p->sense_lock);
+
+ io[0] = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ io[1] = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ irq[0] = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+ if (!io[0] || !io[1] || !irq[0] || !irq[1] || !pdata) {
+ dev_err(&pdev->dev, "missing IRQ, IOMEM or configuration\n");
+ ret = -EINVAL;
+ goto err1;
+ }
+
+ p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
+ if (!p->base0) {
+ dev_err(&pdev->dev, "failed to remap low I/O memory\n");
+ ret = -ENXIO;
+ goto err1;
+ }
+
+ p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
+ if (!p->base1) {
+ dev_err(&pdev->dev, "failed to remap high I/O memory\n");
+ ret = -ENXIO;
+ goto err2;
+ }
+
+ p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
+ pdata->number_of_pins, numa_node_id());
+ if (IS_ERR_VALUE(p->irq_base)) {
+ dev_err(&pdev->dev, "cannot get irq_desc\n");
+ ret = -ENXIO;
+ goto err3;
+ }
+
+ pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
+ pdata->gpio_base, pdata->number_of_pins, p->irq_base);
+
+ irq_chip->name = name;
+ irq_chip->irq_mask = em_gio_irq_disable;
+ irq_chip->irq_unmask = em_gio_irq_enable;
+ irq_chip->irq_enable = em_gio_irq_enable;
+ irq_chip->irq_disable = em_gio_irq_disable;
+ irq_chip->irq_set_type = em_gio_irq_set_type;
+ irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
+
+ for (k = p->irq_base; k < (p->irq_base + pdata->number_of_pins); k++) {
+ irq_set_chip_and_handler_name(k, irq_chip, handle_level_irq,
+ "level");
+ set_irq_flags(k, IRQF_VALID); /* kill me now */
+ }
+
+ if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
+ dev_err(&pdev->dev, "failed to request low IRQ\n");
+ ret = -ENOENT;
+ goto err4;
+ }
+
+ if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
+ dev_err(&pdev->dev, "failed to request high IRQ\n");
+ ret = -ENOENT;
+ goto err5;
+ }
+
+ gpio_chip->direction_input = em_gio_direction_input;
+ gpio_chip->get = em_gio_get;
+ gpio_chip->direction_output = em_gio_direction_output;
+ gpio_chip->set = em_gio_set;
+
+ gpio_chip->to_irq = em_gio_to_irq;
+
+ gpio_chip->label = name;
+ gpio_chip->owner = THIS_MODULE;
+
+ gpio_chip->base = pdata->gpio_base;
+ gpio_chip->ngpio = pdata->number_of_pins;
+
+ ret = gpiochip_add(gpio_chip);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to add GPIO cntroller\n");
+ goto err6;
+ }
+ return 0;
+
+err6:
+ free_irq(irq[1]->start, pdev);
+err5:
+ free_irq(irq[0]->start, pdev);
+err4:
+ irq_free_descs(p->irq_base, pdata->number_of_pins);
+err3:
+ iounmap(p->base1);
+err2:
+ iounmap(p->base0);
+err1:
+ kfree(p);
+err0:
+ return ret;
+}
+
+static int __devexit em_gio_remove(struct platform_device *pdev)
+{
+ struct gpio_em_config *pdata = pdev->dev.platform_data;
+ struct em_gio_priv *p = platform_get_drvdata(pdev);
+ struct resource *irq[2];
+ int ret;
+
+ ret = gpiochip_remove(&p->gpio_chip);
+ if (ret)
+ return ret;
+
+ irq[0] = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
+ irq[1] = platform_get_resource(pdev, IORESOURCE_IRQ, 1);
+
+ free_irq(irq[1]->start, pdev);
+ free_irq(irq[0]->start, pdev);
+ irq_free_descs(p->irq_base, pdata->number_of_pins);
+ iounmap(p->base1);
+ iounmap(p->base0);
+ kfree(p);
+ return 0;
+}
+
+static struct platform_driver em_gio_device_driver = {
+ .probe = em_gio_probe,
+ .remove = __devexit_p(em_gio_remove),
+ .driver = {
+ .name = "em_gio",
+ }
+};
+
+module_platform_driver(em_gio_device_driver);
+
+MODULE_AUTHOR("Magnus Damm");
+MODULE_DESCRIPTION("Renesas Emma Mobile GIO Driver");
+MODULE_LICENSE("GPL v2");
--- /dev/null
+++ work/include/linux/platform_data/gpio-em.h 2012-05-11 18:04:54.000000000 +0900
@@ -0,0 +1,10 @@
+#ifndef __GPIO_EM_H__
+#define __GPIO_EM_H__
+
+struct gpio_em_config {
+ unsigned int gpio_base;
+ unsigned int irq_base;
+ unsigned int number_of_pins;
+};
+
+#endif /* __GPIO_EM_H__ */
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-11 9:41 ` Magnus Damm
@ 2012-05-11 13:10 ` Linus Walleij
-1 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2012-05-11 13:10 UTC (permalink / raw)
To: Magnus Damm
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> +config GPIO_EM
> + tristate "Emma Mobile GPIO"
> + depends on ARM
ARM really? Why should all ARM systems have the option of configuring in an Emma
controller?
> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
> +{
> + if (offs < GIO_IDT0)
> + return ioread32(p->base0 + offs);
> + else
> + return ioread32(p->base1 + (offs - GIO_IDT0));
> +}
> +
> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
> + unsigned long value)
> +{
> + if (offs < GIO_IDT0)
> + iowrite32(value, p->base0 + offs);
> + else
> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
> +}
Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
readl_[relaxed] and writel_[relaxed]()?
> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
> +{
> + struct irq_chip *chip = irq_get_chip(irq);
> + return container_of(chip, struct em_gio_priv, irq_chip);
> +}
Should this be inline maybe?
> +
> +static void em_gio_irq_disable(struct irq_data *data)
> +{
> + struct em_gio_priv *p = irq_to_priv(data->irq);
> + int offset = data->irq - p->irq_base;
> +
> + em_gio_write(p, GIO_IDS, 1 << offset);
Suggest:
#include <linux/bitops.h>
em_gio_write(p, GIO_IDS, BIT(offset));
(No big deal, just having fun...)
> +static void em_gio_irq_enable(struct irq_data *data)
> +{
> + struct em_gio_priv *p = irq_to_priv(data->irq);
> + int offset = data->irq - p->irq_base;
> +
> + em_gio_write(p, GIO_IEN, 1 << offset);
> +}
Dito.
> + /* disable the interrupt in IIA */
> + tmp = em_gio_read(p, GIO_IIA);
> + tmp &= ~(1 << offset);
Dito.
(etc)
> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
> +{
> + struct em_gio_priv *p = dev_id;
> + unsigned long tmp, status;
> + unsigned int offset;
> +
> + status = tmp = em_gio_read(p, GIO_MST);
> + if (!status)
> + return IRQ_NONE;
> +
> + while (status) {
> + offset = __ffs(status);
> + generic_handle_irq(p->irq_base + offset);
Pleas use irqdomain to translate the IRQ numbers.
> + status &= ~(1 << offset);
> + }
We've recently patched a log of IRQ handlers to re-read the IRQ
status register on each iteration. I suspect that is needed here
as well.
while (status = em_gio_read(p, GIO_MST)) {
> +
> + em_gio_write(p, GIO_IIR, tmp);
And then I guess this would need to be moved into the
loop to clear one bit at the time instead.
> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct em_gio_priv, gpio_chip);
> +}
inline?
> +static int __devinit em_gio_probe(struct platform_device *pdev)
> +{
> + struct gpio_em_config *pdata = pdev->dev.platform_data;
> + struct em_gio_priv *p;
> + struct resource *io[2], *irq[2];
> + struct gpio_chip *gpio_chip;
> + struct irq_chip *irq_chip;
> + const char *name = dev_name(&pdev->dev);
> + int k, ret;
> +
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
Use devm_kzalloc() and friends?
Which makes the error path all much simpler.
> + p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
> + if (!p->base0) {
> + dev_err(&pdev->dev, "failed to remap low I/O memory\n");
> + ret = -ENXIO;
> + goto err1;
> + }
> +
> + p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
> + if (!p->base1) {
> + dev_err(&pdev->dev, "failed to remap high I/O memory\n");
> + ret = -ENXIO;
> + goto err2;
> + }
I usually also request the memory region request_mem_region(),
I don't know if I'm particularly picky though.
Isn't there a new nice inline that will both request and
remap a piece of memory?
> + p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
> + pdata->number_of_pins, numa_node_id());
> + if (IS_ERR_VALUE(p->irq_base)) {
> + dev_err(&pdev->dev, "cannot get irq_desc\n");
> + ret = -ENXIO;
> + goto err3;
> + }
> +
> + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
> + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
> +
> + irq_chip->name = name;
> + irq_chip->irq_mask = em_gio_irq_disable;
> + irq_chip->irq_unmask = em_gio_irq_enable;
> + irq_chip->irq_enable = em_gio_irq_enable;
> + irq_chip->irq_disable = em_gio_irq_disable;
> + irq_chip->irq_set_type = em_gio_irq_set_type;
> + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
> +
> + for (k = p->irq_base; k < (p->irq_base + pdata->number_of_pins); k++) {
> + irq_set_chip_and_handler_name(k, irq_chip, handle_level_irq,
> + "level");
> + set_irq_flags(k, IRQF_VALID); /* kill me now */
> + }
> +
> + if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
> + dev_err(&pdev->dev, "failed to request low IRQ\n");
> + ret = -ENOENT;
> + goto err4;
> + }
> +
> + if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
> + dev_err(&pdev->dev, "failed to request high IRQ\n");
> + ret = -ENOENT;
> + goto err5;
> + }
Can you try to use irqdomain for IRQ translation of the base?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-11 13:10 ` Linus Walleij
0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2012-05-11 13:10 UTC (permalink / raw)
To: Magnus Damm
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> +config GPIO_EM
> + tristate "Emma Mobile GPIO"
> + depends on ARM
ARM really? Why should all ARM systems have the option of configuring in an Emma
controller?
> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
> +{
> + if (offs < GIO_IDT0)
> + return ioread32(p->base0 + offs);
> + else
> + return ioread32(p->base1 + (offs - GIO_IDT0));
> +}
> +
> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
> + unsigned long value)
> +{
> + if (offs < GIO_IDT0)
> + iowrite32(value, p->base0 + offs);
> + else
> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
> +}
Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
readl_[relaxed] and writel_[relaxed]()?
> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
> +{
> + struct irq_chip *chip = irq_get_chip(irq);
> + return container_of(chip, struct em_gio_priv, irq_chip);
> +}
Should this be inline maybe?
> +
> +static void em_gio_irq_disable(struct irq_data *data)
> +{
> + struct em_gio_priv *p = irq_to_priv(data->irq);
> + int offset = data->irq - p->irq_base;
> +
> + em_gio_write(p, GIO_IDS, 1 << offset);
Suggest:
#include <linux/bitops.h>
em_gio_write(p, GIO_IDS, BIT(offset));
(No big deal, just having fun...)
> +static void em_gio_irq_enable(struct irq_data *data)
> +{
> + struct em_gio_priv *p = irq_to_priv(data->irq);
> + int offset = data->irq - p->irq_base;
> +
> + em_gio_write(p, GIO_IEN, 1 << offset);
> +}
Dito.
> + /* disable the interrupt in IIA */
> + tmp = em_gio_read(p, GIO_IIA);
> + tmp &= ~(1 << offset);
Dito.
(etc)
> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
> +{
> + struct em_gio_priv *p = dev_id;
> + unsigned long tmp, status;
> + unsigned int offset;
> +
> + status = tmp = em_gio_read(p, GIO_MST);
> + if (!status)
> + return IRQ_NONE;
> +
> + while (status) {
> + offset = __ffs(status);
> + generic_handle_irq(p->irq_base + offset);
Pleas use irqdomain to translate the IRQ numbers.
> + status &= ~(1 << offset);
> + }
We've recently patched a log of IRQ handlers to re-read the IRQ
status register on each iteration. I suspect that is needed here
as well.
while (status = em_gio_read(p, GIO_MST)) {
> +
> + em_gio_write(p, GIO_IIR, tmp);
And then I guess this would need to be moved into the
loop to clear one bit at the time instead.
> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
> +{
> + return container_of(chip, struct em_gio_priv, gpio_chip);
> +}
inline?
> +static int __devinit em_gio_probe(struct platform_device *pdev)
> +{
> + struct gpio_em_config *pdata = pdev->dev.platform_data;
> + struct em_gio_priv *p;
> + struct resource *io[2], *irq[2];
> + struct gpio_chip *gpio_chip;
> + struct irq_chip *irq_chip;
> + const char *name = dev_name(&pdev->dev);
> + int k, ret;
> +
> + p = kzalloc(sizeof(*p), GFP_KERNEL);
Use devm_kzalloc() and friends?
Which makes the error path all much simpler.
> + p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
> + if (!p->base0) {
> + dev_err(&pdev->dev, "failed to remap low I/O memory\n");
> + ret = -ENXIO;
> + goto err1;
> + }
> +
> + p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
> + if (!p->base1) {
> + dev_err(&pdev->dev, "failed to remap high I/O memory\n");
> + ret = -ENXIO;
> + goto err2;
> + }
I usually also request the memory region request_mem_region(),
I don't know if I'm particularly picky though.
Isn't there a new nice inline that will both request and
remap a piece of memory?
> + p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
> + pdata->number_of_pins, numa_node_id());
> + if (IS_ERR_VALUE(p->irq_base)) {
> + dev_err(&pdev->dev, "cannot get irq_desc\n");
> + ret = -ENXIO;
> + goto err3;
> + }
> +
> + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
> + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
> +
> + irq_chip->name = name;
> + irq_chip->irq_mask = em_gio_irq_disable;
> + irq_chip->irq_unmask = em_gio_irq_enable;
> + irq_chip->irq_enable = em_gio_irq_enable;
> + irq_chip->irq_disable = em_gio_irq_disable;
> + irq_chip->irq_set_type = em_gio_irq_set_type;
> + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
> +
> + for (k = p->irq_base; k < (p->irq_base + pdata->number_of_pins); k++) {
> + irq_set_chip_and_handler_name(k, irq_chip, handle_level_irq,
> + "level");
> + set_irq_flags(k, IRQF_VALID); /* kill me now */
> + }
> +
> + if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
> + dev_err(&pdev->dev, "failed to request low IRQ\n");
> + ret = -ENOENT;
> + goto err4;
> + }
> +
> + if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
> + dev_err(&pdev->dev, "failed to request high IRQ\n");
> + ret = -ENOENT;
> + goto err5;
> + }
Can you try to use irqdomain for IRQ translation of the base?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-11 13:10 ` Linus Walleij
@ 2012-05-11 15:45 ` Magnus Damm
-1 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-11 15:45 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
Hey Linus,
Thanks for your review!
On Fri, May 11, 2012 at 10:10 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> +config GPIO_EM
>> + tristate "Emma Mobile GPIO"
>> + depends on ARM
>
> ARM really? Why should all ARM systems have the option of configuring in an Emma
> controller?
Well, first of all, the Emma Mobile line of SoCs are ARM based. You
may think of MIPS based Emma devices. I don' t know so much about
them, but if you happen to know where I can find docs then I'd be very
interested in looking at them to see if I can share any of my recently
developed drivers.
Not sure if your point is that this has nothing to do with the CPU
core itself - at least I usually prefer to omit CPU dependencies for
I/O devices and have as few dependencies as possible to get as much as
compile time testing done. In this particular case I've added ARM as
dependency because I was asked so for the UART driver 8250_em. Either
way is fine with me, so it's up to you GPIO maintainers to decide what
you want. As long as I can enable it on the ARM based SoC family I'm
happy. =)
>> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
>> +{
>> + if (offs < GIO_IDT0)
>> + return ioread32(p->base0 + offs);
>> + else
>> + return ioread32(p->base1 + (offs - GIO_IDT0));
>> +}
>> +
>> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
>> + unsigned long value)
>> +{
>> + if (offs < GIO_IDT0)
>> + iowrite32(value, p->base0 + offs);
>> + else
>> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
>> +}
>
> Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
> readl_[relaxed] and writel_[relaxed]()?
Nothing wrong with readl/writel, I've just have a habit of using the
ioread/iowrite ones. I find the code that allows platforms to select
PORT or IOMEM in lib/iomap.c rather neat.
>> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
>> +{
>> + struct irq_chip *chip = irq_get_chip(irq);
>> + return container_of(chip, struct em_gio_priv, irq_chip);
>> +}
>
> Should this be inline maybe?
I have not checked if they actually turn out inline with my ARM
compiler, but gcc for SH was always rather good at deciding when to
inline by itself. Its a bit like likely()/unlikely() in my mind, it
may be good to give hints for the compiler but in the majority of the
cases the compiler will be fine by itself. I guess this particular
function may end up as just a few instructions.
>> +
>> +static void em_gio_irq_disable(struct irq_data *data)
>> +{
>> + struct em_gio_priv *p = irq_to_priv(data->irq);
>> + int offset = data->irq - p->irq_base;
>> +
>> + em_gio_write(p, GIO_IDS, 1 << offset);
>
> Suggest:
> #include <linux/bitops.h>
> em_gio_write(p, GIO_IDS, BIT(offset));
>
> (No big deal, just having fun...)
Sure, why not?
>> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>> +{
>> + struct em_gio_priv *p = dev_id;
>> + unsigned long tmp, status;
>> + unsigned int offset;
>> +
>> + status = tmp = em_gio_read(p, GIO_MST);
>> + if (!status)
>> + return IRQ_NONE;
>> +
>> + while (status) {
>> + offset = __ffs(status);
>> + generic_handle_irq(p->irq_base + offset);
>
> Pleas use irqdomain to translate the IRQ numbers.
Is this mandatory for regular platform devices not using DT?
I don't object to your idea, but I was planning on adding that in my
DT feature patch.
>> + status &= ~(1 << offset);
>> + }
>
> We've recently patched a log of IRQ handlers to re-read the IRQ
> status register on each iteration. I suspect that is needed here
> as well.
Doesn't that depend on how the hardware works? OTOH, it may be a safe
way to implement correct code for all kinds of controllers, not sure.
> while (status = em_gio_read(p, GIO_MST)) {
>
>> +
>> + em_gio_write(p, GIO_IIR, tmp);
>
> And then I guess this would need to be moved into the
> loop to clear one bit at the time instead.
I guess it can be done like that, but wouldn't that lead to potential
starvation of IRQs with bits that happen to be processed first in the
word?
>> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
>> +{
>> + return container_of(chip, struct em_gio_priv, gpio_chip);
>> +}
>
> inline?
Sure, if you think that is absolutely necessary. May I ask, is it
important for you, or is it ok if I skip and keep this driver in line
with my other ones? =)
>> +static int __devinit em_gio_probe(struct platform_device *pdev)
>> +{
>> + struct gpio_em_config *pdata = pdev->dev.platform_data;
>> + struct em_gio_priv *p;
>> + struct resource *io[2], *irq[2];
>> + struct gpio_chip *gpio_chip;
>> + struct irq_chip *irq_chip;
>> + const char *name = dev_name(&pdev->dev);
>> + int k, ret;
>> +
>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>
> Use devm_kzalloc() and friends?
I tend to prefer not to. This because I need to perform proper error
handling anyway.
> Which makes the error path all much simpler.
Maybe so, perhaps I should look into that anyway though.
>> + p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
>> + if (!p->base0) {
>> + dev_err(&pdev->dev, "failed to remap low I/O memory\n");
>> + ret = -ENXIO;
>> + goto err1;
>> + }
>> +
>> + p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
>> + if (!p->base1) {
>> + dev_err(&pdev->dev, "failed to remap high I/O memory\n");
>> + ret = -ENXIO;
>> + goto err2;
>> + }
>
> I usually also request the memory region request_mem_region(),
> I don't know if I'm particularly picky though.
I have left that out myself of pure laziness in my drivers. It may be
good to add. In general I find that there are a few commonly
duplicated operations in the drivers. I realize each function serve a
certain purpose, but at the same time I have a feeling that every
driver ends up moving data from struct resource into other formats and
then perform the same couple of request_irq() or request_mem_region()
and ioremap(). It would be neat to have some helpers operating on
struct resource directly.
> Isn't there a new nice inline that will both request and
> remap a piece of memory?
If so then that would be excellent. Especially together with
ioread/iowrite so the code can work both for IOMEM and PORT
transparently.
>> + p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
>> + pdata->number_of_pins, numa_node_id());
>> + if (IS_ERR_VALUE(p->irq_base)) {
>> + dev_err(&pdev->dev, "cannot get irq_desc\n");
>> + ret = -ENXIO;
>> + goto err3;
>> + }
>> +
>> + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
>> + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
>> +
>> + irq_chip->name = name;
>> + irq_chip->irq_mask = em_gio_irq_disable;
>> + irq_chip->irq_unmask = em_gio_irq_enable;
>> + irq_chip->irq_enable = em_gio_irq_enable;
>> + irq_chip->irq_disable = em_gio_irq_disable;
>> + irq_chip->irq_set_type = em_gio_irq_set_type;
>> + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
>> +
>> + for (k = p->irq_base; k < (p->irq_base + pdata->number_of_pins); k++) {
>> + irq_set_chip_and_handler_name(k, irq_chip, handle_level_irq,
>> + "level");
>> + set_irq_flags(k, IRQF_VALID); /* kill me now */
>> + }
>> +
>> + if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
>> + dev_err(&pdev->dev, "failed to request low IRQ\n");
>> + ret = -ENOENT;
>> + goto err4;
>> + }
>> +
>> + if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
>> + dev_err(&pdev->dev, "failed to request high IRQ\n");
>> + ret = -ENOENT;
>> + goto err5;
>> + }
>
> Can you try to use irqdomain for IRQ translation of the base?
I was thinking of doing that as part of my DT enabling work, would that be ok?
Do you have any good suggestion what to use to unregister my irqchip?
I believe this version of the driver isn't doing that properly.
Thanks for your help!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-11 15:45 ` Magnus Damm
0 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-11 15:45 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
Hey Linus,
Thanks for your review!
On Fri, May 11, 2012 at 10:10 PM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>
>> +config GPIO_EM
>> + tristate "Emma Mobile GPIO"
>> + depends on ARM
>
> ARM really? Why should all ARM systems have the option of configuring in an Emma
> controller?
Well, first of all, the Emma Mobile line of SoCs are ARM based. You
may think of MIPS based Emma devices. I don' t know so much about
them, but if you happen to know where I can find docs then I'd be very
interested in looking at them to see if I can share any of my recently
developed drivers.
Not sure if your point is that this has nothing to do with the CPU
core itself - at least I usually prefer to omit CPU dependencies for
I/O devices and have as few dependencies as possible to get as much as
compile time testing done. In this particular case I've added ARM as
dependency because I was asked so for the UART driver 8250_em. Either
way is fine with me, so it's up to you GPIO maintainers to decide what
you want. As long as I can enable it on the ARM based SoC family I'm
happy. =)
>> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
>> +{
>> + if (offs < GIO_IDT0)
>> + return ioread32(p->base0 + offs);
>> + else
>> + return ioread32(p->base1 + (offs - GIO_IDT0));
>> +}
>> +
>> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
>> + unsigned long value)
>> +{
>> + if (offs < GIO_IDT0)
>> + iowrite32(value, p->base0 + offs);
>> + else
>> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
>> +}
>
> Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
> readl_[relaxed] and writel_[relaxed]()?
Nothing wrong with readl/writel, I've just have a habit of using the
ioread/iowrite ones. I find the code that allows platforms to select
PORT or IOMEM in lib/iomap.c rather neat.
>> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
>> +{
>> + struct irq_chip *chip = irq_get_chip(irq);
>> + return container_of(chip, struct em_gio_priv, irq_chip);
>> +}
>
> Should this be inline maybe?
I have not checked if they actually turn out inline with my ARM
compiler, but gcc for SH was always rather good at deciding when to
inline by itself. Its a bit like likely()/unlikely() in my mind, it
may be good to give hints for the compiler but in the majority of the
cases the compiler will be fine by itself. I guess this particular
function may end up as just a few instructions.
>> +
>> +static void em_gio_irq_disable(struct irq_data *data)
>> +{
>> + struct em_gio_priv *p = irq_to_priv(data->irq);
>> + int offset = data->irq - p->irq_base;
>> +
>> + em_gio_write(p, GIO_IDS, 1 << offset);
>
> Suggest:
> #include <linux/bitops.h>
> em_gio_write(p, GIO_IDS, BIT(offset));
>
> (No big deal, just having fun...)
Sure, why not?
>> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>> +{
>> + struct em_gio_priv *p = dev_id;
>> + unsigned long tmp, status;
>> + unsigned int offset;
>> +
>> + status = tmp = em_gio_read(p, GIO_MST);
>> + if (!status)
>> + return IRQ_NONE;
>> +
>> + while (status) {
>> + offset = __ffs(status);
>> + generic_handle_irq(p->irq_base + offset);
>
> Pleas use irqdomain to translate the IRQ numbers.
Is this mandatory for regular platform devices not using DT?
I don't object to your idea, but I was planning on adding that in my
DT feature patch.
>> + status &= ~(1 << offset);
>> + }
>
> We've recently patched a log of IRQ handlers to re-read the IRQ
> status register on each iteration. I suspect that is needed here
> as well.
Doesn't that depend on how the hardware works? OTOH, it may be a safe
way to implement correct code for all kinds of controllers, not sure.
> while (status = em_gio_read(p, GIO_MST)) {
>
>> +
>> + em_gio_write(p, GIO_IIR, tmp);
>
> And then I guess this would need to be moved into the
> loop to clear one bit at the time instead.
I guess it can be done like that, but wouldn't that lead to potential
starvation of IRQs with bits that happen to be processed first in the
word?
>> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
>> +{
>> + return container_of(chip, struct em_gio_priv, gpio_chip);
>> +}
>
> inline?
Sure, if you think that is absolutely necessary. May I ask, is it
important for you, or is it ok if I skip and keep this driver in line
with my other ones? =)
>> +static int __devinit em_gio_probe(struct platform_device *pdev)
>> +{
>> + struct gpio_em_config *pdata = pdev->dev.platform_data;
>> + struct em_gio_priv *p;
>> + struct resource *io[2], *irq[2];
>> + struct gpio_chip *gpio_chip;
>> + struct irq_chip *irq_chip;
>> + const char *name = dev_name(&pdev->dev);
>> + int k, ret;
>> +
>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>
> Use devm_kzalloc() and friends?
I tend to prefer not to. This because I need to perform proper error
handling anyway.
> Which makes the error path all much simpler.
Maybe so, perhaps I should look into that anyway though.
>> + p->base0 = ioremap_nocache(io[0]->start, resource_size(io[0]));
>> + if (!p->base0) {
>> + dev_err(&pdev->dev, "failed to remap low I/O memory\n");
>> + ret = -ENXIO;
>> + goto err1;
>> + }
>> +
>> + p->base1 = ioremap_nocache(io[1]->start, resource_size(io[1]));
>> + if (!p->base1) {
>> + dev_err(&pdev->dev, "failed to remap high I/O memory\n");
>> + ret = -ENXIO;
>> + goto err2;
>> + }
>
> I usually also request the memory region request_mem_region(),
> I don't know if I'm particularly picky though.
I have left that out myself of pure laziness in my drivers. It may be
good to add. In general I find that there are a few commonly
duplicated operations in the drivers. I realize each function serve a
certain purpose, but at the same time I have a feeling that every
driver ends up moving data from struct resource into other formats and
then perform the same couple of request_irq() or request_mem_region()
and ioremap(). It would be neat to have some helpers operating on
struct resource directly.
> Isn't there a new nice inline that will both request and
> remap a piece of memory?
If so then that would be excellent. Especially together with
ioread/iowrite so the code can work both for IOMEM and PORT
transparently.
>> + p->irq_base = irq_alloc_descs(pdata->irq_base, 0,
>> + pdata->number_of_pins, numa_node_id());
>> + if (IS_ERR_VALUE(p->irq_base)) {
>> + dev_err(&pdev->dev, "cannot get irq_desc\n");
>> + ret = -ENXIO;
>> + goto err3;
>> + }
>> +
>> + pr_debug("gio: hw base = %d, nr = %d, sw base = %d\n",
>> + pdata->gpio_base, pdata->number_of_pins, p->irq_base);
>> +
>> + irq_chip->name = name;
>> + irq_chip->irq_mask = em_gio_irq_disable;
>> + irq_chip->irq_unmask = em_gio_irq_enable;
>> + irq_chip->irq_enable = em_gio_irq_enable;
>> + irq_chip->irq_disable = em_gio_irq_disable;
>> + irq_chip->irq_set_type = em_gio_irq_set_type;
>> + irq_chip->flags = IRQCHIP_SKIP_SET_WAKE;
>> +
>> + for (k = p->irq_base; k < (p->irq_base + pdata->number_of_pins); k++) {
>> + irq_set_chip_and_handler_name(k, irq_chip, handle_level_irq,
>> + "level");
>> + set_irq_flags(k, IRQF_VALID); /* kill me now */
>> + }
>> +
>> + if (request_irq(irq[0]->start, em_gio_irq_handler, 0, name, p)) {
>> + dev_err(&pdev->dev, "failed to request low IRQ\n");
>> + ret = -ENOENT;
>> + goto err4;
>> + }
>> +
>> + if (request_irq(irq[1]->start, em_gio_irq_handler, 0, name, p)) {
>> + dev_err(&pdev->dev, "failed to request high IRQ\n");
>> + ret = -ENOENT;
>> + goto err5;
>> + }
>
> Can you try to use irqdomain for IRQ translation of the base?
I was thinking of doing that as part of my DT enabling work, would that be ok?
Do you have any good suggestion what to use to unregister my irqchip?
I believe this version of the driver isn't doing that properly.
Thanks for your help!
Cheers,
/ magnus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-11 15:45 ` Magnus Damm
@ 2012-05-11 22:12 ` Linus Walleij
-1 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2012-05-11 22:12 UTC (permalink / raw)
To: Magnus Damm
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
On Fri, May 11, 2012 at 5:45 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> +config GPIO_EM
>>> + tristate "Emma Mobile GPIO"
>>> + depends on ARM
>>
>> ARM really? Why should all ARM systems have the option of configuring in an Emma
>> controller?
>
(...)
> Not sure if your point is that this has nothing to do with the CPU
> core itself
Nah I was more after a more strict dependency, like in this example
from pinctrl:
config PINCTRL_SIRF
bool "CSR SiRFprimaII pin controller driver"
depends on ARCH_PRIMA2
select PINMUX
So if this controller is only in one arch then put that as depends...
>>> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>>> +{
>>> + struct em_gio_priv *p = dev_id;
>>> + unsigned long tmp, status;
>>> + unsigned int offset;
>>> +
>>> + status = tmp = em_gio_read(p, GIO_MST);
>>> + if (!status)
>>> + return IRQ_NONE;
>>> +
>>> + while (status) {
>>> + offset = __ffs(status);
>>> + generic_handle_irq(p->irq_base + offset);
>>
>> Pleas use irqdomain to translate the IRQ numbers.
>
> Is this mandatory for regular platform devices not using DT?
Irqdomain has nothing to do with DT ...
It's just a very nice way to do translation of a range of local
numbers onto the bigger IRQ numberspace, we've been discussing
what to do with IRQ numbers and after some dealing with
irqdomain I think it's really nice for this.
It does the same as offsetting like above, but in a more
abstract way.
> I don't object to your idea, but I was planning on adding that in my
> DT feature patch.
Why not do it now and be done with it :-)
>>> + status &= ~(1 << offset);
>>> + }
>>
>> We've recently patched a log of IRQ handlers to re-read the IRQ
>> status register on each iteration. I suspect that is needed here
>> as well.
>
> Doesn't that depend on how the hardware works? OTOH, it may be a safe
> way to implement correct code for all kinds of controllers, not sure.
Not really, the issue is how the IRQ framework works really.
Check out this pointer from Russell:
http://lists.arm.linux.org.uk/lurker/message/20120402.214614.e7740b12.en.html
and the rest of that thread.
> I guess it can be done like that, but wouldn't that lead to potential
> starvation of IRQs with bits that happen to be processed first in the
> word?
Yes, but the alternative seems to be to miss IRQs which is probably
worse.
>> inline?
>
> Sure, if you think that is absolutely necessary. May I ask, is it
> important for you, or is it ok if I skip and keep this driver in line
> with my other ones? =)
Forget it... no big deal.
>>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>>
>> Use devm_kzalloc() and friends?
>
> I tend to prefer not to. This because I need to perform proper error
> handling anyway.
>
>> Which makes the error path all much simpler.
>
> Maybe so, perhaps I should look into that anyway though.
OK no big deal for me, just seen it a bit lately, it's getting
popular.
>> Isn't there a new nice inline that will both request and
>> remap a piece of memory?
>
> If so then that would be excellent. Especially together with
> ioread/iowrite so the code can work both for IOMEM and PORT
> transparently.
Speaking about the garbage-collecting devm_* stuff, there
is:
devm_ioremap()
Which will auto-release the mapping when the device goes away.
(Makes exit() really thin.)
> Do you have any good suggestion what to use to unregister my irqchip?
> I believe this version of the driver isn't doing that properly.
Is this code going to be used as module? Else one way to
avoid it to to not have an exit() function like drivers/gpio/gpio-pl061.c
Most people seem to avoid that so you really got me on this
one, I really have no good example of how to do that...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-11 22:12 ` Linus Walleij
0 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2012-05-11 22:12 UTC (permalink / raw)
To: Magnus Damm
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
On Fri, May 11, 2012 at 5:45 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>>> +config GPIO_EM
>>> + tristate "Emma Mobile GPIO"
>>> + depends on ARM
>>
>> ARM really? Why should all ARM systems have the option of configuring in an Emma
>> controller?
>
(...)
> Not sure if your point is that this has nothing to do with the CPU
> core itself
Nah I was more after a more strict dependency, like in this example
from pinctrl:
config PINCTRL_SIRF
bool "CSR SiRFprimaII pin controller driver"
depends on ARCH_PRIMA2
select PINMUX
So if this controller is only in one arch then put that as depends...
>>> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>>> +{
>>> + struct em_gio_priv *p = dev_id;
>>> + unsigned long tmp, status;
>>> + unsigned int offset;
>>> +
>>> + status = tmp = em_gio_read(p, GIO_MST);
>>> + if (!status)
>>> + return IRQ_NONE;
>>> +
>>> + while (status) {
>>> + offset = __ffs(status);
>>> + generic_handle_irq(p->irq_base + offset);
>>
>> Pleas use irqdomain to translate the IRQ numbers.
>
> Is this mandatory for regular platform devices not using DT?
Irqdomain has nothing to do with DT ...
It's just a very nice way to do translation of a range of local
numbers onto the bigger IRQ numberspace, we've been discussing
what to do with IRQ numbers and after some dealing with
irqdomain I think it's really nice for this.
It does the same as offsetting like above, but in a more
abstract way.
> I don't object to your idea, but I was planning on adding that in my
> DT feature patch.
Why not do it now and be done with it :-)
>>> + status &= ~(1 << offset);
>>> + }
>>
>> We've recently patched a log of IRQ handlers to re-read the IRQ
>> status register on each iteration. I suspect that is needed here
>> as well.
>
> Doesn't that depend on how the hardware works? OTOH, it may be a safe
> way to implement correct code for all kinds of controllers, not sure.
Not really, the issue is how the IRQ framework works really.
Check out this pointer from Russell:
http://lists.arm.linux.org.uk/lurker/message/20120402.214614.e7740b12.en.html
and the rest of that thread.
> I guess it can be done like that, but wouldn't that lead to potential
> starvation of IRQs with bits that happen to be processed first in the
> word?
Yes, but the alternative seems to be to miss IRQs which is probably
worse.
>> inline?
>
> Sure, if you think that is absolutely necessary. May I ask, is it
> important for you, or is it ok if I skip and keep this driver in line
> with my other ones? =)
Forget it... no big deal.
>>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>>
>> Use devm_kzalloc() and friends?
>
> I tend to prefer not to. This because I need to perform proper error
> handling anyway.
>
>> Which makes the error path all much simpler.
>
> Maybe so, perhaps I should look into that anyway though.
OK no big deal for me, just seen it a bit lately, it's getting
popular.
>> Isn't there a new nice inline that will both request and
>> remap a piece of memory?
>
> If so then that would be excellent. Especially together with
> ioread/iowrite so the code can work both for IOMEM and PORT
> transparently.
Speaking about the garbage-collecting devm_* stuff, there
is:
devm_ioremap()
Which will auto-release the mapping when the device goes away.
(Makes exit() really thin.)
> Do you have any good suggestion what to use to unregister my irqchip?
> I believe this version of the driver isn't doing that properly.
Is this code going to be used as module? Else one way to
avoid it to to not have an exit() function like drivers/gpio/gpio-pl061.c
Most people seem to avoid that so you really got me on this
one, I really have no good example of how to do that...
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-11 15:45 ` Magnus Damm
(?)
(?)
@ 2012-05-12 13:18 ` Arnd Bergmann
2012-05-15 16:14 ` Magnus Damm
-1 siblings, 1 reply; 14+ messages in thread
From: Arnd Bergmann @ 2012-05-12 13:18 UTC (permalink / raw)
To: Magnus Damm
Cc: Linus Walleij, linux-kernel, rjw, linus.walleij, linux-sh, horms,
grant.likely, lethal, olof
On Friday 11 May 2012, Magnus Damm wrote:
> On Fri, May 11, 2012 at 10:10 PM, Linus Walleij
> <linus.walleij@linaro.org> wrote:
> > On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> >
> >> +config GPIO_EM
> >> + tristate "Emma Mobile GPIO"
> >> + depends on ARM
> >
> > ARM really? Why should all ARM systems have the option of configuring in an Emma
> > controller?
>
> Well, first of all, the Emma Mobile line of SoCs are ARM based. You
> may think of MIPS based Emma devices. I don' t know so much about
> them, but if you happen to know where I can find docs then I'd be very
> interested in looking at them to see if I can share any of my recently
> developed drivers.
>
> Not sure if your point is that this has nothing to do with the CPU
> core itself - at least I usually prefer to omit CPU dependencies for
> I/O devices and have as few dependencies as possible to get as much as
> compile time testing done. In this particular case I've added ARM as
> dependency because I was asked so for the UART driver 8250_em. Either
> way is fine with me, so it's up to you GPIO maintainers to decide what
> you want. As long as I can enable it on the ARM based SoC family I'm
> happy. =)
My preference is generally to define the dependencies rather broadly
so you can build each driver on as many platforms as possible, and
just leave them out of defconfig. This seems to be the common way to
do things. Limiting by architecture makes some sense if you don't
want to expose the driver to all architectures that might not be
able to build it (e.g. s390 would not be able to build this one).
> >> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
> >> +{
> >> + if (offs < GIO_IDT0)
> >> + return ioread32(p->base0 + offs);
> >> + else
> >> + return ioread32(p->base1 + (offs - GIO_IDT0));
> >> +}
> >> +
> >> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
> >> + unsigned long value)
> >> +{
> >> + if (offs < GIO_IDT0)
> >> + iowrite32(value, p->base0 + offs);
> >> + else
> >> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
> >> +}
> >
> > Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
> > readl_[relaxed] and writel_[relaxed]()?
>
> Nothing wrong with readl/writel, I've just have a habit of using the
> ioread/iowrite ones. I find the code that allows platforms to select
> PORT or IOMEM in lib/iomap.c rather neat.
For reference: The ioread family is defined to be a superset of the
readl family and the inl family and to be compatible with the calling
convention of readl, so it's correct to use it here, but it may be
slower on architectures that require the lib/iomap.c wrappers. ARM
does not use those wrappers on modern systems because the PIO space
is memory mapped.
For performance sensitive drivers that do not use DMA, it makes
sense to use the readl_relaxed() family instead, because those
omit the expensive barriers that protect us from overlapping with
DMA.
The main advantage of using readl/writel is that it's more common
on ARM so reviewers don't have to wonder why this driver does things
differently. Any driver shared with powerpc, mips or sh would likely
use a different style. E.g. on powerpc, using readl/writel is considered
wrong because it's only well-defined for PCI there.
> >> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
> >> +{
> >> + struct irq_chip *chip = irq_get_chip(irq);
> >> + return container_of(chip, struct em_gio_priv, irq_chip);
> >> +}
> >
> > Should this be inline maybe?
>
> I have not checked if they actually turn out inline with my ARM
> compiler, but gcc for SH was always rather good at deciding when to
> inline by itself. Its a bit like likely()/unlikely() in my mind, it
> may be good to give hints for the compiler but in the majority of the
> cases the compiler will be fine by itself. I guess this particular
> function may end up as just a few instructions.
Right.
> >> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
> >> +{
> >> + struct em_gio_priv *p = dev_id;
> >> + unsigned long tmp, status;
> >> + unsigned int offset;
> >> +
> >> + status = tmp = em_gio_read(p, GIO_MST);
> >> + if (!status)
> >> + return IRQ_NONE;
> >> +
> >> + while (status) {
> >> + offset = __ffs(status);
> >> + generic_handle_irq(p->irq_base + offset);
> >
> > Pleas use irqdomain to translate the IRQ numbers.
>
> Is this mandatory for regular platform devices not using DT?
>
> I don't object to your idea, but I was planning on adding that in my
> DT feature patch.
There is little value in delaying this if you already know how
to do it. irq domains are useful for other reasons as well, e.g.
for allowing to build with sparse IRQ, which is needed when we get
to multiplatform kernels.
> >> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
> >> +{
> >> + return container_of(chip, struct em_gio_priv, gpio_chip);
> >> +}
> >
> > inline?
>
> Sure, if you think that is absolutely necessary. May I ask, is it
> important for you, or is it ok if I skip and keep this driver in line
> with my other ones? =)
I don't think it makes any difference in practice, other for people
reading the code. Adding the 'inline' tells the reader that this is
meant to be a trivial function that doesn't add much object code,
and that the author was aware of that. I don't think it matters much
either way.
> > Isn't there a new nice inline that will both request and
> > remap a piece of memory?
>
> If so then that would be excellent. Especially together with
> ioread/iowrite so the code can work both for IOMEM and PORT
> transparently.
We have a bunch of helpers, but I think none that does
everything yet. io_iomap() locates and remaps the resource.
devm_request_and_ioremap() does the request and the map, but
doesn't pull it out of the device.
Arnd
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-11 22:12 ` Linus Walleij
@ 2012-05-15 16:03 ` Magnus Damm
-1 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-15 16:03 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
Hej Linus,
I just posted a V2 but here's my reply to your comments.
On Sat, May 12, 2012 at 7:12 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 11, 2012 at 5:45 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>
>>>> +config GPIO_EM
>>>> + tristate "Emma Mobile GPIO"
>>>> + depends on ARM
>>>
>>> ARM really? Why should all ARM systems have the option of configuring in an Emma
>>> controller?
>>
> (...)
>> Not sure if your point is that this has nothing to do with the CPU
>> core itself
>
> Nah I was more after a more strict dependency, like in this example
> from pinctrl:
>
> config PINCTRL_SIRF
> bool "CSR SiRFprimaII pin controller driver"
> depends on ARCH_PRIMA2
> select PINMUX
>
> So if this controller is only in one arch then put that as depends...
Right, I understand what you mean. I did however keep it as-is to get
as much compile testing done as possible.
>>>> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>>>> +{
>>>> + struct em_gio_priv *p = dev_id;
>>>> + unsigned long tmp, status;
>>>> + unsigned int offset;
>>>> +
>>>> + status = tmp = em_gio_read(p, GIO_MST);
>>>> + if (!status)
>>>> + return IRQ_NONE;
>>>> +
>>>> + while (status) {
>>>> + offset = __ffs(status);
>>>> + generic_handle_irq(p->irq_base + offset);
>>>
>>> Pleas use irqdomain to translate the IRQ numbers.
>>
>> Is this mandatory for regular platform devices not using DT?
>
> Irqdomain has nothing to do with DT ...
Really? In my mind irqdomain is a requirement for DT IRQ support. Of
course, it is also much more than that. But my impression is that the
real benefit comes when the ->xlat() callback is involved, and I don't
know of any way of using that without DT.
> It's just a very nice way to do translation of a range of local
> numbers onto the bigger IRQ numberspace, we've been discussing
> what to do with IRQ numbers and after some dealing with
> irqdomain I think it's really nice for this.
>
> It does the same as offsetting like above, but in a more
> abstract way.
Right! Thanks for pushing me in that direction, I believe the code
indeed became much cleaner.
>> I don't object to your idea, but I was planning on adding that in my
>> DT feature patch.
>
> Why not do it now and be done with it :-)
Never do today what you can do tomorrow. It's an old Swedish motto.
You should know. =)
>>>> + status &= ~(1 << offset);
>>>> + }
>>>
>>> We've recently patched a log of IRQ handlers to re-read the IRQ
>>> status register on each iteration. I suspect that is needed here
>>> as well.
>>
>> Doesn't that depend on how the hardware works? OTOH, it may be a safe
>> way to implement correct code for all kinds of controllers, not sure.
>
> Not really, the issue is how the IRQ framework works really.
> Check out this pointer from Russell:
> http://lists.arm.linux.org.uk/lurker/message/20120402.214614.e7740b12.en.html
> and the rest of that thread.
Thanks for the pointer!
>> I guess it can be done like that, but wouldn't that lead to potential
>> starvation of IRQs with bits that happen to be processed first in the
>> word?
>
> Yes, but the alternative seems to be to miss IRQs which is probably
> worse.
Indeed!
>>> inline?
>>
>> Sure, if you think that is absolutely necessary. May I ask, is it
>> important for you, or is it ok if I skip and keep this driver in line
>> with my other ones? =)
>
> Forget it... no big deal.
I actually made those functions inline after all. Not sure exactly why
though. =)
>>>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>>>
>>> Use devm_kzalloc() and friends?
>>
>> I tend to prefer not to. This because I need to perform proper error
>> handling anyway.
>>
>>> Which makes the error path all much simpler.
>>
>> Maybe so, perhaps I should look into that anyway though.
>
> OK no big deal for me, just seen it a bit lately, it's getting
> popular.
Right, it sure has. I still find it kind of awkward to use though so I
skipped it in V2.
>>> Isn't there a new nice inline that will both request and
>>> remap a piece of memory?
>>
>> If so then that would be excellent. Especially together with
>> ioread/iowrite so the code can work both for IOMEM and PORT
>> transparently.
>
> Speaking about the garbage-collecting devm_* stuff, there
> is:
> devm_ioremap()
>
> Which will auto-release the mapping when the device goes away.
> (Makes exit() really thin.)
Yeah, that's certainly a step in the right direction. I really wish I
had some spare time to hack on helper functions for filling the space
between struct device and ioremap/request_irq.
>> Do you have any good suggestion what to use to unregister my irqchip?
>> I believe this version of the driver isn't doing that properly.
>
> Is this code going to be used as module? Else one way to
> avoid it to to not have an exit() function like drivers/gpio/gpio-pl061.c
>
> Most people seem to avoid that so you really got me on this
> one, I really have no good example of how to do that...
After looking at the linear irq domain code it seems that the
->unmap() callback can be used somehow. Legacy irq domains do not
allow deregister. However, in both cases there is no code to remove
the irq domain from the list. That would also be good to hack on. Need
more time!
Thanks a lot for your help!
/ magnus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-15 16:03 ` Magnus Damm
0 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-15 16:03 UTC (permalink / raw)
To: Linus Walleij
Cc: linux-kernel, rjw, linus.walleij, arnd, linux-sh, horms,
grant.likely, lethal, olof
Hej Linus,
I just posted a V2 but here's my reply to your comments.
On Sat, May 12, 2012 at 7:12 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, May 11, 2012 at 5:45 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>
>>>> +config GPIO_EM
>>>> + tristate "Emma Mobile GPIO"
>>>> + depends on ARM
>>>
>>> ARM really? Why should all ARM systems have the option of configuring in an Emma
>>> controller?
>>
> (...)
>> Not sure if your point is that this has nothing to do with the CPU
>> core itself
>
> Nah I was more after a more strict dependency, like in this example
> from pinctrl:
>
> config PINCTRL_SIRF
> bool "CSR SiRFprimaII pin controller driver"
> depends on ARCH_PRIMA2
> select PINMUX
>
> So if this controller is only in one arch then put that as depends...
Right, I understand what you mean. I did however keep it as-is to get
as much compile testing done as possible.
>>>> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>>>> +{
>>>> + struct em_gio_priv *p = dev_id;
>>>> + unsigned long tmp, status;
>>>> + unsigned int offset;
>>>> +
>>>> + status = tmp = em_gio_read(p, GIO_MST);
>>>> + if (!status)
>>>> + return IRQ_NONE;
>>>> +
>>>> + while (status) {
>>>> + offset = __ffs(status);
>>>> + generic_handle_irq(p->irq_base + offset);
>>>
>>> Pleas use irqdomain to translate the IRQ numbers.
>>
>> Is this mandatory for regular platform devices not using DT?
>
> Irqdomain has nothing to do with DT ...
Really? In my mind irqdomain is a requirement for DT IRQ support. Of
course, it is also much more than that. But my impression is that the
real benefit comes when the ->xlat() callback is involved, and I don't
know of any way of using that without DT.
> It's just a very nice way to do translation of a range of local
> numbers onto the bigger IRQ numberspace, we've been discussing
> what to do with IRQ numbers and after some dealing with
> irqdomain I think it's really nice for this.
>
> It does the same as offsetting like above, but in a more
> abstract way.
Right! Thanks for pushing me in that direction, I believe the code
indeed became much cleaner.
>> I don't object to your idea, but I was planning on adding that in my
>> DT feature patch.
>
> Why not do it now and be done with it :-)
Never do today what you can do tomorrow. It's an old Swedish motto.
You should know. =)
>>>> + status &= ~(1 << offset);
>>>> + }
>>>
>>> We've recently patched a log of IRQ handlers to re-read the IRQ
>>> status register on each iteration. I suspect that is needed here
>>> as well.
>>
>> Doesn't that depend on how the hardware works? OTOH, it may be a safe
>> way to implement correct code for all kinds of controllers, not sure.
>
> Not really, the issue is how the IRQ framework works really.
> Check out this pointer from Russell:
> http://lists.arm.linux.org.uk/lurker/message/20120402.214614.e7740b12.en.html
> and the rest of that thread.
Thanks for the pointer!
>> I guess it can be done like that, but wouldn't that lead to potential
>> starvation of IRQs with bits that happen to be processed first in the
>> word?
>
> Yes, but the alternative seems to be to miss IRQs which is probably
> worse.
Indeed!
>>> inline?
>>
>> Sure, if you think that is absolutely necessary. May I ask, is it
>> important for you, or is it ok if I skip and keep this driver in line
>> with my other ones? =)
>
> Forget it... no big deal.
I actually made those functions inline after all. Not sure exactly why
though. =)
>>>> + p = kzalloc(sizeof(*p), GFP_KERNEL);
>>>
>>> Use devm_kzalloc() and friends?
>>
>> I tend to prefer not to. This because I need to perform proper error
>> handling anyway.
>>
>>> Which makes the error path all much simpler.
>>
>> Maybe so, perhaps I should look into that anyway though.
>
> OK no big deal for me, just seen it a bit lately, it's getting
> popular.
Right, it sure has. I still find it kind of awkward to use though so I
skipped it in V2.
>>> Isn't there a new nice inline that will both request and
>>> remap a piece of memory?
>>
>> If so then that would be excellent. Especially together with
>> ioread/iowrite so the code can work both for IOMEM and PORT
>> transparently.
>
> Speaking about the garbage-collecting devm_* stuff, there
> is:
> devm_ioremap()
>
> Which will auto-release the mapping when the device goes away.
> (Makes exit() really thin.)
Yeah, that's certainly a step in the right direction. I really wish I
had some spare time to hack on helper functions for filling the space
between struct device and ioremap/request_irq.
>> Do you have any good suggestion what to use to unregister my irqchip?
>> I believe this version of the driver isn't doing that properly.
>
> Is this code going to be used as module? Else one way to
> avoid it to to not have an exit() function like drivers/gpio/gpio-pl061.c
>
> Most people seem to avoid that so you really got me on this
> one, I really have no good example of how to do that...
After looking at the linear irq domain code it seems that the
->unmap() callback can be used somehow. Legacy irq domains do not
allow deregister. However, in both cases there is no code to remove
the irq domain from the list. That would also be good to hack on. Need
more time!
Thanks a lot for your help!
/ magnus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-12 13:18 ` Arnd Bergmann
@ 2012-05-15 16:14 ` Magnus Damm
0 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-15 16:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Walleij, linux-kernel, rjw, linus.walleij, linux-sh, horms,
grant.likely, lethal, olof
On Sat, May 12, 2012 at 10:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 May 2012, Magnus Damm wrote:
>> On Fri, May 11, 2012 at 10:10 PM, Linus Walleij
>> <linus.walleij@linaro.org> wrote:
>> > On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> >
>> >> +config GPIO_EM
>> >> + tristate "Emma Mobile GPIO"
>> >> + depends on ARM
>> >
>> > ARM really? Why should all ARM systems have the option of configuring in an Emma
>> > controller?
>>
>> Well, first of all, the Emma Mobile line of SoCs are ARM based. You
>> may think of MIPS based Emma devices. I don' t know so much about
>> them, but if you happen to know where I can find docs then I'd be very
>> interested in looking at them to see if I can share any of my recently
>> developed drivers.
>>
>> Not sure if your point is that this has nothing to do with the CPU
>> core itself - at least I usually prefer to omit CPU dependencies for
>> I/O devices and have as few dependencies as possible to get as much as
>> compile time testing done. In this particular case I've added ARM as
>> dependency because I was asked so for the UART driver 8250_em. Either
>> way is fine with me, so it's up to you GPIO maintainers to decide what
>> you want. As long as I can enable it on the ARM based SoC family I'm
>> happy. =)
>
> My preference is generally to define the dependencies rather broadly
> so you can build each driver on as many platforms as possible, and
> just leave them out of defconfig. This seems to be the common way to
> do things. Limiting by architecture makes some sense if you don't
> want to expose the driver to all architectures that might not be
> able to build it (e.g. s390 would not be able to build this one).
I believe that is rather close to what i prefer too. I kept the ARM dependency.
>> >> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
>> >> +{
>> >> + if (offs < GIO_IDT0)
>> >> + return ioread32(p->base0 + offs);
>> >> + else
>> >> + return ioread32(p->base1 + (offs - GIO_IDT0));
>> >> +}
>> >> +
>> >> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
>> >> + unsigned long value)
>> >> +{
>> >> + if (offs < GIO_IDT0)
>> >> + iowrite32(value, p->base0 + offs);
>> >> + else
>> >> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
>> >> +}
>> >
>> > Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
>> > readl_[relaxed] and writel_[relaxed]()?
>>
>> Nothing wrong with readl/writel, I've just have a habit of using the
>> ioread/iowrite ones. I find the code that allows platforms to select
>> PORT or IOMEM in lib/iomap.c rather neat.
>
> For reference: The ioread family is defined to be a superset of the
> readl family and the inl family and to be compatible with the calling
> convention of readl, so it's correct to use it here, but it may be
> slower on architectures that require the lib/iomap.c wrappers. ARM
> does not use those wrappers on modern systems because the PIO space
> is memory mapped.
>
> For performance sensitive drivers that do not use DMA, it makes
> sense to use the readl_relaxed() family instead, because those
> omit the expensive barriers that protect us from overlapping with
> DMA.
>
> The main advantage of using readl/writel is that it's more common
> on ARM so reviewers don't have to wonder why this driver does things
> differently. Any driver shared with powerpc, mips or sh would likely
> use a different style. E.g. on powerpc, using readl/writel is considered
> wrong because it's only well-defined for PCI there.
Thanks for zooming out a bit and sharing your knowledge. As you know,
the code is not shared with any other architecture at this point, but
I decided to keep on using ioread/iowrite in V2 after all. I may
optimize the GPIO bits later if I need to hook up some big banging SPI
or I2C, but for now I think this is fine.
>> >> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
>> >> +{
>> >> + struct irq_chip *chip = irq_get_chip(irq);
>> >> + return container_of(chip, struct em_gio_priv, irq_chip);
>> >> +}
>> >
>> > Should this be inline maybe?
>>
>> I have not checked if they actually turn out inline with my ARM
>> compiler, but gcc for SH was always rather good at deciding when to
>> inline by itself. Its a bit like likely()/unlikely() in my mind, it
>> may be good to give hints for the compiler but in the majority of the
>> cases the compiler will be fine by itself. I guess this particular
>> function may end up as just a few instructions.
>
> Right.
>
>> >> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>> >> +{
>> >> + struct em_gio_priv *p = dev_id;
>> >> + unsigned long tmp, status;
>> >> + unsigned int offset;
>> >> +
>> >> + status = tmp = em_gio_read(p, GIO_MST);
>> >> + if (!status)
>> >> + return IRQ_NONE;
>> >> +
>> >> + while (status) {
>> >> + offset = __ffs(status);
>> >> + generic_handle_irq(p->irq_base + offset);
>> >
>> > Pleas use irqdomain to translate the IRQ numbers.
>>
>> Is this mandatory for regular platform devices not using DT?
>>
>> I don't object to your idea, but I was planning on adding that in my
>> DT feature patch.
>
> There is little value in delaying this if you already know how
> to do it. irq domains are useful for other reasons as well, e.g.
> for allowing to build with sparse IRQ, which is needed when we get
> to multiplatform kernels.
Hm, I thought I had sparse irq enabled already even though I'm not
using irq domains. I suppose the static range used with the legacy irq
domain is somewhat limited compared to the linear one if it gets
allocated dynamically, so I agree that static ranges (with or without
irq domains) certainly limit the usefulness of spare irqs.
I agree that there is little value delaying it, so i cooked up some
basic legacy irq domain support in V2.
>> >> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
>> >> +{
>> >> + return container_of(chip, struct em_gio_priv, gpio_chip);
>> >> +}
>> >
>> > inline?
>>
>> Sure, if you think that is absolutely necessary. May I ask, is it
>> important for you, or is it ok if I skip and keep this driver in line
>> with my other ones? =)
>
> I don't think it makes any difference in practice, other for people
> reading the code. Adding the 'inline' tells the reader that this is
> meant to be a trivial function that doesn't add much object code,
> and that the author was aware of that. I don't think it matters much
> either way.
I agree. In V2 for readability I decided to add inline to the
functions getting private data using container_of().
>> > Isn't there a new nice inline that will both request and
>> > remap a piece of memory?
>>
>> If so then that would be excellent. Especially together with
>> ioread/iowrite so the code can work both for IOMEM and PORT
>> transparently.
>
> We have a bunch of helpers, but I think none that does
> everything yet. io_iomap() locates and remaps the resource.
> devm_request_and_ioremap() does the request and the map, but
> doesn't pull it out of the device.
Let me know if you see any reason for not doing that. I'd be happy to
try to cook something up and convert my drivers one by one.
Thank you!
/ magnus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
@ 2012-05-15 16:14 ` Magnus Damm
0 siblings, 0 replies; 14+ messages in thread
From: Magnus Damm @ 2012-05-15 16:14 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Linus Walleij, linux-kernel, rjw, linus.walleij, linux-sh, horms,
grant.likely, lethal, olof
On Sat, May 12, 2012 at 10:18 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 11 May 2012, Magnus Damm wrote:
>> On Fri, May 11, 2012 at 10:10 PM, Linus Walleij
>> <linus.walleij@linaro.org> wrote:
>> > On Fri, May 11, 2012 at 11:41 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> >
>> >> +config GPIO_EM
>> >> + tristate "Emma Mobile GPIO"
>> >> + depends on ARM
>> >
>> > ARM really? Why should all ARM systems have the option of configuring in an Emma
>> > controller?
>>
>> Well, first of all, the Emma Mobile line of SoCs are ARM based. You
>> may think of MIPS based Emma devices. I don' t know so much about
>> them, but if you happen to know where I can find docs then I'd be very
>> interested in looking at them to see if I can share any of my recently
>> developed drivers.
>>
>> Not sure if your point is that this has nothing to do with the CPU
>> core itself - at least I usually prefer to omit CPU dependencies for
>> I/O devices and have as few dependencies as possible to get as much as
>> compile time testing done. In this particular case I've added ARM as
>> dependency because I was asked so for the UART driver 8250_em. Either
>> way is fine with me, so it's up to you GPIO maintainers to decide what
>> you want. As long as I can enable it on the ARM based SoC family I'm
>> happy. =)
>
> My preference is generally to define the dependencies rather broadly
> so you can build each driver on as many platforms as possible, and
> just leave them out of defconfig. This seems to be the common way to
> do things. Limiting by architecture makes some sense if you don't
> want to expose the driver to all architectures that might not be
> able to build it (e.g. s390 would not be able to build this one).
I believe that is rather close to what i prefer too. I kept the ARM dependency.
>> >> +static inline unsigned long em_gio_read(struct em_gio_priv *p, int offs)
>> >> +{
>> >> + if (offs < GIO_IDT0)
>> >> + return ioread32(p->base0 + offs);
>> >> + else
>> >> + return ioread32(p->base1 + (offs - GIO_IDT0));
>> >> +}
>> >> +
>> >> +static inline void em_gio_write(struct em_gio_priv *p, int offs,
>> >> + unsigned long value)
>> >> +{
>> >> + if (offs < GIO_IDT0)
>> >> + iowrite32(value, p->base0 + offs);
>> >> + else
>> >> + iowrite32(value, p->base1 + (offs - GIO_IDT0));
>> >> +}
>> >
>> > Isn't ioread()/iowrite() some ISA I/O-space operations? What's wrong with
>> > readl_[relaxed] and writel_[relaxed]()?
>>
>> Nothing wrong with readl/writel, I've just have a habit of using the
>> ioread/iowrite ones. I find the code that allows platforms to select
>> PORT or IOMEM in lib/iomap.c rather neat.
>
> For reference: The ioread family is defined to be a superset of the
> readl family and the inl family and to be compatible with the calling
> convention of readl, so it's correct to use it here, but it may be
> slower on architectures that require the lib/iomap.c wrappers. ARM
> does not use those wrappers on modern systems because the PIO space
> is memory mapped.
>
> For performance sensitive drivers that do not use DMA, it makes
> sense to use the readl_relaxed() family instead, because those
> omit the expensive barriers that protect us from overlapping with
> DMA.
>
> The main advantage of using readl/writel is that it's more common
> on ARM so reviewers don't have to wonder why this driver does things
> differently. Any driver shared with powerpc, mips or sh would likely
> use a different style. E.g. on powerpc, using readl/writel is considered
> wrong because it's only well-defined for PCI there.
Thanks for zooming out a bit and sharing your knowledge. As you know,
the code is not shared with any other architecture at this point, but
I decided to keep on using ioread/iowrite in V2 after all. I may
optimize the GPIO bits later if I need to hook up some big banging SPI
or I2C, but for now I think this is fine.
>> >> +static struct em_gio_priv *irq_to_priv(unsigned int irq)
>> >> +{
>> >> + struct irq_chip *chip = irq_get_chip(irq);
>> >> + return container_of(chip, struct em_gio_priv, irq_chip);
>> >> +}
>> >
>> > Should this be inline maybe?
>>
>> I have not checked if they actually turn out inline with my ARM
>> compiler, but gcc for SH was always rather good at deciding when to
>> inline by itself. Its a bit like likely()/unlikely() in my mind, it
>> may be good to give hints for the compiler but in the majority of the
>> cases the compiler will be fine by itself. I guess this particular
>> function may end up as just a few instructions.
>
> Right.
>
>> >> +static irqreturn_t em_gio_irq_handler(int irq, void *dev_id)
>> >> +{
>> >> + struct em_gio_priv *p = dev_id;
>> >> + unsigned long tmp, status;
>> >> + unsigned int offset;
>> >> +
>> >> + status = tmp = em_gio_read(p, GIO_MST);
>> >> + if (!status)
>> >> + return IRQ_NONE;
>> >> +
>> >> + while (status) {
>> >> + offset = __ffs(status);
>> >> + generic_handle_irq(p->irq_base + offset);
>> >
>> > Pleas use irqdomain to translate the IRQ numbers.
>>
>> Is this mandatory for regular platform devices not using DT?
>>
>> I don't object to your idea, but I was planning on adding that in my
>> DT feature patch.
>
> There is little value in delaying this if you already know how
> to do it. irq domains are useful for other reasons as well, e.g.
> for allowing to build with sparse IRQ, which is needed when we get
> to multiplatform kernels.
Hm, I thought I had sparse irq enabled already even though I'm not
using irq domains. I suppose the static range used with the legacy irq
domain is somewhat limited compared to the linear one if it gets
allocated dynamically, so I agree that static ranges (with or without
irq domains) certainly limit the usefulness of spare irqs.
I agree that there is little value delaying it, so i cooked up some
basic legacy irq domain support in V2.
>> >> +static struct em_gio_priv *gpio_to_priv(struct gpio_chip *chip)
>> >> +{
>> >> + return container_of(chip, struct em_gio_priv, gpio_chip);
>> >> +}
>> >
>> > inline?
>>
>> Sure, if you think that is absolutely necessary. May I ask, is it
>> important for you, or is it ok if I skip and keep this driver in line
>> with my other ones? =)
>
> I don't think it makes any difference in practice, other for people
> reading the code. Adding the 'inline' tells the reader that this is
> meant to be a trivial function that doesn't add much object code,
> and that the author was aware of that. I don't think it matters much
> either way.
I agree. In V2 for readability I decided to add inline to the
functions getting private data using container_of().
>> > Isn't there a new nice inline that will both request and
>> > remap a piece of memory?
>>
>> If so then that would be excellent. Especially together with
>> ioread/iowrite so the code can work both for IOMEM and PORT
>> transparently.
>
> We have a bunch of helpers, but I think none that does
> everything yet. io_iomap() locates and remaps the resource.
> devm_request_and_ioremap() does the request and the map, but
> doesn't pull it out of the device.
Let me know if you see any reason for not doing that. I'd be happy to
try to cook something up and convert my drivers one by one.
Thank you!
/ magnus
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] gpio: Emma Mobile GPIO driver
2012-05-15 16:14 ` Magnus Damm
(?)
@ 2012-05-15 19:13 ` Arnd Bergmann
-1 siblings, 0 replies; 14+ messages in thread
From: Arnd Bergmann @ 2012-05-15 19:13 UTC (permalink / raw)
To: Magnus Damm
Cc: Linus Walleij, linux-kernel, rjw, linus.walleij, linux-sh, horms,
grant.likely, lethal, olof
On Tuesday 15 May 2012, Magnus Damm wrote:
> >> > Isn't there a new nice inline that will both request and
> >> > remap a piece of memory?
> >>
> >> If so then that would be excellent. Especially together with
> >> ioread/iowrite so the code can work both for IOMEM and PORT
> >> transparently.
> >
> > We have a bunch of helpers, but I think none that does
> > everything yet. io_iomap() locates and remaps the resource.
> > devm_request_and_ioremap() does the request and the map, but
> > doesn't pull it out of the device.
>
> Let me know if you see any reason for not doing that. I'd be happy to
> try to cook something up and convert my drivers one by one.
I think it would be good to have it, so I don't mind if you give it
a go, but I fear that a lot of people cna have conflicting ideas on
what the perfect interface looks like ;-)
Arnd
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2012-05-15 19:13 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-05-11 9:41 [PATCH] gpio: Emma Mobile GPIO driver Magnus Damm
2012-05-11 9:41 ` Magnus Damm
2012-05-11 13:10 ` Linus Walleij
2012-05-11 13:10 ` Linus Walleij
2012-05-11 15:45 ` Magnus Damm
2012-05-11 15:45 ` Magnus Damm
2012-05-11 22:12 ` Linus Walleij
2012-05-11 22:12 ` Linus Walleij
2012-05-15 16:03 ` Magnus Damm
2012-05-15 16:03 ` Magnus Damm
2012-05-12 13:18 ` Arnd Bergmann
2012-05-15 16:14 ` Magnus Damm
2012-05-15 16:14 ` Magnus Damm
2012-05-15 19:13 ` Arnd Bergmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.