2022-12-02 17:22:09

by William Breathitt Gray

[permalink] [raw]
Subject: [PATCH v4 0/9] Migrate i8255 GPIO drivers to regmap API

Changes in v4:
- Align trailing '\' characters in macros to same column in 104-dio-48e
and 104-idi-48
- Remove superfluous trailing commas in macros in 104-dio-48e and
104-idi-48
- Simplify dio48e_handle_mask_sync() regmap_* calls error checks to
return error codes directly
- Refactor dio48e_irq_init_hw() for regmap and comment about spurious
interrupts; in theory this should be enough to prevent spurious
interrupts before the system is ready, but we can pass this function
as a callback where needed in the future
- Cleanup '{0}' as '{}' in probe() callbacks for 104-dio-48e,
104-idi-48, and gpio-mm
- Utilize dev_err_probe() in probe() callbacks for 104-dio-48e and
104-idi-48
- Provide convenience comments for idi48_regmap_irqs initialization
list
- Replace header includes with forward declarations in gpio-i8255.h

The regmap API supports IO port accessors so we can take advantage of
regmap abstractions rather than handling access to the device registers
directly in the driver. The 104-dio-48e and gpio-mm modules depend on
the i8255 library and are thus updated accordingly.

The 104-IDI-48 is hardwired as an input-only device. Because the i8255
control registers are not exposed on the 104-IDI-48, the i8255 library
doesn't provide much benefit here. In this case it's simpler to utilize
the gpio_regmap API directly, so this patch series does such and removes
the i8255 library dependency from the 104-idi-48 module.

The first patch in this series adjusts gpio_regmap to always set
gpio_chip get_direction. This patch is mostly independent of the rest of
the patches in the series and can be picked up separately if desired.
I'm including it here because it allows the input-only 104-idi-48 driver
to continue reporting offset directions after migrating to gpio_regmap.

A handle_mask_sync() callback is introduced for the regmap_irq API. This
is used by the 104-dio-48e driver to handle its enable/disable interrupt
functionality. The 104-DIO-48E has a single register to handle
enabling/disabling interrupts: a write of any value to this register
enables interrupts, while any read disables interrupts.

By leveraging the gpio_regmap API, the i8255 library is reduced to
simply a devm_i8255_regmap_register() function, a configuration
structure struct i8255_regmap_config, and a helper macro
i8255_volatile_regmap_range() provided to simplify volatile PPI register
hinting for the regmap.

The regmap_irq API is leveraged by the 104-idi-48 and 104-dio-48e
modules to support their IRQ functionality. Do their respective regmap
configurations need use_hwlock set to true in this case, or is adequate
locking already handled by the regmap_irq API?

William Breathitt Gray (9):
gpio: regmap: Always set gpio_chip get_direction
regmap-irq: Add handle_mask_sync() callback
gpio: 104-dio-48e: Migrate to the regmap-irq API
gpio: 104-idi-48: Migrate to the regmap-irq API
gpio: 104-idi-48: Migrate to gpio-regmap API
gpio: i8255: Migrate to gpio-regmap API
gpio: 104-dio-48e: Migrate to regmap API
gpio: gpio-mm: Migrate to regmap API
gpio: i8255: Remove unused legacy interface

drivers/base/regmap/regmap-irq.c | 44 +++-
drivers/gpio/Kconfig | 5 +-
drivers/gpio/gpio-104-dio-48e.c | 391 ++++++++++---------------------
drivers/gpio/gpio-104-idi-48.c | 336 +++++++++-----------------
drivers/gpio/gpio-gpio-mm.c | 154 +++---------
drivers/gpio/gpio-i8255.c | 320 +++++++------------------
drivers/gpio/gpio-i8255.h | 54 ++---
drivers/gpio/gpio-regmap.c | 7 +-
include/linux/regmap.h | 5 +
9 files changed, 416 insertions(+), 900 deletions(-)


base-commit: 3b7c7478eda00945987d45f902bc3942c89243d3
--
2.38.1


2022-12-02 17:22:17

by William Breathitt Gray

[permalink] [raw]
Subject: [PATCH v4 8/9] gpio: gpio-mm: Migrate to regmap API

The regmap API supports IO port accessors so we can take advantage of
regmap abstractions rather than handling access to the device registers
directly in the driver. The gpio-mm module is migrated to the new i8255
library interface leveraging the gpio-regmap API.

Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/gpio/gpio-gpio-mm.c | 154 +++++++-----------------------------
1 file changed, 30 insertions(+), 124 deletions(-)

diff --git a/drivers/gpio/gpio-gpio-mm.c b/drivers/gpio/gpio-gpio-mm.c
index 2689671b6b01..43d823a56e59 100644
--- a/drivers/gpio/gpio-gpio-mm.c
+++ b/drivers/gpio/gpio-gpio-mm.c
@@ -8,13 +8,13 @@
*/
#include <linux/device.h>
#include <linux/errno.h>
-#include <linux/gpio/driver.h>
-#include <linux/io.h>
#include <linux/ioport.h>
#include <linux/isa.h>
#include <linux/kernel.h>
#include <linux/module.h>
#include <linux/moduleparam.h>
+#include <linux/regmap.h>
+#include <linux/types.h>

#include "gpio-i8255.h"

@@ -30,83 +30,22 @@ MODULE_PARM_DESC(base, "Diamond Systems GPIO-MM base addresses");

#define GPIOMM_NUM_PPI 2

-/**
- * struct gpiomm_gpio - GPIO device private data structure
- * @chip: instance of the gpio_chip
- * @ppi_state: Programmable Peripheral Interface group states
- * @ppi: Programmable Peripheral Interface groups
- */
-struct gpiomm_gpio {
- struct gpio_chip chip;
- struct i8255_state ppi_state[GPIOMM_NUM_PPI];
- struct i8255 __iomem *ppi;
+static const struct regmap_range gpiomm_volatile_ranges[] = {
+ i8255_volatile_regmap_range(0x0), i8255_volatile_regmap_range(0x4),
+};
+static const struct regmap_access_table gpiomm_volatile_table = {
+ .yes_ranges = gpiomm_volatile_ranges,
+ .n_yes_ranges = ARRAY_SIZE(gpiomm_volatile_ranges),
+};
+static const struct regmap_config gpiomm_regmap_config = {
+ .reg_bits = 8,
+ .reg_stride = 1,
+ .val_bits = 8,
+ .io_port = true,
+ .max_register = 0x7,
+ .volatile_table = &gpiomm_volatile_table,
+ .cache_type = REGCACHE_FLAT,
};
-
-static int gpiomm_gpio_get_direction(struct gpio_chip *chip,
- unsigned int offset)
-{
- struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-
- if (i8255_get_direction(gpiommgpio->ppi_state, offset))
- return GPIO_LINE_DIRECTION_IN;
-
- return GPIO_LINE_DIRECTION_OUT;
-}
-
-static int gpiomm_gpio_direction_input(struct gpio_chip *chip,
- unsigned int offset)
-{
- struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-
- i8255_direction_input(gpiommgpio->ppi, gpiommgpio->ppi_state, offset);
-
- return 0;
-}
-
-static int gpiomm_gpio_direction_output(struct gpio_chip *chip,
- unsigned int offset, int value)
-{
- struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-
- i8255_direction_output(gpiommgpio->ppi, gpiommgpio->ppi_state, offset,
- value);
-
- return 0;
-}
-
-static int gpiomm_gpio_get(struct gpio_chip *chip, unsigned int offset)
-{
- struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-
- return i8255_get(gpiommgpio->ppi, offset);
-}
-
-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);
-
- i8255_get_multiple(gpiommgpio->ppi, mask, bits, chip->ngpio);
-
- return 0;
-}
-
-static void gpiomm_gpio_set(struct gpio_chip *chip, unsigned int offset,
- int value)
-{
- struct gpiomm_gpio *const gpiommgpio = gpiochip_get_data(chip);
-
- i8255_set(gpiommgpio->ppi, gpiommgpio->ppi_state, offset, value);
-}
-
-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);
-
- i8255_set_multiple(gpiommgpio->ppi, gpiommgpio->ppi_state, mask, bits,
- chip->ngpio);
-}

#define GPIOMM_NGPIO 48
static const char *gpiomm_names[GPIOMM_NGPIO] = {
@@ -120,30 +59,11 @@ 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,
- struct i8255_state *const ppi_state)
-{
- 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 < GPIOMM_NUM_PPI; i++) {
- i8255_mode0_output(&ppi[i]);
- i8255_set_multiple(&ppi[i], &ppi_state[i], &mask, &bits, ngpio);
- }
-}
-
static int gpiomm_probe(struct device *dev, unsigned int id)
{
- struct gpiomm_gpio *gpiommgpio;
const char *const name = dev_name(dev);
- int err;
-
- gpiommgpio = devm_kzalloc(dev, sizeof(*gpiommgpio), GFP_KERNEL);
- if (!gpiommgpio)
- return -ENOMEM;
+ struct i8255_regmap_config config = {};
+ void __iomem *regs;

if (!devm_request_region(dev, base[id], GPIOMM_EXTENT, name)) {
dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
@@ -151,34 +71,20 @@ static int gpiomm_probe(struct device *dev, unsigned int id)
return -EBUSY;
}

- gpiommgpio->ppi = devm_ioport_map(dev, base[id], GPIOMM_EXTENT);
- if (!gpiommgpio->ppi)
+ regs = devm_ioport_map(dev, base[id], GPIOMM_EXTENT);
+ if (!regs)
return -ENOMEM;

- gpiommgpio->chip.label = name;
- gpiommgpio->chip.parent = dev;
- gpiommgpio->chip.owner = THIS_MODULE;
- gpiommgpio->chip.base = -1;
- gpiommgpio->chip.ngpio = GPIOMM_NGPIO;
- gpiommgpio->chip.names = gpiomm_names;
- gpiommgpio->chip.get_direction = gpiomm_gpio_get_direction;
- gpiommgpio->chip.direction_input = gpiomm_gpio_direction_input;
- gpiommgpio->chip.direction_output = gpiomm_gpio_direction_output;
- gpiommgpio->chip.get = gpiomm_gpio_get;
- gpiommgpio->chip.get_multiple = gpiomm_gpio_get_multiple;
- gpiommgpio->chip.set = gpiomm_gpio_set;
- gpiommgpio->chip.set_multiple = gpiomm_gpio_set_multiple;
-
- i8255_state_init(gpiommgpio->ppi_state, GPIOMM_NUM_PPI);
- gpiomm_init_dio(gpiommgpio->ppi, gpiommgpio->ppi_state);
-
- err = devm_gpiochip_add_data(dev, &gpiommgpio->chip, gpiommgpio);
- if (err) {
- dev_err(dev, "GPIO registering failed (%d)\n", err);
- return err;
- }
+ config.map = devm_regmap_init_mmio(dev, regs, &gpiomm_regmap_config);
+ if (IS_ERR(config.map))
+ return dev_err_probe(dev, PTR_ERR(config.map),
+ "Unable to initialize register map\n");
+
+ config.parent = dev;
+ config.num_ppi = GPIOMM_NUM_PPI;
+ config.names = gpiomm_names;

- return 0;
+ return devm_i8255_regmap_register(dev, &config);
}

static struct isa_driver gpiomm_driver = {
--
2.38.1

2022-12-02 17:22:45

by William Breathitt Gray

[permalink] [raw]
Subject: [PATCH v4 9/9] gpio: i8255: Remove unused legacy interface

All i8255 library consumers have migrated to the new interface
leveraging the gpio-regmap API. Legacy interface functions and code are
removed as no longer needed.

Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/gpio/gpio-i8255.c | 243 +-------------------------------------
drivers/gpio/gpio-i8255.h | 40 -------
2 files changed, 1 insertion(+), 282 deletions(-)

diff --git a/drivers/gpio/gpio-i8255.c b/drivers/gpio/gpio-i8255.c
index 9ecb2e9b97f9..64ab80fc4a1e 100644
--- a/drivers/gpio/gpio-i8255.c
+++ b/drivers/gpio/gpio-i8255.c
@@ -3,16 +3,13 @@
* Intel 8255 Programmable Peripheral Interface
* Copyright (C) 2022 William Breathitt Gray
*/
-#include <linux/bitmap.h>
+#include <linux/bits.h>
#include <linux/device.h>
#include <linux/err.h>
#include <linux/export.h>
#include <linux/gpio/regmap.h>
-#include <linux/io.h>
#include <linux/module.h>
#include <linux/regmap.h>
-#include <linux/spinlock.h>
-#include <linux/types.h>

#include "gpio-i8255.h"

@@ -30,15 +27,6 @@
#define I8255_REG_DAT_BASE I8255_PORTA
#define I8255_REG_DIR_IN_BASE I8255_CONTROL

-static int i8255_get_port(struct i8255 __iomem *const ppi,
- const unsigned long io_port, const unsigned long mask)
-{
- const unsigned long bank = io_port / 3;
- const unsigned long ppi_port = io_port % 3;
-
- return ioread8(&ppi[bank].port[ppi_port]) & mask;
-}
-
static int i8255_direction_mask(const unsigned int offset)
{
const unsigned int stride = offset / I8255_NGPIO_PER_REG;
@@ -103,235 +91,6 @@ static int i8255_reg_mask_xlate(struct gpio_regmap *gpio, unsigned int base,
}
}

-static void i8255_set_port(struct i8255 __iomem *const ppi,
- struct i8255_state *const state,
- const unsigned long io_port,
- const unsigned long mask, const unsigned long bits)
-{
- const unsigned long bank = io_port / 3;
- const unsigned long ppi_port = io_port % 3;
- unsigned long flags;
- unsigned long out_state;
-
- spin_lock_irqsave(&state[bank].lock, flags);
-
- out_state = ioread8(&ppi[bank].port[ppi_port]);
- out_state = (out_state & ~mask) | (bits & mask);
- iowrite8(out_state, &ppi[bank].port[ppi_port]);
-
- spin_unlock_irqrestore(&state[bank].lock, flags);
-}
-
-/**
- * i8255_direction_input - configure signal offset as input
- * @ppi: Intel 8255 Programmable Peripheral Interface banks
- * @state: devices states of the respective PPI banks
- * @offset: signal offset to configure as input
- *
- * Configures a signal @offset as input for the respective Intel 8255
- * Programmable Peripheral Interface (@ppi) banks. The @state control_state
- * values are updated to reflect the new configuration.
- */
-void i8255_direction_input(struct i8255 __iomem *const ppi,
- struct i8255_state *const state,
- const unsigned long offset)
-{
- const unsigned long io_port = offset / 8;
- const unsigned long bank = io_port / 3;
- unsigned long flags;
-
- spin_lock_irqsave(&state[bank].lock, flags);
-
- state[bank].control_state |= I8255_CONTROL_MODE_SET;
- state[bank].control_state |= i8255_direction_mask(offset % 24);
-
- iowrite8(state[bank].control_state, &ppi[bank].control);
-
- spin_unlock_irqrestore(&state[bank].lock, flags);
-}
-EXPORT_SYMBOL_NS_GPL(i8255_direction_input, I8255);
-
-/**
- * i8255_direction_output - configure signal offset as output
- * @ppi: Intel 8255 Programmable Peripheral Interface banks
- * @state: devices states of the respective PPI banks
- * @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) banks and sets the respective signal
- * output to the desired @value. The @state control_state values are updated to
- * reflect the new configuration.
- */
-void i8255_direction_output(struct i8255 __iomem *const ppi,
- struct i8255_state *const state,
- const unsigned long offset,
- const unsigned long value)
-{
- const unsigned long io_port = offset / 8;
- const unsigned long bank = io_port / 3;
- unsigned long flags;
-
- spin_lock_irqsave(&state[bank].lock, flags);
-
- state[bank].control_state |= I8255_CONTROL_MODE_SET;
- state[bank].control_state &= ~i8255_direction_mask(offset % 24);
-
- iowrite8(state[bank].control_state, &ppi[bank].control);
-
- spin_unlock_irqrestore(&state[bank].lock, flags);
-
- i8255_set(ppi, state, offset, value);
-}
-EXPORT_SYMBOL_NS_GPL(i8255_direction_output, I8255);
-
-/**
- * i8255_get - get signal value at signal offset
- * @ppi: Intel 8255 Programmable Peripheral Interface banks
- * @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) banks.
- */
-int i8255_get(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_NS_GPL(i8255_get, I8255);
-
-/**
- * i8255_get_direction - get the I/O direction for a signal offset
- * @state: devices states of the respective PPI banks
- * @offset: offset of signal to get direction
- *
- * Returns the signal direction (0=output, 1=input) for the signal at @offset.
- */
-int i8255_get_direction(const struct i8255_state *const state,
- const unsigned long offset)
-{
- const unsigned long io_port = offset / 8;
- const unsigned long bank = io_port / 3;
-
- return !!(state[bank].control_state & i8255_direction_mask(offset % 24));
-}
-EXPORT_SYMBOL_NS_GPL(i8255_get_direction, I8255);
-
-/**
- * i8255_get_multiple - get multiple signal values at multiple signal offsets
- * @ppi: Intel 8255 Programmable Peripheral Interface banks
- * @mask: mask of signals to get
- * @bits: bitmap to store signal values
- * @ngpio: number of GPIO signals of the respective PPI banks
- *
- * Stores in @bits the values (0=low, 1=high) for the signals defined by @mask
- * for the respective Intel 8255 Programmable Peripheral Interface (@ppi) banks.
- */
-void i8255_get_multiple(struct i8255 __iomem *const ppi,
- const unsigned long *const mask,
- unsigned long *const bits, const unsigned long ngpio)
-{
- unsigned long offset;
- unsigned long port_mask;
- unsigned long io_port;
- unsigned long port_state;
-
- bitmap_zero(bits, ngpio);
-
- for_each_set_clump8(offset, port_mask, mask, ngpio) {
- io_port = offset / 8;
- port_state = i8255_get_port(ppi, io_port, port_mask);
-
- bitmap_set_value8(bits, port_state, offset);
- }
-}
-EXPORT_SYMBOL_NS_GPL(i8255_get_multiple, I8255);
-
-/**
- * i8255_mode0_output - configure all PPI ports to MODE 0 output mode
- * @ppi: Intel 8255 Programmable Peripheral Interface bank
- *
- * 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_NS_GPL(i8255_mode0_output, I8255);
-
-/**
- * i8255_set - set signal value at signal offset
- * @ppi: Intel 8255 Programmable Peripheral Interface banks
- * @state: devices states of the respective PPI banks
- * @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) banks.
- */
-void i8255_set(struct i8255 __iomem *const ppi, struct i8255_state *const state,
- 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 mask = BIT(port_offset);
- const unsigned long bits = value << port_offset;
-
- i8255_set_port(ppi, state, io_port, mask, bits);
-}
-EXPORT_SYMBOL_NS_GPL(i8255_set, I8255);
-
-/**
- * i8255_set_multiple - set signal values at multiple signal offsets
- * @ppi: Intel 8255 Programmable Peripheral Interface banks
- * @state: devices states of the respective PPI banks
- * @mask: mask of signals to set
- * @bits: bitmap of signal output values
- * @ngpio: number of GPIO signals of the respective PPI banks
- *
- * Assigns output values defined by @bits for the signals defined by @mask for
- * the respective Intel 8255 Programmable Peripheral Interface (@ppi) banks.
- */
-void i8255_set_multiple(struct i8255 __iomem *const ppi,
- struct i8255_state *const state,
- const unsigned long *const mask,
- const unsigned long *const bits,
- const unsigned long ngpio)
-{
- unsigned long offset;
- unsigned long port_mask;
- unsigned long io_port;
- unsigned long value;
-
- for_each_set_clump8(offset, port_mask, mask, ngpio) {
- io_port = offset / 8;
- value = bitmap_get_value8(bits, offset);
- i8255_set_port(ppi, state, io_port, port_mask, value);
- }
-}
-EXPORT_SYMBOL_NS_GPL(i8255_set_multiple, I8255);
-
-/**
- * i8255_state_init - initialize i8255_state structure
- * @state: devices states of the respective PPI banks
- * @nbanks: number of Intel 8255 Programmable Peripheral Interface banks
- *
- * Initializes the @state of each Intel 8255 Programmable Peripheral Interface
- * bank for use in i8255 library functions.
- */
-void i8255_state_init(struct i8255_state *const state,
- const unsigned long nbanks)
-{
- unsigned long bank;
-
- for (bank = 0; bank < nbanks; bank++)
- spin_lock_init(&state[bank].lock);
-}
-EXPORT_SYMBOL_NS_GPL(i8255_state_init, I8255);
-
/**
* devm_i8255_regmap_register - Register an i8255 GPIO controller
* @dev: device that is registering this i8255 GPIO device
diff --git a/drivers/gpio/gpio-i8255.h b/drivers/gpio/gpio-i8255.h
index 3daa0b145890..9dcf639b94df 100644
--- a/drivers/gpio/gpio-i8255.h
+++ b/drivers/gpio/gpio-i8255.h
@@ -3,29 +3,6 @@
#ifndef _I8255_H_
#define _I8255_H_

-#include <linux/spinlock.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;
-};
-
-/**
- * struct i8255_state - Intel 8255 state structure
- * @lock: synchronization lock for accessing device state
- * @control_state: Control register state
- */
-struct i8255_state {
- spinlock_t lock;
- u8 control_state;
-};
-
struct device;
struct irq_domain;
struct regmap;
@@ -54,21 +31,4 @@ struct i8255_regmap_config {
int devm_i8255_regmap_register(struct device *dev,
const struct i8255_regmap_config *config);

-void i8255_direction_input(struct i8255 __iomem *ppi, struct i8255_state *state,
- unsigned long offset);
-void i8255_direction_output(struct i8255 __iomem *ppi,
- struct i8255_state *state, unsigned long offset,
- unsigned long value);
-int i8255_get(struct i8255 __iomem *ppi, unsigned long offset);
-int i8255_get_direction(const struct i8255_state *state, unsigned long offset);
-void i8255_get_multiple(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, struct i8255_state *state,
- unsigned long offset, unsigned long value);
-void i8255_set_multiple(struct i8255 __iomem *ppi, struct i8255_state *state,
- const unsigned long *mask, const unsigned long *bits,
- unsigned long ngpio);
-void i8255_state_init(struct i8255_state *const state, unsigned long nbanks);
-
#endif /* _I8255_H_ */
--
2.38.1

2022-12-02 17:22:48

by William Breathitt Gray

[permalink] [raw]
Subject: [PATCH v4 7/9] gpio: 104-dio-48e: Migrate to regmap API

The regmap API supports IO port accessors so we can take advantage of
regmap abstractions rather than handling access to the device registers
directly in the driver. The 104-dio-48e module is migrated to the new
i8255 library interface leveraging the gpio-regmap API.

Suggested-by: Andy Shevchenko <[email protected]>
Reviewed-by: Andy Shevchenko <[email protected]>
Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/gpio/gpio-104-dio-48e.c | 137 ++------------------------------
1 file changed, 7 insertions(+), 130 deletions(-)

diff --git a/drivers/gpio/gpio-104-dio-48e.c b/drivers/gpio/gpio-104-dio-48e.c
index 4c3660d5dea5..31ae98a277a4 100644
--- a/drivers/gpio/gpio-104-dio-48e.c
+++ b/drivers/gpio/gpio-104-dio-48e.c
@@ -9,7 +9,6 @@
#include <linux/bits.h>
#include <linux/device.h>
#include <linux/err.h>
-#include <linux/gpio/driver.h>
#include <linux/ioport.h>
#include <linux/irq.h>
#include <linux/isa.h>
@@ -42,90 +41,6 @@ MODULE_PARM_DESC(irq, "ACCES 104-DIO-48E interrupt line numbers");

#define DIO48E_NUM_PPI 2

-/**
- * struct dio48e_reg - device register structure
- * @ppi: Programmable Peripheral Interface groups
- */
-struct dio48e_reg {
- struct i8255 ppi[DIO48E_NUM_PPI];
-};
-
-/**
- * struct dio48e_gpio - GPIO device private data structure
- * @chip: instance of the gpio_chip
- * @ppi_state: PPI device states
- * @reg: I/O address offset for the device registers
- */
-struct dio48e_gpio {
- struct gpio_chip chip;
- struct i8255_state ppi_state[DIO48E_NUM_PPI];
- struct dio48e_reg __iomem *reg;
-};
-
-static int dio48e_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
-{
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-
- if (i8255_get_direction(dio48egpio->ppi_state, offset))
- return GPIO_LINE_DIRECTION_IN;
-
- return GPIO_LINE_DIRECTION_OUT;
-}
-
-static int dio48e_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
-{
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-
- i8255_direction_input(dio48egpio->reg->ppi, dio48egpio->ppi_state,
- offset);
-
- return 0;
-}
-
-static int dio48e_gpio_direction_output(struct gpio_chip *chip, unsigned int offset,
- int value)
-{
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-
- i8255_direction_output(dio48egpio->reg->ppi, dio48egpio->ppi_state,
- offset, value);
-
- return 0;
-}
-
-static int dio48e_gpio_get(struct gpio_chip *chip, unsigned int offset)
-{
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-
- return i8255_get(dio48egpio->reg->ppi, offset);
-}
-
-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);
-
- i8255_get_multiple(dio48egpio->reg->ppi, mask, bits, chip->ngpio);
-
- return 0;
-}
-
-static void dio48e_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
-{
- struct dio48e_gpio *const dio48egpio = gpiochip_get_data(chip);
-
- i8255_set(dio48egpio->reg->ppi, dio48egpio->ppi_state, offset, value);
-}
-
-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);
-
- i8255_set_multiple(dio48egpio->reg->ppi, dio48egpio->ppi_state, mask,
- bits, chip->ngpio);
-}
-
static const struct regmap_range dio48e_wr_ranges[] = {
regmap_reg_range(0x0, 0x9), regmap_reg_range(0xB, 0xB),
regmap_reg_range(0xD, 0xD), regmap_reg_range(0xF, 0xF),
@@ -240,25 +155,10 @@ static int dio48e_irq_init_hw(struct regmap *const map)
return regmap_read(map, DIO48E_DISABLE_INTERRUPT, &val);
}

-static void dio48e_init_ppi(struct i8255 __iomem *const ppi,
- struct i8255_state *const ppi_state)
-{
- 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 < DIO48E_NUM_PPI; i++) {
- i8255_mode0_output(&ppi[i]);
- i8255_set_multiple(&ppi[i], &ppi_state[i], &mask, &bits, ngpio);
- }
-}
-
static int dio48e_probe(struct device *dev, unsigned int id)
{
- struct dio48e_gpio *dio48egpio;
const char *const name = dev_name(dev);
+ struct i8255_regmap_config config = {};
void __iomem *regs;
struct regmap *map;
int err;
@@ -266,10 +166,6 @@ static int dio48e_probe(struct device *dev, unsigned int id)
unsigned int irq_mask;
struct regmap_irq_chip_data *chip_data;

- dio48egpio = devm_kzalloc(dev, sizeof(*dio48egpio), GFP_KERNEL);
- if (!dio48egpio)
- return -ENOMEM;
-
if (!devm_request_region(dev, base[id], DIO48E_EXTENT, name)) {
dev_err(dev, "Unable to lock port addresses (0x%X-0x%X)\n",
base[id], base[id] + DIO48E_EXTENT);
@@ -279,7 +175,6 @@ static int dio48e_probe(struct device *dev, unsigned int id)
regs = devm_ioport_map(dev, base[id], DIO48E_EXTENT);
if (!regs)
return -ENOMEM;
- dio48egpio->reg = regs;

map = devm_regmap_init_mmio(dev, regs, &dio48e_regmap_config);
if (IS_ERR(map))
@@ -314,31 +209,13 @@ static int dio48e_probe(struct device *dev, unsigned int id)
if (err)
return dev_err_probe(dev, err, "IRQ registration failed\n");

- dio48egpio->chip.label = name;
- dio48egpio->chip.parent = dev;
- dio48egpio->chip.owner = THIS_MODULE;
- dio48egpio->chip.base = -1;
- dio48egpio->chip.ngpio = DIO48E_NGPIO;
- dio48egpio->chip.names = dio48e_names;
- dio48egpio->chip.get_direction = dio48e_gpio_get_direction;
- dio48egpio->chip.direction_input = dio48e_gpio_direction_input;
- dio48egpio->chip.direction_output = dio48e_gpio_direction_output;
- dio48egpio->chip.get = dio48e_gpio_get;
- dio48egpio->chip.get_multiple = dio48e_gpio_get_multiple;
- dio48egpio->chip.set = dio48e_gpio_set;
- dio48egpio->chip.set_multiple = dio48e_gpio_set_multiple;
-
- i8255_state_init(dio48egpio->ppi_state, DIO48E_NUM_PPI);
- dio48e_init_ppi(dio48egpio->reg->ppi, dio48egpio->ppi_state);
-
- err = devm_gpiochip_add_data(dev, &dio48egpio->chip, dio48egpio);
- if (err) {
- dev_err(dev, "GPIO registering failed (%d)\n", err);
- return err;
- }
+ config.parent = dev;
+ config.map = map;
+ config.num_ppi = DIO48E_NUM_PPI;
+ config.names = dio48e_names;
+ config.domain = regmap_irq_get_domain(chip_data);

- return gpiochip_irqchip_add_domain(&dio48egpio->chip,
- regmap_irq_get_domain(chip_data));
+ return devm_i8255_regmap_register(dev, &config);
}

static struct isa_driver dio48e_driver = {
--
2.38.1

2022-12-02 17:22:52

by William Breathitt Gray

[permalink] [raw]
Subject: [PATCH v4 2/9] regmap-irq: Add handle_mask_sync() callback

Provide a public callback handle_mask_sync() that drivers can use when
they have more complex IRQ masking logic. The default implementation is
regmap_irq_handle_mask_sync(), used if the chip doesn't provide its own
callback.

Signed-off-by: William Breathitt Gray <[email protected]>
---
drivers/base/regmap/regmap-irq.c | 44 ++++++++++++++++++++++----------
include/linux/regmap.h | 5 ++++
2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/drivers/base/regmap/regmap-irq.c b/drivers/base/regmap/regmap-irq.c
index 4ef9488d05cd..968681fa8d09 100644
--- a/drivers/base/regmap/regmap-irq.c
+++ b/drivers/base/regmap/regmap-irq.c
@@ -115,12 +115,20 @@ static void regmap_irq_sync_unlock(struct irq_data *data)
*/
for (i = 0; i < d->chip->num_regs; i++) {
if (d->mask_base) {
- reg = d->get_irq_reg(d, d->mask_base, i);
- ret = regmap_update_bits(d->map, reg,
- d->mask_buf_def[i], d->mask_buf[i]);
- if (ret)
- dev_err(d->map->dev, "Failed to sync masks in %x\n",
- reg);
+ if (d->chip->handle_mask_sync)
+ d->chip->handle_mask_sync(d->map, i,
+ d->mask_buf_def[i],
+ d->mask_buf[i],
+ d->chip->irq_drv_data);
+ else {
+ reg = d->get_irq_reg(d, d->mask_base, i);
+ ret = regmap_update_bits(d->map, reg,
+ d->mask_buf_def[i],
+ d->mask_buf[i]);
+ if (ret)
+ dev_err(d->map->dev, "Failed to sync masks in %x\n",
+ reg);
+ }
}

if (d->unmask_base) {
@@ -917,13 +925,23 @@ int regmap_add_irq_chip_fwnode(struct fwnode_handle *fwnode,
d->mask_buf[i] = d->mask_buf_def[i];

if (d->mask_base) {
- reg = d->get_irq_reg(d, d->mask_base, i);
- ret = regmap_update_bits(d->map, reg,
- d->mask_buf_def[i], d->mask_buf[i]);
- if (ret) {
- dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
- reg, ret);
- goto err_alloc;
+ if (chip->handle_mask_sync) {
+ ret = chip->handle_mask_sync(d->map, i,
+ d->mask_buf_def[i],
+ d->mask_buf[i],
+ chip->irq_drv_data);
+ if (ret)
+ goto err_alloc;
+ } else {
+ reg = d->get_irq_reg(d, d->mask_base, i);
+ ret = regmap_update_bits(d->map, reg,
+ d->mask_buf_def[i],
+ d->mask_buf[i]);
+ if (ret) {
+ dev_err(map->dev, "Failed to set masks in 0x%x: %d\n",
+ reg, ret);
+ goto err_alloc;
+ }
}
}

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index ca3434dca3a0..cc07645501af 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1542,6 +1542,8 @@ struct regmap_irq_chip_data;
* before regmap_irq_handler process the interrupts.
* @handle_post_irq: Driver specific callback to handle interrupt from device
* after handling the interrupts in regmap_irq_handler().
+ * @handle_mask_sync: Callback used to handle IRQ mask syncs. The index will be
+ * in the range [0, num_regs)
* @set_type_virt: Driver specific callback to extend regmap_irq_set_type()
* and configure virt regs. Deprecated, use @set_type_config
* callback and config registers instead.
@@ -1603,6 +1605,9 @@ struct regmap_irq_chip {

int (*handle_pre_irq)(void *irq_drv_data);
int (*handle_post_irq)(void *irq_drv_data);
+ int (*handle_mask_sync)(struct regmap *map, int index,
+ unsigned int mask_buf_def,
+ unsigned int mask_buf, void *irq_drv_data);
int (*set_type_virt)(unsigned int **buf, unsigned int type,
unsigned long hwirq, int reg);
int (*set_type_config)(unsigned int **buf, unsigned int type,
--
2.38.1

2022-12-05 10:41:31

by Bartosz Golaszewski

[permalink] [raw]
Subject: Re: [PATCH v4 0/9] Migrate i8255 GPIO drivers to regmap API

On Fri, Dec 2, 2022 at 5:27 PM William Breathitt Gray
<[email protected]> wrote:
>
> Changes in v4:
> - Align trailing '\' characters in macros to same column in 104-dio-48e
> and 104-idi-48
> - Remove superfluous trailing commas in macros in 104-dio-48e and
> 104-idi-48
> - Simplify dio48e_handle_mask_sync() regmap_* calls error checks to
> return error codes directly
> - Refactor dio48e_irq_init_hw() for regmap and comment about spurious
> interrupts; in theory this should be enough to prevent spurious
> interrupts before the system is ready, but we can pass this function
> as a callback where needed in the future
> - Cleanup '{0}' as '{}' in probe() callbacks for 104-dio-48e,
> 104-idi-48, and gpio-mm
> - Utilize dev_err_probe() in probe() callbacks for 104-dio-48e and
> 104-idi-48
> - Provide convenience comments for idi48_regmap_irqs initialization
> list
> - Replace header includes with forward declarations in gpio-i8255.h
>
> The regmap API supports IO port accessors so we can take advantage of
> regmap abstractions rather than handling access to the device registers
> directly in the driver. The 104-dio-48e and gpio-mm modules depend on
> the i8255 library and are thus updated accordingly.
>
> The 104-IDI-48 is hardwired as an input-only device. Because the i8255
> control registers are not exposed on the 104-IDI-48, the i8255 library
> doesn't provide much benefit here. In this case it's simpler to utilize
> the gpio_regmap API directly, so this patch series does such and removes
> the i8255 library dependency from the 104-idi-48 module.
>
> The first patch in this series adjusts gpio_regmap to always set
> gpio_chip get_direction. This patch is mostly independent of the rest of
> the patches in the series and can be picked up separately if desired.
> I'm including it here because it allows the input-only 104-idi-48 driver
> to continue reporting offset directions after migrating to gpio_regmap.
>
> A handle_mask_sync() callback is introduced for the regmap_irq API. This
> is used by the 104-dio-48e driver to handle its enable/disable interrupt
> functionality. The 104-DIO-48E has a single register to handle
> enabling/disabling interrupts: a write of any value to this register
> enables interrupts, while any read disables interrupts.
>
> By leveraging the gpio_regmap API, the i8255 library is reduced to
> simply a devm_i8255_regmap_register() function, a configuration
> structure struct i8255_regmap_config, and a helper macro
> i8255_volatile_regmap_range() provided to simplify volatile PPI register
> hinting for the regmap.
>
> The regmap_irq API is leveraged by the 104-idi-48 and 104-dio-48e
> modules to support their IRQ functionality. Do their respective regmap
> configurations need use_hwlock set to true in this case, or is adequate
> locking already handled by the regmap_irq API?
>
> William Breathitt Gray (9):
> gpio: regmap: Always set gpio_chip get_direction
> regmap-irq: Add handle_mask_sync() callback
> gpio: 104-dio-48e: Migrate to the regmap-irq API
> gpio: 104-idi-48: Migrate to the regmap-irq API
> gpio: 104-idi-48: Migrate to gpio-regmap API
> gpio: i8255: Migrate to gpio-regmap API
> gpio: 104-dio-48e: Migrate to regmap API
> gpio: gpio-mm: Migrate to regmap API
> gpio: i8255: Remove unused legacy interface
>
> drivers/base/regmap/regmap-irq.c | 44 +++-
> drivers/gpio/Kconfig | 5 +-
> drivers/gpio/gpio-104-dio-48e.c | 391 ++++++++++---------------------
> drivers/gpio/gpio-104-idi-48.c | 336 +++++++++-----------------
> drivers/gpio/gpio-gpio-mm.c | 154 +++---------
> drivers/gpio/gpio-i8255.c | 320 +++++++------------------
> drivers/gpio/gpio-i8255.h | 54 ++---
> drivers/gpio/gpio-regmap.c | 7 +-
> include/linux/regmap.h | 5 +
> 9 files changed, 416 insertions(+), 900 deletions(-)
>
>
> base-commit: 3b7c7478eda00945987d45f902bc3942c89243d3
> --
> 2.38.1
>

It's pretty late in the release cycle and this touches several
existing drivers so I will not take it for v6.2. Please rebase and
resend once v6.3-rc1 is out.

Bart