[PATCH v5 0/3] WCH CH341 GPIO and SPI support

[PATCH v5 0/3] WCH CH341 GPIO and SPI support

[frank zago]

The CH341 is a multifunction chip, presenting 3 different USB PID. One
of these functions is for I2C/SPI/GPIO. This new set of drivers will
manage I2C and GPIO.

Changes from v4:
I should have addressed all the comments: rework of the GPIO interrupt
handling code to be more modern, changes in Kconfig wording, some code
cleanup.
Driver was tested again with up to 4 of these devices. No
error seen.

Changes from v3:
  - really converted to an MFD driver. Driver is now split into 3
    modules (MFD+I2C+GPIO).
  - minor code cleanups

Changes from v2:
  - bug fixes
  - more robust USB enumeration
  - Changed to an MFD driver as suggested


frank zago (3):
  mfd: ch341: add core driver for the WCH CH341 in I2C/SPI/GPIO mode
  gpio: ch341: add GPIO MFD cell driver for the CH341
  i2c: ch341: add I2C MFD cell driver for the CH341

 Documentation/misc-devices/ch341.rst | 114 ++++++++
 Documentation/misc-devices/index.rst |   1 +
 MAINTAINERS                          |   9 +
 drivers/gpio/Kconfig                 |  10 +
 drivers/gpio/Makefile                |   1 +
 drivers/gpio/gpio-ch341.c            | 383 +++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig           |  10 +
 drivers/i2c/busses/Makefile          |   1 +
 drivers/i2c/busses/i2c-ch341.c       | 331 +++++++++++++++++++++++
 drivers/mfd/Kconfig                  |  10 +
 drivers/mfd/Makefile                 |   1 +
 drivers/mfd/ch341-core.c             | 105 ++++++++
 include/linux/mfd/ch341.h            |  26 ++
 13 files changed, 1002 insertions(+)
 create mode 100644 Documentation/misc-devices/ch341.rst
 create mode 100644 drivers/gpio/gpio-ch341.c
 create mode 100644 drivers/i2c/busses/i2c-ch341.c
 create mode 100644 drivers/mfd/ch341-core.c
 create mode 100644 include/linux/mfd/ch341.h

--
2.32.0

[PATCH v5 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/SPI/GPIO mode

[frank zago]

The CH341 is a multifunction chip, presenting 3 different USB PID. One
of these functions is for I2C/SPI/GPIO. This new set of drivers will
manage I2C and GPIO.

Signed-off-by: frank zago <frank@zago.net>
---
 Documentation/misc-devices/ch341.rst | 114 +++++++++++++++++++++++++++
 Documentation/misc-devices/index.rst |   1 +
 MAINTAINERS                          |   7 ++
 drivers/mfd/Kconfig                  |  10 +++
 drivers/mfd/Makefile                 |   1 +
 drivers/mfd/ch341-core.c             |  99 +++++++++++++++++++++++
 include/linux/mfd/ch341.h            |  26 ++++++
 7 files changed, 258 insertions(+)
 create mode 100644 Documentation/misc-devices/ch341.rst
 create mode 100644 drivers/mfd/ch341-core.c
 create mode 100644 include/linux/mfd/ch341.h

diff --git a/Documentation/misc-devices/ch341.rst b/Documentation/misc-devices/ch341.rst
new file mode 100644
index 000000000000..bf0b83f2eb85
--- /dev/null
+++ b/Documentation/misc-devices/ch341.rst
@@ -0,0 +1,114 @@
+.. SPDX-License-Identifier: GPL-2.0-or-later
+
+===========================================================
+WinChipHead (沁恒) CH341 linux driver for I2C and GPIO mode
+===========================================================
+
+The CH341 is declined in several flavors, and may support one or more
+of UART, SPI, I2C and GPIO, but not always simultaneously:
+
+  - CH341 A/B/F: UART, Printer, SPI, I2C and GPIO
+  - CH341 C/T: UART and I2C
+  - CH341 H: SPI
+
+They work in 3 different modes, with only one being presented
+depending on the USB PID:
+
+  - 0x5523: UART mode, covered by the USB `ch341` serial driver
+  - 0x5512: SPI/I2C/GPIO mode, covered by the ch341 MFD drivers
+  - 0x5584: Parallel printer mode, covered by the USB `usblp` driver
+
+Mode selection is done at the hardware level by tying some
+pins. Breakout boards with one of the CH341 chip usually have one or
+more jumpers to select which mode they work on. At least one model
+(CJMCU-341) appears to need bridging some solder pads to select a
+different default. Breakout boards also don't usually offer an option
+to configure the chip into printer mode; for that case, connect the
+SCL and SDA lines directly together.
+
+The various CH341 appear to be indistinguishable from the
+software. For instance the ch341 MFD driver will present a GPIO
+interface for the CH341T although physical pins are not present, and
+the device will accept GPIO commands.
+
+The ch341 MFD driver has been tested with a CH341A, CH341B and
+CH341T.
+
+Some breakout boards work in 3.3V and 5V depending on some jumpers.
+
+The black chip programmer with a ZIF socket will power the CH341 at
+3.3V if a jumper is set, but will only output 5V to the chips to be
+programmed, which is not always desirable. A hardware hack to use 3.3V
+everywhere, involving some soldering, is available at
+https://eevblog.com/forum/repair/ch341a-serial-memory-programmer-power-supply-fix/
+
+Some sample code for the CH341 is available at the manufacturer
+website, at http://wch-ic.com/products/CH341.html
+
+The repository at https://github.com/boseji/CH341-Store contains a lot
+of information on these chips, including datasheets.
+
+This driver is based on the pre-existing work at
+https://github.com/gschorcht/i2c-ch341-usb
+
+
+I2C Caveats
+-----------
+
+The ch341 doesn't work with a Wii nunchuk, possibly because the
+pull-up value is too low (1500 ohms).
+
+i2c AT24 eeproms can be read but not programmed properly because the
+at24 linux driver tries to write a byte at a time, and doesn't wait at
+all (or enough) between writes. Data corruption on writes does occur.
+
+The driver doesn't support detection of I2C devices present on the
+bus. Apparently when a device is not present at a given address, the
+CH341 will return an extra byte of data, but the driver doesn't
+support that. This may be addressed in the future.
+
+
+The GPIOs
+---------
+
+16 GPIOs are available on the CH341 A/B/F. The first 6 are input/output,
+and the last 10 are input only.
+
+Pinout and their names as they appear on some breakout boards::
+
+  CH341A/B/F     GPIO  Names                    Mode
+    pin          line
+
+   15             0     D0, CS0                  input/output
+   16             1     D1, CS1                  input/output
+   17             2     D2, CS2                  input/output
+   18             3     D3, SCK, DCK             input/output
+   19             4     D4, DOUT2, CS3           input/output
+   20             5     D5, MOSI, DOUT, SDO      input/output
+   21             6     D6, DIN2                 input
+   22             7     D7, MISO, DIN            input
+    5             8     ERR                      input
+    6             9     PEMP                     input
+    7            10     INT                      input
+    8            11     SLCT (SELECT)            input
+   26            12     RST# (?)                 input
+   27            13     WT (WAIT)                input
+    4            14     DS (Data Select?)        input
+    3            15     AS (Address Select?)     input
+
+
+GPIO interrupt
+~~~~~~~~~~~~~~
+
+The INT pin, corresponding to GPIO 10 is an input pin that can trigger
+an interrupt on a rising edge. Only that pin is able to generate an
+interrupt, and only on a rising edge. Trying to monitor events on
+another GPIO, or that GPIO on something other than a rising edge, will
+be rejected.
+
+
+SPI
+---
+
+This driver doesn't offer an SPI interface (yet) due to the
+impossibility of declaring an SPI device like I2C does.
diff --git a/Documentation/misc-devices/index.rst b/Documentation/misc-devices/index.rst
index 30ac58f81901..52d03715601e 100644
--- a/Documentation/misc-devices/index.rst
+++ b/Documentation/misc-devices/index.rst
@@ -19,6 +19,7 @@ fit into other categories.
    bh1770glc
    eeprom
    c2port
+   ch341
    dw-xdata-pcie
    ibmvmc
    ics932s401
diff --git a/MAINTAINERS b/MAINTAINERS
index cbbd3ce7e0c2..b61af813fb9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21211,6 +21211,13 @@ M:	David Härdeman <david@hardeman.nu>
 S:	Maintained
 F:	drivers/media/rc/winbond-cir.c
 
+WINCHIPHEAD CH341 I2C/GPIO MFD DRIVER
+M:	Frank Zago <frank@zago.net>
+L:	linux-usb@vger.kernel.org
+S:	Maintained
+F:	drivers/mfd/ch341-core.c
+F:	include/linux/mfd/ch341.h
+
 WINSYSTEMS EBC-C384 WATCHDOG DRIVER
 M:	William Breathitt Gray <vilhelm.gray@gmail.com>
 L:	linux-watchdog@vger.kernel.org
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 3b59456f5545..893acc821a42 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1784,6 +1784,16 @@ config MFD_LOCHNAGAR
 	help
 	  Support for Cirrus Logic Lochnagar audio development board.
 
+config MFD_CH341
+	tristate "WinChipHead CH341 in I2C/SPI/GPIO mode"
+	depends on USB
+	help
+	  If you say yes to this option, support for the CH341 series
+	  of chips, running in I2C/SPI/GPIO mode will be included.
+
+	  This driver can also be built as a module.  If so, the
+	  module will be called ch341-core.
+
 config MFD_ARIZONA
 	select REGMAP
 	select REGMAP_IRQ
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 858cacf659d6..fd615ab3929f 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
 obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
 obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
 obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
+obj-$(CONFIG_MFD_CH341)		+= ch341-core.o
 obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
 obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
 obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
diff --git a/drivers/mfd/ch341-core.c b/drivers/mfd/ch341-core.c
new file mode 100644
index 000000000000..0bb6eb8057e9
--- /dev/null
+++ b/drivers/mfd/ch341-core.c
@@ -0,0 +1,99 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Core driver for the CH341A, CH341B and CH341T in I2C/SPI/GPIO
+ * mode. There are cell drivers available for I2C and GPIO. SPI is not
+ * yet supported.
+ *
+ * Copyright 2022, Frank Zago
+ * Copyright (c) 2017 Gunar Schorcht (gunar@schorcht.net)
+ * Copyright (c) 2016 Tse Lun Bien
+ * Copyright (c) 2014 Marco Gittler
+ * Copyright (c) 2006-2007 Till Harbaum (Till@Harbaum.org)
+ */
+
+#include <linux/kernel.h>
+#include <linux/mfd/ch341.h>
+#include <linux/mfd/core.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/usb.h>
+
+static const struct mfd_cell ch341_devs[] = {
+};
+
+static int ch341_usb_probe(struct usb_interface *iface,
+			   const struct usb_device_id *usb_id)
+{
+	struct usb_host_endpoint *endpoints;
+	struct ch341_device *dev;
+	int rc;
+
+	dev = devm_kzalloc(&iface->dev, sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return -ENOMEM;
+
+	dev->usb_dev = usb_get_dev(interface_to_usbdev(iface));
+	dev->iface = iface;
+	mutex_init(&dev->usb_lock);
+
+	if (iface->cur_altsetting->desc.bNumEndpoints != 3) {
+		rc = -ENODEV;
+		goto free_dev;
+	}
+
+	endpoints = iface->cur_altsetting->endpoint;
+	if (!usb_endpoint_is_bulk_in(&endpoints[0].desc) ||
+	    !usb_endpoint_is_bulk_out(&endpoints[1].desc) ||
+	    !usb_endpoint_xfer_int(&endpoints[2].desc)) {
+		rc = -ENODEV;
+		goto free_dev;
+	}
+
+	dev->ep_in = endpoints[0].desc.bEndpointAddress;
+	dev->ep_out = endpoints[1].desc.bEndpointAddress;
+	dev->ep_intr = endpoints[2].desc.bEndpointAddress;
+	dev->ep_intr_interval = endpoints[2].desc.bInterval;
+
+	usb_set_intfdata(iface, dev);
+
+	rc = mfd_add_hotplug_devices(&iface->dev, ch341_devs,
+				     ARRAY_SIZE(ch341_devs));
+	if (rc) {
+		rc = dev_err_probe(&iface->dev, rc,
+				   "Failed to add mfd devices to core\n");
+		goto free_dev;
+	}
+
+	return 0;
+
+free_dev:
+	usb_put_dev(dev->usb_dev);
+
+	return rc;
+}
+
+static void ch341_usb_disconnect(struct usb_interface *usb_if)
+{
+	struct ch341_device *dev = usb_get_intfdata(usb_if);
+
+	mfd_remove_devices(&usb_if->dev);
+	usb_put_dev(dev->usb_dev);
+}
+
+static const struct usb_device_id ch341_usb_table[] = {
+	{ USB_DEVICE(0x1a86, 0x5512) },
+	{ }
+};
+MODULE_DEVICE_TABLE(usb, ch341_usb_table);
+
+static struct usb_driver ch341_usb_driver = {
+	.name       = "ch341-mfd",
+	.id_table   = ch341_usb_table,
+	.probe      = ch341_usb_probe,
+	.disconnect = ch341_usb_disconnect,
+};
+module_usb_driver(ch341_usb_driver);
+
+MODULE_AUTHOR("Various");
+MODULE_DESCRIPTION("CH341 USB to I2C/SPI/GPIO adapter");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/ch341.h b/include/linux/mfd/ch341.h
new file mode 100644
index 000000000000..a87b23e30123
--- /dev/null
+++ b/include/linux/mfd/ch341.h
@@ -0,0 +1,26 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Definitions for the CH341 MFD driver */
+
+#include <linux/mutex.h>
+#include <linux/types.h>
+
+#define DEFAULT_TIMEOUT_MS 1000	/* 1s USB requests timeout */
+
+/* All commands fit inside a 32-byte segment. There may be several
+ * of these segments in a USB command.
+ */
+#define SEG_SIZE 32
+
+struct usb_device;
+struct usb_interface;
+
+struct ch341_device {
+	struct usb_device *usb_dev;
+	struct usb_interface *iface;
+	struct mutex usb_lock;
+
+	int ep_in;
+	int ep_out;
+	int ep_intr;
+	u8 ep_intr_interval;
+};
-- 
2.32.0

[PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the CH341

[frank zago]

The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
read-only.

Signed-off-by: frank zago <frank@zago.net>
---
 MAINTAINERS               |   1 +
 drivers/gpio/Kconfig      |  10 +
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-ch341.c | 383 ++++++++++++++++++++++++++++++++++++++
 drivers/mfd/ch341-core.c  |   3 +
 5 files changed, 398 insertions(+)
 create mode 100644 drivers/gpio/gpio-ch341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index b61af813fb9f..757ab4f6f9f6 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21215,6 +21215,7 @@ WINCHIPHEAD CH341 I2C/GPIO MFD DRIVER
 M:	Frank Zago <frank@zago.net>
 L:	linux-usb@vger.kernel.org
 S:	Maintained
+F:	drivers/gpio/gpio-ch341.c
 F:	drivers/mfd/ch341-core.c
 F:	include/linux/mfd/ch341.h
 
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 45764ec3b2eb..0e868c26daf6 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1652,6 +1652,16 @@ endmenu
 menu "USB GPIO expanders"
 	depends on USB
 
+config GPIO_CH341
+	tristate "CH341 USB to GPIO support"
+	select MFD_CH341
+	help
+	  If you say yes to this option, GPIO support will be included for the
+	  WCH CH341, a USB to I2C/SPI/GPIO interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called gpio-ch341.
+
 config GPIO_VIPERBOARD
 	tristate "Viperboard GPIO a & b support"
 	depends on MFD_VIPERBOARD
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 14352f6dfe8e..beef802cbfb1 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -44,6 +44,7 @@ obj-$(CONFIG_GPIO_BD9571MWV)		+= gpio-bd9571mwv.o
 obj-$(CONFIG_GPIO_BRCMSTB)		+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)		+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CADENCE)		+= gpio-cadence.o
+obj-$(CONFIG_GPIO_CH341)		+= gpio-ch341.o
 obj-$(CONFIG_GPIO_CLPS711X)		+= gpio-clps711x.o
 obj-$(CONFIG_GPIO_SNPS_CREG)		+= gpio-creg-snps.o
 obj-$(CONFIG_GPIO_CRYSTAL_COVE)		+= gpio-crystalcove.o
diff --git a/drivers/gpio/gpio-ch341.c b/drivers/gpio/gpio-ch341.c
new file mode 100644
index 000000000000..8e81ccf93d71
--- /dev/null
+++ b/drivers/gpio/gpio-ch341.c
@@ -0,0 +1,383 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * GPIO cell driver for the CH341A and CH341B chips.
+ *
+ * Copyright 2022, Frank Zago
+ * Copyright (c) 2017 Gunar Schorcht (gunar@schorcht.net)
+ * Copyright (c) 2016 Tse Lun Bien
+ * Copyright (c) 2014 Marco Gittler
+ * Copyright (c) 2006-2007 Till Harbaum (Till@Harbaum.org)
+ */
+
+/*
+ * Notes.
+ *
+ * For the CH341, 0=IN, 1=OUT, but for the GPIO subsystem, 1=IN and
+ * 0=OUT. Translation happens in a couple places.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+#include <linux/gpio/driver.h>
+
+#include <linux/mfd/ch341.h>
+
+#define CH341_GPIO_NUM_PINS         16    /* Number of GPIO pins */
+
+/* GPIO chip commands */
+#define CH341_PARA_CMD_STS          0xA0  /* Get pins status */
+#define CH341_CMD_UIO_STREAM        0xAB  /* pin IO stream command */
+
+#define CH341_CMD_UIO_STM_OUT       0x80  /* pin IO interface OUT command (D0~D5) */
+#define CH341_CMD_UIO_STM_DIR       0x40  /* pin IO interface DIR command (D0~D5) */
+#define CH341_CMD_UIO_STM_END       0x20  /* pin IO interface END command */
+
+#define CH341_USB_MAX_INTR_SIZE 8
+
+struct ch341_gpio {
+	struct gpio_chip gpio;
+	struct mutex gpio_lock;
+	u16 gpio_dir;		/* 1 bit per pin, 0=IN, 1=OUT. */
+	u16 gpio_last_read;	/* last GPIO values read */
+	u16 gpio_last_written;	/* last GPIO values written */
+	union {
+		u8 gpio_buf[SEG_SIZE];
+		__le16 gpio_buf_status;
+	};
+
+	struct urb *irq_urb;
+	struct usb_anchor irq_urb_out;
+	u8 irq_buf[CH341_USB_MAX_INTR_SIZE];
+	struct irq_chip irq_chip;
+
+	struct ch341_device *ch341;
+};
+
+/*
+ * Masks to describe the 16 GPIOs. Pins D0 to D5 (mapped to GPIOs 0 to
+ * 5) can do input/output, but the other pins are input-only.
+ */
+static const u16 pin_can_output = 0b111111;
+
+/* Only GPIO 10 (INT# line) has hardware interrupt */
+#define CH341_GPIO_INT_LINE 10
+
+/* Send a command and get a reply if requested */
+static int gpio_transfer(struct ch341_gpio *dev, int out_len, int in_len)
+{
+	struct ch341_device *ch341 = dev->ch341;
+	int actual;
+	int rc;
+
+	mutex_lock(&ch341->usb_lock);
+
+	rc = usb_bulk_msg(ch341->usb_dev,
+			  usb_sndbulkpipe(ch341->usb_dev, ch341->ep_out),
+			  dev->gpio_buf, out_len,
+			  &actual, DEFAULT_TIMEOUT_MS);
+	if (rc < 0)
+		goto out_unlock;
+
+	if (in_len == 0)
+		goto out_unlock;
+
+	rc = usb_bulk_msg(ch341->usb_dev,
+			  usb_rcvbulkpipe(ch341->usb_dev, ch341->ep_in),
+			  dev->gpio_buf, SEG_SIZE, &actual, DEFAULT_TIMEOUT_MS);
+
+out_unlock:
+	mutex_unlock(&ch341->usb_lock);
+
+	if (rc < 0)
+		return rc;
+
+	return actual;
+}
+
+/* Read the GPIO line status. */
+static int read_inputs(struct ch341_gpio *dev)
+{
+	int rc;
+
+	mutex_lock(&dev->gpio_lock);
+
+	dev->gpio_buf[0] = CH341_PARA_CMD_STS;
+
+	rc = gpio_transfer(dev, 1, 1);
+
+	/*
+	 * The status command returns 6 bytes of data. Byte 0 has
+	 * status for lines 0 to 7, and byte 1 is lines 8 to 15. The
+	 * 3rd has the status for the SCL/SDA/SCK pins. The 4th byte
+	 * might have some remaining pin status. Byte 5 and 6 content
+	 * is unknown.
+	 */
+	if (rc == 6)
+		dev->gpio_last_read = le16_to_cpu(dev->gpio_buf_status);
+	else
+		rc = -EIO;
+
+	mutex_unlock(&dev->gpio_lock);
+
+	if (rc < 0)
+		return rc;
+
+	return 0;
+}
+
+static int ch341_gpio_get(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+	int rc;
+
+	rc = read_inputs(dev);
+	if (rc)
+		return rc;
+
+	return !!(dev->gpio_last_read & BIT(offset));
+}
+
+static int ch341_gpio_get_multiple(struct gpio_chip *chip,
+				   unsigned long *mask, unsigned long *bits)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+	int rc;
+
+	rc = read_inputs(dev);
+	if (rc)
+		return rc;
+
+	*bits = dev->gpio_last_read & *mask;
+
+	return 0;
+}
+
+static void write_outputs(struct ch341_gpio *dev)
+{
+	mutex_lock(&dev->gpio_lock);
+
+	/* Only the first 6 lines can output. */
+	dev->gpio_buf[0] = CH341_CMD_UIO_STREAM;
+	dev->gpio_buf[1] = CH341_CMD_UIO_STM_DIR | (dev->gpio_dir & pin_can_output);
+	dev->gpio_buf[2] = CH341_CMD_UIO_STM_OUT |
+		(dev->gpio_last_written & dev->gpio_dir & pin_can_output);
+	dev->gpio_buf[3] = CH341_CMD_UIO_STM_END;
+
+	gpio_transfer(dev, 4, 0);
+
+	mutex_unlock(&dev->gpio_lock);
+}
+
+static void ch341_gpio_set(struct gpio_chip *chip, unsigned int offset, int value)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+
+	if (value)
+		dev->gpio_last_written |= BIT(offset);
+	else
+		dev->gpio_last_written &= ~BIT(offset);
+
+	write_outputs(dev);
+}
+
+static void ch341_gpio_set_multiple(struct gpio_chip *chip,
+				    unsigned long *mask, unsigned long *bits)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+
+	dev->gpio_last_written = (dev->gpio_last_written & ~*mask) | (*bits & *mask);
+
+	write_outputs(dev);
+}
+
+static int ch341_gpio_get_direction(struct gpio_chip *chip,
+				    unsigned int offset)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+
+	return !(dev->gpio_dir & BIT(offset));
+}
+
+static int ch341_gpio_direction_input(struct gpio_chip *chip,
+				      unsigned int offset)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+
+	dev->gpio_dir &= ~BIT(offset);
+
+	write_outputs(dev);
+
+	return 0;
+}
+
+static int ch341_gpio_direction_output(struct gpio_chip *chip,
+				       unsigned int offset, int value)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+	u16 mask = BIT(offset);
+
+	if (!(pin_can_output & mask))
+		return -EINVAL;
+
+	dev->gpio_dir |= mask;
+
+	ch341_gpio_set(chip, offset, value);
+
+	return 0;
+}
+
+static void ch341_complete_intr_urb(struct urb *urb)
+{
+	struct ch341_gpio *dev = urb->context;
+	int rc;
+
+	if (urb->status) {
+		usb_unanchor_urb(dev->irq_urb);
+	} else {
+		/*
+		 * Data is 8 bytes. Byte 0 might be the length of
+		 * significant data, which is 3 more bytes. Bytes 1
+		 * and 2, and possibly 3, are the pin status. The byte
+		 * order is different than for the GET_STATUS
+		 * command. Byte 1 is GPIOs 8 to 15, and byte 2 is
+		 * GPIOs 0 to 7.
+		 */
+
+		handle_nested_irq(irq_find_mapping(dev->gpio.irq.domain,
+						   CH341_GPIO_INT_LINE));
+
+		rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
+		if (rc)
+			usb_unanchor_urb(dev->irq_urb);
+	}
+}
+
+static int ch341_gpio_irq_set_type(struct irq_data *data, unsigned int flow_type)
+{
+	const unsigned long offset = irqd_to_hwirq(data);
+
+	if (offset != CH341_GPIO_INT_LINE || flow_type != IRQ_TYPE_EDGE_RISING)
+		return -EINVAL;
+
+	return 0;
+}
+
+static void ch341_gpio_irq_enable(struct irq_data *data)
+{
+	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
+	int rc;
+
+	/*
+	 * The URB might have just been unlinked in
+	 * ch341_gpio_irq_disable, but the completion handler hasn't
+	 * been called yet.
+	 */
+	if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
+		usb_kill_anchored_urbs(&dev->irq_urb_out);
+
+	usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
+	rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
+	if (rc)
+		usb_unanchor_urb(dev->irq_urb);
+}
+
+static void ch341_gpio_irq_disable(struct irq_data *data)
+{
+	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
+
+	usb_unlink_urb(dev->irq_urb);
+}
+
+static int ch341_gpio_remove(struct platform_device *pdev)
+{
+	struct ch341_gpio *dev = platform_get_drvdata(pdev);
+
+	usb_kill_anchored_urbs(&dev->irq_urb_out);
+	gpiochip_remove(&dev->gpio);
+	usb_free_urb(dev->irq_urb);
+
+	return 0;
+}
+
+static int ch341_gpio_probe(struct platform_device *pdev)
+{
+	struct ch341_device *ch341 = dev_get_drvdata(pdev->dev.parent);
+	struct gpio_irq_chip *girq;
+	struct ch341_gpio *dev;
+	struct gpio_chip *gpio;
+	int rc;
+
+	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
+	if (dev == NULL)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, dev);
+	dev->ch341 = ch341;
+	mutex_init(&dev->gpio_lock);
+
+	gpio = &dev->gpio;
+	gpio->label = dev_name(&pdev->dev);
+	gpio->parent = &pdev->dev;
+	gpio->owner = THIS_MODULE;
+	gpio->get_direction = ch341_gpio_get_direction;
+	gpio->direction_input = ch341_gpio_direction_input;
+	gpio->direction_output = ch341_gpio_direction_output;
+	gpio->get = ch341_gpio_get;
+	gpio->get_multiple = ch341_gpio_get_multiple;
+	gpio->set = ch341_gpio_set;
+	gpio->set_multiple = ch341_gpio_set_multiple;
+	gpio->base = -1;
+	gpio->ngpio = CH341_GPIO_NUM_PINS;
+	gpio->can_sleep = true;
+
+	dev->irq_chip.name = dev_name(&pdev->dev);
+	dev->irq_chip.irq_set_type = ch341_gpio_irq_set_type;
+	dev->irq_chip.irq_enable = ch341_gpio_irq_enable;
+	dev->irq_chip.irq_disable = ch341_gpio_irq_disable;
+
+	girq = &gpio->irq;
+	girq->chip = &dev->irq_chip;
+	girq->handler = handle_simple_irq;
+	girq->default_type = IRQ_TYPE_NONE;
+
+	/* Create an URB for handling interrupt */
+	dev->irq_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!dev->irq_urb)
+		return dev_err_probe(&pdev->dev, -ENOMEM, "Cannot allocate the int URB\n");
+
+	usb_fill_int_urb(dev->irq_urb, ch341->usb_dev,
+			 usb_rcvintpipe(ch341->usb_dev, ch341->ep_intr),
+			 dev->irq_buf, CH341_USB_MAX_INTR_SIZE,
+			 ch341_complete_intr_urb, dev, ch341->ep_intr_interval);
+
+	init_usb_anchor(&dev->irq_urb_out);
+
+	rc = gpiochip_add_data(gpio, dev);
+	if (rc) {
+		rc = dev_err_probe(&pdev->dev, rc, "Could not add GPIO\n");
+		goto release_urb;
+	}
+
+	return 0;
+
+release_urb:
+	usb_free_urb(dev->irq_urb);
+
+	return rc;
+}
+
+static struct platform_driver ch341_gpio_driver = {
+	.driver.name	= "ch341-gpio",
+	.probe		= ch341_gpio_probe,
+	.remove		= ch341_gpio_remove,
+};
+module_platform_driver(ch341_gpio_driver);
+
+MODULE_AUTHOR("Various");
+MODULE_DESCRIPTION("CH341 USB to GPIO");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ch341-gpio");
diff --git a/drivers/mfd/ch341-core.c b/drivers/mfd/ch341-core.c
index 0bb6eb8057e9..e919f6901a14 100644
--- a/drivers/mfd/ch341-core.c
+++ b/drivers/mfd/ch341-core.c
@@ -19,6 +19,9 @@
 #include <linux/usb.h>
 
 static const struct mfd_cell ch341_devs[] = {
+	{
+		.name = "ch341-gpio",
+	},
 };
 
 static int ch341_usb_probe(struct usb_interface *iface,
-- 
2.32.0

[PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[frank zago]

The I2C interface can run at 4 different speeds. This driver currently
only offer 100MHz. Tested with a variety of I2C sensors, and the IIO
subsystem.

Signed-off-by: frank zago <frank@zago.net>
---
 MAINTAINERS                    |   1 +
 drivers/i2c/busses/Kconfig     |  10 +
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-ch341.c | 331 +++++++++++++++++++++++++++++++++
 drivers/mfd/ch341-core.c       |   3 +
 5 files changed, 346 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ch341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 757ab4f6f9f6..04ec5095e1d0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -21216,6 +21216,7 @@ M:	Frank Zago <frank@zago.net>
 L:	linux-usb@vger.kernel.org
 S:	Maintained
 F:	drivers/gpio/gpio-ch341.c
+F:	drivers/i2c/busses/i2c-ch341.c
 F:	drivers/mfd/ch341-core.c
 F:	include/linux/mfd/ch341.h
 
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index a1bae59208e3..db9797345ad5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1199,6 +1199,16 @@ config I2C_RCAR
 
 comment "External I2C/SMBus adapter drivers"
 
+config I2C_CH341
+	tristate "CH341 USB to I2C support"
+	select MFD_CH341
+	help
+	  If you say yes to this option, I2C support will be included for the
+	  WCH CH341, a USB to I2C/SPI/GPIO interface.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-ch341.
+
 config I2C_DIOLAN_U2C
 	tristate "Diolan U2C-12 USB adapter"
 	depends on USB
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 479f60e4ee3d..e83ca4a472f2 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -127,6 +127,7 @@ obj-$(CONFIG_I2C_XLP9XX)	+= i2c-xlp9xx.o
 obj-$(CONFIG_I2C_RCAR)		+= i2c-rcar.o
 
 # External I2C/SMBus adapter drivers
+obj-$(CONFIG_I2C_CH341)		+= i2c-ch341.o
 obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
 obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
 obj-$(CONFIG_I2C_CP2615) += i2c-cp2615.o
diff --git a/drivers/i2c/busses/i2c-ch341.c b/drivers/i2c/busses/i2c-ch341.c
new file mode 100644
index 000000000000..3da11e358976
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ch341.c
@@ -0,0 +1,331 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * I2C cell driver for the CH341A, CH341B and CH341T.
+ *
+ * Copyright 2022, Frank Zago
+ * Copyright (c) 2016 Tse Lun Bien
+ * Copyright (c) 2014 Marco Gittler
+ * Copyright (C) 2006-2007 Till Harbaum (Till@Harbaum.org)
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+
+#include <linux/i2c.h>
+
+#include <linux/mfd/ch341.h>
+
+/* I2C bus speed. Speed selection is not implemented. */
+#define CH341_I2C_20KHZ  0
+#define CH341_I2C_100KHZ 1
+#define CH341_I2C_400KHZ 2
+#define CH341_I2C_750KHZ 3
+
+/* I2C chip commands */
+#define CH341_CMD_I2C_STREAM 0xAA
+#define CH341_CMD_I2C_STM_END 0x00
+
+#define CH341_CMD_I2C_STM_STA 0x74
+#define CH341_CMD_I2C_STM_STO 0x75
+#define CH341_CMD_I2C_STM_OUT 0x80
+#define CH341_CMD_I2C_STM_IN 0xC0
+#define CH341_CMD_I2C_STM_SET 0x60
+
+/*
+ * The maximum request size is 4096 bytes, both for reading and
+ * writing, split in up to 128 32-byte segments. The I2C stream must
+ * start and stop in each 32-byte segment. Reading must also be split,
+ * with up to 32-byte per segment.
+ */
+#define SEG_COUNT 128
+
+/*
+ * Limit the transfer size that can be written. 4KiB is the maximum
+ * size of the whole buffer, but it must include all the command
+ * delimiters. 3KiB sounds reasonable.
+ */
+#define MAX_RW_LENGTH 3072
+
+struct ch341_i2c {
+	struct i2c_adapter adapter;
+
+	/* I2C request and response state */
+	int idx_out;		/* current offset in buf */
+	int out_seg;		/* current segment */
+	u8 i2c_buf[SEG_COUNT * SEG_SIZE];
+};
+
+/*
+ * Append a write command to the current request. A set of 32-byte
+ * packets is filled. Each packet starts with STREAM and finishes with
+ * END, and contains an OUT field, leaving up to 29 bytes of data. The
+ * first packet must also include a START and the device address.
+ */
+static int append_write(struct ch341_i2c *dev, const struct i2c_msg *msg)
+{
+	bool start_done = false;
+	u8 *out = dev->i2c_buf;
+	int len;
+	u8 *p;
+
+	len = msg->len;
+	p = msg->buf;
+
+	while (len) {
+		int to_write;
+		int avail;
+
+		if (dev->idx_out % SEG_SIZE) {
+			/* Finish current packet, and advance to the next one */
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_END;
+			dev->out_seg++;
+			dev->idx_out = dev->out_seg * SEG_SIZE;
+
+			if (dev->out_seg == SEG_COUNT)
+				return -E2BIG;
+		}
+
+		out[dev->idx_out++] = CH341_CMD_I2C_STREAM;
+
+		/* account for stream start and end */
+		avail = SEG_SIZE - 3;
+
+		if (!start_done) {
+			/* Each message has a start */
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_STA;
+
+			avail -= 2; /* room for STA and device address */
+		}
+
+		to_write = min_t(int, len, avail);
+
+		if (!start_done) {
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_OUT | (to_write + 1);
+			out[dev->idx_out++] = msg->addr << 1;
+
+			start_done = true;
+		} else {
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_OUT | to_write;
+		}
+
+		memcpy(&out[dev->idx_out], p, to_write);
+		dev->idx_out += to_write;
+		len -= to_write;
+		p += to_write;
+	}
+
+	return 0;
+}
+
+/*
+ * Append a read command to the request. It usually follows a write
+ * command. When that happens, the driver will attempt to concat the
+ * read command into the same packet.  Each read command, of up to 32
+ * bytes, must be written to a new packet. It is not possible to
+ * concat them.
+ */
+static int append_read(struct ch341_i2c *dev, const struct i2c_msg *msg)
+{
+	bool start_done = false;
+	u8 *out = dev->i2c_buf;
+	int len;
+
+	len = msg->len;
+
+	while (len) {
+		int to_read;
+
+		if (dev->idx_out % SEG_SIZE) {
+			if (!start_done &&
+			    (dev->idx_out % SEG_SIZE) <  (SEG_SIZE - 7)) {
+				/* There's enough left for a read */
+			} else {
+				/* Finish current packet, and advance to the next one */
+				out[dev->idx_out++] = CH341_CMD_I2C_STM_END;
+				dev->out_seg++;
+				dev->idx_out = dev->out_seg * SEG_SIZE;
+
+				if (dev->out_seg == SEG_COUNT)
+					return -E2BIG;
+
+				out[dev->idx_out++] = CH341_CMD_I2C_STREAM;
+			}
+		} else {
+			out[dev->idx_out++] = CH341_CMD_I2C_STREAM;
+		}
+
+		if (!start_done) {
+			/* Each message has a start */
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_STA;
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_OUT | 1;
+			out[dev->idx_out++] = msg->addr << 1 | 1;
+
+			start_done = true;
+		}
+
+		/* Apparently the last command must be an STM_IN to
+		 * read the last byte. Without it, the adapter gets
+		 * lost.
+		 */
+		to_read = min_t(int, len, 32);
+		len -= to_read;
+		if (len == 0) {
+			if (to_read > 1)
+				out[dev->idx_out++] = CH341_CMD_I2C_STM_IN | (to_read - 1);
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_IN;
+		} else {
+			out[dev->idx_out++] = CH341_CMD_I2C_STM_IN | to_read;
+		}
+	}
+
+	return 0;
+}
+
+static int ch341_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msgs, int num)
+{
+	struct ch341_i2c *dev = i2c_get_adapdata(adapter);
+	struct ch341_device *ch341 = adapter->algo_data;
+	u8 *out = dev->i2c_buf;
+	int actual;
+	int rc;
+	int i;
+
+	/* Prepare the request */
+	dev->idx_out = 0;
+	dev->out_seg = 0;
+
+	for (i = 0; i != num; i++) {
+		if (msgs[i].flags & I2C_M_RD)
+			rc = append_read(dev, &msgs[i]);
+		else
+			rc = append_write(dev, &msgs[i]);
+
+		if (rc)
+			return rc;
+	}
+
+	/* Finish the last packet */
+	if (SEG_SIZE - (dev->idx_out % SEG_SIZE) < 2) {
+		out[dev->idx_out++] = CH341_CMD_I2C_STM_END;
+
+		dev->out_seg++;
+		if (dev->out_seg == SEG_COUNT)
+			return -E2BIG;
+
+		dev->idx_out = dev->out_seg * SEG_SIZE;
+
+		out[dev->idx_out++] = CH341_CMD_I2C_STREAM;
+	}
+
+	out[dev->idx_out++] = CH341_CMD_I2C_STM_STO;
+	out[dev->idx_out++] = CH341_CMD_I2C_STM_END;
+
+	dev_dbg(&adapter->dev, "bulk_out request with %d bytes\n",
+		dev->idx_out);
+
+	mutex_lock(&ch341->usb_lock);
+
+	/* Issue the request */
+	rc = usb_bulk_msg(ch341->usb_dev,
+			  usb_sndbulkpipe(ch341->usb_dev, ch341->ep_out),
+			  dev->i2c_buf, dev->idx_out, &actual, DEFAULT_TIMEOUT_MS);
+	if (rc < 0) {
+		mutex_unlock(&ch341->usb_lock);
+		return rc;
+	}
+
+	for (i = 0; i != num; i++) {
+		if (!(msgs[i].flags & I2C_M_RD))
+			continue;
+
+		rc = usb_bulk_msg(ch341->usb_dev,
+				  usb_rcvbulkpipe(ch341->usb_dev, ch341->ep_in),
+				  dev->i2c_buf, msgs[i].len, &actual,
+				  DEFAULT_TIMEOUT_MS);
+
+		if (rc) {
+			mutex_unlock(&ch341->usb_lock);
+			return rc;
+		}
+
+		if (actual != msgs[i].len) {
+			mutex_unlock(&ch341->usb_lock);
+			return -EIO;
+		}
+
+		memcpy(msgs[i].buf, dev->i2c_buf, actual);
+	}
+
+	mutex_unlock(&ch341->usb_lock);
+
+	return num;
+}
+
+static u32 ch341_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_algorithm ch341_i2c_algorithm = {
+	.master_xfer = ch341_i2c_xfer,
+	.functionality = ch341_i2c_func,
+};
+
+static const struct i2c_adapter_quirks ch341_i2c_quirks = {
+	.max_read_len = MAX_RW_LENGTH,
+	.max_write_len = MAX_RW_LENGTH,
+};
+
+static int ch341_i2c_probe(struct platform_device *pdev)
+{
+	struct ch341_device *ch341 = dev_get_drvdata(pdev->dev.parent);
+	struct ch341_i2c *ch341_i2c;
+	int actual;
+	int rc;
+
+	ch341_i2c = devm_kzalloc(&pdev->dev, sizeof(*ch341_i2c), GFP_KERNEL);
+	if (ch341_i2c == NULL)
+		return -ENOMEM;
+
+	ch341_i2c->adapter.owner = THIS_MODULE;
+	ch341_i2c->adapter.class = I2C_CLASS_HWMON;
+	ch341_i2c->adapter.algo = &ch341_i2c_algorithm;
+	ch341_i2c->adapter.algo_data = ch341;
+	ch341_i2c->adapter.quirks = &ch341_i2c_quirks;
+	ch341_i2c->adapter.dev.parent = &pdev->dev;
+	snprintf(ch341_i2c->adapter.name, sizeof(ch341_i2c->adapter.name),
+		 "CH341 I2C USB bus %03d device %03d",
+		 ch341->usb_dev->bus->busnum, ch341->usb_dev->devnum);
+
+	i2c_set_adapdata(&ch341_i2c->adapter, ch341_i2c);
+	platform_set_drvdata(pdev, ch341_i2c);
+
+	/* Set ch341 i2c speed */
+	ch341_i2c->i2c_buf[0] = CH341_CMD_I2C_STREAM;
+	ch341_i2c->i2c_buf[1] = CH341_CMD_I2C_STM_SET | CH341_I2C_100KHZ;
+	ch341_i2c->i2c_buf[2] = CH341_CMD_I2C_STM_END;
+	mutex_lock(&ch341->usb_lock);
+	rc = usb_bulk_msg(ch341->usb_dev,
+			  usb_sndbulkpipe(ch341->usb_dev, ch341->ep_out),
+			  ch341_i2c->i2c_buf, 3, &actual, DEFAULT_TIMEOUT_MS);
+	mutex_unlock(&ch341->usb_lock);
+
+	if (rc < 0)
+		return dev_err_probe(&pdev->dev, rc, "Cannot set I2C speed\n");
+
+	return devm_i2c_add_adapter(&pdev->dev, &ch341_i2c->adapter);
+}
+
+static struct platform_driver ch341_i2c_driver = {
+	.driver.name	= "ch341-i2c",
+	.probe		= ch341_i2c_probe,
+};
+module_platform_driver(ch341_i2c_driver);
+
+MODULE_AUTHOR("Various");
+MODULE_DESCRIPTION("CH341 USB to I2C");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:ch341-i2c");
diff --git a/drivers/mfd/ch341-core.c b/drivers/mfd/ch341-core.c
index e919f6901a14..6a326cac2247 100644
--- a/drivers/mfd/ch341-core.c
+++ b/drivers/mfd/ch341-core.c
@@ -22,6 +22,9 @@ static const struct mfd_cell ch341_devs[] = {
 	{
 		.name = "ch341-gpio",
 	},
+	{
+		.name = "ch341-i2c",
+	},
 };
 
 static int ch341_usb_probe(struct usb_interface *iface,
-- 
2.32.0

Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Sergey Shtylyov]

Hello!

On 4/1/22 5:33 AM, frank zago wrote:

> The I2C interface can run at 4 different speeds. This driver currently
> only offer 100MHz. Tested with a variety of I2C sensors, and the IIO

   Maybe 100 KHz? It's a more common I2C speed. :-)

> subsystem.
> 
> Signed-off-by: frank zago <frank@zago.net>
[...]

MBR,  Sergey

Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the CH341

[Linus Walleij]

On Fri, Apr 1, 2022 at 4:33 AM frank zago <frank@zago.net> wrote:

> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.
>
> Signed-off-by: frank zago <frank@zago.net>

Looks good to me:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij

Re: [PATCH v5 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/SPI/GPIO mode

[Lee Jones]

On Thu, 31 Mar 2022, frank zago wrote:

> The CH341 is a multifunction chip, presenting 3 different USB PID. One
> of these functions is for I2C/SPI/GPIO. This new set of drivers will
> manage I2C and GPIO.
> 
> Signed-off-by: frank zago <frank@zago.net>
> ---
>  Documentation/misc-devices/ch341.rst | 114 +++++++++++++++++++++++++++
>  Documentation/misc-devices/index.rst |   1 +

Not sure I've seen this bundled with an MFD driver before.

Please separate them into their own patches.

>  MAINTAINERS                          |   7 ++
>  drivers/mfd/Kconfig                  |  10 +++
>  drivers/mfd/Makefile                 |   1 +
>  drivers/mfd/ch341-core.c             |  99 +++++++++++++++++++++++
>  include/linux/mfd/ch341.h            |  26 ++++++
>  7 files changed, 258 insertions(+)
>  create mode 100644 Documentation/misc-devices/ch341.rst
>  create mode 100644 drivers/mfd/ch341-core.c
>  create mode 100644 include/linux/mfd/ch341.h
> 
> diff --git a/Documentation/misc-devices/ch341.rst b/Documentation/misc-devices/ch341.rst
> new file mode 100644
> index 000000000000..bf0b83f2eb85
> --- /dev/null
> +++ b/Documentation/misc-devices/ch341.rst
> @@ -0,0 +1,114 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +===========================================================
> +WinChipHead (沁恒) CH341 linux driver for I2C and GPIO mode
> +===========================================================
> +
> +The CH341 is declined in several flavors, and may support one or more
> +of UART, SPI, I2C and GPIO, but not always simultaneously:
> +
> +  - CH341 A/B/F: UART, Printer, SPI, I2C and GPIO
> +  - CH341 C/T: UART and I2C
> +  - CH341 H: SPI
> +
> +They work in 3 different modes, with only one being presented
> +depending on the USB PID:
> +
> +  - 0x5523: UART mode, covered by the USB `ch341` serial driver
> +  - 0x5512: SPI/I2C/GPIO mode, covered by the ch341 MFD drivers
> +  - 0x5584: Parallel printer mode, covered by the USB `usblp` driver
> +
> +Mode selection is done at the hardware level by tying some
> +pins. Breakout boards with one of the CH341 chip usually have one or
> +more jumpers to select which mode they work on. At least one model
> +(CJMCU-341) appears to need bridging some solder pads to select a
> +different default. Breakout boards also don't usually offer an option
> +to configure the chip into printer mode; for that case, connect the
> +SCL and SDA lines directly together.
> +
> +The various CH341 appear to be indistinguishable from the
> +software. For instance the ch341 MFD driver will present a GPIO
> +interface for the CH341T although physical pins are not present, and
> +the device will accept GPIO commands.
> +
> +The ch341 MFD driver has been tested with a CH341A, CH341B and
> +CH341T.
> +
> +Some breakout boards work in 3.3V and 5V depending on some jumpers.
> +
> +The black chip programmer with a ZIF socket will power the CH341 at
> +3.3V if a jumper is set, but will only output 5V to the chips to be
> +programmed, which is not always desirable. A hardware hack to use 3.3V
> +everywhere, involving some soldering, is available at
> +https://eevblog.com/forum/repair/ch341a-serial-memory-programmer-power-supply-fix/
> +
> +Some sample code for the CH341 is available at the manufacturer
> +website, at http://wch-ic.com/products/CH341.html
> +
> +The repository at https://github.com/boseji/CH341-Store contains a lot
> +of information on these chips, including datasheets.
> +
> +This driver is based on the pre-existing work at
> +https://github.com/gschorcht/i2c-ch341-usb
> +
> +
> +I2C Caveats
> +-----------
> +
> +The ch341 doesn't work with a Wii nunchuk, possibly because the
> +pull-up value is too low (1500 ohms).
> +
> +i2c AT24 eeproms can be read but not programmed properly because the
> +at24 linux driver tries to write a byte at a time, and doesn't wait at
> +all (or enough) between writes. Data corruption on writes does occur.
> +
> +The driver doesn't support detection of I2C devices present on the
> +bus. Apparently when a device is not present at a given address, the
> +CH341 will return an extra byte of data, but the driver doesn't
> +support that. This may be addressed in the future.
> +
> +
> +The GPIOs
> +---------
> +
> +16 GPIOs are available on the CH341 A/B/F. The first 6 are input/output,
> +and the last 10 are input only.
> +
> +Pinout and their names as they appear on some breakout boards::
> +
> +  CH341A/B/F     GPIO  Names                    Mode
> +    pin          line
> +
> +   15             0     D0, CS0                  input/output
> +   16             1     D1, CS1                  input/output
> +   17             2     D2, CS2                  input/output
> +   18             3     D3, SCK, DCK             input/output
> +   19             4     D4, DOUT2, CS3           input/output
> +   20             5     D5, MOSI, DOUT, SDO      input/output
> +   21             6     D6, DIN2                 input
> +   22             7     D7, MISO, DIN            input
> +    5             8     ERR                      input
> +    6             9     PEMP                     input
> +    7            10     INT                      input
> +    8            11     SLCT (SELECT)            input
> +   26            12     RST# (?)                 input
> +   27            13     WT (WAIT)                input
> +    4            14     DS (Data Select?)        input
> +    3            15     AS (Address Select?)     input
> +
> +
> +GPIO interrupt
> +~~~~~~~~~~~~~~
> +
> +The INT pin, corresponding to GPIO 10 is an input pin that can trigger
> +an interrupt on a rising edge. Only that pin is able to generate an
> +interrupt, and only on a rising edge. Trying to monitor events on
> +another GPIO, or that GPIO on something other than a rising edge, will
> +be rejected.
> +
> +
> +SPI
> +---
> +
> +This driver doesn't offer an SPI interface (yet) due to the
> +impossibility of declaring an SPI device like I2C does.
> diff --git a/Documentation/misc-devices/index.rst b/Documentation/misc-devices/index.rst
> index 30ac58f81901..52d03715601e 100644
> --- a/Documentation/misc-devices/index.rst
> +++ b/Documentation/misc-devices/index.rst
> @@ -19,6 +19,7 @@ fit into other categories.
>     bh1770glc
>     eeprom
>     c2port
> +   ch341
>     dw-xdata-pcie
>     ibmvmc
>     ics932s401
> diff --git a/MAINTAINERS b/MAINTAINERS
> index cbbd3ce7e0c2..b61af813fb9f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -21211,6 +21211,13 @@ M:	David Härdeman <david@hardeman.nu>
>  S:	Maintained
>  F:	drivers/media/rc/winbond-cir.c
>  
> +WINCHIPHEAD CH341 I2C/GPIO MFD DRIVER
> +M:	Frank Zago <frank@zago.net>
> +L:	linux-usb@vger.kernel.org
> +S:	Maintained
> +F:	drivers/mfd/ch341-core.c
> +F:	include/linux/mfd/ch341.h
> +
>  WINSYSTEMS EBC-C384 WATCHDOG DRIVER
>  M:	William Breathitt Gray <vilhelm.gray@gmail.com>
>  L:	linux-watchdog@vger.kernel.org
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 3b59456f5545..893acc821a42 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1784,6 +1784,16 @@ config MFD_LOCHNAGAR
>  	help
>  	  Support for Cirrus Logic Lochnagar audio development board.
>  
> +config MFD_CH341
> +	tristate "WinChipHead CH341 in I2C/SPI/GPIO mode"
> +	depends on USB
> +	help
> +	  If you say yes to this option, support for the CH341 series
> +	  of chips, running in I2C/SPI/GPIO mode will be included.
> +
> +	  This driver can also be built as a module.  If so, the
> +	  module will be called ch341-core.
> +
>  config MFD_ARIZONA
>  	select REGMAP
>  	select REGMAP_IRQ
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 858cacf659d6..fd615ab3929f 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_MFD_ASIC3)		+= asic3.o tmio_core.o
>  obj-$(CONFIG_ARCH_BCM2835)	+= bcm2835-pm.o
>  obj-$(CONFIG_MFD_BCM590XX)	+= bcm590xx.o
>  obj-$(CONFIG_MFD_BD9571MWV)	+= bd9571mwv.o
> +obj-$(CONFIG_MFD_CH341)		+= ch341-core.o
>  obj-$(CONFIG_MFD_CROS_EC_DEV)	+= cros_ec_dev.o
>  obj-$(CONFIG_MFD_ENE_KB3930)	+= ene-kb3930.o
>  obj-$(CONFIG_MFD_EXYNOS_LPASS)	+= exynos-lpass.o
> diff --git a/drivers/mfd/ch341-core.c b/drivers/mfd/ch341-core.c
> new file mode 100644
> index 000000000000..0bb6eb8057e9
> --- /dev/null
> +++ b/drivers/mfd/ch341-core.c
> @@ -0,0 +1,99 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Core driver for the CH341A, CH341B and CH341T in I2C/SPI/GPIO
> + * mode. There are cell drivers available for I2C and GPIO. SPI is not
> + * yet supported.
> + *
> + * Copyright 2022, Frank Zago
> + * Copyright (c) 2017 Gunar Schorcht (gunar@schorcht.net)
> + * Copyright (c) 2016 Tse Lun Bien
> + * Copyright (c) 2014 Marco Gittler
> + * Copyright (c) 2006-2007 Till Harbaum (Till@Harbaum.org)
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/mfd/ch341.h>
> +#include <linux/mfd/core.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/usb.h>
> +
> +static const struct mfd_cell ch341_devs[] = {
> +};
> +
> +static int ch341_usb_probe(struct usb_interface *iface,
> +			   const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_endpoint *endpoints;
> +	struct ch341_device *dev;

Please rename this struct 'ch341_ddata' and s/dev/ddata/.

> +	int rc;

'ret' please.

> +	dev = devm_kzalloc(&iface->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->usb_dev = usb_get_dev(interface_to_usbdev(iface));

Can this come later?

It would save on the goto dance.

> +	dev->iface = iface;
> +	mutex_init(&dev->usb_lock);
> +
> +	if (iface->cur_altsetting->desc.bNumEndpoints != 3) {

Why 3?

> +		rc = -ENODEV;
> +		goto free_dev;
> +	}
> +
> +	endpoints = iface->cur_altsetting->endpoint;
> +	if (!usb_endpoint_is_bulk_in(&endpoints[0].desc) ||
> +	    !usb_endpoint_is_bulk_out(&endpoints[1].desc) ||
> +	    !usb_endpoint_xfer_int(&endpoints[2].desc)) {

What has happened if we get here?

Perhaps a comment would be useful?

> +		rc = -ENODEV;
> +		goto free_dev;
> +	}
> +
> +	dev->ep_in = endpoints[0].desc.bEndpointAddress;
> +	dev->ep_out = endpoints[1].desc.bEndpointAddress;
> +	dev->ep_intr = endpoints[2].desc.bEndpointAddress;
> +	dev->ep_intr_interval = endpoints[2].desc.bInterval;
> +
> +	usb_set_intfdata(iface, dev);
> +
> +	rc = mfd_add_hotplug_devices(&iface->dev, ch341_devs,

Why are you using 'hotplug' here?

ch341_devs is empty right?

So no child devices are registered.

In which case this is not (yet) an MFD and cannot be accepted.

Please add the children.

> +				     ARRAY_SIZE(ch341_devs));
> +	if (rc) {
> +		rc = dev_err_probe(&iface->dev, rc,
> +				   "Failed to add mfd devices to core\n");

I'm not even sure what this means.  Should be:

"Failed to register child devices\n"

> +		goto free_dev;
> +	}
> +
> +	return 0;
> +
> +free_dev:
> +	usb_put_dev(dev->usb_dev);
> +
> +	return rc;
> +}
> +
> +static void ch341_usb_disconnect(struct usb_interface *usb_if)
> +{
> +	struct ch341_device *dev = usb_get_intfdata(usb_if);
> +
> +	mfd_remove_devices(&usb_if->dev);
> +	usb_put_dev(dev->usb_dev);
> +}
> +
> +static const struct usb_device_id ch341_usb_table[] = {
> +	{ USB_DEVICE(0x1a86, 0x5512) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(usb, ch341_usb_table);
> +
> +static struct usb_driver ch341_usb_driver = {
> +	.name       = "ch341-mfd",
> +	.id_table   = ch341_usb_table,
> +	.probe      = ch341_usb_probe,
> +	.disconnect = ch341_usb_disconnect,
> +};
> +module_usb_driver(ch341_usb_driver);
> +
> +MODULE_AUTHOR("Various");

This does not look valid.  Please drop it.

> +MODULE_DESCRIPTION("CH341 USB to I2C/SPI/GPIO adapter");

Is it?  What makes it one of those?

> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/ch341.h b/include/linux/mfd/ch341.h
> new file mode 100644
> index 000000000000..a87b23e30123
> --- /dev/null
> +++ b/include/linux/mfd/ch341.h
> @@ -0,0 +1,26 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/* Definitions for the CH341 MFD driver */

Drop the term MFD please.

> +#include <linux/mutex.h>
> +#include <linux/types.h>
> +
> +#define DEFAULT_TIMEOUT_MS 1000	/* 1s USB requests timeout */

Where is this used?

> +/* All commands fit inside a 32-byte segment. There may be several
> + * of these segments in a USB command.
> + */

This is not a properly formatted multi-line comment.

> +#define SEG_SIZE 32
> +
> +struct usb_device;
> +struct usb_interface;
> +
> +struct ch341_device {
> +	struct usb_device *usb_dev;
> +	struct usb_interface *iface;
> +	struct mutex usb_lock;
> +
> +	int ep_in;
> +	int ep_out;
> +	int ep_intr;
> +	u8 ep_intr_interval;
> +};

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 3678 bytes --]

Hi Frank,

I am not super familiar with USB drivers, so mostly some high level
review questions first:

On Thu, Mar 31, 2022 at 09:33:06PM -0500, frank zago wrote:
> The I2C interface can run at 4 different speeds. This driver currently
> only offer 100MHz. Tested with a variety of I2C sensors, and the IIO

100kHz.

> subsystem.
> 
> Signed-off-by: frank zago <frank@zago.net>

...

> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index a1bae59208e3..db9797345ad5 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1199,6 +1199,16 @@ config I2C_RCAR
>  
>  comment "External I2C/SMBus adapter drivers"
>  
> +config I2C_CH341
> +	tristate "CH341 USB to I2C support"
> +	select MFD_CH341

Hmm, it selects a symbol which depends on USB. Not good AFAIK. I think
this driver should depend on MFD_CH341.

> +	help
> +	  If you say yes to this option, I2C support will be included for the
> +	  WCH CH341, a USB to I2C/SPI/GPIO interface.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c-ch341.
> +
>  config I2C_DIOLAN_U2C
>  	tristate "Diolan U2C-12 USB adapter"
>  	depends on USB

...

> diff --git a/drivers/i2c/busses/i2c-ch341.c b/drivers/i2c/busses/i2c-ch341.c
> new file mode 100644
> index 000000000000..3da11e358976
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-ch341.c
> @@ -0,0 +1,331 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * I2C cell driver for the CH341A, CH341B and CH341T.
> + *
> + * Copyright 2022, Frank Zago
> + * Copyright (c) 2016 Tse Lun Bien
> + * Copyright (c) 2014 Marco Gittler
> + * Copyright (C) 2006-2007 Till Harbaum (Till@Harbaum.org)
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +
> +#include <linux/i2c.h>
> +
> +#include <linux/mfd/ch341.h>

Please sort the includes. No need for emtpy lines.

> +
> +/* I2C bus speed. Speed selection is not implemented. */
> +#define CH341_I2C_20KHZ  0
> +#define CH341_I2C_100KHZ 1
> +#define CH341_I2C_400KHZ 2
> +#define CH341_I2C_750KHZ 3
> +
> +/* I2C chip commands */
> +#define CH341_CMD_I2C_STREAM 0xAA
> +#define CH341_CMD_I2C_STM_END 0x00
> +
> +#define CH341_CMD_I2C_STM_STA 0x74
> +#define CH341_CMD_I2C_STM_STO 0x75
> +#define CH341_CMD_I2C_STM_OUT 0x80
> +#define CH341_CMD_I2C_STM_IN 0xC0
> +#define CH341_CMD_I2C_STM_SET 0x60
> +
> +/*
> + * The maximum request size is 4096 bytes, both for reading and
> + * writing, split in up to 128 32-byte segments. The I2C stream must
> + * start and stop in each 32-byte segment. Reading must also be split,
> + * with up to 32-byte per segment.
> + */
> +#define SEG_COUNT 128

You mean between every 32 bytes, there is a START and STOP condition on
the bus? Then, the maximum message size is 32 byte only, sadly. Or did I
misunderstand?

Can the driver send an arbitrary number of messages within one transfer?
E.g. write, read, read, write, read? All connected with a REPEATED START
and not with STOP and START?

...

> +static u32 ch341_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}

Have you also tested zero length messages AKA SMBus Quick commands?

...

> +
> +MODULE_AUTHOR("Various");

Please name the relevant authors. Only the ones which directly worked
on this driver.

> +MODULE_DESCRIPTION("CH341 USB to I2C");
> +MODULE_LICENSE("GPL");

SPDX header says "GPL v2".

So much for now, thanks for your submission!

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Johan Hovold]

[-- Attachment #1: Type: text/plain, Size: 306 bytes --]

On Sat, May 21, 2022 at 02:03:40PM +0200, Wolfram Sang wrote:
 
> > +MODULE_DESCRIPTION("CH341 USB to I2C");
> > +MODULE_LICENSE("GPL");
> 
> SPDX header says "GPL v2".

There's no (longer) any difference. See bf7fbeeae6db ("module: Cure the
MODULE_LICENSE "GPL" vs. "GPL v2" bogosity").

Johan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/SPI/GPIO mode

[Johan Hovold]

On Thu, Mar 31, 2022 at 09:33:04PM -0500, frank zago wrote:
> The CH341 is a multifunction chip, presenting 3 different USB PID. One
> of these functions is for I2C/SPI/GPIO. This new set of drivers will
> manage I2C and GPIO.
> 
> Signed-off-by: frank zago <frank@zago.net>
> ---

> +static int ch341_usb_probe(struct usb_interface *iface,
> +			   const struct usb_device_id *usb_id)
> +{
> +	struct usb_host_endpoint *endpoints;
> +	struct ch341_device *dev;
> +	int rc;
> +
> +	dev = devm_kzalloc(&iface->dev, sizeof(*dev), GFP_KERNEL);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->usb_dev = usb_get_dev(interface_to_usbdev(iface));

No need to grab a reference unless you're going to hold on to it past
disconnect().

> +	dev->iface = iface;
> +	mutex_init(&dev->usb_lock);
> +
> +	if (iface->cur_altsetting->desc.bNumEndpoints != 3) {
> +		rc = -ENODEV;
> +		goto free_dev;
> +	}
> +
> +	endpoints = iface->cur_altsetting->endpoint;
> +	if (!usb_endpoint_is_bulk_in(&endpoints[0].desc) ||
> +	    !usb_endpoint_is_bulk_out(&endpoints[1].desc) ||
> +	    !usb_endpoint_xfer_int(&endpoints[2].desc)) {
> +		rc = -ENODEV;
> +		goto free_dev;
> +	}

Please use usb_find_common_endpoints() for the above.

> +
> +	dev->ep_in = endpoints[0].desc.bEndpointAddress;
> +	dev->ep_out = endpoints[1].desc.bEndpointAddress;
> +	dev->ep_intr = endpoints[2].desc.bEndpointAddress;
> +	dev->ep_intr_interval = endpoints[2].desc.bInterval;
> +
> +	usb_set_intfdata(iface, dev);
> +
> +	rc = mfd_add_hotplug_devices(&iface->dev, ch341_devs,
> +				     ARRAY_SIZE(ch341_devs));
> +	if (rc) {
> +		rc = dev_err_probe(&iface->dev, rc,
> +				   "Failed to add mfd devices to core\n");
> +		goto free_dev;
> +	}
> +
> +	return 0;
> +
> +free_dev:
> +	usb_put_dev(dev->usb_dev);
> +
> +	return rc;
> +}
> +
> +static void ch341_usb_disconnect(struct usb_interface *usb_if)
> +{
> +	struct ch341_device *dev = usb_get_intfdata(usb_if);
> +
> +	mfd_remove_devices(&usb_if->dev);
> +	usb_put_dev(dev->usb_dev);
> +}

Johan

Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Lee Jones]

On Thu, 31 Mar 2022, frank zago wrote:

> The I2C interface can run at 4 different speeds. This driver currently
> only offer 100MHz. Tested with a variety of I2C sensors, and the IIO
> subsystem.
> 
> Signed-off-by: frank zago <frank@zago.net>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/i2c/busses/Kconfig     |  10 +
>  drivers/i2c/busses/Makefile    |   1 +
>  drivers/i2c/busses/i2c-ch341.c | 331 +++++++++++++++++++++++++++++++++

>  drivers/mfd/ch341-core.c       |   3 +

And this one in its own patch please.

-- 
Lee Jones [李琼斯]
Principal Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the CH341

[Johan Hovold]

On Thu, Mar 31, 2022 at 09:33:05PM -0500, frank zago wrote:
> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.
> 
> Signed-off-by: frank zago <frank@zago.net>
> ---

> +struct ch341_gpio {
> +	struct gpio_chip gpio;
> +	struct mutex gpio_lock;
> +	u16 gpio_dir;		/* 1 bit per pin, 0=IN, 1=OUT. */
> +	u16 gpio_last_read;	/* last GPIO values read */
> +	u16 gpio_last_written;	/* last GPIO values written */
> +	union {
> +		u8 gpio_buf[SEG_SIZE];
> +		__le16 gpio_buf_status;
> +	};
> +
> +	struct urb *irq_urb;
> +	struct usb_anchor irq_urb_out;
> +	u8 irq_buf[CH341_USB_MAX_INTR_SIZE];
> +	struct irq_chip irq_chip;
> +
> +	struct ch341_device *ch341;
> +};

> +static void ch341_complete_intr_urb(struct urb *urb)
> +{
> +	struct ch341_gpio *dev = urb->context;
> +	int rc;
> +
> +	if (urb->status) {
> +		usb_unanchor_urb(dev->irq_urb);

URBs are unanchored by USB core on completion.

> +	} else {
> +		/*
> +		 * Data is 8 bytes. Byte 0 might be the length of
> +		 * significant data, which is 3 more bytes. Bytes 1
> +		 * and 2, and possibly 3, are the pin status. The byte
> +		 * order is different than for the GET_STATUS
> +		 * command. Byte 1 is GPIOs 8 to 15, and byte 2 is
> +		 * GPIOs 0 to 7.
> +		 */
> +
> +		handle_nested_irq(irq_find_mapping(dev->gpio.irq.domain,
> +						   CH341_GPIO_INT_LINE));
> +
> +		rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> +		if (rc)
> +			usb_unanchor_urb(dev->irq_urb);
> +	}
> +}

> +static void ch341_gpio_irq_enable(struct irq_data *data)
> +{
> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> +	int rc;
> +
> +	/*
> +	 * The URB might have just been unlinked in
> +	 * ch341_gpio_irq_disable, but the completion handler hasn't
> +	 * been called yet.
> +	 */
> +	if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
> +		usb_kill_anchored_urbs(&dev->irq_urb_out);
> +
> +	usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
> +	rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> +	if (rc)
> +		usb_unanchor_urb(dev->irq_urb);

This looks confused and broken.

usb_kill_anchored_urbs() can sleep so either calling it is broken or
using GFP_ATOMIC is unnecessary.

And isn't this function called multiple times when enabling more than
one irq?!

> +}
> +
> +static void ch341_gpio_irq_disable(struct irq_data *data)
> +{
> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> +
> +	usb_unlink_urb(dev->irq_urb);

Same here...

> +}
> +
> +static int ch341_gpio_remove(struct platform_device *pdev)
> +{
> +	struct ch341_gpio *dev = platform_get_drvdata(pdev);
> +
> +	usb_kill_anchored_urbs(&dev->irq_urb_out);

You only have one URB...

And what prevents it from being resubmitted here?

> +	gpiochip_remove(&dev->gpio);
> +	usb_free_urb(dev->irq_urb);
> +
> +	return 0;
> +}
> +
> +static int ch341_gpio_probe(struct platform_device *pdev)
> +{
> +	struct ch341_device *ch341 = dev_get_drvdata(pdev->dev.parent);
> +	struct gpio_irq_chip *girq;
> +	struct ch341_gpio *dev;
> +	struct gpio_chip *gpio;
> +	int rc;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);
> +	if (dev == NULL)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, dev);
> +	dev->ch341 = ch341;
> +	mutex_init(&dev->gpio_lock);
> +
> +	gpio = &dev->gpio;
> +	gpio->label = dev_name(&pdev->dev);
> +	gpio->parent = &pdev->dev;
> +	gpio->owner = THIS_MODULE;
> +	gpio->get_direction = ch341_gpio_get_direction;
> +	gpio->direction_input = ch341_gpio_direction_input;
> +	gpio->direction_output = ch341_gpio_direction_output;
> +	gpio->get = ch341_gpio_get;
> +	gpio->get_multiple = ch341_gpio_get_multiple;
> +	gpio->set = ch341_gpio_set;
> +	gpio->set_multiple = ch341_gpio_set_multiple;
> +	gpio->base = -1;
> +	gpio->ngpio = CH341_GPIO_NUM_PINS;
> +	gpio->can_sleep = true;
> +
> +	dev->irq_chip.name = dev_name(&pdev->dev);
> +	dev->irq_chip.irq_set_type = ch341_gpio_irq_set_type;
> +	dev->irq_chip.irq_enable = ch341_gpio_irq_enable;
> +	dev->irq_chip.irq_disable = ch341_gpio_irq_disable;
> +
> +	girq = &gpio->irq;
> +	girq->chip = &dev->irq_chip;
> +	girq->handler = handle_simple_irq;
> +	girq->default_type = IRQ_TYPE_NONE;
> +
> +	/* Create an URB for handling interrupt */
> +	dev->irq_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!dev->irq_urb)
> +		return dev_err_probe(&pdev->dev, -ENOMEM, "Cannot allocate the int URB\n");
> +
> +	usb_fill_int_urb(dev->irq_urb, ch341->usb_dev,
> +			 usb_rcvintpipe(ch341->usb_dev, ch341->ep_intr),
> +			 dev->irq_buf, CH341_USB_MAX_INTR_SIZE,
> +			 ch341_complete_intr_urb, dev, ch341->ep_intr_interval);
> +
> +	init_usb_anchor(&dev->irq_urb_out);
> +
> +	rc = gpiochip_add_data(gpio, dev);
> +	if (rc) {
> +		rc = dev_err_probe(&pdev->dev, rc, "Could not add GPIO\n");
> +		goto release_urb;
> +	}
> +
> +	return 0;
> +
> +release_urb:
> +	usb_free_urb(dev->irq_urb);
> +
> +	return rc;
> +}

Johan

Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 240 bytes --]


> There's no (longer) any difference. See bf7fbeeae6db ("module: Cure the
> MODULE_LICENSE "GPL" vs. "GPL v2" bogosity").

Wow, cool. Thanks for the heads up! Sidenote: just from the title of
this commit I guessed the author correctly ;)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/SPI/GPIO mode

[Frank Zago]

Hi Lee,

>> +	dev->ep_in = endpoints[0].desc.bEndpointAddress;
>> +	dev->ep_out = endpoints[1].desc.bEndpointAddress;
>> +	dev->ep_intr = endpoints[2].desc.bEndpointAddress;
>> +	dev->ep_intr_interval = endpoints[2].desc.bInterval;
>> +
>> +	usb_set_intfdata(iface, dev);
>> +
>> +	rc = mfd_add_hotplug_devices(&iface->dev, ch341_devs,
> 
> Why are you using 'hotplug' here?

I replaced with mfd_add_devices, however I'm not sure what the difference
 is since mfd_add_hotplug_devices() is just a helper with less parameters.
That's what the viperboard driver does; I just copied since both devices 
are similar in functionality.

> 
> ch341_devs is empty right?
> 
> So no child devices are registered.
> 
> In which case this is not (yet) an MFD and cannot be accepted.
> 
> Please add the children.
> 
>> +				     ARRAY_SIZE(ch341_devs));
>> +	if (rc) {
>> +		rc = dev_err_probe(&iface->dev, rc,
>> +				   "Failed to add mfd devices to core\n");
> 
> I'm not even sure what this means.  Should be:
> 
> "Failed to register child devices\n"

Changed. The original string was also from the viperboard driver.

> 
>> +MODULE_DESCRIPTION("CH341 USB to I2C/SPI/GPIO adapter");
> 
> Is it?  What makes it one of those?

That's what this chip does, in addition to UART and printer modes. See patch 4.

Frank.

Re: [PATCH v5 3/3] i2c: ch341: add I2C MFD cell driver for the CH341

[Frank Zago]

Hi Wolfram,

On 5/21/22 07:03, Wolfram Sang wrote:

> Hi Frank,
> 
> I am not super familiar with USB drivers, so mostly some high level
> review questions first:
> 
> On Thu, Mar 31, 2022 at 09:33:06PM -0500, frank zago wrote:
>> The I2C interface can run at 4 different speeds. This driver currently
>> only offer 100MHz. Tested with a variety of I2C sensors, and the IIO
> 
> 100kHz.
> 
>> subsystem.
>>
>> Signed-off-by: frank zago <frank@zago.net>
> 
> ...
> 
>> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
>> index a1bae59208e3..db9797345ad5 100644
>> --- a/drivers/i2c/busses/Kconfig
>> +++ b/drivers/i2c/busses/Kconfig
>> @@ -1199,6 +1199,16 @@ config I2C_RCAR
>>  
>>  comment "External I2C/SMBus adapter drivers"
>>  
>> +config I2C_CH341
>> +	tristate "CH341 USB to I2C support"
>> +	select MFD_CH341
> 
> Hmm, it selects a symbol which depends on USB. Not good AFAIK. I think
> this driver should depend on MFD_CH341.

I've been asked earlier to change it to select. 

>> +
>> +/*
>> + * The maximum request size is 4096 bytes, both for reading and
>> + * writing, split in up to 128 32-byte segments. The I2C stream must
>> + * start and stop in each 32-byte segment. Reading must also be split,
>> + * with up to 32-byte per segment.
>> + */
>> +#define SEG_COUNT 128
> 
> You mean between every 32 bytes, there is a START and STOP condition on
> the bus? Then, the maximum message size is 32 byte only, sadly. Or did I
> misunderstand?

I've tried to reword that section. The usb command is up to 4kb, but each 
32-byte section is a command to the chip, not i2c.

> 
> Can the driver send an arbitrary number of messages within one transfer?
> E.g. write, read, read, write, read? All connected with a REPEATED START
> and not with STOP and START?

Yes it can.

> 
> ...
> 
>> +static u32 ch341_i2c_func(struct i2c_adapter *adap)
>> +{
>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
> 
> Have you also tested zero length messages AKA SMBus Quick commands?

Not properly at the time, although this didn't break the driver. 
I've fixed that by adding the special case.

Regards,
  Frank

Re: [PATCH v5 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/SPI/GPIO mode

[Frank Zago]

Hi Johan,

On 5/23/22 10:56, Johan Hovold wrote:

>> +
>> +	dev = devm_kzalloc(&iface->dev, sizeof(*dev), GFP_KERNEL);
>> +	if (!dev)
>> +		return -ENOMEM;
>> +
>> +	dev->usb_dev = usb_get_dev(interface_to_usbdev(iface));
> 
> No need to grab a reference unless you're going to hold on to it past
> disconnect().

I removed that.


>> +
>> +	endpoints = iface->cur_altsetting->endpoint;
>> +	if (!usb_endpoint_is_bulk_in(&endpoints[0].desc) ||
>> +	    !usb_endpoint_is_bulk_out(&endpoints[1].desc) ||
>> +	    !usb_endpoint_xfer_int(&endpoints[2].desc)) {
>> +		rc = -ENODEV;
>> +		goto free_dev;
>> +	}
> 
> Please use usb_find_common_endpoints() for the above.

Thanks. I wasn't aware on that API. It simplifies things a bit.

Regards,
  Frank.

Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the CH341

[Frank Zago]

On 5/23/22 11:16, Johan Hovold wrote:

>> +static void ch341_complete_intr_urb(struct urb *urb)
>> +{
>> +	struct ch341_gpio *dev = urb->context;
>> +	int rc;
>> +
>> +	if (urb->status) {
>> +		usb_unanchor_urb(dev->irq_urb);
> 
> URBs are unanchored by USB core on completion.

Fixed.

> 
>> +static void ch341_gpio_irq_enable(struct irq_data *data)
>> +{
>> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
>> +	int rc;
>> +
>> +	/*
>> +	 * The URB might have just been unlinked in
>> +	 * ch341_gpio_irq_disable, but the completion handler hasn't
>> +	 * been called yet.
>> +	 */
>> +	if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
>> +		usb_kill_anchored_urbs(&dev->irq_urb_out);
>> +
>> +	usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
>> +	rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
>> +	if (rc)
>> +		usb_unanchor_urb(dev->irq_urb);
> 
> This looks confused and broken.
> 
> usb_kill_anchored_urbs() can sleep so either calling it is broken or
> using GFP_ATOMIC is unnecessary.

Right, that function can sleep. I changed GFP_ATOMIC to GFP_KERNEL.

> 
> And isn't this function called multiple times when enabling more than
> one irq?!

There's only one IRQ, so only one URB will be posted at a time. It
is reposted as soon as it comes back unless the IRQ is disabled or
the device stops.


> 
>> +}
>> +
>> +static void ch341_gpio_irq_disable(struct irq_data *data)
>> +{
>> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
>> +
>> +	usb_unlink_urb(dev->irq_urb);
> 
> Same here...
> 
>> +}
>> +
>> +static int ch341_gpio_remove(struct platform_device *pdev)
>> +{
>> +	struct ch341_gpio *dev = platform_get_drvdata(pdev);
>> +
>> +	usb_kill_anchored_urbs(&dev->irq_urb_out);
> 
> You only have one URB...
> 
> And what prevents it from being resubmitted here?

I don't see what would resubmit it here. The gpio is being released.

Frank.

Re: [PATCH v5 2/3] gpio: ch341: add GPIO MFD cell driver for the CH341

[Johan Hovold]

On Wed, Jun 15, 2022 at 08:29:31PM -0500, Frank Zago wrote:
> On 5/23/22 11:16, Johan Hovold wrote:

> >> +static void ch341_gpio_irq_enable(struct irq_data *data)
> >> +{
> >> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> >> +	int rc;
> >> +
> >> +	/*
> >> +	 * The URB might have just been unlinked in
> >> +	 * ch341_gpio_irq_disable, but the completion handler hasn't
> >> +	 * been called yet.
> >> +	 */
> >> +	if (!usb_wait_anchor_empty_timeout(&dev->irq_urb_out, 5000))
> >> +		usb_kill_anchored_urbs(&dev->irq_urb_out);
> >> +
> >> +	usb_anchor_urb(dev->irq_urb, &dev->irq_urb_out);
> >> +	rc = usb_submit_urb(dev->irq_urb, GFP_ATOMIC);
> >> +	if (rc)
> >> +		usb_unanchor_urb(dev->irq_urb);
> > 
> > This looks confused and broken.
> > 
> > usb_kill_anchored_urbs() can sleep so either calling it is broken or
> > using GFP_ATOMIC is unnecessary.
> 
> Right, that function can sleep. I changed GFP_ATOMIC to GFP_KERNEL.

These callbacks can be called in atomic context so that's not an option,
I'm afraid.

> > And isn't this function called multiple times when enabling more than
> > one irq?!
> 
> There's only one IRQ, so only one URB will be posted at a time. It
> is reposted as soon as it comes back unless the IRQ is disabled or
> the device stops.

AFAICT you have up to 16 (CH341_GPIO_NUM_PINS) interrupts, not one. So I
still say this is broken.

> >> +}
> >> +
> >> +static void ch341_gpio_irq_disable(struct irq_data *data)
> >> +{
> >> +	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
> >> +
> >> +	usb_unlink_urb(dev->irq_urb);
> > 
> > Same here...

> >> +}
> >> +
> >> +static int ch341_gpio_remove(struct platform_device *pdev)
> >> +{
> >> +	struct ch341_gpio *dev = platform_get_drvdata(pdev);
> >> +
> >> +	usb_kill_anchored_urbs(&dev->irq_urb_out);
> > 
> > You only have one URB...
> > 
> > And what prevents it from being resubmitted here?
> 
> I don't see what would resubmit it here. The gpio is being released.

Your implementation needs to handle racing requests. The gpio chip is
still registered here.

Johan