On 20/10/2014 06:57, Alexandre Courbot wrote:
> On Sat, Oct 11, 2014 at 5:28 AM, John Crispin <blogic@xxxxxxxxxxx>
> wrote:
>> Add gpio driver for Ralink SoC. This driver makes the gpio core
>> on MT7621 and MT7628 work.
>
> Basically I have the same remarks as for the rt2880 driver. Both
> seem to be very similar, and to work kind of like the gpio-generic
> driver. I wonder if there wouldn't be a way to leverage this
> driver instead of rewriting the same logic X times?
>
> Some more comments below...
>
>>
>> Signed-off-by: John Crispin <blogic@xxxxxxxxxxx> Cc:
>> linux-mips@xxxxxxxxxxxxxx Cc: linux-gpio@xxxxxxxxxxxxxxx ---
>> drivers/gpio/Kconfig | 6 ++ drivers/gpio/Makefile
>> | 1 + drivers/gpio/gpio-mt7621.c | 195
>> ++++++++++++++++++++++++++++++++++++++++++++ 3 files changed,
>> 202 insertions(+) create mode 100644 drivers/gpio/gpio-mt7621.c
>>
>> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
>> c91b15b..1ef83a0 100644 --- a/drivers/gpio/Kconfig +++
>> b/drivers/gpio/Kconfig @@ -523,6 +523,12 @@ config
>> GPIO_MC9S08DZ60 help Select this to enable the MC9S08DZ60 GPIO
>> driver
>>
>> +config GPIO_MT7621 + bool "Mediatek GPIO Support" +
>> depends on MIPS && (SOC_MT7620 || SOC_MT7621) + help + Say
>> yes here to support the Mediatek SoC GPIO device + config
>> GPIO_PCA953X tristate "PCA95[357]x, PCA9698, TCA64xx, and
>> MAX7310 I/O ports" depends on I2C diff --git
>> a/drivers/gpio/Makefile b/drivers/gpio/Makefile index
>> d8f0f17..60a9e0e 100644 --- a/drivers/gpio/Makefile +++
>> b/drivers/gpio/Makefile @@ -57,6 +57,7 @@
>> obj-$(CONFIG_GPIO_MPC8XXX) += gpio-mpc8xxx.o
>> obj-$(CONFIG_GPIO_MSIC) += gpio-msic.o
>> obj-$(CONFIG_GPIO_MSM_V1) += gpio-msm-v1.o
>> obj-$(CONFIG_GPIO_MSM_V2) += gpio-msm-v2.o
>> +obj-$(CONFIG_GPIO_MT7621) += gpio-mt7621.o
>> obj-$(CONFIG_GPIO_MVEBU) += gpio-mvebu.o
>> obj-$(CONFIG_GPIO_MXC) += gpio-mxc.o
>> obj-$(CONFIG_GPIO_MXS) += gpio-mxs.o diff --git
>> a/drivers/gpio/gpio-mt7621.c b/drivers/gpio/gpio-mt7621.c new
>> file mode 100644 index 0000000..7faf2c0 --- /dev/null +++
>> b/drivers/gpio/gpio-mt7621.c @@ -0,0 +1,195 @@ +/* + * 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. + * + *
>> Copyright (C) 2009-2011 Gabor Juhos <juhosg@xxxxxxxxxxx> + *
>> Copyright (C) 2013 John Crispin <blogic@xxxxxxxxxxx> + */ +
>> +#include <linux/io.h> +#include <linux/err.h> +#include
>> <linux/gpio.h> +#include <linux/module.h> +#include
>> <linux/of_irq.h> +#include <linux/spinlock.h> +#include
>> <linux/irqdomain.h> +#include <linux/interrupt.h> +#include
>> <linux/platform_device.h> + +#define MTK_BANK_WIDTH 32 +
>> +enum mediatek_gpio_reg { + GPIO_REG_CTRL = 0, +
>> GPIO_REG_POL, + GPIO_REG_DATA, + GPIO_REG_DSET, +
>> GPIO_REG_DCLR, +}; + +static void __iomem *mtk_gc_membase; +
>> +struct mtk_gc { + struct gpio_chip chip; + spinlock_t
>> lock; + int bank; +}; + +static inline struct mtk_gc
>> +*to_mediatek_gpio(struct gpio_chip *chip) +{ + struct mtk_gc
>> *mgc; + + mgc = container_of(chip, struct mtk_gc, chip); +
>> + return mgc; +} + +static inline void +mtk_gpio_w32(struct
>> mtk_gc *rg, u8 reg, u32 val) +{ + iowrite32(val, mtk_gc_membase +
>> (reg * 0x10) + (rg->bank * 0x4)); +} + +static inline u32
>> +mtk_gpio_r32(struct mtk_gc *rg, u8 reg) +{ + return
>> ioread32(mtk_gc_membase + (reg * 0x10) + (rg->bank * 0x4)); +} +
>> +static void +mediatek_gpio_set(struct gpio_chip *chip, unsigned
>> offset, int value) +{ + struct mtk_gc *rg =
>> to_mediatek_gpio(chip); + + mtk_gpio_w32(rg, (value) ?
>> GPIO_REG_DSET : GPIO_REG_DCLR, BIT(offset)); +} + +static int
>> +mediatek_gpio_get(struct gpio_chip *chip, unsigned offset) +{ +
>> struct mtk_gc *rg = to_mediatek_gpio(chip); + + return
>> !!(mtk_gpio_r32(rg, GPIO_REG_DATA) & BIT(offset)); +} + +static
>> int +mediatek_gpio_direction_input(struct gpio_chip *chip,
>> unsigned offset) +{ + struct mtk_gc *rg =
>> to_mediatek_gpio(chip); + unsigned long flags; + u32 t; +
>> + spin_lock_irqsave(&rg->lock, flags); + t =
>> mtk_gpio_r32(rg, GPIO_REG_CTRL); + t &= ~BIT(offset); +
>> mtk_gpio_w32(rg, GPIO_REG_CTRL, t); +
>> spin_unlock_irqrestore(&rg->lock, flags); + + return 0; +}
>> + +static int +mediatek_gpio_direction_output(struct gpio_chip
>> *chip, + unsigned offset,
>> int value) +{ + struct mtk_gc *rg = to_mediatek_gpio(chip);
>> + unsigned long flags; + u32 t; + +
>> spin_lock_irqsave(&rg->lock, flags); + t = mtk_gpio_r32(rg,
>> GPIO_REG_CTRL); + t |= BIT(offset); + mtk_gpio_w32(rg,
>> GPIO_REG_CTRL, t); + mediatek_gpio_set(chip, offset, value); +
>> spin_unlock_irqrestore(&rg->lock, flags); + + return 0; +}
>> + +static int +mediatek_gpio_request(struct gpio_chip *chip,
>> unsigned offset) +{ + int gpio = chip->base + offset; + +
>> return pinctrl_request_gpio(gpio); +} + +static void
>> +mediatek_gpio_free(struct gpio_chip *chip, unsigned offset) +{
>> + int gpio = chip->base + offset; + + pinctrl_free_gpio(gpio); +}
>> + +static int +mediatek_gpio_bank_probe(struct platform_device
>> *pdev, struct device_node *bank) +{ + const __be32 *id =
>> of_get_property(bank, "reg", NULL); + struct mtk_gc *rg =
>> devm_kzalloc(&pdev->dev, + sizeof(struct mtk_gc), GFP_KERNEL); +
>> if (!rg || !id) + return -ENOMEM; + +
>> spin_lock_init(&rg->lock); + + rg->chip.dev = &pdev->dev; +
>> rg->chip.label = dev_name(&pdev->dev); + rg->chip.of_node =
>> bank; + rg->chip.base = MTK_BANK_WIDTH * be32_to_cpu(*id);
>
> Argh, no, I don't think so. The GPIO integer space is not something
> you can arbitrarily use like this. Any value that is not -1 is
> dangerous here.
>
> If you add your banks one after the other, like you are seemingly
> doing here, then you have a high probability of ending with
> consecutive numbers. This cannot be guaranteed 100% though. That's
> why we are trying to get away from the integer numberspace and to
> use GPIO descriptors instead.
dou you have an example of a driver already doing so ?
>
>> + rg->chip.ngpio = MTK_BANK_WIDTH; +
>> rg->chip.direction_input = mediatek_gpio_direction_input; +
>> rg->chip.direction_output = mediatek_gpio_direction_output; +
>> rg->chip.get = mediatek_gpio_get; + rg->chip.set =
>> mediatek_gpio_set; + rg->chip.request =
>> mediatek_gpio_request; + rg->chip.free =
>> mediatek_gpio_free; + + /* set polarity to low for all
>> gpios */ + mtk_gpio_w32(rg, GPIO_REG_POL, 0); + +
>> dev_info(&pdev->dev, "registering %d gpios\n", rg->chip.ngpio); +
>> + return gpiochip_add(&rg->chip); +} + +static int
>> +mediatek_gpio_probe(struct platform_device *pdev) +{ + struct
>> device_node *bank, *np = pdev->dev.of_node; + struct
>> resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0); +
>> + mtk_gc_membase = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(mtk_gc_membase)) + return
>> PTR_ERR(mtk_gc_membase); + + for_each_child_of_node(np,
>> bank) + if (of_device_is_compatible(bank,
>> "mediatek,mt7621-gpio-bank")) + mediatek_gpio_bank_probe(pdev,
>> bank);
>
> Here you may want to make sure the banks are parsed in the right
> order if the order of their base number matters to you. Another
> solution would be to just have a property with the number of
> banks, and use that instead of sub-nodes for each bank. This should
> be doable here since your banks do not have special properties of
> their own, nor do they differ from each other.
>
>
that would be redundant or not "i will now list <4> banks", "here are
the 4 banks". that is what the count helpers are for if we need to be
aware of the number of properties prior to parsing the node.
i am not sure i have seen another instance of dts using a count index
for the following properties array/table/... do you have an example of
a driver doing so ?
John
