Re: [PATCH 05/10] gpio: Introduce SAM gpio driver

[Linus Walleij <linus.walleij@linaro.org>]

n Fri, Oct 7, 2016 at 5:18 PM, Pantelis Antoniou
<pantelis.antoniou@konsulko.com> wrote:

> From: Guenter Roeck <groeck@juniper.net>
>
> The SAM GPIO IP block is present in the Juniper PTX series
> of routers as part of the SAM FPGA.
>
> Signed-off-by: Georgi Vlaev <gvlaev@juniper.net>
> Signed-off-by: Guenter Roeck <groeck@juniper.net>
> Signed-off-by: Rajat Jain <rajatjain@juniper.net>
> [Ported from Juniper kernel]
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

First copy/paste my other review comments on the previous driver
I reviewed, this seems to have pretty much all the same issues.

> +config GPIO_SAM
> +       tristate "SAM FPGA GPIO"
> +       depends on MFD_JUNIPER_SAM
> +       default y if MFD_JUNIPER_SAM

I suspect this should use
select GPIOLIB_IRQCHIP

> +#include <linux/kernel.h>
> +#include <linux/init.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>

<linux/gpio/driver.h>

> +#include <linux/interrupt.h>
> +#include <linux/irqdomain.h>

Not needed with GPIOLIB_IRQCHIP

> +#include <linux/errno.h>
> +#include <linux/of_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/sched.h>

Why?

> +#include <linux/mfd/sam.h>
> +
> +/* gpio status/configuration */
> +#define SAM_GPIO_NEG_EDGE      (1 << 8)
> +#define SAM_GPIO_NEG_EDGE_EN   (1 << 7)
> +#define SAM_GPIO_POS_EDGE      (1 << 6)
> +#define SAM_GPIO_POS_EDGE_EN   (1 << 5)

Interrupt triggers I suppose.

> +#define SAM_GPIO_BLINK         (1 << 4)

Cool, we don't support that in gpiolib as of now.
Maybe we should.

> +#define SAM_GPIO_OUT           (1 << 3)
> +#define SAM_GPIO_OUT_TS                (1 << 2)

OUT_TS ... what does TS mean here?

> +#define SAM_GPIO_DEBOUNCE_EN   (1 << 1)
> +#define SAM_GPIO_IN            (1 << 0)
> +
> +#define SAM_GPIO_BASE          0x1000

Base into what, and why is this not coming from PCI
or the device tree?

> +#define SAM_MAX_NGPIO          512

Why do we need to know this and what does it really mean?
That is the max number the GPIO subsystem can handle by
the way.

> +#define SAM_GPIO_ADDR(addr, nr)        ((addr) + SAM_GPIO_BASE + (nr) * sizeof(u32))

Why can't we just offset the address earlier, ah well it's OK.

> +struct sam_gpio_irq_group {
> +       int start;              /* 1st gpio pin */
> +       int count;              /* # of pins in group */
> +       int num_enabled;        /* # of enabled interrupts */
> +};
> +
> +/**
> + * struct sam_gpio - GPIO private data structure.
> + * @base:                      PCI base address of Memory mapped I/O register.
> + * @dev:                       Pointer to device structure.
> + * @gpio:                      Data for GPIO infrastructure.
> + * @gpio_base:                 1st gpio pin
> + * @gpio_count:                        # of gpio pins
> + * @irq_lock:                  Lock used by interrupt subsystem
> + * @domain:                    Pointer to interrupt domain
> + * @irq:                       Interrupt # from parent
> + * @irq_high:                  Second interrupt # from parent
> + *                             (currently unused)
> + * @irq_group:                 Interrupt group descriptions
> + *                             (one group per interrupt bit)
> + * @irq_type:                  The interrupt type for each gpio pin
> + */

Why do you need to keep all of this around? Is is all really
used? gpio_base makes me nervous we generally use dynamic
allocation of GPIO numbers these days.

> +struct sam_gpio {
> +       void __iomem *base;
> +       struct device *dev;
> +       struct gpio_chip gpio;
> +       int gpio_base;
> +       int gpio_count;
> +       struct mutex irq_lock;
> +       struct irq_domain *domain;
> +       int irq;
> +       int irq_high;
> +       struct sam_gpio_irq_group irq_group[18];
> +       u8 irq_type[SAM_MAX_NGPIO];
> +       struct sam_platform_data *pdata;
> +       const char **names;
> +       u32 *export_flags;
> +};
> +#define to_sam(chip)   container_of((chip), struct sam_gpio, gpio)

Instead of this use gpiochip_get_data(). Applies everywhere.

> +static void sam_gpio_bitop(struct sam_gpio *sam, unsigned int nr,
> +                          u32 bit, bool set)
> +{
> +       u32 reg;
> +
> +       reg = ioread32(SAM_GPIO_ADDR(sam->base, nr));
> +       if (set)
> +               reg |= bit;
> +       else
> +               reg &= ~bit;
> +       iowrite32(reg, SAM_GPIO_ADDR(sam->base, nr));
> +       ioread32(SAM_GPIO_ADDR(sam->base, nr));
> +}

Does that rally need a helper function?

Use BIT() and inline like I explained in the previous patch.

> +static void sam_gpio_setup(struct sam_gpio *sam)
> +{
> +       struct gpio_chip *chip = &sam->gpio;
> +
> +       chip->parent = sam->dev;
> +       chip->label = dev_name(sam->dev);
> +       chip->owner = THIS_MODULE;
> +       chip->direction_input = sam_gpio_direction_input;
> +       chip->get = sam_gpio_get;
> +       chip->direction_output = sam_gpio_direction_output;

Implement also chip->get_direction

> +       chip->set = sam_gpio_set;
> +       chip->set_debounce = sam_gpio_debounce;
> +       chip->dbg_show = NULL;
> +       chip->base = sam->gpio_base;

Oh no, why. Use -1 please and let gpiolib decide...

> +       chip->ngpio = sam->gpio_count;
> +#ifdef CONFIG_OF_GPIO
> +       chip->of_node = sam->dev->of_node;
> +#endif

I doubt this #ifdef actually. If the driver needs CONFIG_OF_GPIO to
work it should just depend on it in Kconfig.

> +static int sam_of_get_exports(struct device *dev, struct sam_gpio *sam)
> +{
> +       struct device_node *child, *exports;
> +       int err = 0;
> +
> +       if (dev->of_node == NULL)
> +               return 0;       /* No FDT node, we are done */
> +
> +       exports = of_get_child_by_name(dev->of_node, "gpio-exports");
> +       if (exports == NULL)
> +               return 0;       /* No exports, we are done */
> +
> +       if (of_get_child_count(exports) == 0)
> +               return 0;       /* No children, we are done */
> +
> +       sam->names = devm_kzalloc(dev, sizeof(char *) * sam->gpio_count,
> +                                 GFP_KERNEL);
> +       if (sam->names == NULL) {
> +               err = -ENOMEM;
> +               goto error;
> +       }
> +       sam->export_flags =
> +               devm_kzalloc(dev, sizeof(u32) * sam->gpio_count, GFP_KERNEL);
> +       if (sam->export_flags == NULL) {
> +               err = -ENOMEM;
> +               goto error;
> +       }
> +       for_each_child_of_node(exports, child) {
> +               const char *label;
> +               u32 pin, flags;
> +
> +               label = of_get_property(child, "label", NULL) ? : child->name;
> +               err = of_property_read_u32_index(child, "pin", 0, &pin);
> +               if (err)
> +                       break;
> +               if (pin >= sam->gpio_count) {
> +                       err = -EINVAL;
> +                       break;
> +               }
> +               err = of_property_read_u32_index(child, "pin", 1, &flags);
> +               if (err)
> +                       break;
> +               /*
> +                * flags:
> +                * GPIOF_DIR_IN                 bit 0=1
> +                * GPIOF_DIR_OUT                bit 0=0
> +                *      GPIOF_INIT_HIGH         bit 1=1
> +                * GPIOF_ACTIVE_LOW             bit 2=1
> +                * GPIOF_OPEN_DRAIN             bit 3=1
> +                * GPIOF_OPEN_SOURCE            bit 4=1
> +                * GPIOF_EXPORT                 bit 5=1
> +                * GPIOF_EXPORT_CHANGEABLE      bit 6=1
> +                */
> +               sam->names[pin] = label;
> +               sam->export_flags[pin] = flags;
> +       }
> +error:
> +       of_node_put(exports);
> +       return err;
> +}

What? NAK never in my life. This looks like an old hack to
export stuff to userspace. We don't do that. The kernel supports
gpio-line-names to name lines in the device tree, and you can use
the new chardev ABI to access it from userspace, sysfs is dead.

Delete this function entirely.

> +static int sam_gpio_of_init(struct device *dev, struct sam_gpio *sam)
> +{
> +       int err;
> +       u32 val;
> +       const u32 *igroup;
> +       u32 group, start, count;
> +       int i, iglen, ngpio;
> +
> +       if (of_have_populated_dt() && !dev->of_node) {
> +               dev_err(dev, "No device node\n");
> +               return -ENODEV;
> +       }

So obviously this driver Kconfig should depend on OF_GPIO.

> +
> +       err = of_property_read_u32(dev->of_node, "gpio-base", &val);
> +       if (err)
> +               val = -1;
> +       sam->gpio_base = val;

NAK, No Linux bases in the device tree. Only use -1.

> +       err = of_property_read_u32(dev->of_node, "gpio-count", &val);
> +       if (!err) {
> +               if (val > SAM_MAX_NGPIO)
> +                       val = SAM_MAX_NGPIO;
> +               sam->gpio_count = val;
> +       }

As described in the generic bindings, use "ngpios" for this if you need it.

> +       igroup = of_get_property(dev->of_node, "gpio-interrupts", &iglen);

NAK on that binding.

> +       if (igroup) {
> +               iglen /= sizeof(u32);
> +               if (iglen < 3 || iglen % 3)
> +                       return -EINVAL;
> +               iglen /= 3;
> +               for (i = 0; i < iglen; i++) {
> +                       group = be32_to_cpu(igroup[i * 3]);
> +                       if (group >= ARRAY_SIZE(sam->irq_group))
> +                               return -EINVAL;
> +                       start = be32_to_cpu(igroup[i * 3 + 1]);
> +                       count = be32_to_cpu(igroup[i * 3 + 2]);
> +                       if (start >= sam->gpio_count || count == 0 ||
> +                           start + count > sam->gpio_count)
> +                               return -EINVAL;
> +                       sam->irq_group[group].start = start;
> +                       sam->irq_group[group].count = count;
> +               }
> +       }

Do not invent custom interrupt bindings like this. Use the
standard device tree mechanism to resolve IRQs from the parent
controller. Maybe you also need to use hierarchical irqdomain
in Linux.

> +static int sam_gpio_pin_to_irq_bit(struct sam_gpio *sam, int pin)
> +{
> +       int bit;
> +
> +       for (bit = 0; bit < ARRAY_SIZE(sam->irq_group); bit++) {
> +               struct sam_gpio_irq_group *irq_group = &sam->irq_group[bit];
> +
> +               if (irq_group->count &&
> +                   pin >= irq_group->start &&
> +                   pin <= irq_group->start + irq_group->count)
> +                       return bit;
> +       }
> +       return -EINVAL;
> +}
> +
> +static bool sam_gpio_irq_handle_group(struct sam_gpio *sam,
> +                                     struct sam_gpio_irq_group *irq_group)
> +{
> +       unsigned int virq = 0;
> +       bool handled = false;
> +       bool repeat;
> +       int i;
> +
> +       /* no irq_group for the interrupt bit */
> +       if (!irq_group->count)
> +               return false;
> +
> +       WARN_ON(irq_group->num_enabled == 0);
> +       do {
> +               repeat = false;
> +               for (i = 0; i < irq_group->count; i++) {
> +                       int pin = irq_group->start + i;
> +                       bool low, high;
> +                       u32 regval;
> +                       u8 type;
> +
> +                       regval = ioread32(SAM_GPIO_ADDR(sam->base, pin));
> +                       /*
> +                        * write back status to clear POS_EDGE and NEG_EDGE
> +                        * status for this GPIO pin (status bits are
> +                        * clear-on-one). This is necessary to clear the
> +                        * high level interrupt status.
> +                        * Also consider the interrupt to be handled in that
> +                        * case, even if there is no taker.
> +                        */
> +                       if (regval & (SAM_GPIO_POS_EDGE | SAM_GPIO_NEG_EDGE)) {
> +                               iowrite32(regval,
> +                                         SAM_GPIO_ADDR(sam->base, pin));
> +                               ioread32(SAM_GPIO_ADDR(sam->base, pin));
> +                               handled = true;
> +                       }
> +
> +                       /*
> +                        * Check if the pin changed its state.
> +                        * If it did, and if the expected condition applies,
> +                        * generate a virtual interrupt.
> +                        * A pin can only generate an interrupt if
> +                        * - interrupts are enabled for it
> +                        * - it is configured as input
> +                        */
> +
> +                       if (!sam->irq_type[pin])
> +                               continue;
> +                       if (!(regval & SAM_GPIO_OUT_TS))
> +                               continue;
> +
> +                       high = regval & (SAM_GPIO_IN | SAM_GPIO_POS_EDGE);
> +                       low = !(regval & SAM_GPIO_IN) ||
> +                               (regval & SAM_GPIO_NEG_EDGE);
> +                       type = sam->irq_type[pin];
> +                       if (((type & IRQ_TYPE_EDGE_RISING) &&
> +                            (regval & SAM_GPIO_POS_EDGE)) ||
> +                           ((type & IRQ_TYPE_EDGE_FALLING) &&
> +                            (regval & SAM_GPIO_NEG_EDGE)) ||
> +                           ((type & IRQ_TYPE_LEVEL_LOW) && low) ||
> +                           ((type & IRQ_TYPE_LEVEL_HIGH) && high)) {


This if() clause does not look sane, you have to see that.
If you think this is proper then explain with detailed comments
what is going on.

> +                               virq = irq_find_mapping(sam->domain, pin);
> +                               handle_nested_irq(virq);
> +                               if (type & (IRQ_TYPE_LEVEL_LOW
> +                                           | IRQ_TYPE_LEVEL_HIGH))
> +                                       repeat = true;
> +                       }
> +               }
> +               schedule();

Why? Voluntary preemption? Just use a threaded interrupt handler
instead.

> +       } while (repeat);
> +
> +       return handled;
> +}
> +
> +static irqreturn_t sam_gpio_irq_handler(int irq, void *data)
> +{
> +       struct sam_gpio *sam = data;
> +       struct sam_platform_data *pdata = sam->pdata;
> +       irqreturn_t ret = IRQ_NONE;
> +       bool handled;
> +       u32 status;
> +
> +       do {
> +               handled = false;
> +               status = pdata->irq_status(sam->dev->parent, SAM_IRQ_GPIO,
> +                                          sam->irq);
> +               pdata->irq_status_clear(sam->dev->parent, SAM_IRQ_GPIO,
> +                                       sam->irq, status);
> +               while (status) {
> +                       unsigned int bit;
> +
> +                       bit = __ffs(status);
> +                       status &= ~(1 << bit);
> +                       handled =
> +                         sam_gpio_irq_handle_group(sam, &sam->irq_group[bit]);
> +                       if (handled)
> +                               ret = IRQ_HANDLED;
> +               }
> +       } while (handled);

This handled business looks fragile. But OK.

It is a simple IRQ handler, this driver should definately use
GPIOLIB_IRQCHIP. Please look at other drivers doing that
for inspiration. It is also well described in
Documenation/gpio/driver.txt

> +static int sam_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct sam_gpio *sam = to_sam(chip);
> +
> +       return irq_create_mapping(sam->domain, offset);
> +}

With GPIOLIB_IRQCHIP you do not need to implement .to_irq()

> +static void sam_irq_mask(struct irq_data *data)
> +{
> +       struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +       struct sam_platform_data *pdata = sam->pdata;
> +       int bit = sam_gpio_pin_to_irq_bit(sam, data->hwirq);
> +
> +       if (bit < 0)
> +               return;
> +
> +       if (--sam->irq_group[bit].num_enabled <= 0) {
> +               pdata->disable_irq(sam->dev->parent, SAM_IRQ_GPIO, sam->irq,
> +                                  1 << bit);

Just BIT(bit)

> +static void sam_irq_unmask(struct irq_data *data)
> +{
> +       struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +       struct sam_platform_data *pdata = sam->pdata;
> +       int bit = sam_gpio_pin_to_irq_bit(sam, data->hwirq);
> +
> +       if (bit < 0)
> +               return;

Do you expect this to happen a lot? Else just delete the check or print
an error message.

> +
> +       sam->irq_group[bit].num_enabled++;
> +       pdata->enable_irq(sam->dev->parent, SAM_IRQ_GPIO, sam->irq, 1 << bit);

Dito.

> +static int sam_irq_set_type(struct irq_data *data, unsigned int type)
> +{
> +       struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +       int bit = sam_gpio_pin_to_irq_bit(sam, data->hwirq);
> +
> +       if (bit < 0)
> +               return bit;
> +
> +       sam->irq_type[data->hwirq] = type & 0x0f;


Why storing this and going to all that trouble?

> +       sam_gpio_bitop(sam, data->hwirq, SAM_GPIO_OUT_TS, true);
> +       sam_gpio_bitop(sam, data->hwirq, SAM_GPIO_DEBOUNCE_EN, type & 0x10);
> +       sam_gpio_bitop(sam, data->hwirq,
> +                      SAM_GPIO_POS_EDGE_EN | SAM_GPIO_POS_EDGE,
> +                      type & (IRQ_TYPE_EDGE_RISING | IRQ_TYPE_LEVEL_HIGH));
> +       sam_gpio_bitop(sam, data->hwirq,
> +                      SAM_GPIO_NEG_EDGE_EN | SAM_GPIO_NEG_EDGE,
> +                      type & (IRQ_TYPE_EDGE_FALLING | IRQ_TYPE_LEVEL_LOW));
> +
> +       return 0;
> +}

Just set up different handlers depending on edge. This likely simplifies your
IRQ handler code as well, as a special function (.irq_ack()) gets called for
edge IRQs.

Look how drivers/gpio/gpio-pl061.c does it.

> +static void sam_irq_bus_lock(struct irq_data *data)
> +{
> +       struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +
> +       mutex_lock(&sam->irq_lock);
> +}
> +
> +static void sam_irq_bus_unlock(struct irq_data *data)
> +{
> +       struct sam_gpio *sam = irq_data_get_irq_chip_data(data);
> +
> +       /* Synchronize interrupts to chip */
> +
> +       mutex_unlock(&sam->irq_lock);
> +}

Aha OK if it's necessary then we need to do this.

> +static struct irq_chip sam_irq_chip = {
> +       .name = "gpio-sam",

Maybe this should have an instance number or something.
Just an idea no requirement.

> +       .irq_mask = sam_irq_mask,
> +       .irq_unmask = sam_irq_unmask,

So I think this needs .irq_ack() to solve the mess above in
the interrupt handler.

> +       .irq_set_type = sam_irq_set_type,
> +       .irq_bus_lock = sam_irq_bus_lock,
> +       .irq_bus_sync_unlock = sam_irq_bus_unlock,
> +};
> +
> +static int sam_gpio_irq_map(struct irq_domain *domain, unsigned int irq,
> +                           irq_hw_number_t hwirq)
> +{
> +       irq_set_chip_data(irq, domain->host_data);
> +       irq_set_chip(irq, &sam_irq_chip);
> +       irq_set_nested_thread(irq, true);
> +
> +       irq_set_noprobe(irq);
> +
> +       return 0;
> +}

This will not be needed if you use GPIOLIB_IRQCHIP

> +static const struct irq_domain_ops sam_gpio_irq_domain_ops = {
> +       .map = sam_gpio_irq_map,
> +       .xlate = irq_domain_xlate_twocell,
> +};

Nor this.

> +static int sam_gpio_irq_setup(struct device *dev, struct sam_gpio *sam)
> +{
> +       int ret;
> +
> +       sam->domain = irq_domain_add_linear(dev->of_node,
> +                                           sam->gpio_count,
> +                                           &sam_gpio_irq_domain_ops,
> +                                           sam);
> +       if (sam->domain == NULL)
> +               return -ENOMEM;

Nor this.

> +       ret = devm_request_threaded_irq(dev, sam->irq, NULL,
> +                                       sam_gpio_irq_handler,
> +                                       IRQF_ONESHOT,
> +                                       dev_name(dev), sam);
> +       if (ret)
> +               goto out_remove_domain;

Are you not setting .can_sleep on the gpiochip?

> +
> +       sam->gpio.to_irq = sam_gpio_to_irq;
> +
> +       if (!try_module_get(dev->parent->driver->owner)) {
> +               ret = -EINVAL;
> +               goto out_remove_domain;
> +       }

Why? Is that the MFD device? Isn't the device core
handling this?

> +static int sam_gpio_unexport(struct sam_gpio *sam)
> +{
> +       int i;
> +
> +       if (!sam->export_flags)
> +               return 0;
> +
> +       /* un-export all auto-exported pins */
> +       for (i = 0; i < sam->gpio_count; i++) {
> +               struct gpio_desc *desc = gpio_to_desc(sam->gpio.base + i);
> +
> +               if (desc == NULL)
> +                       continue;
> +
> +               if (sam->export_flags[i] & GPIOF_EXPORT)
> +                       gpiochip_free_own_desc(desc);
> +       }
> +       return 0;
> +}
> +
> +static int sam_gpio_export(struct sam_gpio *sam)
> +{
> +       int i, ret;
> +
> +       if (!sam->export_flags)
> +               return 0;
> +
> +       /* auto-export pins as requested */
> +
> +       for (i = 0; i < sam->gpio_count; i++) {
> +               u32 flags = sam->export_flags[i];
> +               struct gpio_desc *desc;
> +
> +               /* request and initialize exported pins */
> +               if (!(flags & GPIOF_EXPORT))
> +                       continue;
> +
> +               desc  = gpiochip_request_own_desc(&sam->gpio, i, "sam-export");
> +               if (IS_ERR(desc)) {
> +                       ret = PTR_ERR(desc);
> +                       goto error;
> +               }
> +               if (flags & GPIOF_DIR_IN) {
> +                       ret = gpiod_direction_input(desc);
> +                       if (ret)
> +                               goto error;
> +               } else {
> +                       ret = gpiod_direction_output(desc, flags &
> +                                                   (GPIOF_OUT_INIT_HIGH |
> +                                                    GPIOF_ACTIVE_LOW));
> +                       if (ret)
> +                               goto error;
> +               }
> +               ret = gpiod_export(desc, flags & GPIOF_EXPORT_CHANGEABLE);
> +
> +               if (ret)
> +                       goto error;
> +       }
> +       return 0;
> +
> +error:
> +       sam_gpio_unexport(sam);
> +       return ret;
> +}

Just delete this. Use the new chardev ABI to access GPIOs from
userspace as explained earlier.

> +static int sam_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct sam_gpio *sam;
> +       struct resource *res;
> +       int ret;
> +       struct sam_platform_data *pdata = dev_get_platdata(&pdev->dev);
> +
> +       sam = devm_kzalloc(dev, sizeof(*sam), GFP_KERNEL);
> +       if (sam == NULL)
> +               return -ENOMEM;

if (!sam)
  return -ENOMEM;

> +       sam->dev = dev;
> +       sam->pdata = pdata;
> +       platform_set_drvdata(pdev, sam);
> +
> +       res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +       if (!res)
> +               return -ENODEV;
> +
> +       sam->irq = platform_get_irq(pdev, 0);
> +       sam->irq_high = platform_get_irq(pdev, 1);
> +
> +       sam->base = devm_ioremap_nocache(dev, res->start, resource_size(res));
> +       if (!sam->base)
> +               return -ENOMEM;
> +
> +       mutex_init(&sam->irq_lock);
> +
> +       ret = sam_gpio_of_init(dev, sam);
> +       if (ret)
> +               return ret;
> +
> +       sam_gpio_setup(sam);
> +
> +       if (pdata && sam->irq >= 0 && of_find_property(dev->of_node,
> +                                             "interrupt-controller", NULL)) {
> +               ret = sam_gpio_irq_setup(dev, sam);
> +               if (ret < 0)
> +                       return ret;
> +       }

This is fair, but do it after adding the gpiochip and use GPIOLIB_IRQCHIP
accessors gpiochip_irqchip_add() and gpiochip_set_chained_irqchip().

Yours,
Linus Walleij