[PATCH 0/7] gpio: dwapb: Refactor GPIO resources initialization

[PATCH 0/7] gpio: dwapb: Refactor GPIO resources initialization

[Serge Semin]

This series is about the DW APB GPIO device initialization procedure
cleaning up. First of all it has been discovered that having a
vendor-specific "snps,nr-gpios" property isn't only redundant but also
might be dangerous (see the commit log for details). Instead we suggest to
use the generic "ngpios" property to define a number of GPIOs each DW APB
GPIO controller port supports. Secondly seeing a tendency of the other
GPIO drivers getting converted to using the GPIO-lib-based IRQ-chip
interface this series provides a patch, which replaces the DW APB GPIO
driver Generic IRQ-chip implementation with the GPIO-lib IRQ-chip one.
Finally the DW APB GPIO device probe procedure is simplified by
converting the code to be using the device managed resources for the
reference clocks initialization, reset control assertion/de-assertion
and GPIO-chip registration.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: linux-gpio@vger.kernel.org
Cc: devicetree@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (7):
  dt-bindings: gpio: dwapb: Add ngpios property support
  gpio: dwapb: Add ngpios DT-property support
  gpio: dwapb: Move MFD-specific IRQ handler
  gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip
  gpio: dwapb: Get reset control by means of resource managed interface
  gpio: dwapb: Get clocks by means of resource managed interface
  gpio: dwapb: Use resource managed GPIO-chip add data method

 .../bindings/gpio/snps,dw-apb-gpio.yaml       |   6 +
 drivers/gpio/Kconfig                          |   2 +-
 drivers/gpio/gpio-dwapb.c                     | 324 +++++++++---------
 3 files changed, 161 insertions(+), 171 deletions(-)

-- 
2.26.2

[PATCH 1/7] dt-bindings: gpio: dwapb: Add ngpios property support

[Serge Semin]

It's redundant to have a vendor-specific property describing a number of
GPIOS while there is a generic one. Let's mark the former one as
deprecated and define the "ngpios" property supported with constraints
of being within [1; 32] range.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 .../devicetree/bindings/gpio/snps,dw-apb-gpio.yaml          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
index 1240f6289249..b391cc1b4590 100644
--- a/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/snps,dw-apb-gpio.yaml
@@ -61,8 +61,14 @@ patternProperties:
       '#gpio-cells':
         const: 2
 
+      ngpios:
+        default: 32
+        minimum: 1
+        maximum: 32
+
       snps,nr-gpios:
         description: The number of GPIO pins exported by the port.
+        deprecated: true
         $ref: /schemas/types.yaml#/definitions/uint32
         default: 32
         minimum: 1
-- 
2.26.2

Re: [PATCH 1/7] dt-bindings: gpio: dwapb: Add ngpios property support

[Rob Herring]

On Thu, 23 Jul 2020 04:38:52 +0300, Serge Semin wrote:
> It's redundant to have a vendor-specific property describing a number of
> GPIOS while there is a generic one. Let's mark the former one as
> deprecated and define the "ngpios" property supported with constraints
> of being within [1; 32] range.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  .../devicetree/bindings/gpio/snps,dw-apb-gpio.yaml          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

[PATCH 2/7] gpio: dwapb: Add ngpios DT-property support

[Serge Semin]

Indeed generic GPIO DT-schema implies that number of GPIOs should be
described by the "ngpios" property located under a GPIO-provider DT node.
In that case it's redundant to have a vendor-specific "snps,nr-gpios"
property describing the same setting. Moreover it might be errors prone.

Since commit 93d2e4322aa7 ("of: platform: Batch fwnode parsing when adding
all top level devices") the fwnode parsing is resumed after the vast
majority of the platform devices are added. Implicitly that commit
activates re-parsing of the whole device tree GPIOs-phandle properties
detected having "-gpio/-gpios" suffixes. Since the vendor-specific number
of GPIOs property is defined with "-gpios" suffix, then of_link_property()
will consider it as a suffix-property with "#gpio-cells" structure, which
obviously it doesn't have. As a result for two DW APB GPIO controllers
we'll have the next errors printed.

OF: /bus@1f059000/gpio@1f044000/gpio-port@0: could not find phandle
OF: /bus@1f059000/gpio@1f045000/gpio-port@0: could not get #gpio-cells for /opp-table
OF: /bus@1f059000/gpio@1f044000/gpio-port@0: could not find phandle
OF: /bus@1f059000/gpio@1f045000/gpio-port@0: could not get #gpio-cells for /opp-table

See the kernel fwnode parsing procedure even tried to resolve the phandle
ID, which it thought was the opp-table DT-node, while in fact it was just
a number "32". What would happen if that magic number actually referred to
a GPIO DT-node with "#gpio-cells" property defined?..

In order to fix the problem let's mark the "snps,nr-gpios" property as
deprecated and add the generic "ngpios" property support with the same
purpose as the deprecated one. That and the errors log above shall
motivate the platform developer to convert the DW APB GPIO DT-nodes to
using the standard number of GPIOs property.

Signed-off-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 1d8d55bd63aa..ccd80df16062 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -594,7 +594,8 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			return ERR_PTR(-EINVAL);
 		}
 
-		if (fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) {
+		if (fwnode_property_read_u32(fwnode, "ngpios", &pp->ngpio) &&
+		    fwnode_property_read_u32(fwnode, "snps,nr-gpios", &pp->ngpio)) {
 			dev_info(dev,
 				 "failed to get number of gpios for port%d\n",
 				 i);
-- 
2.26.2

[PATCH 3/7] gpio: dwapb: Move MFD-specific IRQ handler

[Serge Semin]

For better readability let's group all the IRQ handler in a single place
of the driver instead of having them scatter around all over the file.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index ccd80df16062..3081213247d8 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -220,6 +220,11 @@ static void dwapb_irq_handler(struct irq_desc *desc)
 	chained_irq_exit(chip, desc);
 }
 
+static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
+{
+	return IRQ_RETVAL(dwapb_do_irq(dev_id));
+}
+
 static void dwapb_irq_enable(struct irq_data *d)
 {
 	struct irq_chip_generic *igc = irq_data_get_irq_chip_data(d);
@@ -349,11 +354,6 @@ static int dwapb_gpio_set_config(struct gpio_chip *gc, unsigned offset,
 	return dwapb_gpio_set_debounce(gc, offset, debounce);
 }
 
-static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
-{
-	return IRQ_RETVAL(dwapb_do_irq(dev_id));
-}
-
 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 				 struct dwapb_gpio_port *port,
 				 struct dwapb_port_property *pp)
-- 
2.26.2

[PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Serge Semin]

GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
top of a GPIO chip. It's better from maintainability and readability
point of view to use one instead of supporting a hand-written Generic
IRQ-chip-based implementation. Moreover the new implementation won't
cause much functional overhead but will provide a cleaner driver code.
All of that makes the DW APB GPIO driver conversion pretty much justified
especially seeing a tendency of the other GPIO drivers getting converted
too.

Here is what we do in the framework of this commit to convert the driver
to using the GPIO-lib-based IRQ-chip interface:
1) IRQ ack, mask and unmask callbacks are locally defined instead of
using the Generic IRQ-chip ones.
2) An irq_chip structure instance is embedded into the dwapb_gpio
private data. Note we can't have a static instance of that structure since
GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks().
A warning about that would have been printed by the GPIO-lib code if we
used a single irq_chip structure instance for multiple DW APB GPIO
controllers.
3) Initialize the gpio_irq_chip structure embedded into the gpio_chip
descriptor. By default there is no IRQ enabled so any event raised will be
handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core
is synthesized to have non-shared reference IRQ-lines, then as before the
hierarchical and cascaded cases are distinguished by checking how many
parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't
initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP
is used on a platform with shared IRQ line, then we simply won't let the
GPIO-lib to initialize the parental IRQs, but will handle them locally in
the driver.
4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since
GPIO-lib IRQ-chip interface will create a new domain and accept a standard
IRQ-chip structure pointer based on the setting we provided in the
gpio_irq_chip structure.
5) Manually select a proper IRQ flow handler directly in the
irq_set_type() callback by calling irq_set_handler_locked() method, since
an ordinary (not Generic) irq_chip descriptor is now utilized.
6) Discard the custom GPIO-to-IRQ mapping function since GPIO-lib defines
the standard method gpiochip_to_irq(), which will be used anyway no matter
whether the custom to_irq callback is specified or not.
7) Discard the acpi_gpiochip_{request,free}_interrupts()
invocations, since they will be called from
gpiochip_add_irqchip()/gpiochip_irqchip_remove() anyway.
8) Alter CONFIG_GPIO_DWAPB kernel config to select
CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/Kconfig      |   2 +-
 drivers/gpio/gpio-dwapb.c | 191 +++++++++++++++++---------------------
 2 files changed, 85 insertions(+), 108 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index c6b5c65c8405..bcd3e541a52b 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -202,7 +202,7 @@ config GPIO_DAVINCI
 config GPIO_DWAPB
 	tristate "Synopsys DesignWare APB GPIO driver"
 	select GPIO_GENERIC
-	select GENERIC_IRQ_CHIP
+	select GPIOLIB_IRQCHIP
 	help
 	  Say Y or M here to build support for the Synopsys DesignWare APB
 	  GPIO block.
diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3081213247d8..3f727ebe7f9a 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -13,7 +13,6 @@
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/irq.h>
-#include <linux/irqdomain.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -92,13 +91,15 @@ struct dwapb_gpio_port {
 #endif
 	unsigned int		idx;
 };
+#define to_dwapb_gpio(_gc) \
+	(container_of(_gc, struct dwapb_gpio_port, gc)->gpio)
 
 struct dwapb_gpio {
 	struct	device		*dev;
 	void __iomem		*regs;
 	struct dwapb_gpio_port	*ports;
 	unsigned int		nr_ports;
-	struct irq_domain	*domain;
+	struct irq_chip		irqchip;
 	unsigned int		flags;
 	struct reset_control	*rst;
 	struct clk_bulk_data	clks[DWAPB_NR_CLOCKS];
@@ -147,14 +148,6 @@ static inline void dwapb_write(struct dwapb_gpio *gpio, unsigned int offset,
 	gc->write_reg(reg_base + gpio_reg_convert(gpio, offset), val);
 }
 
-static int dwapb_gpio_to_irq(struct gpio_chip *gc, unsigned offset)
-{
-	struct dwapb_gpio_port *port = gpiochip_get_data(gc);
-	struct dwapb_gpio *gpio = port->gpio;
-
-	return irq_find_mapping(gpio->domain, offset);
-}
-
 static struct dwapb_gpio_port *dwapb_offs_to_port(struct dwapb_gpio *gpio, unsigned int offs)
 {
 	struct dwapb_gpio_port *port;
@@ -193,12 +186,13 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 
 static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
 {
+	struct gpio_chip *gc = &gpio->ports[0].gc;
 	unsigned long irq_status;
 	irq_hw_number_t hwirq;
 
 	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
 	for_each_set_bit(hwirq, &irq_status, 32) {
-		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
+		int gpio_irq = gc->to_irq(gc, hwirq);
 		u32 irq_type = irq_get_trigger_type(gpio_irq);
 
 		generic_handle_irq(gpio_irq);
@@ -225,11 +219,48 @@ static irqreturn_t dwapb_irq_handler_mfd(int irq, void *dev_id)
 	return IRQ_RETVAL(dwapb_do_irq(dev_id));
 }
 
+static void dwapb_irq_ack(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+	u32 val = BIT(irqd_to_hwirq(d));
+	unsigned long flags;
+
+	spin_lock_irqsave(&gc->bgpio_lock, flags);
+	dwapb_write(gpio, GPIO_PORTA_EOI, val);
+	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+}
+
+static void dwapb_irq_mask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&gc->bgpio_lock, flags);
+	val = dwapb_read(gpio, GPIO_INTMASK) | BIT(irqd_to_hwirq(d));
+	dwapb_write(gpio, GPIO_INTMASK, val);
+	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+}
+
+static void dwapb_irq_unmask(struct irq_data *d)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
+	unsigned long flags;
+	u32 val;
+
+	spin_lock_irqsave(&gc->bgpio_lock, flags);
+	val = dwapb_read(gpio, GPIO_INTMASK) & ~BIT(irqd_to_hwirq(d));
+	dwapb_write(gpio, GPIO_INTMASK, val);
+	spin_unlock_irqrestore(&gc->bgpio_lock, flags);
+}
+
 static void dwapb_irq_enable(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;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
 	unsigned long flags;
 	u32 val;
 
@@ -242,9 +273,8 @@ static void dwapb_irq_enable(struct irq_data *d)
 
 static void dwapb_irq_disable(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;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
 	unsigned long flags;
 	u32 val;
 
@@ -257,11 +287,11 @@ static void dwapb_irq_disable(struct irq_data *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);
-	struct dwapb_gpio *gpio = igc->private;
-	struct gpio_chip *gc = &gpio->ports[0].gc;
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
+	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
 	irq_hw_number_t bit = irqd_to_hwirq(d);
 	unsigned long level, polarity, flags;
+	irq_flow_handler_t handler;
 
 	if (type & ~IRQ_TYPE_SENSE_MASK)
 		return -EINVAL;
@@ -274,26 +304,31 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 	case IRQ_TYPE_EDGE_BOTH:
 		level |= BIT(bit);
 		dwapb_toggle_trigger(gpio, bit);
+		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_EDGE_RISING:
 		level |= BIT(bit);
 		polarity |= BIT(bit);
+		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
 		level |= BIT(bit);
 		polarity &= ~BIT(bit);
+		handler = handle_edge_irq;
 		break;
 	case IRQ_TYPE_LEVEL_HIGH:
 		level &= ~BIT(bit);
 		polarity |= BIT(bit);
+		handler = handle_level_irq;
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
 		level &= ~BIT(bit);
 		polarity &= ~BIT(bit);
+		handler = handle_level_irq;
 		break;
 	}
 
-	irq_setup_alt_chip(d, type);
+	irq_set_handler_locked(d, handler);
 
 	dwapb_write(gpio, GPIO_INTTYPE_LEVEL, level);
 	if (type != IRQ_TYPE_EDGE_BOTH)
@@ -359,73 +394,39 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 				 struct dwapb_port_property *pp)
 {
 	struct gpio_chip *gc = &port->gc;
-	struct fwnode_handle  *fwnode = pp->fwnode;
-	struct irq_chip_generic	*irq_gc = NULL;
-	unsigned int ngpio = gc->ngpio;
-	struct irq_chip_type *ct;
-	irq_hw_number_t hwirq;
-	int err, i;
+	struct gpio_irq_chip *girq;
+	int err;
 
 	if (memchr_inv(pp->irq, 0, sizeof(pp->irq)) == NULL) {
 		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
 		return;
 	}
 
-	gpio->domain = irq_domain_create_linear(fwnode, ngpio,
-						 &irq_generic_chip_ops, gpio);
-	if (!gpio->domain)
-		return;
-
-	err = irq_alloc_domain_generic_chips(gpio->domain, ngpio, 2,
-					     DWAPB_DRIVER_NAME, handle_bad_irq,
-					     IRQ_NOREQUEST, 0,
-					     IRQ_GC_INIT_NESTED_LOCK);
-	if (err) {
-		dev_info(gpio->dev, "irq_alloc_domain_generic_chips failed\n");
-		irq_domain_remove(gpio->domain);
-		gpio->domain = NULL;
-		return;
-	}
-
-	irq_gc = irq_get_domain_generic_chip(gpio->domain, 0);
-	if (!irq_gc) {
-		irq_domain_remove(gpio->domain);
-		gpio->domain = NULL;
-		return;
-	}
-
-	irq_gc->reg_base = gpio->regs;
-	irq_gc->private = gpio;
-
-	for (i = 0; i < 2; i++) {
-		ct = &irq_gc->chip_types[i];
-		ct->chip.irq_ack = irq_gc_ack_set_bit;
-		ct->chip.irq_mask = irq_gc_mask_set_bit;
-		ct->chip.irq_unmask = irq_gc_mask_clr_bit;
-		ct->chip.irq_set_type = dwapb_irq_set_type;
-		ct->chip.irq_enable = dwapb_irq_enable;
-		ct->chip.irq_disable = dwapb_irq_disable;
+	girq = &gc->irq;
+	girq->handler = handle_bad_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+
+	gpio->irqchip.name = DWAPB_DRIVER_NAME;
+	gpio->irqchip.irq_ack = dwapb_irq_ack;
+	gpio->irqchip.irq_mask = dwapb_irq_mask;
+	gpio->irqchip.irq_unmask = dwapb_irq_unmask;
+	gpio->irqchip.irq_set_type = dwapb_irq_set_type;
+	gpio->irqchip.irq_enable = dwapb_irq_enable;
+	gpio->irqchip.irq_disable = dwapb_irq_disable;
 #ifdef CONFIG_PM_SLEEP
-		ct->chip.irq_set_wake = dwapb_irq_set_wake;
+	gpio->irqchip.irq_set_wake = dwapb_irq_set_wake;
 #endif
-		ct->regs.ack = gpio_reg_convert(gpio, GPIO_PORTA_EOI);
-		ct->regs.mask = gpio_reg_convert(gpio, GPIO_INTMASK);
-		ct->type = IRQ_TYPE_LEVEL_MASK;
-	}
-
-	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;
 
 	if (!pp->irq_shared) {
-		int i;
-
-		for (i = 0; i < pp->ngpio; i++) {
-			if (pp->irq[i])
-				irq_set_chained_handler_and_data(pp->irq[i],
-						dwapb_irq_handler, gpio);
-		}
+		/*
+		 * If more than one IRQ line is specified then try to
+		 * initialize the hierarchical interrupts. Otherwise it's
+		 * a simple cascaded case with a common IRQ signal.
+		 */
+		girq->num_parents = pp->irq[1] ? pp->ngpio : 1;
+		girq->parents = pp->irq;
+		girq->parent_handler_data = gpio;
+		girq->parent_handler = dwapb_irq_handler;
 	} else {
 		/*
 		 * Request a shared IRQ since where MFD would have devices
@@ -436,33 +437,15 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 				       IRQF_SHARED, DWAPB_DRIVER_NAME, gpio);
 		if (err) {
 			dev_err(gpio->dev, "error requesting IRQ\n");
-			irq_domain_remove(gpio->domain);
-			gpio->domain = NULL;
 			return;
 		}
+		/* This will let us handle the parent IRQ in the driver */
+		girq->parents = NULL;
+		girq->num_parents = 0;
+		girq->parent_handler = NULL;
 	}
 
-	for (hwirq = 0; hwirq < ngpio; hwirq++)
-		irq_create_mapping(gpio->domain, hwirq);
-
-	port->gc.to_irq = dwapb_gpio_to_irq;
-}
-
-static void dwapb_irq_teardown(struct dwapb_gpio *gpio)
-{
-	struct dwapb_gpio_port *port = &gpio->ports[0];
-	struct gpio_chip *gc = &port->gc;
-	unsigned int ngpio = gc->ngpio;
-	irq_hw_number_t hwirq;
-
-	if (!gpio->domain)
-		return;
-
-	for (hwirq = 0; hwirq < ngpio; hwirq++)
-		irq_dispose_mapping(irq_find_mapping(gpio->domain, hwirq));
-
-	irq_domain_remove(gpio->domain);
-	gpio->domain = NULL;
+	girq->chip = &gpio->irqchip;
 }
 
 static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
@@ -517,9 +500,6 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 		return err;
 	}
 
-	/* Add GPIO-signaled ACPI event support */
-	acpi_gpiochip_request_interrupts(&port->gc);
-
 	port->is_registered = true;
 
 	return 0;
@@ -535,7 +515,6 @@ static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
 		if (!port->is_registered)
 			continue;
 
-		acpi_gpiochip_free_interrupts(&port->gc);
 		gpiochip_remove(&port->gc);
 	}
 }
@@ -699,7 +678,6 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 
 out_unregister:
 	dwapb_gpio_unregister(gpio);
-	dwapb_irq_teardown(gpio);
 	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return err;
@@ -710,7 +688,6 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
 	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
 
 	dwapb_gpio_unregister(gpio);
-	dwapb_irq_teardown(gpio);
 	reset_control_assert(gpio->rst);
 	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
-- 
2.26.2

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Andy Shevchenko]

On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:
> GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
> top of a GPIO chip. It's better from maintainability and readability
> point of view to use one instead of supporting a hand-written Generic
> IRQ-chip-based implementation. Moreover the new implementation won't
> cause much functional overhead but will provide a cleaner driver code.
> All of that makes the DW APB GPIO driver conversion pretty much justified
> especially seeing a tendency of the other GPIO drivers getting converted
> too.
> 
> Here is what we do in the framework of this commit to convert the driver
> to using the GPIO-lib-based IRQ-chip interface:
> 1) IRQ ack, mask and unmask callbacks are locally defined instead of
> using the Generic IRQ-chip ones.
> 2) An irq_chip structure instance is embedded into the dwapb_gpio
> private data. Note we can't have a static instance of that structure since
> GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks().
> A warning about that would have been printed by the GPIO-lib code if we
> used a single irq_chip structure instance for multiple DW APB GPIO
> controllers.
> 3) Initialize the gpio_irq_chip structure embedded into the gpio_chip
> descriptor. By default there is no IRQ enabled so any event raised will be
> handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core
> is synthesized to have non-shared reference IRQ-lines, then as before the
> hierarchical and cascaded cases are distinguished by checking how many
> parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't
> initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP
> is used on a platform with shared IRQ line, then we simply won't let the
> GPIO-lib to initialize the parental IRQs, but will handle them locally in
> the driver.
> 4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since
> GPIO-lib IRQ-chip interface will create a new domain and accept a standard
> IRQ-chip structure pointer based on the setting we provided in the
> gpio_irq_chip structure.
> 5) Manually select a proper IRQ flow handler directly in the
> irq_set_type() callback by calling irq_set_handler_locked() method, since
> an ordinary (not Generic) irq_chip descriptor is now utilized.

Can you also emphasize that this make no regression to the 6a2f4b7dadd5 ("gpio:
dwapb: use a second irq chip")?

(And I hope you have means to test that scenario, because in my case I have
 only one IRQ and it's actually as input from other GPIO IRQ chip).

> 6) Discard the custom GPIO-to-IRQ mapping function since GPIO-lib defines
> the standard method gpiochip_to_irq(), which will be used anyway no matter
> whether the custom to_irq callback is specified or not.
> 7) Discard the acpi_gpiochip_{request,free}_interrupts()
> invocations, since they will be called from
> gpiochip_add_irqchip()/gpiochip_irqchip_remove() anyway.
> 8) Alter CONFIG_GPIO_DWAPB kernel config to select
> CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.

I like the idea, but is it possible to split this?

...

>  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;
> +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> +	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
>  	irq_hw_number_t bit = irqd_to_hwirq(d);
>  	unsigned long level, polarity, flags;
> +	irq_flow_handler_t handler;
>  
>  	if (type & ~IRQ_TYPE_SENSE_MASK)
>  		return -EINVAL;
> @@ -274,26 +304,31 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>  	case IRQ_TYPE_EDGE_BOTH:
>  		level |= BIT(bit);
>  		dwapb_toggle_trigger(gpio, bit);
> +		handler = handle_edge_irq;
>  		break;
>  	case IRQ_TYPE_EDGE_RISING:
>  		level |= BIT(bit);
>  		polarity |= BIT(bit);
> +		handler = handle_edge_irq;
>  		break;
>  	case IRQ_TYPE_EDGE_FALLING:
>  		level |= BIT(bit);
>  		polarity &= ~BIT(bit);
> +		handler = handle_edge_irq;
>  		break;
>  	case IRQ_TYPE_LEVEL_HIGH:
>  		level &= ~BIT(bit);
>  		polarity |= BIT(bit);
> +		handler = handle_level_irq;
>  		break;
>  	case IRQ_TYPE_LEVEL_LOW:
>  		level &= ~BIT(bit);
>  		polarity &= ~BIT(bit);
> +		handler = handle_level_irq;
>  		break;
>  	}
>  
> -	irq_setup_alt_chip(d, type);
> +	irq_set_handler_locked(d, handler);

Can we rather do like other GPIO IRQ chip implementations are doing, i.e.
instead of repeating same handler in each branch, use one conditional:

	if (type & IRQ_TYPE_LEVEL_MASK) {
		...
		irq_set_handler_locked(d, handle_level_irq);
	} else if (type & IRQ_TYPE_EDGE_BOTH) {
		...
		irq_set_handler_locked(d, handle_edge_irq);
	}

?

...

> +		/*
> +		 * If more than one IRQ line is specified then try to
> +		 * initialize the hierarchical interrupts. Otherwise it's
> +		 * a simple cascaded case with a common IRQ signal.
> +		 */
> +		girq->num_parents = pp->irq[1] ? pp->ngpio : 1;

Can it be sparse in the array? (It's actually the main point why I went with
memchr_inv() instead of doing something like above)

> +		girq->parents = pp->irq;
> +		girq->parent_handler_data = gpio;
> +		girq->parent_handler = dwapb_irq_handler;

...

+ blank line.

> +		/* This will let us handle the parent IRQ in the driver */
> +		girq->parents = NULL;
> +		girq->num_parents = 0;
> +		girq->parent_handler = NULL;

Shan't we do this before request_irq() call (at least for consistency with the
rest of the drivers)?

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Serge Semin]

On Thu, Jul 23, 2020 at 01:03:17PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:
> > GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
> > top of a GPIO chip. It's better from maintainability and readability
> > point of view to use one instead of supporting a hand-written Generic
> > IRQ-chip-based implementation. Moreover the new implementation won't
> > cause much functional overhead but will provide a cleaner driver code.
> > All of that makes the DW APB GPIO driver conversion pretty much justified
> > especially seeing a tendency of the other GPIO drivers getting converted
> > too.
> > 
> > Here is what we do in the framework of this commit to convert the driver
> > to using the GPIO-lib-based IRQ-chip interface:
> > 1) IRQ ack, mask and unmask callbacks are locally defined instead of
> > using the Generic IRQ-chip ones.
> > 2) An irq_chip structure instance is embedded into the dwapb_gpio
> > private data. Note we can't have a static instance of that structure since
> > GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks().
> > A warning about that would have been printed by the GPIO-lib code if we
> > used a single irq_chip structure instance for multiple DW APB GPIO
> > controllers.
> > 3) Initialize the gpio_irq_chip structure embedded into the gpio_chip
> > descriptor. By default there is no IRQ enabled so any event raised will be
> > handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core
> > is synthesized to have non-shared reference IRQ-lines, then as before the
> > hierarchical and cascaded cases are distinguished by checking how many
> > parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't
> > initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP
> > is used on a platform with shared IRQ line, then we simply won't let the
> > GPIO-lib to initialize the parental IRQs, but will handle them locally in
> > the driver.
> > 4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since
> > GPIO-lib IRQ-chip interface will create a new domain and accept a standard
> > IRQ-chip structure pointer based on the setting we provided in the
> > gpio_irq_chip structure.
> > 5) Manually select a proper IRQ flow handler directly in the
> > irq_set_type() callback by calling irq_set_handler_locked() method, since
> > an ordinary (not Generic) irq_chip descriptor is now utilized.
> 

> Can you also emphasize that this make no regression to the 6a2f4b7dadd5 ("gpio:
> dwapb: use a second irq chip")?

In fact I don't really see why that commit had been accepted in the first place.
Both level and edge triggered IRQ types are implemented by means of the same
callbacks and the same registers. The only handy thing in our case is the IRQ
flow handler setting in accordance with the requested IRQ type, but that
could be done by just calling irq_set_handler_locked() method without two-types
complication. The commit log says: "So we can have at runtime two users where
one is using edge and the other level." which isn't really correct since if an IRQ
line is shared it can only be requested with the same trigger flags (see the
inline comments in the __setup_irq() method definition). If an IRQ line isn't
shared, then there can't be more than one user.

Am I missing something?

> 
> (And I hope you have means to test that scenario, because in my case I have
>  only one IRQ and it's actually as input from other GPIO IRQ chip).

Alas I have DW APB GPIO with a single IRQ line attached too, so I can't test the
hierarchical case, but only the cascaded one.

> 
> > 6) Discard the custom GPIO-to-IRQ mapping function since GPIO-lib defines
> > the standard method gpiochip_to_irq(), which will be used anyway no matter
> > whether the custom to_irq callback is specified or not.
> > 7) Discard the acpi_gpiochip_{request,free}_interrupts()
> > invocations, since they will be called from
> > gpiochip_add_irqchip()/gpiochip_irqchip_remove() anyway.
> > 8) Alter CONFIG_GPIO_DWAPB kernel config to select
> > CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.
> 

> I like the idea, but is it possible to split this?

Yeah, 6) and 7) could be unpinned to dedicated patches. Thanks for noticing
this. I'll do that. But leaving the changes described before and not applying 8)
will produce buildable but not working driver. So I'd prefer to leave 8) here.

> 
> ...
> 
> >  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;
> > +	struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +	struct dwapb_gpio *gpio = to_dwapb_gpio(gc);
> >  	irq_hw_number_t bit = irqd_to_hwirq(d);
> >  	unsigned long level, polarity, flags;
> > +	irq_flow_handler_t handler;
> >  
> >  	if (type & ~IRQ_TYPE_SENSE_MASK)
> >  		return -EINVAL;
> > @@ -274,26 +304,31 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
> >  	case IRQ_TYPE_EDGE_BOTH:
> >  		level |= BIT(bit);
> >  		dwapb_toggle_trigger(gpio, bit);
> > +		handler = handle_edge_irq;
> >  		break;
> >  	case IRQ_TYPE_EDGE_RISING:
> >  		level |= BIT(bit);
> >  		polarity |= BIT(bit);
> > +		handler = handle_edge_irq;
> >  		break;
> >  	case IRQ_TYPE_EDGE_FALLING:
> >  		level |= BIT(bit);
> >  		polarity &= ~BIT(bit);
> > +		handler = handle_edge_irq;
> >  		break;
> >  	case IRQ_TYPE_LEVEL_HIGH:
> >  		level &= ~BIT(bit);
> >  		polarity |= BIT(bit);
> > +		handler = handle_level_irq;
> >  		break;
> >  	case IRQ_TYPE_LEVEL_LOW:
> >  		level &= ~BIT(bit);
> >  		polarity &= ~BIT(bit);
> > +		handler = handle_level_irq;
> >  		break;
> >  	}
> >  
> > -	irq_setup_alt_chip(d, type);
> > +	irq_set_handler_locked(d, handler);
> 

> Can we rather do like other GPIO IRQ chip implementations are doing, i.e.
> instead of repeating same handler in each branch, use one conditional:
> 
> 	if (type & IRQ_TYPE_LEVEL_MASK) {
> 		...
> 		irq_set_handler_locked(d, handle_level_irq);
> 	} else if (type & IRQ_TYPE_EDGE_BOTH) {
> 		...
> 		irq_set_handler_locked(d, handle_edge_irq);
> 	}
> 
> ?

In fact I've picked up the repeating stuff from another GPIO IRQ driver
./drivers/gpio/gpio-ep93xx.c . But your design of this code I like better.
Thanks for suggesting it.

> 
> ...
> 
> > +		/*
> > +		 * If more than one IRQ line is specified then try to
> > +		 * initialize the hierarchical interrupts. Otherwise it's
> > +		 * a simple cascaded case with a common IRQ signal.
> > +		 */
> > +		girq->num_parents = pp->irq[1] ? pp->ngpio : 1;
> 

> Can it be sparse in the array? (It's actually the main point why I went with
> memchr_inv() instead of doing something like above)

According to the DW APB GPIO databook it can be configured to provide either a
combined IRQ line or multiple interrupt signals for each GPIO. It's up to
the platform which of those signals are connected to an embedded IRQ
controller. So I guess theoretically the array values can be sparse.

Anyway now I see it's rather problematic. I didn't forget about the sparse IRQs
array case. I just thought it would work out-of-box. Before getting your comment
and digging deeper into the IRQ subsystem I had thought that it wasn't a problem
passing invalid IRQ numbers to the irq_set_chained_handler_and_data() especially
seeing zero IRQ number was supposed to be considered as invalid. That method shall
just ignore the invalid IRQs since the method irq_to_desc() calling radix_tree_lookup()
will fail to find a descriptor with invalid IRQ value and return NULL. So after
getting a NULL irq_desc the method irq_set_chained_handler_and_data() would
have stopped setting the handler. But turns out it may work only for
CONFIG_SPARSE_IRQ. If that config isn't enabled, then a very first IRQ
descriptor will be returned for zero IRQ number. That descriptor will be
initialized with the passed parent_handler callback, which isn't what we want.

So in order to fix the problem we could follow either of the next paths:
1) Just make sure the passed IRQs array is not sparse for instance by remapping
   it to be linear.
2) Move "if (gc->irq.parents[i]) irq_set_chained_handler_and_data()" statement to the
   gpiochip_add_irqchip() method.

What to you think? Linus?

> 
> > +		girq->parents = pp->irq;
> > +		girq->parent_handler_data = gpio;
> > +		girq->parent_handler = dwapb_irq_handler;
> 
> ...
> 

> + blank line.

Ok.

> 
> > +		/* This will let us handle the parent IRQ in the driver */
> > +		girq->parents = NULL;
> > +		girq->num_parents = 0;
> > +		girq->parent_handler = NULL;
> 

> Shan't we do this before request_irq() call (at least for consistency with the
> rest of the drivers)?

Technically we shan't. Please elaborate which drivers you are referring to?
Even the recent Linus' series "Use irqchip template" mostly does it in the
same order.

-Sergey

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

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Andy Shevchenko]

On Sat, Jul 25, 2020 at 2:03 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Thu, Jul 23, 2020 at 01:03:17PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:

...

> > > 5) Manually select a proper IRQ flow handler directly in the
> > > irq_set_type() callback by calling irq_set_handler_locked() method, since
> > > an ordinary (not Generic) irq_chip descriptor is now utilized.

> > Can you also emphasize that this make no regression to the 6a2f4b7dadd5 ("gpio:
> > dwapb: use a second irq chip")?
>
> In fact I don't really see why that commit had been accepted in the first place.
> Both level and edge triggered IRQ types are implemented by means of the same
> callbacks and the same registers. The only handy thing in our case is the IRQ
> flow handler setting in accordance with the requested IRQ type, but that
> could be done by just calling irq_set_handler_locked() method without two-types
> complication. The commit log says: "So we can have at runtime two users where
> one is using edge and the other level." which isn't really correct since if an IRQ
> line is shared it can only be requested with the same trigger flags (see the
> inline comments in the __setup_irq() method definition). If an IRQ line isn't
> shared, then there can't be more than one user.
>
> Am I missing something?

I didn't investigate myself, but probably it's a history of changes
you are missing.
That said, in time when the above mentioned commit was made there was
no clear approach like we have nowadays.
But I might be mistaken.
In any case, just add a (small) remark that you were aware of that
change and do not see any problems while doing yours.

> > (And I hope you have means to test that scenario, because in my case I have
> >  only one IRQ and it's actually as input from other GPIO IRQ chip).
>
> Alas I have DW APB GPIO with a single IRQ line attached too, so I can't test the
> hierarchical case, but only the cascaded one.

Alas.

...

> > I like the idea, but is it possible to split this?
>
> Yeah, 6) and 7) could be unpinned to dedicated patches. Thanks for noticing
> this. I'll do that. But leaving the changes described before and not applying 8)
> will produce buildable but not working driver. So I'd prefer to leave 8) here.

I see. Yes, we have to have compile time *and* run-time bisectability in place.

...

> > > +           /*
> > > +            * If more than one IRQ line is specified then try to
> > > +            * initialize the hierarchical interrupts. Otherwise it's
> > > +            * a simple cascaded case with a common IRQ signal.
> > > +            */
> > > +           girq->num_parents = pp->irq[1] ? pp->ngpio : 1;
> >
>
> > Can it be sparse in the array? (It's actually the main point why I went with
> > memchr_inv() instead of doing something like above)
>
> According to the DW APB GPIO databook it can be configured to provide either a
> combined IRQ line or multiple interrupt signals for each GPIO. It's up to
> the platform which of those signals are connected to an embedded IRQ
> controller. So I guess theoretically the array values can be sparse.
>
> Anyway now I see it's rather problematic. I didn't forget about the sparse IRQs
> array case. I just thought it would work out-of-box. Before getting your comment
> and digging deeper into the IRQ subsystem I had thought that it wasn't a problem
> passing invalid IRQ numbers to the irq_set_chained_handler_and_data() especially
> seeing zero IRQ number was supposed to be considered as invalid. That method shall
> just ignore the invalid IRQs since the method irq_to_desc() calling radix_tree_lookup()
> will fail to find a descriptor with invalid IRQ value and return NULL. So after
> getting a NULL irq_desc the method irq_set_chained_handler_and_data() would
> have stopped setting the handler. But turns out it may work only for
> CONFIG_SPARSE_IRQ. If that config isn't enabled, then a very first IRQ
> descriptor will be returned for zero IRQ number. That descriptor will be
> initialized with the passed parent_handler callback, which isn't what we want.
>
> So in order to fix the problem we could follow either of the next paths:
> 1) Just make sure the passed IRQs array is not sparse for instance by remapping
>    it to be linear.
> 2) Move "if (gc->irq.parents[i]) irq_set_chained_handler_and_data()" statement to the
>    gpiochip_add_irqchip() method.
>
> What to you think? Linus?

I am okay with either that Linus will like.

...

> > > +           /* This will let us handle the parent IRQ in the driver */
> > > +           girq->parents = NULL;
> > > +           girq->num_parents = 0;
> > > +           girq->parent_handler = NULL;

> > Shan't we do this before request_irq() call (at least for consistency with the
> > rest of the drivers)?
>
> Technically we shan't. Please elaborate which drivers you are referring to?

All of them? Recent patches for IRQ chip template do something like

girq = &...;
girq->foo = bar;
...
ret = request_irq(...);

...and here no more girq->baz = gaz; lines.

> Even the recent Linus' series "Use irqchip template" mostly does it in the
> same order.

Funny, that's what I;m referring to.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Serge Semin]

On Sat, Jul 25, 2020 at 03:12:49PM +0300, Andy Shevchenko wrote:
> On Sat, Jul 25, 2020 at 2:03 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> > On Thu, Jul 23, 2020 at 01:03:17PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:

...
 
> > > > +           /* This will let us handle the parent IRQ in the driver */
> > > > +           girq->parents = NULL;
> > > > +           girq->num_parents = 0;
> > > > +           girq->parent_handler = NULL;
> 
> > > Shan't we do this before request_irq() call (at least for consistency with the
> > > rest of the drivers)?
> >
> > Technically we shan't. Please elaborate which drivers you are referring to?
> 
> All of them? Recent patches for IRQ chip template do something like
> 
> girq = &...;
> girq->foo = bar;
> ...
> ret = request_irq(...);
> 
> ...and here no more girq->baz = gaz; lines.
> 
> > Even the recent Linus' series "Use irqchip template" mostly does it in the
> > same order.
> 
> Funny, that's what I;m referring to.

It turns out my "mostly" was wrong in this matter. It's 4 out of 17 patches,
which make the initialization in the same order as mine:
drivers/gpio/gpio-max732x.c
drivers/gpio/gpio-pca953x.c
drivers/gpio/gpio-pcf857x.c
drivers/gpio/gpio-adp5588.c

while the rest of them does it in the order suggested by you:
drivers/gpio/gpio-pci-idio-16.c
drivers/gpio/gpio-pcie-idio-24.c
drivers/gpio/gpio-104-idio-16.c
drivers/gpio/gpio-104-dio-48e.c
drivers/gpio/gpio-ws16c48.c
drivers/gpio/gpio-rcar.c
drivers/gpio/gpio-wcove.c
drivers/pinctrl/pinctrl-amd.c
drivers/gpio/gpio-crystalcove.c
drivers/pinctrl/pinctrl-mcp23s08.c
drivers/pinctrl/pinctrl-sx150x.c
drivers/pinctrl/pinctrl-stmfx.c
drivers/gpio/gpio-tc3589x.c

Then, let's use the same order here as the most of the drivers do just for
consistency.

-Sergey 

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

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Linus Walleij]

On Mon, Jul 27, 2020 at 11:50 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:

> It turns out my "mostly" was wrong in this matter. It's 4 out of 17 patches,
> which make the initialization in the same order as mine:

I'll think about fixing them up to all look the same at some point
if noone beats me to it. Sorry for the mess, I was just hacking
along to get this work item finalized.

Yours,
Linus Walleij

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Andy Shevchenko]

On Tue, Jul 28, 2020 at 11:18 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> On Mon, Jul 27, 2020 at 11:50 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
>
> > It turns out my "mostly" was wrong in this matter. It's 4 out of 17 patches,
> > which make the initialization in the same order as mine:
>
> I'll think about fixing them up to all look the same at some point
> if noone beats me to it. Sorry for the mess, I was just hacking
> along to get this work item finalized.

I have sent three patches (two updates according to above matter and
one is a fix on top of your template clean up I missed myself).

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Linus Walleij]

On Sat, Jul 25, 2020 at 1:03 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:

> According to the DW APB GPIO databook it can be configured to provide either a
> combined IRQ line or multiple interrupt signals for each GPIO. It's up to
> the platform which of those signals are connected to an embedded IRQ
> controller. So I guess theoretically the array values can be sparse.
>
> Anyway now I see it's rather problematic. I didn't forget about the sparse IRQs
> array case. I just thought it would work out-of-box. Before getting your comment
> and digging deeper into the IRQ subsystem I had thought that it wasn't a problem
> passing invalid IRQ numbers to the irq_set_chained_handler_and_data() especially
> seeing zero IRQ number was supposed to be considered as invalid. That method shall
> just ignore the invalid IRQs since the method irq_to_desc() calling radix_tree_lookup()
> will fail to find a descriptor with invalid IRQ value and return NULL. So after
> getting a NULL irq_desc the method irq_set_chained_handler_and_data() would
> have stopped setting the handler. But turns out it may work only for
> CONFIG_SPARSE_IRQ. If that config isn't enabled, then a very first IRQ
> descriptor will be returned for zero IRQ number. That descriptor will be
> initialized with the passed parent_handler callback, which isn't what we want.

Ouch but different beahviour on the outside of the irqchip API depending
on whether IRQs are sparse or not on some particular system seems to
be a problem with irqchip reallty, if we wanna get to the bottom of things.
(paging Marc)

> So in order to fix the problem we could follow either of the next paths:
> 1) Just make sure the passed IRQs array is not sparse for instance by remapping
>    it to be linear.
> 2) Move "if (gc->irq.parents[i]) irq_set_chained_handler_and_data()" statement to the
>    gpiochip_add_irqchip() method.
>
> What to you think? Linus?

What about (3) fixing irqchip?

Else (2), making the code inside gpiolib be careful and skip over
invalid IRQs.

Yours,
Linus Walleij

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Serge Semin]

On Mon, Jul 27, 2020 at 12:22:28AM +0200, Linus Walleij wrote:
> On Sat, Jul 25, 2020 at 1:03 AM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> 
> > According to the DW APB GPIO databook it can be configured to provide either a
> > combined IRQ line or multiple interrupt signals for each GPIO. It's up to
> > the platform which of those signals are connected to an embedded IRQ
> > controller. So I guess theoretically the array values can be sparse.
> >
> > Anyway now I see it's rather problematic. I didn't forget about the sparse IRQs
> > array case. I just thought it would work out-of-box. Before getting your comment
> > and digging deeper into the IRQ subsystem I had thought that it wasn't a problem
> > passing invalid IRQ numbers to the irq_set_chained_handler_and_data() especially
> > seeing zero IRQ number was supposed to be considered as invalid. That method shall
> > just ignore the invalid IRQs since the method irq_to_desc() calling radix_tree_lookup()
> > will fail to find a descriptor with invalid IRQ value and return NULL. So after
> > getting a NULL irq_desc the method irq_set_chained_handler_and_data() would
> > have stopped setting the handler. But turns out it may work only for
> > CONFIG_SPARSE_IRQ. If that config isn't enabled, then a very first IRQ
> > descriptor will be returned for zero IRQ number. That descriptor will be
> > initialized with the passed parent_handler callback, which isn't what we want.
> 
> Ouch but different beahviour on the outside of the irqchip API depending
> on whether IRQs are sparse or not on some particular system seems to
> be a problem with irqchip reallty, if we wanna get to the bottom of things.
> (paging Marc)
> 
> > So in order to fix the problem we could follow either of the next paths:
> > 1) Just make sure the passed IRQs array is not sparse for instance by remapping
> >    it to be linear.
> > 2) Move "if (gc->irq.parents[i]) irq_set_chained_handler_and_data()" statement to the
> >    gpiochip_add_irqchip() method.
> >
> > What to you think? Linus?
> 

> What about (3) fixing irqchip?
> 
> Else (2), making the code inside gpiolib be careful and skip over
> invalid IRQs.

Sorry for a delay with a response to this issue. I had to give it a more thorough
thought since the problem is a bit more complex than it seemed originally. As I
see it now It might be wrong to implement the cases 2) and 3), but 1) is more
appropriate.

First of all we need to note that GPIOlib framework provides the next parameters
to describe the IRQ-chip:
gc->irq.num_parents - number of parental IRQ numbers.
gc->irq.parents[] - array of parental IRQ numbers.
*gc->irq.valid_mask - a mask of IRQ/GPIO lines describing a valid IRQ.
*gc->irq.map - mapping of hw IRQ/GPIO ID -> parental IRQ numbers.

Using that set we can handle any case of linear and sparse parental IRQs. Here
is how it can be implemented in the framework of DW APB GPIO controller.

DW APB GPIO can be synthesized with two configs:
1) Combined IRQ line (GPIO_INTR_IO == True),
2) Multiple interrupt signals for each GPIO (GPIO_INTR_IO == False).

Obviously the former case is trivial:

     IRQ_combined
    ______^________
   /_ _ _ _ _ ___ _\
   |_|_|_|_|_|...|_| - GPIOs

In that case
gc->irq.num_parents = 1;
gc->irq.parents[0] = IRQ_combined;
*gc->irq.valid_mask = GENMASK(ngpio - 1, 0); // This is done by the GPIOlib core itself.

The later one (when multiple interrupt signals are involved) can be a bit more
complicated. It can be also split up into two cases:
2a) One-on-one GPIO-IRQ mapping.
2b) Sparse GPIO-IRQ mapping.

It's straightforward to implement 2a):

   i1i2i3i4i5 ... iN
    _ _ _ _ _ ___ _
   |_|_|_|_|_|...|_| - GPIOs

In that case
gc->irq.num_parents = ngpio;
gc->irq.parents[] = {i1, i2, i3, i4, i5, ... iN};
gc->irq.map = {i1, i2, i3, i4, i5, ... iN};
*gc->irq.valid_mask = GENMASK(ngpio - 1, 0);

The complication starts when we get to implementing 2b):

   i1 xi3i4 x ... iN
    _ _ _ _ _ ___ _
   |_|_|_|_|_|...|_| - GPIOs

In order to cover this case we need to answer on two question.
Firstly how to get such platform config? I am not sure about ACPI, but
aside from straightforward platform_data-based setup such configuration
can be reached by setting up the "interrupts-extended" DT-property with
zeroed phandle.

Ok, since it's possible to meet such platform config, we need to think
how to handle it and here is the second question. How to describe such
case in the framework of GPIOlib-IRQchip?

So from my side it was wrong to set the sparse IRQs array to
gc->irq.parents. Instead I should have scanned the sparse IRQs array,
calculated the number of non-empty parental IRQs, created an array of linear
(non-sparse) IRQs, initialized *gc->irq.valid_mask in accordance with the
sparse parental IRQs array. In other words it was wrong to assume, that
each gc->irq.parents entry corresponds to the IRQ/GPIO line. The gc->irq.parents
array just describes the parental IRQs and nothing else.

Shortly speaking here is how the GPIOlib IRQchip parameters should be
initialized in this case:
gc->irq.num_parents - number of valid parental IRQs.
gc->irq.parents - non-sparse, linear array of valid IRQs.
*gc->irq.valid_mask - mask initialized by means of the gc->irq.init_valid_mask()
callback, which indicates valid IRQ/GPIO IDs.
*gc->irq.map - sparse array of parental IRQ numbers (which I mistakenly tried to
pass through the gc->irq.parents pointer).

After that GPIOlib IRQchip should work just fine without need to be patched 
in order to check whether the passed parental IRQs are valid or not.

Please correct me if I am wrong in some aspects of the solution described above.
I'll send a fix of the problem shortly.

-Sergey

> 
> Yours,
> Linus Walleij

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Andy Shevchenko]

On Wed, Jul 29, 2020 at 3:58 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:
> On Mon, Jul 27, 2020 at 12:22:28AM +0200, Linus Walleij wrote:

...

> Sorry for a delay with a response to this issue. I had to give it a more thorough
> thought since the problem is a bit more complex than it seemed originally. As I
> see it now It might be wrong to implement the cases 2) and 3), but 1) is more
> appropriate.
>
> First of all we need to note that GPIOlib framework provides the next parameters
> to describe the IRQ-chip:
> gc->irq.num_parents - number of parental IRQ numbers.
> gc->irq.parents[] - array of parental IRQ numbers.
> *gc->irq.valid_mask - a mask of IRQ/GPIO lines describing a valid IRQ.
> *gc->irq.map - mapping of hw IRQ/GPIO ID -> parental IRQ numbers.
>
> Using that set we can handle any case of linear and sparse parental IRQs. Here
> is how it can be implemented in the framework of DW APB GPIO controller.
>
> DW APB GPIO can be synthesized with two configs:
> 1) Combined IRQ line (GPIO_INTR_IO == True),
> 2) Multiple interrupt signals for each GPIO (GPIO_INTR_IO == False).
>
> Obviously the former case is trivial:
>
>      IRQ_combined
>     ______^________
>    /_ _ _ _ _ ___ _\
>    |_|_|_|_|_|...|_| - GPIOs
>
> In that case
> gc->irq.num_parents = 1;
> gc->irq.parents[0] = IRQ_combined;
> *gc->irq.valid_mask = GENMASK(ngpio - 1, 0); // This is done by the GPIOlib core itself.
>
> The later one (when multiple interrupt signals are involved) can be a bit more
> complicated. It can be also split up into two cases:
> 2a) One-on-one GPIO-IRQ mapping.
> 2b) Sparse GPIO-IRQ mapping.
>
> It's straightforward to implement 2a):
>
>    i1i2i3i4i5 ... iN
>     _ _ _ _ _ ___ _
>    |_|_|_|_|_|...|_| - GPIOs
>
> In that case
> gc->irq.num_parents = ngpio;
> gc->irq.parents[] = {i1, i2, i3, i4, i5, ... iN};
> gc->irq.map = {i1, i2, i3, i4, i5, ... iN};
> *gc->irq.valid_mask = GENMASK(ngpio - 1, 0);
>

This case puzzles me. Why is it not NULL and 0 and actually you handle
everything as a nested case?

> The complication starts when we get to implementing 2b):
>
>    i1 xi3i4 x ... iN
>     _ _ _ _ _ ___ _
>    |_|_|_|_|_|...|_| - GPIOs

So does this.

Valid mask will define exactly GPIOs that are IRQs. So, we will handle
only nested IRQs which are valid.

> In order to cover this case we need to answer on two question.
> Firstly how to get such platform config? I am not sure about ACPI, but
> aside from straightforward platform_data-based setup such configuration
> can be reached by setting up the "interrupts-extended" DT-property with
> zeroed phandle.
>
> Ok, since it's possible to meet such platform config, we need to think
> how to handle it and here is the second question. How to describe such
> case in the framework of GPIOlib-IRQchip?
>
> So from my side it was wrong to set the sparse IRQs array to
> gc->irq.parents. Instead I should have scanned the sparse IRQs array,
> calculated the number of non-empty parental IRQs, created an array of linear
> (non-sparse) IRQs, initialized *gc->irq.valid_mask in accordance with the
> sparse parental IRQs array. In other words it was wrong to assume, that
> each gc->irq.parents entry corresponds to the IRQ/GPIO line. The gc->irq.parents
> array just describes the parental IRQs and nothing else.
>
> Shortly speaking here is how the GPIOlib IRQchip parameters should be
> initialized in this case:
> gc->irq.num_parents - number of valid parental IRQs.
> gc->irq.parents - non-sparse, linear array of valid IRQs.
> *gc->irq.valid_mask - mask initialized by means of the gc->irq.init_valid_mask()
> callback, which indicates valid IRQ/GPIO IDs.
> *gc->irq.map - sparse array of parental IRQ numbers (which I mistakenly tried to
> pass through the gc->irq.parents pointer).
>
> After that GPIOlib IRQchip should work just fine without need to be patched
> in order to check whether the passed parental IRQs are valid or not.
>
> Please correct me if I am wrong in some aspects of the solution described above.
> I'll send a fix of the problem shortly.

Maybe I'm missing something, but looks like you are solving the issue
which is not so complex / doesn't exist.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Serge Semin]

On Wed, Jul 29, 2020 at 06:10:24PM +0300, Andy Shevchenko wrote:
> On Wed, Jul 29, 2020 at 3:58 PM Serge Semin
> <Sergey.Semin@baikalelectronics.ru> wrote:
> > On Mon, Jul 27, 2020 at 12:22:28AM +0200, Linus Walleij wrote:
> 
> ...
> 
> > Sorry for a delay with a response to this issue. I had to give it a more thorough
> > thought since the problem is a bit more complex than it seemed originally. As I
> > see it now It might be wrong to implement the cases 2) and 3), but 1) is more
> > appropriate.
> >
> > First of all we need to note that GPIOlib framework provides the next parameters
> > to describe the IRQ-chip:
> > gc->irq.num_parents - number of parental IRQ numbers.
> > gc->irq.parents[] - array of parental IRQ numbers.
> > *gc->irq.valid_mask - a mask of IRQ/GPIO lines describing a valid IRQ.
> > *gc->irq.map - mapping of hw IRQ/GPIO ID -> parental IRQ numbers.
> >
> > Using that set we can handle any case of linear and sparse parental IRQs. Here
> > is how it can be implemented in the framework of DW APB GPIO controller.
> >
> > DW APB GPIO can be synthesized with two configs:
> > 1) Combined IRQ line (GPIO_INTR_IO == True),
> > 2) Multiple interrupt signals for each GPIO (GPIO_INTR_IO == False).
> >
> > Obviously the former case is trivial:
> >
> >      IRQ_combined
> >     ______^________
> >    /_ _ _ _ _ ___ _\
> >    |_|_|_|_|_|...|_| - GPIOs
> >
> > In that case
> > gc->irq.num_parents = 1;
> > gc->irq.parents[0] = IRQ_combined;
> > *gc->irq.valid_mask = GENMASK(ngpio - 1, 0); // This is done by the GPIOlib core itself.
> >
> > The later one (when multiple interrupt signals are involved) can be a bit more
> > complicated. It can be also split up into two cases:
> > 2a) One-on-one GPIO-IRQ mapping.
> > 2b) Sparse GPIO-IRQ mapping.
> >
> > It's straightforward to implement 2a):
> >
> >    i1i2i3i4i5 ... iN
> >     _ _ _ _ _ ___ _
> >    |_|_|_|_|_|...|_| - GPIOs
> >

> > In that case
> > gc->irq.num_parents = ngpio;
> > gc->irq.parents[] = {i1, i2, i3, i4, i5, ... iN};
> > gc->irq.map = {i1, i2, i3, i4, i5, ... iN};
> > *gc->irq.valid_mask = GENMASK(ngpio - 1, 0);
> >
> 
> This case puzzles me. Why is it not NULL and 0 and actually you handle
> everything as a nested case?

The code provided above is a sketch. Don't consider it literally. In reality
of course valid_mask will be not NULL, memory for which will be allocated by
the GPIOlib itself. The mask will be initialized by means of the
gc->irq.init_valid_mask() callback.

> 
> > The complication starts when we get to implementing 2b):
> >
> >    i1 xi3i4 x ... iN
> >     _ _ _ _ _ ___ _
> >    |_|_|_|_|_|...|_| - GPIOs
> 
> So does this.
> 
> Valid mask will define exactly GPIOs that are IRQs. So, we will handle
> only nested IRQs which are valid.

Right.

> 
> > In order to cover this case we need to answer on two question.
> > Firstly how to get such platform config? I am not sure about ACPI, but
> > aside from straightforward platform_data-based setup such configuration
> > can be reached by setting up the "interrupts-extended" DT-property with
> > zeroed phandle.
> >
> > Ok, since it's possible to meet such platform config, we need to think
> > how to handle it and here is the second question. How to describe such
> > case in the framework of GPIOlib-IRQchip?
> >
> > So from my side it was wrong to set the sparse IRQs array to
> > gc->irq.parents. Instead I should have scanned the sparse IRQs array,
> > calculated the number of non-empty parental IRQs, created an array of linear
> > (non-sparse) IRQs, initialized *gc->irq.valid_mask in accordance with the
> > sparse parental IRQs array. In other words it was wrong to assume, that
> > each gc->irq.parents entry corresponds to the IRQ/GPIO line. The gc->irq.parents
> > array just describes the parental IRQs and nothing else.
> >
> > Shortly speaking here is how the GPIOlib IRQchip parameters should be
> > initialized in this case:
> > gc->irq.num_parents - number of valid parental IRQs.
> > gc->irq.parents - non-sparse, linear array of valid IRQs.
> > *gc->irq.valid_mask - mask initialized by means of the gc->irq.init_valid_mask()
> > callback, which indicates valid IRQ/GPIO IDs.
> > *gc->irq.map - sparse array of parental IRQ numbers (which I mistakenly tried to
> > pass through the gc->irq.parents pointer).
> >
> > After that GPIOlib IRQchip should work just fine without need to be patched
> > in order to check whether the passed parental IRQs are valid or not.
> >
> > Please correct me if I am wrong in some aspects of the solution described above.
> > I'll send a fix of the problem shortly.
> 

> Maybe I'm missing something, but looks like you are solving the issue
> which is not so complex / doesn't exist.

As I see it now the problem was to provide a suitable config for GPIO-lib IRQ-chip
so one would correctly perceive our DW APB GPIO-IRQ setup with single/sparse/linear
IRQs. My point in the message before was to explain how that problem could be solved
without patching GPIO-lib or IRQ-chip implementations. I shared my thoughts to make
sure the suggested solution is correct, to make sure I didn't miss something in
my considerations.

-Sergey

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

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Linus Walleij]

Hi Serge,

On Thu, Jul 23, 2020 at 3:39 AM Serge Semin
<Sergey.Semin@baikalelectronics.ru> wrote:

> GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
> top of a GPIO chip. It's better from maintainability and readability
> point of view to use one instead of supporting a hand-written Generic
> IRQ-chip-based implementation. Moreover the new implementation won't
> cause much functional overhead but will provide a cleaner driver code.
> All of that makes the DW APB GPIO driver conversion pretty much justified
> especially seeing a tendency of the other GPIO drivers getting converted
> too.

Needless to say I am a big fan of this patch. It's what I wanted to
do with the driver but was afraid to dry-code.

Please look into the minor nits pointed out by Andy and respin,
I really want to apply this patch set.

Thanks,
Linus Walleij

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Andy Shevchenko]

On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:
> GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
> top of a GPIO chip. It's better from maintainability and readability
> point of view to use one instead of supporting a hand-written Generic
> IRQ-chip-based implementation. Moreover the new implementation won't
> cause much functional overhead but will provide a cleaner driver code.
> All of that makes the DW APB GPIO driver conversion pretty much justified
> especially seeing a tendency of the other GPIO drivers getting converted
> too.
> 
> Here is what we do in the framework of this commit to convert the driver
> to using the GPIO-lib-based IRQ-chip interface:
> 1) IRQ ack, mask and unmask callbacks are locally defined instead of
> using the Generic IRQ-chip ones.
> 2) An irq_chip structure instance is embedded into the dwapb_gpio
> private data. Note we can't have a static instance of that structure since
> GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks().
> A warning about that would have been printed by the GPIO-lib code if we
> used a single irq_chip structure instance for multiple DW APB GPIO
> controllers.
> 3) Initialize the gpio_irq_chip structure embedded into the gpio_chip
> descriptor. By default there is no IRQ enabled so any event raised will be
> handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core
> is synthesized to have non-shared reference IRQ-lines, then as before the
> hierarchical and cascaded cases are distinguished by checking how many
> parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't
> initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP
> is used on a platform with shared IRQ line, then we simply won't let the
> GPIO-lib to initialize the parental IRQs, but will handle them locally in
> the driver.
> 4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since
> GPIO-lib IRQ-chip interface will create a new domain and accept a standard
> IRQ-chip structure pointer based on the setting we provided in the
> gpio_irq_chip structure.
> 5) Manually select a proper IRQ flow handler directly in the
> irq_set_type() callback by calling irq_set_handler_locked() method, since
> an ordinary (not Generic) irq_chip descriptor is now utilized.
> 6) Discard the custom GPIO-to-IRQ mapping function since GPIO-lib defines
> the standard method gpiochip_to_irq(), which will be used anyway no matter
> whether the custom to_irq callback is specified or not.
> 7) Discard the acpi_gpiochip_{request,free}_interrupts()
> invocations, since they will be called from
> gpiochip_add_irqchip()/gpiochip_irqchip_remove() anyway.
> 8) Alter CONFIG_GPIO_DWAPB kernel config to select
> CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.


...

One more thing...

>  static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
>  {
> +	struct gpio_chip *gc = &gpio->ports[0].gc;
>  	unsigned long irq_status;
>  	irq_hw_number_t hwirq;
>  
>  	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
>  	for_each_set_bit(hwirq, &irq_status, 32) {
> -		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
> +		int gpio_irq = gc->to_irq(gc, hwirq);

Very, very few do this.
Can we stick with the original one?
(See plenty of other examples in the GPIO / pin control subsystems.

>  		u32 irq_type = irq_get_trigger_type(gpio_irq);
>  
>  		generic_handle_irq(gpio_irq);


>  }

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 4/7] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip

[Serge Semin]

On Thu, Jul 23, 2020 at 05:08:15PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 23, 2020 at 04:38:55AM +0300, Serge Semin wrote:
> > GPIO-lib provides a ready-to-use interface to initialize an IRQ-chip on
> > top of a GPIO chip. It's better from maintainability and readability
> > point of view to use one instead of supporting a hand-written Generic
> > IRQ-chip-based implementation. Moreover the new implementation won't
> > cause much functional overhead but will provide a cleaner driver code.
> > All of that makes the DW APB GPIO driver conversion pretty much justified
> > especially seeing a tendency of the other GPIO drivers getting converted
> > too.
> > 
> > Here is what we do in the framework of this commit to convert the driver
> > to using the GPIO-lib-based IRQ-chip interface:
> > 1) IRQ ack, mask and unmask callbacks are locally defined instead of
> > using the Generic IRQ-chip ones.
> > 2) An irq_chip structure instance is embedded into the dwapb_gpio
> > private data. Note we can't have a static instance of that structure since
> > GPIO-lib will add some hooks into it by calling gpiochip_set_irq_hooks().
> > A warning about that would have been printed by the GPIO-lib code if we
> > used a single irq_chip structure instance for multiple DW APB GPIO
> > controllers.
> > 3) Initialize the gpio_irq_chip structure embedded into the gpio_chip
> > descriptor. By default there is no IRQ enabled so any event raised will be
> > handled by the handle_bad_irq() IRQ flow handler. If DW APB GPIO IP-core
> > is synthesized to have non-shared reference IRQ-lines, then as before the
> > hierarchical and cascaded cases are distinguished by checking how many
> > parental IRQs are defined. (Note irq_set_chained_handler_and_data() won't
> > initialize IRQs, which descriptors couldn't be found.) If DW APB GPIO IP
> > is used on a platform with shared IRQ line, then we simply won't let the
> > GPIO-lib to initialize the parental IRQs, but will handle them locally in
> > the driver.
> > 4) Discard linear IRQ-domain and Generic IRQ-chip initialization, since
> > GPIO-lib IRQ-chip interface will create a new domain and accept a standard
> > IRQ-chip structure pointer based on the setting we provided in the
> > gpio_irq_chip structure.
> > 5) Manually select a proper IRQ flow handler directly in the
> > irq_set_type() callback by calling irq_set_handler_locked() method, since
> > an ordinary (not Generic) irq_chip descriptor is now utilized.
> > 6) Discard the custom GPIO-to-IRQ mapping function since GPIO-lib defines
> > the standard method gpiochip_to_irq(), which will be used anyway no matter
> > whether the custom to_irq callback is specified or not.
> > 7) Discard the acpi_gpiochip_{request,free}_interrupts()
> > invocations, since they will be called from
> > gpiochip_add_irqchip()/gpiochip_irqchip_remove() anyway.
> > 8) Alter CONFIG_GPIO_DWAPB kernel config to select
> > CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.
> 
> 
> ...
> 
> One more thing...
> 
> >  static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
> >  {
> > +	struct gpio_chip *gc = &gpio->ports[0].gc;
> >  	unsigned long irq_status;
> >  	irq_hw_number_t hwirq;
> >  
> >  	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
> >  	for_each_set_bit(hwirq, &irq_status, 32) {
> > -		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
> > +		int gpio_irq = gc->to_irq(gc, hwirq);
> 

> Very, very few do this.
> Can we stick with the original one?
> (See plenty of other examples in the GPIO / pin control subsystems.

You are right. After more thorough studying the IRQ-domain code I've found out
that irq_domain_add_simple() provides the on-the-fly mapping creation. We
don't have to call irq_create_mapping() first before using irq_find_mapping().
So the irq_find_mapping(gc->irq.domain, hwirq) method can be freely called here
the same way as the most of the GPIO drivers do. Thanks for noticing this.

-Sergey

> 
> >  		u32 irq_type = irq_get_trigger_type(gpio_irq);
> >  
> >  		generic_handle_irq(gpio_irq);
> 
> 
> >  }
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
> 

[PATCH 5/7] gpio: dwapb: Get reset control by means of resource managed interface

[Serge Semin]

The reset control interface provides the resource managed version of
the reset_control_get() method. The only thing which needs to be also
automated is the reset lane assertion on the device removal. It can be
implemented by means of the custom action definition. After that the reset
control will be purely managed by the device resources interface.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3f727ebe7f9a..693cbd896f08 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -595,6 +595,32 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 	return pdata;
 }
 
+static void dwapb_assert_reset(void *data)
+{
+	struct dwapb_gpio *gpio = data;
+
+	reset_control_assert(gpio->rst);
+}
+
+static int dwapb_get_reset(struct dwapb_gpio *gpio)
+{
+	int err;
+
+	gpio->rst = devm_reset_control_get_optional_shared(gpio->dev, NULL);
+	if (IS_ERR(gpio->rst)) {
+		dev_err(gpio->dev, "Cannot get reset descriptor\n");
+		return PTR_ERR(gpio->rst);
+	}
+
+	err = reset_control_deassert(gpio->rst);
+	if (err) {
+		dev_err(gpio->dev, "Cannot deassert reset lane\n");
+		return err;
+	}
+
+	return devm_add_action_or_reset(gpio->dev, dwapb_assert_reset, gpio);
+}
+
 static const struct of_device_id dwapb_of_match[] = {
 	{ .compatible = "snps,dw-apb-gpio", .data = (void *)0},
 	{ .compatible = "apm,xgene-gpio-v2", .data = (void *)GPIO_REG_OFFSET_V2},
@@ -634,11 +660,9 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	gpio->dev = &pdev->dev;
 	gpio->nr_ports = pdata->nports;
 
-	gpio->rst = devm_reset_control_get_optional_shared(dev, NULL);
-	if (IS_ERR(gpio->rst))
-		return PTR_ERR(gpio->rst);
-
-	reset_control_deassert(gpio->rst);
+	err = dwapb_get_reset(gpio);
+	if (err)
+		return err;
 
 	gpio->ports = devm_kcalloc(&pdev->dev, gpio->nr_ports,
 				   sizeof(*gpio->ports), GFP_KERNEL);
@@ -688,7 +712,6 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
 	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
 
 	dwapb_gpio_unregister(gpio);
-	reset_control_assert(gpio->rst);
 	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return 0;
-- 
2.26.2

[PATCH 6/7] gpio: dwapb: Get clocks by means of resource managed interface

[Serge Semin]

The kernel clock framework provides the resource managed version of
the clk_bulk_get() method. The only thing which needs to be also automated
is the clocks disable/unprepare procedure executed on the device removal.
It can be implemented by means of the custom action definition. After that
the clocks acquisition and release will be purely managed by the device
resources interface.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 48 ++++++++++++++++++++++++++-------------
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 693cbd896f08..8588e45186ad 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -621,6 +621,36 @@ static int dwapb_get_reset(struct dwapb_gpio *gpio)
 	return devm_add_action_or_reset(gpio->dev, dwapb_assert_reset, gpio);
 }
 
+static void dwapb_disable_clks(void *data)
+{
+	struct dwapb_gpio *gpio = data;
+
+	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
+}
+
+static int dwapb_get_clks(struct dwapb_gpio *gpio)
+{
+	int err;
+
+	/* Optional bus and debounce clocks */
+	gpio->clks[0].id = "bus";
+	gpio->clks[1].id = "db";
+	err = devm_clk_bulk_get_optional(gpio->dev, DWAPB_NR_CLOCKS,
+					 gpio->clks);
+	if (err) {
+		dev_err(gpio->dev, "Cannot get APB/Debounce clocks\n");
+		return err;
+	}
+
+	err = clk_bulk_prepare_enable(DWAPB_NR_CLOCKS, gpio->clks);
+	if (err) {
+		dev_err(gpio->dev, "Cannot enable APB/Debounce clocks\n");
+		return err;
+	}
+
+	return devm_add_action_or_reset(gpio->dev, dwapb_disable_clks, gpio);
+}
+
 static const struct of_device_id dwapb_of_match[] = {
 	{ .compatible = "snps,dw-apb-gpio", .data = (void *)0},
 	{ .compatible = "apm,xgene-gpio-v2", .data = (void *)GPIO_REG_OFFSET_V2},
@@ -673,21 +703,9 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	if (IS_ERR(gpio->regs))
 		return PTR_ERR(gpio->regs);
 
-	/* Optional bus and debounce clocks */
-	gpio->clks[0].id = "bus";
-	gpio->clks[1].id = "db";
-	err = devm_clk_bulk_get_optional(&pdev->dev, DWAPB_NR_CLOCKS,
-					 gpio->clks);
-	if (err) {
-		dev_err(&pdev->dev, "Cannot get APB/Debounce clocks\n");
-		return err;
-	}
-
-	err = clk_bulk_prepare_enable(DWAPB_NR_CLOCKS, gpio->clks);
-	if (err) {
-		dev_err(&pdev->dev, "Cannot enable APB/Debounce clocks\n");
+	err = dwapb_get_clks(gpio);
+	if (err)
 		return err;
-	}
 
 	gpio->flags = (uintptr_t)device_get_match_data(dev);
 
@@ -702,7 +720,6 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 
 out_unregister:
 	dwapb_gpio_unregister(gpio);
-	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return err;
 }
@@ -712,7 +729,6 @@ static int dwapb_gpio_remove(struct platform_device *pdev)
 	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
 
 	dwapb_gpio_unregister(gpio);
-	clk_bulk_disable_unprepare(DWAPB_NR_CLOCKS, gpio->clks);
 
 	return 0;
 }
-- 
2.26.2

[PATCH 7/7] gpio: dwapb: Use resource managed GPIO-chip add data method

[Serge Semin]

Since the resource managed version of gpiochip_add_data() will handle the
GPIO-chip data automated cleanup we can freely remove the DW APB GPIO
driver code responsible for that. After doing so the DW APB GPIO driver
removal callback can be also fully discarded since there is nothing left
to be done for it. All the cleanups are now performed by means of the
device managed framework.

Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
 drivers/gpio/gpio-dwapb.c | 37 ++-----------------------------------
 1 file changed, 2 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 8588e45186ad..3d1767e69881 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -84,7 +84,6 @@ struct dwapb_context {
 
 struct dwapb_gpio_port {
 	struct gpio_chip	gc;
-	bool			is_registered;
 	struct dwapb_gpio	*gpio;
 #ifdef CONFIG_PM_SLEEP
 	struct dwapb_context	*ctx;
@@ -493,32 +492,16 @@ static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
 	if (pp->idx == 0)
 		dwapb_configure_irqs(gpio, port, pp);
 
-	err = gpiochip_add_data(&port->gc, port);
+	err = devm_gpiochip_add_data(gpio->dev, &port->gc, port);
 	if (err) {
 		dev_err(gpio->dev, "failed to register gpiochip for port%d\n",
 			port->idx);
 		return err;
 	}
 
-	port->is_registered = true;
-
 	return 0;
 }
 
-static void dwapb_gpio_unregister(struct dwapb_gpio *gpio)
-{
-	unsigned int m;
-
-	for (m = 0; m < gpio->nr_ports; ++m) {
-		struct dwapb_gpio_port *port = &gpio->ports[m];
-
-		if (!port->is_registered)
-			continue;
-
-		gpiochip_remove(&port->gc);
-	}
-}
-
 static void dwapb_get_irq(struct device *dev, struct fwnode_handle *fwnode,
 			  struct dwapb_port_property *pp)
 {
@@ -712,23 +695,8 @@ static int dwapb_gpio_probe(struct platform_device *pdev)
 	for (i = 0; i < gpio->nr_ports; i++) {
 		err = dwapb_gpio_add_port(gpio, &pdata->properties[i], i);
 		if (err)
-			goto out_unregister;
+			return err;
 	}
-	platform_set_drvdata(pdev, gpio);
-
-	return 0;
-
-out_unregister:
-	dwapb_gpio_unregister(gpio);
-
-	return err;
-}
-
-static int dwapb_gpio_remove(struct platform_device *pdev)
-{
-	struct dwapb_gpio *gpio = platform_get_drvdata(pdev);
-
-	dwapb_gpio_unregister(gpio);
 
 	return 0;
 }
@@ -832,7 +800,6 @@ static struct platform_driver dwapb_gpio_driver = {
 		.acpi_match_table = dwapb_acpi_match,
 	},
 	.probe		= dwapb_gpio_probe,
-	.remove		= dwapb_gpio_remove,
 };
 
 module_platform_driver(dwapb_gpio_driver);
-- 
2.26.2

Re: [PATCH 0/7] gpio: dwapb: Refactor GPIO resources initialization

[Andy Shevchenko]

On Thu, Jul 23, 2020 at 04:38:51AM +0300, Serge Semin wrote:
> This series is about the DW APB GPIO device initialization procedure
> cleaning up. First of all it has been discovered that having a
> vendor-specific "snps,nr-gpios" property isn't only redundant but also
> might be dangerous (see the commit log for details). Instead we suggest to
> use the generic "ngpios" property to define a number of GPIOs each DW APB
> GPIO controller port supports. Secondly seeing a tendency of the other
> GPIO drivers getting converted to using the GPIO-lib-based IRQ-chip
> interface this series provides a patch, which replaces the DW APB GPIO
> driver Generic IRQ-chip implementation with the GPIO-lib IRQ-chip one.
> Finally the DW APB GPIO device probe procedure is simplified by
> converting the code to be using the device managed resources for the
> reference clocks initialization, reset control assertion/de-assertion
> and GPIO-chip registration.

Thanks! For non-commented patches, excluding DT one,
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
> Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
> Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: linux-gpio@vger.kernel.org
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> 
> Serge Semin (7):
>   dt-bindings: gpio: dwapb: Add ngpios property support
>   gpio: dwapb: Add ngpios DT-property support
>   gpio: dwapb: Move MFD-specific IRQ handler
>   gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip
>   gpio: dwapb: Get reset control by means of resource managed interface
>   gpio: dwapb: Get clocks by means of resource managed interface
>   gpio: dwapb: Use resource managed GPIO-chip add data method
> 
>  .../bindings/gpio/snps,dw-apb-gpio.yaml       |   6 +
>  drivers/gpio/Kconfig                          |   2 +-
>  drivers/gpio/gpio-dwapb.c                     | 324 +++++++++---------
>  3 files changed, 161 insertions(+), 171 deletions(-)
> 
> -- 
> 2.26.2
> 

-- 
With Best Regards,
Andy Shevchenko