Migrate PCIe-IDIO-24 GPIO driver to the regmap API

The regmap API supports IO port accessors so we can take advantage of regmap abstractions rather than handling access to the device registers directly in the driver.

A patch to pass the device regmap and irq_drv_data as a parameters for the struct regmap_irq_chip set_type_config() is included. This is needed by idio_24_set_type_config() in order to update the type configuration on the device as well as irq_drv_data for idio_24_handle_mask_sync().

A patch moving the struct gpio_regmap declaration to linux/gpio/regmap.h is also included. This is needed by idio_24_reg_mask_xlate() in order to determine the current offset's direction by using gpio->regmap in regmap_read(). One point to consider is whether an alternative solution is better of passing regmap in the reg_mask_xlate() parameter list; this would avoid the need to include <linux/gpio/driver.h> in order to resolve an incomplete type warning for struct gpio_chip due to the move.

William Breathitt Gray (3):
regmap: Pass regmap and irq_drv_data as parameters for
set_type_config()
gpio: gpio-regmap: Expose struct gpio_regmap in linux/gpio/regmap.h
gpio: pcie-idio-24: Migrate to the regmap API

 drivers/base/regmap/regmap-irq.c |  13 +-
 drivers/gpio/Kconfig             |   3 +
 drivers/gpio/gpio-pcie-idio-24.c | 697 ++++++++++++-------------------
 drivers/gpio/gpio-regmap.c       |  20 -
 include/linux/gpio/regmap.h      |  23 +-
 include/linux/regmap.h           |  12 +-
 6 files changed, 303 insertions(+), 465 deletions(-)

base-commit: 4827aae061337251bb91801b316157a78b845ec7

2.39.2

Allow struct regmap_irq_chip set_type_config() callbacks to access the device regmap and irq_drv_data by passing them as parameters.

---
 drivers/base/regmap/regmap-irq.c | 13 +++++++++----
 include/linux/regmap.h           | 12 ++++++++----
 2 files changed, 17 insertions(+), 8 deletions(-)

index a8f185430a07..eac55a3af6d9 100644
@@ -333,8 +333,9 @@ static int regmap_irq_set_type(struct irq_data *data, unsigned int type)
 	}
 
 	if (d->chip->set_type_config) {
-		ret = d->chip->set_type_config(d->config_buf, type,
-					       irq_data, reg);
+		ret = d->chip->set_type_config(map, d->config_buf, type,
+					       irq_data, reg,
+					       d->chip->irq_drv_data);
 		if (ret)
 			return ret;
 	}

@@ -650,18 +651,22 @@ EXPORT_SYMBOL_GPL(regmap_irq_get_irq_reg_linear);
 
 /**
  * regmap_irq_set_type_config_simple() - Simple IRQ type configuration callback.
+ * @map: The regmap for the device.
  * @buf: Buffer containing configuration register values, this is a 2D array of
  *       `num_config_bases` rows, each of `num_config_regs` elements.
  * @type: The requested IRQ type.
  * @irq_data: The IRQ being configured.
  * @idx: Index of the irq's config registers within each array `buf[i]`
+ * @irq_drv_data: Driver specific IRQ data
  *
  * This is a &struct regmap_irq_chip->set_type_config callback suitable for
  * chips with one config register. Register values are updated according to
  * the &struct regmap_irq_type data associated with an IRQ.
  */
-int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
-				      const struct regmap_irq *irq_data, int idx)
+int regmap_irq_set_type_config_simple(struct regmap *map, unsigned int **buf,
+				      unsigned int type,
+				      const struct regmap_irq *irq_data,
+				      int idx, void *irq_drv_data)
 {
 	const struct regmap_irq_type *t = &irq_data->type;
 

index a3bc695bcca0..49073f5ae87a 100644
@@ -1648,8 +1648,10 @@ struct regmap_irq_chip {
 				unsigned int mask_buf, void *irq_drv_data);
 	int (*set_type_virt)(unsigned int **buf, unsigned int type,
 			     unsigned long hwirq, int reg);
-	int (*set_type_config)(unsigned int **buf, unsigned int type,
-			       const struct regmap_irq *irq_data, int idx);
+	int (*set_type_config)(struct regmap *map, unsigned int **buf,
+			       unsigned int type,
+			       const struct regmap_irq *irq_data, int idx,
+			       void *irq_drv_data);
 	unsigned int (*get_irq_reg)(struct regmap_irq_chip_data *data,
 				    unsigned int base, int index);
 	void *irq_drv_data;

@@ -1657,8 +1659,10 @@ struct regmap_irq_chip {
 
 unsigned int regmap_irq_get_irq_reg_linear(struct regmap_irq_chip_data *data,
 					   unsigned int base, int index);
-int regmap_irq_set_type_config_simple(unsigned int **buf, unsigned int type,
-				      const struct regmap_irq *irq_data, int idx);
+int regmap_irq_set_type_config_simple(struct regmap *map, unsigned int **buf,
+				      unsigned int type,
+				      const struct regmap_irq *irq_data,
+				      int idx, void *irq_drv_data);
 
 int regmap_add_irq_chip(struct regmap *map, int irq, int irq_flags,
 			int irq_base, const struct regmap_irq_chip *chip,

2.39.2

A struct gpio_regmap is passed as a parameter for reg_mask_xlate(), but for callbacks to access its members the declaration must be exposed. Move the struct gpio_regmap declaration from drivers/gpio/gpio-regmap.c to include/linux/gpio/regmap.h so callbacks can properly interact with struct gpio_regmap members.

---
 drivers/gpio/gpio-regmap.c  | 20 --------------------
 include/linux/gpio/regmap.h | 23 ++++++++++++++++++++++-
 2 files changed, 22 insertions(+), 21 deletions(-)

index fca17d478984..ad34750779c7 100644
@@ -11,26 +11,6 @@
 #include <linux/module.h>
 #include <linux/regmap.h>
 
-struct gpio_regmap {
-	struct device *parent;
-	struct regmap *regmap;
-	struct gpio_chip gpio_chip;
-
-	int reg_stride;
-	int ngpio_per_reg;
-	unsigned int reg_dat_base;
-	unsigned int reg_set_base;
-	unsigned int reg_clr_base;
-	unsigned int reg_dir_in_base;
-	unsigned int reg_dir_out_base;
-
-	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
-			      unsigned int offset, unsigned int *reg,
-			      unsigned int *mask);
-
-	void *driver_data;
-};
-
 static unsigned int gpio_regmap_addr(unsigned int addr)
 {
 	if (addr == GPIO_REGMAP_ADDR_ZERO)

index a9f7b7faf57b..1132c0f7e907 100644
@@ -3,15 +3,36 @@
 #ifndef _LINUX_GPIO_REGMAP_H
 #define _LINUX_GPIO_REGMAP_H
 
+#include <linux/gpio/driver.h>
+
 struct device;
 struct fwnode_handle;
-struct gpio_regmap;
 struct irq_domain;
 struct regmap;
 
 #define GPIO_REGMAP_ADDR_ZERO ((unsigned int)(-1))
 #define GPIO_REGMAP_ADDR(addr) ((addr) ? : GPIO_REGMAP_ADDR_ZERO)
 
+struct gpio_regmap {
+	struct device *parent;
+	struct regmap *regmap;
+	struct gpio_chip gpio_chip;
+
+	int reg_stride;
+	int ngpio_per_reg;
+	unsigned int reg_dat_base;
+	unsigned int reg_set_base;
+	unsigned int reg_clr_base;
+	unsigned int reg_dir_in_base;
+	unsigned int reg_dir_out_base;
+
+	int (*reg_mask_xlate)(struct gpio_regmap *gpio, unsigned int base,
+			      unsigned int offset, unsigned int *reg,
+			      unsigned int *mask);
+
+	void *driver_data;
+};
+
 /**
  * struct gpio_regmap_config - Description of a generic regmap gpio_chip.
  * @parent:		The parent device

2.39.2

Hi,

Am 2023-02-28 02:53, schrieb William Breathitt Gray:

That parameter is only an opaque one to call any gpio_regmap_*().

That struct should be kept private. It seems you only need the regmap. Either introduce a gpio_regmap_get_regmap() or add the regmap to a private struct and use gpio_regmap_get_drvdata().

-michael

Ah, I'll drop this patch then and use gpio_regmap_get_drvdata() in v2 instead to get access to the regmap.

William Breathitt Gray

The regmap API supports IO port accessors so we can take advantage of regmap abstractions rather than handling access to the device registers directly in the driver.

For the PCIe-IDIO-24 series of devices, the following BARs are available:

    BAR[0]: memory mapped PEX8311
    BAR[1]: I/O mapped PEX8311
    BAR[2]: I/O mapped card registers

There are 24 FET Output lines, 24 Isolated Input lines, and 8 TTL/CMOS lines (which may be configured for either output or input). The GPIO lines are exposed by the following card registers:

    Base +0x0-0x2 (Read/Write): FET Outputs
    Base +0xB (Read/Write): TTL/CMOS
    Base +0x4-0x6 (Read): Isolated Inputs
    Base +0x7 (Read): TTL/CMOS

In order for the device to support interrupts, the PLX PEX8311 internal PCI wire interrupt and local interrupt input must first be enabled.

The following card registers for Change-Of-State may be used:

    Base +0x8-0xA (Read): COS Status Inputs
    Base +0x8-0xA (Write): COS Clear Inputs
    Base +0xB (Read): COS Status TTL/CMOS
    Base +0xB (Write): COS Clear TTL/CMOS
    Base +0xE (Read/Write): COS Enable

The COS Enable register is used to enable/disable interrupts and configure the interrupt levels; each bit maps to a group of eight inputs as described below:

Bit 0: IRQ EN Rising Edge IN0-7
Bit 1: IRQ EN Rising Edge IN8-15
Bit 2: IRQ EN Rising Edge IN16-23
Bit 3: IRQ EN Rising Edge TTL0-7
Bit 4: IRQ EN Falling Edge IN0-7
Bit 5: IRQ EN Falling Edge IN8-15
Bit 6: IRQ EN Falling Edge IN16-23
Bit 7: IRQ EN Falling Edge TTL0-7

An interrupt is asserted when a change-of-state matching the interrupt level configuration respective for a particular group of eight inputs with enabled COS is detected.

The COS Status registers may be read to determine which inputs have changed; if interrupts were enabled, an IRQ will be generated for the set bits in these registers. Writing the value read from the COS Status register back to the respective COS Clear register will clear just those interrupts.

---
 drivers/gpio/Kconfig             |   3 +
 drivers/gpio/gpio-pcie-idio-24.c | 697 ++++++++++++-------------------
 2 files changed, 264 insertions(+), 436 deletions(-)

index 406e8bda487f..06c7a96e6033 100644
@@ -1578,7 +1578,10 @@ config GPIO_PCI_IDIO_16
 
 config GPIO_PCIE_IDIO_24
 	tristate "ACCES PCIe-IDIO-24 GPIO support"
+	select REGMAP_IRQ
+	select REGMAP_MMIO
 	select GPIOLIB_IRQCHIP
+	select GPIO_REGMAP
 	help
 	  Enables GPIO support for the ACCES PCIe-IDIO-24 family (PCIe-IDIO-24,
 	  PCIe-IDI-24, PCIe-IDO-24, PCIe-IDIO-12). An interrupt is generated

2.39.2

The values from the config buffer are supposed to be written out in regmap_irq_sync_unlock() - why is something custom needed here?

The PCIe-IDIO-24 "COS Enable" serves a dual purpose of interrupt enabling/disabling as well as configuring the interrupt types. Since this register is used for masking, config buffer would clobber the register if we use it in this particular case. Instead, we ignore the config buffer and configure the type directly for the device (handling the case where interrupts are masked and shouldn't be enabled).

William Breathitt Gray

Could you be more concrete about what's going on here please? In what way does this "COS Enable" serve these dual functions and why do they clobber each other?

An explanation of the device registers is provided in [PATCH 3/3]; here's the relevant portion:

The COS Enable register is used to enable/disable interrupts and configure the interrupt levels; each bit maps to a group of eight inputs as described below:

Bit 0: IRQ EN Rising Edge IN0-7
Bit 1: IRQ EN Rising Edge IN8-15
Bit 2: IRQ EN Rising Edge IN16-23
Bit 3: IRQ EN Rising Edge TTL0-7
Bit 4: IRQ EN Falling Edge IN0-7
Bit 5: IRQ EN Falling Edge IN8-15
Bit 6: IRQ EN Falling Edge IN16-23
Bit 7: IRQ EN Falling Edge TTL0-7

An interrupt is asserted when a change-of-state matching the interrupt level configuration respective for a particular group of eight inputs with enabled COS is detected.

So in order to mask lines, the respective bits need to be set to 0. However, if we use the regmap-irq config buffer to set the type, this mask will be cloberred and the disabled lines become enabled. To prevent the clobber, we can save the type configuration to irq_drv_data for use later in handle_mask_sync() and then update the type in this COS Enable register only when the lines are unmasked.

William Breathitt Gray