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

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

[William Breathitt Gray]

Changes in v2:
 - Implement support for the Intel 8255 interface as a new gpio-i8255
   module; common code among gpio-104-dio-48e, gpio-104-idi-48, and
   gpio-gpio-mm are consolidated in the gpio-i8255 module
 - Refactor the gpio-104-dio-48e, gpio-104-idi-48, and gpio-gpio-mm to
   utilize the new gpio-i8255 functions; this greatly simplifies the
   changes for these drivers

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.

A new module supporting the Intel 8255 interface is introduced to
consolidate the common code found among the 104-DIO-48E, 104-IDI-48, and
GPIO-MM drivers.

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

William Breathitt Gray (6):
  gpio: i8255: Introduce the i8255 module
  gpio: 104-dio-48e: Implement and utilize register structures
  gpio: 104-idi-48: Implement and utilize register structures
  gpio: gpio-mm: Implement and utilize register structures
  gpio: 104-idio-16: Implement and utilize register structures
  gpio: ws16c48: Implement and utilize register structures

 MAINTAINERS                     |   6 +
 drivers/gpio/Kconfig            |   6 +
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-104-dio-48e.c | 224 +++++++++-------------------
 drivers/gpio/gpio-104-idi-48.c  | 123 +++++++---------
 drivers/gpio/gpio-104-idio-16.c |  58 +++++---
 drivers/gpio/gpio-gpio-mm.c     | 177 +++++------------------
 drivers/gpio/gpio-i8255.c       | 249 ++++++++++++++++++++++++++++++++
 drivers/gpio/gpio-ws16c48.c     | 119 ++++++++++-----
 include/linux/gpio/i8255.h      |  34 +++++
 10 files changed, 575 insertions(+), 422 deletions(-)
 create mode 100644 drivers/gpio/gpio-i8255.c
 create mode 100644 include/linux/gpio/i8255.h


base-commit: f2906aa863381afb0015a9eb7fefad885d4e5a56
-- 
2.36.1

[PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[William Breathitt Gray]

Exposes consumer functions providing support for Intel 8255 Programmable
Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is
introduced; modules wanting access to these functions should select this
Kconfig option.

Tested-by: Fred Eckert <Frede@cmslaser.com>
Cc: John Hentges <jhentges@accesio.com>
Cc: Jay Dolan <jay.dolan@accesio.com>
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 MAINTAINERS                |   6 +
 drivers/gpio/Kconfig       |   3 +
 drivers/gpio/Makefile      |   1 +
 drivers/gpio/gpio-i8255.c  | 249 +++++++++++++++++++++++++++++++++++++
 include/linux/gpio/i8255.h |  34 +++++
 5 files changed, 293 insertions(+)
 create mode 100644 drivers/gpio/gpio-i8255.c
 create mode 100644 include/linux/gpio/i8255.h

diff --git a/MAINTAINERS b/MAINTAINERS
index a6d3bd9d2a8d..c4ae792a8a43 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -9799,6 +9799,12 @@ L:	linux-fbdev@vger.kernel.org
 S:	Maintained
 F:	drivers/video/fbdev/i810/
 
+INTEL 8255 GPIO DRIVER
+M:	William Breathitt Gray <vilhelm.gray@gmail.com>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	drivers/gpio/gpio-i8255.c
+
 INTEL ASoC DRIVERS
 M:	Cezary Rojewski <cezary.rojewski@intel.com>
 M:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index b01961999ced..5cbe93330213 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -829,6 +829,9 @@ endmenu
 menu "Port-mapped I/O GPIO drivers"
 	depends on X86 # Unconditional I/O space access
 
+config GPIO_I8255
+	tristate
+
 config GPIO_104_DIO_48E
 	tristate "ACCES 104-DIO-48E GPIO support"
 	depends on PC104
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 14352f6dfe8e..06057e127949 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -67,6 +67,7 @@ obj-$(CONFIG_GPIO_GW_PLD)		+= gpio-gw-pld.o
 obj-$(CONFIG_GPIO_HISI)                 += gpio-hisi.o
 obj-$(CONFIG_GPIO_HLWD)			+= gpio-hlwd.o
 obj-$(CONFIG_HTC_EGPIO)			+= gpio-htc-egpio.o
+obj-$(CONFIG_GPIO_I8255)		+= gpio-i8255.o
 obj-$(CONFIG_GPIO_ICH)			+= gpio-ich.o
 obj-$(CONFIG_GPIO_IDT3243X)		+= gpio-idt3243x.o
 obj-$(CONFIG_GPIO_IOP)			+= gpio-iop.o
diff --git a/drivers/gpio/gpio-i8255.c b/drivers/gpio/gpio-i8255.c
new file mode 100644
index 000000000000..ef43312015f4
--- /dev/null
+++ b/drivers/gpio/gpio-i8255.c
@@ -0,0 +1,249 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel 8255 Programmable Peripheral Interface
+ * Copyright (C) 2022 William Breathitt Gray
+ */
+#include <linux/bitmap.h>
+#include <linux/compiler_types.h>
+#include <linux/err.h>
+#include <linux/export.h>
+#include <linux/gpio/i8255.h>
+#include <linux/io.h>
+#include <linux/module.h>
+
+#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0)
+#define I8255_CONTROL_PORTB_DIRECTION BIT(1)
+#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3)
+#define I8255_CONTROL_PORTA_DIRECTION BIT(4)
+#define I8255_CONTROL_MODE_SET BIT(7)
+#define I8255_PORTA 0
+#define I8255_PORTB 1
+#define I8255_PORTC 2
+
+static int i8255_get_port(const struct i8255 __iomem *const ppi,
+			  const unsigned long io_port, const unsigned long mask)
+{
+	const unsigned long group = io_port / 3;
+	const unsigned long ppi_port = io_port % 3;
+
+	return ioread8(&ppi[group].port[ppi_port]) & mask;
+}
+
+static u8 i8255_direction_mask(const unsigned long offset)
+{
+	const unsigned long io_port = offset / 8;
+	const unsigned long ppi_port = io_port % 3;
+
+	switch (ppi_port) {
+	case I8255_PORTA:
+		return I8255_CONTROL_PORTA_DIRECTION;
+	case I8255_PORTB:
+		return I8255_CONTROL_PORTB_DIRECTION;
+	case I8255_PORTC:
+		/* Port C can be configured by nibble */
+		if (offset % 8 > 3)
+			return I8255_CONTROL_PORTCUPPER_DIRECTION;
+		return I8255_CONTROL_PORTCLOWER_DIRECTION;
+	default:
+		/* Should never reach this path */
+		return 0;
+	}
+}
+
+static void i8255_set_port(struct i8255 __iomem *const ppi,
+			   const unsigned long io_port,
+			   const unsigned long io_mask,
+			   const unsigned long bit_mask)
+{
+	const unsigned long group = io_port / 3;
+	const unsigned long ppi_port = io_port % 3;
+	unsigned long out_state;
+
+	out_state = ioread8(&ppi[group].port[ppi_port]);
+	out_state &= ~io_mask;
+	out_state |= bit_mask;
+
+	iowrite8(out_state, &ppi[group].port[ppi_port]);
+}
+
+/**
+ * i8255_direction_input - configure signal offset as input
+ * @ppi:		Intel 8255 Programmable Peripheral Interface groups
+ * @control_state:	control register states of the respective PPI groups
+ * @offset:		signal offset to configure as input
+ *
+ * Configures a signal @offset as input for the respective Intel 8255
+ * Programmable Peripheral Interface (@ppi) groups. The @control_state values
+ * are updated to reflect the new configuration.
+ */
+void i8255_direction_input(struct i8255 __iomem *const ppi,
+			   u8 *const control_state, const unsigned long offset)
+{
+	const unsigned long io_port = offset / 8;
+	const unsigned long group = io_port / 3;
+
+	control_state[group] |= I8255_CONTROL_MODE_SET;
+	control_state[group] |= i8255_direction_mask(offset);
+
+	iowrite8(control_state[group], &ppi[group].control);
+}
+EXPORT_SYMBOL_GPL(i8255_direction_input);
+
+/**
+ * i8255_direction_output - configure signal offset as output
+ * @control_state:	control register states of the respective PPI groups
+ * @ppi:		Intel 8255 Programmable Peripheral Interface groups
+ * @offset:		signal offset to configure as output
+ * @value:		signal value to output
+ *
+ * Configures a signal @offset as output for the respective Intel 8255
+ * Programmable Peripheral Interface (@ppi) groups and sets the respective
+ * signal output to the desired @value. The @control_state values are updated to
+ * reflect the new configuration.
+ */
+void i8255_direction_output(struct i8255 __iomem *const ppi,
+			    u8 *const control_state, const unsigned long offset,
+			    const unsigned long value)
+{
+	const unsigned long io_port = offset / 8;
+	const unsigned long group = io_port / 3;
+
+	control_state[group] |= I8255_CONTROL_MODE_SET;
+	control_state[group] &= ~i8255_direction_mask(offset);
+
+	iowrite8(control_state[group], &ppi[group].control);
+	i8255_set(ppi, offset, value);
+}
+EXPORT_SYMBOL_GPL(i8255_direction_output);
+
+/**
+ * i8255_get - get signal value at signal offset
+ * @ppi:		Intel 8255 Programmable Peripheral Interface groups
+ * @offset:		offset of signal to get
+ *
+ * Returns the signal value (0=low, 1=high) for the signal at @offset for the
+ * respective Intel 8255 Programmable Peripheral Interface (@ppi) groups.
+ */
+int i8255_get(const struct i8255 __iomem *const ppi, const unsigned long offset)
+{
+	const unsigned long io_port = offset / 8;
+	const unsigned long offset_mask = BIT(offset % 8);
+
+	return !!i8255_get_port(ppi, io_port, offset_mask);
+}
+EXPORT_SYMBOL_GPL(i8255_get);
+
+/**
+ * i8255_get_direction - get the I/O direction for a signal offset
+ * @control_state:	control register states of the respective PPI groups
+ * @offset:		offset of signal to get direction
+ *
+ * Returns the signal direction (0=output, 1=input) for the signal at @offset.
+ * groups.
+ */
+int i8255_get_direction(const u8 *const control_state,
+			const unsigned long offset)
+{
+	const unsigned long io_port = offset / 8;
+	const unsigned long group = io_port / 3;
+
+	return !!(control_state[group] & i8255_direction_mask(offset));
+}
+EXPORT_SYMBOL_GPL(i8255_get_direction);
+
+/**
+ * i8255_get_multiple - get multiple signal values at multiple signal offsets
+ * @ppi:		Intel 8255 Programmable Peripheral Interface groups
+ * @mask:		mask of signals to get
+ * @bits:		bitmap to store signal values
+ * @ngpio:		number of GPIO signals of the respective PPI groups
+ *
+ * Stores in @bits the values (0=low, 1=high) for the signals defined by @mask
+ * for the respective Intel 8255 Programmable Peripheral Interface (@ppi)
+ * groups.
+ */
+void i8255_get_multiple(const struct i8255 __iomem *const ppi,
+			const unsigned long *const mask,
+			unsigned long *const bits, const unsigned long ngpio)
+{
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned long io_port;
+	unsigned long port_state;
+
+	bitmap_zero(bits, ngpio);
+
+	for_each_set_clump8(offset, gpio_mask, mask, ngpio) {
+		io_port = offset / 8;
+		port_state = i8255_get_port(ppi, io_port, gpio_mask);
+
+		bitmap_set_value8(bits, port_state, offset);
+	}
+}
+EXPORT_SYMBOL_GPL(i8255_get_multiple);
+
+/**
+ * i8255_mode0_output - configure all PPI ports to MODE 0 output mode
+ * @ppi:		Intel 8255 Programmable Peripheral Interface group
+ *
+ * Configures all Intel 8255 Programmable Peripheral Interface (@ppi) ports to
+ * MODE 0 (Basic Input/Output) output mode.
+ */
+void i8255_mode0_output(struct i8255 __iomem *const ppi)
+{
+	iowrite8(I8255_CONTROL_MODE_SET, &ppi->control);
+}
+EXPORT_SYMBOL_GPL(i8255_mode0_output);
+
+/**
+ * i8255_set - set signal value at signal offset
+ * @ppi:		Intel 8255 Programmable Peripheral Interface groups
+ * @offset:		offset of signal to set
+ * @value:		value of signal to set
+ *
+ * Assigns output @value for the signal at @offset for the respective Intel 8255
+ * Programmable Peripheral Interface (@ppi) groups.
+ */
+void i8255_set(struct i8255 __iomem *const ppi, const unsigned long offset,
+	       const unsigned long value)
+{
+	const unsigned long io_port = offset / 8;
+	const unsigned long port_offset = offset % 8;
+	const unsigned long offset_mask = BIT(port_offset);
+	const unsigned long bit_mask = value << port_offset;
+
+	i8255_set_port(ppi, io_port, offset_mask, bit_mask);
+}
+EXPORT_SYMBOL_GPL(i8255_set);
+
+/**
+ * i8255_set_multiple - set signal values at multiple signal offsets
+ * @ppi:		Intel 8255 Programmable Peripheral Interface groups
+ * @mask:		mask of signals to set
+ * @bits:		bitmap of signal output values
+ * @ngpio:		number of GPIO signals of the respective PPI groups
+ *
+ * Assigns output values defined by @bits for the signals defined by @mask for
+ * the respective Intel 8255 Programmable Peripheral Interface (@ppi) groups.
+ */
+void i8255_set_multiple(struct i8255 __iomem *const ppi,
+			const unsigned long *const mask,
+			const unsigned long *const bits,
+			const unsigned long ngpio)
+{
+	unsigned long offset;
+	unsigned long gpio_mask;
+	unsigned long bit_mask;
+	unsigned long io_port;
+
+	for_each_set_clump8(offset, gpio_mask, mask, ngpio) {
+		bit_mask = bitmap_get_value8(bits, offset) & gpio_mask;
+		io_port = offset / 8;
+		i8255_set_port(ppi, io_port, gpio_mask, bit_mask);
+	}
+}
+EXPORT_SYMBOL_GPL(i8255_set_multiple);
+
+MODULE_AUTHOR("William Breathitt Gray");
+MODULE_DESCRIPTION("Intel 8255 Programmable Peripheral Interface");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/gpio/i8255.h b/include/linux/gpio/i8255.h
new file mode 100644
index 000000000000..7ddcf7fcd1dd
--- /dev/null
+++ b/include/linux/gpio/i8255.h
@@ -0,0 +1,34 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2022 William Breathitt Gray */
+#ifndef _I8255_H_
+#define _I8255_H_
+
+#include <linux/compiler_types.h>
+#include <linux/types.h>
+
+/**
+ * struct i8255 - Intel 8255 register structure
+ * @port:	Port A, B, and C
+ * @control:	Control register
+ */
+struct i8255 {
+	u8 port[3];
+	u8 control;
+};
+
+void i8255_direction_input(struct i8255 __iomem *ppi, u8 *control_state,
+			   unsigned long offset);
+void i8255_direction_output(struct i8255 __iomem *ppi, u8 *control_state,
+			    unsigned long offset, unsigned long value);
+int i8255_get(const struct i8255 __iomem *ppi, unsigned long offset);
+int i8255_get_direction(const u8 *control_state, unsigned long offset);
+void i8255_get_multiple(const struct i8255 __iomem *ppi,
+			const unsigned long *mask, unsigned long *bits,
+			unsigned long ngpio);
+void i8255_mode0_output(struct i8255 __iomem *const ppi);
+void i8255_set(struct i8255 __iomem *ppi, unsigned long offset,
+	       unsigned long value);
+void i8255_set_multiple(struct i8255 __iomem *ppi, const unsigned long *mask,
+			const unsigned long *bits, unsigned long ngpio);
+
+#endif /* _I8255_H_ */
-- 
2.36.1

[PATCH v2 2/6] 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. The 104-DIO-48E device
features an Intel 8255 compatible GPIO interface, so the i8255 GPIO
module is selected and utilized as well.

Cc: John Hentges <jhentges@accesio.com>
Cc: Jay Dolan <jay.dolan@accesio.com>
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/Kconfig            |   1 +
 drivers/gpio/gpio-104-dio-48e.c | 224 ++++++++++----------------------
 2 files changed, 69 insertions(+), 156 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 5cbe93330213..31546969f66a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -837,6 +837,7 @@ config GPIO_104_DIO_48E
 	depends on PC104
 	select ISA_BUS_API
 	select GPIOLIB_IRQCHIP
+	select GPIO_I8255
 	help
 	  Enables GPIO support for the ACCES 104-DIO-48E series (104-DIO-48E,
 	  104-DIO-24E). The base port addresses for the devices may be
diff --git a/drivers/gpio/gpio-104-dio-48e.c b/drivers/gpio/gpio-104-dio-48e.c
index f118ad9bcd33..dab1fb44f295 100644
--- a/drivers/gpio/gpio-104-dio-48e.c
+++ b/drivers/gpio/gpio-104-dio-48e.c
@@ -6,11 +6,12 @@
  * This driver supports the following ACCES devices: 104-DIO-48E and
  * 104-DIO-24E.
  */
-#include <linux/bitmap.h>
+#include <linux/bits.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/i8255.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
@@ -20,6 +21,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,34 +35,52 @@ 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 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 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
- * @io_state:	bit I/O state (whether bit is set to input or output)
- * @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
- * @irq_mask:	I/O bits affected by interrupts
+ * @chip:		instance of the gpio_chip
+ * @control_state:	Control registers state
+ * @lock:		synchronization lock to prevent I/O race conditions
+ * @reg:		I/O address offset for the device registers
+ * @irq_mask:		I/O bits affected by interrupts
  */
 struct dio48e_gpio {
 	struct gpio_chip chip;
-	unsigned char io_state[6];
-	unsigned char out_state[6];
-	unsigned char control[2];
+	u8 control_state[2];
 	raw_spinlock_t lock;
-	void __iomem *base;
+	struct dio48e_reg __iomem *reg;
 	unsigned char irq_mask;
 };
 
 static int dio48e_gpio_get_direction(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);
 
-	if (dio48egpio->io_state[port] & mask)
-		return  GPIO_LINE_DIRECTION_IN;
+	if (i8255_get_direction(dio48egpio->control_state, offset))
+		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;
 }
@@ -68,36 +88,12 @@ static int dio48e_gpio_get_direction(struct gpio_chip *chip, unsigned int offset
 static int dio48e_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
 	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;
 	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) {
-		/* Port C can be configured by nibble */
-		if (offset % 8 > 3) {
-			dio48egpio->io_state[io_port] |= 0xF0;
-			dio48egpio->control[control_port] |= BIT(3);
-		} else {
-			dio48egpio->io_state[io_port] |= 0x0F;
-			dio48egpio->control[control_port] |= BIT(0);
-		}
-	} else {
-		dio48egpio->io_state[io_port] |= 0xFF;
-		if (io_port == 0 || io_port == 3)
-			dio48egpio->control[control_port] |= BIT(4);
-		else
-			dio48egpio->control[control_port] |= BIT(1);
-	}
-
-	control = BIT(7) | dio48egpio->control[control_port];
-	iowrite8(control, control_addr);
-	control &= ~BIT(7);
-	iowrite8(control, control_addr);
+	i8255_direction_input(dio48egpio->reg->ppi, dio48egpio->control_state,
+			      offset);
 
 	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 
@@ -108,46 +104,12 @@ static int dio48e_gpio_direction_output(struct gpio_chip *chip, unsigned int off
 					int value)
 {
 	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 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;
 	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) {
-		/* Port C can be configured by nibble */
-		if (offset % 8 > 3) {
-			dio48egpio->io_state[io_port] &= 0x0F;
-			dio48egpio->control[control_port] &= ~BIT(3);
-		} else {
-			dio48egpio->io_state[io_port] &= 0xF0;
-			dio48egpio->control[control_port] &= ~BIT(0);
-		}
-	} else {
-		dio48egpio->io_state[io_port] &= 0x00;
-		if (io_port == 0 || io_port == 3)
-			dio48egpio->control[control_port] &= ~BIT(4);
-		else
-			dio48egpio->control[control_port] &= ~BIT(1);
-	}
-
-	if (value)
-		dio48egpio->out_state[io_port] |= mask;
-	else
-		dio48egpio->out_state[io_port] &= ~mask;
-
-	control = BIT(7) | dio48egpio->control[control_port];
-	iowrite8(control, control_addr);
-
-	iowrite8(dio48egpio->out_state[io_port], dio48egpio->base + out_port);
-
-	control &= ~BIT(7);
-	iowrite8(control, control_addr);
+	i8255_direction_output(dio48egpio->reg->ppi, dio48egpio->control_state,
+			       offset, value);
 
 	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 
@@ -157,47 +119,16 @@ static int dio48e_gpio_direction_output(struct gpio_chip *chip, unsigned int off
 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;
-	unsigned long flags;
-	unsigned int port_state;
-
-	raw_spin_lock_irqsave(&dio48egpio->lock, flags);
-
-	/* ensure that GPIO is set for input */
-	if (!(dio48egpio->io_state[port] & mask)) {
-		raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
-		return -EINVAL;
-	}
 
-	port_state = ioread8(dio48egpio->base + in_port);
-
-	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
-
-	return !!(port_state & mask);
+	return i8255_get(dio48egpio->reg->ppi, offset);
 }
 
-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_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];
-		port_state = ioread8(port_addr) & gpio_mask;
-
-		bitmap_set_value8(bits, port_state, offset);
-	}
+	i8255_get_multiple(dio48egpio->reg->ppi, mask, bits, chip->ngpio);
 
 	return 0;
 }
@@ -205,19 +136,11 @@ static int dio48e_gpio_get_multiple(struct gpio_chip *chip, unsigned long *mask,
 static void dio48e_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
 {
 	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;
 	unsigned long flags;
 
 	raw_spin_lock_irqsave(&dio48egpio->lock, flags);
 
-	if (value)
-		dio48egpio->out_state[port] |= mask;
-	else
-		dio48egpio->out_state[port] &= ~mask;
-
-	iowrite8(dio48egpio->out_state[port], dio48egpio->base + out_port);
+	i8255_set(dio48egpio->reg->ppi, offset, value);
 
 	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 }
@@ -226,28 +149,13 @@ static void dio48e_gpio_set_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;
-	size_t index;
-	void __iomem *port_addr;
-	unsigned long bitmask;
 	unsigned long flags;
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
-		index = offset / 8;
-		port_addr = dio48egpio->base + ports[index];
-
-		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
-
-		raw_spin_lock_irqsave(&dio48egpio->lock, flags);
+	raw_spin_lock_irqsave(&dio48egpio->lock, flags);
 
-		/* update output state data and set device gpio register */
-		dio48egpio->out_state[index] &= ~gpio_mask;
-		dio48egpio->out_state[index] |= bitmask;
-		iowrite8(dio48egpio->out_state[index], port_addr);
+	i8255_set_multiple(dio48egpio->reg->ppi, mask, bits, chip->ngpio);
 
-		raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
-	}
+	raw_spin_unlock_irqrestore(&dio48egpio->lock, flags);
 }
 
 static void dio48e_irq_ack(struct irq_data *data)
@@ -274,7 +182,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 +202,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 +249,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 +281,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 __iomem *const ppi)
+{
+	const unsigned long ngpio = 24;
+	const unsigned long mask = GENMASK(ngpio - 1, 0);
+	const unsigned long bits = 0;
+	unsigned long i;
+
+	/* Initialize all GPIO to output 0 */
+	for (i = 0; i < 2; i++) {
+		i8255_mode0_output(&ppi[i]);
+		i8255_set_multiple(&ppi[i], &mask, &bits, ngpio);
+	}
+}
+
 static int dio48e_probe(struct device *dev, unsigned int id)
 {
 	struct dio48e_gpio *dio48egpio;
@@ -395,8 +317,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;
@@ -425,17 +347,7 @@ 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);
 
 	err = devm_gpiochip_add_data(dev, &dio48egpio->chip, dio48egpio);
 	if (err) {
-- 
2.36.1

[PATCH v2 3/6] 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. The 104-IDI-48 device features
an Intel 8255 compatible GPIO interface, so the i8255 GPIO module is
selected and utilized as well.

Cc: John Hentges <jhentges@accesio.com>
Cc: Jay Dolan <jay.dolan@accesio.com>
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/Kconfig           |   1 +
 drivers/gpio/gpio-104-idi-48.c | 123 +++++++++++++--------------------
 2 files changed, 50 insertions(+), 74 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 31546969f66a..0ae818d548bb 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -861,6 +861,7 @@ config GPIO_104_IDI_48
 	depends on PC104
 	select ISA_BUS_API
 	select GPIOLIB_IRQCHIP
+	select GPIO_I8255
 	help
 	  Enables GPIO support for the ACCES 104-IDI-48 family (104-IDI-48A,
 	  104-IDI-48AC, 104-IDI-48B, 104-IDI-48BC). The base port addresses for
diff --git a/drivers/gpio/gpio-104-idi-48.c b/drivers/gpio/gpio-104-idi-48.c
index 9521ece3ebef..7c8ae95237e6 100644
--- a/drivers/gpio/gpio-104-idi-48.c
+++ b/drivers/gpio/gpio-104-idi-48.c
@@ -6,11 +6,11 @@
  * This driver supports the following ACCES devices: 104-IDI-48A,
  * 104-IDI-48AC, 104-IDI-48B, and 104-IDI-48BC.
  */
-#include <linux/bitmap.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/i8255.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/interrupt.h>
@@ -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,42 +80,18 @@ 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);
+	const void __iomem *const ppi = idi48gpio->reg;
 
-			return !!(ioread8(port_addr) & mask);
-		}
-
-	/* The following line should never execute since offset < 48 */
-	return 0;
+	return i8255_get(ppi, offset);
 }
 
 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);
-	unsigned long offset;
-	unsigned long gpio_mask;
-	static const size_t ports[] = { 0, 1, 2, 4, 5, 6 };
-	void __iomem *port_addr;
-	unsigned long port_state;
-
-	/* clear bits array to a clean slate */
-	bitmap_zero(bits, chip->ngpio);
+	const void __iomem *const ppi = idi48gpio->reg;
 
-	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;
-
-		bitmap_set_value8(bits, port_state, offset);
-	}
+	i8255_get_multiple(ppi, mask, bits, chip->ngpio);
 
 	return 0;
 }
@@ -113,30 +105,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 +127,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 +179,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 +225,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 +248,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 v2 4/6] 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. The GPIO-MM device features an
Intel 8255 compatible GPIO interface, so the i8255 GPIO module is
selected and utilized as well.

Tested-by: Fred Eckert <Frede@cmslaser.com>
Cc: John Hentges <jhentges@accesio.com>
Cc: Jay Dolan <jay.dolan@accesio.com>
Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/Kconfig        |   1 +
 drivers/gpio/gpio-gpio-mm.c | 177 ++++++++----------------------------
 2 files changed, 38 insertions(+), 140 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 0ae818d548bb..20b118b3dd8a 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -882,6 +882,7 @@ config GPIO_GPIO_MM
 	tristate "Diamond Systems GPIO-MM GPIO support"
 	depends on PC104
 	select ISA_BUS_API
+	select GPIO_I8255
 	help
 	  Enables GPIO support for the Diamond Systems GPIO-MM and GPIO-MM-12.
 
diff --git a/drivers/gpio/gpio-gpio-mm.c b/drivers/gpio/gpio-gpio-mm.c
index 097a06463d01..4bbf4e03f436 100644
--- a/drivers/gpio/gpio-gpio-mm.c
+++ b/drivers/gpio/gpio-gpio-mm.c
@@ -6,11 +6,12 @@
  * This driver supports the following Diamond Systems devices: GPIO-MM and
  * GPIO-MM-12.
  */
-#include <linux/bitmap.h>
+#include <linux/bits.h>
 #include <linux/bitops.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
+#include <linux/gpio/i8255.h>
 #include <linux/io.h>
 #include <linux/ioport.h>
 #include <linux/isa.h>
@@ -18,6 +19,7 @@
 #include <linux/module.h>
 #include <linux/moduleparam.h>
 #include <linux/spinlock.h>
+#include <linux/types.h>
 
 #define GPIOMM_EXTENT 8
 #define MAX_NUM_GPIOMM max_num_isa_dev(GPIOMM_EXTENT)
@@ -29,30 +31,24 @@ MODULE_PARM_DESC(base, "Diamond Systems GPIO-MM base addresses");
 
 /**
  * struct gpiomm_gpio - GPIO device private data structure
- * @chip:	instance of the gpio_chip
- * @io_state:	bit I/O state (whether bit is set to input or output)
- * @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
+ * @chip:		instance of the gpio_chip
+ * @control_state:	Control registers state
+ * @lock:		synchronization lock to prevent I/O race conditions
+ * @ppi:		Programmable Peripheral Interface groups
  */
 struct gpiomm_gpio {
 	struct gpio_chip chip;
-	unsigned char io_state[6];
-	unsigned char out_state[6];
-	unsigned char control[2];
+	u8 control_state[2];
 	spinlock_t lock;
-	void __iomem *base;
+	struct i8255 __iomem *ppi;
 };
 
 static int gpiomm_gpio_get_direction(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);
 
-	if (gpiommgpio->io_state[port] & mask)
+	if (i8255_get_direction(gpiommgpio->control_state, offset))
 		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;
@@ -62,33 +58,12 @@ static int gpiomm_gpio_direction_input(struct gpio_chip *chip,
 	unsigned int offset)
 {
 	struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-	const unsigned int io_port = offset / 8;
-	const unsigned int control_port = io_port / 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) {
-		/* Port C can be configured by nibble */
-		if (offset % 8 > 3) {
-			gpiommgpio->io_state[io_port] |= 0xF0;
-			gpiommgpio->control[control_port] |= BIT(3);
-		} else {
-			gpiommgpio->io_state[io_port] |= 0x0F;
-			gpiommgpio->control[control_port] |= BIT(0);
-		}
-	} else {
-		gpiommgpio->io_state[io_port] |= 0xFF;
-		if (io_port == 0 || io_port == 3)
-			gpiommgpio->control[control_port] |= BIT(4);
-		else
-			gpiommgpio->control[control_port] |= BIT(1);
-	}
-
-	control = BIT(7) | gpiommgpio->control[control_port];
-	iowrite8(control, gpiommgpio->base + 3 + control_port*4);
+	i8255_direction_input(gpiommgpio->ppi, gpiommgpio->control_state,
+			      offset);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 
@@ -99,42 +74,12 @@ static int gpiomm_gpio_direction_output(struct gpio_chip *chip,
 	unsigned int offset, int value)
 {
 	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 int mask = BIT(offset % 8);
-	const unsigned int out_port = (io_port > 2) ? io_port + 1 : io_port;
 	unsigned long flags;
-	unsigned int control;
 
 	spin_lock_irqsave(&gpiommgpio->lock, flags);
 
-	/* Check if configuring Port C */
-	if (io_port == 2 || io_port == 5) {
-		/* Port C can be configured by nibble */
-		if (offset % 8 > 3) {
-			gpiommgpio->io_state[io_port] &= 0x0F;
-			gpiommgpio->control[control_port] &= ~BIT(3);
-		} else {
-			gpiommgpio->io_state[io_port] &= 0xF0;
-			gpiommgpio->control[control_port] &= ~BIT(0);
-		}
-	} else {
-		gpiommgpio->io_state[io_port] &= 0x00;
-		if (io_port == 0 || io_port == 3)
-			gpiommgpio->control[control_port] &= ~BIT(4);
-		else
-			gpiommgpio->control[control_port] &= ~BIT(1);
-	}
-
-	if (value)
-		gpiommgpio->out_state[io_port] |= mask;
-	else
-		gpiommgpio->out_state[io_port] &= ~mask;
-
-	control = BIT(7) | gpiommgpio->control[control_port];
-	iowrite8(control, gpiommgpio->base + 3 + control_port*4);
-
-	iowrite8(gpiommgpio->out_state[io_port], gpiommgpio->base + out_port);
+	i8255_direction_output(gpiommgpio->ppi, gpiommgpio->control_state,
+			       offset, value);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 
@@ -144,47 +89,16 @@ static int gpiomm_gpio_direction_output(struct gpio_chip *chip,
 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;
-	unsigned long flags;
-	unsigned int port_state;
-
-	spin_lock_irqsave(&gpiommgpio->lock, flags);
-
-	/* ensure that GPIO is set for input */
-	if (!(gpiommgpio->io_state[port] & mask)) {
-		spin_unlock_irqrestore(&gpiommgpio->lock, flags);
-		return -EINVAL;
-	}
-
-	port_state = ioread8(gpiommgpio->base + in_port);
-
-	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 
-	return !!(port_state & mask);
+	return i8255_get(gpiommgpio->ppi, offset);
 }
 
-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_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];
-		port_state = ioread8(port_addr) & gpio_mask;
-
-		bitmap_set_value8(bits, port_state, offset);
-	}
+	i8255_get_multiple(gpiommgpio->ppi, mask, bits, chip->ngpio);
 
 	return 0;
 }
@@ -193,19 +107,11 @@ static void gpiomm_gpio_set(struct gpio_chip *chip, unsigned int offset,
 	int value)
 {
 	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;
 	unsigned long flags;
 
 	spin_lock_irqsave(&gpiommgpio->lock, flags);
 
-	if (value)
-		gpiommgpio->out_state[port] |= mask;
-	else
-		gpiommgpio->out_state[port] &= ~mask;
-
-	iowrite8(gpiommgpio->out_state[port], gpiommgpio->base + out_port);
+	i8255_set(gpiommgpio->ppi, offset, value);
 
 	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 }
@@ -214,28 +120,13 @@ static void gpiomm_gpio_set_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;
-	size_t index;
-	void __iomem *port_addr;
-	unsigned long bitmask;
 	unsigned long flags;
 
-	for_each_set_clump8(offset, gpio_mask, mask, ARRAY_SIZE(ports) * 8) {
-		index = offset / 8;
-		port_addr = gpiommgpio->base + ports[index];
-
-		bitmask = bitmap_get_value8(bits, offset) & gpio_mask;
-
-		spin_lock_irqsave(&gpiommgpio->lock, flags);
+	spin_lock_irqsave(&gpiommgpio->lock, flags);
 
-		/* update output state data and set device gpio register */
-		gpiommgpio->out_state[index] &= ~gpio_mask;
-		gpiommgpio->out_state[index] |= bitmask;
-		iowrite8(gpiommgpio->out_state[index], port_addr);
+	i8255_set_multiple(gpiommgpio->ppi, mask, bits, chip->ngpio);
 
-		spin_unlock_irqrestore(&gpiommgpio->lock, flags);
-	}
+	spin_unlock_irqrestore(&gpiommgpio->lock, flags);
 }
 
 #define GPIOMM_NGPIO 48
@@ -250,6 +141,20 @@ 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 __iomem *const ppi)
+{
+	const unsigned long ngpio = 24;
+	const unsigned long mask = GENMASK(ngpio - 1, 0);
+	const unsigned long bits = 0;
+	unsigned long i;
+
+	/* Initialize all GPIO to output 0 */
+	for (i = 0; i < 2; i++) {
+		i8255_mode0_output(&ppi[i]);
+		i8255_set_multiple(&ppi[i], &mask, &bits, ngpio);
+	}
+}
+
 static int gpiomm_probe(struct device *dev, unsigned int id)
 {
 	struct gpiomm_gpio *gpiommgpio;
@@ -266,8 +171,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->ppi = devm_ioport_map(dev, base[id], GPIOMM_EXTENT);
+	if (!gpiommgpio->ppi)
 		return -ENOMEM;
 
 	gpiommgpio->chip.label = name;
@@ -292,15 +197,7 @@ static int gpiomm_probe(struct device *dev, unsigned int id)
 		return err;
 	}
 
-	/* 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->ppi);
 
 	return 0;
 }
-- 
2.36.1

[PATCH v2 5/6] 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.

Tested-by: Fred Eckert <Frede@cmslaser.com>
Cc: John Hentges <jhentges@accesio.com>
Cc: Jay Dolan <jay.dolan@accesio.com>
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 v2 6/6] 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.

Cc: Paul Demetrotion <pdemetrotion@winsystems.com>
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 v2 1/6] gpio: i8255: Introduce the i8255 module

[Andy Shevchenko]

On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray
<william.gray@linaro.org> wrote:
>
> Exposes consumer functions providing support for Intel 8255 Programmable
> Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is
> introduced; modules wanting access to these functions should select this
> Kconfig option.

Spent much time with these chips in my teenage times :-)

Very good written library, see my comments below.

...

> +#include <linux/compiler_types.h>

Should be simple types.h as you are using u8, etc.

> +#include <linux/err.h>
> +#include <linux/export.h>

> +#include <linux/gpio/i8255.h>

gpio/driver.h ?

And since it belongs to GPIO, I would group them and move after all
other include/* to emphasize the relationship.

Also, why is it in the global header folder? Do you expect some
drivers outside of drivers/gpio/? Maybe we can move after when the
user comes?

> +#include <linux/io.h>
> +#include <linux/module.h>

...

> +#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0)
> +#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3)

Missed underscore.

...

> +static u8 i8255_direction_mask(const unsigned long offset)
> +{
> +       const unsigned long io_port = offset / 8;
> +       const unsigned long ppi_port = io_port % 3;
> +
> +       switch (ppi_port) {
> +       case I8255_PORTA:
> +               return I8255_CONTROL_PORTA_DIRECTION;
> +       case I8255_PORTB:
> +               return I8255_CONTROL_PORTB_DIRECTION;
> +       case I8255_PORTC:
> +               /* Port C can be configured by nibble */

> +               if (offset % 8 > 3)

I would move it to the local definition block close to offset/8. On
some architectures this may give one assembly instruction for two
variables.

> +                       return I8255_CONTROL_PORTCUPPER_DIRECTION;
> +               return I8255_CONTROL_PORTCLOWER_DIRECTION;
> +       default:
> +               /* Should never reach this path */
> +               return 0;
> +       }
> +}

> +void i8255_direction_input(struct i8255 __iomem *const ppi,
> +                          u8 *const control_state, const unsigned long offset)
> +{
> +       const unsigned long io_port = offset / 8;
> +       const unsigned long group = io_port / 3;
> +
> +       control_state[group] |= I8255_CONTROL_MODE_SET;
> +       control_state[group] |= i8255_direction_mask(offset);
> +
> +       iowrite8(control_state[group], &ppi[group].control);

No I/O serialization? Can this be accessed during interrupt? (It may
be that the code is correct, but please go through it and check with a
question "can this register be accessed during IRQ and if yes, will
the user get the correct / meaningful data?")

> +}
> +EXPORT_SYMBOL_GPL(i8255_direction_input);

Make it with a namespace. Ditto for the rest.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[Linus Walleij]

On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray
<william.gray@linaro.org> wrote:

> Exposes consumer functions providing support for Intel 8255 Programmable
> Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is
> introduced; modules wanting access to these functions should select this
> Kconfig option.
>
> Tested-by: Fred Eckert <Frede@cmslaser.com>
> Cc: John Hentges <jhentges@accesio.com>
> Cc: Jay Dolan <jay.dolan@accesio.com>
> Signed-off-by: William Breathitt Gray <william.gray@linaro.org>

This chip is like 50 years old, but so am I and I am not obsolete, it's about
time that we implement a proper driver for it!

But I suppose you are not really using the actual discrete i8255 component?
This is certainly used as integrated into some bridge or so? (Should be
mentioned in the commit.)

> +config GPIO_I8255
> +       tristate

That's a bit terse :D Explain that this is a Intel 8255 PPI chip first developed
in the first half of the 1970ies.

> +++ b/include/linux/gpio/i8255.h

You need to provide a rationale for the separate .h file in the commit
message even if it is clear
how it is used in the following patches.

Yours,
Linus Walleij

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[William Breathitt Gray]

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

On Fri, Jul 08, 2022 at 04:40:01PM +0200, Andy Shevchenko wrote:
> On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray
> <william.gray@linaro.org> wrote:
> >
> > Exposes consumer functions providing support for Intel 8255 Programmable
> > Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is
> > introduced; modules wanting access to these functions should select this
> > Kconfig option.
> 
> Spent much time with these chips in my teenage times :-)
> 
> Very good written library, see my comments below.
> 
> ...
> 
> > +#include <linux/compiler_types.h>
> 
> Should be simple types.h as you are using u8, etc.

Ack.

> > +#include <linux/err.h>
> > +#include <linux/export.h>
> 
> > +#include <linux/gpio/i8255.h>
> 
> gpio/driver.h ?
> 
> And since it belongs to GPIO, I would group them and move after all
> other include/* to emphasize the relationship.
> 
> Also, why is it in the global header folder? Do you expect some
> drivers outside of drivers/gpio/? Maybe we can move after when the
> user comes?

I think gpio/driver.h does make more sense for now since all the users
of library are located under drivers/gpio/. I'll move the header code
into gpio/driver.h then and adjust the includes accordingly.

> > +#include <linux/io.h>
> > +#include <linux/module.h>
> 
> ...
> 
> > +#define I8255_CONTROL_PORTCLOWER_DIRECTION BIT(0)
> > +#define I8255_CONTROL_PORTCUPPER_DIRECTION BIT(3)
> 
> Missed underscore.

Ack.

> ...
> 
> > +static u8 i8255_direction_mask(const unsigned long offset)
> > +{
> > +       const unsigned long io_port = offset / 8;
> > +       const unsigned long ppi_port = io_port % 3;
> > +
> > +       switch (ppi_port) {
> > +       case I8255_PORTA:
> > +               return I8255_CONTROL_PORTA_DIRECTION;
> > +       case I8255_PORTB:
> > +               return I8255_CONTROL_PORTB_DIRECTION;
> > +       case I8255_PORTC:
> > +               /* Port C can be configured by nibble */
> 
> > +               if (offset % 8 > 3)
> 
> I would move it to the local definition block close to offset/8. On
> some architectures this may give one assembly instruction for two
> variables.

Ack.

> > +                       return I8255_CONTROL_PORTCUPPER_DIRECTION;
> > +               return I8255_CONTROL_PORTCLOWER_DIRECTION;
> > +       default:
> > +               /* Should never reach this path */
> > +               return 0;
> > +       }
> > +}
> 
> > +void i8255_direction_input(struct i8255 __iomem *const ppi,
> > +                          u8 *const control_state, const unsigned long offset)
> > +{
> > +       const unsigned long io_port = offset / 8;
> > +       const unsigned long group = io_port / 3;
> > +
> > +       control_state[group] |= I8255_CONTROL_MODE_SET;
> > +       control_state[group] |= i8255_direction_mask(offset);
> > +
> > +       iowrite8(control_state[group], &ppi[group].control);
> 
> No I/O serialization? Can this be accessed during interrupt? (It may
> be that the code is correct, but please go through it and check with a
> question "can this register be accessed during IRQ and if yes, will
> the user get the correct / meaningful data?")

Writing to the 8255 control register for the device shouldn't be a
problem, but we do have a race condition with the control_state[group]
value. This value is accessed and modified in other functions (e.g. the
i8255_direction_output() right below) so if an interrupt occurs the
value could end up clobbered before it's written.

I'm not sure what the best approach would be here. In the subsequent
patches I have the GPIO drivers take a lock before calling these i8255_*
functions in order to synchronize access to those state arrays. Do you
think it would be better to move the sychronization lock acquisition
internally to the i8255_* functions here?

> > +}
> > +EXPORT_SYMBOL_GPL(i8255_direction_input);
> 
> Make it with a namespace. Ditto for the rest.

Ack.

William Breathitt Gray

> -- 
> With Best Regards,
> Andy Shevchenko

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

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[William Breathitt Gray]

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

On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:
> On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray
> <william.gray@linaro.org> wrote:
> 
> > Exposes consumer functions providing support for Intel 8255 Programmable
> > Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is
> > introduced; modules wanting access to these functions should select this
> > Kconfig option.
> >
> > Tested-by: Fred Eckert <Frede@cmslaser.com>
> > Cc: John Hentges <jhentges@accesio.com>
> > Cc: Jay Dolan <jay.dolan@accesio.com>
> > Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
> 
> This chip is like 50 years old, but so am I and I am not obsolete, it's about
> time that we implement a proper driver for it!
> 
> But I suppose you are not really using the actual discrete i8255 component?
> This is certainly used as integrated into some bridge or so? (Should be
> mentioned in the commit.)

Interestingly, there are some PC/104 devices out there that use actual
i8255 components (e.g. Diamond Systems Onyx-MM with its 82C55 chips),
but honestly the majority of devices I come across are simply emulating
the i8255 interface in an FPGA or similar.

I'll adjust the commit to make it clearer that this is a library for
i8255-compatible interfaces rather than support for any physical Intel
8255 chip in particular.

> > +config GPIO_I8255
> > +       tristate
> 
> That's a bit terse :D Explain that this is a Intel 8255 PPI chip first developed
> in the first half of the 1970ies.

Ack.

> > +++ b/include/linux/gpio/i8255.h
> 
> You need to provide a rationale for the separate .h file in the commit
> message even if it is clear
> how it is used in the following patches.
> 
> Yours,
> Linus Walleij

I think I'll move this to gpio/driver.h as per Andy Shevchenko's
suggestion. For now only a few drivers under drivers/gpio/ use this
library, so it probably doesn't need to be separate just yet.

William Breathitt Gray

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

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[Bartosz Golaszewski]

On Tue, Jul 12, 2022 at 5:06 AM William Breathitt Gray
<william.gray@linaro.org> wrote:
>
> On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:
> > On Fri, Jul 8, 2022 at 1:16 AM William Breathitt Gray
> > <william.gray@linaro.org> wrote:
> >
> > > Exposes consumer functions providing support for Intel 8255 Programmable
> > > Peripheral Interface devices. A CONFIG_GPIO_I8255 Kconfig option is
> > > introduced; modules wanting access to these functions should select this
> > > Kconfig option.
> > >
> > > Tested-by: Fred Eckert <Frede@cmslaser.com>
> > > Cc: John Hentges <jhentges@accesio.com>
> > > Cc: Jay Dolan <jay.dolan@accesio.com>
> > > Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
> >
> > This chip is like 50 years old, but so am I and I am not obsolete, it's about
> > time that we implement a proper driver for it!
> >
> > But I suppose you are not really using the actual discrete i8255 component?
> > This is certainly used as integrated into some bridge or so? (Should be
> > mentioned in the commit.)
>
> Interestingly, there are some PC/104 devices out there that use actual
> i8255 components (e.g. Diamond Systems Onyx-MM with its 82C55 chips),
> but honestly the majority of devices I come across are simply emulating
> the i8255 interface in an FPGA or similar.
>
> I'll adjust the commit to make it clearer that this is a library for
> i8255-compatible interfaces rather than support for any physical Intel
> 8255 chip in particular.
>
> > > +config GPIO_I8255
> > > +       tristate
> >
> > That's a bit terse :D Explain that this is a Intel 8255 PPI chip first developed
> > in the first half of the 1970ies.
>
> Ack.
>
> > > +++ b/include/linux/gpio/i8255.h
> >
> > You need to provide a rationale for the separate .h file in the commit
> > message even if it is clear
> > how it is used in the following patches.
> >
> > Yours,
> > Linus Walleij
>
> I think I'll move this to gpio/driver.h as per Andy Shevchenko's

I don't think this is what Andy meant. I think he suggested moving
this header into drivers/gpio/ because it doesn't make sense for it to
be publicly accessible for anyone else than the GPIO drivers.

Andy: correct me if I'm wrong.

Bart

> suggestion. For now only a few drivers under drivers/gpio/ use this
> library, so it probably doesn't need to be separate just yet.
>

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 9:40 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> On Tue, Jul 12, 2022 at 5:06 AM William Breathitt Gray
> <william.gray@linaro.org> wrote:
> > On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:

...

> > I think I'll move this to gpio/driver.h as per Andy Shevchenko's
>
> I don't think this is what Andy meant. I think he suggested moving
> this header into drivers/gpio/ because it doesn't make sense for it to
> be publicly accessible for anyone else than the GPIO drivers.
>
> Andy: correct me if I'm wrong.

No, you are right. I was talking about localizing the header to drivers/gpio.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[William Breathitt Gray]

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

On Wed, Jul 13, 2022 at 12:10:51PM +0200, Andy Shevchenko wrote:
> On Wed, Jul 13, 2022 at 9:40 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > On Tue, Jul 12, 2022 at 5:06 AM William Breathitt Gray
> > <william.gray@linaro.org> wrote:
> > > On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:
> 
> ...
> 
> > > I think I'll move this to gpio/driver.h as per Andy Shevchenko's
> >
> > I don't think this is what Andy meant. I think he suggested moving
> > this header into drivers/gpio/ because it doesn't make sense for it to
> > be publicly accessible for anyone else than the GPIO drivers.
> >
> > Andy: correct me if I'm wrong.
> 
> No, you are right. I was talking about localizing the header to drivers/gpio.
> 
> -- 
> With Best Regards,
> Andy Shevchenko

Sure, I can move it to drivers/gpio/i8255.h to keep it local to the GPIO
drivers. It'll be trivia to move this out if the need ever arrives in
the future so I have no problem with that.

William Breathitt Gray

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

Re: [PATCH v2 1/6] gpio: i8255: Introduce the i8255 module

[Andy Shevchenko]

On Wed, Jul 13, 2022 at 12:49 PM William Breathitt Gray
<william.gray@linaro.org> wrote:
> On Wed, Jul 13, 2022 at 12:10:51PM +0200, Andy Shevchenko wrote:
> > On Wed, Jul 13, 2022 at 9:40 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > On Tue, Jul 12, 2022 at 5:06 AM William Breathitt Gray
> > > <william.gray@linaro.org> wrote:
> > > > On Mon, Jul 11, 2022 at 03:02:10PM +0200, Linus Walleij wrote:

...

> > > > I think I'll move this to gpio/driver.h as per Andy Shevchenko's
> > >
> > > I don't think this is what Andy meant. I think he suggested moving
> > > this header into drivers/gpio/ because it doesn't make sense for it to
> > > be publicly accessible for anyone else than the GPIO drivers.
> > >
> > > Andy: correct me if I'm wrong.
> >
> > No, you are right. I was talking about localizing the header to drivers/gpio.

> Sure, I can move it to drivers/gpio/i8255.h to keep it local to the GPIO

Please, keep it named in the same pattern as the C-file, but having .h
extension. I believe it's gpio-i8255.h.

> drivers. It'll be trivia to move this out if the need ever arrives in
> the future so I have no problem with that.

Thanks!

-- 
With Best Regards,
Andy Shevchenko