* [PATCH] gpio: New driver for LSI ZEVIO SoCs
@ 2013-08-05 21:59 Fabian Vogt
2013-08-16 14:46 ` Linus Walleij
0 siblings, 1 reply; 5+ messages in thread
From: Fabian Vogt @ 2013-08-05 21:59 UTC (permalink / raw)
To: linux-gpio; +Cc: linux-kernel, linux-doc, devicetree, linus.walleij
From: Fabian Vogt <fabian@ritter-vogt.de>
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@ritter-vogt.de>
---
Notes:
Applies to v3.11-rc3-378-g1aa3b5c
.../devicetree/bindings/gpio/gpio-zevio.txt | 19 ++
drivers/gpio/Kconfig | 7 +
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-zevio.c | 212
+++++++++++++++++++++
4 files changed, 239 insertions(+)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio-zevio.txt
create mode 100644 drivers/gpio/gpio-zevio.c
diff --git a/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
new file mode 100644
index 0000000..96d948b
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-zevio.txt
@@ -0,0 +1,19 @@
+Zevio GPIO controller
+
+Required properties:
+- compatible = "lsi,zevio-gpio"
+- reg = <BASEADDR SIZE>
+- #gpio-cells = <2>
+- gpio-controller;
+
+Optional:
+- #ngpios = <32>: Number of GPIOs. Defaults to 32 if absent
+
+Example:
+ gpio: gpio@90000000 {
+ compatible = "lsi,zevio-gpio";
+ reg = <0x90000000 0x1000>;
+
+ gpio-controller;
+ #gpio-cells = <2>;
+ };
\ No newline at end of file
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b2450ba..ba8c357 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -138,6 +138,13 @@ config GPIO_EP93XX
depends on ARCH_EP93XX
select GPIO_GENERIC
+config GPIO_ZEVIO
+ bool "LSI ZEVIO SoC memory mapped GPIOs"
+ depends on ARCH_NSPIRE
+ select GENERIC_IRQ_CHIP
+ help
+ Say yes here to support the GPIO controller in LSI ZEVIO SoCs.
+
config GPIO_MM_LANTIQ
bool "Lantiq Memory mapped GPIOs"
depends on LANTIQ && SOC_XWAY
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index ef3e983..1e7e926 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -87,3 +87,4 @@ obj-$(CONFIG_GPIO_WM831X) += gpio-wm831x.o
obj-$(CONFIG_GPIO_WM8350) += gpio-wm8350.o
obj-$(CONFIG_GPIO_WM8994) += gpio-wm8994.o
obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o
+obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
\ No newline at end of file
diff --git a/drivers/gpio/gpio-zevio.c b/drivers/gpio/gpio-zevio.c
new file mode 100644
index 0000000..f5c04b3
--- /dev/null
+++ b/drivers/gpio/gpio-zevio.c
@@ -0,0 +1,211 @@
+/*
+ * GPIO controller in LSI ZEVIO SoCs.
+ *
+ * Author: Fabian Vogt <fabian@ritter-vogt.de>
+ *
+ * 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/spinlock.h>
+#include <linux/errno.h>
+#include <linux/module.h>
+#include <linux/io.h>
+#include <linux/of_device.h>
+#include <linux/of_gpio.h>
+#include <linux/slab.h>
+#include <linux/gpio.h>
+
+/*
+ * 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
+ *
+ * 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
+ * 0x40-0x7F: Section 1
+ * 0x80-0xBF: Section 2
+ * 0xC0-0xFF: Section 3
+ */
+
+#define ZEVIO_GPIO_SECTION_SIZE 0x40
+
+#define ZEVIO_GPIO_INT_MASKED_STATUS_OFFSET 0x00
+#define ZEVIO_GPIO_INT_STATUS_OFFSET 0x04
+#define ZEVIO_GPIO_INT_UNMASK_OFFSET 0x08
+#define ZEVIO_GPIO_INT_MASK_OFFSET 0x0C
+#define ZEVIO_GPIO_DIRECTION_OFFSET 0x10
+#define ZEVIO_GPIO_OUTPUT_OFFSET 0x14
+#define ZEVIO_GPIO_INPUT_OFFSET 0x18
+#define ZEVIO_GPIO_INT_STICKY_OFFSET 0x20
+
+#define to_zevio_gpio(chip) container_of(to_of_mm_gpio_chip(chip), \
+ struct zevio_gpio, chip)
+
+/* Bit of GPIO in section */
+#define ZEVIO_GPIO_BIT(gpio) (gpio&7)
+/* Offset to section of GPIO relative to base */
+#define ZEVIO_GPIO_SECTION_OFFSET(gpio)
(((gpio>>3)&3)*ZEVIO_GPIO_SECTION_SIZE)
+/* Address of register, which is responsible for given GPIO */
+#define ZEVIO_GPIO(cntrlr, gpio, reg) IOMEM(cntrlr->chip.regs + \
+ ZEVIO_GPIO_SECTION_OFFSET(gpio) + ZEVIO_GPIO_##reg##_OFFSET)
+
+struct zevio_gpio {
+ spinlock_t lock;
+ struct of_mm_gpio_chip chip;
+};
+
+/* 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));
+
+ return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
+}
+
+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);
+ else
+ val &= ~(1<<ZEVIO_GPIO_BIT(pin));
+
+ 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);
+ 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));
+
+ 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));
+
+ 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;
+}
+
+static struct gpio_chip zevio_gpio_chip = {
+ .direction_input = zevio_gpio_direction_input,
+ .direction_output = zevio_gpio_direction_output,
+ .set = zevio_gpio_set,
+ .get = zevio_gpio_get,
+ .to_irq = zevio_gpio_to_irq,
+ .base = 0,
+ .owner = THIS_MODULE,
+ .ngpio = 32, /* Default, if not given in DT */
+ .of_gpio_n_cells = 2,
+};
+
+/* Initialization */
+static int zevio_gpio_probe(struct platform_device *pdev)
+{
+ struct zevio_gpio *controller;
+ int status, i;
+ u32 ngpio;
+
+ controller = devm_kzalloc(&pdev->dev, sizeof(*controller), GFP_KERNEL);
+ if (!controller) {
+ dev_err(&pdev->dev, "not enough free memory\n");
+ return -ENOMEM;
+ }
+
+ /* Copy our reference */
+ controller->chip.gc = zevio_gpio_chip;
+ controller->chip.gc.dev = &pdev->dev;
+
+ if (!of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio))
+ controller->chip.gc.ngpio = ngpio;
+
+ status = of_mm_gpiochip_add(pdev->dev.of_node, &(controller->chip));
+ if (status) {
+ kfree(controller);
+ dev_err(&pdev->dev, "failed to add gpiochip: %d\n", status);
+ return status;
+ }
+
+ spin_lock_init(&(controller->lock));
+
+ /* Disable interrupts, they only cause errors */
+ for (i = 0; i < controller->chip.gc.ngpio; i += 8)
+ writew(0xFF, ZEVIO_GPIO(controller, i, INT_MASK));
+
+ dev_dbg(controller->chip.gc.dev, "ZEVIO GPIO controller set up!\n");
+
+ return 0;
+}
+
+static struct of_device_id zevio_gpio_of_match[] = {
+ { .compatible = "lsi,zevio-gpio", },
+ { },
+};
+
+MODULE_DEVICE_TABLE(of, zevio_gpio_of_match);
+
+static struct platform_driver zevio_gpio_driver = {
+ .driver = {
+ .name = "gpio-zevio",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(zevio_gpio_of_match),
+ },
+ .probe = zevio_gpio_probe,
+};
+
+module_platform_driver(zevio_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Fabian Vogt <fabian@ritter-vogt.de>");
+MODULE_DESCRIPTION("LSI ZEVIO SoC GPIO driver");
--
1.8.1.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs
2013-08-05 21:59 [PATCH] gpio: New driver for LSI ZEVIO SoCs Fabian Vogt
@ 2013-08-16 14:46 ` Linus Walleij
2013-08-16 16:34 ` Fabian Vogt
0 siblings, 1 reply; 5+ messages in thread
From: Linus Walleij @ 2013-08-16 14:46 UTC (permalink / raw)
To: Fabian Vogt; +Cc: linux-gpio, linux-kernel, linux-doc, devicetree
On Mon, Aug 5, 2013 at 11:59 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:
> From: Fabian Vogt <fabian@ritter-vogt.de>
>
> 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@ritter-vogt.de>
Hi Fabian, sorry for taking so long for getting around to review this.
> +++ 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.)
> +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.
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!
> + * 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?
> +/* 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.
> +
> + return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
Use this construct:
return !!(val & ZEVIO_GPIO_BIT(pin));
> +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));
> + 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?
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs
2013-08-16 14:46 ` Linus Walleij
@ 2013-08-16 16:34 ` Fabian Vogt
2013-08-23 14:09 ` Linus Walleij
0 siblings, 1 reply; 5+ messages in thread
From: Fabian Vogt @ 2013-08-16 16:34 UTC (permalink / raw)
To: Linus Walleij; +Cc: linux-gpio, linux-kernel, linux-doc, devicetree
On Fri, Aug 16, 2013 at 4:46 PM, Linus Walleij <linus.walleij@linaro.org>
wrote:
> On Mon, Aug 5, 2013 at 11:59 PM, Fabian Vogt <fabian@ritter-vogt.de>
> wrote:
>
>> From: Fabian Vogt <fabian@ritter-vogt.de>
>>
>> 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@ritter-vogt.de>
>
> Hi Fabian, sorry for taking so long for getting around to review this.
No problem :)
>> +++ 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.)
Any ideas which mail addresses I should add to CC:?
>> +config GPIO_ZEVIO
>> + bool "LSI ZEVIO SoC memory mapped GPIOs"
>> + depends on ARCH_NSPIRE
>
> Can't this appear in some other SoC?
The problem is, no documents about this SoC are available at all.
Everything we know about this chip has been found out by disassembling the
nspire software (nucleus PLUS), so I have no idea, but probably not.
Also, it can't be tested during bootup, as the only platform we can test
it on
boots from an internal read-only flash, which loads boot2,
which itself is exploitable to start linux.
The entire hardware has already been initialized before booting linux.
> It doesn't seem very arch-dependent in itself.
>
> 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!
Not my work, but indeed, it's very impressive!
(Except for the newer ("complicated") touchpad interface which was indeed
an I2C controller)
>> + * 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 how the GPIO controller signals the VIC.
On sticky it keeps the IRQ high until it has been handled (W to 0x4),
if not sticky, it sets the IRQ line at the same state as the GPIO pin is.
If GPIO high => IRQ high, if GPIO gets low again => IRQ low, which is not
VERY useful..
>> +/* 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.
It's used VERY often and I couldn't find any coding style document which
prefers u16..
>> +
>> + return (val >> ZEVIO_GPIO_BIT(pin)) & 0x1;
>
> Use this construct:
> return !!(val & ZEVIO_GPIO_BIT(pin));
Ok.
>> +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));
Oh, it seems I have overseen that..
>> + 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?
Would be great, but it doesn't support multiple registers (4*8 GPIOs),
so I would have to register 4 of them. Then, they had to share one
interrupt,
which would have to be implemented seperately (or not at all) and then it
has
to be working with device tree without any extra (struct platform_)data.
I prefer two bitops (pin&7 and pin>>3), which are present anyways (for
IRQs)
over an array of four gpio_chips.
I'll resubmit the improved version as V4 after you told me which devicetree
mail addresses I should add.
Bye,
Fabian
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs
2013-08-16 16:34 ` Fabian Vogt
@ 2013-08-23 14:09 ` Linus Walleij
2013-08-23 19:40 ` Stephen Warren
0 siblings, 1 reply; 5+ messages in thread
From: Linus Walleij @ 2013-08-23 14:09 UTC (permalink / raw)
To: Fabian Vogt, Stephen Warren
Cc: linux-gpio, linux-kernel, linux-doc, devicetree
On Fri, Aug 16, 2013 at 6:34 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:
> On Fri, Aug 16, 2013 at 4:46 PM, Linus Walleij <linus.walleij@linaro.org>
> wrote:
>> I want an ACK from one of the DT bindings maintainers for this
>> portion of the driver ideally. (It looks all right to me.)
>
> Any ideas which mail addresses I should add to CC:?
No. devicetree@vger.kernel.org is the place.
But if nothing happens you might want to ask Stephen Warren.
>>> +config GPIO_ZEVIO
>>> + bool "LSI ZEVIO SoC memory mapped GPIOs"
>>> + depends on ARCH_NSPIRE
>>
>>
>> Can't this appear in some other SoC?
>
> The problem is, no documents about this SoC are available at all.
> Everything we know about this chip has been found out by disassembling the
> nspire software (nucleus PLUS), so I have no idea, but probably not.
> Also, it can't be tested during bootup, as the only platform we can test it
> on
> boots from an internal read-only flash, which loads boot2,
> which itself is exploitable to start linux.
> The entire hardware has already been initialized before booting linux.
The thing is that if we don't put in the dependency, we can get
some nice compile coverage on a few different compilers and
platforms.
>>> + * 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 how the GPIO controller signals the VIC.
> On sticky it keeps the IRQ high until it has been handled (W to 0x4),
This is what is called a level interrupt.
So please update the terminology to match the common
one.
> if not sticky, it sets the IRQ line at the same state as the GPIO pin is.
> If GPIO high => IRQ high, if GPIO gets low again => IRQ low, which is not
> VERY useful..
Hm doesn't seem like an interrupt at all, rather some
test operation to test the IRQ line. Well whatever...
>> Use just u16 please. uint16_t is some portable C type.
>>
>> Please replace uint16_t with u16 everywhere.
>
> It's used VERY often and I couldn't find any coding style document which
> prefers u16..
There is no consensus around this.
It is up to the subsystem maintainer to decide this I think.
>>> +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?
>
> Would be great, but it doesn't support multiple registers (4*8 GPIOs),
> so I would have to register 4 of them. Then, they had to share one
> interrupt,
> which would have to be implemented seperately (or not at all) and then it
> has
> to be working with device tree without any extra (struct platform_)data.
> I prefer two bitops (pin&7 and pin>>3), which are present anyways (for IRQs)
> over an array of four gpio_chips.
OK I buy this, but you know you can reuse only part of the
generic MMIO driver, and do not have to use all of it?
> I'll resubmit the improved version as V4 after you told me which devicetree
> mail addresses I should add.
OK thanks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] gpio: New driver for LSI ZEVIO SoCs
2013-08-23 14:09 ` Linus Walleij
@ 2013-08-23 19:40 ` Stephen Warren
0 siblings, 0 replies; 5+ messages in thread
From: Stephen Warren @ 2013-08-23 19:40 UTC (permalink / raw)
To: Linus Walleij
Cc: Fabian Vogt, linux-gpio, linux-kernel, linux-doc, devicetree
On 08/23/2013 08:09 AM, Linus Walleij wrote:
> On Fri, Aug 16, 2013 at 6:34 PM, Fabian Vogt <fabian@ritter-vogt.de> wrote:
>> On Fri, Aug 16, 2013 at 4:46 PM, Linus Walleij <linus.walleij@linaro.org>
>> wrote:
>
>>> I want an ACK from one of the DT bindings maintainers for this
>>> portion of the driver ideally. (It looks all right to me.)
>>
>> Any ideas which mail addresses I should add to CC:?
>
> No. devicetree@vger.kernel.org is the place.
>
> But if nothing happens you might want to ask Stephen Warren.
The full list of CC's is:
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
M: Rob Herring <rob.herring@calxeda.com>
M: Pawel Moll <pawel.moll@arm.com>
M: Mark Rutland <mark.rutland@arm.com>
M: Stephen Warren <swarren@wwwdotorg.org>
M: Ian Campbell <ian.campbell@citrix.com>
L: devicetree@vger.kernel.org
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2013-08-23 19:40 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-05 21:59 [PATCH] gpio: New driver for LSI ZEVIO SoCs Fabian Vogt
2013-08-16 14:46 ` Linus Walleij
2013-08-16 16:34 ` Fabian Vogt
2013-08-23 14:09 ` Linus Walleij
2013-08-23 19:40 ` Stephen Warren
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.