Linux-GPIO Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization
@ 2020-07-30 15:27 Serge Semin
  2020-07-30 15:27 ` [PATCH v3 01/10] dt-bindings: gpio: dwapb: Add ngpios property support Serge Semin
                   ` (10 more replies)
  0 siblings, 11 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:27 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski
  Cc: Serge Semin, Serge Semin, Andy Shevchenko, Andy Shevchenko,
	Alexey Malahov, Pavel Parkhomenko, Rob Herring, linux-gpio,
	devicetree, linux-kernel

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.

Some additional cleanups like replacing a number of GPIOs literal with a
corresponding macro and grouping the IRQ handlers up in a single place of
the driver are also introduced in this patchset.

Link: https://lore.kernel.org/linux-gpio/20200723013858.10766-1-Sergey.Semin@baikalelectronics.ru/
Changelog v2:
- Replace gc->to_irq() with irq_find_mapping() method.
- Refactor dwapb_irq_set_type() method to directly set IRQ flow handler
  instead of using a temporary variable.
- Initialize GPIOlib IRQ-chip settings before calling request_irq()
  method.
- Add a notice regarding regression of commit 6a2f4b7dadd5 ("gpio:
  dwapb: use a second irq chip").
- Move the acpi_gpiochip_{request,free}_interrupts() methods invocation
  removal to a dedicated patch.
- Move GPIO-chip to_irq callback removal to a dedicated patch.
- Add a patch which replaces a max number of GPIO literals with a macro.
- Introduce dwapb_convert_irqs() method to convert the sparse parental
  IRQs array into an array of linearly distributed IRQs correctly
  perceived by GPIO-lib.

Link: https://lore.kernel.org/linux-gpio/20200730135536.19747-1-Sergey.Semin@baikalelectronics.ru
Changelog v3:
- Fix patches SoB's.
- Add Andy' Suggested-by to the patch: "gpio: dwapb: Add max GPIOs macro"
- Add blank lines to the IRQ-chip conversion commit message to make it
  more readable.
- Dynamically allocate memory for the IRQ-chip-related data.

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 (10):
  dt-bindings: gpio: dwapb: Add ngpios property support
  gpio: dwapb: Add ngpios DT-property support
  gpio: dwapb: Move MFD-specific IRQ handler
  gpio: dwapb: Add max GPIOs macro
  gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip
  gpio: dwapb: Discard GPIO-to-IRQ mapping function
  gpio: dwapb: Discard ACPI GPIO-chip IRQs request
  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                     | 352 +++++++++---------
 include/linux/platform_data/gpio-dwapb.h      |   4 +-
 4 files changed, 191 insertions(+), 173 deletions(-)

-- 
2.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 01/10] dt-bindings: gpio: dwapb: Add ngpios property support
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
@ 2020-07-30 15:27 ` Serge Semin
  2020-07-30 15:27 ` [PATCH v3 02/10] gpio: dwapb: Add ngpios DT-property support Serge Semin
                   ` (9 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:27 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin, Rob Herring
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, linux-gpio, devicetree, linux-kernel,
	Rob Herring

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>
Reviewed-by: Rob Herring <robh@kernel.org>
---
 .../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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 02/10] gpio: dwapb: Add ngpios DT-property support
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
  2020-07-30 15:27 ` [PATCH v3 01/10] dt-bindings: gpio: dwapb: Add ngpios property support Serge Semin
@ 2020-07-30 15:27 ` Serge Semin
  2020-07-30 15:28 ` [PATCH v3 03/10] gpio: dwapb: Move MFD-specific IRQ handler Serge Semin
                   ` (8 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:27 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 03/10] gpio: dwapb: Move MFD-specific IRQ handler
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
  2020-07-30 15:27 ` [PATCH v3 01/10] dt-bindings: gpio: dwapb: Add ngpios property support Serge Semin
  2020-07-30 15:27 ` [PATCH v3 02/10] gpio: dwapb: Add ngpios DT-property support Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:28 ` [PATCH v3 04/10] gpio: dwapb: Add max GPIOs macro Serge Semin
                   ` (7 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

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

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 04/10] gpio: dwapb: Add max GPIOs macro
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (2 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 03/10] gpio: dwapb: Move MFD-specific IRQ handler Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:42   ` Andy Shevchenko
  2020-07-30 15:28 ` [PATCH v3 05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip Serge Semin
                   ` (6 subsequent siblings)
  10 siblings, 1 reply; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

Add a new macro DWAPB_MAX_GPIOS which defines the maximum possible number
of GPIO lines corresponding to the maximum DW APB GPIO controller port
width. Use the new macro instead of number literal 32 where it's
applicable.

Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 drivers/gpio/gpio-dwapb.c                | 8 ++++----
 include/linux/platform_data/gpio-dwapb.h | 4 +++-
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index 3081213247d8..f34001152850 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -162,7 +162,7 @@ static struct dwapb_gpio_port *dwapb_offs_to_port(struct dwapb_gpio *gpio, unsig
 
 	for (i = 0; i < gpio->nr_ports; i++) {
 		port = &gpio->ports[i];
-		if (port->idx == offs / 32)
+		if (port->idx == offs / DWAPB_MAX_GPIOS)
 			return port;
 	}
 
@@ -182,7 +182,7 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
 
 	pol = dwapb_read(gpio, GPIO_INT_POLARITY);
 	/* Just read the current value right out of the data register */
-	val = gc->get(gc, offs % 32);
+	val = gc->get(gc, offs % DWAPB_MAX_GPIOS);
 	if (val)
 		pol &= ~BIT(offs);
 	else
@@ -197,7 +197,7 @@ static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
 	irq_hw_number_t hwirq;
 
 	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
-	for_each_set_bit(hwirq, &irq_status, 32) {
+	for_each_set_bit(hwirq, &irq_status, DWAPB_MAX_GPIOS) {
 		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
 		u32 irq_type = irq_get_trigger_type(gpio_irq);
 
@@ -599,7 +599,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
 			dev_info(dev,
 				 "failed to get number of gpios for port%d\n",
 				 i);
-			pp->ngpio = 32;
+			pp->ngpio = DWAPB_MAX_GPIOS;
 		}
 
 		pp->irq_shared	= false;
diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
index ff1be737bad6..0aa5c6720259 100644
--- a/include/linux/platform_data/gpio-dwapb.h
+++ b/include/linux/platform_data/gpio-dwapb.h
@@ -6,12 +6,14 @@
 #ifndef GPIO_DW_APB_H
 #define GPIO_DW_APB_H
 
+#define DWAPB_MAX_GPIOS		32
+
 struct dwapb_port_property {
 	struct fwnode_handle *fwnode;
 	unsigned int	idx;
 	unsigned int	ngpio;
 	unsigned int	gpio_base;
-	int		irq[32];
+	int		irq[DWAPB_MAX_GPIOS];
 	bool		irq_shared;
 };
 
-- 
2.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (3 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 04/10] gpio: dwapb: Add max GPIOs macro Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:45   ` Andy Shevchenko
  2020-07-30 15:28 ` [PATCH v3 06/10] gpio: dwapb: Discard GPIO-to-IRQ mapping function Serge Semin
                   ` (5 subsequent siblings)
  10 siblings, 1 reply; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

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. Note this
shalln't give any regression

6) Alter CONFIG_GPIO_DWAPB kernel config to select
CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.

Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5
("gpio: dwapb: use a second irq chip"), since the later isn't properly
used here anyway.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Note in this patch we omit the next GPIO-lib IRQ-chip settings
initialization:
gc->irq.map
gc->irq.init_valid_mask

That's intentionally done in order to keep the driver functionality at the
same level as before the conversion: each GPIO line will be registered as
the source of IRQs, no matter whether DW APB GPIO IP is configured to
generate combined or multiple IRQ signals.

If more thorough mapping is needed, the driver should be altered to detect
the IRQ signaling mode (for instance by checking a dedicated DT-property).
Then based on that it will be able to create a proper hardware GPIO-IRQ
to/from parental IRQs mapping and valid hardware GPIO-IRQs mask.

Changelog v2:
- Replace gc->to_irq() with irq_find_mapping() method.
- Refactor dwapb_irq_set_type() method to directly set IRQ flow handler
  instead of using a temporary variable.
- Initialize GPIOlib IRQ-chip settings before calling request_irq()
  method.
- Add a notice regarding regression of commit 6a2f4b7dadd5 ("gpio:
dwapb: use a second irq chip").
- Move the acpi_gpiochip_{request,free}_interrupts() methods invocation
  removal to a dedicated commit.
- Move GPIO-chip to_irq callback removal to a dedicated commit.
- Introduce dwapb_convert_irqs() method to convert the sparse parental
  IRQs array into an array of linearly distributed IRQs correctly
  perceived by GPIO-lib.

Changelog v3:
- Add blank lines to the commit message to make it more readable.
- Dynamically allocate memory for the IRQ-chip-related data.
---
 drivers/gpio/Kconfig      |   2 +-
 drivers/gpio/gpio-dwapb.c | 203 +++++++++++++++++++++-----------------
 2 files changed, 111 insertions(+), 94 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 f34001152850..eb3a6da453ff 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>
@@ -83,8 +82,15 @@ struct dwapb_context {
 };
 #endif
 
+struct dwapb_gpio_port_irqchip {
+	struct irq_chip		irqchip;
+	unsigned int		nr_irqs;
+	unsigned int		irq[DWAPB_MAX_GPIOS];
+};
+
 struct dwapb_gpio_port {
 	struct gpio_chip	gc;
+	struct dwapb_gpio_port_irqchip *pirq;
 	bool			is_registered;
 	struct dwapb_gpio	*gpio;
 #ifdef CONFIG_PM_SLEEP
@@ -92,13 +98,14 @@ 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;
 	unsigned int		flags;
 	struct reset_control	*rst;
 	struct clk_bulk_data	clks[DWAPB_NR_CLOCKS];
@@ -193,12 +200,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, DWAPB_MAX_GPIOS) {
-		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
+		int gpio_irq = irq_find_mapping(gc->irq.domain, hwirq);
 		u32 irq_type = irq_get_trigger_type(gpio_irq);
 
 		generic_handle_irq(gpio_irq);
@@ -225,11 +233,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 +287,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,9 +301,8 @@ 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;
 
@@ -293,7 +336,10 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
 		break;
 	}
 
-	irq_setup_alt_chip(d, type);
+	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);
 
 	dwapb_write(gpio, GPIO_INTTYPE_LEVEL, level);
 	if (type != IRQ_TYPE_EDGE_BOTH)
@@ -354,79 +400,67 @@ static int dwapb_gpio_set_config(struct gpio_chip *gc, unsigned offset,
 	return dwapb_gpio_set_debounce(gc, offset, debounce);
 }
 
+static int dwapb_convert_irqs(struct dwapb_gpio_port_irqchip *pirq,
+			      struct dwapb_port_property *pp)
+{
+	int i;
+
+	/* Group all available IRQs into an array of parental IRQs. */
+	for (i = 0; i < pp->ngpio; ++i) {
+		if (!pp->irq[i])
+			continue;
+
+		pirq->irq[pirq->nr_irqs++] = pp->irq[i];
+	}
+
+	return pirq->nr_irqs ? 0 : -ENOENT;
+}
+
 static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 				 struct dwapb_gpio_port *port,
 				 struct dwapb_port_property *pp)
 {
+	struct dwapb_gpio_port_irqchip *pirq;
 	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;
-
-	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;
+	struct gpio_irq_chip *girq;
+	int err;
 
-	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;
+	pirq = devm_kzalloc(gpio->dev, sizeof(*pirq), GFP_KERNEL);
+	if (!pirq)
 		return;
-	}
 
-	irq_gc = irq_get_domain_generic_chip(gpio->domain, 0);
-	if (!irq_gc) {
-		irq_domain_remove(gpio->domain);
-		gpio->domain = NULL;
-		return;
+	if (dwapb_convert_irqs(pirq, pp)) {
+		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
+		goto err_kfree_pirq;
 	}
 
-	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;
+
+	port->pirq = pirq;
+	pirq->irqchip.name = DWAPB_DRIVER_NAME;
+	pirq->irqchip.irq_ack = dwapb_irq_ack;
+	pirq->irqchip.irq_mask = dwapb_irq_mask;
+	pirq->irqchip.irq_unmask = dwapb_irq_unmask;
+	pirq->irqchip.irq_set_type = dwapb_irq_set_type;
+	pirq->irqchip.irq_enable = dwapb_irq_enable;
+	pirq->irqchip.irq_disable = dwapb_irq_disable;
 #ifdef CONFIG_PM_SLEEP
-		ct->chip.irq_set_wake = dwapb_irq_set_wake;
+	pirq->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);
-		}
+		girq->num_parents = pirq->nr_irqs;
+		girq->parents = pirq->irq;
+		girq->parent_handler_data = gpio;
+		girq->parent_handler = dwapb_irq_handler;
 	} else {
+		/* This will let us handle the parent IRQ in the driver */
+		girq->num_parents = 0;
+		girq->parents = NULL;
+		girq->parent_handler = NULL;
+
 		/*
 		 * Request a shared IRQ since where MFD would have devices
 		 * using the same irq pin
@@ -436,33 +470,18 @@ 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;
+			goto err_kfree_pirq;
 		}
 	}
 
-	for (hwirq = 0; hwirq < ngpio; hwirq++)
-		irq_create_mapping(gpio->domain, hwirq);
+	girq->chip = &pirq->irqchip;
 
 	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));
+	return;
 
-	irq_domain_remove(gpio->domain);
-	gpio->domain = NULL;
+err_kfree_pirq:
+	devm_kfree(gpio->dev, pirq);
 }
 
 static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
@@ -699,7 +718,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 +728,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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 06/10] gpio: dwapb: Discard GPIO-to-IRQ mapping function
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (4 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:28 ` [PATCH v3 07/10] gpio: dwapb: Discard ACPI GPIO-chip IRQs request Serge Semin
                   ` (4 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

Since GPIOlib-based IRQ-chip interface is now utilized there is no need in
setting up a custom GPIO-to-IRQ mapping method. GPIO-lib defines the
standard mapping method - gpiochip_to_irq(), which will be used anyway no
matter whether the custom to_irq callback is specified or not.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

---

Changelog v2:
- This is a new patch detached from commit
  "gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip".
---
 drivers/gpio/gpio-dwapb.c | 10 ----------
 1 file changed, 10 deletions(-)

diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
index eb3a6da453ff..b6affe7161b0 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -154,14 +154,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;
@@ -476,8 +468,6 @@ static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
 
 	girq->chip = &pirq->irqchip;
 
-	port->gc.to_irq = dwapb_gpio_to_irq;
-
 	return;
 
 err_kfree_pirq:
-- 
2.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 07/10] gpio: dwapb: Discard ACPI GPIO-chip IRQs request
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (5 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 06/10] gpio: dwapb: Discard GPIO-to-IRQ mapping function Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:28 ` [PATCH v3 08/10] gpio: dwapb: Get reset control by means of resource managed interface Serge Semin
                   ` (3 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

Since GPIOlib-based IRQ-chip interface is now utilized there is no need
in calling the methods acpi_gpiochip_{request,free}_interrupts() here.
They will be called from gpiochip_add_irqchip()/gpiochip_irqchip_remove()
anyway.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

---

Changelog v2:
- This is a new patch detached from commit
  "gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip".
---
 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 b6affe7161b0..707e9087ca59 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -526,9 +526,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;
@@ -544,7 +541,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);
 	}
 }
-- 
2.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 08/10] gpio: dwapb: Get reset control by means of resource managed interface
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (6 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 07/10] gpio: dwapb: Discard ACPI GPIO-chip IRQs request Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:28 ` [PATCH v3 09/10] gpio: dwapb: Get clocks " Serge Semin
                   ` (2 subsequent siblings)
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@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 707e9087ca59..faac594287b3 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -621,6 +621,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},
@@ -660,11 +686,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);
@@ -714,7 +738,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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 09/10] gpio: dwapb: Get clocks by means of resource managed interface
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (7 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 08/10] gpio: dwapb: Get reset control by means of resource managed interface Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-07-30 15:28 ` [PATCH v3 10/10] gpio: dwapb: Use resource managed GPIO-chip add data method Serge Semin
  2020-08-27  8:34 ` [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Linus Walleij
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@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 faac594287b3..803e1d1f4b5a 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -647,6 +647,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},
@@ -699,21 +729,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);
 
@@ -728,7 +746,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;
 }
@@ -738,7 +755,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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH v3 10/10] gpio: dwapb: Use resource managed GPIO-chip add data method
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (8 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 09/10] gpio: dwapb: Get clocks " Serge Semin
@ 2020-07-30 15:28 ` Serge Semin
  2020-08-27  8:34 ` [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Linus Walleij
  10 siblings, 0 replies; 14+ messages in thread
From: Serge Semin @ 2020-07-30 15:28 UTC (permalink / raw)
  To: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin
  Cc: Serge Semin, Andy Shevchenko, Andy Shevchenko, Alexey Malahov,
	Pavel Parkhomenko, Rob Herring, linux-gpio, devicetree,
	linux-kernel

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 <Sergey.Semin@baikalelectronics.ru>
Reviewed-by: Andy Shevchenko <andy.shevchenko@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 803e1d1f4b5a..a5b326754124 100644
--- a/drivers/gpio/gpio-dwapb.c
+++ b/drivers/gpio/gpio-dwapb.c
@@ -91,7 +91,6 @@ struct dwapb_gpio_port_irqchip {
 struct dwapb_gpio_port {
 	struct gpio_chip	gc;
 	struct dwapb_gpio_port_irqchip *pirq;
-	bool			is_registered;
 	struct dwapb_gpio	*gpio;
 #ifdef CONFIG_PM_SLEEP
 	struct dwapb_context	*ctx;
@@ -519,32 +518,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)
 {
@@ -738,23 +721,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;
 }
@@ -858,7 +826,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.27.0


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 04/10] gpio: dwapb: Add max GPIOs macro
  2020-07-30 15:28 ` [PATCH v3 04/10] gpio: dwapb: Add max GPIOs macro Serge Semin
@ 2020-07-30 15:42   ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-07-30 15:42 UTC (permalink / raw)
  To: Serge Semin
  Cc: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin,
	Alexey Malahov, Pavel Parkhomenko, Rob Herring, linux-gpio,
	devicetree, linux-kernel

On Thu, Jul 30, 2020 at 06:28:01PM +0300, Serge Semin wrote:
> Add a new macro DWAPB_MAX_GPIOS which defines the maximum possible number
> of GPIO lines corresponding to the maximum DW APB GPIO controller port
> width. Use the new macro instead of number literal 32 where it's
> applicable.

Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Suggested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>  drivers/gpio/gpio-dwapb.c                | 8 ++++----
>  include/linux/platform_data/gpio-dwapb.h | 4 +++-
>  2 files changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpio/gpio-dwapb.c b/drivers/gpio/gpio-dwapb.c
> index 3081213247d8..f34001152850 100644
> --- a/drivers/gpio/gpio-dwapb.c
> +++ b/drivers/gpio/gpio-dwapb.c
> @@ -162,7 +162,7 @@ static struct dwapb_gpio_port *dwapb_offs_to_port(struct dwapb_gpio *gpio, unsig
>  
>  	for (i = 0; i < gpio->nr_ports; i++) {
>  		port = &gpio->ports[i];
> -		if (port->idx == offs / 32)
> +		if (port->idx == offs / DWAPB_MAX_GPIOS)
>  			return port;
>  	}
>  
> @@ -182,7 +182,7 @@ static void dwapb_toggle_trigger(struct dwapb_gpio *gpio, unsigned int offs)
>  
>  	pol = dwapb_read(gpio, GPIO_INT_POLARITY);
>  	/* Just read the current value right out of the data register */
> -	val = gc->get(gc, offs % 32);
> +	val = gc->get(gc, offs % DWAPB_MAX_GPIOS);
>  	if (val)
>  		pol &= ~BIT(offs);
>  	else
> @@ -197,7 +197,7 @@ static u32 dwapb_do_irq(struct dwapb_gpio *gpio)
>  	irq_hw_number_t hwirq;
>  
>  	irq_status = dwapb_read(gpio, GPIO_INTSTATUS);
> -	for_each_set_bit(hwirq, &irq_status, 32) {
> +	for_each_set_bit(hwirq, &irq_status, DWAPB_MAX_GPIOS) {
>  		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
>  		u32 irq_type = irq_get_trigger_type(gpio_irq);
>  
> @@ -599,7 +599,7 @@ static struct dwapb_platform_data *dwapb_gpio_get_pdata(struct device *dev)
>  			dev_info(dev,
>  				 "failed to get number of gpios for port%d\n",
>  				 i);
> -			pp->ngpio = 32;
> +			pp->ngpio = DWAPB_MAX_GPIOS;
>  		}
>  
>  		pp->irq_shared	= false;
> diff --git a/include/linux/platform_data/gpio-dwapb.h b/include/linux/platform_data/gpio-dwapb.h
> index ff1be737bad6..0aa5c6720259 100644
> --- a/include/linux/platform_data/gpio-dwapb.h
> +++ b/include/linux/platform_data/gpio-dwapb.h
> @@ -6,12 +6,14 @@
>  #ifndef GPIO_DW_APB_H
>  #define GPIO_DW_APB_H
>  
> +#define DWAPB_MAX_GPIOS		32
> +
>  struct dwapb_port_property {
>  	struct fwnode_handle *fwnode;
>  	unsigned int	idx;
>  	unsigned int	ngpio;
>  	unsigned int	gpio_base;
> -	int		irq[32];
> +	int		irq[DWAPB_MAX_GPIOS];
>  	bool		irq_shared;
>  };
>  
> -- 
> 2.27.0
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip
  2020-07-30 15:28 ` [PATCH v3 05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip Serge Semin
@ 2020-07-30 15:45   ` Andy Shevchenko
  0 siblings, 0 replies; 14+ messages in thread
From: Andy Shevchenko @ 2020-07-30 15:45 UTC (permalink / raw)
  To: Serge Semin
  Cc: Hoan Tran, Linus Walleij, Bartosz Golaszewski, Serge Semin,
	Alexey Malahov, Pavel Parkhomenko, Rob Herring, linux-gpio,
	devicetree, linux-kernel

On Thu, Jul 30, 2020 at 06:28:02PM +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. Note this
> shalln't give any regression
> 
> 6) Alter CONFIG_GPIO_DWAPB kernel config to select
> CONFIG_GPIOLIB_IRQCHIP instead of CONFIG_GENERIC_IRQ_CHIP.
> 
> Note neither 4) nor 5) shall cause a regression of commit 6a2f4b7dadd5
> ("gpio: dwapb: use a second irq chip"), since the later isn't properly
> used here anyway.

Looks good, thanks!
Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> 
> ---
> 
> Note in this patch we omit the next GPIO-lib IRQ-chip settings
> initialization:
> gc->irq.map
> gc->irq.init_valid_mask
> 
> That's intentionally done in order to keep the driver functionality at the
> same level as before the conversion: each GPIO line will be registered as
> the source of IRQs, no matter whether DW APB GPIO IP is configured to
> generate combined or multiple IRQ signals.
> 
> If more thorough mapping is needed, the driver should be altered to detect
> the IRQ signaling mode (for instance by checking a dedicated DT-property).
> Then based on that it will be able to create a proper hardware GPIO-IRQ
> to/from parental IRQs mapping and valid hardware GPIO-IRQs mask.
> 
> Changelog v2:
> - Replace gc->to_irq() with irq_find_mapping() method.
> - Refactor dwapb_irq_set_type() method to directly set IRQ flow handler
>   instead of using a temporary variable.
> - Initialize GPIOlib IRQ-chip settings before calling request_irq()
>   method.
> - Add a notice regarding regression of commit 6a2f4b7dadd5 ("gpio:
> dwapb: use a second irq chip").
> - Move the acpi_gpiochip_{request,free}_interrupts() methods invocation
>   removal to a dedicated commit.
> - Move GPIO-chip to_irq callback removal to a dedicated commit.
> - Introduce dwapb_convert_irqs() method to convert the sparse parental
>   IRQs array into an array of linearly distributed IRQs correctly
>   perceived by GPIO-lib.
> 
> Changelog v3:
> - Add blank lines to the commit message to make it more readable.
> - Dynamically allocate memory for the IRQ-chip-related data.
> ---
>  drivers/gpio/Kconfig      |   2 +-
>  drivers/gpio/gpio-dwapb.c | 203 +++++++++++++++++++++-----------------
>  2 files changed, 111 insertions(+), 94 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 f34001152850..eb3a6da453ff 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>
> @@ -83,8 +82,15 @@ struct dwapb_context {
>  };
>  #endif
>  
> +struct dwapb_gpio_port_irqchip {
> +	struct irq_chip		irqchip;
> +	unsigned int		nr_irqs;
> +	unsigned int		irq[DWAPB_MAX_GPIOS];
> +};
> +
>  struct dwapb_gpio_port {
>  	struct gpio_chip	gc;
> +	struct dwapb_gpio_port_irqchip *pirq;
>  	bool			is_registered;
>  	struct dwapb_gpio	*gpio;
>  #ifdef CONFIG_PM_SLEEP
> @@ -92,13 +98,14 @@ 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;
>  	unsigned int		flags;
>  	struct reset_control	*rst;
>  	struct clk_bulk_data	clks[DWAPB_NR_CLOCKS];
> @@ -193,12 +200,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, DWAPB_MAX_GPIOS) {
> -		int gpio_irq = irq_find_mapping(gpio->domain, hwirq);
> +		int gpio_irq = irq_find_mapping(gc->irq.domain, hwirq);
>  		u32 irq_type = irq_get_trigger_type(gpio_irq);
>  
>  		generic_handle_irq(gpio_irq);
> @@ -225,11 +233,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 +287,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,9 +301,8 @@ 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;
>  
> @@ -293,7 +336,10 @@ static int dwapb_irq_set_type(struct irq_data *d, u32 type)
>  		break;
>  	}
>  
> -	irq_setup_alt_chip(d, type);
> +	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);
>  
>  	dwapb_write(gpio, GPIO_INTTYPE_LEVEL, level);
>  	if (type != IRQ_TYPE_EDGE_BOTH)
> @@ -354,79 +400,67 @@ static int dwapb_gpio_set_config(struct gpio_chip *gc, unsigned offset,
>  	return dwapb_gpio_set_debounce(gc, offset, debounce);
>  }
>  
> +static int dwapb_convert_irqs(struct dwapb_gpio_port_irqchip *pirq,
> +			      struct dwapb_port_property *pp)
> +{
> +	int i;
> +
> +	/* Group all available IRQs into an array of parental IRQs. */
> +	for (i = 0; i < pp->ngpio; ++i) {
> +		if (!pp->irq[i])
> +			continue;
> +
> +		pirq->irq[pirq->nr_irqs++] = pp->irq[i];
> +	}
> +
> +	return pirq->nr_irqs ? 0 : -ENOENT;
> +}
> +
>  static void dwapb_configure_irqs(struct dwapb_gpio *gpio,
>  				 struct dwapb_gpio_port *port,
>  				 struct dwapb_port_property *pp)
>  {
> +	struct dwapb_gpio_port_irqchip *pirq;
>  	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;
> -
> -	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;
> +	struct gpio_irq_chip *girq;
> +	int err;
>  
> -	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;
> +	pirq = devm_kzalloc(gpio->dev, sizeof(*pirq), GFP_KERNEL);
> +	if (!pirq)
>  		return;
> -	}
>  
> -	irq_gc = irq_get_domain_generic_chip(gpio->domain, 0);
> -	if (!irq_gc) {
> -		irq_domain_remove(gpio->domain);
> -		gpio->domain = NULL;
> -		return;
> +	if (dwapb_convert_irqs(pirq, pp)) {
> +		dev_warn(gpio->dev, "no IRQ for port%d\n", pp->idx);
> +		goto err_kfree_pirq;
>  	}
>  
> -	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;
> +
> +	port->pirq = pirq;
> +	pirq->irqchip.name = DWAPB_DRIVER_NAME;
> +	pirq->irqchip.irq_ack = dwapb_irq_ack;
> +	pirq->irqchip.irq_mask = dwapb_irq_mask;
> +	pirq->irqchip.irq_unmask = dwapb_irq_unmask;
> +	pirq->irqchip.irq_set_type = dwapb_irq_set_type;
> +	pirq->irqchip.irq_enable = dwapb_irq_enable;
> +	pirq->irqchip.irq_disable = dwapb_irq_disable;
>  #ifdef CONFIG_PM_SLEEP
> -		ct->chip.irq_set_wake = dwapb_irq_set_wake;
> +	pirq->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);
> -		}
> +		girq->num_parents = pirq->nr_irqs;
> +		girq->parents = pirq->irq;
> +		girq->parent_handler_data = gpio;
> +		girq->parent_handler = dwapb_irq_handler;
>  	} else {
> +		/* This will let us handle the parent IRQ in the driver */
> +		girq->num_parents = 0;
> +		girq->parents = NULL;
> +		girq->parent_handler = NULL;
> +
>  		/*
>  		 * Request a shared IRQ since where MFD would have devices
>  		 * using the same irq pin
> @@ -436,33 +470,18 @@ 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;
> +			goto err_kfree_pirq;
>  		}
>  	}
>  
> -	for (hwirq = 0; hwirq < ngpio; hwirq++)
> -		irq_create_mapping(gpio->domain, hwirq);
> +	girq->chip = &pirq->irqchip;
>  
>  	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));
> +	return;
>  
> -	irq_domain_remove(gpio->domain);
> -	gpio->domain = NULL;
> +err_kfree_pirq:
> +	devm_kfree(gpio->dev, pirq);
>  }
>  
>  static int dwapb_gpio_add_port(struct dwapb_gpio *gpio,
> @@ -699,7 +718,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 +728,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.27.0
> 

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization
  2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
                   ` (9 preceding siblings ...)
  2020-07-30 15:28 ` [PATCH v3 10/10] gpio: dwapb: Use resource managed GPIO-chip add data method Serge Semin
@ 2020-08-27  8:34 ` Linus Walleij
  10 siblings, 0 replies; 14+ messages in thread
From: Linus Walleij @ 2020-08-27  8:34 UTC (permalink / raw)
  To: Serge Semin
  Cc: Hoan Tran, Bartosz Golaszewski, Serge Semin, Andy Shevchenko,
	Andy Shevchenko, Alexey Malahov, Pavel Parkhomenko, Rob Herring,
	open list:GPIO SUBSYSTEM,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-kernel

On Thu, Jul 30, 2020 at 5:28 PM Serge Semin
<Sergey.Semin@baikalelectronics.ru> 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.
>
> Some additional cleanups like replacing a number of GPIOs literal with a
> corresponding macro and grouping the IRQ handlers up in a single place of
> the driver are also introduced in this patchset.

Sorry for the delay. Merge window and stress.

All 10 patches applied for v5.10!

Yours,
Linus Walleij

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 15:27 [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Serge Semin
2020-07-30 15:27 ` [PATCH v3 01/10] dt-bindings: gpio: dwapb: Add ngpios property support Serge Semin
2020-07-30 15:27 ` [PATCH v3 02/10] gpio: dwapb: Add ngpios DT-property support Serge Semin
2020-07-30 15:28 ` [PATCH v3 03/10] gpio: dwapb: Move MFD-specific IRQ handler Serge Semin
2020-07-30 15:28 ` [PATCH v3 04/10] gpio: dwapb: Add max GPIOs macro Serge Semin
2020-07-30 15:42   ` Andy Shevchenko
2020-07-30 15:28 ` [PATCH v3 05/10] gpio: dwapb: Convert driver to using the GPIO-lib-based IRQ-chip Serge Semin
2020-07-30 15:45   ` Andy Shevchenko
2020-07-30 15:28 ` [PATCH v3 06/10] gpio: dwapb: Discard GPIO-to-IRQ mapping function Serge Semin
2020-07-30 15:28 ` [PATCH v3 07/10] gpio: dwapb: Discard ACPI GPIO-chip IRQs request Serge Semin
2020-07-30 15:28 ` [PATCH v3 08/10] gpio: dwapb: Get reset control by means of resource managed interface Serge Semin
2020-07-30 15:28 ` [PATCH v3 09/10] gpio: dwapb: Get clocks " Serge Semin
2020-07-30 15:28 ` [PATCH v3 10/10] gpio: dwapb: Use resource managed GPIO-chip add data method Serge Semin
2020-08-27  8:34 ` [PATCH v3 00/10] gpio: dwapb: Refactor GPIO resources initialization Linus Walleij

Linux-GPIO Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-gpio/0 linux-gpio/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-gpio linux-gpio/ https://lore.kernel.org/linux-gpio \
		linux-gpio@vger.kernel.org
	public-inbox-index linux-gpio

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-gpio


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git