Hi Andy Shevchenko,

Thanks for the review.

Accepted comments will address in V3 and Added few comments in inline.

-----Original Message-----
From: Andy Shevchenko <redacted>
Sent: Thursday, July 23, 2020 11:33 PM
To: Srinivas Neeli <redacted>
Cc: Linus Walleij <redacted>; Bartosz Golaszewski
[off-list ref]; Michal Simek [off-list ref];
Shubhrajyoti Datta [off-list ref]; Srinivas Goud
[off-list ref]; open list:GPIO SUBSYSTEM <linux-
gpio@vger.kernel.org>; linux-arm Mailing List <linux-arm-
kernel@lists.infradead.org>; Linux Kernel Mailing List <linux-
kernel@vger.kernel.org>; git [off-list ref]
Subject: Re: [PATCH V2 2/3] gpio: xilinx: Add interrupt support

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

quoted

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.

...

quoted

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

Not sure I see a user of it.

...

we are using chained_irq_enter() and chained_irq_exit()
APIs , so need "chained_irq.h"

quoted

+/**
+ * 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?

...

quoted

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

quoted

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

quoted

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

...

quoted

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

quoted

+ */
+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);
+

quoted

+       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);

quoted

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

Under spin lock?! Hmm...

quoted

+       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);
+}

...

quoted

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

...

gpio_wdith vary depends on design. We can configure gpio pins for each bank.

quoted

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

...

quoted

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

Here intention is just printing a debug message to user.

quoted

+       } else {

...

quoted

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

I didn't get this , Could you please explain more.

--
With Best Regards,
Andy Shevchenko

Adding Robert Hancock to mail chain ( by mistake suppressed cc list) .

Hi Robert, 
Could you please provide your comments.

Thanks
Srinivas Neeli



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