[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