Skip to contentSkip to conversation

Introduce the ACCES IDIO-16 GPIO library module

PATCH Superseded William Breathitt Gray 4 participants 9 messages Sep 11, 2022 – Sep 17, 2022

Subsystems (4)
GPIO Acces 104-idio-16 GPIO driver Acces PCI-idio-16 GPIO driver Acces idio-16 GPIO library

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

William Breathitt Gray

author
#0 lore raw

cover Introduce the ACCES IDIO-16 GPIO library module

In a similar vein as the Intel 8255 interface library module [0], the ACCES IDIO-16 GPIO library module is introduced to consolidate much of the shared code between the existing 104-IDIO-16 and PCI-IDIO-16 GPIO drivers.

The idio-16 module exposes consumer library functions to facilitate communication with devices within the ACCES IDIO-16 family such as the 104-IDIO-16 and the PCI-IDIO-16.

A CONFIG_GPIO_IDIO_16 Kconfig option is introduced by this patch. Modules wanting access to these idio-16 library functions should select this Kconfig option and import the IDIO_16 symbol namespace.

[0] https://lore.kernel.org/all/d1a24895f2ea67f689c24c34a20ddb43cd7246af.1658324498.git.william.gray@linaro.org/

William Breathitt Gray (3):
gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module
gpio: 104-idio-16: Utilize the idio-16 GPIO library
gpio: pci-idio-16: Utilize the idio-16 GPIO library

 MAINTAINERS                     |   6 ++
 drivers/gpio/Kconfig            |  11 +++
 drivers/gpio/Makefile           |   1 +
 drivers/gpio/gpio-104-idio-16.c |  91 +++++-------------
 drivers/gpio/gpio-idio-16.c     | 159 ++++++++++++++++++++++++++++++++
 drivers/gpio/gpio-idio-16.h     |  70 ++++++++++++++
 drivers/gpio/gpio-pci-idio-16.c | 119 +++---------------------
 7 files changed, 281 insertions(+), 176 deletions(-)
 create mode 100644 drivers/gpio/gpio-idio-16.c
 create mode 100644 drivers/gpio/gpio-idio-16.h

base-commit: 3af20d2723be5f70e1ce818504a4c093a81b21f5

Signature · 1 line
2.37.2
author
#1 lore raw

PATCH 1/3 gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module

Exposes consumer library functions to facilitate communication with devices within the ACCES IDIO-16 family such as the 104-IDIO-16 and the PCI-IDIO-16.

A CONFIG_GPIO_IDIO_16 Kconfig option is introduced by this patch. Modules wanting access to these idio-16 library functions should select this Kconfig option and import the IDIO_16 symbol namespace.

Signed-off-by William Breathitt Gray <[email protected]>
---
 MAINTAINERS                 |   6 ++
 drivers/gpio/Kconfig        |   9 ++
 drivers/gpio/Makefile       |   1 +
 drivers/gpio/gpio-idio-16.c | 159 ++++++++++++++++++++++++++++++++++++
 drivers/gpio/gpio-idio-16.h |  70 ++++++++++++++++
 5 files changed, 245 insertions(+)
 create mode 100644 drivers/gpio/gpio-idio-16.c
 create mode 100644 drivers/gpio/gpio-idio-16.h
MAINTAINERS +6 -0
index 8a5012ba6ff9..a143d4bc8547 100644
@@ -312,6 +312,12 @@ L:	[email protected]
 S:	Maintained
 F:	drivers/counter/104-quad-8.c
 
+ACCES IDIO-16 GPIO LIBRARY
+M:	William Breathitt Gray <[email protected]>
+L:	[email protected]
+S:	Maintained
+F:	drivers/gpio/gpio-idio-16.c
+
 ACCES PCI-IDIO-16 GPIO DRIVER
 M:	William Breathitt Gray <[email protected]>
 L:	[email protected]
drivers/gpio/Kconfig +9 -0
index ed9e71d6713e..551351e11365 100644
@@ -109,6 +109,15 @@ config GPIO_REGMAP
 config GPIO_MAX730X
 	tristate
 
+config GPIO_IDIO_16
+	tristate
+	help
+	  Enables support for the idio-16 library functions. The idio-16 library
+	  provides functions to facilitate communication with devices within the
+	  ACCES IDIO-16 family such as the 104-IDIO-16 and the PCI-IDIO-16.
+
+	  If built as a module its name will be gpio-idio-16.
+
 menu "Memory mapped GPIO drivers"
 	depends on HAS_IOMEM
drivers/gpio/Makefile +1 -0
index b67e29d348cf..15abf88c167b 100644
@@ -69,6 +69,7 @@ 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_IDIO_16)		+= gpio-idio-16.o
 obj-$(CONFIG_GPIO_IDT3243X)		+= gpio-idt3243x.o
 obj-$(CONFIG_GPIO_IMX_SCU)		+= gpio-imx-scu.o
 obj-$(CONFIG_GPIO_IOP)			+= gpio-iop.o
drivers/gpio/gpio-idio-16.c new file +159 -0

159 lines not on this page · raw message

drivers/gpio/gpio-idio-16.h new file +70 -0

70 lines not on this page · raw message

Signature · 1 line
2.37.2
#2 lore raw

Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module

Quoted 7 lines · William Breathitt Gray

Exposes consumer library functions to facilitate communication with devices within the ACCES IDIO-16 family such as the 104-IDIO-16 and the PCI-IDIO-16.

A CONFIG_GPIO_IDIO_16 Kconfig option is introduced by this patch. Modules wanting access to these idio-16 library functions should select this Kconfig option and import the IDIO_16 symbol namespace.

...

Quoted 4 lines
drivers/gpio/gpio-idio-16.c new file +4 -0
+int idio_16_get(struct idio_16 __iomem *const reg, const unsigned long offset)
+{
+	const unsigned long mask = BIT(offset);
+

drivers/gpio/gpio-idio-16.c line 35

Quoted 10 lines
drivers/gpio/gpio-idio-16.c new file +10 -0
+	if (offset < 8)
+		return !!(ioread8(&reg->out0_7) & mask);
+
+	if (offset < 16)
+		return !!(ioread8(&reg->out8_15) & (mask >> 8));
+
+	if (offset < 24)
+		return !!(ioread8(&reg->in0_7) & (mask >> 16));
+
+	return !!(ioread8(&reg->in8_15) & (mask >> 24));

For the sake of robustness, since it's a library, I would do

	if (offset < 32)
		...

	return -EINVAL;

drivers/gpio/gpio-idio-16.c line 37

Quoted 2 lines
drivers/gpio/gpio-idio-16.c new file +2 -0
+}
+EXPORT_SYMBOL_NS_GPL(idio_16_get, IDIO_16);

If there is not expected to be more than a single namespace, you may define it just once for all via

#define DEFAULT_SYMBOL_NAMESPACE IDIO_16

And honestly, I would add also GPIO prefix to it, GPIO_IDIO_16.

...

drivers/gpio/gpio-idio-16.c line 64

Quoted 8 lines
drivers/gpio/gpio-idio-16.c new file +8 -0
+	if (*mask & GENMASK(7, 0))
+		bitmap_set_value8(bits, ioread8(&reg->out0_7), 0);
+	if (*mask & GENMASK(15, 8))
+		bitmap_set_value8(bits, ioread8(&reg->out8_15), 8);
+	if (*mask & GENMASK(23, 16))
+		bitmap_set_value8(bits, ioread8(&reg->in0_7), 16);
+	if (*mask & GENMASK(31, 24))
+		bitmap_set_value8(bits, ioread8(&reg->in8_15), 24);

So, the addresses of the ports are not expected to be continuous?

...

drivers/gpio/gpio-idio-16.c line 86

Quoted 8 lines
drivers/gpio/gpio-idio-16.c new file +8 -0
+void idio_16_set(struct idio_16 __iomem *const reg,
+		 struct idio_16_state *const state, const unsigned long offset,
+		 const unsigned long value)
+{
+	unsigned long flags;
+
+	/* offsets greater than 15 are input-only signals */
+	if (offset > 15)

For the sake of consistency:

	if (offset >= 16)

drivers/gpio/gpio-idio-16.c line 94

Quoted 8 lines
drivers/gpio/gpio-idio-16.c new file +7 -0
+		return;
+
+	spin_lock_irqsave(&state->lock, flags);
 
+	if (value)
+		set_bit(offset, state->out_state);
+	else
+		clear_bit(offset, state->out_state);

assign_bit()

But I'm wondering why do you need the atomic bitops under the lock?

Quoted 7 lines
drivers/gpio/gpio-idio-16.c new file +7 -0
+	if (offset < 8)
+		iowrite8(bitmap_get_value8(state->out_state, 0), &reg->out0_7);
+	else
+		iowrite8(bitmap_get_value8(state->out_state, 8), &reg->out8_15);
+
+	spin_unlock_irqrestore(&state->lock, flags);
+}

...

drivers/gpio/gpio-idio-16.c line 132

Quoted 6 lines
drivers/gpio/gpio-idio-16.c new file +6 -0
+	for_each_set_clump8(offset, port_mask, mask, IDIO_16_NOUT) {
+		value = bitmap_get_value8(bits, offset);
+		out_state = bitmap_get_value8(state->out_state, offset);
+
+		out_state = (out_state & ~port_mask) | (value & port_mask);
+		bitmap_set_value8(state->out_state, out_state, offset);

This looks like bitmap_replace(). It might require to redesign the flow a bit.

Quoted 5 lines
drivers/gpio/gpio-idio-16.c new file +5 -0
+		if (offset < 8)
+			iowrite8(out_state, &reg->out0_7);
+		else
+			iowrite8(out_state, &reg->out8_15);
+	}

...

drivers/gpio/gpio-idio-16.h line 56

Quoted 3 lines
drivers/gpio/gpio-idio-16.h new file +3 -0
+static inline int idio_16_get_direction(const unsigned long offset)
+{
+	return (offset < IDIO_16_NOUT) ? 0 : 1;

	return (offset >= IDIO_16_NOUT) ? 1 : 0;

?

Quoted 1 line

+}

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#3 lore raw

Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module

Quoted 11 lines · Andy Shevchenko

drivers/gpio/gpio-idio-16.c line 64

Quoted 8 lines · William Breathitt Gray
drivers/gpio/gpio-idio-16.c new file +8 -0
+	if (*mask & GENMASK(7, 0))
+		bitmap_set_value8(bits, ioread8(&reg->out0_7), 0);
+	if (*mask & GENMASK(15, 8))
+		bitmap_set_value8(bits, ioread8(&reg->out8_15), 8);
+	if (*mask & GENMASK(23, 16))
+		bitmap_set_value8(bits, ioread8(&reg->in0_7), 16);
+	if (*mask & GENMASK(31, 24))
+		bitmap_set_value8(bits, ioread8(&reg->in8_15), 24);

So, the addresses of the ports are not expected to be continuous?

No, unfortunately the IDIO-16 devices allocate the FET outputs to byte offsets 0 and 4 while the isolated inputs are allocated to byte offsets 1 and 5. I don't know the design reason for the split but that's the reason I'm reading these addresses by byte rather than by word.

Quoted 4 lines
Quoted 3 lines
drivers/gpio/gpio-idio-16.c new file +3 -0
+		return;
+
+	spin_lock_irqsave(&state->lock, flags);
Quoted 8 lines

drivers/gpio/gpio-idio-16.c line 94

Quoted 4 lines
drivers/gpio/gpio-idio-16.c new file +4 -0
+	if (value)
+		set_bit(offset, state->out_state);
+	else
+		clear_bit(offset, state->out_state);

assign_bit()

But I'm wondering why do you need the atomic bitops under the lock?

I don't think atomic bitops are necessary in this case because of the lock as you pointedly out, but I felt using these made the intention of the code clearer. Is there a non-atomic version of assign_bit(), or do you recommend I use bitwise operations directly here instead?

Quoted 7 lines

drivers/gpio/gpio-idio-16.h line 56

Quoted 3 lines
drivers/gpio/gpio-idio-16.h new file +3 -0
+static inline int idio_16_get_direction(const unsigned long offset)
+{
+	return (offset < IDIO_16_NOUT) ? 0 : 1;

	return (offset >= IDIO_16_NOUT) ? 1 : 0;

?

I have no particular preference in this case, so I can switch this to the >= version for consistency with the rest of the code.

Thanks,

William Breathitt Gray

#4 lore raw

Re: [PATCH 1/3] gpio: idio-16: Introduce the ACCES IDIO-16 GPIO library module

Thu, Sep 15, 2022 at 11:46:13AM -0400, William Breathitt Gray kirjoitti:

Quoted 2 lines · Andy Shevchenko
Quoted 1 line · William Breathitt Gray

...

Quoted 7 lines
Quoted 7 lines

drivers/gpio/gpio-idio-16.c line 94

Quoted 4 lines
drivers/gpio/gpio-idio-16.c new file +4 -0
+	if (value)
+		set_bit(offset, state->out_state);
+	else
+		clear_bit(offset, state->out_state);

assign_bit()

Quoted 6 lines
Quoted 1 line

But I'm wondering why do you need the atomic bitops under the lock?

I don't think atomic bitops are necessary in this case because of the lock as you pointedly out, but I felt using these made the intention of the code clearer. Is there a non-atomic version of assign_bit(), or do you recommend I use bitwise operations directly here instead?

__assign_bit()

Hint: All __ prefixed bitops (for a single bit operation!) are considered non-atomic. There are exceptions when no __-variant of op is present, but it not the case here AFAICS.

Signature · 2 lines
With Best Regards,
Andy Shevchenko
author
#5 lore raw

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

In reply to William Breathitt Gray

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 <[email protected]>
---
 drivers/gpio/Kconfig            |  1 +
 drivers/gpio/gpio-104-idio-16.c | 91 ++++++++-------------------------
 2 files changed, 22 insertions(+), 70 deletions(-)
drivers/gpio/Kconfig +1 -0
index 551351e11365..48846ee476e2 100644
@@ -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
drivers/gpio/gpio-104-idio-16.c +21 -70

164 lines not on this page · raw message

Signature · 1 line
2.37.2
#6 lore raw

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

drivers/gpio/gpio-104-idio-16.c static int idio_16_gpio_direction_output(struct gpio_chip *chip, · line 82

Quoted 7 lines · William Breathitt Gray

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.

drivers/gpio/gpio-104-idio-16.c +2 -0
+	/* 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?

Signature · 2 lines
With Best Regards,
Andy Shevchenko
maintainer
#7 lore raw

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

Quoted 5 lines · Andy Shevchenko
Quoted 3 lines · William Breathitt Gray

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.

Quoted 10 lines

drivers/gpio/gpio-104-idio-16.c static int idio_16_gpio_direction_output(struct gpio_chip *chip, · line 82

Quoted 3 lines
drivers/gpio/gpio-104-idio-16.c +2 -0
+	/* 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?

Signature · 2 lines
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

author
#8 lore raw

PATCH 3/3 gpio: pci-idio-16: Utilize the idio-16 GPIO library

In reply to William Breathitt Gray

The ACCES PCI-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 <[email protected]>
---
 drivers/gpio/Kconfig            |   1 +
 drivers/gpio/gpio-pci-idio-16.c | 119 ++++----------------------------
 2 files changed, 14 insertions(+), 106 deletions(-)
drivers/gpio/Kconfig +1 -0
index 48846ee476e2..8b90bff7b198 100644
@@ -1585,6 +1585,7 @@ config GPIO_PCH
 config GPIO_PCI_IDIO_16
 	tristate "ACCES PCI-IDIO-16 GPIO support"
 	select GPIOLIB_IRQCHIP
+	select GPIO_IDIO_16
 	help
 	  Enables GPIO support for the ACCES PCI-IDIO-16. An interrupt is
 	  generated when any of the inputs change state (low to high or high to
drivers/gpio/gpio-pci-idio-16.c +13 -106

185 lines not on this page · raw message

Signature · 1 line
2.37.2