[PATCH v2 00/14] gpio: dwapb: Clean up the driver and a fix

[PATCH v2 00/14] gpio: dwapb: Clean up the driver and a fix

[Andy Shevchenko]

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

Cc: Serge Semin <fancer.lancer@gmail.com>

Andy Shevchenko (14):
  gpio: dwapb: Append MODULE_ALIAS for platform driver
  gpio: dwapb: Refactor IRQ handler to use bit operations
  gpio: dwapb: Use chained IRQ prologue and epilogue
  gpio: dwapb: set default handler to be handle_bad_irq()
  gpio: dwapb: Deduplicate IRQ resource management
  gpio: dwapb: Convert to use irqd_to_hwirq()
  gpio: dwapb: Use device_get_match_data() to simplify code
  gpio: dwapb: Convert to use IRQ core provided macros
  gpio: dwapb: Switch to more usual pattern of RMW in
    dwapb_gpio_set_debounce()
  gpio: dwapb: Drop bogus BUG_ON()s
  gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls
  gpio: dwapb: Split out dwapb_get_irq() helper
  gpio: dwapb: Use positive conditional in dwapb_configure_irqs()
  gpio: dwapb: Amend indentation in some cases

 drivers/gpio/gpio-dwapb.c | 205 +++++++++++++++-----------------------
 1 file changed, 79 insertions(+), 126 deletions(-)

-- 
2.25.1

[PATCH v2 01/14] gpio: dwapb: Append MODULE_ALIAS for platform driver

[Andy Shevchenko]

The commit 3d2613c4289f
  ("GPIO: gpio-dwapb: Enable platform driver binding to MFD driver")
introduced a use of the platform driver but missed to add the following line
to it:
  MODULE_ALIAS("platform:gpio-dwapb");

Add this to get driver loaded automatically if platform device is registered.

Fixes: 3d2613c4289f ("GPIO: gpio-dwapb: Enable platform driver binding to MFD driver")
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index d2ed11510f3c..c1b6d4f7307e 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -49,7 +49,9 @@
 #define GPIO_EXT_PORTC		0x58
 #define GPIO_EXT_PORTD		0x5c
 
+#define DWAPB_DRIVER_NAME	"gpio-dwapb"
 #define DWAPB_MAX_PORTS		4
+
 #define GPIO_EXT_PORT_STRIDE	0x04 /* register stride 32 bits */
 #define GPIO_SWPORT_DR_STRIDE	0x0c /* register stride 3*32 bits */
 #define GPIO_SWPORT_DDR_STRIDE	0x0c /* register stride 3*32 bits */
@@ -400,7 +402,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		return;
 
 	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
-					     "gpio-dwapb", handle_level_irq,
+					     DWAPB_DRIVER_NAME, handle_level_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
 	if (err) {
@@ -457,7 +459,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		 */
 		err = devm_request_irq(gpio->dev, pp->irq[0],
 				       dwapb_irq_handler_mfd,
-				       IRQF_SHARED, "gpio-dwapb-mfd", gpio);
+				       IRQF_SHARED, DWAPB_DRIVER_NAME, gpio);
 		if (err) {
 			dev_err(gpio->dev, "error requesting IRQ\n");
 			irq_domain_remove(gpio->domain);
@@ -847,7 +849,7 @@ static SIMPLE_DEV_PM_OPS(dwapb_gpio_pm_ops, dwapb_gpio_suspend,
 
 static struct platform_driver dwapb_gpio_driver = {
 	.driver		= {
-		.name	= "gpio-dwapb",
+		.name	= DWAPB_DRIVER_NAME,
 		.pm	= &dwapb_gpio_pm_ops,
 		.of_match_table = of_match_ptr(dwapb_of_match),
 		.acpi_match_table = ACPI_PTR(dwapb_acpi_match),
@@ -861,3 +863,4 @@ module_platform_driver(dwapb_gpio_driver);
 MODULE_LICENSE("GPL");
 MODULE_AUTHOR("Jamie Iles");
 MODULE_DESCRIPTION("Synopsys DesignWare APB GPIO driver");
+MODULE_ALIAS("platform:" DWAPB_DRIVER_NAME);
-- 
2.25.1

[PATCH v2 02/14] gpio: dwapb: Refactor IRQ handler to use bit operations

[Andy Shevchenko]

Refactor IRQ handler in order to use for_each_set_bit() helper.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 15 +++++++--------
 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index c1b6d4f7307e..e89a3c6877aa 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -193,22 +193,21 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 
 static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
 {
-	u32 irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
-	u32 ret = irq_status;
+	unsigned long irq_status;
+	int hwirq;
 
-	while (irq_status) {
-		int hwirq = fls(irq_status) - 1;
+	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
+	for_each_set_bit(hwirq, &irq_status, 32) {
 		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
+		u32 irq_type = irq_get_trigger_type(gpio_irq);
 
 		generic_handle_irq(gpio_irq);
-		irq_status &= ~BIT(hwirq);
 
-		if ((irq_get_trigger_type(gpio_irq) & IRQ_TYPE_SENSE_MASK)
-			== IRQ_TYPE_EDGE_BOTH)
+		if ((irq_type & IRQ_TYPE_SENSE_MASK) == IRQ_TYPE_EDGE_BOTH)
 			dwapb_toggle_trigger(gpio, hwirq);
 	}
 
-	return ret;
+	return irq_status;
 }
 
 static void dwapb_irq_handler(struct irq_desc *desc)
-- 
2.25.1

[PATCH v2 03/14] gpio: dwapb: Use chained IRQ prologue and epilogue

[Andy Shevchenko]

Refactor IRQ handler in order to enter and exit chained IRQ by using
respective prologue and epilogue calls.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index e89a3c6877aa..f61139f787d9 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -215,10 +215,9 @@ static void dwapb_irq_handler(struct irq_desc *desc)
 	struct dwapb_gpio *gpio = irq_desc_get_handler_data(desc);
 	struct irq_chip *chip = irq_desc_get_chip(desc);
 
+	chained_irq_enter(chip, desc);
 	dwapb_do_irq(gpio);
-
-	if (chip->irq_eoi)
-		chip->irq_eoi(irq_desc_get_irq_data(desc));
+	chained_irq_exit(chip, desc);
 }
 
 static void dwapb_irq_enable(struct irq_data *d)
-- 
2.25.1

[PATCH v2 04/14] gpio: dwapb: set default handler to be handle_bad_irq()

[Andy Shevchenko]

We switch the default handler to be handle_bad_irq() instead of
handle_level_irq(), though for now apply it later in the code,
to make the difference between IRQ chips more visible.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index f61139f787d9..588d5c61ae42 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -400,7 +400,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		return;
 
 	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
-					     DWAPB_DRIVER_NAME, handle_level_irq,
+					     DWAPB_DRIVER_NAME, handle_bad_irq,
 					     IRQ_NOREQUEST, 0,
 					     IRQ_GC_INIT_NESTED_LOCK);
 	if (err) {
@@ -439,6 +439,7 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 	}
 
 	irq_gc->chip_types[0].type = IRQ_TYPE_LEVEL_MASK;
+	irq_gc->chip_types[0].handler = handle_level_irq;
 	irq_gc->chip_types[1].type = IRQ_TYPE_EDGE_BOTH;
 	irq_gc->chip_types[1].handler = handle_edge_irq;
 
-- 
2.25.1

[PATCH v2 05/14] gpio: dwapb: Deduplicate IRQ resource management

[Andy Shevchenko]

GPIO library provides default IRQ resource management hooks,
there is no need to repeat this in the individual driver.

Remove them for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 27 ---------------------------
 1 file changed, 27 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 588d5c61ae42..c0c267cddd80 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -250,31 +250,6 @@ static void dwapb_irq_disable(struct irq_data *d)
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 }
 
-static int dwapb_irq_reqres(struct irq_data *d)
-{
-	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 ret;
-
-	ret = gpiochip_lock_as_irq(gc, irqd_to_hwirq(d));
-	if (ret) {
-		dev_err(gpio->dev, "unable to lock HW IRQ %lu for IRQ\n",
-			irqd_to_hwirq(d));
-		return ret;
-	}
-	return 0;
-}
-
-static void dwapb_irq_relres(struct irq_data *d)
-{
-	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;
-
-	gpiochip_unlock_as_irq(gc, irqd_to_hwirq(d));
-}
-
 static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
@@ -428,8 +403,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 		ct->chip.irq_set_type = dwapb_irq_set_type;
 		ct->chip.irq_enable = dwapb_irq_enable;
 		ct->chip.irq_disable = dwapb_irq_disable;
-		ct->chip.irq_request_resources = dwapb_irq_reqres;
-		ct->chip.irq_release_resources = dwapb_irq_relres;
 #ifdef CONFIG_PM_SLEEP
 		ct->chip.irq_set_wake = dwapb_irq_set_wake;
 #endif
-- 
2.25.1

[PATCH v2 06/14] gpio: dwapb: Convert to use irqd_to_hwirq()

[Andy Shevchenko]

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 <fancer.lancer@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index c0c267cddd80..2edccc5bbc25 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -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,
-- 
2.25.1

[PATCH v2 07/14] gpio: dwapb: Use device_get_match_data() to simplify code

[Andy Shevchenko]

Use device_get_match_data() here to simplify the code a bit.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 13 +------------
 1 file changed, 1 insertion(+), 12 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 2edccc5bbc25..8b30ded9322a 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -683,18 +683,7 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	gpio->flags = 0;
-	if (dev->of_node) {
-		gpio->flags = (uintptr_t)of_device_get_match_data(dev);
-	} else if (has_acpi_companion(dev)) {
-		const struct acpi_device_id *acpi_id;
-
-		acpi_id = acpi_match_device(dwapb_acpi_match, dev);
-		if (acpi_id) {
-			if (acpi_id->driver_data)
-				gpio->flags = acpi_id->driver_data;
-		}
-	}
+	gpio->flags = (uintptr_t)device_get_match_data(dev);
 
 	for (i = 0; i < gpio->nr_ports; i++) {
 		err = dwapb_gpio_add_port(gpio, &pdata->properties[i], i);
-- 
2.25.1

[PATCH v2 08/14] gpio: dwapb: Convert to use IRQ core provided macros

[Andy Shevchenko]

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

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 8b30ded9322a..4edac592c253 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -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,
 
 static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
 {
-	u32 worked;
-	struct dwapb_gpio *gpio = dev_id;
-
-	worked = dwapb_do_irq(gpio);
-
-	return worked ? IRQ_HANDLED : IRQ_NONE;
+	return IRQ_RETVAL(dwapb_do_irq(dev_id));
 }
 
 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
-- 
2.25.1

[PATCH v2 09/14] gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()

[Andy Shevchenko]

More usual pattern is to prepare value and then write it in a single place.
Switch code in dwapb_gpio_set_debounce() to it.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 4edac592c253..0b073cbc003b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -327,9 +327,10 @@ static int dwapb_gpio_set_debounce(struct gpio_chip *gc,
 
 	val_deb = dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
 	if (debounce)
-		dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, val_deb | mask);
+		val_deb |= mask;
 	else
-		dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, val_deb & ~mask);
+		val_deb &= ~mask;
+	dwapb_write(gpio, GPIO_PORTA_DEBOUNCE, val_deb);
 
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
 
-- 
2.25.1

[PATCH v2 10/14] gpio: dwapb: Drop bogus BUG_ON()s

[Andy Shevchenko]

There is no case when no context is provided in the ->suspend() and
->resume() hooks. Moreover, BUG_ON() is harmful to user and makes kernel
inoperable after the crash. Drop the BUG_ON()s for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 0b073cbc003b..ae4c4db8b156 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -723,8 +723,6 @@ static int dwapb_gpio_suspend(struct device *dev)
 		unsigned int idx = gpio->ports[i].idx;
 		struct dwapb_context *ctx = gpio->ports[i].ctx;
 
-		BUG_ON(!ctx);
-
 		offset = GPIO_SWPORTA_DDR + idx * GPIO_SWPORT_DDR_STRIDE;
 		ctx->dir = dwapb_read(gpio, offset);
 
@@ -773,8 +771,6 @@ static int dwapb_gpio_resume(struct device *dev)
 		unsigned int idx = gpio->ports[i].idx;
 		struct dwapb_context *ctx = gpio->ports[i].ctx;
 
-		BUG_ON(!ctx);
-
 		offset = GPIO_SWPORTA_DR + idx * GPIO_SWPORT_DR_STRIDE;
 		dwapb_write(gpio, offset, ctx->data);
 
-- 
2.25.1

[PATCH v2 11/14] gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls

[Andy Shevchenko]

Since we always have a table of IDs compiled in, there is no use
for of_match_ptr() nor ACPI_PTR() call. Besides that it brings
a warning (depending on configuration):

.../gpio-dwapb.c:638:34: warning: ‘dwapb_of_match’ defined but not used [-Wunused-const-variable=]
  638 | static const struct of_device_id dwapb_of_match[] = {
      |                                  ^~~~~~~~~~~~~~

Get rid of them for good.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index ae4c4db8b156..98e1ffcd432b 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -805,8 +805,8 @@ static struct platform_driver dwapb_gpio_driver = {
 	.driver		= {
 		.name	= DWAPB_DRIVER_NAME,
 		.pm	= &dwapb_gpio_pm_ops,
-		.of_match_table = of_match_ptr(dwapb_of_match),
-		.acpi_match_table = ACPI_PTR(dwapb_acpi_match),
+		.of_match_table = dwapb_of_match,
+		.acpi_match_table = dwapb_acpi_match,
 	},
 	.probe		= dwapb_gpio_probe,
 	.remove		= dwapb_gpio_remove,
-- 
2.25.1

[PATCH v2 12/14] gpio: dwapb: Split out dwapb_get_irq() helper

[Andy Shevchenko]

Split out dwapb_get_irq() helper for better readability and maintenance.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 56 ++++++++++++++++++++-------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 98e1ffcd432b..31d29ec6ab5c 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -528,14 +528,38 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 			gpiochip_remove(&gpio->ports[m].gc);
 }
 
-static struct dwapb_platform_data *
-dwapb_gpio_get_pdata(struct device *dev)
+static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
+			  struct dwapb_port_property *pp)
+{
+	struct device_node *np = NULL;
+	int j;
+
+	if (fwnode_property_read_bool(fwnode, "interrupt-controller"))
+		np = to_of_node(fwnode);
+
+	for (j = 0; j < pp->ngpio; j++) {
+		pp->irq[j] = -ENXIO;
+
+		if (np)
+			pp->irq[j] = of_irq_get(np, j);
+		else if (has_acpi_companion(dev))
+			pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
+
+		if (pp->irq[j] >= 0)
+			pp->has_irq = true;
+	}
+
+	if (!pp->has_irq)
+		dev_warn(dev, "no irq for port%d\n", pp->idx);
+}
+
+static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 {
 	struct fwnode_handle *fwnode;
 	struct dwapb_platform_data *pdata;
 	struct dwapb_port_property *pp;
 	int nports;
-	int i, j;
+	int i;
 
 	nports = device_get_child_node_count(dev);
 	if (nports == 0)
@@ -553,8 +577,6 @@ dwapb_gpio_get_pdata(struct device *dev)
 
 	i = 0;
 	device_for_each_child_node(dev, fwnode)  {
-		struct device_node *np = NULL;
-
 		pp = &pdata->properties[i++];
 		pp->fwnode = fwnode;
 
@@ -581,28 +603,8 @@ dwapb_gpio_get_pdata(struct device *dev)
 		 * Only port A can provide interrupts in all configurations of
 		 * the IP.
 		 */
-		if (pp->idx != 0)
-			continue;
-
-		if (dev->of_node && fwnode_property_read_bool(fwnode,
-						  "interrupt-controller")) {
-			np = to_of_node(fwnode);
-		}
-
-		for (j = 0; j < pp->ngpio; j++) {
-			pp->irq[j] = -ENXIO;
-
-			if (np)
-				pp->irq[j] = of_irq_get(np, j);
-			else if (has_acpi_companion(dev))
-				pp->irq[j] = platform_get_irq(to_platform_device(dev), j);
-
-			if (pp->irq[j] >= 0)
-				pp->has_irq = true;
-		}
-
-		if (!pp->has_irq)
-			dev_warn(dev, "no irq for port%d\n", pp->idx);
+		if (pp->idx == 0)
+			dwapb_get_irq(dev, fwnode, pp);
 	}
 
 	return pdata;
-- 
2.25.1

[PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Andy Shevchenko]

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

Cc: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 31d29ec6ab5c..84c971e0adf0 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -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++)
-- 
2.25.1

[PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

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

Cc: Serge Semin <fancer.lancer@gmail.com>
Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Tested-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 24 +++++++++---------------
 1 file changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 84c971e0adf0..a09332d9c7fe 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -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,
@@ -582,17 +581,13 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 
 		if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
 		    pp->idx >= DWAPB_MAX_PORTS) {
-			dev_err(dev,
-				"missing/invalid port index for port%d\n", i);
+			dev_err(dev, "missing/invalid port index for port%d\n", i);
 			fwnode_handle_put(fwnode);
 			return ERR_PTR(-EINVAL);
 		}
 
-		if (fwnode_property_read_u32(fwnode, "snps,nr-gpios",
-					 &pp->ngpio)) {
-			dev_info(dev,
-				 "failed to get number of gpios for port%d\n",
-				 i);
+		if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) {
+			dev_info(dev, "failed to get number of gpios for port%d\n", i);
 			pp->ngpio = 32;
 		}
 
@@ -743,8 +738,7 @@ static int dwapb_gpio_suspend(struct device *dev)
 			ctx->int_deb	= dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
 
 			/* Mask out interrupts */
-			dwapb_write(gpio, GPIO_INTMASK,
-				    0xffffffff & ~ctx->wake_en);
+			dwapb_write(gpio, GPIO_INTMASK, 0xffffffff & ~ctx->wake_en);
 		}
 	}
 	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
-- 
2.25.1

Re: [PATCH v2 06/14] gpio: dwapb: Convert to use irqd_to_hwirq()

[Serge Semin]

On Wed, Apr 15, 2020 at 05:15:26PM +0300, Andy Shevchenko wrote:
> 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.

I meant to leave the int-s in the cases where the corresponding variable was used
as the BIT()-macro argument, because in that context it's used as the bit
number. Though using irq_hw_number_t everywhere seems also fine.

Reviewed-by: Serge Semin <fancer.lancer@gmail.com>

> 
> Cc: Serge Semin <fancer.lancer@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index c0c267cddd80..2edccc5bbc25 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -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,
> -- 
> 2.25.1
> 

Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Serge Semin]

On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote:
> 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.

Regards,
-Sergey

> 
> Cc: Serge Semin <fancer.lancer@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 31d29ec6ab5c..84c971e0adf0 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -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++)
> -- 
> 2.25.1
> 

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Serge Semin]

On Wed, Apr 15, 2020 at 05:15:34PM +0300, Andy Shevchenko wrote:
> 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 <fancer.lancer@gmail.com>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 24 +++++++++---------------
>  1 file changed, 9 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 84c971e0adf0..a09332d9c7fe 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -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,
> @@ -582,17 +581,13 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  
>  		if (fwnode_property_read_u32(fwnode, "reg", &pp->idx) ||
>  		    pp->idx >= DWAPB_MAX_PORTS) {
> -			dev_err(dev,
> -				"missing/invalid port index for port%d\n", i);

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

>  			fwnode_handle_put(fwnode);
>  			return ERR_PTR(-EINVAL);
>  		}
>  
> -		if (fwnode_property_read_u32(fwnode, "snps,nr-gpios",
> -					 &pp->ngpio)) {
> -			dev_info(dev,
> -				 "failed to get number of gpios for port%d\n",
> -				 i);
> +		if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) {

> +			dev_info(dev, "failed to get number of gpios for port%d\n", i);

The same here. For instance "no snps,nr-gpios found for port%d" would work here.

>  			pp->ngpio = 32;
>  		}
>  
> @@ -743,8 +738,7 @@ static int dwapb_gpio_suspend(struct device *dev)
>  			ctx->int_deb	= dwapb_read(gpio, GPIO_PORTA_DEBOUNCE);
>  
>  			/* 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"))

-Sergey

>  		}
>  	}
>  	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
> -- 
> 2.25.1
> 

Re: [PATCH v2 08/14] gpio: dwapb: Convert to use IRQ core provided macros

[Serge Semin]

On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote:
> IRQ core provides macros such as IRQ_RETVAL().
> Convert code to use them.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Tested-by: Serge Semin <fancer.lancer@gmail.com>
> Reviewed-by: Serge Semin <fancer.lancer@gmail.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 8b30ded9322a..4edac592c253 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -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,
>  
>  static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
>  {
> -	u32 worked;
> -	struct dwapb_gpio *gpio = dev_id;
> -
> -	worked = dwapb_do_irq(gpio);
> -
> -	return worked ? IRQ_HANDLED : IRQ_NONE;
> +	return IRQ_RETVAL(dwapb_do_irq(dev_id));
>  }

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

Regards,
-Sergey

>  
>  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
> -- 
> 2.25.1
> 

Re: [PATCH v2 00/14] gpio: dwapb: Clean up the driver and a fix

[Linus Walleij]

On Wed, Apr 15, 2020 at 4:15 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:

> 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

I managed to pull this v2 series out of lore with the b4 tool and applied
it check the result!

Yours,
Linus Walleij

Re: [PATCH v2 08/14] gpio: dwapb: Convert to use IRQ core provided macros

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 08:53:09PM +0300, Serge Semin wrote:
> On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote:
> > 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

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 08:15:16PM +0300, Serge Semin wrote:
> On Wed, Apr 15, 2020 at 05:15:34PM +0300, Andy Shevchenko wrote:
> > In some cases indentation makes code harder to read. Amend indentation
> > in those cases despite of lines go a bit over 80 character limit.

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

...

> >  			/* 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"))

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 08/14] gpio: dwapb: Convert to use IRQ core provided macros

[Serge Semin]

On Thu, Apr 16, 2020 at 01:39:11PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 15, 2020 at 08:53:09PM +0300, Serge Semin wrote:
> > On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote:
> > > 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
> 
> 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

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Serge Semin]

On Thu, Apr 16, 2020 at 01:56:14PM +0300, Andy Shevchenko wrote:
> On Wed, Apr 15, 2020 at 08:15:16PM +0300, Serge Semin wrote:
> > On Wed, Apr 15, 2020 at 05:15:34PM +0300, Andy Shevchenko wrote:
> > > In some cases indentation makes code harder to read. Amend indentation
> > > in those cases despite of lines go a bit over 80 character limit.
> 
> > > +			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.
> 
> ...
> 
> > >  			/* 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"))
> 
> 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).

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

Regards,
-Sergey

> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Linus Walleij]

On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote:
> > 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.

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

Re: [PATCH v2 08/14] gpio: dwapb: Convert to use IRQ core provided macros

[Jan Kiszka]

On 16.04.20 13:01, Serge Semin wrote:
> On Thu, Apr 16, 2020 at 01:39:11PM +0300, Andy Shevchenko wrote:
>> On Wed, Apr 15, 2020 at 08:53:09PM +0300, Serge Semin wrote:
>>> On Wed, Apr 15, 2020 at 05:15:28PM +0300, Andy Shevchenko wrote:
>>>> 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
>>
>> 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.

Yeah, never had the time to dig into details. Also, that patch was only 
tested on one particular device with one particular kernel series 
(4.4-cip-rt). That's why it's a "workaround", not a fix.

Jan

> 
> [1] Documentation/driver-api/gpio/driver.rst
> 
> Regards,
> -Sergey
> 
>>
>> -- 
>> With Best Regards,
>> Andy Shevchenko
>>
>>
-- 
Siemens AG, Corporate Technology, CT RDA IOT SES-DE
Corporate Competence Center Embedded Linux

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Linus Walleij]

On Thu, Apr 16, 2020 at 1:06 PM Serge Semin <fancer.lancer@gmail.com> wrote:

> 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

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Serge Semin]

On Thu, Apr 16, 2020 at 02:14:10PM +0200, Linus Walleij wrote:
> On Thu, Apr 16, 2020 at 1:06 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> 
> > 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.

Regards,
-Sergey

> 
> Yours,
> Linus Walleij

Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Serge Semin]

On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote:
> On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote:
> > > 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.
> 
> 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 was against this patch in the first
place. As for me it seemed redundant (see my justification above). But you or
Bartosz may think differently that's why I asked your opinion to decide its
destiny. So if you are ok with what the patch provides, then there is no need
to revert or reset anything. But if you agree with me, then pulling the patch
out with resetting the git history will be the best option.

Regards,
-Sergey

> 
> Yours,
> Linus Walleij

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Linus Walleij]

On Thu, Apr 16, 2020 at 3:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:

> caused the 80 chars line rule violation in some cases.

As subsystem maintainer that's a coding style thing I don't
really care much about, I personally violate it all the time.
But you can have it any way you want of course :)

Yours,
Linus Walleij

Re: [PATCH v2 14/14] gpio: dwapb: Amend indentation in some cases

[Andy Shevchenko]

On Thu, Apr 16, 2020 at 04:37:37PM +0300, Serge Semin wrote:
> On Thu, Apr 16, 2020 at 02:14:10PM +0200, Linus Walleij wrote:
> > On Thu, Apr 16, 2020 at 1:06 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > 
> > > 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.

I see an immutable branch with this series, so, it means it will be easier
to all of us to move on from this point now.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Linus Walleij]

On Thu, Apr 16, 2020 at 3:48 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote:
> > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote:
> > > > 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.
> >
> > 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

Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Andy Shevchenko]

On Fri, Apr 17, 2020 at 12:42:33PM +0200, Linus Walleij wrote:
> On Thu, Apr 16, 2020 at 3:48 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote:
> > > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote:
> > > > > 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.
> > >
> > > 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.

Fine with me, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 13/14] gpio: dwapb: Use positive conditional in dwapb_configure_irqs()

[Serge Semin]

On Fri, Apr 17, 2020 at 12:42:33PM +0200, Linus Walleij wrote:
> On Thu, Apr 16, 2020 at 3:48 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > On Thu, Apr 16, 2020 at 01:53:25PM +0200, Linus Walleij wrote:
> > > On Wed, Apr 15, 2020 at 6:37 PM Serge Semin <fancer.lancer@gmail.com> wrote:
> > > > On Wed, Apr 15, 2020 at 05:15:33PM +0300, Andy Shevchenko wrote:
> > > > > 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.
> > >
> > > 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.

Ok. Thanks.

Regards,
-Sergey

> 
> Yours,
> Linus Walleij