linux-arm-kernel.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
From: Bartosz Golaszewski <bgolaszewski@baylibre.com>
To: Linus Walleij <linus.walleij@linaro.org>
Cc: Arnd Bergmann <arnd@arndb.de>, Tim Harvey <tharvey@gateworks.com>,
	Krzysztof Halasa <khalasa@piap.pl>,
	Olof Johansson <olof@lixom.net>, Imre Kaloz <kaloz@openwrt.org>,
	arm-soc <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH 05/17 v1] gpio: ixp4xx: Add driver for the IXP4xx GPIO
Date: Wed, 6 Feb 2019 17:03:58 +0100	[thread overview]
Message-ID: <CAMpxmJWRH9+fbwyhjHe-ERjQfMCXTGzOLWeq1HJbbYXsZ14nXA@mail.gmail.com> (raw)
In-Reply-To: <20190203214205.13594-6-linus.walleij@linaro.org>

niedz., 3 lut 2019 o 22:42 Linus Walleij <linus.walleij@linaro.org> napisał(a):
>
> This adds a driver for the IXP4xx GPIO block found in
> the Intel XScale IXP4xx systems.
>
> The GPIO part of this block is pretty straight-forward and
> just uses the generic MMIO GPIO library.
>
> The irqchip side of this driver is hierarchical where
> the main irqchip will receive a processed level trigger
> in response to the edge detector of the GPIO block,
> so for this reason the v2 version of the irqdomain API
> is used (as well as in the parent IXP4xx irqchip) and
> masking, unmasking and setting up the type on IRQ
> happens on several levels.
>
> Currently this GPIO controller will grab the parent
> irqdomain using a special function, but as the platform
> move toward device tree probing, this will not be needed:
> we can just look up the parent irqdomain from the device
> tree.
>
> Cc: Bartosz Golaszewski <bgolaszewski@baylibre.com>
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
> Bartosz: looking for your ACK on this, it'd be good if
> the other GPIO maintainer is aligned with my ideas here.
> I intend to merge this through the ARM SoC tree.
> ---

Linus,

My remarks are below. Mostly just nits (feel free to ignore) or
questions that I'd like clarified.

>  MAINTAINERS                |   1 +
>  drivers/gpio/Kconfig       |  12 +
>  drivers/gpio/Makefile      |   1 +
>  drivers/gpio/gpio-ixp4xx.c | 443 +++++++++++++++++++++++++++++++++++++
>  4 files changed, 457 insertions(+)
>  create mode 100644 drivers/gpio/gpio-ixp4xx.c
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0d48faa3e635..6c161bd82238 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -1651,6 +1651,7 @@ M:        Krzysztof Halasa <khalasa@piap.pl>
>  L:     linux-arm-kernel@lists.infradead.org (moderated for non-subscribers)
>  S:     Maintained
>  F:     arch/arm/mach-ixp4xx/
> +F:     drivers/gpio/gpio-ixp4xx.c
>  F:     drivers/irqchip/irq-ixp4xx.c
>  F:     include/linux/irqchip/irq-ixp4xx.h
>
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 699a8118c433..fe4a47e49a24 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -286,6 +286,18 @@ config GPIO_IOP
>
>           If unsure, say N.
>
> +config GPIO_IXP4XX
> +       bool "Intel IXP4xx GPIO"
> +       depends on ARCH_IXP4XX || COMPILE_TEST
> +       select GPIO_GENERIC
> +       select IRQ_DOMAIN
> +       select IRQ_DOMAIN_HIERARCHY
> +       help
> +         Say yes here to support the GPIO functionality of a number of Intel
> +         IXP4xx series of chips.
> +
> +         If unsure, say N.
> +
>  config GPIO_LOONGSON
>         bool "Loongson-2/3 GPIO support"
>         depends on CPU_LOONGSON2 || CPU_LOONGSON3
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 0568bbe6fe68..cf66523b5eec 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -60,6 +60,7 @@ obj-$(CONFIG_GPIO_HLWD)               += gpio-hlwd.o
>  obj-$(CONFIG_HTC_EGPIO)                += gpio-htc-egpio.o
>  obj-$(CONFIG_GPIO_ICH)         += gpio-ich.o
>  obj-$(CONFIG_GPIO_IOP)         += gpio-iop.o
> +obj-$(CONFIG_GPIO_IXP4XX)      += gpio-ixp4xx.o
>  obj-$(CONFIG_GPIO_IT87)                += gpio-it87.o
>  obj-$(CONFIG_GPIO_JANZ_TTL)    += gpio-janz-ttl.o
>  obj-$(CONFIG_GPIO_KEMPLD)      += gpio-kempld.o
> diff --git a/drivers/gpio/gpio-ixp4xx.c b/drivers/gpio/gpio-ixp4xx.c
> new file mode 100644
> index 000000000000..44c24948379d
> --- /dev/null
> +++ b/drivers/gpio/gpio-ixp4xx.c
> @@ -0,0 +1,443 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * IXP4 GPIO driver
> + * Copyright (C) 2019 Linus Walleij <linus.walleij@linaro.org>
> + *
> + * based on previous work and know-how from:
> + * Deepak Saxena <dsaxena@plexity.net>
> + */

Just a nit, but I'd add a newline between the includes and the header.
Also: C++ style comment for the header.

> +#include <linux/gpio/driver.h>
> +#include <linux/io.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/irqchip.h>
> +#include <linux/platform_device.h>
> +#include <linux/bitops.h>
> +/* Include that go away with DT transition */
> +#include <linux/irqchip/irq-ixp4xx.h>

I think a newline between linux/ and asm/ includes is a good rule.

> +#include <asm/mach-types.h>
> +
> +#define IXP4XX_GPIO_GPOUTR     0x00
> +#define IXP4XX_GPIO_GPOER      0x04
> +#define IXP4XX_GPIO_GPINR      0x08
> +#define IXP4XX_GPIO_GPISR      0x0C
> +#define IXP4XX_GPIO_GPIT1R     0x10
> +#define IXP4XX_GPIO_GPIT2R     0x14
> +#define IXP4XX_GPIO_GPCLKR     0x18
> +#define IXP4XX_GPIO_GPDBSELR   0x1C
> +

Since these are registers offsets - maybe the prefix could be
IXP4XX_GPIO_REG_* or IXP4XX_GPIO_OFF_*? I think it's more readable.

> +/*
> + * The hardware uses 3 bits to indicate interrupt "style".
> + * we clear and set these three bits accordingly. The lower 24
> + * bits in two registers (GPIT1R and GPIT2R) are used to set up
> + * the style for 8 lines each for a total of 16 GPIO lines.
> + */
> +#define IXP4XX_GPIO_STYLE_ACTIVE_HIGH  0x0
> +#define IXP4XX_GPIO_STYLE_ACTIVE_LOW   0x1
> +#define IXP4XX_GPIO_STYLE_RISING_EDGE  0x2
> +#define IXP4XX_GPIO_STYLE_FALLING_EDGE 0x3
> +#define IXP4XX_GPIO_STYLE_TRANSITIONAL 0x4
> +#define IXP4XX_GPIO_STYLE_MASK         0x7

I'd use GENMASK(2, 0) here.

> +#define IXP4XX_GPIO_STYLE_SIZE         3
> +
> +/**
> + * struct ixp4xx_gpio - IXP4 GPIO state container
> + * @dev: containing device for this instance
> + * @fwnode: the fwnode for this GPIO chip
> + * @gc: gpiochip for this instance
> + * @domain: irqdomain for this chip instance
> + * @base: remapped I/O-memory base
> + * @irq_edge: Each bit represents an IRQ: 1: edge-triggered,
> + * 0: level triggered
> + */
> +struct ixp4xx_gpio {
> +       struct device *dev;
> +       struct fwnode_handle *fwnode;
> +       struct gpio_chip gc;
> +       struct irq_domain *domain;
> +       void __iomem *base;
> +       unsigned long long irq_edge;
> +};
> +
> +/**
> + * struct ixp4xx_gpio_map - IXP4 GPIO to parent IRQ map
> + * @gpio_offset: offset of the IXP4 GPIO line
> + * @parent_hwirq: hwirq on the parent IRQ controller
> + */
> +struct ixp4xx_gpio_map {
> +       int gpio_offset;
> +       int parent_hwirq;
> +};
> +
> +/* GPIO lines 0..12 have corresponding IRQs, GPIOs 13..15 have no IRQs */
> +const struct ixp4xx_gpio_map ixp4xx_gpiomap[] = {
> +       { .gpio_offset = 0, .parent_hwirq = 6 },
> +       { .gpio_offset = 1, .parent_hwirq = 7 },
> +       { .gpio_offset = 2, .parent_hwirq = 19 },
> +       { .gpio_offset = 3, .parent_hwirq = 20 },
> +       { .gpio_offset = 4, .parent_hwirq = 21 },
> +       { .gpio_offset = 5, .parent_hwirq = 22 },
> +       { .gpio_offset = 6, .parent_hwirq = 23 },
> +       { .gpio_offset = 7, .parent_hwirq = 24 },
> +       { .gpio_offset = 8, .parent_hwirq = 25 },
> +       { .gpio_offset = 9, .parent_hwirq = 26 },
> +       { .gpio_offset = 10, .parent_hwirq = 27 },
> +       { .gpio_offset = 11, .parent_hwirq = 28 },
> +       { .gpio_offset = 12, .parent_hwirq = 29 },
> +};
> +
> +static void ixp4xx_gpio_irq_ack(struct irq_data *d)
> +{
> +       struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d);
> +
> +       __raw_writel(BIT(d->hwirq), g->base + IXP4XX_GPIO_GPISR);
> +}
> +
> +static void ixp4xx_gpio_irq_unmask(struct irq_data *d)
> +{
> +       struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d);
> +
> +       /* ACK when unmasking if not edge-triggered */
> +       if (!(g->irq_edge & BIT(d->hwirq)))
> +               ixp4xx_gpio_irq_ack(d);
> +
> +       irq_chip_unmask_parent(d);
> +}
> +
> +static int ixp4xx_gpio_irq_set_type(struct irq_data *d, unsigned int type)
> +{
> +       struct ixp4xx_gpio *g = irq_data_get_irq_chip_data(d);
> +       int line = d->hwirq;
> +       u32 int_style;
> +       unsigned long flags;
> +       u32 int_reg;
> +       u32 val;
> +

I'm personally a fan of putting variables of the same type in the same
line and arranging them in reverse-christmas tree order, but feel free
to ignore it.

> +       switch (type) {
> +       case IRQ_TYPE_EDGE_BOTH:
> +               irq_set_handler_locked(d, handle_edge_irq);
> +               int_style = IXP4XX_GPIO_STYLE_TRANSITIONAL;
> +               g->irq_edge |= BIT(d->hwirq);
> +               break;
> +       case IRQ_TYPE_EDGE_RISING:
> +               irq_set_handler_locked(d, handle_edge_irq);
> +               int_style = IXP4XX_GPIO_STYLE_RISING_EDGE;
> +               g->irq_edge |= BIT(d->hwirq);
> +               break;
> +       case IRQ_TYPE_EDGE_FALLING:
> +               irq_set_handler_locked(d, handle_edge_irq);
> +               int_style = IXP4XX_GPIO_STYLE_FALLING_EDGE;
> +               g->irq_edge |= BIT(d->hwirq);
> +               break;
> +       case IRQ_TYPE_LEVEL_HIGH:
> +               irq_set_handler_locked(d, handle_level_irq);
> +               int_style = IXP4XX_GPIO_STYLE_ACTIVE_HIGH;
> +               g->irq_edge &= ~BIT(d->hwirq);
> +               break;
> +       case IRQ_TYPE_LEVEL_LOW:
> +               irq_set_handler_locked(d, handle_level_irq);
> +               int_style = IXP4XX_GPIO_STYLE_ACTIVE_LOW;
> +               g->irq_edge &= ~BIT(d->hwirq);
> +               break;
> +       default:
> +               return -EINVAL;
> +       }
> +
> +       if (line >= 8) {
> +               /* pins 8-15 */
> +               line -= 8;
> +               int_reg = IXP4XX_GPIO_GPIT2R;
> +       } else {
> +               /* pins 0-7 */
> +               int_reg = IXP4XX_GPIO_GPIT1R;
> +       }
> +
> +       spin_lock_irqsave(&g->gc.bgpio_lock, flags);
> +
> +       /* Clear the style for the appropriate pin */
> +       val = __raw_readl(g->base + int_reg);
> +       val &= ~(IXP4XX_GPIO_STYLE_MASK << (line * IXP4XX_GPIO_STYLE_SIZE));
> +       __raw_writel(val, g->base + int_reg);

I know you're not using regmap, because this driver's based on
gpio-mmio, but I'm wondering if you could maybe wrap those three
operations in a helper wrapper e.g. ixp4xx_update_reg() or something
to make it easier to read. Same below.

> +
> +       __raw_writel(BIT(line), g->base + IXP4XX_GPIO_GPISR);
> +
> +       /* Set the new style */
> +       val = __raw_readl(g->base + int_reg);
> +       val |= (int_style << (line * IXP4XX_GPIO_STYLE_SIZE));
> +       __raw_writel(val, g->base + int_reg);
> +
> +       /* Force-configure this line as an input */
> +       val = __raw_readl(g->base + IXP4XX_GPIO_GPOER);
> +       val |= BIT(d->hwirq);
> +       __raw_writel(val, g->base + IXP4XX_GPIO_GPOER);
> +
> +       spin_unlock_irqrestore(&g->gc.bgpio_lock, flags);
> +
> +       /* This parent only accept level high (asserted) */
> +       return irq_chip_set_type_parent(d, IRQ_TYPE_LEVEL_HIGH);
> +}
> +
> +static struct irq_chip ixp4xx_gpio_irqchip = {
> +       .name = "IXP4GPIO",
> +       .irq_ack = ixp4xx_gpio_irq_ack,
> +       .irq_mask = irq_chip_mask_parent,
> +       .irq_unmask = ixp4xx_gpio_irq_unmask,
> +       .irq_set_type = ixp4xx_gpio_irq_set_type,
> +};

I assume this device cannot have multiple instances, so it's safe to
have a static irqchip?

> +
> +static int ixp4xx_gpio_to_irq(struct gpio_chip *gc, unsigned int offset)
> +{
> +       struct ixp4xx_gpio *g = gpiochip_get_data(gc);
> +       struct irq_fwspec fwspec;
> +
> +       fwspec.fwnode = g->fwnode;
> +       fwspec.param_count = 2;
> +       fwspec.param[0] = offset;
> +       fwspec.param[1] = IRQ_TYPE_NONE;
> +
> +       return irq_create_fwspec_mapping(&fwspec);
> +}
> +

This is were I start to struggle. I'm still not very well versed in
irq domain hierarchies yet. You already explain the concept in the
commit message, but it would be great if you could add a comment
describing the technical details of implementation here and in other
related callbacks. The whole fwspec thingy is not very obvious and it
would serve as an example for the future.

> +static int ixp4xx_gpio_irq_domain_translate(struct irq_domain *domain,
> +                                           struct irq_fwspec *fwspec,
> +                                           unsigned long *hwirq,
> +                                           unsigned int *type)
> +{
> +
> +       /* We support standard DT translation */
> +       if (is_of_node(fwspec->fwnode) && fwspec->param_count == 2) {
> +               *hwirq = fwspec->param[0];
> +               *type = fwspec->param[1];
> +               return 0;
> +       }

This logic is a bit non-intuitive - maybe you could first check for
error conditions and then do any processing?

> +
> +       /* This goes away when we transition to DT */
> +       if (is_fwnode_irqchip(fwspec->fwnode)) {
> +               if (fwspec->param_count != 2)
> +                       return -EINVAL;
> +               *hwirq = fwspec->param[0];
> +               *type = fwspec->param[1];
> +               WARN_ON(*type == IRQ_TYPE_NONE);
> +               return 0;
> +       }
> +       return -EINVAL;
> +}
> +
> +static int ixp4xx_gpio_irq_domain_alloc(struct irq_domain *d,
> +                                       unsigned int irq, unsigned int nr_irqs,
> +                                       void *data)
> +{
> +       struct ixp4xx_gpio *g = d->host_data;
> +       irq_hw_number_t hwirq;
> +       unsigned int type = IRQ_TYPE_NONE;
> +       struct irq_fwspec *fwspec = data;
> +       int ret;
> +       int i;
> +
> +       ret = ixp4xx_gpio_irq_domain_translate(d, fwspec, &hwirq, &type);
> +       if (ret)
> +               return ret;
> +
> +       dev_dbg(g->dev, "allocate IRQ %d..%d, hwirq %lu..%lu\n",
> +               irq, irq + nr_irqs - 1,
> +               hwirq, hwirq + nr_irqs - 1);
> +
> +       for (i = 0; i < nr_irqs; i++) {
> +               struct irq_fwspec parent_fwspec;
> +               const struct ixp4xx_gpio_map *map;
> +               int j;
> +
> +               /* Not all lines support IRQs */
> +               for (j = 0; j < ARRAY_SIZE(ixp4xx_gpiomap); j++) {
> +                       map = &ixp4xx_gpiomap[j];
> +                       if (map->gpio_offset == hwirq)
> +                               break;
> +               }
> +               if (j == ARRAY_SIZE(ixp4xx_gpiomap)) {
> +                       dev_err(g->dev, "can't look up hwirq %lu\n", hwirq);
> +                       return -EINVAL;
> +               }
> +               dev_dbg(g->dev, "found parent hwirq %u\n", map->parent_hwirq);
> +
> +               /*
> +                * We set handle_bad_irq because the .set_type() should
> +                * always be invoked and set the right type of handler.
> +                */
> +               irq_domain_set_info(d,
> +                                   irq + i,
> +                                   hwirq + i,
> +                                   &ixp4xx_gpio_irqchip,
> +                                   g,
> +                                   handle_bad_irq,
> +                                   NULL, NULL);
> +               irq_set_probe(irq + i);
> +
> +               /*
> +                * Create a IRQ fwspec to send up to the parent irqdomain:
> +                * specify the hwirq we address on the parent and tie it
> +                * all together up the chain.
> +                */
> +               parent_fwspec.fwnode = d->parent->fwnode;
> +               parent_fwspec.param_count = 2;
> +               parent_fwspec.param[0] = map->parent_hwirq;
> +               /* This parent only handles asserted level IRQs */
> +               parent_fwspec.param[1] = IRQ_TYPE_LEVEL_HIGH;
> +               dev_dbg(g->dev, "alloc_irqs_parent for %d parent hwirq %d\n",
> +                       irq + i, map->parent_hwirq);
> +               ret = irq_domain_alloc_irqs_parent(d, irq + i, 1,
> +                                                  &parent_fwspec);
> +               if (ret)
> +                       dev_err(g->dev,
> +                               "failed to allocate parent hwirq %d for hwirq %lu\n",
> +                               map->parent_hwirq, hwirq);
> +       }
> +
> +       return 0;
> +}
> +
> +static const struct irq_domain_ops ixp4xx_gpio_irqdomain_ops = {
> +       .translate = ixp4xx_gpio_irq_domain_translate,
> +       .alloc = ixp4xx_gpio_irq_domain_alloc,
> +       .free = irq_domain_free_irqs_common,
> +};
> +
> +static int ixp4xx_gpio_probe(struct platform_device *pdev)
> +{
> +       unsigned long flags;
> +       struct device *dev = &pdev->dev;
> +       struct irq_domain *parent;
> +       struct resource *res;
> +       struct ixp4xx_gpio *g;
> +       int ret;
> +       int i;
> +
> +       g = devm_kzalloc(dev, sizeof(*g), GFP_KERNEL);
> +       if (!g)
> +               return -ENOMEM;
> +       g->dev = dev;
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       g->base = devm_ioremap_resource(dev, res);
> +       if (IS_ERR(g->base)) {
> +               dev_err(dev, "ioremap error\n");
> +               return PTR_ERR(g->base);
> +       }
> +
> +       /*
> +        * Make sure GPIO 14 and 15 are NOT used as clocks but GPIO on
> +        * specific machines.
> +        */
> +       if (machine_is_dsmg600() || machine_is_nas100d())
> +               __raw_writel(0x0, g->base + IXP4XX_GPIO_GPCLKR);
> +
> +       /*
> +        * This is a very special big-endian ARM issue: when the IXP4xx is
> +        * run in big endian mode, all registers in the machine are switched
> +        * around to the CPU-native endianness. As you see mostly in the
> +        * driver we use __raw_readl()/__raw_writel() to access the registers
> +        * in the appropriate order. With the GPIO library we need to specify
> +        * byte order explicitly, so this flag needs to be set when compiling
> +        * for big endian.
> +        */
> +#if defined(CONFIG_CPU_BIG_ENDIAN)
> +       flags = BGPIOF_BIG_ENDIAN_BYTE_ORDER;
> +#else
> +       flags = 0;
> +#endif
> +
> +       /* Populate and register gpio chip */
> +       ret = bgpio_init(&g->gc, dev, 4,
> +                        g->base + IXP4XX_GPIO_GPINR,
> +                        g->base + IXP4XX_GPIO_GPOUTR,
> +                        NULL,
> +                        NULL,
> +                        g->base + IXP4XX_GPIO_GPOER,
> +                        flags);
> +       if (ret) {
> +               dev_err(dev, "unable to init generic GPIO\n");
> +               return ret;
> +       }
> +       g->gc.to_irq = ixp4xx_gpio_to_irq;
> +       g->gc.ngpio = 16;
> +       g->gc.label = "IXP4XX_GPIO_CHIP";
> +       /*
> +        * TODO: when we have migrated to device tree and all GPIOs
> +        * are fetched using phandles, set this to -1 to get rid of
> +        * the fixed gpiochip base.
> +        */
> +       g->gc.base = 0;
> +       g->gc.parent = &pdev->dev;
> +       g->gc.owner = THIS_MODULE;
> +
> +       ret = devm_gpiochip_add_data(dev, &g->gc, g);
> +       if (ret) {
> +               dev_err(dev, "failed to add SoC gpiochip\n");
> +               return ret;
> +       }
> +
> +       /*
> +        * When we convert to device tree we will simply look up the
> +        * parent irqdomain using irq_find_host(parent) as parent comes
> +        * from IRQCHIP_DECLARE(), then use of_node_to_fwnode() to get
> +        * the fwnode. For now we need this boardfile style code.
> +        */
> +       parent = ixp4xx_get_irq_domain();
> +       g->fwnode = irq_domain_alloc_fwnode(g->base);
> +       if (!g->fwnode) {
> +               dev_err(dev, "no domain base\n");
> +               return -ENODEV;
> +       }
> +       g->domain = irq_domain_create_hierarchy(parent,
> +                                               IRQ_DOMAIN_FLAG_HIERARCHY,
> +                                               ARRAY_SIZE(ixp4xx_gpiomap),
> +                                               g->fwnode,
> +                                               &ixp4xx_gpio_irqdomain_ops,
> +                                               g);
> +       if (!g->domain) {
> +               irq_domain_free_fwnode(g->fwnode);
> +               dev_err(dev, "no hierarchical irq domain\n");
> +               return ret;
> +       }
> +
> +       /*
> +        * After adding OF support, this is no longer needed: irqs
> +        * will be allocated for the respective fwnodes.
> +        */
> +       for (i = 0; i < ARRAY_SIZE(ixp4xx_gpiomap); i++) {
> +               const struct ixp4xx_gpio_map *map = &ixp4xx_gpiomap[i];
> +               struct irq_fwspec fwspec;
> +
> +               fwspec.fwnode = g->fwnode;
> +               /* This is the hwirq for the GPIO line side of things */
> +               fwspec.param[0] = map->gpio_offset;
> +               fwspec.param[1] = IRQ_TYPE_EDGE_RISING;
> +               fwspec.param_count = 2;
> +               ret = __irq_domain_alloc_irqs(g->domain,
> +                                             -1, /* just pick something */
> +                                             1,
> +                                             NUMA_NO_NODE,
> +                                             &fwspec,
> +                                             false,
> +                                             NULL);
> +               if (ret < 0) {
> +                       irq_domain_free_fwnode(g->fwnode);
> +                       dev_err(dev,
> +                               "can not allocate irq for GPIO line %d parent hwirq %d in hierarchy domain: %d\n",
> +                               map->gpio_offset, map->parent_hwirq, ret);
> +                       return ret;
> +               }
> +       }
> +
> +       platform_set_drvdata(pdev, g);
> +       dev_info(dev, "IXP4 GPIO @%p registered\n", g->base);
> +
> +       return 0;
> +}
> +

Don't you need to dispose of the domain in the remove callback? They
don't seem to have devm_ variants yet.

> +static struct platform_driver ixp4xx_gpio_driver = {
> +       .driver = {
> +               .name           = "ixp4xx-gpio",
> +       },
> +       .probe = ixp4xx_gpio_probe,
> +};
> +builtin_platform_driver(ixp4xx_gpio_driver);
> --
> 2.20.1
>

Best regards,
Bartosz Golaszewski

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2019-02-06 16:04 UTC|newest]

Thread overview: 43+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-03 21:41 [PATCH 00/17 v1] ARM: ixp4xx: Modernize and DT support Linus Walleij
2019-02-03 21:41 ` [PATCH 01/17 v1] ARM: ixp4xx: Convert to MULTI_IRQ_HANDLER Linus Walleij
2019-02-03 21:41 ` [PATCH 02/17 v1] ARM: ixp4xx: Pass IRQ resource to beeper Linus Walleij
2019-02-03 21:41 ` [PATCH 03/17 v1] ARM: ixp4xx: Convert to SPARSE_IRQ Linus Walleij
2019-02-03 21:41 ` [PATCH 04/17 v1] irqchip: Add driver for IXP4xx Linus Walleij
2019-02-11 15:30   ` Marc Zyngier
2019-02-11 20:58     ` Linus Walleij
2019-02-11 22:11       ` Marc Zyngier
2019-02-18  7:06         ` Krzysztof Hałasa
2019-02-18  7:16           ` Linus Walleij
2019-02-18  7:35             ` Krzysztof Hałasa
2019-02-18  9:40             ` Arnd Bergmann
2019-02-18 12:03               ` Krzysztof Hałasa
2019-02-18 12:44                 ` Arnd Bergmann
2019-02-19  6:51                   ` Krzysztof Hałasa
2019-02-19  9:46                     ` Arnd Bergmann
2019-02-20  7:35                       ` Krzysztof Hałasa
2019-02-18  9:18         ` Arnd Bergmann
2019-02-03 21:41 ` [PATCH 05/17 v1] gpio: ixp4xx: Add driver for the IXP4xx GPIO Linus Walleij
2019-02-06 16:03   ` Bartosz Golaszewski [this message]
2019-02-21  8:50     ` Linus Walleij
2019-02-03 21:41 ` [PATCH 06/17 v1] ARM: ixp4xx: Switch to use new IRQ+GPIO drivers Linus Walleij
2019-02-03 21:41 ` [PATCH 07/17 v1] clocksource/drivers/ixp4xx: Add driver Linus Walleij
2019-02-03 21:41 ` [PATCH 08/17 v1] ARM: ixp4xx: Switch to use new timer driver Linus Walleij
2019-02-03 21:41 ` [PATCH 09/17 v1] irqchip: ixp4xx: Add DT bindings Linus Walleij
2019-02-18 21:25   ` Rob Herring
2019-02-03 21:41 ` [PATCH 10/17 v1] irqchip: ixp4xx: Add OF initialization support Linus Walleij
2019-02-03 21:41 ` [PATCH 11/17 v1] clocksource/drivers/ixp4xx: Add DT bindings Linus Walleij
2019-02-18 21:26   ` Rob Herring
2019-02-18 22:10     ` Daniel Lezcano
2019-02-03 21:42 ` [PATCH 12/17 v1] clocksource/drivers/ixp4xx: Add OF initialization support Linus Walleij
2019-02-11 11:26   ` Daniel Lezcano
2019-02-03 21:42 ` [PATCH 13/17 v1] gpio: ixp4xx: Add DT bindings Linus Walleij
2019-02-06 16:05   ` Bartosz Golaszewski
2019-02-18 21:27   ` Rob Herring
2019-02-03 21:42 ` [PATCH 14/17 v1] gpio: ixp4xx: Add OF probing support Linus Walleij
2019-02-06 16:13   ` Bartosz Golaszewski
2019-02-21  8:55     ` Linus Walleij
2019-02-03 21:42 ` [PATCH 15/17 v1] ARM: ixp4xx: Add DT bindings Linus Walleij
2019-02-04 15:16   ` Rob Herring
2019-02-08 19:37     ` Linus Walleij
2019-02-03 21:42 ` [PATCH 16/17 v1] ARM: ixp4xx: Add device tree boot support Linus Walleij
2019-02-03 21:42 ` [PATCH 17/17 v1] RFC: ARM: dts: Add some initial IXP4xx device trees Linus Walleij

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAMpxmJWRH9+fbwyhjHe-ERjQfMCXTGzOLWeq1HJbbYXsZ14nXA@mail.gmail.com \
    --to=bgolaszewski@baylibre.com \
    --cc=arnd@arndb.de \
    --cc=kaloz@openwrt.org \
    --cc=khalasa@piap.pl \
    --cc=linus.walleij@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=olof@lixom.net \
    --cc=tharvey@gateworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).