diff mbox series

[2/3] gpio: 104-idio-16: Utilize the idio-16 GPIO library

Message ID 6f8b7d379a83e1509ec790bbaf0a9e15fdf26180.1662927941.git.william.gray@linaro.org
State Superseded
Headers show
Series Introduce the ACCES IDIO-16 GPIO library module | expand

Commit Message

William Breathitt Gray Sept. 11, 2022, 8:34 p.m. UTC 
The ACCES 104-IDIO-16 device is part of the ACCES IDIO-16 family, so the
idio-16 GPIO library module is selected and utilized to consolidate
code.

Signed-off-by: William Breathitt Gray <william.gray@linaro.org>
---
 drivers/gpio/Kconfig            |  1 +
 drivers/gpio/gpio-104-idio-16.c | 91 ++++++++-------------------------
 2 files changed, 22 insertions(+), 70 deletions(-)

Comments

Andy Shevchenko Sept. 13, 2022, 4:19 p.m. UTC | #1 
On Sun, Sep 11, 2022 at 04:34:39PM -0400, William Breathitt Gray wrote:
> The ACCES 104-IDIO-16 device is part of the ACCES IDIO-16 family, so the
> idio-16 GPIO library module is selected and utilized to consolidate
> code.

> +	/* Reading output signals is not supported */
> +	if (offset < IDIO_16_NOUT)
>  		return -EINVAL;

I see it's in the original code, but isn't it possible to cache and return
cached value in such cases?
William Breathitt Gray Sept. 13, 2022, 4:25 p.m. UTC | #2 
On Tue, Sep 13, 2022 at 07:19:03PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 04:34:39PM -0400, William Breathitt Gray wrote:
> > The ACCES 104-IDIO-16 device is part of the ACCES IDIO-16 family, so the
> > idio-16 GPIO library module is selected and utilized to consolidate
> > code.
> 
> > +	/* Reading output signals is not supported */
> > +	if (offset < IDIO_16_NOUT)
> >  		return -EINVAL;
> 
> I see it's in the original code, but isn't it possible to cache and return
> cached value in such cases?
> 
> -- 
> With Best Regards,
> Andy Shevchenko

I think you're right, we're already caching the outputs for the sake of
the signal set calls, so we can just return those respective output
values for the signal get calls. It should also save us some cycles by
avoiding the read8() calls for the outputs in the PCI-IDIO-16 case.

William Breathitt Gray
diff mbox series

Patch

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 551351e11365..48846ee476e2 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -866,6 +866,7 @@  config GPIO_104_IDIO_16
 	depends on PC104
 	select ISA_BUS_API
 	select GPIOLIB_IRQCHIP
+	select GPIO_IDIO_16
 	help
 	  Enables GPIO support for the ACCES 104-IDIO-16 family (104-IDIO-16,
 	  104-IDIO-16E, 104-IDO-16, 104-IDIO-8, 104-IDIO-8E, 104-IDO-8). The
diff --git a/drivers/gpio/gpio-104-idio-16.c b/drivers/gpio/gpio-104-idio-16.c
index 65a5f581d981..75b5f019676f 100644
--- a/drivers/gpio/gpio-104-idio-16.c
+++ b/drivers/gpio/gpio-104-idio-16.c
@@ -6,7 +6,7 @@ 
  * This driver supports the following ACCES devices: 104-IDIO-16,
  * 104-IDIO-16E, 104-IDO-16, 104-IDIO-8, 104-IDIO-8E, and 104-IDO-8.
  */
-#include <linux/bits.h>
+#include <linux/bitmap.h>
 #include <linux/device.h>
 #include <linux/errno.h>
 #include <linux/gpio/driver.h>
@@ -21,6 +21,8 @@ 
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
+#include "gpio-idio-16.h"
+
 #define IDIO_16_EXTENT 8
 #define MAX_NUM_IDIO_16 max_num_isa_dev(IDIO_16_EXTENT)
 
@@ -33,49 +35,26 @@  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
  * @reg:	I/O address offset for the device registers
- * @out_state:	output bits state
+ * @state:	ACCES IDIO-16 device state
  */
 struct idio_16_gpio {
 	struct gpio_chip chip;
 	raw_spinlock_t lock;
 	unsigned long irq_mask;
-	struct idio_16_reg __iomem *reg;
-	unsigned int out_state;
+	struct idio_16 __iomem *reg;
+	struct idio_16_state state;
 };
 
 static int idio_16_gpio_get_direction(struct gpio_chip *chip,
 				      unsigned int offset)
 {
-	if (offset > 15)
+	if (idio_16_get_direction(offset))
 		return GPIO_LINE_DIRECTION_IN;
 
 	return GPIO_LINE_DIRECTION_OUT;
@@ -97,15 +76,12 @@  static int idio_16_gpio_direction_output(struct gpio_chip *chip,
 static int idio_16_gpio_get(struct gpio_chip *chip, unsigned int offset)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
-	const unsigned int mask = BIT(offset-16);
 
-	if (offset < 16)
+	/* Reading output signals is not supported */
+	if (offset < IDIO_16_NOUT)
 		return -EINVAL;
 
-	if (offset < 24)
-		return !!(ioread8(&idio16gpio->reg->in0_7) & mask);
-
-	return !!(ioread8(&idio16gpio->reg->in8_15) & (mask>>8));
+	return idio_16_get(idio16gpio->reg, offset);
 }
 
 static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
@@ -113,12 +89,11 @@  static int idio_16_gpio_get_multiple(struct gpio_chip *chip,
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
 
-	*bits = 0;
-	if (*mask & GENMASK(23, 16))
-		*bits |= (unsigned long)ioread8(&idio16gpio->reg->in0_7) << 16;
-	if (*mask & GENMASK(31, 24))
-		*bits |= (unsigned long)ioread8(&idio16gpio->reg->in8_15) << 24;
+	/* Reading output signals is not supported */
+	if (*mask & GENMASK(IDIO_16_NOUT - 1, 0))
+		return -EINVAL;
 
+	idio_16_get_multiple(idio16gpio->reg, &idio16gpio->state, mask, bits);
 	return 0;
 }
 
@@ -126,44 +101,16 @@  static void idio_16_gpio_set(struct gpio_chip *chip, unsigned int offset,
 			     int value)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
-	const unsigned int mask = BIT(offset);
-	unsigned long flags;
-
-	if (offset > 15)
-		return;
 
-	raw_spin_lock_irqsave(&idio16gpio->lock, flags);
-
-	if (value)
-		idio16gpio->out_state |= mask;
-	else
-		idio16gpio->out_state &= ~mask;
-
-	if (offset > 7)
-		iowrite8(idio16gpio->out_state >> 8, &idio16gpio->reg->out8_15);
-	else
-		iowrite8(idio16gpio->out_state, &idio16gpio->reg->out0_7);
-
-	raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
+	idio_16_set(idio16gpio->reg, &idio16gpio->state, offset, value);
 }
 
 static void idio_16_gpio_set_multiple(struct gpio_chip *chip,
 	unsigned long *mask, unsigned long *bits)
 {
 	struct idio_16_gpio *const idio16gpio = gpiochip_get_data(chip);
-	unsigned long flags;
 
-	raw_spin_lock_irqsave(&idio16gpio->lock, flags);
-
-	idio16gpio->out_state &= ~*mask;
-	idio16gpio->out_state |= *mask & *bits;
-
-	if (*mask & 0xFF)
-		iowrite8(idio16gpio->out_state, &idio16gpio->reg->out0_7);
-	if ((*mask >> 8) & 0xFF)
-		iowrite8(idio16gpio->out_state >> 8, &idio16gpio->reg->out8_15);
-
-	raw_spin_unlock_irqrestore(&idio16gpio->lock, flags);
+	idio_16_set_multiple(idio16gpio->reg, &idio16gpio->state, mask, bits);
 }
 
 static void idio_16_irq_ack(struct irq_data *data)
@@ -296,7 +243,10 @@  static int idio_16_probe(struct device *dev, unsigned int id)
 	idio16gpio->chip.get_multiple = idio_16_gpio_get_multiple;
 	idio16gpio->chip.set = idio_16_gpio_set;
 	idio16gpio->chip.set_multiple = idio_16_gpio_set_multiple;
-	idio16gpio->out_state = 0xFFFF;
+
+	/* FET off states are represented by bit values of "1" */
+	bitmap_fill(idio16gpio->state.out_state, IDIO_16_NOUT);
+	idio_16_state_init(&idio16gpio->state);
 
 	girq = &idio16gpio->chip.irq;
 	girq->chip = &idio_16_irqchip;
@@ -338,3 +288,4 @@  module_isa_driver(idio_16_driver, num_idio_16);
 MODULE_AUTHOR("William Breathitt Gray <vilhelm.gray@gmail.com>");
 MODULE_DESCRIPTION("ACCES 104-IDIO-16 GPIO driver");
 MODULE_LICENSE("GPL v2");
+MODULE_IMPORT_NS(IDIO_16);