[OpenWrt-Devel] [PATCH v2 1/4] brcm63xx: add bcm6345-gpio driver
Jonas Gorski
jogo at openwrt.org
Tue Dec 16 08:45:02 EST 2014
On Mon, Dec 15, 2014 at 3:46 PM, Álvaro Fernández Rojas
<noltari at gmail.com> wrote:
Please add a description for your bindings in
Documentation/devicetree/bindings/gpio
> Signed-off-by: Álvaro Fernández Rojas <noltari at gmail.com>
> ---
> v2: use vendor for DT properties.
>
> diff --git a/target/linux/brcm63xx/config-3.14 b/target/linux/brcm63xx/config-3.14
> index dd27f47..f94c567 100644
> --- a/target/linux/brcm63xx/config-3.14
> +++ b/target/linux/brcm63xx/config-3.14
> @@ -76,6 +76,7 @@ CONFIG_GENERIC_IRQ_SHOW=y
> CONFIG_GENERIC_PCI_IOMAP=y
> CONFIG_GENERIC_SMP_IDLE_THREAD=y
> CONFIG_GPIOLIB=y
> +CONFIG_GPIO_BCM6345=y
> CONFIG_GPIO_DEVRES=y
> CONFIG_GPIO_SYSFS=y
> # CONFIG_HAMRADIO is not set
> diff --git a/target/linux/brcm63xx/config-3.18 b/target/linux/brcm63xx/config-3.18
> index e3cf020..7957d02 100644
> --- a/target/linux/brcm63xx/config-3.18
> +++ b/target/linux/brcm63xx/config-3.18
> @@ -80,6 +80,7 @@ CONFIG_GENERIC_IRQ_SHOW=y
> CONFIG_GENERIC_PCI_IOMAP=y
> CONFIG_GENERIC_SMP_IDLE_THREAD=y
> CONFIG_GPIOLIB=y
> +CONFIG_GPIO_BCM6345=y
> CONFIG_GPIO_DEVRES=y
> CONFIG_GPIO_SYSFS=y
> # CONFIG_HAMRADIO is not set
> diff --git a/target/linux/brcm63xx/patches-3.14/374-GPIO-add-bcm6345-driver.patch b/target/linux/brcm63xx/patches-3.14/374-GPIO-add-bcm6345-driver.patch
> new file mode 100644
> index 0000000..847933b
> --- /dev/null
> +++ b/target/linux/brcm63xx/patches-3.14/374-GPIO-add-bcm6345-driver.patch
> @@ -0,0 +1,244 @@
> +--- /dev/null
> ++++ b/drivers/gpio/gpio-bcm6345.c
> +@@ -0,0 +1,216 @@
> ++/*
> ++ * This file is subject to the terms and conditions of the GNU General Public
> ++ * License. See the file "COPYING" in the main directory of this archive
> ++ * for more details.
> ++ *
> ++ * Copyright (C) 2008 Maxime Bizon <mbizon at freebox.fr>
> ++ * Copyright (C) 2008-2011 Florian Fainelli <florian at openwrt.org>
> ++ * Copyright (C) 2014 Álvaro Fernández Rojas <noltari at gmail.com>
> ++ */
> ++
> ++#include <linux/kernel.h>
> ++#include <linux/module.h>
> ++#include <linux/spinlock.h>
> ++#include <linux/platform_device.h>
> ++#include <linux/gpio.h>
> ++
> ++enum bcm6345_gpio_reg {
> ++ GPIO_REG_CTL_HI = 0,
> ++ GPIO_REG_CTL_LO,
> ++ GPIO_REG_DATA_HI,
> ++ GPIO_REG_DATA_LO,
> ++ GPIO_REG_MAX
> ++};
> ++
> ++struct bcm6345_gpio_chip {
> ++ struct gpio_chip chip;
> ++ u8 regs[GPIO_REG_MAX];
I don't think we need to be that flexible, as there are essentially
only two (three) layouts: bcm6345 and everything else, so we can cover
these through appropriate compatible strings (the arm chips and the
newest mips one have five registers instead of two for a total of 160
gpios).
Also the reg property should only cover what you need, not the whole
gpio range. So only e.g. <0xfffe0404 0x8> in case of bcm6345, and
<0xfffe0400 0x10> / <0xfffe0080 0x10> / <0x10000080 0x10> for anything
else.
Or add both as separate resources and use reg-names to distinguish them.
You could also use the length of the registers to infer the offsets of
the hi/lo registers, and don't need to tell bcm6345 and later apart.
> ++
> ++ spinlock_t lock;
> ++ void __iomem *membase;
> ++};
> ++
> ++static inline struct bcm6345_gpio_chip *to_bcm6345_gpio(struct gpio_chip *chip)
> ++{
> ++ struct bcm6345_gpio_chip *bg;
> ++
> ++ bg = container_of(chip, struct bcm6345_gpio_chip, chip);
> ++
> ++ return bg;
> ++}
> ++
> ++static inline u32 bc_gpio_r32(struct bcm6345_gpio_chip *bg, u8 reg)
Please call this bcm6345_gpio_read32.
> ++{
> ++ return ioread32be(bg->membase + bg->regs[reg]);
> ++}
> ++
> ++static inline void bc_gpio_w32(struct bcm6345_gpio_chip *bg, u8 reg, u32 val)
And this one bcm6345_gpio_write32
> ++{
> ++ iowrite32be(val, bg->membase + bg->regs[reg]);
> ++}
> ++
> ++static void bcm6345_gpio_set(struct gpio_chip *chip, unsigned gpio, int value)
> ++{
> ++ struct bcm6345_gpio_chip *bg = to_bcm6345_gpio(chip);
> ++ unsigned long flags;
> ++ u32 mask, val;
> ++ u8 reg;
> ++
> ++ if (gpio < 32) {
> ++ reg = GPIO_REG_DATA_LO;
> ++ mask = BIT(gpio);
> ++ } else {
> ++ reg = GPIO_REG_DATA_HI;
> ++ mask = BIT(gpio - 32);
> ++ }
I would suggest doing something similar like I did for the "reverse"
order of the interrupt registers.
Let the bcm6456_gpio_chip have
u8 data_reg[2];
u8 dir_reg[2];
which you then populate on probe with { 0x0 }, { 0x4 } for bcm6345,
and { 0x4, 0x0 } and { 0xc, 0x8 }
Then you can do just
reg = bg->data_reg[gpio / 32];
mask = BIT(gpio % 32);
...
val = bcm6345_gpio_read32(bg, reg);
...
This will also make it easy to expand it for support of more than 64 gpios.
> ++
> ++ spin_lock_irqsave(&bg->lock, flags);
> ++ val = bc_gpio_r32(bg, reg);
> ++ if (value)
> ++ val |= mask;
> ++ else
> ++ val &= ~mask;
> ++ bc_gpio_w32(bg, reg, val);
> ++ spin_unlock_irqrestore(&bg->lock, flags);
> ++}
> ++
> ++static int bcm6345_gpio_get(struct gpio_chip *chip, unsigned gpio)
> ++{
> ++ struct bcm6345_gpio_chip *bg = to_bcm6345_gpio(chip);
> ++ u32 mask;
> ++ u8 reg;
> ++
> ++ if (gpio < 32) {
> ++ reg = GPIO_REG_DATA_LO;
> ++ mask = BIT(gpio);
> ++ } else {
> ++ reg = GPIO_REG_DATA_HI;
> ++ mask = BIT(gpio - 32);
> ++ }
> ++
> ++ return !!(bc_gpio_r32(bg, reg) & mask);
> ++}
> ++
> ++static int bcm6345_gpio_direction_input(struct gpio_chip *chip, unsigned gpio)
> ++{
> ++ struct bcm6345_gpio_chip *bg = to_bcm6345_gpio(chip);
> ++ unsigned long flags;
> ++ u32 mask, val;
> ++ u8 reg;
> ++
> ++ if (gpio < 32) {
> ++ reg = GPIO_REG_CTL_LO;
> ++ mask = BIT(gpio);
> ++ } else {
> ++ reg = GPIO_REG_CTL_HI;
> ++ mask = BIT(gpio - 32);
> ++ }
> ++
> ++ spin_lock_irqsave(&bg->lock, flags);
> ++ val = bc_gpio_r32(bg, reg) & ~mask;
> ++ bc_gpio_w32(bg, reg, val);
> ++ spin_unlock_irqrestore(&bg->lock, flags);
> ++
> ++ return 0;
> ++}
> ++
> ++static int bcm6345_gpio_direction_output(struct gpio_chip *chip, unsigned gpio)
> ++{
> ++ struct bcm6345_gpio_chip *bg = to_bcm6345_gpio(chip);
> ++ unsigned long flags;
> ++ u32 mask, val;
> ++ u8 reg;
> ++
> ++ if (gpio < 32) {
> ++ reg = GPIO_REG_CTL_LO;
> ++ mask = BIT(gpio);
> ++ } else {
> ++ reg = GPIO_REG_CTL_HI;
> ++ mask = BIT(gpio - 32);
> ++ }
> ++
> ++ spin_lock_irqsave(&bg->lock, flags);
> ++ val = bc_gpio_r32(bg, reg) | mask;
> ++ bc_gpio_w32(bg, reg, val);
> ++ spin_unlock_irqrestore(&bg->lock, flags);
> ++
> ++ return 0;
> ++}
> ++
> ++int __init bcm6345_gpio_probe(struct platform_device *pdev)
> ++{
> ++ struct device_node *np = pdev->dev.of_node;
> ++ struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> ++ struct bcm6345_gpio_chip *bg;
> ++ const __be32 *ngpio, *gpiobase;
> ++
> ++ if (!res) {
> ++ dev_err(&pdev->dev, "failed to find resource\n");
> ++ return -ENOMEM;
> ++ }
> ++
> ++ bg = devm_kzalloc(&pdev->dev,
> ++ sizeof(struct gpio_chip), GFP_KERNEL);
> ++ if (!bg)
> ++ return -ENOMEM;
> ++
> ++ bg->membase = devm_ioremap_resource(&pdev->dev, res);
> ++ if (IS_ERR(bg->membase)) {
> ++ dev_err(&pdev->dev, "cannot remap I/O memory region\n");
No need to add error messages here, ioremap will already complain for you.
> ++ return -ENOMEM;
> ++ }
> ++
> ++ if (of_property_read_u8_array(np, "brcm,register-map",
> ++ bg->regs, GPIO_REG_MAX)) {
> ++ dev_err(&pdev->dev, "failed to read register definition\n");
As I explain above, no need for this one.
> ++ return -EINVAL;
> ++ }
> ++
> ++ ngpio = of_get_property(np, "brcm,num-gpios", NULL);
> ++ if (!ngpio) {
> ++ dev_err(&pdev->dev, "failed to read number of pins\n");
> ++ return -EINVAL;
> ++ }
> ++
> ++ gpiobase = of_get_property(np, "brcm,gpio-base", NULL);
NAK on this part. This should be at best set in the driver itself, but
not be in the devicetree itself.
> ++ if (gpiobase)
> ++ bg->chip.base = be32_to_cpu(*gpiobase);
> ++ else
> ++ bg->chip.base = -1;
> ++
> ++ spin_lock_init(&bg->lock);
> ++
> ++ bg->chip.dev = &pdev->dev;
> ++ bg->chip.label = dev_name(&pdev->dev);
> ++ bg->chip.of_node = np;
> ++ bg->chip.ngpio = be32_to_cpu(*ngpio);
No need to manually convert it, just use of_property_read_u32().
> ++ bg->chip.direction_input = bcm6345_gpio_direction_input;
> ++ bg->chip.direction_output = bcm6345_gpio_direction_output;
> ++ bg->chip.get = bcm6345_gpio_get;
> ++ bg->chip.set = bcm6345_gpio_set;
> ++
> ++ dev_info(&pdev->dev, "registering %d gpios\n", bg->chip.ngpio);
> ++
> ++ return gpiochip_add(&bg->chip);
> ++}
> ++
> ++static struct of_device_id bcm6345_gpio_match[] = {
> ++ { .compatible = "brcm,bcm6345-gpio" },
> ++ { },
> ++};
> ++MODULE_DEVICE_TABLE(of, bcm6345_gpio_match);
> ++
> ++static struct platform_driver bcm6345_gpio_driver = {
> ++ .driver = {
> ++ .name = "bcm6345-gpio",
> ++ .owner = THIS_MODULE,
> ++ .of_match_table = bcm6345_gpio_match,
> ++ },
> ++ .probe = bcm6345_gpio_probe,
> ++};
> ++
> ++int __init bcm6345_gpio_init(void)
> ++{
> ++ return platform_driver_register(&bcm6345_gpio_driver);
> ++}
> ++subsys_initcall(bcm6345_gpio_init);
> +--- a/drivers/gpio/Kconfig
> ++++ b/drivers/gpio/Kconfig
> +@@ -108,6 +108,12 @@ config GPIO_MAX730X
> +
> + comment "Memory mapped GPIO drivers:"
> +
> ++config GPIO_BCM6345
> ++ bool "Broadcom 6345 GPIO Support"
> ++ depends on BCM63XX
> ++ help
> ++ Say yes here to support the Broadcom 6345 SoC GPIO device
> ++
> + config GPIO_CLPS711X
> + tristate "CLPS711X GPIO support"
> + depends on ARCH_CLPS711X || COMPILE_TEST
> +--- a/drivers/gpio/Makefile
> ++++ b/drivers/gpio/Makefile
> +@@ -17,6 +17,7 @@ obj-$(CONFIG_GPIO_ADP5588) += gpio-adp55
> + obj-$(CONFIG_GPIO_AMD8111) += gpio-amd8111.o
> + obj-$(CONFIG_GPIO_ARIZONA) += gpio-arizona.o
> + obj-$(CONFIG_GPIO_BCM_KONA) += gpio-bcm-kona.o
> ++obj-$(CONFIG_GPIO_BCM6345) += gpio-bcm6345.o
> + obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o
> + obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o
> + obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o
Jonas
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel
More information about the openwrt-devel
mailing list