gpio: dwapb: Clean up the driver and a fix

Convert to use irqd_to_hwirq() instead of direct access to the hwirq member. Also amend the type of the hwirq holding variable to be irq_hw_number_t.

Cc Serge Semin <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Tested-by Serge Semin <[email protected]>

---
 drivers/gpio/gpio-dwapb.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

drivers/gpio/gpio-dwapb.c +9 -7

index c0c267cddd80..2edccc5bbc25 100644
@@ -194,7 +194,7 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
 {
 	unsigned long irq_status;
-	int hwirq;
+	irq_hw_number_t hwirq;
 
 	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
 	for_each_set_bit(hwirq, &irq_status, 32) {

@@ -230,7 +230,7 @@ static void dwapb_irq_enable(struct irq_data *d)
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
 	val = dwapb_read(gpio, GPIO_INTEN);
-	val |= BIT(d->hwirq);
+	val |= BIT(irqd_to_hwirq(d));
 	dwapb_write(gpio, GPIO_INTEN, val);
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }

@@ -245,7 +245,7 @@ static void dwapb_irq_disable(struct irq_data *d)
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);
 	val = dwapb_read(gpio, GPIO_INTEN);
-	val &= ~BIT(d->hwirq);
+	val &= ~BIT(irqd_to_hwirq(d));
 	dwapb_write(gpio, GPIO_INTEN, val);
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }

@@ -255,7 +255,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
 	struct gpio_chip *gc = &gpio->ports[0].gc;
-	int bit = d->hwirq;
+	irq_hw_number_t bit = irqd_to_hwirq(d);
 	unsigned long level, polarity, flags;
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |

@@ -305,11 +305,12 @@ static int dwapb_irq_set_wake(struct irq_data *d, unsigned int enable)
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
 	struct dwapb_gpio *gpio = igc->private;
 	struct dwapb_context *ctx = gpio->ports[0].ctx;
+	irq_hw_number_t bit = irqd_to_hwirq(d);
 
 	if (enable)
-		ctx->wake_en |= BIT(d->hwirq);
+		ctx->wake_en |= BIT(bit);
 	else
-		ctx->wake_en &= ~BIT(d->hwirq);
+		ctx->wake_en &= ~BIT(bit);
 
 	return 0;
 }

@@ -365,8 +366,9 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	struct gpio_chip *gc = &port->gc;
 	struct fwnode_handle  *fwnode = pp->fwnode;
 	struct irq_chip_generic	*irq_gc = NULL;
-	unsigned int hwirq, ngpio = gc->ngpio;
+	unsigned int ngpio = gc->ngpio;
 	struct irq_chip_type *ct;
+	irq_hw_number_t hwirq;
 	int err, i;
 
 	gpio->domain = irq_domain_create_linear(fwnode, ngpio,

Signature · 1 line

2.25.1

IRQ core provides macros such as IRQ_RETVAL().
Convert code to use them.

Signed-off-by Andy Shevchenko <[email protected]>

Tested-by Serge Semin <[email protected]>

Reviewed-by Serge Semin <[email protected]>

---
 drivers/gpio/gpio-dwapb.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

drivers/gpio/gpio-dwapb.c +1 -2

index 8b30ded9322a..4edac592c253 100644
@@ -258,8 +258,7 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 	irq_hw_number_t bit = irqd_to_hwirq(d);
 	unsigned long level, polarity, flags;
 
-	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
-		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
+	if (type & ~IRQ_TYPE_SENSE_MASK)
 		return -EINVAL;
 
 	spin_lock_irqsave(&gc->bgpio_lock, flags);

@@ -351,12 +350,7 @@ static int dwapb_gpio_set_config(struct gpio_chip *gc, unsigned offset,
 

drivers/gpio/gpio-dwapb.c +0 -0

 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,

Signature · 1 line

2.25.1

Quoted 2 lines · Andy Shevchenko

IRQ core provides macros such as IRQ_RETVAL().
Convert code to use them.

BTW Forgot to mention. Irrelevantly to this patch just so you know seeing you are from Intel and this part is being utilized by the Intel Quark SoC. dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch is applied and the FULL-RT scheduler is enabled all interrupt handlers specified by request_irq()-based methods will be handled by a kernel thread, while generic_handle_irq() is supposed to be called from the atomic context only (with interrupts disabled). As a result an ugly stack dump will be printed to the kernel log by the next code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152

A way to fix this is described in Documentation/driver-api/gpio/driver.rst

Quoted 15 lines · Serge Semin

Quoted 2 lines · Andy Shevchenko

IRQ core provides macros such as IRQ_RETVAL().
Convert code to use them.

BTW Forgot to mention. Irrelevantly to this patch just so you know seeing you are from Intel and this part is being utilized by the Intel Quark SoC. dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch is applied and the FULL-RT scheduler is enabled all interrupt handlers specified by request_irq()-based methods will be handled by a kernel thread, while generic_handle_irq() is supposed to be called from the atomic context only (with interrupts disabled). As a result an ugly stack dump will be printed to the kernel log by the next code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152

Quoted 1 line

A way to fix this is described in Documentation/driver-api/gpio/driver.rst

There is patch from Siemens to fix that [1]. I dunno if they are going to upstream it. Jan?

[1]: https://github.com/siemens/meta-iot2000/blob/master/meta-iot2000-bsp/recipes-kernel/linux/patches/rt-0002-gpio-dwapb-Work-around-RT-full-s-enforced-IRQ-thread.patch

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Serge Semin

Quoted 2 lines · Andy Shevchenko

IRQ core provides macros such as IRQ_RETVAL().
Convert code to use them.

BTW Forgot to mention. Irrelevantly to this patch just so you know seeing you are from Intel and this part is being utilized by the Intel Quark SoC. dwapb_irq_handler_mfd() handler will cause a problem in RT-patched kernel (I've seen such issue in another GPIO-driver). So if PREEMP_RT_FULL patch is applied and the FULL-RT scheduler is enabled all interrupt handlers specified by request_irq()-based methods will be handled by a kernel thread, while generic_handle_irq() is supposed to be called from the atomic context only (with interrupts disabled). As a result an ugly stack dump will be printed to the kernel log by the next code:
https://elixir.bootlin.com/linux/latest/source/kernel/irq/handle.c#L152

Quoted 6 lines

Quoted 1 line

A way to fix this is described in Documentation/driver-api/gpio/driver.rst

There is patch from Siemens to fix that [1]. I dunno if they are going to upstream it. Jan?

[1]: https://github.com/siemens/meta-iot2000/blob/master/meta-iot2000-bsp/recipes-kernel/linux/patches/rt-0002-gpio-dwapb-Work-around-RT-full-s-enforced-IRQ-thread.patch

Just to note I wouldn't accept that patch as it is, because it's applied to all types of IRQ handlers supported by the driver. The chained cascaded IRQ handlers won't be converted to the kernel threads in RT-patched kernels [1], so using the wa-lock is redundant in that case. One of the ways to fix it is to have a conditional wa_lock acquisition depended on the irq_shared flag state. Alternatively we could create a dedicated handlers for each types of the IRQs.

[1] Documentation/driver-api/gpio/driver.rst

Regards,
-Sergey

Quoted 6 lines

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

The negative conditionals are harder to parse by reader. Switch to positive one in dwapb_configure_irqs().

Cc Serge Semin <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Tested-by Serge Semin <[email protected]>

---
 drivers/gpio/gpio-dwapb.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

drivers/gpio/gpio-dwapb.c +9 -9

index 31d29ec6ab5c..84c971e0adf0 100644
@@ -413,15 +413,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
 	irq_gc->chip_types[1].handler = handle_edge_irq;
 
-	if (!pp->irq_shared) {
-		int i;
-
-		for (i = 0; i < pp->ngpio; i++) {
-			if (pp->irq[i] >= 0)
-				irq_set_chained_handler_and_data(pp->irq[i],
-						dwapb_irq_handler, gpio);
-		}
-	} else {
+	if (pp->irq_shared) {
 		/*
 		 * Request a shared IRQ since where MFD would have devices
 		 * using the same irq pin

@@ -435,6 +427,14 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 			gpio->domain = NULL;
 			return;
 		}
+	} else {
+		int i;
+
+		for (i = 0; i < pp->ngpio; i++) {
+			if (pp->irq[i] >= 0)
+				irq_set_chained_handler_and_data(pp->irq[i],
+						dwapb_irq_handler, gpio);
+		}
 	}
 
 	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)

Signature · 1 line

2.25.1

Quoted 2 lines · Andy Shevchenko

The negative conditionals are harder to parse by reader. Switch to positive one in dwapb_configure_irqs().

Sorry as for me this modification is redundant. Yes, I know that if-else statement in some cases better to start with positive expression to make it a bit more clear, but in this case I'd leave it as is. First this rule is applicable if both branches are more or less equal, but here I see the most normal case of using the dt-based generic device, which doesn't declare the IRQs as shared seeing it is selected by far more devices at the moment. Second the non-shared IRQs case also covers a combined and multiple-lined GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have only a single IRQ source supported. Finally If the code was like you suggested from the very beginning I wouldn't say a word, but this patch seems to me at least just moving the code around with gaining less than we have at the moment.

Linus, Bartosz and other GPIO-ers may think differently though. Lets see their opinion.

Quoted 17 lines · Serge Semin

Quoted 2 lines · Andy Shevchenko

The negative conditionals are harder to parse by reader. Switch to positive one in dwapb_configure_irqs().

Sorry as for me this modification is redundant. Yes, I know that if-else statement in some cases better to start with positive expression to make it a bit more clear, but in this case I'd leave it as is. First this rule is applicable if both branches are more or less equal, but here I see the most normal case of using the dt-based generic device, which doesn't declare the IRQs as shared seeing it is selected by far more devices at the moment. Second the non-shared IRQs case also covers a combined and multiple-lined GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have only a single IRQ source supported. Finally If the code was like you suggested from the very beginning I wouldn't say a word, but this patch seems to me at least just moving the code around with gaining less than we have at the moment.

Quoted 2 lines

Linus, Bartosz and other GPIO-ers may think differently though. Lets see their opinion.

I think I already applied all patches with the batch application tool b4, without properly checking which patches you reviewed and not, sorry :(

However if any change is controversial I can revert or pull the patch out.

Yours,
Linus Walleij

Quoted 21 lines · Linus Walleij

Quoted 19 lines · Serge Semin

Quoted 2 lines · Andy Shevchenko

The negative conditionals are harder to parse by reader. Switch to positive one in dwapb_configure_irqs().

Sorry as for me this modification is redundant. Yes, I know that if-else statement in some cases better to start with positive expression to make it a bit more clear, but in this case I'd leave it as is. First this rule is applicable if both branches are more or less equal, but here I see the most normal case of using the dt-based generic device, which doesn't declare the IRQs as shared seeing it is selected by far more devices at the moment. Second the non-shared IRQs case also covers a combined and multiple-lined GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have only a single IRQ source supported. Finally If the code was like you suggested from the very beginning I wouldn't say a word, but this patch seems to me at least just moving the code around with gaining less than we have at the moment.

Linus, Bartosz and other GPIO-ers may think differently though. Lets see their opinion.

Quoted 4 lines

I think I already applied all patches with the batch application tool b4, without properly checking which patches you reviewed and not, sorry :(

However if any change is controversial I can revert or pull the patch out.

In this case it's up to you to decide.

Quoted 22 lines · Serge Semin

Quoted 21 lines · Linus Walleij

Quoted 19 lines · Serge Semin

Quoted 2 lines · Andy Shevchenko

The negative conditionals are harder to parse by reader. Switch to positive one in dwapb_configure_irqs().

Sorry as for me this modification is redundant. Yes, I know that if-else statement in some cases better to start with positive expression to make it a bit more clear, but in this case I'd leave it as is. First this rule is applicable if both branches are more or less equal, but here I see the most normal case of using the dt-based generic device, which doesn't declare the IRQs as shared seeing it is selected by far more devices at the moment. Second the non-shared IRQs case also covers a combined and multiple-lined GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have only a single IRQ source supported. Finally If the code was like you suggested from the very beginning I wouldn't say a word, but this patch seems to me at least just moving the code around with gaining less than we have at the moment.

Linus, Bartosz and other GPIO-ers may think differently though. Lets see their opinion.

Quoted 6 lines

Quoted 4 lines

I think I already applied all patches with the batch application tool b4, without properly checking which patches you reviewed and not, sorry :(

However if any change is controversial I can revert or pull the patch out.

In this case it's up to you to decide.

I backed out the last two patches now and kept the rest except 13 and 14.

Quoted 22 lines · Serge Semin

Quoted 21 lines · Linus Walleij

Quoted 19 lines · Serge Semin

Quoted 2 lines · Andy Shevchenko

The negative conditionals are harder to parse by reader. Switch to positive one in dwapb_configure_irqs().

Sorry as for me this modification is redundant. Yes, I know that if-else statement in some cases better to start with positive expression to make it a bit more clear, but in this case I'd leave it as is. First this rule is applicable if both branches are more or less equal, but here I see the most normal case of using the dt-based generic device, which doesn't declare the IRQs as shared seeing it is selected by far more devices at the moment. Second the non-shared IRQs case also covers a combined and multiple-lined GPIO IRQs (chained cascaded GPIO irqchip), while the irq_shared clause have only a single IRQ source supported. Finally If the code was like you suggested from the very beginning I wouldn't say a word, but this patch seems to me at least just moving the code around with gaining less than we have at the moment.

Linus, Bartosz and other GPIO-ers may think differently though. Lets see their opinion.

Quoted 6 lines

Quoted 4 lines

I think I already applied all patches with the batch application tool b4, without properly checking which patches you reviewed and not, sorry :(

However if any change is controversial I can revert or pull the patch out.

In this case it's up to you to decide.

I backed out the last two patches now and kept the rest except 13 and 14.

Yours,
Linus Walleij

In some cases indentation makes code harder to read. Amend indentation in those cases despite of lines go a bit over 80 character limit.

Cc Serge Semin <[email protected]>

Signed-off-by Andy Shevchenko <[email protected]>

Tested-by Serge Semin <[email protected]>

---
 drivers/gpio/gpio-dwapb.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

drivers/gpio/gpio-dwapb.c +5 -6

index 84c971e0adf0..a09332d9c7fe 100644
@@ -437,7 +437,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		}
 	}
 
-	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
+	for (hwirq = 0; hwirq < ngpio; hwirq++)
 		irq_create_mapping(gpio->domain, hwirq);
 
 	port->gc.to_irq = dwapb_gpio_to_irq;

@@ -453,7 +453,7 @@ static void dwapb_irq_teardown(struct dwapb_gpio *gpio)
 	if (!gpio->domain)
 		return;
 
-	for (hwirq = 0 ; hwirq < ngpio ; hwirq++)
+	for (hwirq = 0; hwirq < ngpio; hwirq++)
 		irq_dispose_mapping(irq_find_mapping(gpio->domain, hwirq));
 
 	irq_domain_remove(gpio->domain);

@@ -478,10 +478,9 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 		return -ENOMEM;
 #endif
 
-	dat = gpio->regs + GPIO_EXT_PORTA + (pp->idx * GPIO_EXT_PORT_STRIDE);
-	set = gpio->regs + GPIO_SWPORTA_DR + (pp->idx * GPIO_SWPORT_DR_STRIDE);
-	dirout = gpio->regs + GPIO_SWPORTA_DDR +
-		(pp->idx * GPIO_SWPORT_DDR_STRIDE);
+	dat = gpio->regs + GPIO_EXT_PORTA + pp->idx * GPIO_EXT_PORT_STRIDE;
+	set = gpio->regs + GPIO_SWPORTA_DR + pp->idx * GPIO_SWPORT_DR_STRIDE;
+	dirout = gpio->regs + GPIO_SWPORTA_DDR + pp->idx * GPIO_SWPORT_DDR_STRIDE;
 
 	/* This registers 32 GPIO lines per port */
 	err = bgpio_init(&port->gc, gpio->dev, 4, dat, set, NULL, dirout,

drivers/gpio/gpio-dwapb.c +0 -0

 		}
 	}

spin_unlock_irqrestore(&gc->bgpio_lock, flags);

Signature · 1 line

2.25.1

drivers/gpio/gpio-dwapb.c static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev) · line 584

Quoted 4 lines · Andy Shevchenko

In some cases indentation makes code harder to read. Amend indentation in those cases despite of lines go a bit over 80 character limit.

drivers/gpio/gpio-dwapb.c +1 -0

+			dev_err(dev, "missing/invalid port index for port%d\n", i);

What about shortening the message text to fit the 80 chars per line rule? I suppose the "missing" word could be omitted.

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_suspend(struct device *dev) · line 741

Quoted 5 lines

drivers/gpio/gpio-dwapb.c +1 -2

 			/* Mask out interrupts */
-			dwapb_write(gpio, GPIO_INTMASK,
-				    0xffffffff & ~ctx->wake_en);
 
+			dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en);

Hm, do I need some rest and missing something or the &-operation with 1s here does nothing seeing the operands data types have the same width?

(the change introduced by commit 6437c7ba69c3 ("gpio: dwapb: Add wakeup source support"))

Quoted 8 lines · Serge Semin

drivers/gpio/gpio-dwapb.c static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev) · line 584

Quoted 4 lines · Andy Shevchenko

In some cases indentation makes code harder to read. Amend indentation in those cases despite of lines go a bit over 80 character limit.

drivers/gpio/gpio-dwapb.c +1 -0

+			dev_err(dev, "missing/invalid port index for port%d\n", i);

What about shortening the message text to fit the 80 chars per line rule? I suppose the "missing" word could be omitted.

More likely port is not needed, but I think this kind of changes are material for another (logically separated) patch.

...

Quoted 9 lines

drivers/gpio/gpio-dwapb.c static int dwapb_gpio_suspend(struct device *dev) · line 741

Quoted 5 lines

drivers/gpio/gpio-dwapb.c +1 -2

 			/* Mask out interrupts */
-			dwapb_write(gpio, GPIO_INTMASK,
-				    0xffffffff & ~ctx->wake_en);
 
+			dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en);

Hm, do I need some rest and missing something or the &-operation with 1s here does nothing seeing the operands data types have the same width?

Quoted 1 line

(the change introduced by commit 6437c7ba69c3 ("gpio: dwapb: Add wakeup source support"))

No, you are right, it seems no-op to me, I have noticed it as well, but I think we may improve this by separate change (as you seems also prefer not to mix logically different changes in one patch).

Signature · 3 lines

-- 
With Best Regards,
Andy Shevchenko

Ah, Linus already pulled the series in. Next time then.)

Quoted 1 line · Serge Semin

Ah, Linus already pulled the series in. Next time then.)

Yeah sorry, I was a bit stressed by a big mail backlog and also a bit infatuated with my new b4 tool.

Is it fine to fix any remaining issues with extra patches?

Yours,
Linus Walleij

caused the 80 chars line rule violation in some cases.

Quoted 8 lines · Linus Walleij

Quoted 1 line · Serge Semin

Ah, Linus already pulled the series in. Next time then.)

Yeah sorry, I was a bit stressed by a big mail backlog and also a bit infatuated with my new b4 tool.

Is it fine to fix any remaining issues with extra patches?

I see. No worries. Andy did a good work fixing the indentations. But that caused the 80 chars line rule violation in some cases. The best way would be to avoid the rule violation in the first place, though sometimes it's just impossible without weakening the code readability. I suggested to fix some of the issues by reducing the error messages length and in another case just to remove the no-op &-operation. So If there were following up patches with fixes it would have been great. Though since we have got a violation for several chars in just a few lines of code, we can leave with that for now. So if Andy doesn't have a time to send the followup patches, I'll do this sometime later in the framework of my next patchset.

It appears that GPIO DW APB driver wasn't touched for a long time. Here is the fix for long standing issue, i.e. missed module alias to make the driver be loaded automatically.

On top of above a lot small clean ups here and there.

The series based on the v3 by Serge Semin which he sent earlier.

Driver has been tested on Intel Galileo Gen2 with AT25 SPI EEPROM using it for a chip select.

Changelog since v1:
- added Tested-by and Reviewed-by tags (Serge)
- split IRQ refactoring patch to two (Serge)
- update hwirq variable type to be irq_hw_number_t