On Fri, 2014-12-05 at 18:14 -0800, Ray Jui wrote:

On 12/5/2014 5:28 PM, Joe Perches wrote:

quoted

On Fri, 2014-12-05 at 16:40 -0800, Ray Jui wrote:

quoted

+static void bcm_cygnus_gpio_irq_handler(unsigned int irq,
+		struct irq_desc *desc)
+{
+	struct bcm_cygnus_gpio *cygnus_gpio;
+	struct irq_chip *chip = irq_desc_get_chip(desc);
+	int i, bit;
+
+	chained_irq_enter(chip, desc);
+
+	cygnus_gpio = irq_get_handler_data(irq);
+
+	/* go through the entire GPIO banks and handle all interrupts */
+	for (i = 0; i < cygnus_gpio->num_banks; i++) {
+		unsigned long val = readl(cygnus_gpio->base +
+				(i * GPIO_BANK_SIZE) +
+				CYGNUS_GPIO_INT_MSTAT_OFFSET);
+		if (val) {

This if (val) and indentation isn't really necessary

Note for_each_set_bit in this case iterates 32 times searching for bits 
that are set.

No it doesn't.

#define for_each_set_bit(bit, addr, size) \
	for ((bit) = find_first_bit((addr), (size));		\
	     (bit) < (size);					\
	     (bit) = find_next_bit((addr), (size), (bit) + 1))

find_first_bit:

 * Returns the bit number of the first set bit.
 * If no bits are set, returns @size.

 By having the if (val) check here, it can potentially save 
some of such processing in the ISR. I agree with you that it introduces 
one extra indent here but I think it's required.

quoted

quoted

+			for_each_set_bit(bit, &val, 32) {

for_each_set_bit will effectively do the if above.

32 bit only code?
otherwise isn't this endian unsafe?

Will change 'unsigned long val' to 'u32 val'.

All the bit operations only work on long *