diff mbox series

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

Message ID 6b28fb497c35def57c1920362c82402bed4bd23f.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 
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 <william.gray@linaro.org>
---
 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

Comments

Andy Shevchenko Sept. 13, 2022, 4:16 p.m. UTC | #1 
On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:
> 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.

...

> +int idio_16_get(struct idio_16 __iomem *const reg, const unsigned long offset)
> +{
> +	const unsigned long mask = BIT(offset);
> +
> +	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;

> +}
> +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.

...

> +	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?

...

> +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)

> +		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?

> +	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);
> +}

...

> +	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.

> +		if (offset < 8)
> +			iowrite8(out_state, &reg->out0_7);
> +		else
> +			iowrite8(out_state, &reg->out8_15);
> +	}

...

> +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;

?

> +}
William Breathitt Gray Sept. 15, 2022, 3:46 p.m. UTC | #2 
On Tue, Sep 13, 2022 at 07:16:23PM +0300, Andy Shevchenko wrote:
> On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:
> > +	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.

> > +		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?

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?

> > +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
Andy Shevchenko Sept. 17, 2022, 8:59 a.m. UTC | #3 
Thu, Sep 15, 2022 at 11:46:13AM -0400, William Breathitt Gray kirjoitti:
> On Tue, Sep 13, 2022 at 07:16:23PM +0300, Andy Shevchenko wrote:
> > On Sun, Sep 11, 2022 at 04:34:38PM -0400, William Breathitt Gray wrote:

...

> > > +	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?

__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.
diff mbox series

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index 8a5012ba6ff9..a143d4bc8547 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -312,6 +312,12 @@  L:	linux-iio@vger.kernel.org
 S:	Maintained
 F:	drivers/counter/104-quad-8.c
 
+ACCES IDIO-16 GPIO LIBRARY
+M:	William Breathitt Gray <william.gray@linaro.org>
+L:	linux-gpio@vger.kernel.org
+S:	Maintained
+F:	drivers/gpio/gpio-idio-16.c
+
 ACCES PCI-IDIO-16 GPIO DRIVER
 M:	William Breathitt Gray <william.gray@linaro.org>
 L:	linux-gpio@vger.kernel.org
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index ed9e71d6713e..551351e11365 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -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
 
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index b67e29d348cf..15abf88c167b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -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
diff --git a/drivers/gpio/gpio-idio-16.c b/drivers/gpio/gpio-idio-16.c
new file mode 100644
index 000000000000..e244032bac35
--- /dev/null
+++ b/drivers/gpio/gpio-idio-16.c
@@ -0,0 +1,159 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO library for the ACCES IDIO-16 family
+ * Copyright (C) 2022 William Breathitt Gray
+ */
+#include <linux/bitmap.h>
+#include <linux/export.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#include "gpio-idio-16.h"
+
+/**
+ * idio_16_get - get signal value at signal offset
+ * @reg:	ACCES IDIO-16 device registers
+ * @offset:	offset of signal to get
+ *
+ * Returns the signal value (0=low, 1=high) for the signal at @offset.
+ */
+int idio_16_get(struct idio_16 __iomem *const reg, const unsigned long offset)
+{
+	const unsigned long mask = BIT(offset);
+
+	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));
+}
+EXPORT_SYMBOL_NS_GPL(idio_16_get, IDIO_16);
+
+/**
+ * idio_16_get_multiple - get multiple signal values at multiple signal offsets
+ * @reg:	ACCES IDIO-16 device registers
+ * @state:	ACCES IDIO-16 device state
+ * @mask:	mask of signals to get
+ * @bits:	bitmap to store signal values
+ *
+ * Stores in @bits the values (0=low, 1=high) for the signals defined by @mask.
+ */
+void idio_16_get_multiple(struct idio_16 __iomem *const reg,
+			  struct idio_16_state *const state,
+			  const unsigned long *const mask,
+			  unsigned long *const bits)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&state->lock, flags);
+
+	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);
+
+	spin_unlock_irqrestore(&state->lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(idio_16_get_multiple, IDIO_16);
+
+/**
+ * idio_16_set - set signal value at signal offset
+ * @reg:	ACCES IDIO-16 device registers
+ * @state:	ACCES IDIO-16 device state
+ * @offset:	offset of signal to set
+ * @value:	value of signal to set
+ *
+ * Assigns output @value for the signal at @offset.
+ */
+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)
+		return;
+
+	spin_lock_irqsave(&state->lock, flags);
+
+	if (value)
+		set_bit(offset, state->out_state);
+	else
+		clear_bit(offset, state->out_state);
+
+	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);
+}
+EXPORT_SYMBOL_NS_GPL(idio_16_set, IDIO_16);
+
+/**
+ * idio_16_set_multiple - set signal values at multiple signal offsets
+ * @reg:	ACCES IDIO-16 device registers
+ * @state:	ACCES IDIO-16 device state
+ * @mask:	mask of signals to set
+ * @bits:	bitmap of signal output values
+ *
+ * Assigns output values defined by @bits for the signals defined by @mask.
+ */
+void idio_16_set_multiple(struct idio_16 __iomem *const reg,
+			  struct idio_16_state *const state,
+			  const unsigned long *const mask,
+			  const unsigned long *const bits)
+{
+	unsigned long flags;
+	unsigned long offset;
+	unsigned long port_mask;
+	unsigned long value;
+	unsigned long out_state;
+
+	spin_lock_irqsave(&state->lock, flags);
+
+	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);
+
+		if (offset < 8)
+			iowrite8(out_state, &reg->out0_7);
+		else
+			iowrite8(out_state, &reg->out8_15);
+	}
+
+	spin_unlock_irqrestore(&state->lock, flags);
+}
+EXPORT_SYMBOL_NS_GPL(idio_16_set_multiple, IDIO_16);
+
+/**
+ * idio_16_state_init - initialize idio_16_state structure
+ * @state:	ACCES IDIO-16 device state
+ *
+ * Initializes the ACCES IDIO-16 device @state for use in idio-16 library
+ * functions.
+ */
+void idio_16_state_init(struct idio_16_state *const state)
+{
+	spin_lock_init(&state->lock);
+}
+EXPORT_SYMBOL_NS_GPL(idio_16_state_init, IDIO_16);
+
+MODULE_AUTHOR("William Breathitt Gray");
+MODULE_DESCRIPTION("ACCES IDIO-16 GPIO Library");
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpio/gpio-idio-16.h b/drivers/gpio/gpio-idio-16.h
new file mode 100644
index 000000000000..ce1fa0639243
--- /dev/null
+++ b/drivers/gpio/gpio-idio-16.h
@@ -0,0 +1,70 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright 2022 William Breathitt Gray */
+#ifndef _IDIO_16_H_
+#define _IDIO_16_H_
+
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+/**
+ * struct idio_16 - IDIO-16 registers structure
+ * @out0_7:	Read: FET Drive Outputs 0-7
+ *		Write: FET Drive Outputs 0-7
+ * @in0_7:	Read: Isolated Inputs 0-7
+ *		Write: Clear Interrupt
+ * @irq_ctl:	Read: Enable IRQ
+ *		Write: Disable IRQ
+ * @filter_ctl:	Read: Activate Input Filters 0-15
+ *		Write: Deactivate Input Filters 0-15
+ * @out8_15:	Read: FET Drive Outputs 8-15
+ *		Write: FET Drive Outputs 8-15
+ * @in8_15:	Read: Isolated Inputs 8-15
+ *		Write: Unused
+ * @irq_status:	Read: Interrupt status
+ *		Write: Unused
+ */
+struct idio_16 {
+	u8 out0_7;
+	u8 in0_7;
+	u8 irq_ctl;
+	u8 filter_ctl;
+	u8 out8_15;
+	u8 in8_15;
+	u8 irq_status;
+};
+
+#define IDIO_16_NOUT 16
+
+/**
+ * struct idio_16_state - IDIO-16 state structure
+ * @lock:	synchronization lock for accessing device state
+ * @out_state:	output signals state
+ */
+struct idio_16_state {
+	spinlock_t lock;
+	DECLARE_BITMAP(out_state, IDIO_16_NOUT);
+};
+
+/**
+ * idio_16_get_direction - get the I/O direction for a signal offset
+ * @offset:	offset of signal to get direction
+ *
+ * Returns the signal direction (0=output, 1=input) for the signal at @offset.
+ */
+static inline int idio_16_get_direction(const unsigned long offset)
+{
+	return (offset < IDIO_16_NOUT) ? 0 : 1;
+}
+
+int idio_16_get(struct idio_16 __iomem *reg, unsigned long offset);
+void idio_16_get_multiple(struct idio_16 __iomem *reg,
+			  struct idio_16_state *state,
+			  const unsigned long *mask, unsigned long *bits);
+void idio_16_set(struct idio_16 __iomem *reg, struct idio_16_state *state,
+		 unsigned long offset, unsigned long value);
+void idio_16_set_multiple(struct idio_16 __iomem *reg,
+			  struct idio_16_state *state,
+			  const unsigned long *mask, const unsigned long *bits);
+void idio_16_state_init(struct idio_16_state *state);
+
+#endif /* _IDIO_16_H_ */