* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
[not found] ` <1400263432-922-2-git-send-email-fkan@apm.com>
@ 2014-05-20 6:04 ` Alexandre Courbot
2014-05-27 7:59 ` Linus Walleij
1 sibling, 0 replies; 24+ messages in thread
From: Alexandre Courbot @ 2014-05-20 6:04 UTC (permalink / raw
To: Feng Kan
Cc: devicetree@vger.kernel.org, Catalin Marinas, Linus Walleij,
patches, Will Deacon, linux-gpio@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
On Sat, May 17, 2014 at 3:03 AM, Feng Kan <fkan@apm.com> wrote:
> Add APM X-Gene SoC gpio controller driver.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/gpio/Kconfig | 9 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-xgene.c | 264 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 274 insertions(+)
> create mode 100644 drivers/gpio/gpio-xgene.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..8f89643 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -324,6 +324,15 @@ config GPIO_TZ1090_PDC
> help
> Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs.
>
> +config GPIO_XGENE
> + bool "APM X-Gene GPIO controller support"
> + depends on ARM64 && OF_GPIO
> + help
> + This driver is to support the GPIO block within the APM X-Gene
> + platform's generic flash controller. The GPIO pins are muxed with
> + the generic flash controller's address and data pins. Say yes
> + here to enable the GFC GPIO functionality.
> +
> config GPIO_XILINX
> bool "Xilinx GPIO support"
> depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..f0f2830 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
> obj-$(CONFIG_GPIO_WM831X) += gpio-wm831x.o
> obj-$(CONFIG_GPIO_WM8350) += gpio-wm8350.o
> obj-$(CONFIG_GPIO_WM8994) += gpio-wm8994.o
> +obj-$(CONFIG_GPIO_XGENE) += gpio-xgene.o
> obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o
> obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o
> obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
> diff --git a/drivers/gpio/gpio-xgene.c b/drivers/gpio/gpio-xgene.c
> new file mode 100644
> index 0000000..5c26a19
> --- /dev/null
> +++ b/drivers/gpio/gpio-xgene.c
> @@ -0,0 +1,264 @@
> +/*
> + * AppliedMicro X-Gene SoC GPIO Driver
> + *
> + * Copyright (c) 2014, Applied Micro Circuits Corporation
> + * Author: Feng Kan <fkan@apm.com>.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/spinlock.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +
> +#define GPIO_MASK(x) (1U << ((x) % 32))
BIT((x) % 32) maybe? (same below where it applies)
> +#define GPIO_MUX_SEL(x) (3U << ((x * 2) % 32))
> +#define GPIO_SET_MASK(x) (1U << ((x + 16) % 32))
> +
> +#define GPIO_SET_DR 0x0c
> +#define GPIO_SET_DR_OFFSET 0x0c
> +#define GPIO_DATA 0x14
> +#define GPIO_DATA_OFFSET 0x0c
> +#define GPIO_OD 0x30
> +#define GPIO_OD_OFFSET 0x04
> +#define GPIO_MUX 0x10
> +#define GPIO_MUX_OFFSET 0x0c
> +
> +#define XGENE_GPIO_MAX_PORTS 3
> +
> +struct xgene_gpio;
> +
> +struct xgene_gpio_bank {
> + struct gpio_chip chip;
> + void __iomem *data;
> + void __iomem *set_dr;
> + void __iomem *od;
> + void __iomem *mux;
> + struct xgene_gpio *gpio;
> + spinlock_t lock;
> +};
> +
> +struct xgene_gpio {
> + struct device *dev;
> + void __iomem *regs;
> + struct xgene_gpio_bank *banks;
> + unsigned int nr_banks;
> +};
At first sight it really looks like the gpio_chip should belong to the
xgene_gpio struct. What is the purpose of having the driver
instanciated once per bank?
> +
> +static int xgene_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> +
> + return (ioread32(bank->data) & GPIO_MASK(gpio));
Since you are using ioread32, wouldn't the member be better defined as
"u32 __iomem *data" ?
> +}
> +
> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
Couldn't you use set_bit() / clear_bit() instead here?
> + iowrite32(data, bank->set_dr);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +static void xgene_gpio_muxcfg(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + u32 data;
> +
> + data = ioread32(bank->mux);
> + data |= GPIO_MUX_SEL(gpio);
> + iowrite32(data, bank->mux);
> +}
> +
> +static int xgene_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data |= GPIO_MASK(gpio);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
> +
> +static int xgene_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> + data = ioread32(bank->set_dr);
> + data = data & ~GPIO_MASK(gpio);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
> +
> +static int xgene_gpio_add_bank(struct xgene_gpio *gpio,
> + struct device_node *bank_np,
> + unsigned int offs)
> +{
> + struct xgene_gpio_bank *bank;
> + u32 bank_idx, ngpio, odcfg;
> + int err;
> +
> + if (of_property_read_u32(bank_np, "bank", &bank_idx) ||
> + bank_idx >= XGENE_GPIO_MAX_PORTS) {
> + dev_err(gpio->dev, "missing/invalid bank index for %s index %d\n",
> + bank_np->full_name, bank_idx);
> + return -EINVAL;
Could you return the error returned by of_property_read_u32() instead?
> + }
> +
> + bank = &gpio->banks[offs];
> + bank->gpio = gpio;
> + spin_lock_init(&bank->lock);
> +
> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
> + ngpio = 16;
Here I think you want to distinguish between "property not defined"
(-EINVAL) and "property has no value" (-ENODATA) and other possible
errors. In the later cases you will want to signal the error and
return from here.
> +
> + if ((ngpio > 16) || (ngpio < 1)) {
> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
> + bank_np->full_name);
> + return -EINVAL;
> + }
> +
> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
> + bank->set_dr = gpio->regs + GPIO_SET_DR
> + + (bank_idx * GPIO_SET_DR_OFFSET);
> +
> + bank->chip.direction_input = xgene_gpio_dir_in;
> + bank->chip.direction_output = xgene_gpio_dir_out;
> + bank->chip.get = xgene_gpio_get;
> + bank->chip.set = xgene_gpio_set;
> +
> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
> + iowrite32(odcfg, bank->od);
> +
> + bank->chip.ngpio = ngpio;
> + bank->chip.of_node = bank_np;
> + bank->chip.base = bank_idx * ngpio;
> +
> + err = gpiochip_add(&bank->chip);
> + if (err)
> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
> + bank_np->full_name);
> +
> + return err;
> +}
> +
> +static int xgene_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + struct xgene_gpio *gpio;
> + int err;
> + unsigned int offs = 0;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> + gpio->dev = &pdev->dev;
> +
> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
> + if (!gpio->nr_banks) {
> + err = -EINVAL;
> + goto err;
> + }
Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
return an error here.
> +
> + gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
> + sizeof(*gpio->banks), GFP_KERNEL);
> + if (!gpio->banks) {
> + err = -ENOMEM;
> + goto err;
> + }
I'm fine with printing the error status the way you do, but could you
also do the same for the first kzalloc too?
Or maybe you could put your banks member at the end of the xgene_gpio
struct as a flexible array member and perform only one kzalloc of size
(sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(gpio->regs)) {
> + err = PTR_ERR(gpio->regs);
> + goto err;
> + }
> +
> + for_each_child_of_node(pdev->dev.of_node, np) {
> + err = xgene_gpio_add_bank(gpio, np, offs++);
> + if (err)
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, gpio);
> +
> + dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
> + return 0;
> +err:
> + dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
> + return err;
> +}
> +
> +static const struct of_device_id xgene_gpio_of_match[] = {
> + { .compatible = "apm,xgene-gpio", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
> +
> +static struct platform_driver xgene_gpio_driver = {
> + .driver = {
> + .name = "xgene-gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = xgene_gpio_of_match,
> + },
> + .probe = xgene_gpio_probe,
> +};
> +
> +static int __init xgene_gpio_init(void)
> +{
> + return platform_driver_register(&xgene_gpio_driver);
> +}
> +postcore_initcall(xgene_gpio_init);
> +
> +MODULE_AUTHOR("AppliedMicro");
> +MODULE_DESCRIPTION("APM X-Gene GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 1.9.1
>
Globally the driver looks well-written, but I don't understand the
need to have one gpio_chip per bank. Could you elaborate on the
reasons for this design?
Alex.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-20 6:04 ` Alexandre Courbot
0 siblings, 0 replies; 24+ messages in thread
From: Alexandre Courbot @ 2014-05-20 6:04 UTC (permalink / raw
To: linux-arm-kernel
On Sat, May 17, 2014 at 3:03 AM, Feng Kan <fkan@apm.com> wrote:
> Add APM X-Gene SoC gpio controller driver.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> drivers/gpio/Kconfig | 9 ++
> drivers/gpio/Makefile | 1 +
> drivers/gpio/gpio-xgene.c | 264 ++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 274 insertions(+)
> create mode 100644 drivers/gpio/gpio-xgene.c
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index a86c49a..8f89643 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -324,6 +324,15 @@ config GPIO_TZ1090_PDC
> help
> Say yes here to support Toumaz Xenif TZ1090 PDC GPIOs.
>
> +config GPIO_XGENE
> + bool "APM X-Gene GPIO controller support"
> + depends on ARM64 && OF_GPIO
> + help
> + This driver is to support the GPIO block within the APM X-Gene
> + platform's generic flash controller. The GPIO pins are muxed with
> + the generic flash controller's address and data pins. Say yes
> + here to enable the GFC GPIO functionality.
> +
> config GPIO_XILINX
> bool "Xilinx GPIO support"
> depends on PPC_OF || MICROBLAZE || ARCH_ZYNQ
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 6309aff..f0f2830 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -98,6 +98,7 @@ obj-$(CONFIG_GPIO_VX855) += gpio-vx855.o
> obj-$(CONFIG_GPIO_WM831X) += gpio-wm831x.o
> obj-$(CONFIG_GPIO_WM8350) += gpio-wm8350.o
> obj-$(CONFIG_GPIO_WM8994) += gpio-wm8994.o
> +obj-$(CONFIG_GPIO_XGENE) += gpio-xgene.o
> obj-$(CONFIG_GPIO_XILINX) += gpio-xilinx.o
> obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o
> obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
> diff --git a/drivers/gpio/gpio-xgene.c b/drivers/gpio/gpio-xgene.c
> new file mode 100644
> index 0000000..5c26a19
> --- /dev/null
> +++ b/drivers/gpio/gpio-xgene.c
> @@ -0,0 +1,264 @@
> +/*
> + * AppliedMicro X-Gene SoC GPIO Driver
> + *
> + * Copyright (c) 2014, Applied Micro Circuits Corporation
> + * Author: Feng Kan <fkan@apm.com>.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/spinlock.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of.h>
> +#include <linux/gpio.h>
> +#include <linux/types.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +
> +#define GPIO_MASK(x) (1U << ((x) % 32))
BIT((x) % 32) maybe? (same below where it applies)
> +#define GPIO_MUX_SEL(x) (3U << ((x * 2) % 32))
> +#define GPIO_SET_MASK(x) (1U << ((x + 16) % 32))
> +
> +#define GPIO_SET_DR 0x0c
> +#define GPIO_SET_DR_OFFSET 0x0c
> +#define GPIO_DATA 0x14
> +#define GPIO_DATA_OFFSET 0x0c
> +#define GPIO_OD 0x30
> +#define GPIO_OD_OFFSET 0x04
> +#define GPIO_MUX 0x10
> +#define GPIO_MUX_OFFSET 0x0c
> +
> +#define XGENE_GPIO_MAX_PORTS 3
> +
> +struct xgene_gpio;
> +
> +struct xgene_gpio_bank {
> + struct gpio_chip chip;
> + void __iomem *data;
> + void __iomem *set_dr;
> + void __iomem *od;
> + void __iomem *mux;
> + struct xgene_gpio *gpio;
> + spinlock_t lock;
> +};
> +
> +struct xgene_gpio {
> + struct device *dev;
> + void __iomem *regs;
> + struct xgene_gpio_bank *banks;
> + unsigned int nr_banks;
> +};
At first sight it really looks like the gpio_chip should belong to the
xgene_gpio struct. What is the purpose of having the driver
instanciated once per bank?
> +
> +static int xgene_gpio_get(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> +
> + return (ioread32(bank->data) & GPIO_MASK(gpio));
Since you are using ioread32, wouldn't the member be better defined as
"u32 __iomem *data" ?
> +}
> +
> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
Couldn't you use set_bit() / clear_bit() instead here?
> + iowrite32(data, bank->set_dr);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +static void xgene_gpio_muxcfg(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + u32 data;
> +
> + data = ioread32(bank->mux);
> + data |= GPIO_MUX_SEL(gpio);
> + iowrite32(data, bank->mux);
> +}
> +
> +static int xgene_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data |= GPIO_MASK(gpio);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
> +
> +static int xgene_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> + data = ioread32(bank->set_dr);
> + data = data & ~GPIO_MASK(gpio);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
> +
> +static int xgene_gpio_add_bank(struct xgene_gpio *gpio,
> + struct device_node *bank_np,
> + unsigned int offs)
> +{
> + struct xgene_gpio_bank *bank;
> + u32 bank_idx, ngpio, odcfg;
> + int err;
> +
> + if (of_property_read_u32(bank_np, "bank", &bank_idx) ||
> + bank_idx >= XGENE_GPIO_MAX_PORTS) {
> + dev_err(gpio->dev, "missing/invalid bank index for %s index %d\n",
> + bank_np->full_name, bank_idx);
> + return -EINVAL;
Could you return the error returned by of_property_read_u32() instead?
> + }
> +
> + bank = &gpio->banks[offs];
> + bank->gpio = gpio;
> + spin_lock_init(&bank->lock);
> +
> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
> + ngpio = 16;
Here I think you want to distinguish between "property not defined"
(-EINVAL) and "property has no value" (-ENODATA) and other possible
errors. In the later cases you will want to signal the error and
return from here.
> +
> + if ((ngpio > 16) || (ngpio < 1)) {
> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
> + bank_np->full_name);
> + return -EINVAL;
> + }
> +
> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
> + bank->set_dr = gpio->regs + GPIO_SET_DR
> + + (bank_idx * GPIO_SET_DR_OFFSET);
> +
> + bank->chip.direction_input = xgene_gpio_dir_in;
> + bank->chip.direction_output = xgene_gpio_dir_out;
> + bank->chip.get = xgene_gpio_get;
> + bank->chip.set = xgene_gpio_set;
> +
> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
> + iowrite32(odcfg, bank->od);
> +
> + bank->chip.ngpio = ngpio;
> + bank->chip.of_node = bank_np;
> + bank->chip.base = bank_idx * ngpio;
> +
> + err = gpiochip_add(&bank->chip);
> + if (err)
> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
> + bank_np->full_name);
> +
> + return err;
> +}
> +
> +static int xgene_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + struct xgene_gpio *gpio;
> + int err;
> + unsigned int offs = 0;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> + gpio->dev = &pdev->dev;
> +
> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
> + if (!gpio->nr_banks) {
> + err = -EINVAL;
> + goto err;
> + }
Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
return an error here.
> +
> + gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
> + sizeof(*gpio->banks), GFP_KERNEL);
> + if (!gpio->banks) {
> + err = -ENOMEM;
> + goto err;
> + }
I'm fine with printing the error status the way you do, but could you
also do the same for the first kzalloc too?
Or maybe you could put your banks member at the end of the xgene_gpio
struct as a flexible array member and perform only one kzalloc of size
(sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?
> +
> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
> + if (IS_ERR(gpio->regs)) {
> + err = PTR_ERR(gpio->regs);
> + goto err;
> + }
> +
> + for_each_child_of_node(pdev->dev.of_node, np) {
> + err = xgene_gpio_add_bank(gpio, np, offs++);
> + if (err)
> + goto err;
> + }
> +
> + platform_set_drvdata(pdev, gpio);
> +
> + dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
> + return 0;
> +err:
> + dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
> + return err;
> +}
> +
> +static const struct of_device_id xgene_gpio_of_match[] = {
> + { .compatible = "apm,xgene-gpio", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
> +
> +static struct platform_driver xgene_gpio_driver = {
> + .driver = {
> + .name = "xgene-gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = xgene_gpio_of_match,
> + },
> + .probe = xgene_gpio_probe,
> +};
> +
> +static int __init xgene_gpio_init(void)
> +{
> + return platform_driver_register(&xgene_gpio_driver);
> +}
> +postcore_initcall(xgene_gpio_init);
> +
> +MODULE_AUTHOR("AppliedMicro");
> +MODULE_DESCRIPTION("APM X-Gene GPIO driver");
> +MODULE_LICENSE("GPL");
> --
> 1.9.1
>
Globally the driver looks well-written, but I don't understand the
need to have one gpio_chip per bank. Could you elaborate on the
reasons for this design?
Alex.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding
[not found] ` <1400263432-922-4-git-send-email-fkan@apm.com>
@ 2014-05-20 6:11 ` Alexandre Courbot
2014-05-27 8:30 ` Linus Walleij
1 sibling, 0 replies; 24+ messages in thread
From: Alexandre Courbot @ 2014-05-20 6:11 UTC (permalink / raw
To: Feng Kan
Cc: devicetree@vger.kernel.org, Catalin Marinas, Linus Walleij,
patches, Will Deacon, linux-gpio@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
On Sat, May 17, 2014 at 3:03 AM, Feng Kan <fkan@apm.com> wrote:
> Documentation for APM X-Gene SoC GPIO controller DTS binding.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> .../devicetree/bindings/gpio/gpio-xgene.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> new file mode 100644
> index 0000000..e19eb5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> @@ -0,0 +1,57 @@
> +APM X-Gene SoC GPIO controller bindings
> +
> +This is a gpio controller that is part of the flash controller.
> +All registers are 32bit and in little endian format.
> +
> +Required properties:
> +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller
> +- reg: Physical base address and length of the controller's registers
> +
> +The gpio controller has multiple banks, each bank will have 16 or less
> +gpio pins. All cfg field will be little endian and least significant
> +bit indicate lowest number of gpio.
> +
> +Required properties:
> +- #gpio-cells: Should be two.
> + - first cell is the pin number
> + - second cell is used to specify optional parameters (unused)
> +- gpio-controller: Marks the device node as a GPIO controller.
Shouldnt these properties be part of the parent instead of being
repeated for each bank?
> +- bank: This is to specifiy which gpio bank the dts node is describing.
I suspect you could use the reg property for this, as it seems like
you would need it for DT compliance anyway (but I'm not 100% sure
here).
> +
> +Optional properties:
> +- ngpio: Specify the number of GPIOs per bank. It is defaulted to 16
> + if not specified. Number of gpio can't be greater than 16 or
> + 0.
> +- odcfg: This is gpio open drain/normal configuration. It is a 16 bit
> + field, setting 1 will mean the pin is set in open drain
> + configuration, 0 will mean normal configuration. Default will
> + be normal configuration.
> +
> +Example:
> + gpio: gpio@1701c000 {
> + compatible = "apm,xgene-gpio";
> + reg = <0x0 0x1701c000 0x0 0x1000>;
> +
> + banka: gpio-controller@0 {
> + compatible = "apm,xgene-gpio-port";
This property is not documented. Besides I don't see it anywhere in
your driver implementation so I would say it is probably not needed at
all.
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <0>;
> + odcfg = <0xffff>; /* Optional */
> + };
> + bankb: gpio-controller@1 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <1>;
> + };
> + bankc: gpio-controller@2 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <2>;
> + };
> + };
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding
@ 2014-05-20 6:11 ` Alexandre Courbot
0 siblings, 0 replies; 24+ messages in thread
From: Alexandre Courbot @ 2014-05-20 6:11 UTC (permalink / raw
To: linux-arm-kernel
On Sat, May 17, 2014 at 3:03 AM, Feng Kan <fkan@apm.com> wrote:
> Documentation for APM X-Gene SoC GPIO controller DTS binding.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
> ---
> .../devicetree/bindings/gpio/gpio-xgene.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> new file mode 100644
> index 0000000..e19eb5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> @@ -0,0 +1,57 @@
> +APM X-Gene SoC GPIO controller bindings
> +
> +This is a gpio controller that is part of the flash controller.
> +All registers are 32bit and in little endian format.
> +
> +Required properties:
> +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller
> +- reg: Physical base address and length of the controller's registers
> +
> +The gpio controller has multiple banks, each bank will have 16 or less
> +gpio pins. All cfg field will be little endian and least significant
> +bit indicate lowest number of gpio.
> +
> +Required properties:
> +- #gpio-cells: Should be two.
> + - first cell is the pin number
> + - second cell is used to specify optional parameters (unused)
> +- gpio-controller: Marks the device node as a GPIO controller.
Shouldnt these properties be part of the parent instead of being
repeated for each bank?
> +- bank: This is to specifiy which gpio bank the dts node is describing.
I suspect you could use the reg property for this, as it seems like
you would need it for DT compliance anyway (but I'm not 100% sure
here).
> +
> +Optional properties:
> +- ngpio: Specify the number of GPIOs per bank. It is defaulted to 16
> + if not specified. Number of gpio can't be greater than 16 or
> + 0.
> +- odcfg: This is gpio open drain/normal configuration. It is a 16 bit
> + field, setting 1 will mean the pin is set in open drain
> + configuration, 0 will mean normal configuration. Default will
> + be normal configuration.
> +
> +Example:
> + gpio: gpio at 1701c000 {
> + compatible = "apm,xgene-gpio";
> + reg = <0x0 0x1701c000 0x0 0x1000>;
> +
> + banka: gpio-controller at 0 {
> + compatible = "apm,xgene-gpio-port";
This property is not documented. Besides I don't see it anywhere in
your driver implementation so I would say it is probably not needed at
all.
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <0>;
> + odcfg = <0xffff>; /* Optional */
> + };
> + bankb: gpio-controller at 1 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <1>;
> + };
> + bankc: gpio-controller at 2 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <2>;
> + };
> + };
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-20 6:04 ` Alexandre Courbot
@ 2014-05-20 23:54 ` Feng Kan
-1 siblings, 0 replies; 24+ messages in thread
From: Feng Kan @ 2014-05-20 23:54 UTC (permalink / raw
To: Alexandre Courbot
Cc: devicetree@vger.kernel.org, Catalin Marinas, Linus Walleij,
patches, Will Deacon, linux-gpio@vger.kernel.org,
linux-arm-kernel@lists.infradead.org
> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
>Couldn't you use set_bit() / clear_bit() instead here?
Set_bit/clear_bit seems a bit heavy, I can use the non atomic methods
of _set_bit/clear_bit
if you agree? In which case I will rewrite all the other method in
likewise fashion.
>> + bank = &gpio->banks[offs];
>> + bank->gpio = gpio;
>> + spin_lock_init(&bank->lock);
>> +
>> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
>> + ngpio = 16;
>
> Here I think you want to distinguish between "property not defined"
> (-EINVAL) and "property has no value" (-ENODATA) and other possible
> errors. In the later cases you will want to signal the error and
> return from here.
I kinda messed up here a bit, I am trying to support ACPI booting but other
properties are also affected. I will rewrite this part so minimum of_ access
methods are required.
>
>> +
>> + if ((ngpio > 16) || (ngpio < 1)) {
>> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
>> + bank_np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
>> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
>> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
>> + bank->set_dr = gpio->regs + GPIO_SET_DR
>> + + (bank_idx * GPIO_SET_DR_OFFSET);
>> +
>> + bank->chip.direction_input = xgene_gpio_dir_in;
>> + bank->chip.direction_output = xgene_gpio_dir_out;
>> + bank->chip.get = xgene_gpio_get;
>> + bank->chip.set = xgene_gpio_set;
>> +
>> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
>> + iowrite32(odcfg, bank->od);
>> +
>> + bank->chip.ngpio = ngpio;
>> + bank->chip.of_node = bank_np;
>> + bank->chip.base = bank_idx * ngpio;
>> +
>> + err = gpiochip_add(&bank->chip);
>> + if (err)
>> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>> + bank_np->full_name);
>> +
>> + return err;
>> +}
>> +
>> +static int xgene_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct resource *res;
>> + struct xgene_gpio *gpio;
>> + int err;
>> + unsigned int offs = 0;
>> +
>> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>> + if (!gpio)
>> + return -ENOMEM;
>> + gpio->dev = &pdev->dev;
>> +
>> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
>> + if (!gpio->nr_banks) {
>> + err = -EINVAL;
>> + goto err;
>> + }
>
> Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
> return an error here.
>
>> +
>> + gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
>> + sizeof(*gpio->banks), GFP_KERNEL);
>> + if (!gpio->banks) {
>> + err = -ENOMEM;
>> + goto err;
>> + }
>
> I'm fine with printing the error status the way you do, but could you
> also do the same for the first kzalloc too?
>
> Or maybe you could put your banks member at the end of the xgene_gpio
> struct as a flexible array member and perform only one kzalloc of size
> (sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?
>
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(gpio->regs)) {
>> + err = PTR_ERR(gpio->regs);
>> + goto err;
>> + }
>> +
>> + for_each_child_of_node(pdev->dev.of_node, np) {
>> + err = xgene_gpio_add_bank(gpio, np, offs++);
>> + if (err)
>> + goto err;
>> + }
>> +
>> + platform_set_drvdata(pdev, gpio);
>> +
>> + dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
>> + return 0;
>> +err:
>> + dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
>> + return err;
>> +}
>> +
>> +static const struct of_device_id xgene_gpio_of_match[] = {
>> + { .compatible = "apm,xgene-gpio", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
>> +
>> +static struct platform_driver xgene_gpio_driver = {
>> + .driver = {
>> + .name = "xgene-gpio",
>> + .owner = THIS_MODULE,
>> + .of_match_table = xgene_gpio_of_match,
>> + },
>> + .probe = xgene_gpio_probe,
>> +};
>> +
>> +static int __init xgene_gpio_init(void)
>> +{
>> + return platform_driver_register(&xgene_gpio_driver);
>> +}
>> +postcore_initcall(xgene_gpio_init);
>> +
>> +MODULE_AUTHOR("AppliedMicro");
>> +MODULE_DESCRIPTION("APM X-Gene GPIO driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 1.9.1
>>
>
> Globally the driver looks well-written, but I don't understand the
> need to have one gpio_chip per bank. Could you elaborate on the
> reasons for this design?
I see each bank as separate gpio chip. It is a simple way to
abstract the banks since they can operate independently. It also
provided me a way to fix the sysfs gpio base number, regardless if
a particular bank node is pulled out. This is also done in similar way
in some other gpio drivers such as the dwapb gpio driver.
>
> Alex.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-20 23:54 ` Feng Kan
0 siblings, 0 replies; 24+ messages in thread
From: Feng Kan @ 2014-05-20 23:54 UTC (permalink / raw
To: linux-arm-kernel
> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
>Couldn't you use set_bit() / clear_bit() instead here?
Set_bit/clear_bit seems a bit heavy, I can use the non atomic methods
of _set_bit/clear_bit
if you agree? In which case I will rewrite all the other method in
likewise fashion.
>> + bank = &gpio->banks[offs];
>> + bank->gpio = gpio;
>> + spin_lock_init(&bank->lock);
>> +
>> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
>> + ngpio = 16;
>
> Here I think you want to distinguish between "property not defined"
> (-EINVAL) and "property has no value" (-ENODATA) and other possible
> errors. In the later cases you will want to signal the error and
> return from here.
I kinda messed up here a bit, I am trying to support ACPI booting but other
properties are also affected. I will rewrite this part so minimum of_ access
methods are required.
>
>> +
>> + if ((ngpio > 16) || (ngpio < 1)) {
>> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
>> + bank_np->full_name);
>> + return -EINVAL;
>> + }
>> +
>> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
>> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
>> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
>> + bank->set_dr = gpio->regs + GPIO_SET_DR
>> + + (bank_idx * GPIO_SET_DR_OFFSET);
>> +
>> + bank->chip.direction_input = xgene_gpio_dir_in;
>> + bank->chip.direction_output = xgene_gpio_dir_out;
>> + bank->chip.get = xgene_gpio_get;
>> + bank->chip.set = xgene_gpio_set;
>> +
>> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
>> + iowrite32(odcfg, bank->od);
>> +
>> + bank->chip.ngpio = ngpio;
>> + bank->chip.of_node = bank_np;
>> + bank->chip.base = bank_idx * ngpio;
>> +
>> + err = gpiochip_add(&bank->chip);
>> + if (err)
>> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>> + bank_np->full_name);
>> +
>> + return err;
>> +}
>> +
>> +static int xgene_gpio_probe(struct platform_device *pdev)
>> +{
>> + struct device_node *np = pdev->dev.of_node;
>> + struct resource *res;
>> + struct xgene_gpio *gpio;
>> + int err;
>> + unsigned int offs = 0;
>> +
>> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>> + if (!gpio)
>> + return -ENOMEM;
>> + gpio->dev = &pdev->dev;
>> +
>> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
>> + if (!gpio->nr_banks) {
>> + err = -EINVAL;
>> + goto err;
>> + }
>
> Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
> return an error here.
>
>> +
>> + gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
>> + sizeof(*gpio->banks), GFP_KERNEL);
>> + if (!gpio->banks) {
>> + err = -ENOMEM;
>> + goto err;
>> + }
>
> I'm fine with printing the error status the way you do, but could you
> also do the same for the first kzalloc too?
>
> Or maybe you could put your banks member at the end of the xgene_gpio
> struct as a flexible array member and perform only one kzalloc of size
> (sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?
>
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
>> + if (IS_ERR(gpio->regs)) {
>> + err = PTR_ERR(gpio->regs);
>> + goto err;
>> + }
>> +
>> + for_each_child_of_node(pdev->dev.of_node, np) {
>> + err = xgene_gpio_add_bank(gpio, np, offs++);
>> + if (err)
>> + goto err;
>> + }
>> +
>> + platform_set_drvdata(pdev, gpio);
>> +
>> + dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
>> + return 0;
>> +err:
>> + dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
>> + return err;
>> +}
>> +
>> +static const struct of_device_id xgene_gpio_of_match[] = {
>> + { .compatible = "apm,xgene-gpio", },
>> + {},
>> +};
>> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
>> +
>> +static struct platform_driver xgene_gpio_driver = {
>> + .driver = {
>> + .name = "xgene-gpio",
>> + .owner = THIS_MODULE,
>> + .of_match_table = xgene_gpio_of_match,
>> + },
>> + .probe = xgene_gpio_probe,
>> +};
>> +
>> +static int __init xgene_gpio_init(void)
>> +{
>> + return platform_driver_register(&xgene_gpio_driver);
>> +}
>> +postcore_initcall(xgene_gpio_init);
>> +
>> +MODULE_AUTHOR("AppliedMicro");
>> +MODULE_DESCRIPTION("APM X-Gene GPIO driver");
>> +MODULE_LICENSE("GPL");
>> --
>> 1.9.1
>>
>
> Globally the driver looks well-written, but I don't understand the
> need to have one gpio_chip per bank. Could you elaborate on the
> reasons for this design?
I see each bank as separate gpio chip. It is a simple way to
abstract the banks since they can operate independently. It also
provided me a way to fix the sysfs gpio base number, regardless if
a particular bank node is pulled out. This is also done in similar way
in some other gpio drivers such as the dwapb gpio driver.
>
> Alex.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-20 23:54 ` Feng Kan
@ 2014-05-25 7:35 ` Alexandre Courbot
-1 siblings, 0 replies; 24+ messages in thread
From: Alexandre Courbot @ 2014-05-25 7:35 UTC (permalink / raw
To: Feng Kan
Cc: patches, Linus Walleij, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Wed, May 21, 2014 at 8:54 AM, Feng Kan <fkan@apm.com> wrote:
>> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>> +{
>> + struct xgene_gpio_bank *bank =
>> + container_of(gc, struct xgene_gpio_bank, chip);
>> + unsigned long flags;
>> + u32 data;
>> +
>> + spin_lock_irqsave(&bank->lock, flags);
>> +
>> + data = ioread32(bank->set_dr);
>> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
>
>>Couldn't you use set_bit() / clear_bit() instead here?
> Set_bit/clear_bit seems a bit heavy, I can use the non atomic methods
> of _set_bit/clear_bit
> if you agree? In which case I will rewrite all the other method in
> likewise fashion.
Sounds good.
>
>
>>> + bank = &gpio->banks[offs];
>>> + bank->gpio = gpio;
>>> + spin_lock_init(&bank->lock);
>>> +
>>> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
>>> + ngpio = 16;
>>
>> Here I think you want to distinguish between "property not defined"
>> (-EINVAL) and "property has no value" (-ENODATA) and other possible
>> errors. In the later cases you will want to signal the error and
>> return from here.
>
> I kinda messed up here a bit, I am trying to support ACPI booting but other
> properties are also affected. I will rewrite this part so minimum of_ access
> methods are required.
>
>
>>
>>> +
>>> + if ((ngpio > 16) || (ngpio < 1)) {
>>> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
>>> + bank_np->full_name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
>>> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
>>> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
>>> + bank->set_dr = gpio->regs + GPIO_SET_DR
>>> + + (bank_idx * GPIO_SET_DR_OFFSET);
>>> +
>>> + bank->chip.direction_input = xgene_gpio_dir_in;
>>> + bank->chip.direction_output = xgene_gpio_dir_out;
>>> + bank->chip.get = xgene_gpio_get;
>>> + bank->chip.set = xgene_gpio_set;
>>> +
>>> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
>>> + iowrite32(odcfg, bank->od);
>>> +
>>> + bank->chip.ngpio = ngpio;
>>> + bank->chip.of_node = bank_np;
>>> + bank->chip.base = bank_idx * ngpio;
>>> +
>>> + err = gpiochip_add(&bank->chip);
>>> + if (err)
>>> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>>> + bank_np->full_name);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int xgene_gpio_probe(struct platform_device *pdev)
>>> +{
>>> + struct device_node *np = pdev->dev.of_node;
>>> + struct resource *res;
>>> + struct xgene_gpio *gpio;
>>> + int err;
>>> + unsigned int offs = 0;
>>> +
>>> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>>> + if (!gpio)
>>> + return -ENOMEM;
>>> + gpio->dev = &pdev->dev;
>>> +
>>> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
>>> + if (!gpio->nr_banks) {
>>> + err = -EINVAL;
>>> + goto err;
>>> + }
>>
>> Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
>> return an error here.
>>
>>> +
>>> + gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
>>> + sizeof(*gpio->banks), GFP_KERNEL);
>>> + if (!gpio->banks) {
>>> + err = -ENOMEM;
>>> + goto err;
>>> + }
>>
>> I'm fine with printing the error status the way you do, but could you
>> also do the same for the first kzalloc too?
>>
>> Or maybe you could put your banks member at the end of the xgene_gpio
>> struct as a flexible array member and perform only one kzalloc of size
>> (sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?
>>
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(gpio->regs)) {
>>> + err = PTR_ERR(gpio->regs);
>>> + goto err;
>>> + }
>>> +
>>> + for_each_child_of_node(pdev->dev.of_node, np) {
>>> + err = xgene_gpio_add_bank(gpio, np, offs++);
>>> + if (err)
>>> + goto err;
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, gpio);
>>> +
>>> + dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
>>> + return 0;
>>> +err:
>>> + dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
>>> + return err;
>>> +}
>>> +
>>> +static const struct of_device_id xgene_gpio_of_match[] = {
>>> + { .compatible = "apm,xgene-gpio", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
>>> +
>>> +static struct platform_driver xgene_gpio_driver = {
>>> + .driver = {
>>> + .name = "xgene-gpio",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = xgene_gpio_of_match,
>>> + },
>>> + .probe = xgene_gpio_probe,
>>> +};
>>> +
>>> +static int __init xgene_gpio_init(void)
>>> +{
>>> + return platform_driver_register(&xgene_gpio_driver);
>>> +}
>>> +postcore_initcall(xgene_gpio_init);
>>> +
>>> +MODULE_AUTHOR("AppliedMicro");
>>> +MODULE_DESCRIPTION("APM X-Gene GPIO driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 1.9.1
>>>
>>
>> Globally the driver looks well-written, but I don't understand the
>> need to have one gpio_chip per bank. Could you elaborate on the
>> reasons for this design?
> I see each bank as separate gpio chip. It is a simple way to
> abstract the banks since they can operate independently. It also
> provided me a way to fix the sysfs gpio base number, regardless if
> a particular bank node is pulled out. This is also done in similar way
> in some other gpio drivers such as the dwapb gpio driver.
I'd like to see what Linus thinks about this, but if there is
precedence, and the banks can work independently, then I agree it may
make sense to have it that way. In that case you should probably have
each bank appearing as its own, independent node in the device tree,
and be probed directly by this driver instead of going through a
higher-level device which only practical use is to provide the number
of banks in your GPIO chip. I.e. if your bank are independent GPIO
devices, let the driver also reflect that fact.
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-25 7:35 ` Alexandre Courbot
0 siblings, 0 replies; 24+ messages in thread
From: Alexandre Courbot @ 2014-05-25 7:35 UTC (permalink / raw
To: linux-arm-kernel
On Wed, May 21, 2014 at 8:54 AM, Feng Kan <fkan@apm.com> wrote:
>> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
>> +{
>> + struct xgene_gpio_bank *bank =
>> + container_of(gc, struct xgene_gpio_bank, chip);
>> + unsigned long flags;
>> + u32 data;
>> +
>> + spin_lock_irqsave(&bank->lock, flags);
>> +
>> + data = ioread32(bank->set_dr);
>> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
>
>>Couldn't you use set_bit() / clear_bit() instead here?
> Set_bit/clear_bit seems a bit heavy, I can use the non atomic methods
> of _set_bit/clear_bit
> if you agree? In which case I will rewrite all the other method in
> likewise fashion.
Sounds good.
>
>
>>> + bank = &gpio->banks[offs];
>>> + bank->gpio = gpio;
>>> + spin_lock_init(&bank->lock);
>>> +
>>> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
>>> + ngpio = 16;
>>
>> Here I think you want to distinguish between "property not defined"
>> (-EINVAL) and "property has no value" (-ENODATA) and other possible
>> errors. In the later cases you will want to signal the error and
>> return from here.
>
> I kinda messed up here a bit, I am trying to support ACPI booting but other
> properties are also affected. I will rewrite this part so minimum of_ access
> methods are required.
>
>
>>
>>> +
>>> + if ((ngpio > 16) || (ngpio < 1)) {
>>> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
>>> + bank_np->full_name);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
>>> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
>>> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
>>> + bank->set_dr = gpio->regs + GPIO_SET_DR
>>> + + (bank_idx * GPIO_SET_DR_OFFSET);
>>> +
>>> + bank->chip.direction_input = xgene_gpio_dir_in;
>>> + bank->chip.direction_output = xgene_gpio_dir_out;
>>> + bank->chip.get = xgene_gpio_get;
>>> + bank->chip.set = xgene_gpio_set;
>>> +
>>> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
>>> + iowrite32(odcfg, bank->od);
>>> +
>>> + bank->chip.ngpio = ngpio;
>>> + bank->chip.of_node = bank_np;
>>> + bank->chip.base = bank_idx * ngpio;
>>> +
>>> + err = gpiochip_add(&bank->chip);
>>> + if (err)
>>> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
>>> + bank_np->full_name);
>>> +
>>> + return err;
>>> +}
>>> +
>>> +static int xgene_gpio_probe(struct platform_device *pdev)
>>> +{
>>> + struct device_node *np = pdev->dev.of_node;
>>> + struct resource *res;
>>> + struct xgene_gpio *gpio;
>>> + int err;
>>> + unsigned int offs = 0;
>>> +
>>> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
>>> + if (!gpio)
>>> + return -ENOMEM;
>>> + gpio->dev = &pdev->dev;
>>> +
>>> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
>>> + if (!gpio->nr_banks) {
>>> + err = -EINVAL;
>>> + goto err;
>>> + }
>>
>> Might be worth checking whether nr_banks >= XGENE_GPIO_MAX_PORTS and
>> return an error here.
>>
>>> +
>>> + gpio->banks = devm_kzalloc(&pdev->dev, gpio->nr_banks *
>>> + sizeof(*gpio->banks), GFP_KERNEL);
>>> + if (!gpio->banks) {
>>> + err = -ENOMEM;
>>> + goto err;
>>> + }
>>
>> I'm fine with printing the error status the way you do, but could you
>> also do the same for the first kzalloc too?
>>
>> Or maybe you could put your banks member at the end of the xgene_gpio
>> struct as a flexible array member and perform only one kzalloc of size
>> (sizeof(*gpio) + (sizeof(*gpio->banks) * gpio->nr_banks))?
>>
>>> +
>>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> + gpio->regs = devm_ioremap_resource(&pdev->dev, res);
>>> + if (IS_ERR(gpio->regs)) {
>>> + err = PTR_ERR(gpio->regs);
>>> + goto err;
>>> + }
>>> +
>>> + for_each_child_of_node(pdev->dev.of_node, np) {
>>> + err = xgene_gpio_add_bank(gpio, np, offs++);
>>> + if (err)
>>> + goto err;
>>> + }
>>> +
>>> + platform_set_drvdata(pdev, gpio);
>>> +
>>> + dev_info(&pdev->dev, "X-Gene GPIO driver registered\n");
>>> + return 0;
>>> +err:
>>> + dev_err(&pdev->dev, "X-Gene GPIO driver registration failed\n");
>>> + return err;
>>> +}
>>> +
>>> +static const struct of_device_id xgene_gpio_of_match[] = {
>>> + { .compatible = "apm,xgene-gpio", },
>>> + {},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
>>> +
>>> +static struct platform_driver xgene_gpio_driver = {
>>> + .driver = {
>>> + .name = "xgene-gpio",
>>> + .owner = THIS_MODULE,
>>> + .of_match_table = xgene_gpio_of_match,
>>> + },
>>> + .probe = xgene_gpio_probe,
>>> +};
>>> +
>>> +static int __init xgene_gpio_init(void)
>>> +{
>>> + return platform_driver_register(&xgene_gpio_driver);
>>> +}
>>> +postcore_initcall(xgene_gpio_init);
>>> +
>>> +MODULE_AUTHOR("AppliedMicro");
>>> +MODULE_DESCRIPTION("APM X-Gene GPIO driver");
>>> +MODULE_LICENSE("GPL");
>>> --
>>> 1.9.1
>>>
>>
>> Globally the driver looks well-written, but I don't understand the
>> need to have one gpio_chip per bank. Could you elaborate on the
>> reasons for this design?
> I see each bank as separate gpio chip. It is a simple way to
> abstract the banks since they can operate independently. It also
> provided me a way to fix the sysfs gpio base number, regardless if
> a particular bank node is pulled out. This is also done in similar way
> in some other gpio drivers such as the dwapb gpio driver.
I'd like to see what Linus thinks about this, but if there is
precedence, and the banks can work independently, then I agree it may
make sense to have it that way. In that case you should probably have
each bank appearing as its own, independent node in the device tree,
and be probed directly by this driver instead of going through a
higher-level device which only practical use is to provide the number
of banks in your GPIO chip. I.e. if your bank are independent GPIO
devices, let the driver also reflect that fact.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
[not found] ` <1400263432-922-2-git-send-email-fkan@apm.com>
@ 2014-05-27 7:59 ` Linus Walleij
2014-05-27 7:59 ` Linus Walleij
1 sibling, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 7:59 UTC (permalink / raw
To: Feng Kan
Cc: patches, Alexandre Courbot, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Fri, May 16, 2014 at 8:03 PM, Feng Kan <fkan@apm.com> wrote:
> Add APM X-Gene SoC gpio controller driver.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
That's a very terse commit message for an entirely new driver. This needs
to be descriptive, mostly the same as in the Kconfig entry, but also some
info on the target platform and so on.
> +config GPIO_XGENE
> + bool "APM X-Gene GPIO controller support"
> + depends on ARM64 && OF_GPIO
> + help
> + This driver is to support the GPIO block within the APM X-Gene
> + platform's generic flash controller. The GPIO pins are muxed with
> + the generic flash controller's address and data pins. Say yes
> + here to enable the GFC GPIO functionality.
Stop it right there. If there is pin muxing involved:
- The driver definately goes into drivers/pinctrl
- You need to read Documentation/pinctrl.txt
- Implement the proper muxing interface and select CONFIG_PINMUX
apart from CONFIG_PINCTRL
> +#define GPIO_MASK(x) (1U << ((x) % 32))
I think the %32 modifier is unnecessary, see below.
> +#define GPIO_MUX_SEL(x) (3U << ((x * 2) % 32))
Yeah that is a pin multiplexing register all right.
Instead of these scary macros, write static inline functions in C.
> +#define GPIO_SET_MASK(x) (1U << ((x + 16) % 32))
This should be a static inline or similar as well, and I think the
%32 modifier is unnecessary, see below.
> +
> +#define GPIO_SET_DR 0x0c
> +#define GPIO_SET_DR_OFFSET 0x0c
> +#define GPIO_DATA 0x14
> +#define GPIO_DATA_OFFSET 0x0c
> +#define GPIO_OD 0x30
> +#define GPIO_OD_OFFSET 0x04
> +#define GPIO_MUX 0x10
> +#define GPIO_MUX_OFFSET 0x0c
And that is pinmux too.
> +struct xgene_gpio_bank {
> + struct gpio_chip chip;
> + void __iomem *data;
> + void __iomem *set_dr;
> + void __iomem *od;
> + void __iomem *mux;
Instead of putting a pointer to every register, store a base pointer
and make all writes relative to that pointer.
ioread32(xgene_bank->base + offset)... etc
> + struct xgene_gpio *gpio;
> + spinlock_t lock;
> +};
> +
> +struct xgene_gpio {
> + struct device *dev;
> + void __iomem *regs;
> + struct xgene_gpio_bank *banks;
> + unsigned int nr_banks;
> +};
If you're instantiating one struct xgene_gpio_bank and that
contains the struct gpio_chip why can you not register one
device per chip too? That is usually much better. And you don't
need two different state container structs like this.
If the registers are mingled in the address space I can understand
this layout, but if they are separated then there is no point in
complicating things like this.
> +static int xgene_gpio_get(struct gpio_chip *gc, unsigned int gpio)
Rename "gpio" to "offset" as this shall be local to the current
gpio_chip offset from 0.
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
Break out all these calls to static inline function like this:
static inline struct xgene_gpio_bank *to_xgene_bank(struct gpio_chip *gc)
{
return container_of(gc, struct xgene_gpio_bank, chip);
}
So it becomes:
struct xgene_gpio_bank *bank = to_xgene_bank(gc);
> +
> + return (ioread32(bank->data) & GPIO_MASK(gpio));
> +}
This GPIO_MASK() seems completely surplus. As the "gpio" (that I
say shall be renamed "offset") is local to the chip and will be in
the range 0..31.
You also should clamp the returned value to [0,1].
Just do this:
#include <linux/bitops.h>
return !!(ioread32(bank->data) & BIT(offset));
> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
Rename gpio->offset
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
Call it "val" rather than "data". Data is usually used for void * pointers.
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
That was complicated to read. Just do this so it's readable:
if (val)
data |= gpio_set_mask(offset);
else
data &= ~gpio_set_mask(offset);
> + iowrite32(data, bank->set_dr);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +static void xgene_gpio_muxcfg(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + u32 data;
> +
> + data = ioread32(bank->mux);
> + data |= GPIO_MUX_SEL(gpio);
> + iowrite32(data, bank->mux);
> +}
You're simply not allowed to do this kind of stuff in a GPIO driver.
This should be part of the pinctrl interface, and to mux in the GPIO
function of a pin from the GPIO side of the world you use
pinctrl_request_gpio()
pinctrl_free_gpio()
pinctrl_gpio_direction_input()
pinctrl_gpio_direction_output()
After first registering a GPIO range for the gpio_chip in the
probe() function using gpiochip_add_pin_range() right after
registering firs the pin controller and then the gpio_chip.
> +static int xgene_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
Rename gpio->offset
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data |= GPIO_MASK(gpio);
Probably just data |= BIT(offset);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
> +
> +static int xgene_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> + data = ioread32(bank->set_dr);
> + data = data & ~GPIO_MASK(gpio);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
Much the same comments here.
> +static int xgene_gpio_add_bank(struct xgene_gpio *gpio,
> + struct device_node *bank_np,
> + unsigned int offs)
> +{
> + struct xgene_gpio_bank *bank;
> + u32 bank_idx, ngpio, odcfg;
> + int err;
> +
> + if (of_property_read_u32(bank_np, "bank", &bank_idx) ||
> + bank_idx >= XGENE_GPIO_MAX_PORTS) {
> + dev_err(gpio->dev, "missing/invalid bank index for %s index %d\n",
> + bank_np->full_name, bank_idx);
> + return -EINVAL;
> + }
I strongly question the usefulness of sub-"banks" here. You should try
to have one DT entry per "bank" and let each one be a device on its
own.
> + bank = &gpio->banks[offs];
> + bank->gpio = gpio;
> + spin_lock_init(&bank->lock);
> +
> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
> + ngpio = 16;
Does this really vary, or will it always be 16 in all device trees?
Then there is no point in having it in the device tree...
> + if ((ngpio > 16) || (ngpio < 1)) {
> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
> + bank_np->full_name);
> + return -EINVAL;
> + }
Like is this always 16?
> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
> + bank->set_dr = gpio->regs + GPIO_SET_DR
> + + (bank_idx * GPIO_SET_DR_OFFSET);
As I said: don't do this. Save a base pointer in the state container
and make all read/writes relative to the base.
> + bank->chip.direction_input = xgene_gpio_dir_in;
> + bank->chip.direction_output = xgene_gpio_dir_out;
> + bank->chip.get = xgene_gpio_get;
> + bank->chip.set = xgene_gpio_set;
> +
> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
> + iowrite32(odcfg, bank->od);
> +
> + bank->chip.ngpio = ngpio;
> + bank->chip.of_node = bank_np;
> + bank->chip.base = bank_idx * ngpio;
Also set up chip.dev and chip.name etc.
> + err = gpiochip_add(&bank->chip);
> + if (err)
> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
> + bank_np->full_name);
Right after here you should register a gpio pin range.
> + return err;
> +}
> +
> +static int xgene_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + struct xgene_gpio *gpio;
> + int err;
> + unsigned int offs = 0;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> + gpio->dev = &pdev->dev;
> +
> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
> + if (!gpio->nr_banks) {
> + err = -EINVAL;
> + goto err;
> + }
This is too complicated. Use one device per bank and split up
the register range.
(...)
> +static const struct of_device_id xgene_gpio_of_match[] = {
> + { .compatible = "apm,xgene-gpio", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
> +
> +static struct platform_driver xgene_gpio_driver = {
> + .driver = {
> + .name = "xgene-gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = xgene_gpio_of_match,
> + },
> + .probe = xgene_gpio_probe,
> +};
> +
> +static int __init xgene_gpio_init(void)
> +{
> + return platform_driver_register(&xgene_gpio_driver);
> +}
> +postcore_initcall(xgene_gpio_init);
Why does this have to register so early? Use a simple module
driver and module_platform_driver() please.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-27 7:59 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 7:59 UTC (permalink / raw
To: linux-arm-kernel
On Fri, May 16, 2014 at 8:03 PM, Feng Kan <fkan@apm.com> wrote:
> Add APM X-Gene SoC gpio controller driver.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
That's a very terse commit message for an entirely new driver. This needs
to be descriptive, mostly the same as in the Kconfig entry, but also some
info on the target platform and so on.
> +config GPIO_XGENE
> + bool "APM X-Gene GPIO controller support"
> + depends on ARM64 && OF_GPIO
> + help
> + This driver is to support the GPIO block within the APM X-Gene
> + platform's generic flash controller. The GPIO pins are muxed with
> + the generic flash controller's address and data pins. Say yes
> + here to enable the GFC GPIO functionality.
Stop it right there. If there is pin muxing involved:
- The driver definately goes into drivers/pinctrl
- You need to read Documentation/pinctrl.txt
- Implement the proper muxing interface and select CONFIG_PINMUX
apart from CONFIG_PINCTRL
> +#define GPIO_MASK(x) (1U << ((x) % 32))
I think the %32 modifier is unnecessary, see below.
> +#define GPIO_MUX_SEL(x) (3U << ((x * 2) % 32))
Yeah that is a pin multiplexing register all right.
Instead of these scary macros, write static inline functions in C.
> +#define GPIO_SET_MASK(x) (1U << ((x + 16) % 32))
This should be a static inline or similar as well, and I think the
%32 modifier is unnecessary, see below.
> +
> +#define GPIO_SET_DR 0x0c
> +#define GPIO_SET_DR_OFFSET 0x0c
> +#define GPIO_DATA 0x14
> +#define GPIO_DATA_OFFSET 0x0c
> +#define GPIO_OD 0x30
> +#define GPIO_OD_OFFSET 0x04
> +#define GPIO_MUX 0x10
> +#define GPIO_MUX_OFFSET 0x0c
And that is pinmux too.
> +struct xgene_gpio_bank {
> + struct gpio_chip chip;
> + void __iomem *data;
> + void __iomem *set_dr;
> + void __iomem *od;
> + void __iomem *mux;
Instead of putting a pointer to every register, store a base pointer
and make all writes relative to that pointer.
ioread32(xgene_bank->base + offset)... etc
> + struct xgene_gpio *gpio;
> + spinlock_t lock;
> +};
> +
> +struct xgene_gpio {
> + struct device *dev;
> + void __iomem *regs;
> + struct xgene_gpio_bank *banks;
> + unsigned int nr_banks;
> +};
If you're instantiating one struct xgene_gpio_bank and that
contains the struct gpio_chip why can you not register one
device per chip too? That is usually much better. And you don't
need two different state container structs like this.
If the registers are mingled in the address space I can understand
this layout, but if they are separated then there is no point in
complicating things like this.
> +static int xgene_gpio_get(struct gpio_chip *gc, unsigned int gpio)
Rename "gpio" to "offset" as this shall be local to the current
gpio_chip offset from 0.
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
Break out all these calls to static inline function like this:
static inline struct xgene_gpio_bank *to_xgene_bank(struct gpio_chip *gc)
{
return container_of(gc, struct xgene_gpio_bank, chip);
}
So it becomes:
struct xgene_gpio_bank *bank = to_xgene_bank(gc);
> +
> + return (ioread32(bank->data) & GPIO_MASK(gpio));
> +}
This GPIO_MASK() seems completely surplus. As the "gpio" (that I
say shall be renamed "offset") is local to the chip and will be in
the range 0..31.
You also should clamp the returned value to [0,1].
Just do this:
#include <linux/bitops.h>
return !!(ioread32(bank->data) & BIT(offset));
> +static void xgene_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)
Rename gpio->offset
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
Call it "val" rather than "data". Data is usually used for void * pointers.
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data = val ? data | GPIO_SET_MASK(gpio) : data & ~GPIO_SET_MASK(gpio);
That was complicated to read. Just do this so it's readable:
if (val)
data |= gpio_set_mask(offset);
else
data &= ~gpio_set_mask(offset);
> + iowrite32(data, bank->set_dr);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +}
> +
> +static void xgene_gpio_muxcfg(struct gpio_chip *gc, unsigned int gpio)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + u32 data;
> +
> + data = ioread32(bank->mux);
> + data |= GPIO_MUX_SEL(gpio);
> + iowrite32(data, bank->mux);
> +}
You're simply not allowed to do this kind of stuff in a GPIO driver.
This should be part of the pinctrl interface, and to mux in the GPIO
function of a pin from the GPIO side of the world you use
pinctrl_request_gpio()
pinctrl_free_gpio()
pinctrl_gpio_direction_input()
pinctrl_gpio_direction_output()
After first registering a GPIO range for the gpio_chip in the
probe() function using gpiochip_add_pin_range() right after
registering firs the pin controller and then the gpio_chip.
> +static int xgene_gpio_dir_in(struct gpio_chip *gc, unsigned int gpio)
Rename gpio->offset
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> +
> + data = ioread32(bank->set_dr);
> + data |= GPIO_MASK(gpio);
Probably just data |= BIT(offset);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
> +
> +static int xgene_gpio_dir_out(struct gpio_chip *gc, unsigned int gpio, int val)
> +{
> + struct xgene_gpio_bank *bank =
> + container_of(gc, struct xgene_gpio_bank, chip);
> + unsigned long flags;
> + u32 data;
> +
> + spin_lock_irqsave(&bank->lock, flags);
> + data = ioread32(bank->set_dr);
> + data = data & ~GPIO_MASK(gpio);
> + iowrite32(data, bank->set_dr);
> + xgene_gpio_muxcfg(gc, gpio);
> +
> + spin_unlock_irqrestore(&bank->lock, flags);
> +
> + return 0;
> +}
Much the same comments here.
> +static int xgene_gpio_add_bank(struct xgene_gpio *gpio,
> + struct device_node *bank_np,
> + unsigned int offs)
> +{
> + struct xgene_gpio_bank *bank;
> + u32 bank_idx, ngpio, odcfg;
> + int err;
> +
> + if (of_property_read_u32(bank_np, "bank", &bank_idx) ||
> + bank_idx >= XGENE_GPIO_MAX_PORTS) {
> + dev_err(gpio->dev, "missing/invalid bank index for %s index %d\n",
> + bank_np->full_name, bank_idx);
> + return -EINVAL;
> + }
I strongly question the usefulness of sub-"banks" here. You should try
to have one DT entry per "bank" and let each one be a device on its
own.
> + bank = &gpio->banks[offs];
> + bank->gpio = gpio;
> + spin_lock_init(&bank->lock);
> +
> + if (of_property_read_u32(bank_np, "ngpios", &ngpio))
> + ngpio = 16;
Does this really vary, or will it always be 16 in all device trees?
Then there is no point in having it in the device tree...
> + if ((ngpio > 16) || (ngpio < 1)) {
> + dev_info(gpio->dev, "Incorrect number of gpio specified: %s\n",
> + bank_np->full_name);
> + return -EINVAL;
> + }
Like is this always 16?
> + bank->data = gpio->regs + GPIO_DATA + (bank_idx * GPIO_DATA_OFFSET);
> + bank->od = gpio->regs + GPIO_OD + (bank_idx * GPIO_OD_OFFSET);
> + bank->mux = gpio->regs + GPIO_MUX + (bank_idx * GPIO_MUX_OFFSET);
> + bank->set_dr = gpio->regs + GPIO_SET_DR
> + + (bank_idx * GPIO_SET_DR_OFFSET);
As I said: don't do this. Save a base pointer in the state container
and make all read/writes relative to the base.
> + bank->chip.direction_input = xgene_gpio_dir_in;
> + bank->chip.direction_output = xgene_gpio_dir_out;
> + bank->chip.get = xgene_gpio_get;
> + bank->chip.set = xgene_gpio_set;
> +
> + if (!of_property_read_u32(bank_np, "odcfg", &odcfg))
> + iowrite32(odcfg, bank->od);
> +
> + bank->chip.ngpio = ngpio;
> + bank->chip.of_node = bank_np;
> + bank->chip.base = bank_idx * ngpio;
Also set up chip.dev and chip.name etc.
> + err = gpiochip_add(&bank->chip);
> + if (err)
> + dev_err(gpio->dev, "failed to register gpiochip for %s\n",
> + bank_np->full_name);
Right after here you should register a gpio pin range.
> + return err;
> +}
> +
> +static int xgene_gpio_probe(struct platform_device *pdev)
> +{
> + struct device_node *np = pdev->dev.of_node;
> + struct resource *res;
> + struct xgene_gpio *gpio;
> + int err;
> + unsigned int offs = 0;
> +
> + gpio = devm_kzalloc(&pdev->dev, sizeof(*gpio), GFP_KERNEL);
> + if (!gpio)
> + return -ENOMEM;
> + gpio->dev = &pdev->dev;
> +
> + gpio->nr_banks = of_get_child_count(pdev->dev.of_node);
> + if (!gpio->nr_banks) {
> + err = -EINVAL;
> + goto err;
> + }
This is too complicated. Use one device per bank and split up
the register range.
(...)
> +static const struct of_device_id xgene_gpio_of_match[] = {
> + { .compatible = "apm,xgene-gpio", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, xgene_gpio_of_match);
> +
> +static struct platform_driver xgene_gpio_driver = {
> + .driver = {
> + .name = "xgene-gpio",
> + .owner = THIS_MODULE,
> + .of_match_table = xgene_gpio_of_match,
> + },
> + .probe = xgene_gpio_probe,
> +};
> +
> +static int __init xgene_gpio_init(void)
> +{
> + return platform_driver_register(&xgene_gpio_driver);
> +}
> +postcore_initcall(xgene_gpio_init);
Why does this have to register so early? Use a simple module
driver and module_platform_driver() please.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-20 23:54 ` Feng Kan
@ 2014-05-27 8:25 ` Linus Walleij
-1 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 8:25 UTC (permalink / raw
To: Feng Kan
Cc: Alexandre Courbot, patches, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
> I see each bank as separate gpio chip. It is a simple way to
> abstract the banks since they can operate independently.
I think they should also be separate devices, and separate nodes
in the device tree.
> It also
> provided me a way to fix the sysfs gpio base number, regardless if
> a particular bank node is pulled out.
The GPIO sysfs is unpredictable for this and many other
reasons and should not be relied upon.
> This is also done in similar way
> in some other gpio drivers such as the dwapb gpio driver.
I think the dwapb has it's registers mingled while your banks
seem to be separate chunks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-27 8:25 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 8:25 UTC (permalink / raw
To: linux-arm-kernel
On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
> I see each bank as separate gpio chip. It is a simple way to
> abstract the banks since they can operate independently.
I think they should also be separate devices, and separate nodes
in the device tree.
> It also
> provided me a way to fix the sysfs gpio base number, regardless if
> a particular bank node is pulled out.
The GPIO sysfs is unpredictable for this and many other
reasons and should not be relied upon.
> This is also done in similar way
> in some other gpio drivers such as the dwapb gpio driver.
I think the dwapb has it's registers mingled while your banks
seem to be separate chunks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/3] arm64:dts: Add APM X-Gene SoC GPIO controller DTS entries
[not found] ` <1400263432-922-3-git-send-email-fkan@apm.com>
@ 2014-05-27 8:27 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 8:27 UTC (permalink / raw
To: Feng Kan
Cc: patches, Alexandre Courbot, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Fri, May 16, 2014 at 8:03 PM, Feng Kan <fkan@apm.com> wrote:
> Add gpio dts node for APM X-Gene SoC platform.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
(...)
> + gpio: gpio@1701c000 {
> + compatible = "apm,xgene-gpio";
> + reg = <0x0 0x1701c000 0x0 0x1000>;
> +
> + banka: gpio-controller@0 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <0>;
> + };
> + bankb: gpio-controller@1 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <1>;
> + };
> + bankc: gpio-controller@2 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <2>;
> + };
> + };
NAK, please construct this with one gpio per bank and
three independent entries instead of sub-banks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/3] arm64:dts: Add APM X-Gene SoC GPIO controller DTS entries
@ 2014-05-27 8:27 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 8:27 UTC (permalink / raw
To: linux-arm-kernel
On Fri, May 16, 2014 at 8:03 PM, Feng Kan <fkan@apm.com> wrote:
> Add gpio dts node for APM X-Gene SoC platform.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
(...)
> + gpio: gpio at 1701c000 {
> + compatible = "apm,xgene-gpio";
> + reg = <0x0 0x1701c000 0x0 0x1000>;
> +
> + banka: gpio-controller at 0 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <0>;
> + };
> + bankb: gpio-controller at 1 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <1>;
> + };
> + bankc: gpio-controller at 2 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <2>;
> + };
> + };
NAK, please construct this with one gpio per bank and
three independent entries instead of sub-banks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding
[not found] ` <1400263432-922-4-git-send-email-fkan@apm.com>
@ 2014-05-27 8:30 ` Linus Walleij
2014-05-27 8:30 ` Linus Walleij
1 sibling, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 8:30 UTC (permalink / raw
To: Feng Kan
Cc: patches, Alexandre Courbot, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Fri, May 16, 2014 at 8:03 PM, Feng Kan <fkan@apm.com> wrote:
> Documentation for APM X-Gene SoC GPIO controller DTS binding.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
(...)
> ---
> .../devicetree/bindings/gpio/gpio-xgene.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> new file mode 100644
> index 0000000..e19eb5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> @@ -0,0 +1,57 @@
> +APM X-Gene SoC GPIO controller bindings
> +
> +This is a gpio controller that is part of the flash controller.
> +All registers are 32bit and in little endian format.
> +
> +Required properties:
> +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller
> +- reg: Physical base address and length of the controller's registers
> +
> +The gpio controller has multiple banks, each bank will have 16 or less
> +gpio pins. All cfg field will be little endian and least significant
> +bit indicate lowest number of gpio.
> +
> +Required properties:
> +- #gpio-cells: Should be two.
> + - first cell is the pin number
> + - second cell is used to specify optional parameters (unused)
> +- gpio-controller: Marks the device node as a GPIO controller.
> +- bank: This is to specifiy which gpio bank the dts node is describing.
A lot of stuff like the separate compatible-string on the sub-banks
in the example, is undocumented.
> +Optional properties:
> +- ngpio: Specify the number of GPIOs per bank. It is defaulted to 16
> + if not specified. Number of gpio can't be greater than 16 or
> + 0.
If this is always 16 I don't see why you need the binding at all,
until there is such a system out there that use < 16 of the gpios.
> +- odcfg: This is gpio open drain/normal configuration. It is a 16 bit
> + field, setting 1 will mean the pin is set in open drain
> + configuration, 0 will mean normal configuration. Default will
> + be normal configuration.
NAK: this shall be handled by pin config.
> +Example:
> + gpio: gpio@1701c000 {
> + compatible = "apm,xgene-gpio";
> + reg = <0x0 0x1701c000 0x0 0x1000>;
> +
> + banka: gpio-controller@0 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <0>;
> + odcfg = <0xffff>; /* Optional */
> + };
> + bankb: gpio-controller@1 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <1>;
> + };
> + bankc: gpio-controller@2 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <2>;
> + };
> + };
And as mentioned before, please create three independent
GPIOs instead of three subbanks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding
@ 2014-05-27 8:30 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-27 8:30 UTC (permalink / raw
To: linux-arm-kernel
On Fri, May 16, 2014 at 8:03 PM, Feng Kan <fkan@apm.com> wrote:
> Documentation for APM X-Gene SoC GPIO controller DTS binding.
>
> Signed-off-by: Feng Kan <fkan@apm.com>
(...)
> ---
> .../devicetree/bindings/gpio/gpio-xgene.txt | 57 ++++++++++++++++++++++
> 1 file changed, 57 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/gpio/gpio-xgene.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-xgene.txt b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> new file mode 100644
> index 0000000..e19eb5f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-xgene.txt
> @@ -0,0 +1,57 @@
> +APM X-Gene SoC GPIO controller bindings
> +
> +This is a gpio controller that is part of the flash controller.
> +All registers are 32bit and in little endian format.
> +
> +Required properties:
> +- compatible: "apm,xgene-gpio" for X-Gene GPIO controller
> +- reg: Physical base address and length of the controller's registers
> +
> +The gpio controller has multiple banks, each bank will have 16 or less
> +gpio pins. All cfg field will be little endian and least significant
> +bit indicate lowest number of gpio.
> +
> +Required properties:
> +- #gpio-cells: Should be two.
> + - first cell is the pin number
> + - second cell is used to specify optional parameters (unused)
> +- gpio-controller: Marks the device node as a GPIO controller.
> +- bank: This is to specifiy which gpio bank the dts node is describing.
A lot of stuff like the separate compatible-string on the sub-banks
in the example, is undocumented.
> +Optional properties:
> +- ngpio: Specify the number of GPIOs per bank. It is defaulted to 16
> + if not specified. Number of gpio can't be greater than 16 or
> + 0.
If this is always 16 I don't see why you need the binding at all,
until there is such a system out there that use < 16 of the gpios.
> +- odcfg: This is gpio open drain/normal configuration. It is a 16 bit
> + field, setting 1 will mean the pin is set in open drain
> + configuration, 0 will mean normal configuration. Default will
> + be normal configuration.
NAK: this shall be handled by pin config.
> +Example:
> + gpio: gpio at 1701c000 {
> + compatible = "apm,xgene-gpio";
> + reg = <0x0 0x1701c000 0x0 0x1000>;
> +
> + banka: gpio-controller at 0 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <0>;
> + odcfg = <0xffff>; /* Optional */
> + };
> + bankb: gpio-controller at 1 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <1>;
> + };
> + bankc: gpio-controller at 2 {
> + compatible = "apm,xgene-gpio-port";
> + gpio-controller;
> + #gpio-cells = <2>;
> + ngpios = <16>;
> + bank = <2>;
> + };
> + };
And as mentioned before, please create three independent
GPIOs instead of three subbanks.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-27 8:25 ` Linus Walleij
@ 2014-05-27 16:59 ` Feng Kan
-1 siblings, 0 replies; 24+ messages in thread
From: Feng Kan @ 2014-05-27 16:59 UTC (permalink / raw
To: Linus Walleij
Cc: Alexandre Courbot, patches, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Tue, May 27, 2014 at 1:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
>
>> I see each bank as separate gpio chip. It is a simple way to
>> abstract the banks since they can operate independently.
>
> I think they should also be separate devices, and separate nodes
> in the device tree.
>
>> It also
>> provided me a way to fix the sysfs gpio base number, regardless if
>> a particular bank node is pulled out.
>
> The GPIO sysfs is unpredictable for this and many other
> reasons and should not be relied upon.
>
>> This is also done in similar way
>> in some other gpio drivers such as the dwapb gpio driver.
>
> I think the dwapb has it's registers mingled while your banks
> seem to be separate chunks.
Thanks for the comments, I will fix these problems. Just some details
in terms of the register layout. I will break out the gpio to separate
device nodes as instructed, I will have to use the reg attribute to offset
bank registers. Please let me know if it is acceptable.
gfc_gpio_fl0 GFC42'h1701c00c
gfc_gpio_fl0_mux GFC42'h1701c010
gfc_gpio_fl1 GFC42'h1701c018
gfc_gpio_fl1_mux GFC42'h1701c01c
gfc_gpio_fl2 GFC42'h1701c024
gfc_gpio_fl2_mux GFC42'h1701c028
gfc_gpio_fl0_od GFC42'h1701c030
gfc_gpio_fl1_od GFC42'h1701c034
gfc_gpio_fl2_od GFC42'h1701c038
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-27 16:59 ` Feng Kan
0 siblings, 0 replies; 24+ messages in thread
From: Feng Kan @ 2014-05-27 16:59 UTC (permalink / raw
To: linux-arm-kernel
On Tue, May 27, 2014 at 1:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
>
>> I see each bank as separate gpio chip. It is a simple way to
>> abstract the banks since they can operate independently.
>
> I think they should also be separate devices, and separate nodes
> in the device tree.
>
>> It also
>> provided me a way to fix the sysfs gpio base number, regardless if
>> a particular bank node is pulled out.
>
> The GPIO sysfs is unpredictable for this and many other
> reasons and should not be relied upon.
>
>> This is also done in similar way
>> in some other gpio drivers such as the dwapb gpio driver.
>
> I think the dwapb has it's registers mingled while your banks
> seem to be separate chunks.
Thanks for the comments, I will fix these problems. Just some details
in terms of the register layout. I will break out the gpio to separate
device nodes as instructed, I will have to use the reg attribute to offset
bank registers. Please let me know if it is acceptable.
gfc_gpio_fl0 GFC42'h1701c00c
gfc_gpio_fl0_mux GFC42'h1701c010
gfc_gpio_fl1 GFC42'h1701c018
gfc_gpio_fl1_mux GFC42'h1701c01c
gfc_gpio_fl2 GFC42'h1701c024
gfc_gpio_fl2_mux GFC42'h1701c028
gfc_gpio_fl0_od GFC42'h1701c030
gfc_gpio_fl1_od GFC42'h1701c034
gfc_gpio_fl2_od GFC42'h1701c038
>
> Yours,
> Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-27 16:59 ` Feng Kan
@ 2014-05-28 17:01 ` Feng Kan
-1 siblings, 0 replies; 24+ messages in thread
From: Feng Kan @ 2014-05-28 17:01 UTC (permalink / raw
To: Linus Walleij
Cc: Alexandre Courbot, patches, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Tue, May 27, 2014 at 9:59 AM, Feng Kan <fkan@apm.com> wrote:
> On Tue, May 27, 2014 at 1:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
>>
>>> I see each bank as separate gpio chip. It is a simple way to
>>> abstract the banks since they can operate independently.
>>
>> I think they should also be separate devices, and separate nodes
>> in the device tree.
>>
>>> It also
>>> provided me a way to fix the sysfs gpio base number, regardless if
>>> a particular bank node is pulled out.
>>
>> The GPIO sysfs is unpredictable for this and many other
>> reasons and should not be relied upon.
>>
>>> This is also done in similar way
>>> in some other gpio drivers such as the dwapb gpio driver.
>>
>> I think the dwapb has it's registers mingled while your banks
>> seem to be separate chunks.
>
> Thanks for the comments, I will fix these problems. Just some details
> in terms of the register layout. I will break out the gpio to separate
> device nodes as instructed, I will have to use the reg attribute to offset
> bank registers. Please let me know if it is acceptable.
>
> gfc_gpio_fl0 GFC42'h1701c00c
> gfc_gpio_fl0_mux GFC42'h1701c010
> gfc_gpio_fl1 GFC42'h1701c018
> gfc_gpio_fl1_mux GFC42'h1701c01c
> gfc_gpio_fl2 GFC42'h1701c024
> gfc_gpio_fl2_mux GFC42'h1701c028
> gfc_gpio_fl0_od GFC42'h1701c030
> gfc_gpio_fl1_od GFC42'h1701c034
> gfc_gpio_fl2_od GFC42'h1701c038
Please kindly give some guidance on this, as you can see the individual
gpio bank is interleaved in the address space, it is not likely to break this
into multiple device nodes.
>
>
>
>>
>> Yours,
>> Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-28 17:01 ` Feng Kan
0 siblings, 0 replies; 24+ messages in thread
From: Feng Kan @ 2014-05-28 17:01 UTC (permalink / raw
To: linux-arm-kernel
On Tue, May 27, 2014 at 9:59 AM, Feng Kan <fkan@apm.com> wrote:
> On Tue, May 27, 2014 at 1:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
>>
>>> I see each bank as separate gpio chip. It is a simple way to
>>> abstract the banks since they can operate independently.
>>
>> I think they should also be separate devices, and separate nodes
>> in the device tree.
>>
>>> It also
>>> provided me a way to fix the sysfs gpio base number, regardless if
>>> a particular bank node is pulled out.
>>
>> The GPIO sysfs is unpredictable for this and many other
>> reasons and should not be relied upon.
>>
>>> This is also done in similar way
>>> in some other gpio drivers such as the dwapb gpio driver.
>>
>> I think the dwapb has it's registers mingled while your banks
>> seem to be separate chunks.
>
> Thanks for the comments, I will fix these problems. Just some details
> in terms of the register layout. I will break out the gpio to separate
> device nodes as instructed, I will have to use the reg attribute to offset
> bank registers. Please let me know if it is acceptable.
>
> gfc_gpio_fl0 GFC42'h1701c00c
> gfc_gpio_fl0_mux GFC42'h1701c010
> gfc_gpio_fl1 GFC42'h1701c018
> gfc_gpio_fl1_mux GFC42'h1701c01c
> gfc_gpio_fl2 GFC42'h1701c024
> gfc_gpio_fl2_mux GFC42'h1701c028
> gfc_gpio_fl0_od GFC42'h1701c030
> gfc_gpio_fl1_od GFC42'h1701c034
> gfc_gpio_fl2_od GFC42'h1701c038
Please kindly give some guidance on this, as you can see the individual
gpio bank is interleaved in the address space, it is not likely to break this
into multiple device nodes.
>
>
>
>>
>> Yours,
>> Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-27 16:59 ` Feng Kan
@ 2014-05-28 19:10 ` Linus Walleij
-1 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-28 19:10 UTC (permalink / raw
To: Feng Kan
Cc: Alexandre Courbot, patches, linux-gpio@vger.kernel.org,
devicetree@vger.kernel.org, Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Tue, May 27, 2014 at 6:59 PM, Feng Kan <fkan@apm.com> wrote:
> I will break out the gpio to separate
> device nodes as instructed, I will have to use the reg attribute to offset
> bank registers. Please let me know if it is acceptable.
Yeah sure regs = <a>, <b>, <c> for a single node is OK if that's what
you mean.
> gfc_gpio_fl0 GFC42'h1701c00c
> gfc_gpio_fl0_mux GFC42'h1701c010
> gfc_gpio_fl1 GFC42'h1701c018
> gfc_gpio_fl1_mux GFC42'h1701c01c
> gfc_gpio_fl2 GFC42'h1701c024
> gfc_gpio_fl2_mux GFC42'h1701c028
> gfc_gpio_fl0_od GFC42'h1701c030
> gfc_gpio_fl1_od GFC42'h1701c034
> gfc_gpio_fl2_od GFC42'h1701c038
Strange syntax in the last line there. GFC42'h uhm.
But that looks a bit mingled actually. However I think you will be
much happier if you split it into individual devices as I wanted to.
It will be easier to maintain and simpler code even if the device tree
may look a bit strange.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-28 19:10 ` Linus Walleij
0 siblings, 0 replies; 24+ messages in thread
From: Linus Walleij @ 2014-05-28 19:10 UTC (permalink / raw
To: linux-arm-kernel
On Tue, May 27, 2014 at 6:59 PM, Feng Kan <fkan@apm.com> wrote:
> I will break out the gpio to separate
> device nodes as instructed, I will have to use the reg attribute to offset
> bank registers. Please let me know if it is acceptable.
Yeah sure regs = <a>, <b>, <c> for a single node is OK if that's what
you mean.
> gfc_gpio_fl0 GFC42'h1701c00c
> gfc_gpio_fl0_mux GFC42'h1701c010
> gfc_gpio_fl1 GFC42'h1701c018
> gfc_gpio_fl1_mux GFC42'h1701c01c
> gfc_gpio_fl2 GFC42'h1701c024
> gfc_gpio_fl2_mux GFC42'h1701c028
> gfc_gpio_fl0_od GFC42'h1701c030
> gfc_gpio_fl1_od GFC42'h1701c034
> gfc_gpio_fl2_od GFC42'h1701c038
Strange syntax in the last line there. GFC42'h uhm.
But that looks a bit mingled actually. However I think you will be
much happier if you split it into individual devices as I wanted to.
It will be easier to maintain and simpler code even if the device tree
may look a bit strange.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
2014-05-28 17:01 ` Feng Kan
@ 2014-05-28 19:10 ` Rob Herring
-1 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2014-05-28 19:10 UTC (permalink / raw
To: Feng Kan
Cc: Linus Walleij, Alexandre Courbot, patches,
linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
Catalin Marinas, Will Deacon,
linux-arm-kernel@lists.infradead.org
On Wed, May 28, 2014 at 12:01 PM, Feng Kan <fkan@apm.com> wrote:
> On Tue, May 27, 2014 at 9:59 AM, Feng Kan <fkan@apm.com> wrote:
>> On Tue, May 27, 2014 at 1:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
>>>
>>>> I see each bank as separate gpio chip. It is a simple way to
>>>> abstract the banks since they can operate independently.
>>>
>>> I think they should also be separate devices, and separate nodes
>>> in the device tree.
>>>
>>>> It also
>>>> provided me a way to fix the sysfs gpio base number, regardless if
>>>> a particular bank node is pulled out.
>>>
>>> The GPIO sysfs is unpredictable for this and many other
>>> reasons and should not be relied upon.
>>>
>>>> This is also done in similar way
>>>> in some other gpio drivers such as the dwapb gpio driver.
>>>
>>> I think the dwapb has it's registers mingled while your banks
>>> seem to be separate chunks.
>>
>> Thanks for the comments, I will fix these problems. Just some details
>> in terms of the register layout. I will break out the gpio to separate
>> device nodes as instructed, I will have to use the reg attribute to offset
>> bank registers. Please let me know if it is acceptable.
>>
>> gfc_gpio_fl0 GFC42'h1701c00c
>> gfc_gpio_fl0_mux GFC42'h1701c010
>> gfc_gpio_fl1 GFC42'h1701c018
>> gfc_gpio_fl1_mux GFC42'h1701c01c
>> gfc_gpio_fl2 GFC42'h1701c024
>> gfc_gpio_fl2_mux GFC42'h1701c028
>> gfc_gpio_fl0_od GFC42'h1701c030
>> gfc_gpio_fl1_od GFC42'h1701c034
>> gfc_gpio_fl2_od GFC42'h1701c038
>
> Please kindly give some guidance on this, as you can see the individual
> gpio bank is interleaved in the address space, it is not likely to break this
> into multiple device nodes.
It should be 1 node.
Rob
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support
@ 2014-05-28 19:10 ` Rob Herring
0 siblings, 0 replies; 24+ messages in thread
From: Rob Herring @ 2014-05-28 19:10 UTC (permalink / raw
To: linux-arm-kernel
On Wed, May 28, 2014 at 12:01 PM, Feng Kan <fkan@apm.com> wrote:
> On Tue, May 27, 2014 at 9:59 AM, Feng Kan <fkan@apm.com> wrote:
>> On Tue, May 27, 2014 at 1:25 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
>>> On Wed, May 21, 2014 at 1:54 AM, Feng Kan <fkan@apm.com> wrote:
>>>
>>>> I see each bank as separate gpio chip. It is a simple way to
>>>> abstract the banks since they can operate independently.
>>>
>>> I think they should also be separate devices, and separate nodes
>>> in the device tree.
>>>
>>>> It also
>>>> provided me a way to fix the sysfs gpio base number, regardless if
>>>> a particular bank node is pulled out.
>>>
>>> The GPIO sysfs is unpredictable for this and many other
>>> reasons and should not be relied upon.
>>>
>>>> This is also done in similar way
>>>> in some other gpio drivers such as the dwapb gpio driver.
>>>
>>> I think the dwapb has it's registers mingled while your banks
>>> seem to be separate chunks.
>>
>> Thanks for the comments, I will fix these problems. Just some details
>> in terms of the register layout. I will break out the gpio to separate
>> device nodes as instructed, I will have to use the reg attribute to offset
>> bank registers. Please let me know if it is acceptable.
>>
>> gfc_gpio_fl0 GFC42'h1701c00c
>> gfc_gpio_fl0_mux GFC42'h1701c010
>> gfc_gpio_fl1 GFC42'h1701c018
>> gfc_gpio_fl1_mux GFC42'h1701c01c
>> gfc_gpio_fl2 GFC42'h1701c024
>> gfc_gpio_fl2_mux GFC42'h1701c028
>> gfc_gpio_fl0_od GFC42'h1701c030
>> gfc_gpio_fl1_od GFC42'h1701c034
>> gfc_gpio_fl2_od GFC42'h1701c038
>
> Please kindly give some guidance on this, as you can see the individual
> gpio bank is interleaved in the address space, it is not likely to break this
> into multiple device nodes.
It should be 1 node.
Rob
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2014-05-28 19:10 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1400263432-922-1-git-send-email-fkan@apm.com>
[not found] ` <1400263432-922-2-git-send-email-fkan@apm.com>
2014-05-20 6:04 ` [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support Alexandre Courbot
2014-05-20 6:04 ` Alexandre Courbot
2014-05-20 23:54 ` Feng Kan
2014-05-20 23:54 ` Feng Kan
2014-05-25 7:35 ` Alexandre Courbot
2014-05-25 7:35 ` Alexandre Courbot
2014-05-27 8:25 ` Linus Walleij
2014-05-27 8:25 ` Linus Walleij
2014-05-27 16:59 ` Feng Kan
2014-05-27 16:59 ` Feng Kan
2014-05-28 17:01 ` Feng Kan
2014-05-28 17:01 ` Feng Kan
2014-05-28 19:10 ` Rob Herring
2014-05-28 19:10 ` Rob Herring
2014-05-28 19:10 ` Linus Walleij
2014-05-28 19:10 ` Linus Walleij
2014-05-27 7:59 ` Linus Walleij
2014-05-27 7:59 ` Linus Walleij
[not found] ` <1400263432-922-4-git-send-email-fkan@apm.com>
2014-05-20 6:11 ` [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding Alexandre Courbot
2014-05-20 6:11 ` Alexandre Courbot
2014-05-27 8:30 ` Linus Walleij
2014-05-27 8:30 ` Linus Walleij
[not found] ` <1400263432-922-3-git-send-email-fkan@apm.com>
2014-05-27 8:27 ` [PATCH 2/3] arm64:dts: Add APM X-Gene SoC GPIO controller DTS entries Linus Walleij
2014-05-27 8:27 ` Linus Walleij
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.