Re: [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding

Re: [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding

[Alexandre Courbot]

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
>

Re: [PATCH 3/3] Documentation: gpio: Add APM X-Gene SoC GPIO controller DTS binding

[Linus Walleij]

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

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Alexandre Courbot]

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.

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Feng Kan]

> +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.

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Alexandre Courbot]

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.

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Linus Walleij]

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

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Feng Kan]

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

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Feng Kan]

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

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Rob Herring]

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

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Linus Walleij]

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

Re: [PATCH 1/3] gpio: Add APM X-Gene SoC GPIO controller support

[Linus Walleij]

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

Re: [PATCH 2/3] arm64:dts: Add APM X-Gene SoC GPIO controller DTS entries

[Linus Walleij]

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