On Fri, Jul 3, 2015 at 11:11 AM, Alban Bedel <albeu@xxxxxxx> wrote:
> +++ b/drivers/gpio/gpio-ath79.c
> @@ -0,0 +1,236 @@
> +/*
> + * Atheros AR71XX/AR724X/AR913X GPIO API support
> + *
> + * Copyright (C) 2010-2011 Jaiganesh Narayanan <jnarayanan@xxxxxxxxxxx>
> + * Copyright (C) 2008-2011 Gabor Juhos <juhosg@xxxxxxxxxxx>
> + * Copyright (C) 2008 Imre Kaloz <kaloz@xxxxxxxxxxx>
> + *
> + * Parts of this file are based on Atheros' 2.6.15/2.6.31 BSP
> + *
> + * 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/kernel.h>
> +#include <linux/init.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/spinlock.h>
> +#include <linux/io.h>
> +#include <linux/ioport.h>
> +#include <linux/gpio.h>
Nominally only <linux/gpio/driver.h> should be enough for a driver.
> +#include <linux/platform_data/gpio-ath79.h>
> +#include <linux/of_device.h>
> +
> +#include <asm/mach-ath79/ar71xx_regs.h>
> +
> +static void __iomem *ath79_gpio_base;
> +static u32 ath79_gpio_count;
> +static DEFINE_SPINLOCK(ath79_gpio_lock);
Would prefer to use the state container design pattern from
Documentation/driver-model/design-patterns.txt
> +static void __ath79_gpio_set_value(unsigned gpio, int value)
> +{
> + void __iomem *base = ath79_gpio_base;
> +
> + if (value)
> + __raw_writel(1 << gpio, base + AR71XX_GPIO_REG_SET);
> + else
> + __raw_writel(1 << gpio, base + AR71XX_GPIO_REG_CLEAR);
I have a vague memory of some semantics being strange in the MIPS
camp, but doesn't writel_relaxed() do what you want? (Benji brought
something related up for discussion on the ksummit list...)
> +static int __ath79_gpio_get_value(unsigned gpio)
> +{
> + return (__raw_readl(ath79_gpio_base + AR71XX_GPIO_REG_IN) >> gpio) & 1;
> +}
> +
> +static int ath79_gpio_get_value(struct gpio_chip *chip, unsigned offset)
> +{
> + return __ath79_gpio_get_value(offset);
> +}
Strange double functions that can be refactored out I think.
All internal uses should be able to reference the gpio_chip.
> +static void ath79_gpio_set_value(struct gpio_chip *chip,
> + unsigned offset, int value)
> +{
> + __ath79_gpio_set_value(offset, value);
> +}
Same.
> + __raw_writel(__raw_readl(base + AR71XX_GPIO_REG_OE) | (1 << offset),
> + base + AR71XX_GPIO_REG_OE);
I tend to #include <linux/bitops.h>
And just | BIT(offset) there in the end.
Some prefer it like this, I just think the explit bitops look neat.
> +int gpio_get_value(unsigned gpio)
> +EXPORT_SYMBOL(gpio_get_value);
> +void gpio_set_value(unsigned gpio, int value)
> +EXPORT_SYMBOL(gpio_set_value);
> +int gpio_to_irq(unsigned gpio)
> +EXPORT_SYMBOL(gpio_to_irq);
These are some quite horrific overrides of the gpiolib functions.
I would like it rooted out and replaced with the gpiolib implementations.
I think we managed to exorcise this "generic GPIO" from ARM but
I'm honestly not certain.
> +int irq_to_gpio(unsigned irq)
> +{
> + /* FIXME */
> + return -EINVAL;
> +}
> +EXPORT_SYMBOL(irq_to_gpio);
Can't you just delete this? It has been removed from generic GPIO and
gpiolib because of ambiguity.
Yours,
Linus Walleij
