FTDI FT2232H is a USB to GPIO chip. Sealevel produces some devices
with this chip. FT2232H presents itself as a composite device with two
interfaces (each is an "MPSSE"). Each MPSSE has two banks (high and low)
of 8 GPIO each. I believe some MPSSE's have only one bank, but I don't
know how to identify them (I don't have any for testing) and as a result
are unsupported for the time being.

Additionally, this driver provides software polling-based interrupts for
edge detection. For the Sealevel device I have to test with, this works
well because there is hardware debouncing. From talking to Sealevel's
people, this is their preferred way to do edge detection.

Signed-off-by: Mary Strodl <mstrodl@csh.rit.edu>
---

Changes since last time:

* Make sure we select GPIOLIB_IRQCHIP
* Get rid of unneeded extra irq_chip
* set_bit -> __set_bit
* Actually use GPIOLIB_IRQCHIP for irq stuff
* Remove unnecessary MODULE_ALIAS

 drivers/gpio/Kconfig      |   7 +
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-mpsse.c | 528 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 536 insertions(+)
 create mode 100644 drivers/gpio/gpio-mpsse.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d93cd4f722b4..cbe3baa1b3de 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1844,6 +1844,13 @@ config GPIO_VIPERBOARD
 	  River Tech's viperboard.h for detailed meaning
 	  of the module parameters.
 
+config GPIO_MPSSE
+	tristate "FTDI MPSSE GPIO support"
+	select GPIOLIB_IRQCHIP
+	help
+	  GPIO driver for FTDI's MPSSE interface. These can do input and
+	  output. Each MPSSE provides 16 IO pins.
+
 endmenu
 
 menu "Virtual GPIO drivers"
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 1429e8c0229b..cc5d5519ba4b 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -114,6 +114,7 @@ obj-$(CONFIG_GPIO_MOCKUP)		+= gpio-mockup.o
 obj-$(CONFIG_GPIO_MOXTET)		+= gpio-moxtet.o
 obj-$(CONFIG_GPIO_MPC5200)		+= gpio-mpc5200.o
 obj-$(CONFIG_GPIO_MPC8XXX)		+= gpio-mpc8xxx.o
+obj-$(CONFIG_GPIO_MPSSE)		+= gpio-mpsse.o
 obj-$(CONFIG_GPIO_MSC313)		+= gpio-msc313.o
 obj-$(CONFIG_GPIO_MT7621)		+= gpio-mt7621.o
 obj-$(CONFIG_GPIO_MVEBU)		+= gpio-mvebu.o
diff --git a/drivers/gpio/gpio-mpsse.c b/drivers/gpio/gpio-mpsse.c
new file mode 100644
index 000000000000..ccf21646b32a
--- /dev/null
+++ b/drivers/gpio/gpio-mpsse.c
@@ -0,0 +1,528 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * FTDI MPSSE GPIO support
+ *
+ * Based on code by Anatolij Gustschin
+ *
+ * Copyright (C) 2024 Mary Strodl <mstrodl@csh.rit.edu>
+ */
+
+#include <linux/gpio/driver.h>
+#include <linux/mutex.h>
+#include <linux/usb.h>
+
+struct mpsse_priv {
+	struct gpio_chip gpio;
+	struct usb_device *udev;     /* USB device encompassing all MPSSEs */
+	struct usb_interface *intf;  /* USB interface for this MPSSE */
+	u8 intf_id;                  /* USB interface number for this MPSSE */
+	struct work_struct irq_work; /* polling work thread */
+	struct mutex irq_mutex;	     /* lock over irq_data */
+	atomic_t irq_type[16];	     /* pin -> edge detection type */
+	atomic_t irq_enabled;
+	int id;
+
+	u8 gpio_outputs[2];	     /* Output states for GPIOs [L, H] */
+	u8 gpio_dir[2];		     /* Directions for GPIOs [L, H] */
+
+	u8 *bulk_in_buf;	     /* Extra recv buffer to grab status bytes */
+
+	struct usb_endpoint_descriptor *bulk_in;
+	struct usb_endpoint_descriptor *bulk_out;
+
+	struct mutex io_mutex;	    /* sync I/O with disconnect */
+};
+
+struct bulk_desc {
+	bool tx;	            /* direction of bulk transfer */
+	u8 *data;                   /* input (tx) or output (rx) */
+	int len;                    /* Length of `data` if tx, or length of */
+				    /* Data to read if rx */
+	int len_actual;		    /* Length successfully transferred */
+	int timeout;
+};
+
+static const struct usb_device_id gpio_mpsse_table[] = {
+	{ USB_DEVICE(0x0c52, 0xa064) },   /* SeaLevel Systems, Inc. */
+	{ }                               /* Terminating entry */
+};
+
+MODULE_DEVICE_TABLE(usb, gpio_mpsse_table);
+
+static DEFINE_IDA(gpio_mpsse_ida);
+
+/* MPSSE commands */
+#define SET_BITS_CMD 0x80
+#define GET_BITS_CMD 0x81
+
+#define SET_BITMODE_REQUEST 0x0B
+#define MODE_MPSSE (2 << 8)
+#define MODE_RESET 0
+
+/* Arbitrarily decided. This could probably be much less */
+#define MPSSE_WRITE_TIMEOUT 5000
+#define MPSSE_READ_TIMEOUT 5000
+
+/* 1 millisecond, also pretty arbitrary */
+#define MPSSE_POLL_INTERVAL 1000
+
+static int mpsse_bulk_xfer(struct usb_interface *intf, struct bulk_desc *desc)
+{
+	struct mpsse_priv *priv = usb_get_intfdata(intf);
+	struct usb_device *udev = priv->udev;
+	unsigned int pipe;
+	int ret;
+
+	if (desc->tx)
+		pipe = usb_sndbulkpipe(udev, priv->bulk_out->bEndpointAddress);
+	else
+		pipe = usb_rcvbulkpipe(udev, priv->bulk_in->bEndpointAddress);
+
+	ret = usb_bulk_msg(udev, pipe, desc->data, desc->len,
+			   &desc->len_actual, desc->timeout);
+	if (ret)
+		dev_dbg(&udev->dev, "mpsse: bulk transfer failed: %d\n", ret);
+
+	return ret;
+}
+
+static int mpsse_write(struct usb_interface *intf,
+		       u8 *buf, size_t len)
+{
+	int ret;
+	struct bulk_desc desc;
+
+	desc.len_actual = 0;
+	desc.tx = true;
+	desc.data = buf;
+	desc.len = len;
+	desc.timeout = MPSSE_WRITE_TIMEOUT;
+
+	ret = mpsse_bulk_xfer(intf, &desc);
+
+	return ret;
+}
+
+static int mpsse_read(struct usb_interface *intf, u8 *buf, size_t len)
+{
+	int ret;
+	struct bulk_desc desc;
+	struct mpsse_priv *priv = usb_get_intfdata(intf);
+
+	desc.len_actual = 0;
+	desc.tx = false;
+	desc.data = priv->bulk_in_buf;
+	/* Device sends 2 additional status bytes, read len + 2 */
+	desc.len = min_t(size_t, len + 2, usb_endpoint_maxp(priv->bulk_in));
+	desc.timeout = MPSSE_READ_TIMEOUT;
+
+	ret = mpsse_bulk_xfer(intf, &desc);
+	if (ret)
+		return ret;
+
+	/* Did we get enough data? */
+	if (desc.len_actual < desc.len)
+		return -EIO;
+
+	memcpy(buf, desc.data + 2, desc.len_actual - 2);
+
+	return ret;
+}
+
+static int gpio_mpsse_set_bank(struct mpsse_priv *priv, u8 bank)
+{
+	int ret;
+	u8 tx_buf[3] = {
+		SET_BITS_CMD | (bank << 1),
+		priv->gpio_outputs[bank],
+		priv->gpio_dir[bank],
+	};
+
+	ret = mpsse_write(priv->intf, tx_buf, 3);
+
+	return ret;
+}
+
+static int gpio_mpsse_get_bank(struct mpsse_priv *priv, u8 bank)
+{
+	int ret;
+	u8 buf = GET_BITS_CMD | (bank << 1);
+
+	ret = mpsse_write(priv->intf, &buf, 1);
+	if (ret)
+		return ret;
+
+	ret = mpsse_read(priv->intf, &buf, 1);
+	if (ret)
+		return ret;
+
+	return buf;
+}
+
+static void gpio_mpsse_set_multiple(struct gpio_chip *chip, unsigned long *mask,
+				    unsigned long *bits)
+{
+	unsigned long i, bank, bank_mask, bank_bits;
+	int ret;
+	struct mpsse_priv *priv = gpiochip_get_data(chip);
+
+	mutex_lock(&priv->io_mutex);
+	for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
+		bank = i / 8;
+
+		if (bank_mask) {
+			bank_bits = bitmap_get_value8(bits, i);
+			/* Zero out pins we want to change */
+			priv->gpio_outputs[bank] &= ~bank_mask;
+			/* Set pins we care about */
+			priv->gpio_outputs[bank] |= bank_bits & bank_mask;
+
+			ret = gpio_mpsse_set_bank(priv, bank);
+			if (ret)
+				dev_err(&priv->intf->dev,
+					"Couldn't set values for bank %ld!",
+					bank);
+		}
+	}
+	mutex_unlock(&priv->io_mutex);
+}
+
+static int gpio_mpsse_get_multiple(struct gpio_chip *chip, unsigned long *mask,
+				   unsigned long *bits)
+{
+	unsigned long i, bank, bank_mask;
+	int ret;
+	struct mpsse_priv *priv = gpiochip_get_data(chip);
+
+	mutex_lock(&priv->io_mutex);
+	for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
+		bank = i / 8;
+
+		if (bank_mask) {
+			ret = gpio_mpsse_get_bank(priv, bank);
+			if (ret < 0)
+				return ret;
+
+			bitmap_set_value8(bits, ret & bank_mask, i);
+		}
+	}
+	mutex_unlock(&priv->io_mutex);
+
+	return 0;
+}
+
+static int gpio_mpsse_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	int err;
+	unsigned long mask = 0, bits = 0;
+
+	__set_bit(offset, &mask);
+	err = gpio_mpsse_get_multiple(chip, &mask, &bits);
+	if (err)
+		return err;
+
+	/* == is not guaranteed to give 1 if true */
+	if (bits)
+		return 1;
+	else
+		return 0;
+}
+
+static void gpio_mpsse_gpio_set(struct gpio_chip *chip, unsigned int offset,
+			       int value)
+{
+	unsigned long mask = 0, bits = 0;
+
+	__set_bit(offset, &mask);
+	if (value)
+		__set_bit(offset, &bits);
+
+	gpio_mpsse_set_multiple(chip, &mask, &bits);
+}
+
+static int gpio_mpsse_direction_output(struct gpio_chip *chip,
+				       unsigned int offset, int value)
+{
+	struct mpsse_priv *priv = gpiochip_get_data(chip);
+	int bank = (offset & 8) >> 3;
+	int bank_offset = offset & 7;
+
+	mutex_lock(&priv->io_mutex);
+	priv->gpio_dir[bank] |= BIT(bank_offset);
+	mutex_unlock(&priv->io_mutex);
+	gpio_mpsse_gpio_set(chip, offset, value);
+
+	return 0;
+}
+
+static int gpio_mpsse_direction_input(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	struct mpsse_priv *priv = gpiochip_get_data(chip);
+	int bank = (offset & 8) >> 3;
+	int bank_offset = offset & 7;
+
+	mutex_lock(&priv->io_mutex);
+	priv->gpio_dir[bank] &= ~BIT(bank_offset);
+	gpio_mpsse_set_bank(priv, bank);
+	mutex_unlock(&priv->io_mutex);
+
+	return 0;
+}
+
+static int gpio_mpsse_get_direction(struct gpio_chip *chip,
+				    unsigned int offset)
+{
+	int ret;
+	int bank = (offset & 8) >> 3;
+	int bank_offset = offset & 7;
+	struct mpsse_priv *priv = gpiochip_get_data(chip);
+
+	mutex_lock(&priv->io_mutex);
+	/* MPSSE directions are inverted */
+	if (priv->gpio_dir[bank] & BIT(bank_offset))
+		ret = 0;
+	else
+		ret = 1;
+	mutex_unlock(&priv->io_mutex);
+
+	return ret;
+}
+
+static void gpio_mpsse_poll(struct work_struct *work)
+{
+	unsigned long pin_mask, pin_states, flags;
+	int irq_enabled, offset, err, value, fire_irq,
+		irq, old_value[16], irq_type[16];
+	struct mpsse_priv *priv = container_of(work, struct mpsse_priv,
+					       irq_work);
+
+	for (offset = 0; offset < priv->gpio.ngpio; ++offset)
+		old_value[offset] = -1;
+
+	while ((irq_enabled = atomic_read(&priv->irq_enabled))) {
+		mutex_lock(&priv->irq_mutex);
+
+		pin_mask = 0;
+		pin_states = 0;
+		for (offset = 0; offset < priv->gpio.ngpio; ++offset) {
+			irq_type[offset] = atomic_read(&priv->irq_type[offset]);
+			if (irq_type[offset] != IRQ_TYPE_NONE &&
+			    irq_enabled & BIT(offset))
+				pin_mask |= BIT(offset);
+			else
+				old_value[offset] = -1;
+		}
+
+		err = gpio_mpsse_get_multiple(&priv->gpio, &pin_mask,
+					      &pin_states);
+		if (err)
+			dev_err_ratelimited(&priv->intf->dev,
+					    "Error polling!\n");
+
+		/* Check each value */
+		for (offset = 0; offset < priv->gpio.ngpio; ++offset) {
+			if (old_value[offset] == -1)
+				continue;
+
+			fire_irq = 0;
+			value = pin_states & BIT(offset);
+
+			switch (irq_type[offset]) {
+			case IRQ_TYPE_EDGE_RISING:
+				fire_irq = value > old_value[offset];
+				break;
+			case IRQ_TYPE_EDGE_FALLING:
+				fire_irq = value < old_value[offset];
+				break;
+			case IRQ_TYPE_EDGE_BOTH:
+				fire_irq = value != old_value[offset];
+				break;
+			}
+			if (!fire_irq)
+				continue;
+
+			irq = irq_find_mapping(priv->gpio.irq.domain,
+					       offset);
+			local_irq_save(flags);
+			generic_handle_irq(irq);
+			local_irq_disable();
+			local_irq_restore(flags);
+		}
+
+		/* Sync back values so we can refer to them next tick */
+		for (offset = 0; offset < priv->gpio.ngpio; ++offset)
+			if (irq_type[offset] != IRQ_TYPE_NONE &&
+			    irq_enabled & BIT(offset))
+				old_value[offset] = pin_states & BIT(offset);
+
+		mutex_unlock(&priv->irq_mutex);
+		usleep_range(MPSSE_POLL_INTERVAL, MPSSE_POLL_INTERVAL + 1000);
+	}
+}
+
+static int gpio_mpsse_set_irq_type(struct irq_data *irqd, unsigned int type)
+{
+	int offset;
+	struct mpsse_priv *priv = irq_data_get_irq_chip_data(irqd);
+
+	offset = irqd->hwirq;
+	atomic_set(&priv->irq_type[offset], type & IRQ_TYPE_EDGE_BOTH);
+
+	return 0;
+}
+
+static void gpio_mpsse_irq_disable(struct irq_data *irqd)
+{
+	struct mpsse_priv *priv = irq_data_get_irq_chip_data(irqd);
+
+	atomic_and(~BIT(irqd->hwirq), &priv->irq_enabled);
+	gpiochip_disable_irq(&priv->gpio, irqd->hwirq);
+}
+
+static void gpio_mpsse_irq_enable(struct irq_data *irqd)
+{
+	struct mpsse_priv *priv = irq_data_get_irq_chip_data(irqd);
+
+	gpiochip_enable_irq(&priv->gpio, irqd->hwirq);
+	/* If no-one else was using the IRQ, enable it */
+	if (!atomic_fetch_or(BIT(irqd->hwirq), &priv->irq_enabled)) {
+		INIT_WORK(&priv->irq_work, gpio_mpsse_poll);
+		schedule_work(&priv->irq_work);
+	}
+}
+
+static const struct irq_chip gpio_mpsse_irq_chip = {
+	.name = "gpio-mpsse-irq",
+	.irq_enable = gpio_mpsse_irq_enable,
+	.irq_disable = gpio_mpsse_irq_disable,
+	.irq_set_type = gpio_mpsse_set_irq_type,
+	.flags = IRQCHIP_IMMUTABLE,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static int gpio_mpsse_probe(struct usb_interface *interface,
+			    const struct usb_device_id *id)
+{
+	struct mpsse_priv *priv;
+	struct device *dev;
+	int err;
+
+	dev = &interface->dev;
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->udev = usb_get_dev(interface_to_usbdev(interface));
+	priv->intf = interface;
+	priv->intf_id = interface->cur_altsetting->desc.bInterfaceNumber;
+
+	priv->id = ida_simple_get(&gpio_mpsse_ida, 0, 0, GFP_KERNEL);
+	if (priv->id < 0)
+		return priv->id;
+
+	devm_mutex_init(dev, &priv->io_mutex);
+	devm_mutex_init(dev, &priv->irq_mutex);
+
+	priv->gpio.label = devm_kasprintf(dev, GFP_KERNEL,
+					  "gpio-mpsse.%d.%d",
+					  priv->id, priv->intf_id);
+	if (!priv->gpio.label) {
+		err = -ENOMEM;
+		goto err;
+	}
+
+	priv->gpio.owner = THIS_MODULE;
+	priv->gpio.parent = interface->usb_dev;
+	priv->gpio.get_direction = gpio_mpsse_get_direction;
+	priv->gpio.direction_input = gpio_mpsse_direction_input;
+	priv->gpio.direction_output = gpio_mpsse_direction_output;
+	priv->gpio.get = gpio_mpsse_gpio_get;
+	priv->gpio.set = gpio_mpsse_gpio_set;
+	priv->gpio.get_multiple = gpio_mpsse_get_multiple;
+	priv->gpio.set_multiple = gpio_mpsse_set_multiple;
+	priv->gpio.base = -1;
+	priv->gpio.ngpio = 16;
+	priv->gpio.offset = priv->intf_id * priv->gpio.ngpio;
+	priv->gpio.can_sleep = 1;
+
+	err = usb_find_common_endpoints(interface->cur_altsetting,
+					&priv->bulk_in, &priv->bulk_out,
+					NULL, NULL);
+	if (err)
+		goto err;
+
+	priv->bulk_in_buf = devm_kmalloc(dev, usb_endpoint_maxp(priv->bulk_in),
+					 GFP_KERNEL);
+	if (!priv->bulk_in_buf) {
+		err = -ENOMEM;
+		goto err;
+	}
+
+	usb_set_intfdata(interface, priv);
+
+
+	/* Reset mode, needed to correctly enter MPSSE mode */
+	err = usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
+			      SET_BITMODE_REQUEST,
+			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+			      MODE_RESET, priv->intf_id + 1, NULL, 0,
+			      USB_CTRL_SET_TIMEOUT);
+	if (err)
+		goto err;
+
+	/* Enter MPSSE mode */
+	err = usb_control_msg(priv->udev, usb_sndctrlpipe(priv->udev, 0),
+			      SET_BITMODE_REQUEST,
+			      USB_TYPE_VENDOR | USB_RECIP_DEVICE | USB_DIR_OUT,
+			      MODE_MPSSE, priv->intf_id + 1, NULL, 0,
+			      USB_CTRL_SET_TIMEOUT);
+	if (err)
+		goto err;
+
+	gpio_irq_chip_set_chip(&priv->gpio.irq, &gpio_mpsse_irq_chip);
+
+	priv->gpio.irq.parent_handler = NULL;
+	priv->gpio.irq.num_parents = 0;
+	priv->gpio.irq.parents = NULL;
+	priv->gpio.irq.default_type = IRQ_TYPE_NONE;
+	priv->gpio.irq.handler = handle_simple_irq;
+
+	err = devm_gpiochip_add_data(dev, &priv->gpio, priv);
+	if (err)
+		goto err_irq_domain;
+
+	return 0;
+
+err_irq_domain:
+	irq_domain_remove(priv->gpio.irq.domain);
+
+err:
+	ida_simple_remove(&gpio_mpsse_ida, priv->id);
+
+	return err;
+}
+
+static void gpio_mpsse_disconnect(struct usb_interface *intf)
+{
+	struct mpsse_priv *priv = usb_get_intfdata(intf);
+
+	ida_simple_remove(&gpio_mpsse_ida, priv->id);
+
+	priv->intf = NULL;
+	usb_set_intfdata(intf, NULL);
+	usb_put_dev(priv->udev);
+}
+
+static struct usb_driver gpio_mpsse_driver = {
+	.name           = "gpio-mpsse",
+	.probe          = gpio_mpsse_probe,
+	.disconnect     = gpio_mpsse_disconnect,
+	.id_table       = gpio_mpsse_table,
+};
+
+module_usb_driver(gpio_mpsse_driver);
+
+MODULE_AUTHOR("Mary Strodl <mstrodl@csh.rit.edu>");
+MODULE_DESCRIPTION("MPSSE GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.45.2

Hi Mary,

kernel test robot noticed the following build warnings:

https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Mary-Strodl/gpio-add-support-for-FTDI-s-MPSSE-as-GPIO/20240919-221626
base:   https://git.kernel.org/pub/scm/linux/kernel/git/brgl/linux.git gpio/for-next
patch link:    https://lore.kernel.org/r/20240919141014.4000958-1-mstrodl%40csh.rit.edu
patch subject: [PATCH v2] gpio: add support for FTDI's MPSSE as GPIO
config: x86_64-randconfig-161-20240922 (https://download.01.org/0day-ci/archive/20240923/202409230158.XhvhLOyY-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
| Closes: https://lore.kernel.org/r/202409230158.XhvhLOyY-lkp@intel.com/

smatch warnings:
drivers/gpio/gpio-mpsse.c:211 gpio_mpsse_get_multiple() warn: inconsistent returns '&priv->io_mutex'.

vim +211 drivers/gpio/gpio-mpsse.c

be777399e9baea Mary Strodl 2024-09-19  190  static int gpio_mpsse_get_multiple(struct gpio_chip *chip, unsigned long *mask,
be777399e9baea Mary Strodl 2024-09-19  191  				   unsigned long *bits)
be777399e9baea Mary Strodl 2024-09-19  192  {
be777399e9baea Mary Strodl 2024-09-19  193  	unsigned long i, bank, bank_mask;
be777399e9baea Mary Strodl 2024-09-19  194  	int ret;
be777399e9baea Mary Strodl 2024-09-19  195  	struct mpsse_priv *priv = gpiochip_get_data(chip);
be777399e9baea Mary Strodl 2024-09-19  196  
be777399e9baea Mary Strodl 2024-09-19  197  	mutex_lock(&priv->io_mutex);
be777399e9baea Mary Strodl 2024-09-19  198  	for_each_set_clump8(i, bank_mask, mask, chip->ngpio) {
be777399e9baea Mary Strodl 2024-09-19  199  		bank = i / 8;
be777399e9baea Mary Strodl 2024-09-19  200  
be777399e9baea Mary Strodl 2024-09-19  201  		if (bank_mask) {
be777399e9baea Mary Strodl 2024-09-19  202  			ret = gpio_mpsse_get_bank(priv, bank);
be777399e9baea Mary Strodl 2024-09-19  203  			if (ret < 0)
be777399e9baea Mary Strodl 2024-09-19  204  				return ret;

Needs to mutex_unlock(&priv->io_mutex) before returning.

be777399e9baea Mary Strodl 2024-09-19  205  
be777399e9baea Mary Strodl 2024-09-19  206  			bitmap_set_value8(bits, ret & bank_mask, i);
be777399e9baea Mary Strodl 2024-09-19  207  		}
be777399e9baea Mary Strodl 2024-09-19  208  	}
be777399e9baea Mary Strodl 2024-09-19  209  	mutex_unlock(&priv->io_mutex);
be777399e9baea Mary Strodl 2024-09-19  210  
be777399e9baea Mary Strodl 2024-09-19 @211  	return 0;
be777399e9baea Mary Strodl 2024-09-19  212  }

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

On Thu, Sep 19, 2024 at 4:10 PM Mary Strodl <mstrodl@csh.rit.edu> wrote:

> FTDI FT2232H is a USB to GPIO chip.

Just came to think about:

> +       priv->gpio.owner = THIS_MODULE;
> +       priv->gpio.parent = interface->usb_dev;
> +       priv->gpio.get_direction = gpio_mpsse_get_direction;
> +       priv->gpio.direction_input = gpio_mpsse_direction_input;
> +       priv->gpio.direction_output = gpio_mpsse_direction_output;
> +       priv->gpio.get = gpio_mpsse_gpio_get;
> +       priv->gpio.set = gpio_mpsse_gpio_set;
> +       priv->gpio.get_multiple = gpio_mpsse_get_multiple;
> +       priv->gpio.set_multiple = gpio_mpsse_set_multiple;
> +       priv->gpio.base = -1;
> +       priv->gpio.ngpio = 16;
> +       priv->gpio.offset = priv->intf_id * priv->gpio.ngpio;
> +       priv->gpio.can_sleep = 1;

Maybe you want to provide the gpio.names array for this
device?

It makes it easier to use the lines from userspace if they
have meaningful names, it looks like those may be printed
on the board on the Sealevel device.

Yours,
Linus Walleij

On Mon, Sep 23, 2024 at 12:44:15AM +0200, Linus Walleij wrote:
> Maybe you want to provide the gpio.names array for this
> device?
> 
> It makes it easier to use the lines from userspace if they
> have meaningful names, it looks like those may be printed
> on the board on the Sealevel device.

There are a bunch of devices using this chip in very different ways,
so I didn't want to tie it too closely to this specific device.
Also this device kinda doesn't have a good way to label the pins.
Part of the issue is that sealevel hasn't given us a schematic of how
the chip is connected, I've had to figure stuff out by trial and error... :S

> Yours,
> Linus Walleij

Thanks for reviewing! I'll have a new patch out soon with the feedback
from your previous email addressed :)

Hi Mary,

thanks for your patch!

On Thu, Sep 19, 2024 at 4:10 PM Mary Strodl <mstrodl@csh.rit.edu> wrote:

> FTDI FT2232H is a USB to GPIO chip. Sealevel produces some devices
> with this chip. FT2232H presents itself as a composite device with two
> interfaces (each is an "MPSSE"). Each MPSSE has two banks (high and low)
> of 8 GPIO each. I believe some MPSSE's have only one bank, but I don't
> know how to identify them (I don't have any for testing) and as a result
> are unsupported for the time being.
>
> Additionally, this driver provides software polling-based interrupts for
> edge detection. For the Sealevel device I have to test with, this works
> well because there is hardware debouncing. From talking to Sealevel's
> people, this is their preferred way to do edge detection.
>
> Signed-off-by: Mary Strodl <mstrodl@csh.rit.edu>

Interesting device! Is it working nicely with hotplug
and using libgpiod for userspace access?

Overall this looks very good, some comments:

> +static int gpio_mpsse_get_direction(struct gpio_chip *chip,
> +                                   unsigned int offset)
> +{
> +       int ret;
> +       int bank = (offset & 8) >> 3;
> +       int bank_offset = offset & 7;
> +       struct mpsse_priv *priv = gpiochip_get_data(chip);
> +
> +       mutex_lock(&priv->io_mutex);

In all instances of mutex, I suggest using the new guarded
mutexes from <linux/cleanup.h>. I this case just

guard(mutex)(&priv->io_mutex);

And then the mutex will be dropped when the function exits.
Tighter scopes are possible, just:
git grep guard drivers/gpio/
for a ton of examples.

> +       /* MPSSE directions are inverted */
> +       if (priv->gpio_dir[bank] & BIT(bank_offset))
> +               ret = 0;
> +       else
> +               ret = 1;

Please use the new defines from <linux/gpio/driver.h>:
#define GPIO_LINE_DIRECTION_IN  1
#define GPIO_LINE_DIRECTION_OUT 0

With the above addressed:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij