* [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-23 11:59 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-07-23 11:59 UTC (permalink / raw)
To: linux-kernel; +Cc: Grant Likely, Rob Herring, devicetree-discuss, Linus Walleij
This commit adds a driver for the Avionic Design N-bit GPIO expander.
The expander provides a variable number of GPIO pins with interrupt
support.
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Rob Herring <rob.herring@calxeda.com>
Cc: devicetree-discuss@lists.ozlabs.org
Cc: Linus Walleij <linus.walleij@stericsson.com>
Signed-off-by: Thierry Reding <thierry.reding@avionic-design.de>
---
Changes in v2:
- allow building the driver as a module
- assign of_node unconditionally
- use linear mapping IRQ domain
- properly cleanup IRQ domain
- add OF device table and annotate device tables
- emulate rising and falling edge triggers
- increase #gpio-cells to 2
- drop support for !OF
- use IS_ENABLED to conditionalize DEBUG_FS code
---
.../devicetree/bindings/gpio/gpio-adnp.txt | 38 ++
drivers/gpio/Kconfig | 18 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-adnp.c | 615 +++++++++++++++++++++
4 files changed, 672 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-adnp.txt
create mode 100644 drivers/gpio/gpio-adnp.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
new file mode 100644
index 0000000..513a18e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
@@ -0,0 +1,38 @@
+Avionic Design N-bit GPIO expander bindings
+
+Required properties:
+- compatible: should be "ad,gpio-adnp"
+- reg: The I2C slave address for this device.
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: Interrupt specifier for the controllers interrupt.
+- #gpio-cells: Should be 2. The first cell is the GPIO number and the
+ second cell is used to specify optional parameters:
+ - bit 0: polarity (0: normal, 1: inverted)
+- gpio-controller: Marks the device as a GPIO controller
+- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
+ whereas the second cell is used to specify flags:
+ bits[3:0] trigger type and level flags
+ 1 = low-to-high edge triggered
+ 2 = high-to-low edge triggered
+ 4 = active high level-sensitive
+ 8 = active low level-sensitive
+- interrupt-controller: Marks the device as an interrupt controller.
+- nr-gpios: The number of pins supported by the controller.
+
+Example:
+
+ gpioext: gpio-adnp@41 {
+ compatible = "ad,gpio-adnp";
+ reg = <0x41>;
+
+ interrupt-parent = <&gpio>;
+ interrupts = <160 1>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ nr-gpios = <64>;
+ };
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 502b5ea..d1b0f7d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
Say yes here to enable the adp5588 to be used as an interrupt
controller. It requires the driver to be built in the kernel.
+config GPIO_ADNP
+ tristate "Avionic Design N-bit GPIO expander"
+ depends on I2C && OF
+ help
+ This option enables support for N GPIOs found on Avionic Design
+ I2C GPIO expanders. The register space will be extended by powers
+ of two, so the controller will need to accomodate for that. For
+ example: if a controller provides 48 pins, 6 registers will be
+ enough to represent all pins, but the driver will assume a
+ register layout for 64 pins (8 registers).
+
+config GPIO_ADNP_IRQ
+ tristate "Interrupt controller support"
+ depends on GPIO_ADNP
+ help
+ Say yes here to enable the Avionic Design N-bit GPIO expander to
+ be used as an interrupt controller.
+
comment "PCI GPIO expanders:"
config GPIO_CS5535
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d370481..73affce 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o
obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
+obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
new file mode 100644
index 0000000..c122ff4
--- /dev/null
+++ b/drivers/gpio/gpio-adnp.c
@@ -0,0 +1,615 @@
+/*
+ * Copyright (C) 2011-2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+
+#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
+#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
+#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
+#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
+#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
+
+struct adnp {
+ struct i2c_client *client;
+ struct gpio_chip gpio;
+ unsigned int reg_shift;
+
+ struct mutex i2c_lock;
+
+ struct irq_domain *domain;
+ struct mutex irq_lock;
+
+ u8 *irq_mask;
+ u8 *irq_mask_cur;
+ u8 *irq_level;
+ u8 *irq_rise;
+ u8 *irq_fall;
+ u8 *irq_high;
+ u8 *irq_low;
+};
+
+static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
+{
+ int err;
+
+ err = i2c_smbus_read_byte_data(gpio->client, offset);
+ if (err < 0) {
+ dev_err(gpio->gpio.dev, "%s failed: %d\n",
+ "i2c_smbus_read_byte_data()", err);
+ return err;
+ }
+
+ *value = err;
+ return 0;
+}
+
+static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
+{
+ int err;
+
+ err = i2c_smbus_write_byte_data(gpio->client, offset, value);
+ if (err < 0) {
+ dev_err(gpio->gpio.dev, "%s failed: %d\n",
+ "i2c_smbus_write_byte_data()", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ u8 value;
+ int err;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + reg, &value);
+ if (err < 0)
+ goto out;
+
+ err = (value & BIT(pos)) ? 1 : 0;
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+ return err;
+}
+
+static void adnp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ int err;
+ u8 val;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + reg, &val);
+ if (err < 0)
+ goto out;
+
+ if (value)
+ val |= BIT(pos);
+ else
+ val &= ~BIT(pos);
+
+ adnp_write(gpio, GPIO_PLR(gpio) + reg, val);
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+}
+
+static int adnp_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ u8 value;
+ int err;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &value);
+ if (err < 0)
+ goto out;
+
+ value &= ~BIT(pos);
+
+ err = adnp_write(gpio, GPIO_DDR(gpio) + reg, value);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &value);
+ if (err < 0)
+ goto out;
+
+ if (err & BIT(pos))
+ err = -EACCES;
+
+ err = 0;
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+ return err;
+}
+
+static int adnp_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ int err;
+ u8 val;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &val);
+ if (err < 0)
+ goto out;
+
+ val |= BIT(pos);
+
+ err = adnp_write(gpio, GPIO_DDR(gpio) + reg, val);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &val);
+ if (err < 0)
+ goto out;
+
+ if (!(val & BIT(pos))) {
+ err = -EPERM;
+ goto out;
+ }
+
+ adnp_gpio_set(chip, offset, value);
+ err = 0;
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+ return err;
+}
+
+static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ u8 *base, *ddr, *plr, *ier, *isr, *ptr;
+ unsigned int regs, i, j;
+ int err;
+
+ regs = 1 << gpio->reg_shift;
+
+ base = kzalloc(regs * 5, GFP_KERNEL);
+ if (!base)
+ return;
+
+ ddr = base + (regs * 0);
+ plr = base + (regs * 1);
+ ier = base + (regs * 2);
+ isr = base + (regs * 3);
+ ptr = base + (regs * 4);
+
+ for (i = 0; i < regs; i++) {
+ err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
+ if (err < 0)
+ goto out;
+ }
+
+ for (i = 0; i < regs; i++) {
+ for (j = 0; j < 8; j++) {
+ unsigned int bit = (i << gpio->reg_shift) + j;
+ const char *direction = "input ";
+ const char *level = "low ";
+ const char *interrupt = "disabled";
+ const char *pending = "";
+
+ if (ddr[i] & BIT(j))
+ direction = "output";
+
+ if (plr[i] & BIT(j))
+ level = "high";
+
+ if (ier[i] & BIT(j))
+ interrupt = "enabled ";
+
+ if (isr[i] & BIT(j))
+ pending = "pending";
+
+ seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
+ direction, level, interrupt, pending);
+ }
+ }
+
+out:
+ kfree(base);
+}
+
+static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
+{
+ struct gpio_chip *chip = &gpio->gpio;
+
+ gpio->reg_shift = get_count_order(num_gpios) - 3;
+
+ chip->direction_input = adnp_gpio_direction_input;
+ chip->direction_output = adnp_gpio_direction_output;
+ chip->get = adnp_gpio_get;
+ chip->set = adnp_gpio_set;
+ chip->can_sleep = 1;
+
+ if (IS_ENABLED(CONFIG_DEBUG_FS))
+ chip->dbg_show = adnp_gpio_dbg_show;
+
+ chip->base = -1;
+ chip->ngpio = num_gpios;
+ chip->label = gpio->client->name;
+ chip->dev = &gpio->client->dev;
+ chip->of_node = chip->dev->of_node;
+ chip->owner = THIS_MODULE;
+
+ return 0;
+}
+
+static irqreturn_t adnp_irq(int irq, void *data)
+{
+ struct adnp *gpio = data;
+ unsigned int regs, i;
+
+ regs = 1 << gpio->reg_shift;
+
+ for (i = 0; i < regs; i++) {
+ unsigned int base = i << gpio->reg_shift, bit;
+ u8 changed, level, isr;
+ unsigned long pending;
+ int err;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + i, &level);
+ if (err < 0) {
+ mutex_unlock(&gpio->i2c_lock);
+ continue;
+ }
+
+ err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr);
+ if (err < 0) {
+ mutex_unlock(&gpio->i2c_lock);
+ continue;
+ }
+
+ mutex_unlock(&gpio->i2c_lock);
+
+ /* determine pins that changed levels */
+ changed = level ^ gpio->irq_level[i];
+
+ /* compute edge-triggered interrupts */
+ pending = changed & ((gpio->irq_fall[i] & ~level) |
+ (gpio->irq_rise[i] & level));
+
+ /* add in level-triggered interrupts */
+ pending |= (gpio->irq_high[i] & level) |
+ (gpio->irq_low[i] & ~level);
+
+ /* mask out non-pending and disabled interrupts */
+ pending &= isr & gpio->irq_mask_cur[i];
+
+ for_each_set_bit(bit, &pending, 8) {
+ unsigned int virq;
+ virq = irq_find_mapping(gpio->domain, base + bit);
+ handle_nested_irq(virq);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void adnp_irq_update_mask(struct adnp *gpio)
+{
+ unsigned int regs = 1 << gpio->reg_shift;
+ bool equal = true;
+ unsigned int i;
+
+ for (i = 0; i < regs; i++) {
+ if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
+ equal = false;
+ break;
+ }
+ }
+
+ if (equal)
+ return;
+
+ memcpy(gpio->irq_mask, gpio->irq_mask_cur, regs);
+
+ mutex_lock(&gpio->i2c_lock);
+
+ for (i = 0; i < regs; i++)
+ adnp_write(gpio, GPIO_IER(gpio) + i, gpio->irq_mask[i]);
+
+ mutex_unlock(&gpio->i2c_lock);
+}
+
+static int adnp_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ return irq_create_mapping(gpio->domain, offset);
+}
+
+static void adnp_irq_mask(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int reg = data->hwirq >> gpio->reg_shift;
+ unsigned int pos = data->hwirq & 7;
+
+ gpio->irq_mask_cur[reg] &= ~BIT(pos);
+}
+
+static void adnp_irq_unmask(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int reg = data->hwirq >> gpio->reg_shift;
+ unsigned int pos = data->hwirq & 7;
+
+ gpio->irq_mask_cur[reg] |= BIT(pos);
+}
+
+static int adnp_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int reg = data->hwirq >> gpio->reg_shift;
+ unsigned int pos = data->hwirq & 7;
+
+ if (type & IRQ_TYPE_EDGE_RISING)
+ gpio->irq_rise[reg] |= BIT(pos);
+ else
+ gpio->irq_rise[reg] &= ~BIT(pos);
+
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ gpio->irq_fall[reg] |= BIT(pos);
+ else
+ gpio->irq_fall[reg] &= ~BIT(pos);
+
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ gpio->irq_high[reg] |= BIT(pos);
+ else
+ gpio->irq_high[reg] &= ~BIT(pos);
+
+ if (type & IRQ_TYPE_LEVEL_LOW)
+ gpio->irq_low[reg] |= BIT(pos);
+ else
+ gpio->irq_low[reg] &= ~BIT(pos);
+
+ return 0;
+}
+
+static void adnp_irq_bus_lock(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int regs = 1 << gpio->reg_shift;
+
+ mutex_lock(&gpio->irq_lock);
+ memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
+}
+
+static void adnp_irq_bus_unlock(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+
+ adnp_irq_update_mask(gpio);
+ mutex_unlock(&gpio->irq_lock);
+}
+
+static struct irq_chip adnp_irq_chip = {
+ .name = "gpio-adnp",
+ .irq_mask = adnp_irq_mask,
+ .irq_unmask = adnp_irq_unmask,
+ .irq_set_type = adnp_irq_set_type,
+ .irq_bus_lock = adnp_irq_bus_lock,
+ .irq_bus_sync_unlock = adnp_irq_bus_unlock,
+};
+
+static int adnp_irq_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_data(irq, domain->host_data);
+ irq_set_chip(irq, &adnp_irq_chip);
+ irq_set_nested_thread(irq, true);
+
+#ifdef CONFIG_ARM
+ set_irq_flags(irq, IRQF_VALID);
+#else
+ irq_set_noprobe(irq);
+#endif
+
+ return 0;
+}
+
+static const struct irq_domain_ops adnp_irq_domain_ops = {
+ .map = adnp_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+static int adnp_irq_setup(struct adnp *gpio)
+{
+ unsigned int regs = 1 << gpio->reg_shift, i;
+ struct gpio_chip *chip = &gpio->gpio;
+ int err;
+
+ mutex_init(&gpio->irq_lock);
+
+ gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
+ if (!gpio->irq_mask)
+ return -ENOMEM;
+
+ gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
+ gpio->irq_level = gpio->irq_mask + (regs * 2);
+ gpio->irq_rise = gpio->irq_mask + (regs * 3);
+ gpio->irq_fall = gpio->irq_mask + (regs * 4);
+ gpio->irq_high = gpio->irq_mask + (regs * 5);
+ gpio->irq_low = gpio->irq_mask + (regs * 6);
+
+ for (i = 0; i < regs; i++) {
+ err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
+ if (err < 0)
+ return err;
+ }
+
+ gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
+ &adnp_irq_domain_ops, gpio);
+
+ err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ dev_name(chip->dev), gpio);
+ if (err != 0) {
+ dev_err(chip->dev, "can't request IRQ#%d: %d\n",
+ gpio->client->irq, err);
+ goto error;
+ }
+
+ gpio->gpio.to_irq = adnp_gpio_to_irq;
+ return 0;
+
+error:
+ irq_domain_remove(gpio->domain);
+ return err;
+}
+
+static void adnp_irq_teardown(struct adnp *gpio)
+{
+ unsigned int irq, i;
+
+ free_irq(gpio->client->irq, gpio);
+
+ for (i = 0; i < gpio->gpio.ngpio; i++) {
+ irq = irq_find_mapping(gpio->domain, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(gpio->domain);
+}
+
+static __devinit int adnp_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct adnp *gpio;
+ u32 num_gpios;
+ int err;
+
+ err = of_property_read_u32(client->dev.of_node, "nr-gpios",
+ &num_gpios);
+ if (err < 0)
+ return err;
+
+ client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
+ if (client->irq == NO_IRQ)
+ return -EPROBE_DEFER;
+
+ gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ mutex_init(&gpio->i2c_lock);
+ gpio->client = client;
+
+ err = adnp_gpio_setup(gpio, num_gpios);
+ if (err < 0)
+ return err;
+
+ if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
+ err = adnp_irq_setup(gpio);
+ if (err < 0)
+ goto teardown;
+ }
+
+ err = gpiochip_add(&gpio->gpio);
+ if (err < 0)
+ goto teardown;
+
+ i2c_set_clientdata(client, gpio);
+ return 0;
+
+teardown:
+ if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
+ adnp_irq_teardown(gpio);
+
+ return err;
+}
+
+static __devexit int adnp_i2c_remove(struct i2c_client *client)
+{
+ struct adnp *gpio = i2c_get_clientdata(client);
+ int err;
+
+ err = gpiochip_remove(&gpio->gpio);
+ if (err < 0) {
+ dev_err(&client->dev, "%s failed: %d\n", "gpiochip_remove()",
+ err);
+ return err;
+ }
+
+ if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
+ adnp_irq_teardown(gpio);
+
+ return 0;
+}
+
+static const struct i2c_device_id adnp_i2c_id[] __devinitconst = {
+ { "gpio-adnp" },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, adnp_i2c_id);
+
+static const struct of_device_id adnp_of_match[] __devinitconst = {
+ { .compatible = "ad,gpio-adnp", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, adnp_of_match);
+
+static struct i2c_driver adnp_i2c_driver = {
+ .driver = {
+ .name = "gpio-adnp",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(adnp_of_match),
+ },
+ .probe = adnp_i2c_probe,
+ .remove = __devexit_p(adnp_i2c_remove),
+ .id_table = adnp_i2c_id,
+};
+module_i2c_driver(adnp_i2c_driver);
+
+MODULE_DESCRIPTION("Avionic Design N-bit GPIO expander");
+MODULE_AUTHOR("Thierry Reding <thierry.reding@avionic-design.de>");
+MODULE_LICENSE("GPL");
--
1.7.11.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-23 11:59 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-07-23 11:59 UTC (permalink / raw)
To: linux-kernel-u79uwXL29TY76Z2rM5mHXA
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Linus Walleij, Rob Herring
This commit adds a driver for the Avionic Design N-bit GPIO expander.
The expander provides a variable number of GPIO pins with interrupt
support.
Cc: Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org>
Cc: Rob Herring <rob.herring-bsGFqQB8/DxBDgjK7y7TUQ@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: Linus Walleij <linus.walleij-0IS4wlFg1OjSUeElwK9/Pw@public.gmane.org>
Signed-off-by: Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>
---
Changes in v2:
- allow building the driver as a module
- assign of_node unconditionally
- use linear mapping IRQ domain
- properly cleanup IRQ domain
- add OF device table and annotate device tables
- emulate rising and falling edge triggers
- increase #gpio-cells to 2
- drop support for !OF
- use IS_ENABLED to conditionalize DEBUG_FS code
---
.../devicetree/bindings/gpio/gpio-adnp.txt | 38 ++
drivers/gpio/Kconfig | 18 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-adnp.c | 615 +++++++++++++++++++++
4 files changed, 672 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-adnp.txt
create mode 100644 drivers/gpio/gpio-adnp.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
new file mode 100644
index 0000000..513a18e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
@@ -0,0 +1,38 @@
+Avionic Design N-bit GPIO expander bindings
+
+Required properties:
+- compatible: should be "ad,gpio-adnp"
+- reg: The I2C slave address for this device.
+- interrupt-parent: phandle of the parent interrupt controller.
+- interrupts: Interrupt specifier for the controllers interrupt.
+- #gpio-cells: Should be 2. The first cell is the GPIO number and the
+ second cell is used to specify optional parameters:
+ - bit 0: polarity (0: normal, 1: inverted)
+- gpio-controller: Marks the device as a GPIO controller
+- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
+ whereas the second cell is used to specify flags:
+ bits[3:0] trigger type and level flags
+ 1 = low-to-high edge triggered
+ 2 = high-to-low edge triggered
+ 4 = active high level-sensitive
+ 8 = active low level-sensitive
+- interrupt-controller: Marks the device as an interrupt controller.
+- nr-gpios: The number of pins supported by the controller.
+
+Example:
+
+ gpioext: gpio-adnp@41 {
+ compatible = "ad,gpio-adnp";
+ reg = <0x41>;
+
+ interrupt-parent = <&gpio>;
+ interrupts = <160 1>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+
+ interrupt-controller;
+ #interrupt-cells = <2>;
+
+ nr-gpios = <64>;
+ };
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 502b5ea..d1b0f7d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
Say yes here to enable the adp5588 to be used as an interrupt
controller. It requires the driver to be built in the kernel.
+config GPIO_ADNP
+ tristate "Avionic Design N-bit GPIO expander"
+ depends on I2C && OF
+ help
+ This option enables support for N GPIOs found on Avionic Design
+ I2C GPIO expanders. The register space will be extended by powers
+ of two, so the controller will need to accomodate for that. For
+ example: if a controller provides 48 pins, 6 registers will be
+ enough to represent all pins, but the driver will assume a
+ register layout for 64 pins (8 registers).
+
+config GPIO_ADNP_IRQ
+ tristate "Interrupt controller support"
+ depends on GPIO_ADNP
+ help
+ Say yes here to enable the Avionic Design N-bit GPIO expander to
+ be used as an interrupt controller.
+
comment "PCI GPIO expanders:"
config GPIO_CS5535
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index d370481..73affce 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -10,6 +10,7 @@ obj-$(CONFIG_GPIO_GENERIC) += gpio-generic.o
obj-$(CONFIG_GPIO_74X164) += gpio-74x164.o
obj-$(CONFIG_GPIO_AB8500) += gpio-ab8500.o
+obj-$(CONFIG_GPIO_ADNP) += gpio-adnp.o
obj-$(CONFIG_GPIO_ADP5520) += gpio-adp5520.o
obj-$(CONFIG_GPIO_ADP5588) += gpio-adp5588.o
obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
new file mode 100644
index 0000000..c122ff4
--- /dev/null
+++ b/drivers/gpio/gpio-adnp.c
@@ -0,0 +1,615 @@
+/*
+ * Copyright (C) 2011-2012 Avionic Design GmbH
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/gpio.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/irqdomain.h>
+#include <linux/module.h>
+#include <linux/of_irq.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+
+#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
+#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
+#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
+#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
+#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
+
+struct adnp {
+ struct i2c_client *client;
+ struct gpio_chip gpio;
+ unsigned int reg_shift;
+
+ struct mutex i2c_lock;
+
+ struct irq_domain *domain;
+ struct mutex irq_lock;
+
+ u8 *irq_mask;
+ u8 *irq_mask_cur;
+ u8 *irq_level;
+ u8 *irq_rise;
+ u8 *irq_fall;
+ u8 *irq_high;
+ u8 *irq_low;
+};
+
+static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
+{
+ int err;
+
+ err = i2c_smbus_read_byte_data(gpio->client, offset);
+ if (err < 0) {
+ dev_err(gpio->gpio.dev, "%s failed: %d\n",
+ "i2c_smbus_read_byte_data()", err);
+ return err;
+ }
+
+ *value = err;
+ return 0;
+}
+
+static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
+{
+ int err;
+
+ err = i2c_smbus_write_byte_data(gpio->client, offset, value);
+ if (err < 0) {
+ dev_err(gpio->gpio.dev, "%s failed: %d\n",
+ "i2c_smbus_write_byte_data()", err);
+ return err;
+ }
+
+ return 0;
+}
+
+static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ u8 value;
+ int err;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + reg, &value);
+ if (err < 0)
+ goto out;
+
+ err = (value & BIT(pos)) ? 1 : 0;
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+ return err;
+}
+
+static void adnp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ int err;
+ u8 val;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + reg, &val);
+ if (err < 0)
+ goto out;
+
+ if (value)
+ val |= BIT(pos);
+ else
+ val &= ~BIT(pos);
+
+ adnp_write(gpio, GPIO_PLR(gpio) + reg, val);
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+}
+
+static int adnp_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ u8 value;
+ int err;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &value);
+ if (err < 0)
+ goto out;
+
+ value &= ~BIT(pos);
+
+ err = adnp_write(gpio, GPIO_DDR(gpio) + reg, value);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &value);
+ if (err < 0)
+ goto out;
+
+ if (err & BIT(pos))
+ err = -EACCES;
+
+ err = 0;
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+ return err;
+}
+
+static int adnp_gpio_direction_output(struct gpio_chip *chip, unsigned offset,
+ int value)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ unsigned int reg = offset >> gpio->reg_shift;
+ unsigned int pos = offset & 7;
+ int err;
+ u8 val;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &val);
+ if (err < 0)
+ goto out;
+
+ val |= BIT(pos);
+
+ err = adnp_write(gpio, GPIO_DDR(gpio) + reg, val);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_DDR(gpio) + reg, &val);
+ if (err < 0)
+ goto out;
+
+ if (!(val & BIT(pos))) {
+ err = -EPERM;
+ goto out;
+ }
+
+ adnp_gpio_set(chip, offset, value);
+ err = 0;
+
+out:
+ mutex_unlock(&gpio->i2c_lock);
+ return err;
+}
+
+static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ u8 *base, *ddr, *plr, *ier, *isr, *ptr;
+ unsigned int regs, i, j;
+ int err;
+
+ regs = 1 << gpio->reg_shift;
+
+ base = kzalloc(regs * 5, GFP_KERNEL);
+ if (!base)
+ return;
+
+ ddr = base + (regs * 0);
+ plr = base + (regs * 1);
+ ier = base + (regs * 2);
+ isr = base + (regs * 3);
+ ptr = base + (regs * 4);
+
+ for (i = 0; i < regs; i++) {
+ err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
+ if (err < 0)
+ goto out;
+
+ err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
+ if (err < 0)
+ goto out;
+ }
+
+ for (i = 0; i < regs; i++) {
+ for (j = 0; j < 8; j++) {
+ unsigned int bit = (i << gpio->reg_shift) + j;
+ const char *direction = "input ";
+ const char *level = "low ";
+ const char *interrupt = "disabled";
+ const char *pending = "";
+
+ if (ddr[i] & BIT(j))
+ direction = "output";
+
+ if (plr[i] & BIT(j))
+ level = "high";
+
+ if (ier[i] & BIT(j))
+ interrupt = "enabled ";
+
+ if (isr[i] & BIT(j))
+ pending = "pending";
+
+ seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
+ direction, level, interrupt, pending);
+ }
+ }
+
+out:
+ kfree(base);
+}
+
+static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
+{
+ struct gpio_chip *chip = &gpio->gpio;
+
+ gpio->reg_shift = get_count_order(num_gpios) - 3;
+
+ chip->direction_input = adnp_gpio_direction_input;
+ chip->direction_output = adnp_gpio_direction_output;
+ chip->get = adnp_gpio_get;
+ chip->set = adnp_gpio_set;
+ chip->can_sleep = 1;
+
+ if (IS_ENABLED(CONFIG_DEBUG_FS))
+ chip->dbg_show = adnp_gpio_dbg_show;
+
+ chip->base = -1;
+ chip->ngpio = num_gpios;
+ chip->label = gpio->client->name;
+ chip->dev = &gpio->client->dev;
+ chip->of_node = chip->dev->of_node;
+ chip->owner = THIS_MODULE;
+
+ return 0;
+}
+
+static irqreturn_t adnp_irq(int irq, void *data)
+{
+ struct adnp *gpio = data;
+ unsigned int regs, i;
+
+ regs = 1 << gpio->reg_shift;
+
+ for (i = 0; i < regs; i++) {
+ unsigned int base = i << gpio->reg_shift, bit;
+ u8 changed, level, isr;
+ unsigned long pending;
+ int err;
+
+ mutex_lock(&gpio->i2c_lock);
+
+ err = adnp_read(gpio, GPIO_PLR(gpio) + i, &level);
+ if (err < 0) {
+ mutex_unlock(&gpio->i2c_lock);
+ continue;
+ }
+
+ err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr);
+ if (err < 0) {
+ mutex_unlock(&gpio->i2c_lock);
+ continue;
+ }
+
+ mutex_unlock(&gpio->i2c_lock);
+
+ /* determine pins that changed levels */
+ changed = level ^ gpio->irq_level[i];
+
+ /* compute edge-triggered interrupts */
+ pending = changed & ((gpio->irq_fall[i] & ~level) |
+ (gpio->irq_rise[i] & level));
+
+ /* add in level-triggered interrupts */
+ pending |= (gpio->irq_high[i] & level) |
+ (gpio->irq_low[i] & ~level);
+
+ /* mask out non-pending and disabled interrupts */
+ pending &= isr & gpio->irq_mask_cur[i];
+
+ for_each_set_bit(bit, &pending, 8) {
+ unsigned int virq;
+ virq = irq_find_mapping(gpio->domain, base + bit);
+ handle_nested_irq(virq);
+ }
+ }
+
+ return IRQ_HANDLED;
+}
+
+static void adnp_irq_update_mask(struct adnp *gpio)
+{
+ unsigned int regs = 1 << gpio->reg_shift;
+ bool equal = true;
+ unsigned int i;
+
+ for (i = 0; i < regs; i++) {
+ if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
+ equal = false;
+ break;
+ }
+ }
+
+ if (equal)
+ return;
+
+ memcpy(gpio->irq_mask, gpio->irq_mask_cur, regs);
+
+ mutex_lock(&gpio->i2c_lock);
+
+ for (i = 0; i < regs; i++)
+ adnp_write(gpio, GPIO_IER(gpio) + i, gpio->irq_mask[i]);
+
+ mutex_unlock(&gpio->i2c_lock);
+}
+
+static int adnp_gpio_to_irq(struct gpio_chip *chip, unsigned offset)
+{
+ struct adnp *gpio = container_of(chip, struct adnp, gpio);
+ return irq_create_mapping(gpio->domain, offset);
+}
+
+static void adnp_irq_mask(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int reg = data->hwirq >> gpio->reg_shift;
+ unsigned int pos = data->hwirq & 7;
+
+ gpio->irq_mask_cur[reg] &= ~BIT(pos);
+}
+
+static void adnp_irq_unmask(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int reg = data->hwirq >> gpio->reg_shift;
+ unsigned int pos = data->hwirq & 7;
+
+ gpio->irq_mask_cur[reg] |= BIT(pos);
+}
+
+static int adnp_irq_set_type(struct irq_data *data, unsigned int type)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int reg = data->hwirq >> gpio->reg_shift;
+ unsigned int pos = data->hwirq & 7;
+
+ if (type & IRQ_TYPE_EDGE_RISING)
+ gpio->irq_rise[reg] |= BIT(pos);
+ else
+ gpio->irq_rise[reg] &= ~BIT(pos);
+
+ if (type & IRQ_TYPE_EDGE_FALLING)
+ gpio->irq_fall[reg] |= BIT(pos);
+ else
+ gpio->irq_fall[reg] &= ~BIT(pos);
+
+ if (type & IRQ_TYPE_LEVEL_HIGH)
+ gpio->irq_high[reg] |= BIT(pos);
+ else
+ gpio->irq_high[reg] &= ~BIT(pos);
+
+ if (type & IRQ_TYPE_LEVEL_LOW)
+ gpio->irq_low[reg] |= BIT(pos);
+ else
+ gpio->irq_low[reg] &= ~BIT(pos);
+
+ return 0;
+}
+
+static void adnp_irq_bus_lock(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+ unsigned int regs = 1 << gpio->reg_shift;
+
+ mutex_lock(&gpio->irq_lock);
+ memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
+}
+
+static void adnp_irq_bus_unlock(struct irq_data *data)
+{
+ struct adnp *gpio = irq_data_get_irq_chip_data(data);
+
+ adnp_irq_update_mask(gpio);
+ mutex_unlock(&gpio->irq_lock);
+}
+
+static struct irq_chip adnp_irq_chip = {
+ .name = "gpio-adnp",
+ .irq_mask = adnp_irq_mask,
+ .irq_unmask = adnp_irq_unmask,
+ .irq_set_type = adnp_irq_set_type,
+ .irq_bus_lock = adnp_irq_bus_lock,
+ .irq_bus_sync_unlock = adnp_irq_bus_unlock,
+};
+
+static int adnp_irq_map(struct irq_domain *domain, unsigned int irq,
+ irq_hw_number_t hwirq)
+{
+ irq_set_chip_data(irq, domain->host_data);
+ irq_set_chip(irq, &adnp_irq_chip);
+ irq_set_nested_thread(irq, true);
+
+#ifdef CONFIG_ARM
+ set_irq_flags(irq, IRQF_VALID);
+#else
+ irq_set_noprobe(irq);
+#endif
+
+ return 0;
+}
+
+static const struct irq_domain_ops adnp_irq_domain_ops = {
+ .map = adnp_irq_map,
+ .xlate = irq_domain_xlate_twocell,
+};
+
+static int adnp_irq_setup(struct adnp *gpio)
+{
+ unsigned int regs = 1 << gpio->reg_shift, i;
+ struct gpio_chip *chip = &gpio->gpio;
+ int err;
+
+ mutex_init(&gpio->irq_lock);
+
+ gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
+ if (!gpio->irq_mask)
+ return -ENOMEM;
+
+ gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
+ gpio->irq_level = gpio->irq_mask + (regs * 2);
+ gpio->irq_rise = gpio->irq_mask + (regs * 3);
+ gpio->irq_fall = gpio->irq_mask + (regs * 4);
+ gpio->irq_high = gpio->irq_mask + (regs * 5);
+ gpio->irq_low = gpio->irq_mask + (regs * 6);
+
+ for (i = 0; i < regs; i++) {
+ err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
+ if (err < 0)
+ return err;
+ }
+
+ gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
+ &adnp_irq_domain_ops, gpio);
+
+ err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
+ IRQF_TRIGGER_RISING | IRQF_ONESHOT,
+ dev_name(chip->dev), gpio);
+ if (err != 0) {
+ dev_err(chip->dev, "can't request IRQ#%d: %d\n",
+ gpio->client->irq, err);
+ goto error;
+ }
+
+ gpio->gpio.to_irq = adnp_gpio_to_irq;
+ return 0;
+
+error:
+ irq_domain_remove(gpio->domain);
+ return err;
+}
+
+static void adnp_irq_teardown(struct adnp *gpio)
+{
+ unsigned int irq, i;
+
+ free_irq(gpio->client->irq, gpio);
+
+ for (i = 0; i < gpio->gpio.ngpio; i++) {
+ irq = irq_find_mapping(gpio->domain, i);
+ if (irq > 0)
+ irq_dispose_mapping(irq);
+ }
+
+ irq_domain_remove(gpio->domain);
+}
+
+static __devinit int adnp_i2c_probe(struct i2c_client *client,
+ const struct i2c_device_id *id)
+{
+ struct adnp *gpio;
+ u32 num_gpios;
+ int err;
+
+ err = of_property_read_u32(client->dev.of_node, "nr-gpios",
+ &num_gpios);
+ if (err < 0)
+ return err;
+
+ client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
+ if (client->irq == NO_IRQ)
+ return -EPROBE_DEFER;
+
+ gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
+ if (!gpio)
+ return -ENOMEM;
+
+ mutex_init(&gpio->i2c_lock);
+ gpio->client = client;
+
+ err = adnp_gpio_setup(gpio, num_gpios);
+ if (err < 0)
+ return err;
+
+ if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
+ err = adnp_irq_setup(gpio);
+ if (err < 0)
+ goto teardown;
+ }
+
+ err = gpiochip_add(&gpio->gpio);
+ if (err < 0)
+ goto teardown;
+
+ i2c_set_clientdata(client, gpio);
+ return 0;
+
+teardown:
+ if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
+ adnp_irq_teardown(gpio);
+
+ return err;
+}
+
+static __devexit int adnp_i2c_remove(struct i2c_client *client)
+{
+ struct adnp *gpio = i2c_get_clientdata(client);
+ int err;
+
+ err = gpiochip_remove(&gpio->gpio);
+ if (err < 0) {
+ dev_err(&client->dev, "%s failed: %d\n", "gpiochip_remove()",
+ err);
+ return err;
+ }
+
+ if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
+ adnp_irq_teardown(gpio);
+
+ return 0;
+}
+
+static const struct i2c_device_id adnp_i2c_id[] __devinitconst = {
+ { "gpio-adnp" },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, adnp_i2c_id);
+
+static const struct of_device_id adnp_of_match[] __devinitconst = {
+ { .compatible = "ad,gpio-adnp", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, adnp_of_match);
+
+static struct i2c_driver adnp_i2c_driver = {
+ .driver = {
+ .name = "gpio-adnp",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(adnp_of_match),
+ },
+ .probe = adnp_i2c_probe,
+ .remove = __devexit_p(adnp_i2c_remove),
+ .id_table = adnp_i2c_id,
+};
+module_i2c_driver(adnp_i2c_driver);
+
+MODULE_DESCRIPTION("Avionic Design N-bit GPIO expander");
+MODULE_AUTHOR("Thierry Reding <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org>");
+MODULE_LICENSE("GPL");
--
1.7.11.2
^ permalink raw reply related [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-29 17:13 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-07-29 17:13 UTC (permalink / raw)
To: Thierry Reding, Grant Likely, Arnd Bergmann
Cc: linux-kernel, devicetree-discuss, Linus Walleij, Rob Herring,
Wolfram Sang
On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> This commit adds a driver for the Avionic Design N-bit GPIO expander.
> The expander provides a variable number of GPIO pins with interrupt
> support.
(...)
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> new file mode 100644
> index 0000000..513a18e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> @@ -0,0 +1,38 @@
> +Avionic Design N-bit GPIO expander bindings
> +
> +Required properties:
> +- compatible: should be "ad,gpio-adnp"
> +- reg: The I2C slave address for this device.
> +- interrupt-parent: phandle of the parent interrupt controller.
> +- interrupts: Interrupt specifier for the controllers interrupt.
> +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
> + second cell is used to specify optional parameters:
> + - bit 0: polarity (0: normal, 1: inverted)
> +- gpio-controller: Marks the device as a GPIO controller
> +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> + whereas the second cell is used to specify flags:
> + bits[3:0] trigger type and level flags
> + 1 = low-to-high edge triggered
> + 2 = high-to-low edge triggered
> + 4 = active high level-sensitive
> + 8 = active low level-sensitive
Why on earth would a bunch of flags be an "interrupt cell"?
Maybe there is something about DT bindings I don't get so
please educate me.
I can see that OMAP is doing this, but is it a good idea?
I really need Rob/Grant to comment on this.
> +- interrupt-controller: Marks the device as an interrupt controller.
> +- nr-gpios: The number of pins supported by the controller.
These two last things look very generic, like something every GPIO
driver could want to expose.
I'd really like to have Grant's word on GPIO DT bindings and how these
should look, I had some discussion with Wolfram (the I2C maintainer)
about bindings turning out less generic than they ought to be, so we
need some discussion on this.
Arnd recently consolidated some MMC props, maybe we need to do
the same for GPIO drivers.
(...)
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 502b5ea..d1b0f7d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
> Say yes here to enable the adp5588 to be used as an interrupt
> controller. It requires the driver to be built in the kernel.
>
> +config GPIO_ADNP
> + tristate "Avionic Design N-bit GPIO expander"
> + depends on I2C && OF
> + help
> + This option enables support for N GPIOs found on Avionic Design
> + I2C GPIO expanders. The register space will be extended by powers
> + of two, so the controller will need to accomodate for that. For
> + example: if a controller provides 48 pins, 6 registers will be
> + enough to represent all pins, but the driver will assume a
> + register layout for 64 pins (8 registers).
> +
> +config GPIO_ADNP_IRQ
> + tristate "Interrupt controller support"
> + depends on GPIO_ADNP
> + help
> + Say yes here to enable the Avionic Design N-bit GPIO expander to
> + be used as an interrupt controller.
First: please describe the usecase where the Avionic driver is used
without making use of the IRQ, and *why* it should be possible
to configure this out. E.g. is there a hardware which isn't using the
IRQ portions? If there is no non-irq usecase just drop this
config option.
If you're keeping it:
Now this is going to appear as a single line under the ADNP driver
config entry saying "Interrupt controller support", which is too
generic.
Please make it expand to a submenu so that it gets indented below the
controller driver.
(...)
> diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
> new file mode 100644
> index 0000000..c122ff4
> --- /dev/null
> +++ b/drivers/gpio/gpio-adnp.c
> @@ -0,0 +1,615 @@
> +/*
> + * Copyright (C) 2011-2012 Avionic Design GmbH
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
Good!
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +
> +#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
> +#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
> +#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
> +#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
> +#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
> +
> +struct adnp {
> + struct i2c_client *client;
> + struct gpio_chip gpio;
> + unsigned int reg_shift;
> +
> + struct mutex i2c_lock;
> +
> + struct irq_domain *domain;
> + struct mutex irq_lock;
> +
> + u8 *irq_mask;
> + u8 *irq_mask_cur;
> + u8 *irq_level;
> + u8 *irq_rise;
> + u8 *irq_fall;
> + u8 *irq_high;
> + u8 *irq_low;
Some or all of this looks like a regmap reimplementation, see below.
> +};
> +
> +static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
I don't know why you name the struct adnp variable "gpio" everywhere,
that is quite ambigous. Why not name this variable "adnp" consequently
everywhere?
> +{
> + int err;
> +
> + err = i2c_smbus_read_byte_data(gpio->client, offset);
> + if (err < 0) {
> + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> + "i2c_smbus_read_byte_data()", err);
> + return err;
> + }
> +
> + *value = err;
> + return 0;
> +}
> +
> +static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
> +{
> + int err;
> +
> + err = i2c_smbus_write_byte_data(gpio->client, offset, value);
> + if (err < 0) {
> + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> + "i2c_smbus_write_byte_data()", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + struct adnp *gpio = container_of(chip, struct adnp, gpio);
When I do this at several places in a driver I usually define
a macro like
#define to_adnp(foo) container_of(foo, struct adnp, gpio)
Used like so:
struct adnp *adnp = to_adnp(chip);
> + unsigned int reg = offset >> gpio->reg_shift;
> + unsigned int pos = offset & 7;
> + u8 value;
> + int err;
> +
> + mutex_lock(&gpio->i2c_lock);
The point of taking this mutex here avoids me.
adnp_read() only performs a single transaction.
> +
> + err = adnp_read(gpio, GPIO_PLR(gpio) + reg, &value);
> + if (err < 0)
> + goto out;
> +
> + err = (value & BIT(pos)) ? 1 : 0;
> +
> +out:
> + mutex_unlock(&gpio->i2c_lock);
> + return err;
> +}
> +
> +static void adnp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct adnp *gpio = container_of(chip, struct adnp, gpio);
to_adnp()
> + unsigned int reg = offset >> gpio->reg_shift;
> + unsigned int pos = offset & 7;
> + int err;
> + u8 val;
> +
> + mutex_lock(&gpio->i2c_lock);
But here the mutex is needed, I can see that...
(etc)
(...)
> +static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> + struct adnp *gpio = container_of(chip, struct adnp, gpio);
> + u8 *base, *ddr, *plr, *ier, *isr, *ptr;
> + unsigned int regs, i, j;
> + int err;
> +
> + regs = 1 << gpio->reg_shift;
> +
> + base = kzalloc(regs * 5, GFP_KERNEL);
Why kzalloc()/kfree() when you can just use a
static u8 base[N];
where N is the max number of registers on any HW instead?
> + if (!base)
> + return;
> +
> + ddr = base + (regs * 0);
> + plr = base + (regs * 1);
> + ier = base + (regs * 2);
> + isr = base + (regs * 3);
> + ptr = base + (regs * 4);
> +
> + for (i = 0; i < regs; i++) {
> + err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
> + if (err < 0)
> + goto out;
> + }
You must take the mutex around this for-loop, don't you?
Else browing debugfs will sporadically corrupt transfers,
or something like that. Or was this intended to cut into any
ongoing transfer couple?
> + for (i = 0; i < regs; i++) {
> + for (j = 0; j < 8; j++) {
> + unsigned int bit = (i << gpio->reg_shift) + j;
> + const char *direction = "input ";
> + const char *level = "low ";
> + const char *interrupt = "disabled";
> + const char *pending = "";
> +
> + if (ddr[i] & BIT(j))
> + direction = "output";
> +
> + if (plr[i] & BIT(j))
> + level = "high";
> +
> + if (ier[i] & BIT(j))
> + interrupt = "enabled ";
> +
> + if (isr[i] & BIT(j))
> + pending = "pending";
> +
> + seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
> + direction, level, interrupt, pending);
> + }
> + }
> +
> +out:
> + kfree(base);
> +}
> +
> +static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
> +{
> + struct gpio_chip *chip = &gpio->gpio;
> +
> + gpio->reg_shift = get_count_order(num_gpios) - 3;
> +
> + chip->direction_input = adnp_gpio_direction_input;
> + chip->direction_output = adnp_gpio_direction_output;
> + chip->get = adnp_gpio_get;
> + chip->set = adnp_gpio_set;
> + chip->can_sleep = 1;
> + if (IS_ENABLED(CONFIG_DEBUG_FS))
> + chip->dbg_show = adnp_gpio_dbg_show;
> +
> + chip->base = -1;
> + chip->ngpio = num_gpios;
> + chip->label = gpio->client->name;
> + chip->dev = &gpio->client->dev;
> + chip->of_node = chip->dev->of_node;
> + chip->owner = THIS_MODULE;
Usually we define the struct gpio_chip as a static const and have
the state containter hold a static const struct gpio_chip *chip;
entry assigned to point to the static const at probe time.
All other GPIO drivers #ifdef their debugfs code, this probably
works fine too, but never saw it before.
I'm ambivalent about this, I sort of like it because it avoids
#ifdefs and also makes sure the code is always compiled,
so if you like it like this, keep it.
(...)
> +static void adnp_irq_update_mask(struct adnp *gpio)
> +{
> + unsigned int regs = 1 << gpio->reg_shift;
> + bool equal = true;
> + unsigned int i;
> +
> + for (i = 0; i < regs; i++) {
> + if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
> + equal = false;
> + break;
> + }
> + }
This is not looking good. It looks like you're reimplementing
parts of regmap.
In that case, please use <linux/regmap.h> to cache registers
instead.
But I'm not sure ...
(...)
> +static void adnp_irq_bus_lock(struct irq_data *data)
> +{
> + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> + unsigned int regs = 1 << gpio->reg_shift;
> +
> + mutex_lock(&gpio->irq_lock);
> + memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
> +}
> +
> +static void adnp_irq_bus_unlock(struct irq_data *data)
> +{
> + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> +
> + adnp_irq_update_mask(gpio);
> + mutex_unlock(&gpio->irq_lock);
> +}
Actually I'm not following why the IRQ mask registers are shadowed
at bus_lock and restored at bus_unlock().
A comment describing why that's needed and how it works is
definately needed.
(...)
> +static const struct irq_domain_ops adnp_irq_domain_ops = {
> + .map = adnp_irq_map,
> + .xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int adnp_irq_setup(struct adnp *gpio)
> +{
> + unsigned int regs = 1 << gpio->reg_shift, i;
> + struct gpio_chip *chip = &gpio->gpio;
> + int err;
> +
> + mutex_init(&gpio->irq_lock);
> +
> + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> + if (!gpio->irq_mask)
> + return -ENOMEM;
> +
> + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> + gpio->irq_level = gpio->irq_mask + (regs * 2);
> + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> + gpio->irq_high = gpio->irq_mask + (regs * 5);
> + gpio->irq_low = gpio->irq_mask + (regs * 6);
I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
here? Explain with some comment atleast.
> +
> + for (i = 0; i < regs; i++) {
> + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
> + if (err < 0)
> + return err;
> + }
Looks like regmap reimplementation.
> + gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
> + &adnp_irq_domain_ops, gpio);
> +
> + err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + dev_name(chip->dev), gpio);
Since you're using devm_* above use it here too:
devm_request_threaded_irq().
> + if (err != 0) {
> + dev_err(chip->dev, "can't request IRQ#%d: %d\n",
> + gpio->client->irq, err);
> + goto error;
> + }
> +
> + gpio->gpio.to_irq = adnp_gpio_to_irq;
> + return 0;
> +
> +error:
> + irq_domain_remove(gpio->domain);
> + return err;
> +}
> +
> +static void adnp_irq_teardown(struct adnp *gpio)
> +{
> + unsigned int irq, i;
> +
> + free_irq(gpio->client->irq, gpio);
If you're using devm to grab the IRQ this is not needed.
> +
> + for (i = 0; i < gpio->gpio.ngpio; i++) {
> + irq = irq_find_mapping(gpio->domain, i);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(gpio->domain);
> +}
> +
> +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct adnp *gpio;
> + u32 num_gpios;
> + int err;
> +
> + err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> + &num_gpios);
> + if (err < 0)
> + return err;
> +
> + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> + if (client->irq == NO_IRQ)
Just if (!client->irq) since NO_IRQ is 0 nowadays.
> + return -EPROBE_DEFER;
Why would you defer in this case? If the IRQ controller appear later
than the GPIO driver?
> + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + mutex_init(&gpio->i2c_lock);
> + gpio->client = client;
> +
> + err = adnp_gpio_setup(gpio, num_gpios);
> + if (err < 0)
> + return err;
> +
> + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
> + err = adnp_irq_setup(gpio);
> + if (err < 0)
> + goto teardown;
> + }
And that activates the question why this should be conditional,
please elaborate.
> + err = gpiochip_add(&gpio->gpio);
> + if (err < 0)
> + goto teardown;
> +
> + i2c_set_clientdata(client, gpio);
> + return 0;
> +
> +teardown:
> + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
> + adnp_irq_teardown(gpio);
Here too.
> +
> + return err;
> +}
Most of the driver looks very good though! The code is
clean and easy to read and maintain.
But we need to iron out the details.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-29 17:13 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-07-29 17:13 UTC (permalink / raw)
To: Thierry Reding, Grant Likely, Arnd Bergmann
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Wolfram Sang,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Linus Walleij
On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> This commit adds a driver for the Avionic Design N-bit GPIO expander.
> The expander provides a variable number of GPIO pins with interrupt
> support.
(...)
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> new file mode 100644
> index 0000000..513a18e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> @@ -0,0 +1,38 @@
> +Avionic Design N-bit GPIO expander bindings
> +
> +Required properties:
> +- compatible: should be "ad,gpio-adnp"
> +- reg: The I2C slave address for this device.
> +- interrupt-parent: phandle of the parent interrupt controller.
> +- interrupts: Interrupt specifier for the controllers interrupt.
> +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
> + second cell is used to specify optional parameters:
> + - bit 0: polarity (0: normal, 1: inverted)
> +- gpio-controller: Marks the device as a GPIO controller
> +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> + whereas the second cell is used to specify flags:
> + bits[3:0] trigger type and level flags
> + 1 = low-to-high edge triggered
> + 2 = high-to-low edge triggered
> + 4 = active high level-sensitive
> + 8 = active low level-sensitive
Why on earth would a bunch of flags be an "interrupt cell"?
Maybe there is something about DT bindings I don't get so
please educate me.
I can see that OMAP is doing this, but is it a good idea?
I really need Rob/Grant to comment on this.
> +- interrupt-controller: Marks the device as an interrupt controller.
> +- nr-gpios: The number of pins supported by the controller.
These two last things look very generic, like something every GPIO
driver could want to expose.
I'd really like to have Grant's word on GPIO DT bindings and how these
should look, I had some discussion with Wolfram (the I2C maintainer)
about bindings turning out less generic than they ought to be, so we
need some discussion on this.
Arnd recently consolidated some MMC props, maybe we need to do
the same for GPIO drivers.
(...)
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 502b5ea..d1b0f7d 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
> Say yes here to enable the adp5588 to be used as an interrupt
> controller. It requires the driver to be built in the kernel.
>
> +config GPIO_ADNP
> + tristate "Avionic Design N-bit GPIO expander"
> + depends on I2C && OF
> + help
> + This option enables support for N GPIOs found on Avionic Design
> + I2C GPIO expanders. The register space will be extended by powers
> + of two, so the controller will need to accomodate for that. For
> + example: if a controller provides 48 pins, 6 registers will be
> + enough to represent all pins, but the driver will assume a
> + register layout for 64 pins (8 registers).
> +
> +config GPIO_ADNP_IRQ
> + tristate "Interrupt controller support"
> + depends on GPIO_ADNP
> + help
> + Say yes here to enable the Avionic Design N-bit GPIO expander to
> + be used as an interrupt controller.
First: please describe the usecase where the Avionic driver is used
without making use of the IRQ, and *why* it should be possible
to configure this out. E.g. is there a hardware which isn't using the
IRQ portions? If there is no non-irq usecase just drop this
config option.
If you're keeping it:
Now this is going to appear as a single line under the ADNP driver
config entry saying "Interrupt controller support", which is too
generic.
Please make it expand to a submenu so that it gets indented below the
controller driver.
(...)
> diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
> new file mode 100644
> index 0000000..c122ff4
> --- /dev/null
> +++ b/drivers/gpio/gpio-adnp.c
> @@ -0,0 +1,615 @@
> +/*
> + * Copyright (C) 2011-2012 Avionic Design GmbH
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#include <linux/gpio.h>
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>
Good!
> +#include <linux/module.h>
> +#include <linux/of_irq.h>
> +#include <linux/seq_file.h>
> +#include <linux/slab.h>
> +
> +#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
> +#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
> +#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
> +#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
> +#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
> +
> +struct adnp {
> + struct i2c_client *client;
> + struct gpio_chip gpio;
> + unsigned int reg_shift;
> +
> + struct mutex i2c_lock;
> +
> + struct irq_domain *domain;
> + struct mutex irq_lock;
> +
> + u8 *irq_mask;
> + u8 *irq_mask_cur;
> + u8 *irq_level;
> + u8 *irq_rise;
> + u8 *irq_fall;
> + u8 *irq_high;
> + u8 *irq_low;
Some or all of this looks like a regmap reimplementation, see below.
> +};
> +
> +static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
I don't know why you name the struct adnp variable "gpio" everywhere,
that is quite ambigous. Why not name this variable "adnp" consequently
everywhere?
> +{
> + int err;
> +
> + err = i2c_smbus_read_byte_data(gpio->client, offset);
> + if (err < 0) {
> + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> + "i2c_smbus_read_byte_data()", err);
> + return err;
> + }
> +
> + *value = err;
> + return 0;
> +}
> +
> +static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
> +{
> + int err;
> +
> + err = i2c_smbus_write_byte_data(gpio->client, offset, value);
> + if (err < 0) {
> + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> + "i2c_smbus_write_byte_data()", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
> +{
> + struct adnp *gpio = container_of(chip, struct adnp, gpio);
When I do this at several places in a driver I usually define
a macro like
#define to_adnp(foo) container_of(foo, struct adnp, gpio)
Used like so:
struct adnp *adnp = to_adnp(chip);
> + unsigned int reg = offset >> gpio->reg_shift;
> + unsigned int pos = offset & 7;
> + u8 value;
> + int err;
> +
> + mutex_lock(&gpio->i2c_lock);
The point of taking this mutex here avoids me.
adnp_read() only performs a single transaction.
> +
> + err = adnp_read(gpio, GPIO_PLR(gpio) + reg, &value);
> + if (err < 0)
> + goto out;
> +
> + err = (value & BIT(pos)) ? 1 : 0;
> +
> +out:
> + mutex_unlock(&gpio->i2c_lock);
> + return err;
> +}
> +
> +static void adnp_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> +{
> + struct adnp *gpio = container_of(chip, struct adnp, gpio);
to_adnp()
> + unsigned int reg = offset >> gpio->reg_shift;
> + unsigned int pos = offset & 7;
> + int err;
> + u8 val;
> +
> + mutex_lock(&gpio->i2c_lock);
But here the mutex is needed, I can see that...
(etc)
(...)
> +static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> + struct adnp *gpio = container_of(chip, struct adnp, gpio);
> + u8 *base, *ddr, *plr, *ier, *isr, *ptr;
> + unsigned int regs, i, j;
> + int err;
> +
> + regs = 1 << gpio->reg_shift;
> +
> + base = kzalloc(regs * 5, GFP_KERNEL);
Why kzalloc()/kfree() when you can just use a
static u8 base[N];
where N is the max number of registers on any HW instead?
> + if (!base)
> + return;
> +
> + ddr = base + (regs * 0);
> + plr = base + (regs * 1);
> + ier = base + (regs * 2);
> + isr = base + (regs * 3);
> + ptr = base + (regs * 4);
> +
> + for (i = 0; i < regs; i++) {
> + err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
> + if (err < 0)
> + goto out;
> +
> + err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
> + if (err < 0)
> + goto out;
> + }
You must take the mutex around this for-loop, don't you?
Else browing debugfs will sporadically corrupt transfers,
or something like that. Or was this intended to cut into any
ongoing transfer couple?
> + for (i = 0; i < regs; i++) {
> + for (j = 0; j < 8; j++) {
> + unsigned int bit = (i << gpio->reg_shift) + j;
> + const char *direction = "input ";
> + const char *level = "low ";
> + const char *interrupt = "disabled";
> + const char *pending = "";
> +
> + if (ddr[i] & BIT(j))
> + direction = "output";
> +
> + if (plr[i] & BIT(j))
> + level = "high";
> +
> + if (ier[i] & BIT(j))
> + interrupt = "enabled ";
> +
> + if (isr[i] & BIT(j))
> + pending = "pending";
> +
> + seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
> + direction, level, interrupt, pending);
> + }
> + }
> +
> +out:
> + kfree(base);
> +}
> +
> +static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
> +{
> + struct gpio_chip *chip = &gpio->gpio;
> +
> + gpio->reg_shift = get_count_order(num_gpios) - 3;
> +
> + chip->direction_input = adnp_gpio_direction_input;
> + chip->direction_output = adnp_gpio_direction_output;
> + chip->get = adnp_gpio_get;
> + chip->set = adnp_gpio_set;
> + chip->can_sleep = 1;
> + if (IS_ENABLED(CONFIG_DEBUG_FS))
> + chip->dbg_show = adnp_gpio_dbg_show;
> +
> + chip->base = -1;
> + chip->ngpio = num_gpios;
> + chip->label = gpio->client->name;
> + chip->dev = &gpio->client->dev;
> + chip->of_node = chip->dev->of_node;
> + chip->owner = THIS_MODULE;
Usually we define the struct gpio_chip as a static const and have
the state containter hold a static const struct gpio_chip *chip;
entry assigned to point to the static const at probe time.
All other GPIO drivers #ifdef their debugfs code, this probably
works fine too, but never saw it before.
I'm ambivalent about this, I sort of like it because it avoids
#ifdefs and also makes sure the code is always compiled,
so if you like it like this, keep it.
(...)
> +static void adnp_irq_update_mask(struct adnp *gpio)
> +{
> + unsigned int regs = 1 << gpio->reg_shift;
> + bool equal = true;
> + unsigned int i;
> +
> + for (i = 0; i < regs; i++) {
> + if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
> + equal = false;
> + break;
> + }
> + }
This is not looking good. It looks like you're reimplementing
parts of regmap.
In that case, please use <linux/regmap.h> to cache registers
instead.
But I'm not sure ...
(...)
> +static void adnp_irq_bus_lock(struct irq_data *data)
> +{
> + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> + unsigned int regs = 1 << gpio->reg_shift;
> +
> + mutex_lock(&gpio->irq_lock);
> + memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
> +}
> +
> +static void adnp_irq_bus_unlock(struct irq_data *data)
> +{
> + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> +
> + adnp_irq_update_mask(gpio);
> + mutex_unlock(&gpio->irq_lock);
> +}
Actually I'm not following why the IRQ mask registers are shadowed
at bus_lock and restored at bus_unlock().
A comment describing why that's needed and how it works is
definately needed.
(...)
> +static const struct irq_domain_ops adnp_irq_domain_ops = {
> + .map = adnp_irq_map,
> + .xlate = irq_domain_xlate_twocell,
> +};
> +
> +static int adnp_irq_setup(struct adnp *gpio)
> +{
> + unsigned int regs = 1 << gpio->reg_shift, i;
> + struct gpio_chip *chip = &gpio->gpio;
> + int err;
> +
> + mutex_init(&gpio->irq_lock);
> +
> + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> + if (!gpio->irq_mask)
> + return -ENOMEM;
> +
> + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> + gpio->irq_level = gpio->irq_mask + (regs * 2);
> + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> + gpio->irq_high = gpio->irq_mask + (regs * 5);
> + gpio->irq_low = gpio->irq_mask + (regs * 6);
I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
here? Explain with some comment atleast.
> +
> + for (i = 0; i < regs; i++) {
> + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
> + if (err < 0)
> + return err;
> + }
Looks like regmap reimplementation.
> + gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
> + &adnp_irq_domain_ops, gpio);
> +
> + err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
> + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> + dev_name(chip->dev), gpio);
Since you're using devm_* above use it here too:
devm_request_threaded_irq().
> + if (err != 0) {
> + dev_err(chip->dev, "can't request IRQ#%d: %d\n",
> + gpio->client->irq, err);
> + goto error;
> + }
> +
> + gpio->gpio.to_irq = adnp_gpio_to_irq;
> + return 0;
> +
> +error:
> + irq_domain_remove(gpio->domain);
> + return err;
> +}
> +
> +static void adnp_irq_teardown(struct adnp *gpio)
> +{
> + unsigned int irq, i;
> +
> + free_irq(gpio->client->irq, gpio);
If you're using devm to grab the IRQ this is not needed.
> +
> + for (i = 0; i < gpio->gpio.ngpio; i++) {
> + irq = irq_find_mapping(gpio->domain, i);
> + if (irq > 0)
> + irq_dispose_mapping(irq);
> + }
> +
> + irq_domain_remove(gpio->domain);
> +}
> +
> +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct adnp *gpio;
> + u32 num_gpios;
> + int err;
> +
> + err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> + &num_gpios);
> + if (err < 0)
> + return err;
> +
> + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> + if (client->irq == NO_IRQ)
Just if (!client->irq) since NO_IRQ is 0 nowadays.
> + return -EPROBE_DEFER;
Why would you defer in this case? If the IRQ controller appear later
than the GPIO driver?
> + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> +
> + mutex_init(&gpio->i2c_lock);
> + gpio->client = client;
> +
> + err = adnp_gpio_setup(gpio, num_gpios);
> + if (err < 0)
> + return err;
> +
> + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
> + err = adnp_irq_setup(gpio);
> + if (err < 0)
> + goto teardown;
> + }
And that activates the question why this should be conditional,
please elaborate.
> + err = gpiochip_add(&gpio->gpio);
> + if (err < 0)
> + goto teardown;
> +
> + i2c_set_clientdata(client, gpio);
> + return 0;
> +
> +teardown:
> + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
> + adnp_irq_teardown(gpio);
Here too.
> +
> + return err;
> +}
Most of the driver looks very good though! The code is
clean and easy to read and maintain.
But we need to iron out the details.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-07-29 17:13 ` Linus Walleij
(?)
@ 2012-07-29 20:27 ` Arnd Bergmann
-1 siblings, 0 replies; 27+ messages in thread
From: Arnd Bergmann @ 2012-07-29 20:27 UTC (permalink / raw)
To: Linus Walleij
Cc: Thierry Reding, Grant Likely, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
On Sunday 29 July 2012, Linus Walleij wrote:
> > +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> > + whereas the second cell is used to specify flags:
> > + bits[3:0] trigger type and level flags
> > + 1 = low-to-high edge triggered
> > + 2 = high-to-low edge triggered
> > + 4 = active high level-sensitive
> > + 8 = active low level-sensitive
>
> Why on earth would a bunch of flags be an "interrupt cell"?
>
> Maybe there is something about DT bindings I don't get so
> please educate me.
>
> I can see that OMAP is doing this, but is it a good idea?
> I really need Rob/Grant to comment on this.
It is in fact a good idea, and it's very common to have it. The point is that
the interrupt controller requires this setting in order to configure that IRQ
for use, so putting it into a property of the device that triggers the IRQ
is the right place to have it.
> > +- interrupt-controller: Marks the device as an interrupt controller.
> > +- nr-gpios: The number of pins supported by the controller.
>
> These two last things look very generic, like something every GPIO
> driver could want to expose.
>
> I'd really like to have Grant's word on GPIO DT bindings and how these
> should look, I had some discussion with Wolfram (the I2C maintainer)
> about bindings turning out less generic than they ought to be, so we
> need some discussion on this.
>
> Arnd recently consolidated some MMC props, maybe we need to do
> the same for GPIO drivers.
The "interrupt-controller" property is standardized by IEEE. Not sure
about what specifies the "nr-gpios" one, but it seems to be a good idea
to have everyone do the same thing here.
Arnd
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-30 7:47 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-07-30 7:47 UTC (permalink / raw)
To: Linus Walleij
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 21865 bytes --]
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
>
> > This commit adds a driver for the Avionic Design N-bit GPIO expander.
> > The expander provides a variable number of GPIO pins with interrupt
> > support.
> (...)
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> > new file mode 100644
> > index 0000000..513a18e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> > @@ -0,0 +1,38 @@
> > +Avionic Design N-bit GPIO expander bindings
> > +
> > +Required properties:
> > +- compatible: should be "ad,gpio-adnp"
> > +- reg: The I2C slave address for this device.
> > +- interrupt-parent: phandle of the parent interrupt controller.
> > +- interrupts: Interrupt specifier for the controllers interrupt.
> > +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
> > + second cell is used to specify optional parameters:
> > + - bit 0: polarity (0: normal, 1: inverted)
> > +- gpio-controller: Marks the device as a GPIO controller
> > +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> > + whereas the second cell is used to specify flags:
> > + bits[3:0] trigger type and level flags
> > + 1 = low-to-high edge triggered
> > + 2 = high-to-low edge triggered
> > + 4 = active high level-sensitive
> > + 8 = active low level-sensitive
>
> Why on earth would a bunch of flags be an "interrupt cell"?
>
> Maybe there is something about DT bindings I don't get so
> please educate me.
>
> I can see that OMAP is doing this, but is it a good idea?
> I really need Rob/Grant to comment on this.
>
> > +- interrupt-controller: Marks the device as an interrupt controller.
> > +- nr-gpios: The number of pins supported by the controller.
>
> These two last things look very generic, like something every GPIO
> driver could want to expose.
As Arnd mentioned, interrupt-controller is a generic property required
by all interrupt controller nodes. Perhaps it shouldn't be listed in the
DT binding for this driver.
As to the nr-gpios property, this is actually not only for informational
purposes, but it also allows the driver to be configured to handle any
number of bits (powers of two). However since this is also a description
of the hardware it may be useful to make this into a generic property
for GPIO controllers.
> I'd really like to have Grant's word on GPIO DT bindings and how these
> should look, I had some discussion with Wolfram (the I2C maintainer)
> about bindings turning out less generic than they ought to be, so we
> need some discussion on this.
>
> Arnd recently consolidated some MMC props, maybe we need to do
> the same for GPIO drivers.
>
> (...)
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 502b5ea..d1b0f7d 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
> > Say yes here to enable the adp5588 to be used as an interrupt
> > controller. It requires the driver to be built in the kernel.
> >
> > +config GPIO_ADNP
> > + tristate "Avionic Design N-bit GPIO expander"
> > + depends on I2C && OF
> > + help
> > + This option enables support for N GPIOs found on Avionic Design
> > + I2C GPIO expanders. The register space will be extended by powers
> > + of two, so the controller will need to accomodate for that. For
> > + example: if a controller provides 48 pins, 6 registers will be
> > + enough to represent all pins, but the driver will assume a
> > + register layout for 64 pins (8 registers).
> > +
> > +config GPIO_ADNP_IRQ
> > + tristate "Interrupt controller support"
> > + depends on GPIO_ADNP
> > + help
> > + Say yes here to enable the Avionic Design N-bit GPIO expander to
> > + be used as an interrupt controller.
>
> First: please describe the usecase where the Avionic driver is used
> without making use of the IRQ, and *why* it should be possible
> to configure this out. E.g. is there a hardware which isn't using the
> IRQ portions? If there is no non-irq usecase just drop this
> config option.
This expander is used in a number of Tegra-based boards and handles
things like enabling or disabling power to a given IC but on other
boards it is also used to handle interrupts from input devices or
card-detect for example.
The controller is synthesized in a CPLD, which is one of the reasons for
the nr-gpios DT property. There is at least one platform that currently
doesn't use the interrupt functionality. Mainly I allowed this to be
configured out in order to reduce the number of interrupts required for
a platform. Another reason was that at the time I first wrote this, IRQ
domains hadn't been available, so the driver couldn't be built as a
module if interrupt support was required. This also no longer applies.
I'm actually fine either way, but I thought it'd be most flexible by
keeping the IRQ support optional for the above reasons.
> If you're keeping it:
>
> Now this is going to appear as a single line under the ADNP driver
> config entry saying "Interrupt controller support", which is too
> generic.
>
> Please make it expand to a submenu so that it gets indented below the
> controller driver.
>
> (...)
> > diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
> > new file mode 100644
> > index 0000000..c122ff4
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-adnp.c
> > @@ -0,0 +1,615 @@
> > +/*
> > + * Copyright (C) 2011-2012 Avionic Design GmbH
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqdomain.h>
>
> Good!
>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +
> > +#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
> > +#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
> > +#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
> > +#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
> > +#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
> > +
> > +struct adnp {
> > + struct i2c_client *client;
> > + struct gpio_chip gpio;
> > + unsigned int reg_shift;
> > +
> > + struct mutex i2c_lock;
> > +
> > + struct irq_domain *domain;
> > + struct mutex irq_lock;
> > +
> > + u8 *irq_mask;
> > + u8 *irq_mask_cur;
> > + u8 *irq_level;
> > + u8 *irq_rise;
> > + u8 *irq_fall;
> > + u8 *irq_high;
> > + u8 *irq_low;
>
> Some or all of this looks like a regmap reimplementation, see below.
Actually none of these represent actual registers, except for irq_mask
and irq_mask_cur. They are used to emulate various IRQ trigger modes.
>
> > +};
> > +
> > +static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
>
> I don't know why you name the struct adnp variable "gpio" everywhere,
> that is quite ambigous. Why not name this variable "adnp" consequently
> everywhere?
I can change that, no problem.
> > +{
> > + int err;
> > +
> > + err = i2c_smbus_read_byte_data(gpio->client, offset);
> > + if (err < 0) {
> > + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> > + "i2c_smbus_read_byte_data()", err);
> > + return err;
> > + }
> > +
> > + *value = err;
> > + return 0;
> > +}
> > +
> > +static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
> > +{
> > + int err;
> > +
> > + err = i2c_smbus_write_byte_data(gpio->client, offset, value);
> > + if (err < 0) {
> > + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> > + "i2c_smbus_write_byte_data()", err);
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > + struct adnp *gpio = container_of(chip, struct adnp, gpio);
>
> When I do this at several places in a driver I usually define
> a macro like
>
> #define to_adnp(foo) container_of(foo, struct adnp, gpio)
>
> Used like so:
>
> struct adnp *adnp = to_adnp(chip);
Yes, I usually do that as well. I guess this driver has been in the
works in too many variants for too long. =)
>
> > + unsigned int reg = offset >> gpio->reg_shift;
> > + unsigned int pos = offset & 7;
> > + u8 value;
> > + int err;
> > +
> > + mutex_lock(&gpio->i2c_lock);
>
> The point of taking this mutex here avoids me.
> adnp_read() only performs a single transaction.
I think that's a relic from an earlier version that used to access the
PTR (Pin Tristate Register) as well. At the time I used to return 2 here
to signify a tristated input, which was dependent on the contents of the
PTR. Tristating an output is, I believe, better done using pinmux/
pinctrl nowadays, so I took that code because the only platform where
this was ever used will probably never be mainlined.
On that note, provided there is special additional circuitry, the GPIO
controller is able to detect tristate on an input. I'm not aware that
the pinctrl subsystem provides for that functionality yet, right?
> > +static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > +{
> > + struct adnp *gpio = container_of(chip, struct adnp, gpio);
> > + u8 *base, *ddr, *plr, *ier, *isr, *ptr;
> > + unsigned int regs, i, j;
> > + int err;
> > +
> > + regs = 1 << gpio->reg_shift;
> > +
> > + base = kzalloc(regs * 5, GFP_KERNEL);
>
> Why kzalloc()/kfree() when you can just use a
>
> static u8 base[N];
>
> where N is the max number of registers on any HW instead?
As I explained above, the number of pins is configurable, so it'd be
quite a waste to always assume a maximum of, say, 256 pins if the
hardware actually only uses 8.
> > + if (!base)
> > + return;
> > +
> > + ddr = base + (regs * 0);
> > + plr = base + (regs * 1);
> > + ier = base + (regs * 2);
> > + isr = base + (regs * 3);
> > + ptr = base + (regs * 4);
> > +
> > + for (i = 0; i < regs; i++) {
> > + err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
> > + if (err < 0)
> > + goto out;
> > + }
>
> You must take the mutex around this for-loop, don't you?
>
> Else browing debugfs will sporadically corrupt transfers,
> or something like that. Or was this intended to cut into any
> ongoing transfer couple?
No, you're right. This should be protected by the mutex.
> > + for (i = 0; i < regs; i++) {
> > + for (j = 0; j < 8; j++) {
> > + unsigned int bit = (i << gpio->reg_shift) + j;
> > + const char *direction = "input ";
> > + const char *level = "low ";
> > + const char *interrupt = "disabled";
> > + const char *pending = "";
> > +
> > + if (ddr[i] & BIT(j))
> > + direction = "output";
> > +
> > + if (plr[i] & BIT(j))
> > + level = "high";
> > +
> > + if (ier[i] & BIT(j))
> > + interrupt = "enabled ";
> > +
> > + if (isr[i] & BIT(j))
> > + pending = "pending";
> > +
> > + seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
> > + direction, level, interrupt, pending);
> > + }
> > + }
> > +
> > +out:
> > + kfree(base);
> > +}
> > +
> > +static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
> > +{
> > + struct gpio_chip *chip = &gpio->gpio;
> > +
> > + gpio->reg_shift = get_count_order(num_gpios) - 3;
> > +
> > + chip->direction_input = adnp_gpio_direction_input;
> > + chip->direction_output = adnp_gpio_direction_output;
> > + chip->get = adnp_gpio_get;
> > + chip->set = adnp_gpio_set;
> > + chip->can_sleep = 1;
> > + if (IS_ENABLED(CONFIG_DEBUG_FS))
> > + chip->dbg_show = adnp_gpio_dbg_show;
> > +
> > + chip->base = -1;
> > + chip->ngpio = num_gpios;
> > + chip->label = gpio->client->name;
> > + chip->dev = &gpio->client->dev;
> > + chip->of_node = chip->dev->of_node;
> > + chip->owner = THIS_MODULE;
>
> Usually we define the struct gpio_chip as a static const and have
> the state containter hold a static const struct gpio_chip *chip;
> entry assigned to point to the static const at probe time.
The problem with a static const is that you can no longer configure the
number of GPIOs at runtime, which is sort of essential for this driver.
> All other GPIO drivers #ifdef their debugfs code, this probably
> works fine too, but never saw it before.
>
> I'm ambivalent about this, I sort of like it because it avoids
> #ifdefs and also makes sure the code is always compiled,
> so if you like it like this, keep it.
I've started to use this wherever possible because otherwise you have to
build numerous configurations to ensure complete compile coverage. Then
again this also has the drawback to potentially hide issues if you don't
properly separate conditionalized code.
> (...)
> > +static void adnp_irq_update_mask(struct adnp *gpio)
> > +{
> > + unsigned int regs = 1 << gpio->reg_shift;
> > + bool equal = true;
> > + unsigned int i;
> > +
> > + for (i = 0; i < regs; i++) {
> > + if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
> > + equal = false;
> > + break;
> > + }
> > + }
>
> This is not looking good. It looks like you're reimplementing
> parts of regmap.
>
> In that case, please use <linux/regmap.h> to cache registers
> instead.
>
> But I'm not sure ...
>
> (...)
> > +static void adnp_irq_bus_lock(struct irq_data *data)
> > +{
> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> > + unsigned int regs = 1 << gpio->reg_shift;
> > +
> > + mutex_lock(&gpio->irq_lock);
> > + memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
> > +}
> > +
> > +static void adnp_irq_bus_unlock(struct irq_data *data)
> > +{
> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> > +
> > + adnp_irq_update_mask(gpio);
> > + mutex_unlock(&gpio->irq_lock);
> > +}
>
> Actually I'm not following why the IRQ mask registers are shadowed
> at bus_lock and restored at bus_unlock().
>
> A comment describing why that's needed and how it works is
> definately needed.
I'm not sure that this is required anymore. IIRC I did copy this from
some other driver at the time. This is probably supposed to be an
optimization, but I think I can live without it.
>
> (...)
> > +static const struct irq_domain_ops adnp_irq_domain_ops = {
> > + .map = adnp_irq_map,
> > + .xlate = irq_domain_xlate_twocell,
> > +};
> > +
> > +static int adnp_irq_setup(struct adnp *gpio)
> > +{
> > + unsigned int regs = 1 << gpio->reg_shift, i;
> > + struct gpio_chip *chip = &gpio->gpio;
> > + int err;
> > +
> > + mutex_init(&gpio->irq_lock);
> > +
> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> > + if (!gpio->irq_mask)
> > + return -ENOMEM;
> > +
> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
>
> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
> here? Explain with some comment atleast.
Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
irq_low to keep track of the current level and trigger modes for each
interrupt. Instead of allocating small chunks for each of these I
allocate one chunk and just make the others point into that.
> > +
> > + for (i = 0; i < regs; i++) {
> > + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
> > + if (err < 0)
> > + return err;
> > + }
>
> Looks like regmap reimplementation.
This is used to obtain the initial pin levels, which in turn is required
to check for rising and falling edges.
> > + gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
> > + &adnp_irq_domain_ops, gpio);
> > +
> > + err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
> > + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > + dev_name(chip->dev), gpio);
>
> Since you're using devm_* above use it here too:
> devm_request_threaded_irq().
>
> > + if (err != 0) {
> > + dev_err(chip->dev, "can't request IRQ#%d: %d\n",
> > + gpio->client->irq, err);
> > + goto error;
> > + }
> > +
> > + gpio->gpio.to_irq = adnp_gpio_to_irq;
> > + return 0;
> > +
> > +error:
> > + irq_domain_remove(gpio->domain);
> > + return err;
> > +}
> > +
> > +static void adnp_irq_teardown(struct adnp *gpio)
> > +{
> > + unsigned int irq, i;
> > +
> > + free_irq(gpio->client->irq, gpio);
>
> If you're using devm to grab the IRQ this is not needed.
I don't think that'll work. In this case the interrupt needs to be freed
before cleaning up, because otherwise the interrupt handler may still be
run after the IRQ domain has already been removed.
>
> > +
> > + for (i = 0; i < gpio->gpio.ngpio; i++) {
> > + irq = irq_find_mapping(gpio->domain, i);
> > + if (irq > 0)
> > + irq_dispose_mapping(irq);
> > + }
> > +
> > + irq_domain_remove(gpio->domain);
> > +}
> > +
> > +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct adnp *gpio;
> > + u32 num_gpios;
> > + int err;
> > +
> > + err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> > + &num_gpios);
> > + if (err < 0)
> > + return err;
> > +
> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> > + if (client->irq == NO_IRQ)
>
> Just if (!client->irq) since NO_IRQ is 0 nowadays.
Okay, will do.
>
> > + return -EPROBE_DEFER;
>
> Why would you defer in this case? If the IRQ controller appear later
> than the GPIO driver?
Yes. In particular when using DT it can happen that the parent interrupt
controller is probed later than this.
> > + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + mutex_init(&gpio->i2c_lock);
> > + gpio->client = client;
> > +
> > + err = adnp_gpio_setup(gpio, num_gpios);
> > + if (err < 0)
> > + return err;
> > +
> > + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
> > + err = adnp_irq_setup(gpio);
> > + if (err < 0)
> > + goto teardown;
> > + }
>
> And that activates the question why this should be conditional,
> please elaborate.
I think I've answered this before.
> > + err = gpiochip_add(&gpio->gpio);
> > + if (err < 0)
> > + goto teardown;
> > +
> > + i2c_set_clientdata(client, gpio);
> > + return 0;
> > +
> > +teardown:
> > + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
> > + adnp_irq_teardown(gpio);
>
> Here too.
>
> > +
> > + return err;
> > +}
>
> Most of the driver looks very good though! The code is
> clean and easy to read and maintain.
>
> But we need to iron out the details.
Thanks for reviewing. I'll fixup the problems you've pointed out and
will have to retest.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-30 7:47 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-07-30 7:47 UTC (permalink / raw)
To: Linus Walleij
Cc: Linus Walleij, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Wolfram Sang
[-- Attachment #1.1: Type: text/plain, Size: 21897 bytes --]
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>
> > This commit adds a driver for the Avionic Design N-bit GPIO expander.
> > The expander provides a variable number of GPIO pins with interrupt
> > support.
> (...)
> > diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> > new file mode 100644
> > index 0000000..513a18e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
> > @@ -0,0 +1,38 @@
> > +Avionic Design N-bit GPIO expander bindings
> > +
> > +Required properties:
> > +- compatible: should be "ad,gpio-adnp"
> > +- reg: The I2C slave address for this device.
> > +- interrupt-parent: phandle of the parent interrupt controller.
> > +- interrupts: Interrupt specifier for the controllers interrupt.
> > +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
> > + second cell is used to specify optional parameters:
> > + - bit 0: polarity (0: normal, 1: inverted)
> > +- gpio-controller: Marks the device as a GPIO controller
> > +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
> > + whereas the second cell is used to specify flags:
> > + bits[3:0] trigger type and level flags
> > + 1 = low-to-high edge triggered
> > + 2 = high-to-low edge triggered
> > + 4 = active high level-sensitive
> > + 8 = active low level-sensitive
>
> Why on earth would a bunch of flags be an "interrupt cell"?
>
> Maybe there is something about DT bindings I don't get so
> please educate me.
>
> I can see that OMAP is doing this, but is it a good idea?
> I really need Rob/Grant to comment on this.
>
> > +- interrupt-controller: Marks the device as an interrupt controller.
> > +- nr-gpios: The number of pins supported by the controller.
>
> These two last things look very generic, like something every GPIO
> driver could want to expose.
As Arnd mentioned, interrupt-controller is a generic property required
by all interrupt controller nodes. Perhaps it shouldn't be listed in the
DT binding for this driver.
As to the nr-gpios property, this is actually not only for informational
purposes, but it also allows the driver to be configured to handle any
number of bits (powers of two). However since this is also a description
of the hardware it may be useful to make this into a generic property
for GPIO controllers.
> I'd really like to have Grant's word on GPIO DT bindings and how these
> should look, I had some discussion with Wolfram (the I2C maintainer)
> about bindings turning out less generic than they ought to be, so we
> need some discussion on this.
>
> Arnd recently consolidated some MMC props, maybe we need to do
> the same for GPIO drivers.
>
> (...)
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> > index 502b5ea..d1b0f7d 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -444,6 +444,24 @@ config GPIO_ADP5588_IRQ
> > Say yes here to enable the adp5588 to be used as an interrupt
> > controller. It requires the driver to be built in the kernel.
> >
> > +config GPIO_ADNP
> > + tristate "Avionic Design N-bit GPIO expander"
> > + depends on I2C && OF
> > + help
> > + This option enables support for N GPIOs found on Avionic Design
> > + I2C GPIO expanders. The register space will be extended by powers
> > + of two, so the controller will need to accomodate for that. For
> > + example: if a controller provides 48 pins, 6 registers will be
> > + enough to represent all pins, but the driver will assume a
> > + register layout for 64 pins (8 registers).
> > +
> > +config GPIO_ADNP_IRQ
> > + tristate "Interrupt controller support"
> > + depends on GPIO_ADNP
> > + help
> > + Say yes here to enable the Avionic Design N-bit GPIO expander to
> > + be used as an interrupt controller.
>
> First: please describe the usecase where the Avionic driver is used
> without making use of the IRQ, and *why* it should be possible
> to configure this out. E.g. is there a hardware which isn't using the
> IRQ portions? If there is no non-irq usecase just drop this
> config option.
This expander is used in a number of Tegra-based boards and handles
things like enabling or disabling power to a given IC but on other
boards it is also used to handle interrupts from input devices or
card-detect for example.
The controller is synthesized in a CPLD, which is one of the reasons for
the nr-gpios DT property. There is at least one platform that currently
doesn't use the interrupt functionality. Mainly I allowed this to be
configured out in order to reduce the number of interrupts required for
a platform. Another reason was that at the time I first wrote this, IRQ
domains hadn't been available, so the driver couldn't be built as a
module if interrupt support was required. This also no longer applies.
I'm actually fine either way, but I thought it'd be most flexible by
keeping the IRQ support optional for the above reasons.
> If you're keeping it:
>
> Now this is going to appear as a single line under the ADNP driver
> config entry saying "Interrupt controller support", which is too
> generic.
>
> Please make it expand to a submenu so that it gets indented below the
> controller driver.
>
> (...)
> > diff --git a/drivers/gpio/gpio-adnp.c b/drivers/gpio/gpio-adnp.c
> > new file mode 100644
> > index 0000000..c122ff4
> > --- /dev/null
> > +++ b/drivers/gpio/gpio-adnp.c
> > @@ -0,0 +1,615 @@
> > +/*
> > + * Copyright (C) 2011-2012 Avionic Design GmbH
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#include <linux/gpio.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/irqdomain.h>
>
> Good!
>
> > +#include <linux/module.h>
> > +#include <linux/of_irq.h>
> > +#include <linux/seq_file.h>
> > +#include <linux/slab.h>
> > +
> > +#define GPIO_DDR(gpio) (0x00 << (gpio)->reg_shift)
> > +#define GPIO_PLR(gpio) (0x01 << (gpio)->reg_shift)
> > +#define GPIO_IER(gpio) (0x02 << (gpio)->reg_shift)
> > +#define GPIO_ISR(gpio) (0x03 << (gpio)->reg_shift)
> > +#define GPIO_PTR(gpio) (0x04 << (gpio)->reg_shift)
> > +
> > +struct adnp {
> > + struct i2c_client *client;
> > + struct gpio_chip gpio;
> > + unsigned int reg_shift;
> > +
> > + struct mutex i2c_lock;
> > +
> > + struct irq_domain *domain;
> > + struct mutex irq_lock;
> > +
> > + u8 *irq_mask;
> > + u8 *irq_mask_cur;
> > + u8 *irq_level;
> > + u8 *irq_rise;
> > + u8 *irq_fall;
> > + u8 *irq_high;
> > + u8 *irq_low;
>
> Some or all of this looks like a regmap reimplementation, see below.
Actually none of these represent actual registers, except for irq_mask
and irq_mask_cur. They are used to emulate various IRQ trigger modes.
>
> > +};
> > +
> > +static int adnp_read(struct adnp *gpio, unsigned offset, uint8_t *value)
>
> I don't know why you name the struct adnp variable "gpio" everywhere,
> that is quite ambigous. Why not name this variable "adnp" consequently
> everywhere?
I can change that, no problem.
> > +{
> > + int err;
> > +
> > + err = i2c_smbus_read_byte_data(gpio->client, offset);
> > + if (err < 0) {
> > + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> > + "i2c_smbus_read_byte_data()", err);
> > + return err;
> > + }
> > +
> > + *value = err;
> > + return 0;
> > +}
> > +
> > +static int adnp_write(struct adnp *gpio, unsigned offset, uint8_t value)
> > +{
> > + int err;
> > +
> > + err = i2c_smbus_write_byte_data(gpio->client, offset, value);
> > + if (err < 0) {
> > + dev_err(gpio->gpio.dev, "%s failed: %d\n",
> > + "i2c_smbus_write_byte_data()", err);
> > + return err;
> > + }
> > +
> > + return 0;
> > +}
> > +
> > +static int adnp_gpio_get(struct gpio_chip *chip, unsigned offset)
> > +{
> > + struct adnp *gpio = container_of(chip, struct adnp, gpio);
>
> When I do this at several places in a driver I usually define
> a macro like
>
> #define to_adnp(foo) container_of(foo, struct adnp, gpio)
>
> Used like so:
>
> struct adnp *adnp = to_adnp(chip);
Yes, I usually do that as well. I guess this driver has been in the
works in too many variants for too long. =)
>
> > + unsigned int reg = offset >> gpio->reg_shift;
> > + unsigned int pos = offset & 7;
> > + u8 value;
> > + int err;
> > +
> > + mutex_lock(&gpio->i2c_lock);
>
> The point of taking this mutex here avoids me.
> adnp_read() only performs a single transaction.
I think that's a relic from an earlier version that used to access the
PTR (Pin Tristate Register) as well. At the time I used to return 2 here
to signify a tristated input, which was dependent on the contents of the
PTR. Tristating an output is, I believe, better done using pinmux/
pinctrl nowadays, so I took that code because the only platform where
this was ever used will probably never be mainlined.
On that note, provided there is special additional circuitry, the GPIO
controller is able to detect tristate on an input. I'm not aware that
the pinctrl subsystem provides for that functionality yet, right?
> > +static void adnp_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> > +{
> > + struct adnp *gpio = container_of(chip, struct adnp, gpio);
> > + u8 *base, *ddr, *plr, *ier, *isr, *ptr;
> > + unsigned int regs, i, j;
> > + int err;
> > +
> > + regs = 1 << gpio->reg_shift;
> > +
> > + base = kzalloc(regs * 5, GFP_KERNEL);
>
> Why kzalloc()/kfree() when you can just use a
>
> static u8 base[N];
>
> where N is the max number of registers on any HW instead?
As I explained above, the number of pins is configurable, so it'd be
quite a waste to always assume a maximum of, say, 256 pins if the
hardware actually only uses 8.
> > + if (!base)
> > + return;
> > +
> > + ddr = base + (regs * 0);
> > + plr = base + (regs * 1);
> > + ier = base + (regs * 2);
> > + isr = base + (regs * 3);
> > + ptr = base + (regs * 4);
> > +
> > + for (i = 0; i < regs; i++) {
> > + err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &plr[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_IER(gpio) + i, &ier[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_ISR(gpio) + i, &isr[i]);
> > + if (err < 0)
> > + goto out;
> > +
> > + err = adnp_read(gpio, GPIO_PTR(gpio) + i, &ptr[i]);
> > + if (err < 0)
> > + goto out;
> > + }
>
> You must take the mutex around this for-loop, don't you?
>
> Else browing debugfs will sporadically corrupt transfers,
> or something like that. Or was this intended to cut into any
> ongoing transfer couple?
No, you're right. This should be protected by the mutex.
> > + for (i = 0; i < regs; i++) {
> > + for (j = 0; j < 8; j++) {
> > + unsigned int bit = (i << gpio->reg_shift) + j;
> > + const char *direction = "input ";
> > + const char *level = "low ";
> > + const char *interrupt = "disabled";
> > + const char *pending = "";
> > +
> > + if (ddr[i] & BIT(j))
> > + direction = "output";
> > +
> > + if (plr[i] & BIT(j))
> > + level = "high";
> > +
> > + if (ier[i] & BIT(j))
> > + interrupt = "enabled ";
> > +
> > + if (isr[i] & BIT(j))
> > + pending = "pending";
> > +
> > + seq_printf(s, "%2u: %s %s IRQ %s %s\n", bit,
> > + direction, level, interrupt, pending);
> > + }
> > + }
> > +
> > +out:
> > + kfree(base);
> > +}
> > +
> > +static int adnp_gpio_setup(struct adnp *gpio, unsigned int num_gpios)
> > +{
> > + struct gpio_chip *chip = &gpio->gpio;
> > +
> > + gpio->reg_shift = get_count_order(num_gpios) - 3;
> > +
> > + chip->direction_input = adnp_gpio_direction_input;
> > + chip->direction_output = adnp_gpio_direction_output;
> > + chip->get = adnp_gpio_get;
> > + chip->set = adnp_gpio_set;
> > + chip->can_sleep = 1;
> > + if (IS_ENABLED(CONFIG_DEBUG_FS))
> > + chip->dbg_show = adnp_gpio_dbg_show;
> > +
> > + chip->base = -1;
> > + chip->ngpio = num_gpios;
> > + chip->label = gpio->client->name;
> > + chip->dev = &gpio->client->dev;
> > + chip->of_node = chip->dev->of_node;
> > + chip->owner = THIS_MODULE;
>
> Usually we define the struct gpio_chip as a static const and have
> the state containter hold a static const struct gpio_chip *chip;
> entry assigned to point to the static const at probe time.
The problem with a static const is that you can no longer configure the
number of GPIOs at runtime, which is sort of essential for this driver.
> All other GPIO drivers #ifdef their debugfs code, this probably
> works fine too, but never saw it before.
>
> I'm ambivalent about this, I sort of like it because it avoids
> #ifdefs and also makes sure the code is always compiled,
> so if you like it like this, keep it.
I've started to use this wherever possible because otherwise you have to
build numerous configurations to ensure complete compile coverage. Then
again this also has the drawback to potentially hide issues if you don't
properly separate conditionalized code.
> (...)
> > +static void adnp_irq_update_mask(struct adnp *gpio)
> > +{
> > + unsigned int regs = 1 << gpio->reg_shift;
> > + bool equal = true;
> > + unsigned int i;
> > +
> > + for (i = 0; i < regs; i++) {
> > + if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
> > + equal = false;
> > + break;
> > + }
> > + }
>
> This is not looking good. It looks like you're reimplementing
> parts of regmap.
>
> In that case, please use <linux/regmap.h> to cache registers
> instead.
>
> But I'm not sure ...
>
> (...)
> > +static void adnp_irq_bus_lock(struct irq_data *data)
> > +{
> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> > + unsigned int regs = 1 << gpio->reg_shift;
> > +
> > + mutex_lock(&gpio->irq_lock);
> > + memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
> > +}
> > +
> > +static void adnp_irq_bus_unlock(struct irq_data *data)
> > +{
> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
> > +
> > + adnp_irq_update_mask(gpio);
> > + mutex_unlock(&gpio->irq_lock);
> > +}
>
> Actually I'm not following why the IRQ mask registers are shadowed
> at bus_lock and restored at bus_unlock().
>
> A comment describing why that's needed and how it works is
> definately needed.
I'm not sure that this is required anymore. IIRC I did copy this from
some other driver at the time. This is probably supposed to be an
optimization, but I think I can live without it.
>
> (...)
> > +static const struct irq_domain_ops adnp_irq_domain_ops = {
> > + .map = adnp_irq_map,
> > + .xlate = irq_domain_xlate_twocell,
> > +};
> > +
> > +static int adnp_irq_setup(struct adnp *gpio)
> > +{
> > + unsigned int regs = 1 << gpio->reg_shift, i;
> > + struct gpio_chip *chip = &gpio->gpio;
> > + int err;
> > +
> > + mutex_init(&gpio->irq_lock);
> > +
> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> > + if (!gpio->irq_mask)
> > + return -ENOMEM;
> > +
> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
>
> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
> here? Explain with some comment atleast.
Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
irq_low to keep track of the current level and trigger modes for each
interrupt. Instead of allocating small chunks for each of these I
allocate one chunk and just make the others point into that.
> > +
> > + for (i = 0; i < regs; i++) {
> > + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
> > + if (err < 0)
> > + return err;
> > + }
>
> Looks like regmap reimplementation.
This is used to obtain the initial pin levels, which in turn is required
to check for rising and falling edges.
> > + gpio->domain = irq_domain_add_linear(chip->of_node, chip->ngpio,
> > + &adnp_irq_domain_ops, gpio);
> > +
> > + err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
> > + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
> > + dev_name(chip->dev), gpio);
>
> Since you're using devm_* above use it here too:
> devm_request_threaded_irq().
>
> > + if (err != 0) {
> > + dev_err(chip->dev, "can't request IRQ#%d: %d\n",
> > + gpio->client->irq, err);
> > + goto error;
> > + }
> > +
> > + gpio->gpio.to_irq = adnp_gpio_to_irq;
> > + return 0;
> > +
> > +error:
> > + irq_domain_remove(gpio->domain);
> > + return err;
> > +}
> > +
> > +static void adnp_irq_teardown(struct adnp *gpio)
> > +{
> > + unsigned int irq, i;
> > +
> > + free_irq(gpio->client->irq, gpio);
>
> If you're using devm to grab the IRQ this is not needed.
I don't think that'll work. In this case the interrupt needs to be freed
before cleaning up, because otherwise the interrupt handler may still be
run after the IRQ domain has already been removed.
>
> > +
> > + for (i = 0; i < gpio->gpio.ngpio; i++) {
> > + irq = irq_find_mapping(gpio->domain, i);
> > + if (irq > 0)
> > + irq_dispose_mapping(irq);
> > + }
> > +
> > + irq_domain_remove(gpio->domain);
> > +}
> > +
> > +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct adnp *gpio;
> > + u32 num_gpios;
> > + int err;
> > +
> > + err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> > + &num_gpios);
> > + if (err < 0)
> > + return err;
> > +
> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> > + if (client->irq == NO_IRQ)
>
> Just if (!client->irq) since NO_IRQ is 0 nowadays.
Okay, will do.
>
> > + return -EPROBE_DEFER;
>
> Why would you defer in this case? If the IRQ controller appear later
> than the GPIO driver?
Yes. In particular when using DT it can happen that the parent interrupt
controller is probed later than this.
> > + gpio = devm_kzalloc(&client->dev, sizeof(*gpio), GFP_KERNEL);
> > + if (!gpio)
> > + return -ENOMEM;
> > +
> > + mutex_init(&gpio->i2c_lock);
> > + gpio->client = client;
> > +
> > + err = adnp_gpio_setup(gpio, num_gpios);
> > + if (err < 0)
> > + return err;
> > +
> > + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
> > + err = adnp_irq_setup(gpio);
> > + if (err < 0)
> > + goto teardown;
> > + }
>
> And that activates the question why this should be conditional,
> please elaborate.
I think I've answered this before.
> > + err = gpiochip_add(&gpio->gpio);
> > + if (err < 0)
> > + goto teardown;
> > +
> > + i2c_set_clientdata(client, gpio);
> > + return 0;
> > +
> > +teardown:
> > + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ))
> > + adnp_irq_teardown(gpio);
>
> Here too.
>
> > +
> > + return err;
> > +}
>
> Most of the driver looks very good though! The code is
> clean and easy to read and maintain.
>
> But we need to iron out the details.
Thanks for reviewing. I'll fixup the problems you've pointed out and
will have to retest.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-31 22:03 ` Rob Herring
0 siblings, 0 replies; 27+ messages in thread
From: Rob Herring @ 2012-07-31 22:03 UTC (permalink / raw)
To: Thierry Reding
Cc: Linus Walleij, Linus Walleij, devicetree-discuss, linux-kernel,
Wolfram Sang
On 07/30/2012 02:47 AM, Thierry Reding wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
>> <thierry.reding@avionic-design.de> wrote:
>>
>>> This commit adds a driver for the Avionic Design N-bit GPIO expander.
>>> The expander provides a variable number of GPIO pins with interrupt
>>> support.
>> (...)
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
>>> new file mode 100644
>>> index 0000000..513a18e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
>>> @@ -0,0 +1,38 @@
>>> +Avionic Design N-bit GPIO expander bindings
>>> +
>>> +Required properties:
>>> +- compatible: should be "ad,gpio-adnp"
>>> +- reg: The I2C slave address for this device.
>>> +- interrupt-parent: phandle of the parent interrupt controller.
>>> +- interrupts: Interrupt specifier for the controllers interrupt.
>>> +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
>>> + second cell is used to specify optional parameters:
>>> + - bit 0: polarity (0: normal, 1: inverted)
>>> +- gpio-controller: Marks the device as a GPIO controller
>>> +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
>>> + whereas the second cell is used to specify flags:
>>> + bits[3:0] trigger type and level flags
>>> + 1 = low-to-high edge triggered
>>> + 2 = high-to-low edge triggered
>>> + 4 = active high level-sensitive
>>> + 8 = active low level-sensitive
>>
>> Why on earth would a bunch of flags be an "interrupt cell"?
>>
>> Maybe there is something about DT bindings I don't get so
>> please educate me.
>>
>> I can see that OMAP is doing this, but is it a good idea?
>> I really need Rob/Grant to comment on this.
>>
>>> +- interrupt-controller: Marks the device as an interrupt controller.
>>> +- nr-gpios: The number of pins supported by the controller.
>>
>> These two last things look very generic, like something every GPIO
>> driver could want to expose.
>
> As Arnd mentioned, interrupt-controller is a generic property required
> by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> DT binding for this driver.
>
> As to the nr-gpios property, this is actually not only for informational
> purposes, but it also allows the driver to be configured to handle any
> number of bits (powers of two). However since this is also a description
> of the hardware it may be useful to make this into a generic property
> for GPIO controllers.
>
>> I'd really like to have Grant's word on GPIO DT bindings and how these
>> should look, I had some discussion with Wolfram (the I2C maintainer)
>> about bindings turning out less generic than they ought to be, so we
>> need some discussion on this.
>>
>> Arnd recently consolidated some MMC props, maybe we need to do
>> the same for GPIO drivers.
For nr-gpios, I think it is typically not needed. Generally, you will
know how many gpio lines the h/w has based on the compatible string. If
this part really is the same part but different packages with different
numbers of gpio, then this property makes sense. Otherwise, I would say
drop it.
If your goal is how many gpios are you using, you really need a bitmap
instead if you want it to be generic.
IIRC, Tegra also needed this in that they N instances of some number of
bits and the registers are interleaved so they can't have separate nodes.
Rob
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-07-31 22:03 ` Rob Herring
0 siblings, 0 replies; 27+ messages in thread
From: Rob Herring @ 2012-07-31 22:03 UTC (permalink / raw)
To: Thierry Reding
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Wolfram Sang,
Linus Walleij, linux-kernel-u79uwXL29TY76Z2rM5mHXA
On 07/30/2012 02:47 AM, Thierry Reding wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
>> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>>
>>> This commit adds a driver for the Avionic Design N-bit GPIO expander.
>>> The expander provides a variable number of GPIO pins with interrupt
>>> support.
>> (...)
>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
>>> new file mode 100644
>>> index 0000000..513a18e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
>>> @@ -0,0 +1,38 @@
>>> +Avionic Design N-bit GPIO expander bindings
>>> +
>>> +Required properties:
>>> +- compatible: should be "ad,gpio-adnp"
>>> +- reg: The I2C slave address for this device.
>>> +- interrupt-parent: phandle of the parent interrupt controller.
>>> +- interrupts: Interrupt specifier for the controllers interrupt.
>>> +- #gpio-cells: Should be 2. The first cell is the GPIO number and the
>>> + second cell is used to specify optional parameters:
>>> + - bit 0: polarity (0: normal, 1: inverted)
>>> +- gpio-controller: Marks the device as a GPIO controller
>>> +- #interrupt-cells: Should be 2. The first cell contains the GPIO number,
>>> + whereas the second cell is used to specify flags:
>>> + bits[3:0] trigger type and level flags
>>> + 1 = low-to-high edge triggered
>>> + 2 = high-to-low edge triggered
>>> + 4 = active high level-sensitive
>>> + 8 = active low level-sensitive
>>
>> Why on earth would a bunch of flags be an "interrupt cell"?
>>
>> Maybe there is something about DT bindings I don't get so
>> please educate me.
>>
>> I can see that OMAP is doing this, but is it a good idea?
>> I really need Rob/Grant to comment on this.
>>
>>> +- interrupt-controller: Marks the device as an interrupt controller.
>>> +- nr-gpios: The number of pins supported by the controller.
>>
>> These two last things look very generic, like something every GPIO
>> driver could want to expose.
>
> As Arnd mentioned, interrupt-controller is a generic property required
> by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> DT binding for this driver.
>
> As to the nr-gpios property, this is actually not only for informational
> purposes, but it also allows the driver to be configured to handle any
> number of bits (powers of two). However since this is also a description
> of the hardware it may be useful to make this into a generic property
> for GPIO controllers.
>
>> I'd really like to have Grant's word on GPIO DT bindings and how these
>> should look, I had some discussion with Wolfram (the I2C maintainer)
>> about bindings turning out less generic than they ought to be, so we
>> need some discussion on this.
>>
>> Arnd recently consolidated some MMC props, maybe we need to do
>> the same for GPIO drivers.
For nr-gpios, I think it is typically not needed. Generally, you will
know how many gpio lines the h/w has based on the compatible string. If
this part really is the same part but different packages with different
numbers of gpio, then this property makes sense. Otherwise, I would say
drop it.
If your goal is how many gpios are you using, you really need a bitmap
instead if you want it to be generic.
IIRC, Tegra also needed this in that they N instances of some number of
bits and the registers are interleaved so they can't have separate nodes.
Rob
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-07-31 22:03 ` Rob Herring
(?)
@ 2012-07-31 23:22 ` Stephen Warren
-1 siblings, 0 replies; 27+ messages in thread
From: Stephen Warren @ 2012-07-31 23:22 UTC (permalink / raw)
To: Rob Herring
Cc: Thierry Reding, devicetree-discuss, Wolfram Sang, Linus Walleij,
linux-kernel
On 07/31/2012 04:03 PM, Rob Herring wrote:
> On 07/30/2012 02:47 AM, Thierry Reding wrote:
>> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>>> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
>>> <thierry.reding@avionic-design.de> wrote:
>>>
>>>> This commit adds a driver for the Avionic Design N-bit GPIO expander.
>>>> The expander provides a variable number of GPIO pins with interrupt
>>>> support.
>>>> diff --git a/Documentation/devicetree/bindings/gpio/gpio-adnp.txt b/Documentation/devicetree/bindings/gpio/gpio-adnp.txt
>>>> +Required properties:
>>>> +- nr-gpios: The number of pins supported by the controller.
> For nr-gpios, I think it is typically not needed. Generally, you will
> know how many gpio lines the h/w has based on the compatible string. If
> this part really is the same part but different packages with different
> numbers of gpio, then this property makes sense. Otherwise, I would say
> drop it.
>
> If your goal is how many gpios are you using, you really need a bitmap
> instead if you want it to be generic.
>
> IIRC, Tegra also needed this in that they N instances of some number of
> bits and the registers are interleaved so they can't have separate nodes.
In the end, I got away without having a separate property to represent
this. Instead, the code keys off the number of interrupts listed in the
interrupts property, there being one interrupt per GPIO bank, and hence
dynamically instantiates the number of banks based on that.
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-02 6:18 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-08-02 6:18 UTC (permalink / raw)
To: Rob Herring
Cc: Linus Walleij, Linus Walleij, devicetree-discuss, linux-kernel,
Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 522 bytes --]
On Tue, Jul 31, 2012 at 05:03:05PM -0500, Rob Herring wrote:
> For nr-gpios, I think it is typically not needed. Generally, you will
> know how many gpio lines the h/w has based on the compatible string. If
> this part really is the same part but different packages with different
> numbers of gpio, then this property makes sense.
That's exactly what I need it for. The controller is synthesized in a
CPLD, so technically the part is always the same, but the configured
logic can implement any number of GPIOs.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-02 6:18 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-08-02 6:18 UTC (permalink / raw)
To: Rob Herring
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Wolfram Sang,
Linus Walleij, linux-kernel-u79uwXL29TY76Z2rM5mHXA
[-- Attachment #1.1: Type: text/plain, Size: 522 bytes --]
On Tue, Jul 31, 2012 at 05:03:05PM -0500, Rob Herring wrote:
> For nr-gpios, I think it is typically not needed. Generally, you will
> know how many gpio lines the h/w has based on the compatible string. If
> this part really is the same part but different packages with different
> numbers of gpio, then this property makes sense.
That's exactly what I need it for. The controller is synthesized in a
CPLD, so technically the part is always the same, but the configured
logic can implement any number of GPIOs.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-05 10:50 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-05 10:50 UTC (permalink / raw)
To: Thierry Reding
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
>> <thierry.reding@avionic-design.de> wrote:
>> > +- interrupt-controller: Marks the device as an interrupt controller.
>> > +- nr-gpios: The number of pins supported by the controller.
>>
>> These two last things look very generic, like something every GPIO
>> driver could want to expose.
>
> As Arnd mentioned, interrupt-controller is a generic property required
> by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> DT binding for this driver.
After reading Rob's etc comments I think nr-gpios should be in this
binding, but interrupt-controller seems like it should go into
Documentation/devicetree/bindings/gpio/gpio.txt
can you take care of this?
(Anyone agains, beat me up...)
>> > +config GPIO_ADNP
>> > + tristate "Avionic Design N-bit GPIO expander"
>> > + depends on I2C && OF
>> > + help
>> > + This option enables support for N GPIOs found on Avionic Design
>> > + I2C GPIO expanders. The register space will be extended by powers
>> > + of two, so the controller will need to accomodate for that. For
>> > + example: if a controller provides 48 pins, 6 registers will be
>> > + enough to represent all pins, but the driver will assume a
>> > + register layout for 64 pins (8 registers).
>> > +
>> > +config GPIO_ADNP_IRQ
>> > + tristate "Interrupt controller support"
>> > + depends on GPIO_ADNP
>> > + help
>> > + Say yes here to enable the Avionic Design N-bit GPIO expander to
>> > + be used as an interrupt controller.
>>
>> First: please describe the usecase where the Avionic driver is used
>> without making use of the IRQ, and *why* it should be possible
>> to configure this out. E.g. is there a hardware which isn't using the
>> IRQ portions? If there is no non-irq usecase just drop this
>> config option.
>
> This expander is used in a number of Tegra-based boards and handles
> things like enabling or disabling power to a given IC but on other
> boards it is also used to handle interrupts from input devices or
> card-detect for example.
>
> The controller is synthesized in a CPLD, which is one of the reasons for
> the nr-gpios DT property. There is at least one platform that currently
> doesn't use the interrupt functionality. Mainly I allowed this to be
> configured out in order to reduce the number of interrupts required for
> a platform. Another reason was that at the time I first wrote this, IRQ
> domains hadn't been available, so the driver couldn't be built as a
> module if interrupt support was required. This also no longer applies.
>
> I'm actually fine either way, but I thought it'd be most flexible by
> keeping the IRQ support optional for the above reasons.
We're working on a goal of a "single zImage" (one unified ARM
kernel) which means your platform must be able to handle the
case where this is turned on anyway, so I would suggest you
drop the optional compile-time IRQ support, just make it
optional at runtime instead.
>> > + u8 *irq_mask;
>> > + u8 *irq_mask_cur;
>> > + u8 *irq_level;
>> > + u8 *irq_rise;
>> > + u8 *irq_fall;
>> > + u8 *irq_high;
>> > + u8 *irq_low;
>>
>> Some or all of this looks like a regmap reimplementation, see below.
>
> Actually none of these represent actual registers, except for irq_mask
> and irq_mask_cur. They are used to emulate various IRQ trigger modes.
OK.
>> When I do this at several places in a driver I usually define
>> a macro like
>>
>> #define to_adnp(foo) container_of(foo, struct adnp, gpio)
>>
>> Used like so:
>>
>> struct adnp *adnp = to_adnp(chip);
>
> Yes, I usually do that as well. I guess this driver has been in the
> works in too many variants for too long. =)
OK expect it to be changed in v3 then...
>> > + unsigned int reg = offset >> gpio->reg_shift;
>> > + unsigned int pos = offset & 7;
>> > + u8 value;
>> > + int err;
>> > +
>> > + mutex_lock(&gpio->i2c_lock);
>>
>> The point of taking this mutex here avoids me.
>> adnp_read() only performs a single transaction.
>
> I think that's a relic from an earlier version that used to access the
> PTR (Pin Tristate Register) as well. At the time I used to return 2 here
> to signify a tristated input, which was dependent on the contents of the
> PTR. Tristating an output is, I believe, better done using pinmux/
> pinctrl nowadays, so I took that code because the only platform where
> this was ever used will probably never be mainlined.
OK so will be gone in v3 I guess.
> On that note, provided there is special additional circuitry, the GPIO
> controller is able to detect tristate on an input. I'm not aware that
> the pinctrl subsystem provides for that functionality yet, right?
pinctrl is usually about *setting* things into tristate, but I'm all
for adding support for getting it as well. But that depends on
the generic pin configurations being adopted first (still most
controllers have their own way of doing pin config, so this
cannot be represented in a generic way).
>> > + base = kzalloc(regs * 5, GFP_KERNEL);
>>
>> Why kzalloc()/kfree() when you can just use a
>>
>> static u8 base[N];
>>
>> where N is the max number of registers on any HW instead?
>
> As I explained above, the number of pins is configurable, so it'd be
> quite a waste to always assume a maximum of, say, 256 pins if the
> hardware actually only uses 8.
OK but atleast find a way to move this to the probe() function,
what happens if the debugfs file is browsed and you run out
of memory? Not nice, and you were using this to debug as
well...
>> Usually we define the struct gpio_chip as a static const and have
>> the state containter hold a static const struct gpio_chip *chip;
>> entry assigned to point to the static const at probe time.
>
> The problem with a static const is that you can no longer configure the
> number of GPIOs at runtime, which is sort of essential for this driver.
OK keep it like this...
>> All other GPIO drivers #ifdef their debugfs code, this probably
>> works fine too, but never saw it before.
>>
>> I'm ambivalent about this, I sort of like it because it avoids
>> #ifdefs and also makes sure the code is always compiled,
>> so if you like it like this, keep it.
>
> I've started to use this wherever possible because otherwise you have to
> build numerous configurations to ensure complete compile coverage. Then
> again this also has the drawback to potentially hide issues if you don't
> properly separate conditionalized code.
OK I'll adopt to liking this.
>> (...)
>> > +static void adnp_irq_update_mask(struct adnp *gpio)
>> > +{
>> > + unsigned int regs = 1 << gpio->reg_shift;
>> > + bool equal = true;
>> > + unsigned int i;
>> > +
>> > + for (i = 0; i < regs; i++) {
>> > + if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
>> > + equal = false;
>> > + break;
>> > + }
>> > + }
>>
>> This is not looking good. It looks like you're reimplementing
>> parts of regmap.
>>
>> In that case, please use <linux/regmap.h> to cache registers
>> instead.
>>
>> But I'm not sure ...
>>
>> (...)
>> > +static void adnp_irq_bus_lock(struct irq_data *data)
>> > +{
>> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
>> > + unsigned int regs = 1 << gpio->reg_shift;
>> > +
>> > + mutex_lock(&gpio->irq_lock);
>> > + memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
>> > +}
>> > +
>> > +static void adnp_irq_bus_unlock(struct irq_data *data)
>> > +{
>> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
>> > +
>> > + adnp_irq_update_mask(gpio);
>> > + mutex_unlock(&gpio->irq_lock);
>> > +}
>>
>> Actually I'm not following why the IRQ mask registers are shadowed
>> at bus_lock and restored at bus_unlock().
>>
>> A comment describing why that's needed and how it works is
>> definately needed.
>
> I'm not sure that this is required anymore. IIRC I did copy this from
> some other driver at the time. This is probably supposed to be an
> optimization, but I think I can live without it.
If it's an optimization, please see if you can live without it and
add it back in using regmap if you want it later. regmap rocks...
>> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
>> > + if (!gpio->irq_mask)
>> > + return -ENOMEM;
>> > +
>> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
>> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
>> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
>> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
>> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
>> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
>>
>> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
>> here? Explain with some comment atleast.
>
> Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
> irq_low to keep track of the current level and trigger modes for each
> interrupt. Instead of allocating small chunks for each of these I
> allocate one chunk and just make the others point into that.
Maybe you said this would go away in this case not comments
of course.
But I wasn't getting the multiplication part. I understand the
allocation, 7 registers time the number of registers (hm, there
is something about the naming too....)
You're storing the things in such an awkward way: all
current masks for all registers sets, then all levels for
all register etc.
Instead could you store all the flags for *one* instance
then the next set of registers etc.
>> > + for (i = 0; i < regs; i++) {
>> > + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
>> > + if (err < 0)
>> > + return err;
>> > + }
>>
>> Looks like regmap reimplementation.
>
> This is used to obtain the initial pin levels, which in turn is required
> to check for rising and falling edges.
Yeah sorry I misread this code.
>> > + err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
>> > + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> > + dev_name(chip->dev), gpio);
>>
>> Since you're using devm_* above use it here too:
>> devm_request_threaded_irq().
>> > (...)
>> > +static void adnp_irq_teardown(struct adnp *gpio)
>> > +{
>> > + unsigned int irq, i;
>> > +
>> > + free_irq(gpio->client->irq, gpio);
>>
>> If you're using devm to grab the IRQ this is not needed.
>
> I don't think that'll work. In this case the interrupt needs to be freed
> before cleaning up, because otherwise the interrupt handler may still be
> run after the IRQ domain has already been removed.
Hm. Paging Grant on this one.
Grant: can't we add devm_* managed IRQ domains so this
ordering etc goes into the core as well?
>> > + return -EPROBE_DEFER;
>>
>> Why would you defer in this case? If the IRQ controller appear later
>> than the GPIO driver?
>
> Yes. In particular when using DT it can happen that the parent interrupt
> controller is probed later than this.
OK.
>> > + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
>> > + err = adnp_irq_setup(gpio);
>> > + if (err < 0)
>> > + goto teardown;
>> > + }
>>
>> And that activates the question why this should be conditional,
>> please elaborate.
>
> I think I've answered this before.
Yep, and I suggest to make this 100% runtime, not compile-time.
> Thanks for reviewing. I'll fixup the problems you've pointed out and
> will have to retest.
Thanks, waiting for v3.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-05 10:50 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-05 10:50 UTC (permalink / raw)
To: Thierry Reding
Cc: Linus Walleij, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Wolfram Sang
On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
>> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> > +- interrupt-controller: Marks the device as an interrupt controller.
>> > +- nr-gpios: The number of pins supported by the controller.
>>
>> These two last things look very generic, like something every GPIO
>> driver could want to expose.
>
> As Arnd mentioned, interrupt-controller is a generic property required
> by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> DT binding for this driver.
After reading Rob's etc comments I think nr-gpios should be in this
binding, but interrupt-controller seems like it should go into
Documentation/devicetree/bindings/gpio/gpio.txt
can you take care of this?
(Anyone agains, beat me up...)
>> > +config GPIO_ADNP
>> > + tristate "Avionic Design N-bit GPIO expander"
>> > + depends on I2C && OF
>> > + help
>> > + This option enables support for N GPIOs found on Avionic Design
>> > + I2C GPIO expanders. The register space will be extended by powers
>> > + of two, so the controller will need to accomodate for that. For
>> > + example: if a controller provides 48 pins, 6 registers will be
>> > + enough to represent all pins, but the driver will assume a
>> > + register layout for 64 pins (8 registers).
>> > +
>> > +config GPIO_ADNP_IRQ
>> > + tristate "Interrupt controller support"
>> > + depends on GPIO_ADNP
>> > + help
>> > + Say yes here to enable the Avionic Design N-bit GPIO expander to
>> > + be used as an interrupt controller.
>>
>> First: please describe the usecase where the Avionic driver is used
>> without making use of the IRQ, and *why* it should be possible
>> to configure this out. E.g. is there a hardware which isn't using the
>> IRQ portions? If there is no non-irq usecase just drop this
>> config option.
>
> This expander is used in a number of Tegra-based boards and handles
> things like enabling or disabling power to a given IC but on other
> boards it is also used to handle interrupts from input devices or
> card-detect for example.
>
> The controller is synthesized in a CPLD, which is one of the reasons for
> the nr-gpios DT property. There is at least one platform that currently
> doesn't use the interrupt functionality. Mainly I allowed this to be
> configured out in order to reduce the number of interrupts required for
> a platform. Another reason was that at the time I first wrote this, IRQ
> domains hadn't been available, so the driver couldn't be built as a
> module if interrupt support was required. This also no longer applies.
>
> I'm actually fine either way, but I thought it'd be most flexible by
> keeping the IRQ support optional for the above reasons.
We're working on a goal of a "single zImage" (one unified ARM
kernel) which means your platform must be able to handle the
case where this is turned on anyway, so I would suggest you
drop the optional compile-time IRQ support, just make it
optional at runtime instead.
>> > + u8 *irq_mask;
>> > + u8 *irq_mask_cur;
>> > + u8 *irq_level;
>> > + u8 *irq_rise;
>> > + u8 *irq_fall;
>> > + u8 *irq_high;
>> > + u8 *irq_low;
>>
>> Some or all of this looks like a regmap reimplementation, see below.
>
> Actually none of these represent actual registers, except for irq_mask
> and irq_mask_cur. They are used to emulate various IRQ trigger modes.
OK.
>> When I do this at several places in a driver I usually define
>> a macro like
>>
>> #define to_adnp(foo) container_of(foo, struct adnp, gpio)
>>
>> Used like so:
>>
>> struct adnp *adnp = to_adnp(chip);
>
> Yes, I usually do that as well. I guess this driver has been in the
> works in too many variants for too long. =)
OK expect it to be changed in v3 then...
>> > + unsigned int reg = offset >> gpio->reg_shift;
>> > + unsigned int pos = offset & 7;
>> > + u8 value;
>> > + int err;
>> > +
>> > + mutex_lock(&gpio->i2c_lock);
>>
>> The point of taking this mutex here avoids me.
>> adnp_read() only performs a single transaction.
>
> I think that's a relic from an earlier version that used to access the
> PTR (Pin Tristate Register) as well. At the time I used to return 2 here
> to signify a tristated input, which was dependent on the contents of the
> PTR. Tristating an output is, I believe, better done using pinmux/
> pinctrl nowadays, so I took that code because the only platform where
> this was ever used will probably never be mainlined.
OK so will be gone in v3 I guess.
> On that note, provided there is special additional circuitry, the GPIO
> controller is able to detect tristate on an input. I'm not aware that
> the pinctrl subsystem provides for that functionality yet, right?
pinctrl is usually about *setting* things into tristate, but I'm all
for adding support for getting it as well. But that depends on
the generic pin configurations being adopted first (still most
controllers have their own way of doing pin config, so this
cannot be represented in a generic way).
>> > + base = kzalloc(regs * 5, GFP_KERNEL);
>>
>> Why kzalloc()/kfree() when you can just use a
>>
>> static u8 base[N];
>>
>> where N is the max number of registers on any HW instead?
>
> As I explained above, the number of pins is configurable, so it'd be
> quite a waste to always assume a maximum of, say, 256 pins if the
> hardware actually only uses 8.
OK but atleast find a way to move this to the probe() function,
what happens if the debugfs file is browsed and you run out
of memory? Not nice, and you were using this to debug as
well...
>> Usually we define the struct gpio_chip as a static const and have
>> the state containter hold a static const struct gpio_chip *chip;
>> entry assigned to point to the static const at probe time.
>
> The problem with a static const is that you can no longer configure the
> number of GPIOs at runtime, which is sort of essential for this driver.
OK keep it like this...
>> All other GPIO drivers #ifdef their debugfs code, this probably
>> works fine too, but never saw it before.
>>
>> I'm ambivalent about this, I sort of like it because it avoids
>> #ifdefs and also makes sure the code is always compiled,
>> so if you like it like this, keep it.
>
> I've started to use this wherever possible because otherwise you have to
> build numerous configurations to ensure complete compile coverage. Then
> again this also has the drawback to potentially hide issues if you don't
> properly separate conditionalized code.
OK I'll adopt to liking this.
>> (...)
>> > +static void adnp_irq_update_mask(struct adnp *gpio)
>> > +{
>> > + unsigned int regs = 1 << gpio->reg_shift;
>> > + bool equal = true;
>> > + unsigned int i;
>> > +
>> > + for (i = 0; i < regs; i++) {
>> > + if (gpio->irq_mask[i] != gpio->irq_mask_cur[i]) {
>> > + equal = false;
>> > + break;
>> > + }
>> > + }
>>
>> This is not looking good. It looks like you're reimplementing
>> parts of regmap.
>>
>> In that case, please use <linux/regmap.h> to cache registers
>> instead.
>>
>> But I'm not sure ...
>>
>> (...)
>> > +static void adnp_irq_bus_lock(struct irq_data *data)
>> > +{
>> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
>> > + unsigned int regs = 1 << gpio->reg_shift;
>> > +
>> > + mutex_lock(&gpio->irq_lock);
>> > + memcpy(gpio->irq_mask_cur, gpio->irq_mask, regs);
>> > +}
>> > +
>> > +static void adnp_irq_bus_unlock(struct irq_data *data)
>> > +{
>> > + struct adnp *gpio = irq_data_get_irq_chip_data(data);
>> > +
>> > + adnp_irq_update_mask(gpio);
>> > + mutex_unlock(&gpio->irq_lock);
>> > +}
>>
>> Actually I'm not following why the IRQ mask registers are shadowed
>> at bus_lock and restored at bus_unlock().
>>
>> A comment describing why that's needed and how it works is
>> definately needed.
>
> I'm not sure that this is required anymore. IIRC I did copy this from
> some other driver at the time. This is probably supposed to be an
> optimization, but I think I can live without it.
If it's an optimization, please see if you can live without it and
add it back in using regmap if you want it later. regmap rocks...
>> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
>> > + if (!gpio->irq_mask)
>> > + return -ENOMEM;
>> > +
>> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
>> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
>> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
>> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
>> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
>> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
>>
>> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
>> here? Explain with some comment atleast.
>
> Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
> irq_low to keep track of the current level and trigger modes for each
> interrupt. Instead of allocating small chunks for each of these I
> allocate one chunk and just make the others point into that.
Maybe you said this would go away in this case not comments
of course.
But I wasn't getting the multiplication part. I understand the
allocation, 7 registers time the number of registers (hm, there
is something about the naming too....)
You're storing the things in such an awkward way: all
current masks for all registers sets, then all levels for
all register etc.
Instead could you store all the flags for *one* instance
then the next set of registers etc.
>> > + for (i = 0; i < regs; i++) {
>> > + err = adnp_read(gpio, GPIO_PLR(gpio) + i, &gpio->irq_level[i]);
>> > + if (err < 0)
>> > + return err;
>> > + }
>>
>> Looks like regmap reimplementation.
>
> This is used to obtain the initial pin levels, which in turn is required
> to check for rising and falling edges.
Yeah sorry I misread this code.
>> > + err = request_threaded_irq(gpio->client->irq, NULL, adnp_irq,
>> > + IRQF_TRIGGER_RISING | IRQF_ONESHOT,
>> > + dev_name(chip->dev), gpio);
>>
>> Since you're using devm_* above use it here too:
>> devm_request_threaded_irq().
>> > (...)
>> > +static void adnp_irq_teardown(struct adnp *gpio)
>> > +{
>> > + unsigned int irq, i;
>> > +
>> > + free_irq(gpio->client->irq, gpio);
>>
>> If you're using devm to grab the IRQ this is not needed.
>
> I don't think that'll work. In this case the interrupt needs to be freed
> before cleaning up, because otherwise the interrupt handler may still be
> run after the IRQ domain has already been removed.
Hm. Paging Grant on this one.
Grant: can't we add devm_* managed IRQ domains so this
ordering etc goes into the core as well?
>> > + return -EPROBE_DEFER;
>>
>> Why would you defer in this case? If the IRQ controller appear later
>> than the GPIO driver?
>
> Yes. In particular when using DT it can happen that the parent interrupt
> controller is probed later than this.
OK.
>> > + if (IS_ENABLED(CONFIG_GPIO_ADNP_IRQ)) {
>> > + err = adnp_irq_setup(gpio);
>> > + if (err < 0)
>> > + goto teardown;
>> > + }
>>
>> And that activates the question why this should be conditional,
>> please elaborate.
>
> I think I've answered this before.
Yep, and I suggest to make this 100% runtime, not compile-time.
> Thanks for reviewing. I'll fixup the problems you've pointed out and
> will have to retest.
Thanks, waiting for v3.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-06 5:11 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-08-06 5:11 UTC (permalink / raw)
To: Linus Walleij
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 7566 bytes --]
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
> On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> >> <thierry.reding@avionic-design.de> wrote:
>
> >> > +- interrupt-controller: Marks the device as an interrupt controller.
> >> > +- nr-gpios: The number of pins supported by the controller.
> >>
> >> These two last things look very generic, like something every GPIO
> >> driver could want to expose.
> >
> > As Arnd mentioned, interrupt-controller is a generic property required
> > by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> > DT binding for this driver.
>
> After reading Rob's etc comments I think nr-gpios should be in this
> binding, but interrupt-controller seems like it should go into
> Documentation/devicetree/bindings/gpio/gpio.txt
> can you take care of this?
>
> (Anyone agains, beat me up...)
Yes, that makes sense. The interrupt-controller property is not
explicitly mentioned in Documentation/devicetree at all. Perhaps the
reason is that it is in fact a standard property and, I think, is
already defined by IEEE 1275. I don't think it would hurt to repeat it
explicitly for GPIO bindings in general, though.
> >> > +config GPIO_ADNP
> >> > + tristate "Avionic Design N-bit GPIO expander"
> >> > + depends on I2C && OF
> >> > + help
> >> > + This option enables support for N GPIOs found on Avionic Design
> >> > + I2C GPIO expanders. The register space will be extended by powers
> >> > + of two, so the controller will need to accomodate for that. For
> >> > + example: if a controller provides 48 pins, 6 registers will be
> >> > + enough to represent all pins, but the driver will assume a
> >> > + register layout for 64 pins (8 registers).
> >> > +
> >> > +config GPIO_ADNP_IRQ
> >> > + tristate "Interrupt controller support"
> >> > + depends on GPIO_ADNP
> >> > + help
> >> > + Say yes here to enable the Avionic Design N-bit GPIO expander to
> >> > + be used as an interrupt controller.
> >>
> >> First: please describe the usecase where the Avionic driver is used
> >> without making use of the IRQ, and *why* it should be possible
> >> to configure this out. E.g. is there a hardware which isn't using the
> >> IRQ portions? If there is no non-irq usecase just drop this
> >> config option.
> >
> > This expander is used in a number of Tegra-based boards and handles
> > things like enabling or disabling power to a given IC but on other
> > boards it is also used to handle interrupts from input devices or
> > card-detect for example.
> >
> > The controller is synthesized in a CPLD, which is one of the reasons for
> > the nr-gpios DT property. There is at least one platform that currently
> > doesn't use the interrupt functionality. Mainly I allowed this to be
> > configured out in order to reduce the number of interrupts required for
> > a platform. Another reason was that at the time I first wrote this, IRQ
> > domains hadn't been available, so the driver couldn't be built as a
> > module if interrupt support was required. This also no longer applies.
> >
> > I'm actually fine either way, but I thought it'd be most flexible by
> > keeping the IRQ support optional for the above reasons.
>
> We're working on a goal of a "single zImage" (one unified ARM
> kernel) which means your platform must be able to handle the
> case where this is turned on anyway, so I would suggest you
> drop the optional compile-time IRQ support, just make it
> optional at runtime instead.
I don't quite understand. Do you want me to add a module parameter to
make it optional at runtime? Since the driver is now OF only I suppose I
could make it optional on the interrupt-controller property as well.
> > On that note, provided there is special additional circuitry, the GPIO
> > controller is able to detect tristate on an input. I'm not aware that
> > the pinctrl subsystem provides for that functionality yet, right?
>
> pinctrl is usually about *setting* things into tristate, but I'm all
> for adding support for getting it as well. But that depends on
> the generic pin configurations being adopted first (still most
> controllers have their own way of doing pin config, so this
> cannot be represented in a generic way).
As I mentioned, the only hardware where this was ever used is already
EOL and I don't expect this functionality to be required anytime soon
for another project.
> >> > + base = kzalloc(regs * 5, GFP_KERNEL);
> >>
> >> Why kzalloc()/kfree() when you can just use a
> >>
> >> static u8 base[N];
> >>
> >> where N is the max number of registers on any HW instead?
> >
> > As I explained above, the number of pins is configurable, so it'd be
> > quite a waste to always assume a maximum of, say, 256 pins if the
> > hardware actually only uses 8.
>
> OK but atleast find a way to move this to the probe() function,
> what happens if the debugfs file is browsed and you run out
> of memory? Not nice, and you were using this to debug as
> well...
Alright, I can do that. Alternatively I could probably drop the
allocations altogether and use local variables within the second loop to
store the variables:
for (i = 0; i < num_regs; i++) {
u8 ddr, plr, ier, isr, ptr;
err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr);
if (err < 0)
goto out;
...
}
With the proper locking this shouldn't be a problem. The reason why I
used the block-wise approach in the first place was that the register
accesses were more "atomic". Of course without locking this is non-
sense.
> >> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> >> > + if (!gpio->irq_mask)
> >> > + return -ENOMEM;
> >> > +
> >> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> >> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
> >> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> >> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> >> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
> >> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
> >>
> >> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
> >> here? Explain with some comment atleast.
> >
> > Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
> > irq_low to keep track of the current level and trigger modes for each
> > interrupt. Instead of allocating small chunks for each of these I
> > allocate one chunk and just make the others point into that.
>
> Maybe you said this would go away in this case not comments
> of course.
irq_mask and irq_mask_cur can go away I think.
> But I wasn't getting the multiplication part. I understand the
> allocation, 7 registers time the number of registers (hm, there
> is something about the naming too....)
You're right, regs should probably be called num_regs.
> You're storing the things in such an awkward way: all
> current masks for all registers sets, then all levels for
> all register etc.
>
> Instead could you store all the flags for *one* instance
> then the next set of registers etc.
I was following the register layout of the controller to keep things
consistent.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-06 5:11 ` Thierry Reding
0 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-08-06 5:11 UTC (permalink / raw)
To: Linus Walleij
Cc: Linus Walleij, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Wolfram Sang
[-- Attachment #1.1: Type: text/plain, Size: 7630 bytes --]
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
> On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding
> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> >> <thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>
> >> > +- interrupt-controller: Marks the device as an interrupt controller.
> >> > +- nr-gpios: The number of pins supported by the controller.
> >>
> >> These two last things look very generic, like something every GPIO
> >> driver could want to expose.
> >
> > As Arnd mentioned, interrupt-controller is a generic property required
> > by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> > DT binding for this driver.
>
> After reading Rob's etc comments I think nr-gpios should be in this
> binding, but interrupt-controller seems like it should go into
> Documentation/devicetree/bindings/gpio/gpio.txt
> can you take care of this?
>
> (Anyone agains, beat me up...)
Yes, that makes sense. The interrupt-controller property is not
explicitly mentioned in Documentation/devicetree at all. Perhaps the
reason is that it is in fact a standard property and, I think, is
already defined by IEEE 1275. I don't think it would hurt to repeat it
explicitly for GPIO bindings in general, though.
> >> > +config GPIO_ADNP
> >> > + tristate "Avionic Design N-bit GPIO expander"
> >> > + depends on I2C && OF
> >> > + help
> >> > + This option enables support for N GPIOs found on Avionic Design
> >> > + I2C GPIO expanders. The register space will be extended by powers
> >> > + of two, so the controller will need to accomodate for that. For
> >> > + example: if a controller provides 48 pins, 6 registers will be
> >> > + enough to represent all pins, but the driver will assume a
> >> > + register layout for 64 pins (8 registers).
> >> > +
> >> > +config GPIO_ADNP_IRQ
> >> > + tristate "Interrupt controller support"
> >> > + depends on GPIO_ADNP
> >> > + help
> >> > + Say yes here to enable the Avionic Design N-bit GPIO expander to
> >> > + be used as an interrupt controller.
> >>
> >> First: please describe the usecase where the Avionic driver is used
> >> without making use of the IRQ, and *why* it should be possible
> >> to configure this out. E.g. is there a hardware which isn't using the
> >> IRQ portions? If there is no non-irq usecase just drop this
> >> config option.
> >
> > This expander is used in a number of Tegra-based boards and handles
> > things like enabling or disabling power to a given IC but on other
> > boards it is also used to handle interrupts from input devices or
> > card-detect for example.
> >
> > The controller is synthesized in a CPLD, which is one of the reasons for
> > the nr-gpios DT property. There is at least one platform that currently
> > doesn't use the interrupt functionality. Mainly I allowed this to be
> > configured out in order to reduce the number of interrupts required for
> > a platform. Another reason was that at the time I first wrote this, IRQ
> > domains hadn't been available, so the driver couldn't be built as a
> > module if interrupt support was required. This also no longer applies.
> >
> > I'm actually fine either way, but I thought it'd be most flexible by
> > keeping the IRQ support optional for the above reasons.
>
> We're working on a goal of a "single zImage" (one unified ARM
> kernel) which means your platform must be able to handle the
> case where this is turned on anyway, so I would suggest you
> drop the optional compile-time IRQ support, just make it
> optional at runtime instead.
I don't quite understand. Do you want me to add a module parameter to
make it optional at runtime? Since the driver is now OF only I suppose I
could make it optional on the interrupt-controller property as well.
> > On that note, provided there is special additional circuitry, the GPIO
> > controller is able to detect tristate on an input. I'm not aware that
> > the pinctrl subsystem provides for that functionality yet, right?
>
> pinctrl is usually about *setting* things into tristate, but I'm all
> for adding support for getting it as well. But that depends on
> the generic pin configurations being adopted first (still most
> controllers have their own way of doing pin config, so this
> cannot be represented in a generic way).
As I mentioned, the only hardware where this was ever used is already
EOL and I don't expect this functionality to be required anytime soon
for another project.
> >> > + base = kzalloc(regs * 5, GFP_KERNEL);
> >>
> >> Why kzalloc()/kfree() when you can just use a
> >>
> >> static u8 base[N];
> >>
> >> where N is the max number of registers on any HW instead?
> >
> > As I explained above, the number of pins is configurable, so it'd be
> > quite a waste to always assume a maximum of, say, 256 pins if the
> > hardware actually only uses 8.
>
> OK but atleast find a way to move this to the probe() function,
> what happens if the debugfs file is browsed and you run out
> of memory? Not nice, and you were using this to debug as
> well...
Alright, I can do that. Alternatively I could probably drop the
allocations altogether and use local variables within the second loop to
store the variables:
for (i = 0; i < num_regs; i++) {
u8 ddr, plr, ier, isr, ptr;
err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr);
if (err < 0)
goto out;
...
}
With the proper locking this shouldn't be a problem. The reason why I
used the block-wise approach in the first place was that the register
accesses were more "atomic". Of course without locking this is non-
sense.
> >> > + gpio->irq_mask = devm_kzalloc(chip->dev, regs * 7, GFP_KERNEL);
> >> > + if (!gpio->irq_mask)
> >> > + return -ENOMEM;
> >> > +
> >> > + gpio->irq_mask_cur = gpio->irq_mask + (regs * 1);
> >> > + gpio->irq_level = gpio->irq_mask + (regs * 2);
> >> > + gpio->irq_rise = gpio->irq_mask + (regs * 3);
> >> > + gpio->irq_fall = gpio->irq_mask + (regs * 4);
> >> > + gpio->irq_high = gpio->irq_mask + (regs * 5);
> >> > + gpio->irq_low = gpio->irq_mask + (regs * 6);
> >>
> >> I'm not following this regs * 1, regs * 2 ... regs *7 stuff. What are you doing
> >> here? Explain with some comment atleast.
> >
> > Basically I need at least irq_level, irq_rise, irq_fall, irq_high and
> > irq_low to keep track of the current level and trigger modes for each
> > interrupt. Instead of allocating small chunks for each of these I
> > allocate one chunk and just make the others point into that.
>
> Maybe you said this would go away in this case not comments
> of course.
irq_mask and irq_mask_cur can go away I think.
> But I wasn't getting the multiplication part. I understand the
> allocation, 7 registers time the number of registers (hm, there
> is something about the naming too....)
You're right, regs should probably be called num_regs.
> You're storing the things in such an awkward way: all
> current masks for all registers sets, then all levels for
> all register etc.
>
> Instead could you store all the flags for *one* instance
> then the next set of registers etc.
I was following the register layout of the controller to keep things
consistent.
Thierry
[-- Attachment #1.2: Type: application/pgp-signature, Size: 836 bytes --]
[-- Attachment #2: Type: text/plain, Size: 192 bytes --]
_______________________________________________
devicetree-discuss mailing list
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-06 6:39 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-06 6:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
On Mon, Aug 6, 2012 at 7:11 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
>> We're working on a goal of a "single zImage" (one unified ARM
>> kernel) which means your platform must be able to handle the
>> case where this is turned on anyway, so I would suggest you
>> drop the optional compile-time IRQ support, just make it
>> optional at runtime instead.
>
> I don't quite understand. Do you want me to add a module parameter to
> make it optional at runtime? Since the driver is now OF only I suppose I
> could make it optional on the interrupt-controller property as well.
No, no module parameter. Just don't register the IRQ domain if there
are not IRQ resources in the device tree, if the interrupt-controller
property is not present I guess?
>> OK but atleast find a way to move this to the probe() function,
>> what happens if the debugfs file is browsed and you run out
>> of memory? Not nice, and you were using this to debug as
>> well...
>
> Alright, I can do that. Alternatively I could probably drop the
> allocations altogether and use local variables within the second loop to
> store the variables:
>
> for (i = 0; i < num_regs; i++) {
> u8 ddr, plr, ier, isr, ptr;
>
> err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr);
> if (err < 0)
> goto out;
>
> ...
> }
>
> With the proper locking this shouldn't be a problem. The reason why I
> used the block-wise approach in the first place was that the register
> accesses were more "atomic". Of course without locking this is non-
> sense.
Either approach works, the above seems more elegant though!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-06 6:39 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-06 6:39 UTC (permalink / raw)
To: Thierry Reding
Cc: Linus Walleij, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Wolfram Sang
On Mon, Aug 6, 2012 at 7:11 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
>> We're working on a goal of a "single zImage" (one unified ARM
>> kernel) which means your platform must be able to handle the
>> case where this is turned on anyway, so I would suggest you
>> drop the optional compile-time IRQ support, just make it
>> optional at runtime instead.
>
> I don't quite understand. Do you want me to add a module parameter to
> make it optional at runtime? Since the driver is now OF only I suppose I
> could make it optional on the interrupt-controller property as well.
No, no module parameter. Just don't register the IRQ domain if there
are not IRQ resources in the device tree, if the interrupt-controller
property is not present I guess?
>> OK but atleast find a way to move this to the probe() function,
>> what happens if the debugfs file is browsed and you run out
>> of memory? Not nice, and you were using this to debug as
>> well...
>
> Alright, I can do that. Alternatively I could probably drop the
> allocations altogether and use local variables within the second loop to
> store the variables:
>
> for (i = 0; i < num_regs; i++) {
> u8 ddr, plr, ier, isr, ptr;
>
> err = adnp_read(gpio, GPIO_DDR(gpio) + i, &ddr);
> if (err < 0)
> goto out;
>
> ...
> }
>
> With the proper locking this shouldn't be a problem. The reason why I
> used the block-wise approach in the first place was that the register
> accesses were more "atomic". Of course without locking this is non-
> sense.
Either approach works, the above seems more elegant though!
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-08-05 10:50 ` Linus Walleij
(?)
(?)
@ 2012-08-09 6:27 ` Thierry Reding
-1 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-08-09 6:27 UTC (permalink / raw)
To: Linus Walleij
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 1593 bytes --]
On Sun, Aug 05, 2012 at 12:50:54PM +0200, Linus Walleij wrote:
> On Mon, Jul 30, 2012 at 9:47 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> >> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> >> <thierry.reding@avionic-design.de> wrote:
>
> >> > +- interrupt-controller: Marks the device as an interrupt controller.
> >> > +- nr-gpios: The number of pins supported by the controller.
> >>
> >> These two last things look very generic, like something every GPIO
> >> driver could want to expose.
> >
> > As Arnd mentioned, interrupt-controller is a generic property required
> > by all interrupt controller nodes. Perhaps it shouldn't be listed in the
> > DT binding for this driver.
>
> After reading Rob's etc comments I think nr-gpios should be in this
> binding, but interrupt-controller seems like it should go into
> Documentation/devicetree/bindings/gpio/gpio.txt
> can you take care of this?
>
> (Anyone agains, beat me up...)
Perhaps this should go into some more generic document (perhaps
something like Documentation/devicetree/bindings/interrupts.txt) to
describe the common properties for interrupt controllers and refer to
that document in bindings that use them.
It might be good to document the #interrupt-cells property as well, with
the common meaning as defined by the various irq_domain_ops.xlate()
callbacks.
Do you want me to replace occurrences of this in other binding documents
by a reference to the new document while I'm at it?
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-07-29 17:13 ` Linus Walleij
` (2 preceding siblings ...)
(?)
@ 2012-08-09 20:20 ` Thierry Reding
2012-08-10 8:19 ` Linus Walleij
-1 siblings, 1 reply; 27+ messages in thread
From: Thierry Reding @ 2012-08-09 20:20 UTC (permalink / raw)
To: Linus Walleij
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 989 bytes --]
On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> On Mon, Jul 23, 2012 at 1:59 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > +static __devinit int adnp_i2c_probe(struct i2c_client *client,
> > + const struct i2c_device_id *id)
> > +{
> > + struct adnp *gpio;
> > + u32 num_gpios;
> > + int err;
> > +
> > + err = of_property_read_u32(client->dev.of_node, "nr-gpios",
> > + &num_gpios);
> > + if (err < 0)
> > + return err;
> > +
> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> > + if (client->irq == NO_IRQ)
>
> Just if (!client->irq) since NO_IRQ is 0 nowadays.
At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
However, irq_of_parse_and_map() returns 0 if the interrupt cannot be
parsed or mapped, so checking for !client->irq is, as you say, correct.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-10 8:19 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-10 8:19 UTC (permalink / raw)
To: Thierry Reding
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang,
Russell King - ARM Linux
On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
>> > + if (client->irq == NO_IRQ)
>>
>> Just if (!client->irq) since NO_IRQ is 0 nowadays.
>
> At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
No. A year back, yes, but not anymore. We went to great lengths in the
ARM architecture to ensure NO_IRQ is *always 0. Russell spent
a lot of time on this.
Consult the following article on LWN:
http://lwn.net/Articles/470820/
Then grep your gitlog and you'll see we got rid of it from ARM.
If this driver is for some other arch like openrisc I might accept
it but please reconsider.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-10 8:19 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-10 8:19 UTC (permalink / raw)
To: Thierry Reding
Cc: Russell King - ARM Linux, Linus Walleij,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Wolfram Sang
On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
> On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
>> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
>> > + if (client->irq == NO_IRQ)
>>
>> Just if (!client->irq) since NO_IRQ is 0 nowadays.
>
> At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
No. A year back, yes, but not anymore. We went to great lengths in the
ARM architecture to ensure NO_IRQ is *always 0. Russell spent
a lot of time on this.
Consult the following article on LWN:
http://lwn.net/Articles/470820/
Then grep your gitlog and you'll see we got rid of it from ARM.
If this driver is for some other arch like openrisc I might accept
it but please reconsider.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-08-10 8:19 ` Linus Walleij
(?)
@ 2012-08-10 8:35 ` Thierry Reding
2012-08-10 8:41 ` Linus Walleij
-1 siblings, 1 reply; 27+ messages in thread
From: Thierry Reding @ 2012-08-10 8:35 UTC (permalink / raw)
To: Linus Walleij
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang,
Russell King - ARM Linux
[-- Attachment #1: Type: text/plain, Size: 1491 bytes --]
On Fri, Aug 10, 2012 at 10:19:02AM +0200, Linus Walleij wrote:
> On Thu, Aug 9, 2012 at 10:20 PM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> > On Sun, Jul 29, 2012 at 07:13:57PM +0200, Linus Walleij wrote:
> >> > + client->irq = irq_of_parse_and_map(client->dev.of_node, 0);
> >> > + if (client->irq == NO_IRQ)
> >>
> >> Just if (!client->irq) since NO_IRQ is 0 nowadays.
> >
> > At the risk of seeming pedantic, NO_IRQ is in fact quite often not 0.
>
> No. A year back, yes, but not anymore. We went to great lengths in the
> ARM architecture to ensure NO_IRQ is *always 0. Russell spent
> a lot of time on this.
>
> Consult the following article on LWN:
> http://lwn.net/Articles/470820/
>
> Then grep your gitlog and you'll see we got rid of it from ARM.
Then why is there still the following in arch/arm/include/asm/irq.h?
/*
* Use this value to indicate lack of interrupt
* capability
*/
#ifndef NO_IRQ
#define NO_IRQ ((unsigned int)(-1))
#endif
> If this driver is for some other arch like openrisc I might accept
> it but please reconsider.
There's nothing in the driver that makes it ARM specific, so it could be
used on other platforms just as well. But as I also said in my previous
mail, in this particular case the value for the interrupt comes from the
call to irq_of_parse_and_map(), which will return 0 on failure,
regardless of the architecture, so there is actually no problem.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-10 8:41 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-10 8:41 UTC (permalink / raw)
To: Thierry Reding, Russell King - ARM Linux
Cc: Grant Likely, Arnd Bergmann, linux-kernel, devicetree-discuss,
Linus Walleij, Rob Herring, Wolfram Sang
On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
<thierry.reding@avionic-design.de> wrote:
>> Consult the following article on LWN:
>> http://lwn.net/Articles/470820/
>>
>> Then grep your gitlog and you'll see we got rid of it from ARM.
>
> Then why is there still the following in arch/arm/include/asm/irq.h?
>
> /*
> * Use this value to indicate lack of interrupt
> * capability
> */
> #ifndef NO_IRQ
> #define NO_IRQ ((unsigned int)(-1))
> #endif
That's a question for Russell but I think it's basically there for
old platforms, on a "don't use it"-basis. (Maybe a comment could
be good.)
As you see non-sparse platforms can redefine NO_IRQS in their
<mach/irqs.h> file, but in practice things like the VIC and GIC
drivers have been switched over to using irqdomain which
in turn does *not* allow IRQ 0 to be used, so most platforms
are indirectly disallowed to use IRQ 0 anyway. In fact I think
some of them are just broken now.
>> If this driver is for some other arch like openrisc I might accept
>> it but please reconsider.
>
> There's nothing in the driver that makes it ARM specific, so it could be
> used on other platforms just as well.
The linked article makes it clear that the direction for the entire
kernel is to get rid of NO_IRQ and !irq is used all over the place.
> But as I also said in my previous
> mail, in this particular case the value for the interrupt comes from the
> call to irq_of_parse_and_map(), which will return 0 on failure,
> regardless of the architecture, so there is actually no problem.
OK cool.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
@ 2012-08-10 8:41 ` Linus Walleij
0 siblings, 0 replies; 27+ messages in thread
From: Linus Walleij @ 2012-08-10 8:41 UTC (permalink / raw)
To: Thierry Reding, Russell King - ARM Linux
Cc: Linus Walleij, devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ,
linux-kernel-u79uwXL29TY76Z2rM5mHXA, Rob Herring, Wolfram Sang
On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
<thierry.reding-RM9K5IK7kjKj5M59NBduVrNAH6kLmebB@public.gmane.org> wrote:
>> Consult the following article on LWN:
>> http://lwn.net/Articles/470820/
>>
>> Then grep your gitlog and you'll see we got rid of it from ARM.
>
> Then why is there still the following in arch/arm/include/asm/irq.h?
>
> /*
> * Use this value to indicate lack of interrupt
> * capability
> */
> #ifndef NO_IRQ
> #define NO_IRQ ((unsigned int)(-1))
> #endif
That's a question for Russell but I think it's basically there for
old platforms, on a "don't use it"-basis. (Maybe a comment could
be good.)
As you see non-sparse platforms can redefine NO_IRQS in their
<mach/irqs.h> file, but in practice things like the VIC and GIC
drivers have been switched over to using irqdomain which
in turn does *not* allow IRQ 0 to be used, so most platforms
are indirectly disallowed to use IRQ 0 anyway. In fact I think
some of them are just broken now.
>> If this driver is for some other arch like openrisc I might accept
>> it but please reconsider.
>
> There's nothing in the driver that makes it ARM specific, so it could be
> used on other platforms just as well.
The linked article makes it clear that the direction for the entire
kernel is to get rid of NO_IRQ and !irq is used all over the place.
> But as I also said in my previous
> mail, in this particular case the value for the interrupt comes from the
> call to irq_of_parse_and_map(), which will return 0 on failure,
> regardless of the architecture, so there is actually no problem.
OK cool.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-08-10 8:41 ` Linus Walleij
(?)
@ 2012-08-10 8:48 ` Thierry Reding
-1 siblings, 0 replies; 27+ messages in thread
From: Thierry Reding @ 2012-08-10 8:48 UTC (permalink / raw)
To: Linus Walleij
Cc: Russell King - ARM Linux, Grant Likely, Arnd Bergmann,
linux-kernel, devicetree-discuss, Linus Walleij, Rob Herring,
Wolfram Sang
[-- Attachment #1: Type: text/plain, Size: 1281 bytes --]
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote:
> On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> >> Consult the following article on LWN:
> >> http://lwn.net/Articles/470820/
> >>
> >> Then grep your gitlog and you'll see we got rid of it from ARM.
> >
> > Then why is there still the following in arch/arm/include/asm/irq.h?
> >
> > /*
> > * Use this value to indicate lack of interrupt
> > * capability
> > */
> > #ifndef NO_IRQ
> > #define NO_IRQ ((unsigned int)(-1))
> > #endif
>
> That's a question for Russell but I think it's basically there for
> old platforms, on a "don't use it"-basis. (Maybe a comment could
> be good.)
>
> As you see non-sparse platforms can redefine NO_IRQS in their
> <mach/irqs.h> file, but in practice things like the VIC and GIC
> drivers have been switched over to using irqdomain which
> in turn does *not* allow IRQ 0 to be used, so most platforms
> are indirectly disallowed to use IRQ 0 anyway. In fact I think
> some of them are just broken now.
In that case it might be better to just drop it altogether and wait
until people with the broken platforms start complaining.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 27+ messages in thread
* Re: [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support
2012-08-10 8:41 ` Linus Walleij
(?)
(?)
@ 2012-08-10 9:15 ` Russell King - ARM Linux
-1 siblings, 0 replies; 27+ messages in thread
From: Russell King - ARM Linux @ 2012-08-10 9:15 UTC (permalink / raw)
To: Linus Walleij
Cc: Thierry Reding, Grant Likely, Arnd Bergmann, linux-kernel,
devicetree-discuss, Linus Walleij, Rob Herring, Wolfram Sang
On Fri, Aug 10, 2012 at 10:41:58AM +0200, Linus Walleij wrote:
> On Fri, Aug 10, 2012 at 10:35 AM, Thierry Reding
> <thierry.reding@avionic-design.de> wrote:
> >> Consult the following article on LWN:
> >> http://lwn.net/Articles/470820/
> >>
> >> Then grep your gitlog and you'll see we got rid of it from ARM.
> >
> > Then why is there still the following in arch/arm/include/asm/irq.h?
> >
> > /*
> > * Use this value to indicate lack of interrupt
> > * capability
> > */
> > #ifndef NO_IRQ
> > #define NO_IRQ ((unsigned int)(-1))
> > #endif
>
> That's a question for Russell but I think it's basically there for
> old platforms, on a "don't use it"-basis. (Maybe a comment could
> be good.)
Just don't use it. It's there for old stuff which still needs fixing.
New code should not use it, and should test for one of:
irq <= 0
irq == 0
And new code should set irq = 0 to indicate a lack of interrupt.
^ permalink raw reply [flat|nested] 27+ messages in thread
end of thread, other threads:[~2012-08-10 9:16 UTC | newest]
Thread overview: 27+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-23 11:59 [PATCH v2] gpio: Add Avionic Design N-bit GPIO expander support Thierry Reding
2012-07-23 11:59 ` Thierry Reding
2012-07-29 17:13 ` Linus Walleij
2012-07-29 17:13 ` Linus Walleij
2012-07-29 20:27 ` Arnd Bergmann
2012-07-30 7:47 ` Thierry Reding
2012-07-30 7:47 ` Thierry Reding
2012-07-31 22:03 ` Rob Herring
2012-07-31 22:03 ` Rob Herring
2012-07-31 23:22 ` Stephen Warren
2012-08-02 6:18 ` Thierry Reding
2012-08-02 6:18 ` Thierry Reding
2012-08-05 10:50 ` Linus Walleij
2012-08-05 10:50 ` Linus Walleij
2012-08-06 5:11 ` Thierry Reding
2012-08-06 5:11 ` Thierry Reding
2012-08-06 6:39 ` Linus Walleij
2012-08-06 6:39 ` Linus Walleij
2012-08-09 6:27 ` Thierry Reding
2012-08-09 20:20 ` Thierry Reding
2012-08-10 8:19 ` Linus Walleij
2012-08-10 8:19 ` Linus Walleij
2012-08-10 8:35 ` Thierry Reding
2012-08-10 8:41 ` Linus Walleij
2012-08-10 8:41 ` Linus Walleij
2012-08-10 8:48 ` Thierry Reding
2012-08-10 9:15 ` Russell King - ARM Linux
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.