[PATCH 0/5] gpio: Implement and utilize register structures for ISA drivers

[PATCH 0/5] gpio: Implement and utilize register structures for ISA drivers

[William Breathitt Gray]

The PC104/ISA drivers were updated to use I/O memory accessor calls such
as ioread8()/iowrite8() in a previous patch series [1]. This
patchset is a continuation of the effort to improve the code readability
and reduce magic numbers by implementing and utilizing named register
data structures.

One of the benefits is that we can now observe more easily similarities
in devices that share similar interfaces; such as the i8255 interfaces
used by the 104-DIO-48E, 104-IDI-48, and GPIO-MM drivers -- as well as
the similar interface used by the 104-IDIO-16 and PCI-IDIO-16 drivers. I
hope to consolidate some of these code blocks in future patchsets.

[1] https://lore.kernel.org/all/cover.1652201921.git.william.gray@linaro.org/

William Breathitt Gray (5):
  gpio: 104-dio-48e: Implement and utilize register structures
  gpio: 104-idi-48: Implement and utilize register structures
  gpio: 104-idio-16: Implement and utilize register structures
  gpio: gpio-mm: Implement and utilize register structures
  gpio: ws16c48: Implement and utilize register structures

 drivers/gpio/gpio-104-dio-48e.c | 157 +++++++++++++++++++++-----------
 drivers/gpio/gpio-104-idi-48.c  | 128 +++++++++++++-------------
 drivers/gpio/gpio-104-idio-16.c |  58 ++++++++----
 drivers/gpio/gpio-gpio-mm.c     | 116 ++++++++++++++---------
 drivers/gpio/gpio-ws16c48.c     | 119 +++++++++++++++++-------
 5 files changed, 366 insertions(+), 212 deletions(-)


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1

[PATCH 1/5] gpio: 104-dio-48e: Implement and utilize register structures

[William Breathitt Gray]

Reduce magic numbers and improve code readability by implementing and
utilizing named register data structures.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/gpio-104-dio-48e.c | 157 +++++++++++++++++++++-----------
 1 file changed, 106 insertions(+), 51 deletions(-)

diff --git a/drivers/gpio/gpio-104-dio-48e.c b/drivers/gpio/gpio-104-dio-48e.c
index f118ad9bcd33..e1c5759e0902 100644
--- a/drivers/gpio/gpio-104-dio-48e.c
+++ b/drivers/gpio/gpio-104-dio-48e.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 
 #define DIO48E_EXTENT 16
 #define MAX_NUM_DIO48E max_num_isa_dev(DIO48E_EXTENT)
@@ -33,6 +34,40 @@ static unsigned int irq[MAX_NUM_DIO48E];
 module_param_hw_array(irq, uint, irq, NULL, 0);
 MODULE_PARM_DESC(irq, "ACCES 104-DIO-48E interrupt line numbers");
 
+/**
+ * struct i8255_reg - Intel 8255 register structure
+ * @port:	Port A, B, and C
+ * @control:	Control register
+ */
+struct i8255_reg {
+	u8 port[3];
+	u8 control;
+};
+
+/**
+ * struct dio48e_reg - device register structure
+ * @ppi:		Programmable Peripheral Interface groups
+ * @enable_buffer:	Enable/Disable Buffer groups
+ * @unused1:		Unused
+ * @enable_interrupt:	Write: Enable Interrupt
+ *			Read: Disable Interrupt
+ * @unused2:		Unused
+ * @enable_counter:	Write: Enable Counter/Timer Addressing
+ *			Read: Disable Counter/Timer Addressing
+ * @unused3:		Unused
+ * @clear_interrupt:	Clear Interrupt
+ */
+struct dio48e_reg {
+	struct i8255_reg ppi[2];
+	u8 enable_buffer[2];
+	u8 unused1;
+	u8 enable_interrupt;
+	u8 unused2;
+	u8 enable_counter;
+	u8 unused3;
+	u8 clear_interrupt;
+};
+
 /**
  * struct dio48e_gpio - GPIO device private data structure
  * @chip:	instance of the gpio_chip
@@ -40,7 +75,7 @@ MODULE_PARM_DESC(irq, "ACCES 104-DIO-48E interrupt line numbers");
  * @out_state:	output bits state
  * @control:	Control registers state
  * @lock:	synchronization lock to prevent I/O race conditions
- * @base:	base port address of the GPIO device
+ * @reg:	I/O address offset for the device registers
  * @irq_mask:	I/O bits affected by interrupts
  */
 struct dio48e_gpio {
@@ -49,7 +84,7 @@ struct dio48e_gpio {
 	unsigned char out_state[6];
 	unsigned char control[2];
 	raw_spinlock_t lock;
-	void __iomem *base;
+	struct dio48e_reg __iomem *reg;
 	unsigned char irq_mask;
 };
 
@@ -69,32 +104,33 @@ static int dio48e_gpio_direction_input(struct gpio_chip *chip, unsigned int offs
 {
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
 	const unsigned int io_port = offset / 8;
-	const unsigned int control_port = io_port / 3;
-	void __iomem *const control_addr = dio48egpio->base + 3 + control_port * 4;
+	const unsigned long group = io_port / 3;
+	const unsigned long port = io_port - (group * 3);
+	u8 __iomem *const control_addr = &dio48egpio->reg->ppi[group].control;
 	unsigned long flags;
 	unsigned int control;
 
 	raw_spin_lock_irqsave(&dio48egpio->lock, flags);
 
 	/* Check if configuring Port C */
-	if (io_port == 2 || io_port == 5) {
+	if (port == 2) {
 		/* Port C can be configured by nibble */
 		if (offset % 8 > 3) {
 			dio48egpio->io_state[io_port] |= 0xF0;
-			dio48egpio->control[control_port] |= BIT(3);
+			dio48egpio->control[group] |= BIT(3);
 		} else {
 			dio48egpio->io_state[io_port] |= 0x0F;
-			dio48egpio->control[control_port] |= BIT(0);
+			dio48egpio->control[group] |= BIT(0);
 		}
 	} else {
 		dio48egpio->io_state[io_port] |= 0xFF;
-		if (io_port == 0 || io_port == 3)
-			dio48egpio->control[control_port] |= BIT(4);
+		if (port == 0)
+			dio48egpio->control[group] |= BIT(4);
 		else
-			dio48egpio->control[control_port] |= BIT(1);
+			dio48egpio->control[group] |= BIT(1);
 	}
 
-	control = BIT(7) | dio48egpio->control[control_port];
+	control = BIT(7) | dio48egpio->control[group];
 	iowrite8(control, control_addr);
 	control &= ~BIT(7);
 	iowrite8(control, control_addr);
@@ -109,31 +145,33 @@ static int dio48e_gpio_direction_output(struct gpio_chip *chip, unsigned int off
 {
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
 	const unsigned int io_port = offset / 8;
-	const unsigned int control_port = io_port / 3;
+	const unsigned long group = io_port / 3;
+	const unsigned long port = io_port - (group * 3);
 	const unsigned int mask = BIT(offset % 8);
-	void __iomem *const control_addr = dio48egpio->base + 3 + control_port * 4;
-	const unsigned int out_port = (io_port > 2) ? io_port + 1 : io_port;
+	struct i8255_reg __iomem *const ppi = dio48egpio->reg->ppi + group;
+	u8 __iomem *const control_addr = &ppi->control;
+	u8 __iomem *const port_addr = ppi->port + port;
 	unsigned long flags;
 	unsigned int control;
 
 	raw_spin_lock_irqsave(&dio48egpio->lock, flags);
 
 	/* Check if configuring Port C */
-	if (io_port == 2 || io_port == 5) {
+	if (port == 2) {
 		/* Port C can be configured by nibble */
 		if (offset % 8 > 3) {
 			dio48egpio->io_state[io_port] &= 0x0F;
-			dio48egpio->control[control_port] &= ~BIT(3);
+			dio48egpio->control[group] &= ~BIT(3);
 		} else {
 			dio48egpio->io_state[io_port] &= 0xF0;
-			dio48egpio->control[control_port] &= ~BIT(0);
+			dio48egpio->control[group] &= ~BIT(0);
 		}
 	} else {
 		dio48egpio->io_state[io_port] &= 0x00;
-		if (io_port == 0 || io_port == 3)
-			dio48egpio->control[control_port] &= ~BIT(4);
+		if (port == 0)
+			dio48egpio->control[group] &= ~BIT(4);
 		else
-			dio48egpio->control[control_port] &= ~BIT(1);
+			dio48egpio->control[group] &= ~BIT(1);
 	}
 
 	if (value)
@@ -141,10 +179,10 @@ static int dio48e_gpio_direction_output(struct gpio_chip *chip, unsigned int off
 	else
 		dio48egpio->out_state[io_port] &= ~mask;
 
-	control = BIT(7) | dio48egpio->control[control_port];
+	control = BIT(7) | dio48egpio->control[group];
 	iowrite8(control, control_addr);
 
-	iowrite8(dio48egpio->out_state[io_port], dio48egpio->base + out_port);
+	iowrite8(dio48egpio->out_state[io_port], port_addr);
 
 	control &= ~BIT(7);
 	iowrite8(control, control_addr);
@@ -159,7 +197,8 @@ static int dio48e_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
 	const unsigned int port = offset / 8;
 	const unsigned int mask = BIT(offset % 8);
-	const unsigned int in_port = (port > 2) ? port + 1 : port;
+	const unsigned long group = port / 3;
+	const unsigned long in_port = port - (group * 3);
 	unsigned long flags;
 	unsigned int port_state;
 
@@ -171,29 +210,33 @@ static int dio48e_gpio_get(struct gpio_chip *chip, unsigned int offset)
 		return -EINVAL;
 	}
 
-	port_state = ioread8(dio48egpio->base + in_port);
+	port_state = ioread8(dio48egpio->reg->ppi[group].port + in_port);
 
 	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 
 	return !!(port_state & mask);
 }
 
-static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-
 static int dio48e_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
 	unsigned long offset;
 	unsigned long gpio_mask;
-	void __iomem *port_addr;
+	unsigned long port;
+	unsigned long group;
+	unsigned long in_port;
+	u8 __iomem *port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
-		port_addr = dio48egpio->base + ports[offset / 8];
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		port = offset / 8;
+		group = port / 3;
+		in_port = port - (group * 3);
+		port_addr = dio48egpio->reg->ppi[group].port + in_port;
 		port_state = ioread8(port_addr) & gpio_mask;
 
 		bitmap_set_value8(bits, port_state, offset);
@@ -207,7 +250,8 @@ static void dio48e_gpio_set(struct gpio_chip *chip, unsigned int offset, int val
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
 	const unsigned int port = offset / 8;
 	const unsigned int mask = BIT(offset % 8);
-	const unsigned int out_port = (port > 2) ? port + 1 : port;
+	const unsigned long group = port / 3;
+	const unsigned long out_port = port - (group * 3);
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&dio48egpio->lock, flags);
@@ -217,7 +261,8 @@ static void dio48e_gpio_set(struct gpio_chip *chip, unsigned int offset, int val
 	else
 		dio48egpio->out_state[port] &= ~mask;
 
-	iowrite8(dio48egpio->out_state[port], dio48egpio->base + out_port);
+	iowrite8(dio48egpio->out_state[port],
+		 dio48egpio->reg->ppi[group].port + out_port);
 
 	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 }
@@ -229,13 +274,17 @@ static void dio48e_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long offset;
 	unsigned long gpio_mask;
 	size_t index;
-	void __iomem *port_addr;
+	size_t group;
+	size_t out_port;
+	u8 __iomem *port_addr;
 	unsigned long bitmask;
 	unsigned long flags;
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
 		index = offset / 8;
-		port_addr = dio48egpio->base + ports[index];
+		group = index / 3;
+		out_port = index - (group * 3);
+		port_addr = dio48egpio->reg->ppi[group].port + out_port;
 
 		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
@@ -274,7 +323,7 @@ static void dio48e_irq_mask(struct irq_data *data)
 
 	if (!dio48egpio->irq_mask)
 		/* disable interrupts */
-		ioread8(dio48egpio->base + 0xB);
+		ioread8(&dio48egpio->reg->enable_interrupt);
 
 	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 }
@@ -294,8 +343,8 @@ static void dio48e_irq_unmask(struct irq_data *data)
 
 	if (!dio48egpio->irq_mask) {
 		/* enable interrupts */
-		iowrite8(0x00, dio48egpio->base + 0xF);
-		iowrite8(0x00, dio48egpio->base + 0xB);
+		iowrite8(0x00, &dio48egpio->reg->clear_interrupt);
+		iowrite8(0x00, &dio48egpio->reg->enable_interrupt);
 	}
 
 	if (offset == 19)
@@ -341,7 +390,7 @@ static irqreturn_t dio48e_irq_handler(int irq, void *dev_id)
 
 	raw_spin_lock(&dio48egpio->lock);
 
-	iowrite8(0x00, dio48egpio->base + 0xF);
+	iowrite8(0x00, &dio48egpio->reg->clear_interrupt);
 
 	raw_spin_unlock(&dio48egpio->lock);
 
@@ -373,11 +422,25 @@ static int dio48e_irq_init_hw(struct gpio_chip *gc)
 	struct dio48e_gpio *const dio48egpio = gpiochip_get_data(gc);
 
 	/* Disable IRQ by default */
-	ioread8(dio48egpio->base + 0xB);
+	ioread8(&dio48egpio->reg->enable_interrupt);
 
 	return 0;
 }
 
+static void dio48e_init_ppi(struct i8255_reg __iomem *const ppi)
+{
+	/* Activate Mode Set; set Mode 0 output mode for Port A, B, and C */
+	iowrite8(0x80, &ppi->control);
+
+	/* Initialize all GPIO to 0 */
+	iowrite8(0x00, &ppi->port[0]);
+	iowrite8(0x00, &ppi->port[1]);
+	iowrite8(0x00, &ppi->port[2]);
+
+	/* Deactivate Mode Set */
+	iowrite8(0x00, &ppi->control);
+}
+
 static int dio48e_probe(struct device *dev, unsigned int id)
 {
 	struct dio48e_gpio *dio48egpio;
@@ -395,8 +458,8 @@ static int dio48e_probe(struct device *dev, unsigned int id)
 		return -EBUSY;
 	}
 
-	dio48egpio->base = devm_ioport_map(dev, base[id], DIO48E_EXTENT);
-	if (!dio48egpio->base)
+	dio48egpio->reg = devm_ioport_map(dev, base[id], DIO48E_EXTENT);
+	if (!dio48egpio->reg)
 		return -ENOMEM;
 
 	dio48egpio->chip.label = name;
@@ -426,16 +489,8 @@ static int dio48e_probe(struct device *dev, unsigned int id)
 	raw_spin_lock_init(&dio48egpio->lock);
 
 	/* initialize all GPIO as output */
-	iowrite8(0x80, dio48egpio->base + 3);
-	iowrite8(0x00, dio48egpio->base);
-	iowrite8(0x00, dio48egpio->base + 1);
-	iowrite8(0x00, dio48egpio->base + 2);
-	iowrite8(0x00, dio48egpio->base + 3);
-	iowrite8(0x80, dio48egpio->base + 7);
-	iowrite8(0x00, dio48egpio->base + 4);
-	iowrite8(0x00, dio48egpio->base + 5);
-	iowrite8(0x00, dio48egpio->base + 6);
-	iowrite8(0x00, dio48egpio->base + 7);
+	dio48e_init_ppi(&dio48egpio->reg->ppi[0]);
+	dio48e_init_ppi(&dio48egpio->reg->ppi[1]);
 
 	err = devm_gpiochip_add_data(dev, &dio48egpio->chip, dio48egpio);
 	if (err) {
-- 
2.36.1

[PATCH 2/5] gpio: 104-idi-48: Implement and utilize register structures

[William Breathitt Gray]

Reduce magic numbers and improve code readability by implementing and
utilizing named register data structures.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/gpio-104-idi-48.c | 128 ++++++++++++++++-----------------
 1 file changed, 63 insertions(+), 65 deletions(-)

diff --git a/drivers/gpio/gpio-104-idi-48.c b/drivers/gpio/gpio-104-idi-48.c
index 9521ece3ebef..1f8c556d9013 100644
--- a/drivers/gpio/gpio-104-idi-48.c
+++ b/drivers/gpio/gpio-104-idi-48.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 
 #define IDI_48_EXTENT 8
 #define MAX_NUM_IDI_48 max_num_isa_dev(IDI_48_EXTENT)
@@ -33,13 +34,28 @@ static unsigned int irq[MAX_NUM_IDI_48];
 module_param_hw_array(irq, uint, irq, NULL, 0);
 MODULE_PARM_DESC(irq, "ACCES 104-IDI-48 interrupt line numbers");
 
+/**
+ * struct idi_48_reg - device register structure
+ * @port0:	Port 0 Inputs
+ * @unused:	Unused
+ * @port1:	Port 1 Inputs
+ * @irq:	Read: IRQ Status Register/IRQ Clear
+ *		Write: IRQ Enable/Disable
+ */
+struct idi_48_reg {
+	u8 port0[3];
+	u8 unused;
+	u8 port1[3];
+	u8 irq;
+};
+
 /**
  * struct idi_48_gpio - GPIO device private data structure
  * @chip:	instance of the gpio_chip
  * @lock:	synchronization lock to prevent I/O race conditions
  * @ack_lock:	synchronization lock to prevent IRQ handler race conditions
  * @irq_mask:	input bits affected by interrupts
- * @base:	base port address of the GPIO device
+ * @reg:	I/O address offset for the device registers
  * @cos_enb:	Change-Of-State IRQ enable boundaries mask
  */
 struct idi_48_gpio {
@@ -47,7 +63,7 @@ struct idi_48_gpio {
 	raw_spinlock_t lock;
 	spinlock_t ack_lock;
 	unsigned char irq_mask[6];
-	void __iomem *base;
+	struct idi_48_reg __iomem *reg;
 	unsigned char cos_enb;
 };
 
@@ -64,39 +80,38 @@ static int idi_48_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 static int idi_48_gpio_get(struct gpio_chip *chip, unsigned offset)
 {
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
-	unsigned i;
-	static const unsigned int register_offset[6] = { 0, 1, 2, 4, 5, 6 };
-	void __iomem *port_addr;
-	unsigned mask;
-
-	for (i = 0; i < 48; i += 8)
-		if (offset < i + 8) {
-			port_addr = idi48gpio->base + register_offset[i / 8];
-			mask = BIT(offset - i);
-
-			return !!(ioread8(port_addr) & mask);
-		}
-
-	/* The following line should never execute since offset < 48 */
-	return 0;
+	struct idi_48_reg __iomem *const reg = idi48gpio->reg;
+	const unsigned long boundary = offset / 8;
+	const unsigned long group = boundary / 3;
+	u8 __iomem *const port_addr = group ? reg->port1 : reg->port0;
+	const unsigned long in_port = boundary - (group * 3);
+	const unsigned long mask = BIT(offset % 8);
+
+	return !!(ioread8(port_addr + in_port) & mask);
 }
 
 static int idi_48_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
+	struct idi_48_reg __iomem *const reg = idi48gpio->reg;
 	unsigned long offset;
 	unsigned long gpio_mask;
-	static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-	void __iomem *port_addr;
+	unsigned long boundary;
+	unsigned long group;
+	unsigned long in_port;
+	u8 __iomem *port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
-		port_addr = idi48gpio->base + ports[offset / 8];
-		port_state = ioread8(port_addr) & gpio_mask;
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		boundary = offset / 8;
+		group = boundary / 3;
+		port_addr = group ? reg->port1 : reg->port0;
+		in_port = boundary - (group * 3);
+		port_state = ioread8(port_addr + in_port) & gpio_mask;
 
 		bitmap_set_value8(bits, port_state, offset);
 	}
@@ -113,30 +128,21 @@ static void idi_48_irq_mask(struct irq_data *data)
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
 	const unsigned offset = irqd_to_hwirq(data);
-	unsigned i;
-	unsigned mask;
-	unsigned boundary;
+	const unsigned long boundary = offset / 8;
+	const unsigned long mask = BIT(offset % 8);
 	unsigned long flags;
 
-	for (i = 0; i < 48; i += 8)
-		if (offset < i + 8) {
-			mask = BIT(offset - i);
-			boundary = i / 8;
-
-			idi48gpio->irq_mask[boundary] &= ~mask;
-
-			if (!idi48gpio->irq_mask[boundary]) {
-				idi48gpio->cos_enb &= ~BIT(boundary);
-
-				raw_spin_lock_irqsave(&idi48gpio->lock, flags);
+	idi48gpio->irq_mask[boundary] &= ~mask;
 
-				iowrite8(idi48gpio->cos_enb, idi48gpio->base + 7);
+	/* Exit early if there are still input lines with IRQ unmasked */
+	if (idi48gpio->irq_mask[boundary])
+		return;
 
-				raw_spin_unlock_irqrestore(&idi48gpio->lock, flags);
-			}
+	idi48gpio->cos_enb &= ~BIT(boundary);
 
-			return;
-		}
+	raw_spin_lock_irqsave(&idi48gpio->lock, flags);
+	iowrite8(idi48gpio->cos_enb, &idi48gpio->reg->irq);
+	raw_spin_unlock_irqrestore(&idi48gpio->lock, flags);
 }
 
 static void idi_48_irq_unmask(struct irq_data *data)
@@ -144,32 +150,24 @@ static void idi_48_irq_unmask(struct irq_data *data)
 	struct gpio_chip *chip = irq_data_get_irq_chip_data(data);
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(chip);
 	const unsigned offset = irqd_to_hwirq(data);
-	unsigned i;
-	unsigned mask;
-	unsigned boundary;
+	const unsigned long boundary = offset / 8;
+	const unsigned long mask = BIT(offset % 8);
 	unsigned prev_irq_mask;
 	unsigned long flags;
 
-	for (i = 0; i < 48; i += 8)
-		if (offset < i + 8) {
-			mask = BIT(offset - i);
-			boundary = i / 8;
-			prev_irq_mask = idi48gpio->irq_mask[boundary];
-
-			idi48gpio->irq_mask[boundary] |= mask;
+	prev_irq_mask = idi48gpio->irq_mask[boundary];
 
-			if (!prev_irq_mask) {
-				idi48gpio->cos_enb |= BIT(boundary);
+	idi48gpio->irq_mask[boundary] |= mask;
 
-				raw_spin_lock_irqsave(&idi48gpio->lock, flags);
+	/* Exit early if IRQ was already unmasked for this boundary */
+	if (prev_irq_mask)
+		return;
 
-				iowrite8(idi48gpio->cos_enb, idi48gpio->base + 7);
+	idi48gpio->cos_enb |= BIT(boundary);
 
-				raw_spin_unlock_irqrestore(&idi48gpio->lock, flags);
-			}
-
-			return;
-		}
+	raw_spin_lock_irqsave(&idi48gpio->lock, flags);
+	iowrite8(idi48gpio->cos_enb, &idi48gpio->reg->irq);
+	raw_spin_unlock_irqrestore(&idi48gpio->lock, flags);
 }
 
 static int idi_48_irq_set_type(struct irq_data *data, unsigned flow_type)
@@ -204,7 +202,7 @@ static irqreturn_t idi_48_irq_handler(int irq, void *dev_id)
 
 	raw_spin_lock(&idi48gpio->lock);
 
-	cos_status = ioread8(idi48gpio->base + 7);
+	cos_status = ioread8(&idi48gpio->reg->irq);
 
 	raw_spin_unlock(&idi48gpio->lock);
 
@@ -250,8 +248,8 @@ static int idi_48_irq_init_hw(struct gpio_chip *gc)
 	struct idi_48_gpio *const idi48gpio = gpiochip_get_data(gc);
 
 	/* Disable IRQ by default */
-	iowrite8(0, idi48gpio->base + 7);
-	ioread8(idi48gpio->base + 7);
+	iowrite8(0, &idi48gpio->reg->irq);
+	ioread8(&idi48gpio->reg->irq);
 
 	return 0;
 }
@@ -273,8 +271,8 @@ static int idi_48_probe(struct device *dev, unsigned int id)
 		return -EBUSY;
 	}
 
-	idi48gpio->base = devm_ioport_map(dev, base[id], IDI_48_EXTENT);
-	if (!idi48gpio->base)
+	idi48gpio->reg = devm_ioport_map(dev, base[id], IDI_48_EXTENT);
+	if (!idi48gpio->reg)
 		return -ENOMEM;
 
 	idi48gpio->chip.label = name;
-- 
2.36.1

[PATCH 3/5] gpio: 104-idio-16: Implement and utilize register structures

[William Breathitt Gray]

Reduce magic numbers and improve code readability by implementing and
utilizing named register data structures.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/gpio-104-idio-16.c | 58 +++++++++++++++++++++++----------
 1 file changed, 41 insertions(+), 17 deletions(-)

diff --git a/drivers/gpio/gpio-104-idio-16.c b/drivers/gpio/gpio-104-idio-16.c
index 45f7ad8573e1..2f8e295f5541 100644
--- a/drivers/gpio/gpio-104-idio-16.c
+++ b/drivers/gpio/gpio-104-idio-16.c
@@ -19,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 
 #define IDIO_16_EXTENT 8
 #define MAX_NUM_IDIO_16 max_num_isa_dev(IDIO_16_EXTENT)
@@ -32,19 +33,42 @@ static unsigned int irq[MAX_NUM_IDIO_16];
 module_param_hw_array(irq, uint, irq, NULL, 0);
 MODULE_PARM_DESC(irq, "ACCES 104-IDIO-16 interrupt line numbers");
 
+/**
+ * struct idio_16_reg - device registers structure
+ * @out0_7:	Read: N/A
+ *		Write: FET Drive Outputs 0-7
+ * @in0_7:	Read: Isolated Inputs 0-7
+ *		Write: Clear Interrupt
+ * @irq_ctl:	Read: Enable IRQ
+ *		Write: Disable IRQ
+ * @unused:	N/A
+ * @out8_15:	Read: N/A
+ *		Write: FET Drive Outputs 8-15
+ * @in8_15:	Read: Isolated Inputs 8-15
+ *		Write: N/A
+ */
+struct idio_16_reg {
+	u8 out0_7;
+	u8 in0_7;
+	u8 irq_ctl;
+	u8 unused;
+	u8 out8_15;
+	u8 in8_15;
+};
+
 /**
  * struct idio_16_gpio - GPIO device private data structure
  * @chip:	instance of the gpio_chip
  * @lock:	synchronization lock to prevent I/O race conditions
  * @irq_mask:	I/O bits affected by interrupts
- * @base:	base port address of the GPIO device
+ * @reg:	I/O address offset for the device registers
  * @out_state:	output bits state
  */
 struct idio_16_gpio {
 	struct gpio_chip chip;
 	raw_spinlock_t lock;
 	unsigned long irq_mask;
-	void __iomem *base;
+	struct idio_16_reg __iomem *reg;
 	unsigned int out_state;
 };
 
@@ -79,9 +103,9 @@ static int idio_16_gpio_get(struct gpio_chip *chip, unsigned int offset)
 		return -EINVAL;
 
 	if (offset < 24)
-		return !!(ioread8(idio16gpio->base + 1) & mask);
+		return !!(ioread8(&idio16gpio->reg->in0_7) & mask);
 
-	return !!(ioread8(idio16gpio->base + 5) & (mask>>8));
+	return !!(ioread8(&idio16gpio->reg->in8_15) & (mask>>8));
 }
 
 static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
@@ -91,9 +115,9 @@ static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
 
 	*bits = 0;
 	if (*mask & GENMASK(23, 16))
-		*bits |= (unsigned long)ioread8(idio16gpio->base + 1) << 16;
+		*bits |= (unsigned long)ioread8(&idio16gpio->reg->in0_7) << 16;
 	if (*mask & GENMASK(31, 24))
-		*bits |= (unsigned long)ioread8(idio16gpio->base + 5) << 24;
+		*bits |= (unsigned long)ioread8(&idio16gpio->reg->in8_15) << 24;
 
 	return 0;
 }
@@ -116,9 +140,9 @@ static void idio_16_gpio_set(struct gpio_chip *chip, unsigned int offset,
 		idio16gpio->out_state &= ~mask;
 
 	if (offset > 7)
-		iowrite8(idio16gpio->out_state >> 8, idio16gpio->base + 4);
+		iowrite8(idio16gpio->out_state >> 8, &idio16gpio->reg->out8_15);
 	else
-		iowrite8(idio16gpio->out_state, idio16gpio->base);
+		iowrite8(idio16gpio->out_state, &idio16gpio->reg->out0_7);
 
 	raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 }
@@ -135,9 +159,9 @@ static void idio_16_gpio_set_multiple(struct gpio_chip *chip,
 	idio16gpio->out_state |= *mask & *bits;
 
 	if (*mask & 0xFF)
-		iowrite8(idio16gpio->out_state, idio16gpio->base);
+		iowrite8(idio16gpio->out_state, &idio16gpio->reg->out0_7);
 	if ((*mask >> 8) & 0xFF)
-		iowrite8(idio16gpio->out_state >> 8, idio16gpio->base + 4);
+		iowrite8(idio16gpio->out_state >> 8, &idio16gpio->reg->out8_15);
 
 	raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 }
@@ -158,7 +182,7 @@ static void idio_16_irq_mask(struct irq_data *data)
 	if (!idio16gpio->irq_mask) {
 		raw_spin_lock_irqsave(&idio16gpio->lock, flags);
 
-		iowrite8(0, idio16gpio->base + 2);
+		iowrite8(0, &idio16gpio->reg->irq_ctl);
 
 		raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 	}
@@ -177,7 +201,7 @@ static void idio_16_irq_unmask(struct irq_data *data)
 	if (!prev_irq_mask) {
 		raw_spin_lock_irqsave(&idio16gpio->lock, flags);
 
-		ioread8(idio16gpio->base + 2);
+		ioread8(&idio16gpio->reg->irq_ctl);
 
 		raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
 	}
@@ -212,7 +236,7 @@ static irqreturn_t idio_16_irq_handler(int irq, void *dev_id)
 
 	raw_spin_lock(&idio16gpio->lock);
 
-	iowrite8(0, idio16gpio->base + 1);
+	iowrite8(0, &idio16gpio->reg->in0_7);
 
 	raw_spin_unlock(&idio16gpio->lock);
 
@@ -232,8 +256,8 @@ static int idio_16_irq_init_hw(struct gpio_chip *gc)
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(gc);
 
 	/* Disable IRQ by default */
-	iowrite8(0, idio16gpio->base + 2);
-	iowrite8(0, idio16gpio->base + 1);
+	iowrite8(0, &idio16gpio->reg->irq_ctl);
+	iowrite8(0, &idio16gpio->reg->in0_7);
 
 	return 0;
 }
@@ -255,8 +279,8 @@ static int idio_16_probe(struct device *dev, unsigned int id)
 		return -EBUSY;
 	}
 
-	idio16gpio->base = devm_ioport_map(dev, base[id], IDIO_16_EXTENT);
-	if (!idio16gpio->base)
+	idio16gpio->reg = devm_ioport_map(dev, base[id], IDIO_16_EXTENT);
+	if (!idio16gpio->reg)
 		return -ENOMEM;
 
 	idio16gpio->chip.label = name;
-- 
2.36.1

[PATCH 4/5] gpio: gpio-mm: Implement and utilize register structures

[William Breathitt Gray]

Reduce magic numbers and improve code readability by implementing and
utilizing named register data structures.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/gpio-gpio-mm.c | 116 ++++++++++++++++++++++--------------
 1 file changed, 72 insertions(+), 44 deletions(-)

diff --git a/drivers/gpio/gpio-gpio-mm.c b/drivers/gpio/gpio-gpio-mm.c
index 097a06463d01..8f67442b8715 100644
--- a/drivers/gpio/gpio-gpio-mm.c
+++ b/drivers/gpio/gpio-gpio-mm.c
@@ -27,6 +27,16 @@ static unsigned int num_gpiomm;
 module_param_hw_array(base, uint, ioport, &num_gpiomm, 0);
 MODULE_PARM_DESC(base, "Diamond Systems GPIO-MM base addresses");
 
+/**
+ * struct i8255_reg - Intel 8255 register structure
+ * @port:	Port A, B, and C
+ * @control:	Control register
+ */
+struct i8255_reg {
+	u8 port[3];
+	u8 control;
+};
+
 /**
  * struct gpiomm_gpio - GPIO device private data structure
  * @chip:	instance of the gpio_chip
@@ -34,7 +44,7 @@ MODULE_PARM_DESC(base, "Diamond Systems GPIO-MM base addresses");
  * @out_state:	output bits state
  * @control:	Control registers state
  * @lock:	synchronization lock to prevent I/O race conditions
- * @base:	base port address of the GPIO device
+ * @dio:	Digital I/O register groups
  */
 struct gpiomm_gpio {
 	struct gpio_chip chip;
@@ -42,7 +52,7 @@ struct gpiomm_gpio {
 	unsigned char out_state[6];
 	unsigned char control[2];
 	spinlock_t lock;
-	void __iomem *base;
+	struct i8255_reg __iomem *dio;
 };
 
 static int gpiomm_gpio_get_direction(struct gpio_chip *chip,
@@ -63,32 +73,33 @@ static int gpiomm_gpio_direction_input(struct gpio_chip *chip,
 {
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
 	const unsigned int io_port = offset / 8;
-	const unsigned int control_port = io_port / 3;
+	const unsigned long group = io_port / 3;
+	const unsigned long port = io_port - (group * 3);
 	unsigned long flags;
 	unsigned int control;
 
 	spin_lock_irqsave(&gpiommgpio->lock, flags);
 
 	/* Check if configuring Port C */
-	if (io_port == 2 || io_port == 5) {
+	if (port == 2) {
 		/* Port C can be configured by nibble */
 		if (offset % 8 > 3) {
 			gpiommgpio->io_state[io_port] |= 0xF0;
-			gpiommgpio->control[control_port] |= BIT(3);
+			gpiommgpio->control[group] |= BIT(3);
 		} else {
 			gpiommgpio->io_state[io_port] |= 0x0F;
-			gpiommgpio->control[control_port] |= BIT(0);
+			gpiommgpio->control[group] |= BIT(0);
 		}
 	} else {
 		gpiommgpio->io_state[io_port] |= 0xFF;
-		if (io_port == 0 || io_port == 3)
-			gpiommgpio->control[control_port] |= BIT(4);
+		if (port == 0)
+			gpiommgpio->control[group] |= BIT(4);
 		else
-			gpiommgpio->control[control_port] |= BIT(1);
+			gpiommgpio->control[group] |= BIT(1);
 	}
 
-	control = BIT(7) | gpiommgpio->control[control_port];
-	iowrite8(control, gpiommgpio->base + 3 + control_port*4);
+	control = BIT(7) | gpiommgpio->control[group];
+	iowrite8(control, &gpiommgpio->dio[group].control);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 
@@ -100,30 +111,31 @@ static int gpiomm_gpio_direction_output(struct gpio_chip *chip,
 {
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
 	const unsigned int io_port = offset / 8;
-	const unsigned int control_port = io_port / 3;
+	const unsigned long group = io_port / 3;
+	const unsigned long port = io_port - (group * 3);
 	const unsigned int mask = BIT(offset % 8);
-	const unsigned int out_port = (io_port > 2) ? io_port + 1 : io_port;
+	struct i8255_reg __iomem *const dio = gpiommgpio->dio + group;
 	unsigned long flags;
 	unsigned int control;
 
 	spin_lock_irqsave(&gpiommgpio->lock, flags);
 
 	/* Check if configuring Port C */
-	if (io_port == 2 || io_port == 5) {
+	if (port == 2) {
 		/* Port C can be configured by nibble */
 		if (offset % 8 > 3) {
 			gpiommgpio->io_state[io_port] &= 0x0F;
-			gpiommgpio->control[control_port] &= ~BIT(3);
+			gpiommgpio->control[group] &= ~BIT(3);
 		} else {
 			gpiommgpio->io_state[io_port] &= 0xF0;
-			gpiommgpio->control[control_port] &= ~BIT(0);
+			gpiommgpio->control[group] &= ~BIT(0);
 		}
 	} else {
 		gpiommgpio->io_state[io_port] &= 0x00;
-		if (io_port == 0 || io_port == 3)
-			gpiommgpio->control[control_port] &= ~BIT(4);
+		if (port == 0)
+			gpiommgpio->control[group] &= ~BIT(4);
 		else
-			gpiommgpio->control[control_port] &= ~BIT(1);
+			gpiommgpio->control[group] &= ~BIT(1);
 	}
 
 	if (value)
@@ -131,10 +143,10 @@ static int gpiomm_gpio_direction_output(struct gpio_chip *chip,
 	else
 		gpiommgpio->out_state[io_port] &= ~mask;
 
-	control = BIT(7) | gpiommgpio->control[control_port];
-	iowrite8(control, gpiommgpio->base + 3 + control_port*4);
+	control = BIT(7) | gpiommgpio->control[group];
+	iowrite8(control, &dio->control);
 
-	iowrite8(gpiommgpio->out_state[io_port], gpiommgpio->base + out_port);
+	iowrite8(gpiommgpio->out_state[io_port], dio->port + port);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 
@@ -146,7 +158,8 @@ static int gpiomm_gpio_get(struct gpio_chip *chip, unsigned int offset)
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
 	const unsigned int port = offset / 8;
 	const unsigned int mask = BIT(offset % 8);
-	const unsigned int in_port = (port > 2) ? port + 1 : port;
+	const unsigned long group = port / 3;
+	const unsigned long in_port = port - (group * 3);
 	unsigned long flags;
 	unsigned int port_state;
 
@@ -158,29 +171,33 @@ static int gpiomm_gpio_get(struct gpio_chip *chip, unsigned int offset)
 		return -EINVAL;
 	}
 
-	port_state = ioread8(gpiommgpio->base + in_port);
+	port_state = ioread8(gpiommgpio->dio[group].port + in_port);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 
 	return !!(port_state & mask);
 }
 
-static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-
 static int gpiomm_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 	unsigned long *bits)
 {
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
 	unsigned long offset;
 	unsigned long gpio_mask;
-	void __iomem *port_addr;
+	unsigned long port;
+	unsigned long group;
+	unsigned long in_port;
+	u8 __iomem *port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
-		port_addr = gpiommgpio->base + ports[offset / 8];
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
+		port = offset / 8;
+		group = port / 3;
+		in_port = port - (group * 3);
+		port_addr = gpiommgpio->dio[group].port + in_port;
 		port_state = ioread8(port_addr) & gpio_mask;
 
 		bitmap_set_value8(bits, port_state, offset);
@@ -195,7 +212,8 @@ static void gpiomm_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
 	const unsigned int port = offset / 8;
 	const unsigned int mask = BIT(offset % 8);
-	const unsigned int out_port = (port > 2) ? port + 1 : port;
+	const unsigned long group = port / 3;
+	const unsigned long out_port = port - (group * 3);
 	unsigned long flags;
 
 	spin_lock_irqsave(&gpiommgpio->lock, flags);
@@ -205,7 +223,8 @@ static void gpiomm_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	else
 		gpiommgpio->out_state[port] &= ~mask;
 
-	iowrite8(gpiommgpio->out_state[port], gpiommgpio->base + out_port);
+	iowrite8(gpiommgpio->out_state[port],
+		 gpiommgpio->dio[group].port + out_port);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 }
@@ -217,13 +236,17 @@ static void gpiomm_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long offset;
 	unsigned long gpio_mask;
 	size_t index;
-	void __iomem *port_addr;
+	size_t group;
+	size_t out_port;
+	u8 __iomem *port_addr;
 	unsigned long bitmask;
 	unsigned long flags;
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
+	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
 		index = offset / 8;
-		port_addr = gpiommgpio->base + ports[index];
+		group = index / 3;
+		out_port = index - (group * 3);
+		port_addr = gpiommgpio->dio[group].port + out_port;
 
 		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
 
@@ -250,6 +273,17 @@ static const char *gpiomm_names[GPIOMM_NGPIO] = {
 	"Port 2C2", "Port 2C3", "Port 2C4", "Port 2C5", "Port 2C6", "Port 2C7",
 };
 
+static void gpiomm_init_dio(struct i8255_reg __iomem *const dio)
+{
+	/* Activate Mode Set; set Mode 0 output mode for Port A, B, and C */
+	iowrite8(0x80, &dio->control);
+
+	/* Initialize all GPIO to 0 */
+	iowrite8(0x00, &dio->port[0]);
+	iowrite8(0x00, &dio->port[1]);
+	iowrite8(0x00, &dio->port[2]);
+}
+
 static int gpiomm_probe(struct device *dev, unsigned int id)
 {
 	struct gpiomm_gpio *gpiommgpio;
@@ -266,8 +300,8 @@ static int gpiomm_probe(struct device *dev, unsigned int id)
 		return -EBUSY;
 	}
 
-	gpiommgpio->base = devm_ioport_map(dev, base[id], GPIOMM_EXTENT);
-	if (!gpiommgpio->base)
+	gpiommgpio->dio = devm_ioport_map(dev, base[id], GPIOMM_EXTENT);
+	if (!gpiommgpio->dio)
 		return -ENOMEM;
 
 	gpiommgpio->chip.label = name;
@@ -293,14 +327,8 @@ static int gpiomm_probe(struct device *dev, unsigned int id)
 	}
 
 	/* initialize all GPIO as output */
-	iowrite8(0x80, gpiommgpio->base + 3);
-	iowrite8(0x00, gpiommgpio->base);
-	iowrite8(0x00, gpiommgpio->base + 1);
-	iowrite8(0x00, gpiommgpio->base + 2);
-	iowrite8(0x80, gpiommgpio->base + 7);
-	iowrite8(0x00, gpiommgpio->base + 4);
-	iowrite8(0x00, gpiommgpio->base + 5);
-	iowrite8(0x00, gpiommgpio->base + 6);
+	gpiomm_init_dio(&gpiommgpio->dio[0]);
+	gpiomm_init_dio(&gpiommgpio->dio[1]);
 
 	return 0;
 }
-- 
2.36.1

[PATCH 5/5] gpio: ws16c48: Implement and utilize register structures

[William Breathitt Gray]

Reduce magic numbers and improve code readability by implementing and
utilizing named register data structures.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/gpio-ws16c48.c | 119 +++++++++++++++++++++++++-----------
 1 file changed, 84 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpio-ws16c48.c b/drivers/gpio/gpio-ws16c48.c
index 5078631d8014..663d4491b90f 100644
--- a/drivers/gpio/gpio-ws16c48.c
+++ b/drivers/gpio/gpio-ws16c48.c
@@ -17,8 +17,9 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 
-#define WS16C48_EXTENT 16
+#define WS16C48_EXTENT 10
 #define MAX_NUM_WS16C48 max_num_isa_dev(WS16C48_EXTENT)
 
 static unsigned int base[MAX_NUM_WS16C48];
@@ -30,6 +31,20 @@ static unsigned int irq[MAX_NUM_WS16C48];
 module_param_hw_array(irq, uint, irq, NULL, 0);
 MODULE_PARM_DESC(irq, "WinSystems WS16C48 interrupt line numbers");
 
+/**
+ * struct ws16c48_reg - device register structure
+ * @port:		Port 0 through 5 I/O
+ * @int_pending:	Interrupt Pending
+ * @page_lock:		Register page (Bits 7-6) and I/O port lock (Bits 5-0)
+ * @pol_enab_int_id:	Interrupt polarity, enable, and ID
+ */
+struct ws16c48_reg {
+	u8 port[6];
+	u8 int_pending;
+	u8 page_lock;
+	u8 pol_enab_int_id[3];
+};
+
 /**
  * struct ws16c48_gpio - GPIO device private data structure
  * @chip:	instance of the gpio_chip
@@ -38,7 +53,7 @@ MODULE_PARM_DESC(irq, "WinSystems WS16C48 interrupt line numbers");
  * @lock:	synchronization lock to prevent I/O race conditions
  * @irq_mask:	I/O bits affected by interrupts
  * @flow_mask:	IRQ flow type mask for the respective I/O bits
- * @base:	base port address of the GPIO device
+ * @reg:	I/O address offset for the device registers
  */
 struct ws16c48_gpio {
 	struct gpio_chip chip;
@@ -47,7 +62,7 @@ struct ws16c48_gpio {
 	raw_spinlock_t lock;
 	unsigned long irq_mask;
 	unsigned long flow_mask;
-	void __iomem *base;
+	struct ws16c48_reg __iomem *reg;
 };
 
 static int ws16c48_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
@@ -73,7 +88,7 @@ static int ws16c48_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
 
 	ws16c48gpio->io_state[port] |= mask;
 	ws16c48gpio->out_state[port] &= ~mask;
-	iowrite8(ws16c48gpio->out_state[port], ws16c48gpio->base + port);
+	iowrite8(ws16c48gpio->out_state[port], ws16c48gpio->reg->port + port);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 
@@ -95,7 +110,7 @@ static int ws16c48_gpio_direction_output(struct gpio_chip *chip,
 		ws16c48gpio->out_state[port] |= mask;
 	else
 		ws16c48gpio->out_state[port] &= ~mask;
-	iowrite8(ws16c48gpio->out_state[port], ws16c48gpio->base + port);
+	iowrite8(ws16c48gpio->out_state[port], ws16c48gpio->reg->port + port);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 
@@ -118,7 +133,7 @@ static int ws16c48_gpio_get(struct gpio_chip *chip, unsigned offset)
 		return -EINVAL;
 	}
 
-	port_state = ioread8(ws16c48gpio->base + port);
+	port_state = ioread8(ws16c48gpio->reg->port + port);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 
@@ -131,14 +146,16 @@ static int ws16c48_gpio_get_multiple(struct gpio_chip *chip,
 	struct ws16c48_gpio *const ws16c48gpio = gpiochip_get_data(chip);
 	unsigned long offset;
 	unsigned long gpio_mask;
-	void __iomem *port_addr;
+	size_t index;
+	u8 __iomem *port_addr;
 	unsigned long port_state;
 
 	/* clear bits array to a clean slate */
 	bitmap_zero(bits, chip->ngpio);
 
 	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
-		port_addr = ws16c48gpio->base + offset / 8;
+		index = offset / 8;
+		port_addr = ws16c48gpio->reg->port + index;
 		port_state = ioread8(port_addr) & gpio_mask;
 
 		bitmap_set_value8(bits, port_state, offset);
@@ -166,7 +183,7 @@ static void ws16c48_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
 		ws16c48gpio->out_state[port] |= mask;
 	else
 		ws16c48gpio->out_state[port] &= ~mask;
-	iowrite8(ws16c48gpio->out_state[port], ws16c48gpio->base + port);
+	iowrite8(ws16c48gpio->out_state[port], ws16c48gpio->reg->port + port);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 }
@@ -178,13 +195,13 @@ static void ws16c48_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long offset;
 	unsigned long gpio_mask;
 	size_t index;
-	void __iomem *port_addr;
+	u8 __iomem *port_addr;
 	unsigned long bitmask;
 	unsigned long flags;
 
 	for_each_set_clump8(offset, gpio_mask, mask, chip->ngpio) {
 		index = offset / 8;
-		port_addr = ws16c48gpio->base + index;
+		port_addr = ws16c48gpio->reg->port + index;
 
 		/* mask out GPIO configured for input */
 		gpio_mask &= ~ws16c48gpio->io_state[index];
@@ -219,10 +236,15 @@ static void ws16c48_irq_ack(struct irq_data *data)
 
 	port_state = ws16c48gpio->irq_mask >> (8*port);
 
-	iowrite8(0x80, ws16c48gpio->base + 7);
-	iowrite8(port_state & ~mask, ws16c48gpio->base + 8 + port);
-	iowrite8(port_state | mask, ws16c48gpio->base + 8 + port);
-	iowrite8(0xC0, ws16c48gpio->base + 7);
+	/* Select Register Page 2; Unlock all I/O ports */
+	iowrite8(0x80, &ws16c48gpio->reg->page_lock);
+
+	/* Clear pending interrupt */
+	iowrite8(port_state & ~mask, ws16c48gpio->reg->pol_enab_int_id + port);
+	iowrite8(port_state | mask, ws16c48gpio->reg->pol_enab_int_id + port);
+
+	/* Select Register Page 3; Unlock all I/O ports */
+	iowrite8(0xC0, &ws16c48gpio->reg->page_lock);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 }
@@ -235,6 +257,7 @@ static void ws16c48_irq_mask(struct irq_data *data)
 	const unsigned long mask = BIT(offset);
 	const unsigned port = offset / 8;
 	unsigned long flags;
+	unsigned long port_state;
 
 	/* only the first 3 ports support interrupts */
 	if (port > 2)
@@ -243,10 +266,16 @@ static void ws16c48_irq_mask(struct irq_data *data)
 	raw_spin_lock_irqsave(&ws16c48gpio->lock, flags);
 
 	ws16c48gpio->irq_mask &= ~mask;
+	port_state = ws16c48gpio->irq_mask >> (8 * port);
+
+	/* Select Register Page 2; Unlock all I/O ports */
+	iowrite8(0x80, &ws16c48gpio->reg->page_lock);
 
-	iowrite8(0x80, ws16c48gpio->base + 7);
-	iowrite8(ws16c48gpio->irq_mask >> (8*port), ws16c48gpio->base + 8 + port);
-	iowrite8(0xC0, ws16c48gpio->base + 7);
+	/* Disable interrupt */
+	iowrite8(port_state, ws16c48gpio->reg->pol_enab_int_id + port);
+
+	/* Select Register Page 3; Unlock all I/O ports */
+	iowrite8(0xC0, &ws16c48gpio->reg->page_lock);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 }
@@ -259,6 +288,7 @@ static void ws16c48_irq_unmask(struct irq_data *data)
 	const unsigned long mask = BIT(offset);
 	const unsigned port = offset / 8;
 	unsigned long flags;
+	unsigned long port_state;
 
 	/* only the first 3 ports support interrupts */
 	if (port > 2)
@@ -267,10 +297,16 @@ static void ws16c48_irq_unmask(struct irq_data *data)
 	raw_spin_lock_irqsave(&ws16c48gpio->lock, flags);
 
 	ws16c48gpio->irq_mask |= mask;
+	port_state = ws16c48gpio->irq_mask >> (8 * port);
+
+	/* Select Register Page 2; Unlock all I/O ports */
+	iowrite8(0x80, &ws16c48gpio->reg->page_lock);
 
-	iowrite8(0x80, ws16c48gpio->base + 7);
-	iowrite8(ws16c48gpio->irq_mask >> (8*port), ws16c48gpio->base + 8 + port);
-	iowrite8(0xC0, ws16c48gpio->base + 7);
+	/* Enable interrupt */
+	iowrite8(port_state, ws16c48gpio->reg->pol_enab_int_id + port);
+
+	/* Select Register Page 3; Unlock all I/O ports */
+	iowrite8(0xC0, &ws16c48gpio->reg->page_lock);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 }
@@ -283,6 +319,7 @@ static int ws16c48_irq_set_type(struct irq_data *data, unsigned flow_type)
 	const unsigned long mask = BIT(offset);
 	const unsigned port = offset / 8;
 	unsigned long flags;
+	unsigned long port_state;
 
 	/* only the first 3 ports support interrupts */
 	if (port > 2)
@@ -304,9 +341,16 @@ static int ws16c48_irq_set_type(struct irq_data *data, unsigned flow_type)
 		return -EINVAL;
 	}
 
-	iowrite8(0x40, ws16c48gpio->base + 7);
-	iowrite8(ws16c48gpio->flow_mask >> (8*port), ws16c48gpio->base + 8 + port);
-	iowrite8(0xC0, ws16c48gpio->base + 7);
+	port_state = ws16c48gpio->flow_mask >> (8 * port);
+
+	/* Select Register Page 1; Unlock all I/O ports */
+	iowrite8(0x40, &ws16c48gpio->reg->page_lock);
+
+	/* Set interrupt polarity */
+	iowrite8(port_state, ws16c48gpio->reg->pol_enab_int_id + port);
+
+	/* Select Register Page 3; Unlock all I/O ports */
+	iowrite8(0xC0, &ws16c48gpio->reg->page_lock);
 
 	raw_spin_unlock_irqrestore(&ws16c48gpio->lock, flags);
 
@@ -325,25 +369,26 @@ static irqreturn_t ws16c48_irq_handler(int irq, void *dev_id)
 {
 	struct ws16c48_gpio *const ws16c48gpio = dev_id;
 	struct gpio_chip *const chip = &ws16c48gpio->chip;
+	struct ws16c48_reg __iomem *const reg = ws16c48gpio->reg;
 	unsigned long int_pending;
 	unsigned long port;
 	unsigned long int_id;
 	unsigned long gpio;
 
-	int_pending = ioread8(ws16c48gpio->base + 6) & 0x7;
+	int_pending = ioread8(&reg->int_pending) & 0x7;
 	if (!int_pending)
 		return IRQ_NONE;
 
 	/* loop until all pending interrupts are handled */
 	do {
 		for_each_set_bit(port, &int_pending, 3) {
-			int_id = ioread8(ws16c48gpio->base + 8 + port);
+			int_id = ioread8(reg->pol_enab_int_id + port);
 			for_each_set_bit(gpio, &int_id, 8)
 				generic_handle_domain_irq(chip->irq.domain,
 							  gpio + 8*port);
 		}
 
-		int_pending = ioread8(ws16c48gpio->base + 6) & 0x7;
+		int_pending = ioread8(&reg->int_pending) & 0x7;
 	} while (int_pending);
 
 	return IRQ_HANDLED;
@@ -369,12 +414,16 @@ static int ws16c48_irq_init_hw(struct gpio_chip *gc)
 {
 	struct ws16c48_gpio *const ws16c48gpio = gpiochip_get_data(gc);
 
-	/* Disable IRQ by default */
-	iowrite8(0x80, ws16c48gpio->base + 7);
-	iowrite8(0, ws16c48gpio->base + 8);
-	iowrite8(0, ws16c48gpio->base + 9);
-	iowrite8(0, ws16c48gpio->base + 10);
-	iowrite8(0xC0, ws16c48gpio->base + 7);
+	/* Select Register Page 2; Unlock all I/O ports */
+	iowrite8(0x80, &ws16c48gpio->reg->page_lock);
+
+	/* Disable interrupts for all lines */
+	iowrite8(0, &ws16c48gpio->reg->pol_enab_int_id[0]);
+	iowrite8(0, &ws16c48gpio->reg->pol_enab_int_id[1]);
+	iowrite8(0, &ws16c48gpio->reg->pol_enab_int_id[2]);
+
+	/* Select Register Page 3; Unlock all I/O ports */
+	iowrite8(0xC0, &ws16c48gpio->reg->page_lock);
 
 	return 0;
 }
@@ -396,8 +445,8 @@ static int ws16c48_probe(struct device *dev, unsigned int id)
 		return -EBUSY;
 	}
 
-	ws16c48gpio->base = devm_ioport_map(dev, base[id], WS16C48_EXTENT);
-	if (!ws16c48gpio->base)
+	ws16c48gpio->reg = devm_ioport_map(dev, base[id], WS16C48_EXTENT);
+	if (!ws16c48gpio->reg)
 		return -ENOMEM;
 
 	ws16c48gpio->chip.label = name;
-- 
2.36.1

Re: [PATCH 0/5] gpio: Implement and utilize register structures for ISA drivers

[Bartosz Golaszewski]

On Mon, Jun 6, 2022 at 4:36 PM William Breathitt Gray
<william.gray@linaro.org> wrote:
>
> The PC104/ISA drivers were updated to use I/O memory accessor calls such
> as ioread8()/iowrite8() in a previous patch series [1]. This
> patchset is a continuation of the effort to improve the code readability
> and reduce magic numbers by implementing and utilizing named register
> data structures.
>
> One of the benefits is that we can now observe more easily similarities
> in devices that share similar interfaces; such as the i8255 interfaces
> used by the 104-DIO-48E, 104-IDI-48, and GPIO-MM drivers -- as well as
> the similar interface used by the 104-IDIO-16 and PCI-IDIO-16 drivers. I
> hope to consolidate some of these code blocks in future patchsets.
>
> [1] https://lore.kernel.org/all/cover.1652201921.git.william.gray@linaro.org/
>
> William Breathitt Gray (5):
>   gpio: 104-dio-48e: Implement and utilize register structures
>   gpio: 104-idi-48: Implement and utilize register structures
>   gpio: 104-idio-16: Implement and utilize register structures
>   gpio: gpio-mm: Implement and utilize register structures
>   gpio: ws16c48: Implement and utilize register structures
>
>  drivers/gpio/gpio-104-dio-48e.c | 157 +++++++++++++++++++++-----------
>  drivers/gpio/gpio-104-idi-48.c  | 128 +++++++++++++-------------
>  drivers/gpio/gpio-104-idio-16.c |  58 ++++++++----
>  drivers/gpio/gpio-gpio-mm.c     | 116 ++++++++++++++---------
>  drivers/gpio/gpio-ws16c48.c     | 119 +++++++++++++++++-------
>  5 files changed, 366 insertions(+), 212 deletions(-)
>
>
> base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> --
> 2.36.1
>

Hi William,

Unlike the previous patches which were relatively simple, these seem
like there's a lot of space for breakage (even though they're
attempting to do a good thing). Have you tested the code on real
hardware?

Bart

Re: [PATCH 0/5] gpio: Implement and utilize register structures for ISA drivers

[William Breathitt Gray]

[-- Attachment #1: Type: text/plain, Size: 2677 bytes --]

On Thu, Jun 09, 2022 at 04:39:41PM +0200, Bartosz Golaszewski wrote:
> On Mon, Jun 6, 2022 at 4:36 PM William Breathitt Gray
> <william.gray@linaro.org> wrote:
> >
> > The PC104/ISA drivers were updated to use I/O memory accessor calls such
> > as ioread8()/iowrite8() in a previous patch series [1]. This
> > patchset is a continuation of the effort to improve the code readability
> > and reduce magic numbers by implementing and utilizing named register
> > data structures.
> >
> > One of the benefits is that we can now observe more easily similarities
> > in devices that share similar interfaces; such as the i8255 interfaces
> > used by the 104-DIO-48E, 104-IDI-48, and GPIO-MM drivers -- as well as
> > the similar interface used by the 104-IDIO-16 and PCI-IDIO-16 drivers. I
> > hope to consolidate some of these code blocks in future patchsets.
> >
> > [1] https://lore.kernel.org/all/cover.1652201921.git.william.gray@linaro.org/
> >
> > William Breathitt Gray (5):
> >   gpio: 104-dio-48e: Implement and utilize register structures
> >   gpio: 104-idi-48: Implement and utilize register structures
> >   gpio: 104-idio-16: Implement and utilize register structures
> >   gpio: gpio-mm: Implement and utilize register structures
> >   gpio: ws16c48: Implement and utilize register structures
> >
> >  drivers/gpio/gpio-104-dio-48e.c | 157 +++++++++++++++++++++-----------
> >  drivers/gpio/gpio-104-idi-48.c  | 128 +++++++++++++-------------
> >  drivers/gpio/gpio-104-idio-16.c |  58 ++++++++----
> >  drivers/gpio/gpio-gpio-mm.c     | 116 ++++++++++++++---------
> >  drivers/gpio/gpio-ws16c48.c     | 119 +++++++++++++++++-------
> >  5 files changed, 366 insertions(+), 212 deletions(-)
> >
> >
> > base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
> > --
> > 2.36.1
> >
> 
> Hi William,
> 
> Unlike the previous patches which were relatively simple, these seem
> like there's a lot of space for breakage (even though they're
> attempting to do a good thing). Have you tested the code on real
> hardware?
> 
> Bart

I have a tester testing the changes for to the 104-IDIO-16 and GPIO-MM
GPIO drivers on real hardware. I don't have access to the other three
devices so I've only compile tested and loaded them in a VM to verify;
I'll send out a request to ACCESIO and WinSystems to see if they have
engineers willing to test these changes for their respective devices.

I've also refactored the code in this patch series to simplify the
changes in these patches. It should make the changes much easier to
review so I'll release it in a v2 submission once the series is ready.

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]