| Message ID | 1430901477-10678-3-git-send-email-gregory.0xf0@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Just a nit: a license mismatch. On Wed, 2015-05-06 at 01:37 -0700, Gregory Fong wrote: > --- /dev/null > +++ b/drivers/gpio/gpio-brcmstb.c > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License as > + * published by the Free Software Foundation version 2. > + * > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > + * kind, whether express or implied; without even the implied warranty > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. This states the license is GPL v2. > +MODULE_LICENSE("GPL"); And, according to include/linux/module.h, this states the license is GPL v2 or later. So I think either the comment at the top of this file or the ident used in the MODULE_LICENSE() macro needs to change. Thanks, Paul Bolle
On Thu, May 07, 2015 at 10:18:49AM +0200, Paul Bolle wrote: > Just a nit: a license mismatch. > > On Wed, 2015-05-06 at 01:37 -0700, Gregory Fong wrote: > > --- /dev/null > > +++ b/drivers/gpio/gpio-brcmstb.c > > > + * This program is free software; you can redistribute it and/or > > + * modify it under the terms of the GNU General Public License as > > + * published by the Free Software Foundation version 2. > > + * > > + * This program is distributed "as is" WITHOUT ANY WARRANTY of any > > + * kind, whether express or implied; without even the implied warranty > > + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > > + * GNU General Public License for more details. > > This states the license is GPL v2. > > > +MODULE_LICENSE("GPL"); > > And, according to include/linux/module.h, this states the license is GPL > v2 or later. So I think either the comment at the top of this file or > the ident used in the MODULE_LICENSE() macro needs to change. Will fix that, thanks. Gregory
On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote: > This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs > (BCM7XXX and some others). Uses basic_mmio_gpio to instantiate a > gpio_chip for each bank. The driver assumes that it handles the base > set of GPIOs on the system and that it can start its numbering sequence > from 0, so any GPIO expanders used with it must dynamically assign GPIO > numbers after this driver has finished registering its GPIOs. > > Does not implement the interrupt-controller portion yet, will be done in a > future commit. > > List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com> > Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com> (...) > arch/arm/mach-bcm/Kconfig | 1 + (...) > --- a/arch/arm/mach-bcm/Kconfig > +++ b/arch/arm/mach-bcm/Kconfig > @@ -144,6 +144,7 @@ config ARCH_BRCMSTB > select BRCMSTB_GISB_ARB > select BRCMSTB_L2_IRQ > select BCM7120_L2_IRQ > + select ARCH_WANT_OPTIONAL_GPIOLIB Please remove this from this patch. I don't want to patch around in the platforms from the GPIO tree. Take this oneliner through ARM SoC. > +config GPIO_BRCMSTB > + tristate "BRCMSTB GPIO support" > + default y if ARCH_BRCMSTB > + depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST) > + select GPIO_GENERIC Nice. > +++ b/drivers/gpio/gpio-brcmstb.c (...) > + > +#include <linux/bitops.h> > +#include <linux/gpio.h> #include <linux/gpio/driver.h> should be sufficient. > +struct brcmstb_gpio_bank { > + struct list_head node; > + int id; > + struct bgpio_chip bgc; > + u32 imask; /* irq mask shadow register */ Why? Is it a write-only register that can't be read back? > + struct brcmstb_gpio_priv *parent_priv; /* used in interrupt handler */ ...and this patch does not enable IRQs... > +struct brcmstb_gpio_priv { > + struct list_head bank_list; > + void __iomem *reg_base; > + int num_banks; > + struct platform_device *pdev; > + int gpio_base; Usually we don't like it when you hardcode gpio_base, and this field should anyway be present inside the bgpio_chip.gc.base isn't it? > +#define GPIO_PER_BANK 32 > +#define GPIO_BANK(gpio) ((gpio) >> 5) > +/* assumes GPIO_PER_BANK is a multiple of 2 */ > +#define GPIO_BIT(gpio) ((gpio) & (GPIO_PER_BANK - 1)) But this macro and GPIO_PER_BANK does not respect the DT binding stating the number of used lines. You need to call these MAX_GPIO_PER_BANK or something. > +static int brcmstb_gpio_probe(struct platform_device *pdev) > +{ > + struct device *dev = &pdev->dev; > + struct device_node *np = dev->of_node; > + void __iomem *reg_base; > + struct brcmstb_gpio_priv *priv; > + struct resource *res; > + struct property *prop; > + const __be32 *p; > + u32 bank_width; > + int err; > + static int gpio_base; > + > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > + if (!priv) > + return -ENOMEM; > + > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + reg_base = devm_ioremap_resource(dev, res); > + if (IS_ERR(reg_base)) > + return PTR_ERR(reg_base); > + > + priv->gpio_base = gpio_base; > + priv->reg_base = reg_base; > + priv->pdev = pdev; > + > + INIT_LIST_HEAD(&priv->bank_list); > + if (brcmstb_gpio_sanity_check_banks(dev, np, res)) > + return -EINVAL; > + > + of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p, > + bank_width) { > + struct brcmstb_gpio_bank *bank; > + struct bgpio_chip *bgc; > + struct gpio_chip *gc; > + > + bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL); > + if (!bank) { > + err = -ENOMEM; > + goto fail; > + } > + > + bank->parent_priv = priv; > + bank->id = priv->num_banks; > + > + /* > + * Regs are 4 bytes wide, have data reg, no set/clear regs, > + * and direction bits have 0 = output and 1 = input > + */ > + bgc = &bank->bgc; > + err = bgpio_init(bgc, dev, 4, > + reg_base + GIO_DATA(bank->id), > + NULL, NULL, NULL, > + reg_base + GIO_IODIR(bank->id), 0); > + if (err) { > + dev_err(dev, "bgpio_init() failed\n"); > + goto fail; > + } > + > + gc = &bgc->gc; > + gc->of_node = np; > + gc->owner = THIS_MODULE; > + gc->label = np->full_name; > + gc->base = gpio_base; I strongly suggest that you try using -1 as base here instead for dynamic assignment of GPIO numbers. > + gc->of_gpio_n_cells = 2; > + gc->of_xlate = brcmstb_gpio_of_xlate; > + > + if (bank_width <= 0 || bank_width > GPIO_PER_BANK) { > + gc->ngpio = GPIO_PER_BANK; > + dev_warn(dev, "Invalid bank width %d, assume %d\n", > + bank_width, gc->ngpio); I wonder if this should not simply return an error. If that number is wrong the DTS is completely screwed up. > + } else { > + gc->ngpio = bank_width; > + } > + > + bank->imask = > + bgc->read_reg(reg_base + GIO_MASK(bank->id)); And this mask also mask the unused pins as GIO_MASK() does not respect bank_width. > + err = gpiochip_add(gc); > + if (err) { > + dev_err(dev, "Could not add gpiochip for bank %d\n", > + bank->id); > + goto fail; > + } > + gpio_base += gc->ngpio; This complicates things. Use -1 as base for dynamic assignment of GPIO numbers. Yours, Linus Walleij
Hi Linus, On Tue, May 12, 2015 at 3:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, May 6, 2015 at 10:37 AM, Gregory Fong <gregory.0xf0@gmail.com> wrote: > >> This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs >> (BCM7XXX and some others). Uses basic_mmio_gpio to instantiate a >> gpio_chip for each bank. The driver assumes that it handles the base >> set of GPIOs on the system and that it can start its numbering sequence >> from 0, so any GPIO expanders used with it must dynamically assign GPIO >> numbers after this driver has finished registering its GPIOs. >> >> Does not implement the interrupt-controller portion yet, will be done in a >> future commit. >> >> List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com> >> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com> > > (...) >> arch/arm/mach-bcm/Kconfig | 1 + > (...) >> --- a/arch/arm/mach-bcm/Kconfig >> +++ b/arch/arm/mach-bcm/Kconfig >> @@ -144,6 +144,7 @@ config ARCH_BRCMSTB >> select BRCMSTB_GISB_ARB >> select BRCMSTB_L2_IRQ >> select BCM7120_L2_IRQ >> + select ARCH_WANT_OPTIONAL_GPIOLIB > > Please remove this from this patch. I don't want to patch around > in the platforms from the GPIO tree. Take this oneliner through > ARM SoC. Will move to a separate patch. > >> +config GPIO_BRCMSTB >> + tristate "BRCMSTB GPIO support" >> + default y if ARCH_BRCMSTB >> + depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST) >> + select GPIO_GENERIC > > Nice. > >> +++ b/drivers/gpio/gpio-brcmstb.c > (...) >> + >> +#include <linux/bitops.h> >> +#include <linux/gpio.h> > > #include <linux/gpio/driver.h> > > should be sufficient. OK. > >> +struct brcmstb_gpio_bank { >> + struct list_head node; >> + int id; >> + struct bgpio_chip bgc; >> + u32 imask; /* irq mask shadow register */ > > Why? Is it a write-only register that can't be read back? No, that wasn't necessary. Will change to just read the register. > >> + struct brcmstb_gpio_priv *parent_priv; /* used in interrupt handler */ > > ...and this patch does not enable IRQs... OK, I will move it to the patch that does. > >> +struct brcmstb_gpio_priv { >> + struct list_head bank_list; >> + void __iomem *reg_base; >> + int num_banks; >> + struct platform_device *pdev; >> + int gpio_base; > > Usually we don't like it when you hardcode gpio_base, and this > field should anyway be present inside the bgpio_chip.gc.base > isn't it? This was needed to deal with having a single irq_chip shared across all of the gpio_chips in a GIO block. You mentioned that this might not be the Right Way to do this in your reply on the cover page so I'll try to explain the reasoning better there. FWIW: yes, this is inside the first bank's bgpio_chip, and it would be possible to extract that info. However, since it is used in - brcmstb_gpio_to_irq - brcmstb_gpio_hwirq_to_offset - brcmstb_gpio_irq_bank_handler - brcmstb_gpio_of_xlate It seemed like it would be easier to follow if this were just stored this in the priv struct, even if it is duplication of information. > >> +#define GPIO_PER_BANK 32 >> +#define GPIO_BANK(gpio) ((gpio) >> 5) >> +/* assumes GPIO_PER_BANK is a multiple of 2 */ >> +#define GPIO_BIT(gpio) ((gpio) & (GPIO_PER_BANK - 1)) > > But this macro and GPIO_PER_BANK does not respect the DT binding > stating the number of used lines. > > You need to call these MAX_GPIO_PER_BANK or something. Will change the name to MAX_GPIO_PER_BANK. > >> +static int brcmstb_gpio_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct device_node *np = dev->of_node; >> + void __iomem *reg_base; >> + struct brcmstb_gpio_priv *priv; >> + struct resource *res; >> + struct property *prop; >> + const __be32 *p; >> + u32 bank_width; >> + int err; >> + static int gpio_base; >> + >> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); >> + if (!priv) >> + return -ENOMEM; >> + >> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); >> + reg_base = devm_ioremap_resource(dev, res); >> + if (IS_ERR(reg_base)) >> + return PTR_ERR(reg_base); >> + >> + priv->gpio_base = gpio_base; >> + priv->reg_base = reg_base; >> + priv->pdev = pdev; >> + >> + INIT_LIST_HEAD(&priv->bank_list); >> + if (brcmstb_gpio_sanity_check_banks(dev, np, res)) >> + return -EINVAL; >> + >> + of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p, >> + bank_width) { >> + struct brcmstb_gpio_bank *bank; >> + struct bgpio_chip *bgc; >> + struct gpio_chip *gc; >> + >> + bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL); >> + if (!bank) { >> + err = -ENOMEM; >> + goto fail; >> + } >> + >> + bank->parent_priv = priv; >> + bank->id = priv->num_banks; >> + >> + /* >> + * Regs are 4 bytes wide, have data reg, no set/clear regs, >> + * and direction bits have 0 = output and 1 = input >> + */ >> + bgc = &bank->bgc; >> + err = bgpio_init(bgc, dev, 4, >> + reg_base + GIO_DATA(bank->id), >> + NULL, NULL, NULL, >> + reg_base + GIO_IODIR(bank->id), 0); >> + if (err) { >> + dev_err(dev, "bgpio_init() failed\n"); >> + goto fail; >> + } >> + >> + gc = &bgc->gc; >> + gc->of_node = np; >> + gc->owner = THIS_MODULE; >> + gc->label = np->full_name; >> + gc->base = gpio_base; > > I strongly suggest that you try using -1 as base here instead > for dynamic assignment of GPIO numbers. That is what I did originally. However, this results in a very unpleasant numbering scheme, at least as currently implemented. When -1 is base, as you know, numbering goes descending from 255 (IIRC). Right now I'm using the of_property_for_each_u32 loop over bank widths to go through the banks. To keep the example straightforward, let's pretend our GIO block only has two banks. Here's how they're arranged: bank 0: starts at 0xf040a700, contains GPIOs 0-31 bank 1: starts at 0xf040a720, contains GPIOs 32-63 Right now, with -1 as base, calling gpiochip_add() inside of that loop will results in them getting this numbering: bank 0: linux GPIOs 224-255 bank 1: linux GPIOs 192-223 which has the obvious downside that the ordering doesn't at all match the hardware, making it completely unintuitive for anyone using the (admittedly quite awful) sysfs interface. Looking at this now, I think I could just add another loop afterward to do the gpiochip_add()'s in reverse order, resulting in the numbering ascending with banks as expected. Does this seem sensible? > >> + gc->of_gpio_n_cells = 2; >> + gc->of_xlate = brcmstb_gpio_of_xlate; >> + >> + if (bank_width <= 0 || bank_width > GPIO_PER_BANK) { >> + gc->ngpio = GPIO_PER_BANK; >> + dev_warn(dev, "Invalid bank width %d, assume %d\n", >> + bank_width, gc->ngpio); > > I wonder if this should not simply return an error. > If that number is wrong the DTS is completely screwed up. You're right, probably better to have that be an error. Will change that. > >> + } else { >> + gc->ngpio = bank_width; >> + } >> + >> + bank->imask = >> + bgc->read_reg(reg_base + GIO_MASK(bank->id)); > > And this mask also mask the unused pins as GIO_MASK() > does not respect bank_width. I'll be getting rid of imask anyway as you suggested. But since you mentioned it, I just realized that the register specification says that reads of the reserved bits can return an unknown value but writes of those bits should be 0. While in practice it doesn't seem to cause any problems, I'll change that anyway just to be safe. > >> + err = gpiochip_add(gc); >> + if (err) { >> + dev_err(dev, "Could not add gpiochip for bank %d\n", >> + bank->id); >> + goto fail; >> + } >> + gpio_base += gc->ngpio; > > This complicates things. Use -1 as base for dynamic assignment > of GPIO numbers. (this was covered above) Thanks for the review, Gregory
On Tue, May 12, 2015 at 8:46 PM, Gregory Fong <gregory.0xf0@gmail.com> wrote: > On Tue, May 12, 2015 at 3:55 AM, Linus Walleij <linus.walleij@linaro.org> wrote: >> Usually we don't like it when you hardcode gpio_base, and this >> field should anyway be present inside the bgpio_chip.gc.base >> isn't it? > > This was needed to deal with having a single irq_chip shared across > all of the gpio_chips in a GIO block. You mentioned that this might > not be the Right Way to do this in your reply on the cover page so > I'll try to explain the reasoning better there. > > FWIW: yes, this is inside the first bank's bgpio_chip, and it would be > possible to extract that info. However, since it is used in > - brcmstb_gpio_to_irq > - brcmstb_gpio_hwirq_to_offset > - brcmstb_gpio_irq_bank_handler > - brcmstb_gpio_of_xlate > > It seemed like it would be easier to follow if this were just stored > this in the priv struct, even if it is duplication of information. OK I see. Even if you go for the approach I suggest in the cover letter it is indeed necessary to keep track of the base I can see, since we span multiple gpio_chips So it's fine with a local variable for this. >>> + gc->base = gpio_base; >> >> I strongly suggest that you try using -1 as base here instead >> for dynamic assignment of GPIO numbers. > > That is what I did originally. However, this results in a very > unpleasant numbering scheme, at least as currently implemented. > > When -1 is base, as you know, numbering goes descending from 255 > (IIRC). Right now I'm using the of_property_for_each_u32 loop over > bank widths to go through the banks. To keep the example > straightforward, let's pretend our GIO block only has two banks. > Here's how they're arranged: > > bank 0: starts at 0xf040a700, contains GPIOs 0-31 > bank 1: starts at 0xf040a720, contains GPIOs 32-63 > > Right now, with -1 as base, calling gpiochip_add() inside of that loop > will results in them getting this numbering: > > bank 0: linux GPIOs 224-255 > bank 1: linux GPIOs 192-223 Yeah I kind of think it's a feature because we don't want people to rely on the static GPIO numbering ;) Buy OK yeah I see the point. Let's keep the base static for now. > Looking at this now, I think I could just add another loop afterward > to do the gpiochip_add()'s in reverse order, resulting in the > numbering ascending with banks as expected. Does this seem sensible? No, that relies on the internal semantic structure of the gpiolib. It's better to use .base as a hint then. >> And this mask also mask the unused pins as GIO_MASK() >> does not respect bank_width. > > I'll be getting rid of imask anyway as you suggested. Awesome. Yours, Linus Walleij
diff --git a/MAINTAINERS b/MAINTAINERS index b399b34..781806a 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -2235,6 +2235,13 @@ N: bcm9583* N: bcm583* N: bcm113* +BROADCOM BRCMSTB GPIO DRIVER +M: Gregory Fong <gregory.0xf0@gmail.com> +L: bcm-kernel-feedback-list@broadcom.com> +S: Supported +F: drivers/gpio/gpio-brcmstb.c +F: Documentation/devicetree/bindings/gpio/brcm,brcmstb-gpio.txt + BROADCOM KONA GPIO DRIVER M: Ray Jui <rjui@broadcom.com> L: bcm-kernel-feedback-list@broadcom.com diff --git a/arch/arm/mach-bcm/Kconfig b/arch/arm/mach-bcm/Kconfig index 8b11f44..0ac9e4b3 100644 --- a/arch/arm/mach-bcm/Kconfig +++ b/arch/arm/mach-bcm/Kconfig @@ -144,6 +144,7 @@ config ARCH_BRCMSTB select BRCMSTB_GISB_ARB select BRCMSTB_L2_IRQ select BCM7120_L2_IRQ + select ARCH_WANT_OPTIONAL_GPIOLIB help Say Y if you intend to run the kernel on a Broadcom ARM-based STB chipset. diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index caefe80..5f79b7f 100644 --- a/drivers/gpio/Kconfig +++ b/drivers/gpio/Kconfig @@ -126,6 +126,14 @@ config GPIO_BCM_KONA help Turn on GPIO support for Broadcom "Kona" chips. +config GPIO_BRCMSTB + tristate "BRCMSTB GPIO support" + default y if ARCH_BRCMSTB + depends on OF_GPIO && (ARCH_BRCMSTB || COMPILE_TEST) + select GPIO_GENERIC + help + Say yes here to enable GPIO support for Broadcom STB (BCM7XXX) SoCs. + config GPIO_CLPS711X tristate "CLPS711X GPIO support" depends on ARCH_CLPS711X || COMPILE_TEST diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile index f71bb97..9bfaaa8 100644 --- a/drivers/gpio/Makefile +++ b/drivers/gpio/Makefile @@ -21,6 +21,7 @@ obj-$(CONFIG_GPIO_ALTERA) += gpio-altera.o 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_BRCMSTB) += gpio-brcmstb.o obj-$(CONFIG_GPIO_BT8XX) += gpio-bt8xx.o obj-$(CONFIG_GPIO_CLPS711X) += gpio-clps711x.o obj-$(CONFIG_GPIO_CS5535) += gpio-cs5535.o diff --git a/drivers/gpio/gpio-brcmstb.c b/drivers/gpio/gpio-brcmstb.c new file mode 100644 index 0000000..c8f9014 --- /dev/null +++ b/drivers/gpio/gpio-brcmstb.c @@ -0,0 +1,243 @@ +/* + * Copyright (C) 2015 Broadcom Corporation + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation version 2. + * + * This program is distributed "as is" WITHOUT ANY WARRANTY of any + * kind, whether express or implied; without even the implied warranty + * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/bitops.h> +#include <linux/gpio.h> +#include <linux/of_device.h> +#include <linux/of_irq.h> +#include <linux/module.h> +#include <linux/basic_mmio_gpio.h> + +#define GIO_BANK_SIZE 0x20 +#define GIO_ODEN(bank) (((bank) * GIO_BANK_SIZE) + 0x00) +#define GIO_DATA(bank) (((bank) * GIO_BANK_SIZE) + 0x04) +#define GIO_IODIR(bank) (((bank) * GIO_BANK_SIZE) + 0x08) +#define GIO_EC(bank) (((bank) * GIO_BANK_SIZE) + 0x0c) +#define GIO_EI(bank) (((bank) * GIO_BANK_SIZE) + 0x10) +#define GIO_MASK(bank) (((bank) * GIO_BANK_SIZE) + 0x14) +#define GIO_LEVEL(bank) (((bank) * GIO_BANK_SIZE) + 0x18) +#define GIO_STAT(bank) (((bank) * GIO_BANK_SIZE) + 0x1c) + +struct brcmstb_gpio_bank { + struct list_head node; + int id; + struct bgpio_chip bgc; + u32 imask; /* irq mask shadow register */ + struct brcmstb_gpio_priv *parent_priv; /* used in interrupt handler */ +}; + +struct brcmstb_gpio_priv { + struct list_head bank_list; + void __iomem *reg_base; + int num_banks; + struct platform_device *pdev; + int gpio_base; +}; + +#define GPIO_PER_BANK 32 +#define GPIO_BANK(gpio) ((gpio) >> 5) +/* assumes GPIO_PER_BANK is a multiple of 2 */ +#define GPIO_BIT(gpio) ((gpio) & (GPIO_PER_BANK - 1)) + +static inline struct brcmstb_gpio_priv * +brcmstb_gpio_gc_to_priv(struct gpio_chip *gc) +{ + struct bgpio_chip *bgc = to_bgpio_chip(gc); + struct brcmstb_gpio_bank *bank = + container_of(bgc, struct brcmstb_gpio_bank, bgc); + + return bank->parent_priv; +} + +/* Make sure that the number of banks matches up between properties */ +static int brcmstb_gpio_sanity_check_banks(struct device *dev, + struct device_node *np, struct resource *res) +{ + int res_num_banks = resource_size(res) / GIO_BANK_SIZE; + int num_banks = of_property_count_u32_elems(np, "brcm,gpio-bank-widths"); + + if (res_num_banks != num_banks) { + dev_err(dev, "Mismatch in banks: res had %d, bank-widths had %d\n", + res_num_banks, num_banks); + return -EINVAL; + } else { + return 0; + } +} + +static int brcmstb_gpio_remove(struct platform_device *pdev) +{ + struct brcmstb_gpio_priv *priv = platform_get_drvdata(pdev); + struct list_head *pos; + struct brcmstb_gpio_bank *bank; + int ret = 0; + + list_for_each(pos, &priv->bank_list) { + bank = list_entry(pos, struct brcmstb_gpio_bank, node); + ret = bgpio_remove(&bank->bgc); + if (ret) + dev_err(&pdev->dev, "gpiochip_remove fail in cleanup"); + } + return ret; +} + +static int brcmstb_gpio_of_xlate(struct gpio_chip *gc, + const struct of_phandle_args *gpiospec, u32 *flags) +{ + struct brcmstb_gpio_priv *priv = brcmstb_gpio_gc_to_priv(gc); + int offset; + + if (gc->of_gpio_n_cells != 2) { + WARN_ON(1); + return -EINVAL; + } + + if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells)) + return -EINVAL; + + offset = gpiospec->args[0] - (gc->base - priv->gpio_base); + if (offset >= gc->ngpio) + return -EINVAL; + + if (flags) + *flags = gpiospec->args[1]; + + return offset; +} + +static int brcmstb_gpio_probe(struct platform_device *pdev) +{ + struct device *dev = &pdev->dev; + struct device_node *np = dev->of_node; + void __iomem *reg_base; + struct brcmstb_gpio_priv *priv; + struct resource *res; + struct property *prop; + const __be32 *p; + u32 bank_width; + int err; + static int gpio_base; + + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); + reg_base = devm_ioremap_resource(dev, res); + if (IS_ERR(reg_base)) + return PTR_ERR(reg_base); + + priv->gpio_base = gpio_base; + priv->reg_base = reg_base; + priv->pdev = pdev; + + INIT_LIST_HEAD(&priv->bank_list); + if (brcmstb_gpio_sanity_check_banks(dev, np, res)) + return -EINVAL; + + of_property_for_each_u32(np, "brcm,gpio-bank-widths", prop, p, + bank_width) { + struct brcmstb_gpio_bank *bank; + struct bgpio_chip *bgc; + struct gpio_chip *gc; + + bank = devm_kzalloc(dev, sizeof(*bank), GFP_KERNEL); + if (!bank) { + err = -ENOMEM; + goto fail; + } + + bank->parent_priv = priv; + bank->id = priv->num_banks; + + /* + * Regs are 4 bytes wide, have data reg, no set/clear regs, + * and direction bits have 0 = output and 1 = input + */ + bgc = &bank->bgc; + err = bgpio_init(bgc, dev, 4, + reg_base + GIO_DATA(bank->id), + NULL, NULL, NULL, + reg_base + GIO_IODIR(bank->id), 0); + if (err) { + dev_err(dev, "bgpio_init() failed\n"); + goto fail; + } + + gc = &bgc->gc; + gc->of_node = np; + gc->owner = THIS_MODULE; + gc->label = np->full_name; + gc->base = gpio_base; + gc->of_gpio_n_cells = 2; + gc->of_xlate = brcmstb_gpio_of_xlate; + + if (bank_width <= 0 || bank_width > GPIO_PER_BANK) { + gc->ngpio = GPIO_PER_BANK; + dev_warn(dev, "Invalid bank width %d, assume %d\n", + bank_width, gc->ngpio); + } else { + gc->ngpio = bank_width; + } + + bank->imask = + bgc->read_reg(reg_base + GIO_MASK(bank->id)); + + err = gpiochip_add(gc); + if (err) { + dev_err(dev, "Could not add gpiochip for bank %d\n", + bank->id); + goto fail; + } + gpio_base += gc->ngpio; + dev_dbg(dev, "bank=%d, base=%d, ngpio=%d\n", bank->id, + gc->base, gc->ngpio); + + /* Everything looks good, so add bank to list */ + list_add(&bank->node, &priv->bank_list); + + priv->num_banks++; + } + + dev_info(dev, "Registered %d banks (GPIO(s): %d-%d)\n", + priv->num_banks, priv->gpio_base, gpio_base - 1); + + platform_set_drvdata(pdev, priv); + + return 0; + +fail: + (void) brcmstb_gpio_remove(pdev); + return err; +} + +static struct of_device_id brcmstb_gpio_of_match[] = { + { .compatible = "brcm,brcmstb-gpio" }, + {}, +}; + +MODULE_DEVICE_TABLE(of, brcmstb_gpio_of_match); + +static struct platform_driver brcmstb_gpio_driver = { + .driver = { + .name = "brcmstb-gpio", + .of_match_table = brcmstb_gpio_of_match, + }, + .probe = brcmstb_gpio_probe, + .remove = brcmstb_gpio_remove, +}; +module_platform_driver(brcmstb_gpio_driver); + +MODULE_AUTHOR("Broadcom"); +MODULE_DESCRIPTION("Driver for BRCMSTB UPG GPIO"); +MODULE_LICENSE("GPL");
This adds support for the GPIO IP "UPG GIO" used on Broadcom STB SoCs (BCM7XXX and some others). Uses basic_mmio_gpio to instantiate a gpio_chip for each bank. The driver assumes that it handles the base set of GPIOs on the system and that it can start its numbering sequence from 0, so any GPIO expanders used with it must dynamically assign GPIO numbers after this driver has finished registering its GPIOs. Does not implement the interrupt-controller portion yet, will be done in a future commit. List-usage-fixed-by: Brian Norris <computersforpeace@gmail.com> Signed-off-by: Gregory Fong <gregory.0xf0@gmail.com> --- MAINTAINERS | 7 ++ arch/arm/mach-bcm/Kconfig | 1 + drivers/gpio/Kconfig | 8 ++ drivers/gpio/Makefile | 1 + drivers/gpio/gpio-brcmstb.c | 243 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 260 insertions(+) create mode 100644 drivers/gpio/gpio-brcmstb.c