[PATCH v1 00/13] gpio: dwapb: Clean up the driver and a fix

[PATCH v1 00/13] 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.

Andy Shevchenko (13):
  gpio: dwapb: Append MODULE_ALIAS for platform driver
  gpio: dwapb: Refactor IRQ handler
  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 v1 01/13] 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>
---
 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 v1 02/13] gpio: dwapb: Refactor IRQ handler

[Andy Shevchenko]

Refactor IRQ handler in order to:
- enter and exit chained IRQ
- use for_each_set_bit() helper

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c | 20 +++++++++-----------
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index c1b6d4f7307e..f61139f787d9 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)
@@ -216,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 v1 03/13] 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>
---
 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 v1 04/13] 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>
---
 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 v1 05/13] gpio: dwapb: Convert to use irqd_to_hwirq()

[Andy Shevchenko]

Convert to use irqd_to_hwirq() instead of direct access to the hwirq member.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/gpio/gpio-dwapb.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index c0c267cddd80..3a1f3fae923f 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -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,8 +255,8 @@ 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;
 	unsigned long level, polarity, flags;
+	u32 bit = irqd_to_hwirq(d);
 
 	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
 		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
@@ -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;
+	u32 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 v1 06/13] 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>
---
 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 3a1f3fae923f..a0a0288bb73e 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 v1 07/13] 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>
---
 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 a0a0288bb73e..916a42fea456 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)
 	unsigned long level, polarity, flags;
 	u32 bit = irqd_to_hwirq(d);
 
-	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 v1 08/13] 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>
---
 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 916a42fea456..a15652ff9495 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 v1 09/13] 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>
---
 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 a15652ff9495..c03d856be703 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 v1 10/13] 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>
---
 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 c03d856be703..5b93967a4c96 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 v1 11/13] 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>
---
 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 5b93967a4c96..7789410fe15e 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 v1 12/13] 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().

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 7789410fe15e..bbaf909a463a 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 v1 13/13] 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.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 bbaf909a463a..e4c946cafab0 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 v1 00/13] gpio: dwapb: Clean up the driver and a fix

[Serge Semin]

Hello Andy

On Thu, Apr 09, 2020 at 05:12:15PM +0300, Andy Shevchenko 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.

Thanks for the series. I'll review it in one-two days and test it out on our
Baikal-T1-based hardware.

-Sergey

> 
> Andy Shevchenko (13):
>   gpio: dwapb: Append MODULE_ALIAS for platform driver
>   gpio: dwapb: Refactor IRQ handler
>   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
> 

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

[Serge Semin]

Hello Andy

On Thu, Apr 09, 2020 at 05:12:15PM +0300, Andy Shevchenko 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.

Thanks one more time for the series of nice cleanups. I've successfully
tested it on our board with Baikal-T1 SoC, which has two DW APB IP-cores
embedded with Ports A being configured as GPIOx32 and irqless GPIOx3.
So for the whole series
Tested-by: Serge Semin <fancer.lancer@gmail.com>

(Note since until my series is merged in to the kernel technically I'm not
the driver maintainer so I'll use the reviewers tag for now where it's
relevant.)

Regards,
-Sergey

> 
> Andy Shevchenko (13):
>   gpio: dwapb: Append MODULE_ALIAS for platform driver
>   gpio: dwapb: Refactor IRQ handler
>   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
> 

Re: [PATCH v1 01/13] gpio: dwapb: Append MODULE_ALIAS for platform driver

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:16PM +0300, Andy Shevchenko wrote:
> 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.

Looks good. Thanks.

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

> 
> Fixes: 3d2613c4289f ("GPIO: gpio-dwapb: Enable platform driver binding to MFD driver")
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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
> 

Re: [PATCH v1 02/13] gpio: dwapb: Refactor IRQ handler

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:17PM +0300, Andy Shevchenko wrote:
> Refactor IRQ handler in order to:
> - enter and exit chained IRQ
> - use for_each_set_bit() helper

Please split these two changes into the dedicated patches.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 20 +++++++++-----------
>  1 file changed, 9 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index c1b6d4f7307e..f61139f787d9 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)
> @@ -216,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
> 

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

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 02:55:57PM +0300, Serge Semin wrote:
> Hello Andy
> 
> On Thu, Apr 09, 2020 at 05:12:15PM +0300, Andy Shevchenko 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.
> 
> Thanks one more time for the series of nice cleanups. I've successfully
> tested it on our board with Baikal-T1 SoC, which has two DW APB IP-cores
> embedded with Ports A being configured as GPIOx32 and irqless GPIOx3.
> So for the whole series
> Tested-by: Serge Semin <fancer.lancer@gmail.com>

Thank you very much.
I hope Linus will figure out how to proceed with this.

> (Note since until my series is merged in to the kernel technically I'm not
> the driver maintainer so I'll use the reviewers tag for now where it's
> relevant.)

Even if you are not (yet) maintainer, your tags count and be appreciated.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 03/13] gpio: dwapb: set default handler to be handle_bad_irq()

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:18PM +0300, Andy Shevchenko wrote:
> 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.

s/IRQ chips/IRQ types ?

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

Note though I'd better refactored the whole dwapb_configure_irqs()
method with using GPIOlib irqchip API, which is preferred by the current
GPIO subsystem. This isn't a subject for this series. Just to note, that
such change is very actual.

-Sergey

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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
> 

Re: [PATCH v1 04/13] gpio: dwapb: Deduplicate IRQ resource management

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:19PM +0300, Andy Shevchenko wrote:
> GPIO library provides default IRQ resource management hooks,
> there is no need to repeat this in the individual driver.
> 
> Remove them for good.

Great catch. Thanks.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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
> 

Re: [PATCH v1 03/13] gpio: dwapb: set default handler to be handle_bad_irq()

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 03:22:08PM +0300, Serge Semin wrote:
> On Thu, Apr 09, 2020 at 05:12:18PM +0300, Andy Shevchenko wrote:
> > 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.
> 
> s/IRQ chips/IRQ types ?

Actually IRQ chips. The code instantiates two IRQ chips.

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

Thanks!

> Note though I'd better refactored the whole dwapb_configure_irqs()
> method with using GPIOlib irqchip API, which is preferred by the current
> GPIO subsystem. This isn't a subject for this series. Just to note, that
> such change is very actual.

I know, but it's indeed out of scope of this series.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 02/13] gpio: dwapb: Refactor IRQ handler

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 03:04:49PM +0300, Serge Semin wrote:
> On Thu, Apr 09, 2020 at 05:12:17PM +0300, Andy Shevchenko wrote:
> > Refactor IRQ handler in order to:
> > - enter and exit chained IRQ
> > - use for_each_set_bit() helper
> 
> Please split these two changes into the dedicated patches.

Will do in v2. Hope your Tested-by holds.

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

Thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v1 05/13] gpio: dwapb: Convert to use irqd_to_hwirq()

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:20PM +0300, Andy Shevchenko wrote:
> Convert to use irqd_to_hwirq() instead of direct access to the hwirq member.
> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>  drivers/gpio/gpio-dwapb.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index c0c267cddd80..3a1f3fae923f 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -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,8 +255,8 @@ 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;
>  	unsigned long level, polarity, flags;
> +	u32 bit = irqd_to_hwirq(d);

I'm not saying that the rest of the driver code is highly coherent with
ideal design and style. But here I don't really see a point in converting
the type to u32. As I see it int-like type is more appropriate since
we don't need to signify the data type width in this context.

-Sergey

>  
>  	if (type & ~(IRQ_TYPE_EDGE_RISING | IRQ_TYPE_EDGE_FALLING |
>  		     IRQ_TYPE_LEVEL_HIGH | IRQ_TYPE_LEVEL_LOW))
> @@ -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;
> +	u32 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 v1 06/13] gpio: dwapb: Use device_get_match_data() to simplify code

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:21PM +0300, Andy Shevchenko wrote:
> Use device_get_match_data() here to simplify the code a bit.

Nice simplification. Thanks.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 3a1f3fae923f..a0a0288bb73e 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
> 

Re: [PATCH v1 07/13] gpio: dwapb: Convert to use IRQ core provided macros

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:22PM +0300, Andy Shevchenko wrote:
> IRQ core provides macros such as IRQ_RETVAL().
> Convert code to use them.

Looks good. Thanks.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 a0a0288bb73e..916a42fea456 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)
>  	unsigned long level, polarity, flags;
>  	u32 bit = irqd_to_hwirq(d);
>  
> -	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
> 

Re: [PATCH v1 08/13] gpio: dwapb: Switch to more usual pattern of RMW in dwapb_gpio_set_debounce()

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:23PM +0300, Andy Shevchenko wrote:
> More usual pattern is to prepare value and then write it in a single place.
> Switch code in dwapb_gpio_set_debounce() to it.

Seems appropriate. Thanks.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 916a42fea456..a15652ff9495 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
> 

Re: [PATCH v1 09/13] gpio: dwapb: Drop bogus BUG_ON()s

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:24PM +0300, Andy Shevchenko wrote:
> 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.

Right. Thanks. Though I wouldn't name them bogus, but redundant or
unnecessary instead.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 a15652ff9495..c03d856be703 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
> 

Re: [PATCH v1 10/13] gpio: dwapb: Drop of_match_ptr() & ACPI_PTR() calls

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:25PM +0300, Andy Shevchenko wrote:
> 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.

Looks good. Thanks.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 c03d856be703..5b93967a4c96 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
> 

Re: [PATCH v1 11/13] gpio: dwapb: Split out dwapb_get_irq() helper

[Serge Semin]

On Thu, Apr 09, 2020 at 05:12:26PM +0300, Andy Shevchenko wrote:
> Split out dwapb_get_irq() helper for better readability and maintenance.

Seems appropriate. Thanks.

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

> 
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.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 5b93967a4c96..7789410fe15e 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
> 

Re: [PATCH v1 05/13] gpio: dwapb: Convert to use irqd_to_hwirq()

[Andy Shevchenko]

On Wed, Apr 15, 2020 at 03:42:42PM +0300, Serge Semin wrote:
> On Thu, Apr 09, 2020 at 05:12:20PM +0300, Andy Shevchenko wrote:
> > Convert to use irqd_to_hwirq() instead of direct access to the hwirq member.

> > -	int bit = d->hwirq;

> > +	u32 bit = irqd_to_hwirq(d);
> 
> I'm not saying that the rest of the driver code is highly coherent with
> ideal design and style. But here I don't really see a point in converting
> the type to u32. As I see it int-like type is more appropriate since
> we don't need to signify the data type width in this context.

> > +	u32 bit = irqd_to_hwirq(d);

The idea is that it corresponds the hardware register width. But, i think the
proper one is irq_hw_number_t as return type in all cases.
Let me check this and fix accordingly.

> > +	irq_hw_number_t hwirq;

-- 
With Best Regards,
Andy Shevchenko