lkml.org 
[lkml]   [2013]   [Aug]   [16]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] gpio: New driver for LSI ZEVIO SoCs
From
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


\
 
 \ /
  Last update: 2013-08-16 17:01    [W:0.060 / U:2.320 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site