On Mon, Aug 5, 2013 at 11:59 PM, Fabian Vogt <fabian@xxxxxxxxxxxxxx> wrote:No problem :)
From: Fabian Vogt <fabian@xxxxxxxxxxxxxx>
This driver supports the GPIO controller found in LSI ZEVIO SoCs.
It has been successfully tested on a TI nspire CX calculator.
Signed-off-by: Fabian Vogt <fabian@xxxxxxxxxxxxxx>
Hi Fabian, sorry for taking so long for getting around to review this.
Any ideas which mail addresses I should add to CC:?+++ b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
I want an ACK from one of the DT bindings maintainers for this
portion of the driver ideally. (It looks all right to me.)
The problem is, no documents about this SoC are available at all.+config GPIO_ZEVIO
+ bool "LSI ZEVIO SoC memory mapped GPIOs"
+ depends on ARCH_NSPIRE
Can't this appear in some other SoC?
It doesn't seem very arch-dependent in itself.Not my work, but indeed, it's very impressive!
I would rather have this depend on OF so any device-tree enabled
SoC may use it, plus we get compile coverage by allmodconfig.
diff --git a/drivers/gpio/gpio-zevio.c b/drivers/gpio/gpio-zevio.c(...)+/*
+ * Memory layout:
+ * This chip has four gpio sections, each controls 8 GPIOs.
+ * Bit 0 in section 0 is GPIO 0, bit 2 in section 1 is GPIO 10.
+ * Disclaimer: Reverse engineered!
+ * For more information refer to:
+ *
http://hackspire.unsads.com/wiki/index.php/Memory-mapped_I/O_ports#90000000_-_General_Purpose_I.2FO_.28GPIO.29
Very ambitious. Impressive!
It's how the GPIO controller signals the VIC.+ * 0x00-0x3F: Section 0
+ * +0x00: Masked interrupt status (read-only)
+ * +0x04: R: Interrupt status W: Reset interrupt status
+ * +0x08: R: Interrupt mask W: Mask interrupt
+ * +0x0C: W: Unmask interrupt (write-only)
+ * +0x10: Direction: I/O=1/0
+ * +0x14: Output
+ * +0x18: Input (read-only)
+ * +0x20: R: Sticky interrupts W: Set sticky interrupt
What is a sticky interrupt? Do you mean it is a level IRQ?
Then it's edge triggered if zero and level triggered if "sticky"
is set to 1, right?
It's used VERY often and I couldn't find any coding style document which prefers u16..+/* Functions for struct gpio_chip */
+static int zevio_gpio_get(struct gpio_chip *chip, unsigned pin)
+{
+ struct zevio_gpio *controller = to_zevio_gpio(chip);
+
+ /* Only reading allowed, so no spinlock needed */
+ uint16_t val = readw(ZEVIO_GPIO(controller, pin, INPUT));
Use just u16 please. uint16_t is some portable C type.
Please replace uint16_t with u16 everywhere.
Ok.+
+ return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
Use this construct:
return !!(val & ZEVIO_GPIO_BIT(pin));
Oh, it seems I have overseen that..+static void zevio_gpio_set(struct gpio_chip *chip, unsigned pin, int value)
+{
+ struct zevio_gpio *controller = to_zevio_gpio(chip);
+ uint16_t val;
+
+ spin_lock(&controller->lock);
+ val = readw(ZEVIO_GPIO(controller, pin, OUTPUT));
+ if (value)
+ val |= 1<<ZEVIO_GPIO_BIT(pin);
I usually do this:
#include <linux/bitops.h>
val |= BIT(ZEVIO_GPIO_BIT(pin));
Would be great, but it doesn't support multiple registers (4*8 GPIOs),+ else
+ val &= ~(1<<ZEVIO_GPIO_BIT(pin));
Dito, sort of...
+
+ writew(val, ZEVIO_GPIO(controller, pin, OUTPUT));
+ spin_unlock(&controller->lock);
+}
+
+static int zevio_gpio_direction_input(struct gpio_chip *chip, unsigned pin)
+{
+ struct zevio_gpio *controller = to_zevio_gpio(chip);
+ uint16_t val;
+
+ spin_lock(&controller->lock);
+
+ val = readw(ZEVIO_GPIO(controller, pin, DIRECTION));
+ val |= 1<<ZEVIO_GPIO_BIT(pin);
Same idea as above.
+ writew(val, ZEVIO_GPIO(controller, pin, DIRECTION));
+
+ spin_unlock(&controller->lock);
+
+ return 0;
+}
+
+static int zevio_gpio_direction_output(struct gpio_chip *chip,
+ unsigned pin, int value)
+{
+ struct zevio_gpio *controller = to_zevio_gpio(chip);
+ uint16_t val;
+
+ spin_lock(&controller->lock);
+ val = readw(ZEVIO_GPIO(controller, pin, OUTPUT));
+ if (value)
+ val |= 1<<ZEVIO_GPIO_BIT(pin);
+ else
+ val &= ~(1<<ZEVIO_GPIO_BIT(pin));
And here too.
+
+ writew(val, ZEVIO_GPIO(controller, pin, OUTPUT));
+ val = readw(ZEVIO_GPIO(controller, pin, DIRECTION));
+ val &= ~(1<<ZEVIO_GPIO_BIT(pin));
+ writew(val, ZEVIO_GPIO(controller, pin, DIRECTION));
And here.
+
+ spin_unlock(&controller->lock);
+
+ return 0;
+}
+
+static int zevio_gpio_to_irq(struct gpio_chip *chip, unsigned pin)
+{
+ /* Not implemented due to weird lockups */
+ return -ENXIO;
Hm. I guess this should be marked TODO: or something.
So when you figure this out you also add an irqchip.
The way this looks I was thinking it could use the
drivers/gpio/gpio-generic.c driver, but maybe not?