On Thu, Jul 23, 2020 at 5:08 PM Srinivas Neeli
[off-list ref] wrote:

Adds interrupt support to the Xilinx GPIO driver so that rising and
falling edge line events can be supported. Since interrupt support is
an optional feature in the Xilinx IP, the driver continues to support
devices which have no interrupt provided.

...

+#include <linux/irqchip/chained_irq.h>

Not sure I see a user of it.

...

+/**
+ * xgpio_xlate - Translate gpio_spec to the GPIO number and flags
+ * @gc: Pointer to gpio_chip device structure.
+ * @gpiospec:  gpio specifier as found in the device tree
+ * @flags: A flags pointer based on binding
+ *
+ * Return:
+ * irq number otherwise -EINVAL
+ */
+static int xgpio_xlate(struct gpio_chip *gc,
+                      const struct of_phandle_args *gpiospec, u32 *flags)
+{
+       if (gc->of_gpio_n_cells < 2) {
+               WARN_ON(1);
+               return -EINVAL;
+       }
+
+       if (WARN_ON(gpiospec->args_count < gc->of_gpio_n_cells))
+               return -EINVAL;
+
+       if (gpiospec->args[0] >= gc->ngpio)
+               return -EINVAL;
+
+       if (flags)
+               *flags = gpiospec->args[1];
+
+       return gpiospec->args[0];
+}

This looks like a very standart xlate function for GPIO. Why do you
need to open-code it?

...

+/**
+ * xgpio_irq_ack - Acknowledge a child GPIO interrupt.

+ * This currently does nothing, but irq_ack is unconditionally called by
+ * handle_edge_irq and therefore must be defined.

This should go after parameter description(s).

+ * @irq_data: per irq and chip data passed down to chip functions
+ */

...

 /**
+ * xgpio_irq_mask - Write the specified signal of the GPIO device.
+ * @irq_data: per irq and chip data passed down to chip functions

In all comments irq -> IRQ.

+ */
+static void xgpio_irq_mask(struct irq_data *irq_data)
+{
+       unsigned long flags;
+       struct xgpio_instance *chip = irq_data_get_irq_chip_data(irq_data);
+       int irq_offset = irqd_to_hwirq(irq_data);
+       int index = xgpio_index(chip, irq_offset);
+       int offset = xgpio_offset(chip, irq_offset);
+
+       spin_lock_irqsave(&chip->gpio_lock, flags);
+

+       chip->irq_enable[index] &= ~BIT(offset);

If you convert your data structure to use bitmaps (and respective API) like

#define XILINX_NGPIOS  64
...
  DECLARE_BITMAP(irq_enable, XILINX_NGPIOS);
...

it will make code better to read and understand. For example, here it
will be just
__clear_bit(offset, chip->irq_enable);

+       dev_dbg(chip->gc.parent, "Disable %d irq, irq_enable_mask 0x%x\n",
+               irq_offset, chip->irq_enable[index]);

Under spin lock?! Hmm...

+       if (!chip->irq_enable[index]) {
+               /* Disable per channel interrupt */
+               u32 temp = xgpio_readreg(chip->regs + XGPIO_IPIER_OFFSET);
+
+               temp &= ~BIT(index);
+               xgpio_writereg(chip->regs + XGPIO_IPIER_OFFSET, temp);
+       }
+       spin_unlock_irqrestore(&chip->gpio_lock, flags);
+}

...

+       for (index = 0; index < num_channels; index++) {
+               if ((status & BIT(index))) {

If gpio_width is the same among banks, you can use for_each_set_bit()
here as well.

...

+                       for_each_set_bit(bit, &all_events, 32) {
+                               generic_handle_irq(irq_find_mapping
+                                       (chip->gc.irq.domain, offset + bit));

Strange indentation. Maybe a temporary variable helps?

...

+       chip->irq = platform_get_irq_optional(pdev, 0);
+       if (chip->irq <= 0) {
+               dev_info(&pdev->dev, "GPIO IRQ not set\n");

Why do you need an optional variant if you print an error anyway?

+       } else {

...

+               chip->gc.irq.parents = (unsigned int *)&chip->irq;
+               chip->gc.irq.num_parents = 1;

Current pattern is to use devm_kcalloc() for it (Linus has plans to
simplify this in the future and this will help him to find what
patterns are being used)

-- 
With Best Regards,
Andy Shevchenko

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