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

[PATCH v4 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 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

During testing I found that i2c handles hot removal, but not gpio. The
gpio subsystem will complain with 'REMOVING GPIOCHIP WITH GPIOS STILL
REQUESTED', but it's a gpiolib issue.

Changes from v1:
  - Removed double Signed-off-by
  - Move Kconfig into the same directory as the driver


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

 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            | 421 +++++++++++++++++++++++++++
 drivers/i2c/busses/Kconfig           |  10 +
 drivers/i2c/busses/Makefile          |   1 +
 drivers/i2c/busses/i2c-ch341.c       | 325 +++++++++++++++++++++
 drivers/mfd/Kconfig                  |  12 +
 drivers/mfd/Makefile                 |   1 +
 drivers/mfd/ch341-core.c             | 109 +++++++
 include/linux/mfd/ch341.h            |  25 ++
 13 files changed, 1039 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 v4 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/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                  |  12 +++
 drivers/mfd/Makefile                 |   1 +
 drivers/mfd/ch341-core.c             | 103 ++++++++++++++++++++++++
 include/linux/mfd/ch341.h            |  25 ++++++
 7 files changed, 263 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..a4884918cd32
--- /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 MFD `ch341-mfd` driver
+  - 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 device 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..190f93b56ec6 100644
--- a/Documentation/misc-devices/index.rst
+++ b/Documentation/misc-devices/index.rst
@@ -29,3 +29,4 @@ fit into other categories.
    spear-pcie-gadget
    uacce
    xilinx_sdfec
+   ch341
diff --git a/MAINTAINERS b/MAINTAINERS
index cd0f68d4a34a..a6b2805fd1a1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20845,6 +20845,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 ba0b3eb131f1..84067e454c90 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1778,6 +1778,18 @@ 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.
+
+	  The chip's SPI mode is not supported.
+
+	  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 df1ecc4a4c95..a0364a0d5e33 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..fbb2897cd956
--- /dev/null
+++ b/drivers/mfd/ch341-core.c
@@ -0,0 +1,103 @@
+// 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/module.h>
+#include <linux/slab.h>
+#include <linux/mfd/core.h>
+
+#include <linux/mfd/ch341.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 = kzalloc(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) {
+		dev_err(&iface->dev, "Failed to add mfd devices to core.");
+		goto free_dev;
+	}
+
+	return 0;
+
+free_dev:
+	usb_put_dev(dev->usb_dev);
+	kfree(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_set_intfdata(dev->iface, NULL);
+	usb_put_dev(dev->usb_dev);
+	kfree(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 v2");
diff --git a/include/linux/mfd/ch341.h b/include/linux/mfd/ch341.h
new file mode 100644
index 000000000000..246159477cdf
--- /dev/null
+++ b/include/linux/mfd/ch341.h
@@ -0,0 +1,25 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Definitions for CH341 MFD driver
+ */
+
+#include <linux/usb.h>
+#include <linux/mutex.h>
+
+#define DEFAULT_TIMEOUT 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 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 v4 2/3] gpio: ch341: add 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 | 421 ++++++++++++++++++++++++++++++++++++++
 drivers/mfd/ch341-core.c  |   3 +
 5 files changed, 436 insertions(+)
 create mode 100644 drivers/gpio/gpio-ch341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a6b2805fd1a1..fdff76a5d9b0 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20849,6 +20849,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 1c211b4c63be..02a1624cd736 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1634,6 +1634,16 @@ endmenu
 menu "USB GPIO expanders"
 	depends on USB
 
+config GPIO_CH341
+	tristate "CH341 USB adapter in GPIO/I2C/SPI mode"
+	depends on 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 edbaa3cb343c..b2b47b185257 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..faa224372473
--- /dev/null
+++ b/drivers/gpio/gpio-ch341.c
@@ -0,0 +1,421 @@
+// 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. Some translation happens in a couple places.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/mfd/core.h>
+#include <linux/gpio.h>
+
+#include <linux/mfd/ch341.h>
+
+#define CH341_GPIO_NUM_PINS         16    /* Number of GPIO pins */
+
+#define CH341_PARA_CMD_STS          0xA0  /* Get pins status */
+#define CH341_CMD_UIO_STREAM        0xAB  /* UIO stream command */
+
+#define CH341_CMD_UIO_STM_OUT       0x80  /* UIO interface OUT command (D0~D5) */
+#define CH341_CMD_UIO_STM_DIR       0x40  /* UIO interface DIR command (D0~D5) */
+#define CH341_CMD_UIO_STM_END       0x20  /* UIO 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 */
+	u8 gpio_buf[SEG_SIZE];
+
+	struct {
+		char name[32];
+		bool enabled;
+		struct irq_chip irq;
+		int num;
+		struct urb *urb;
+		struct usb_anchor urb_out;
+		u8 buf[CH341_USB_MAX_INTR_SIZE];
+	} gpio_irq;
+
+	struct ch341_device *ch341;
+};
+
+/* Masks to describe the 16 GPIOs. Pins D0 to D5 (mapped to GPIOs 0 to
+ * 5) can read/write, but the other pins can only read.
+ */
+static const u16 pin_can_output = 0b111111;
+
+/* Only GPIO 10 (INT# line) has hardware interrupt */
+#define CH341_GPIO_INT_LINE 10
+
+static void ch341_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+
+	seq_printf(s, "pin config  : %04x  (0=IN, 1=OUT)\n", dev->gpio_dir);
+	seq_printf(s, "last read   : %04x\n", dev->gpio_last_read);
+	seq_printf(s, "last written: %04x\n", dev->gpio_last_written);
+}
+
+/* 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);
+	if (rc < 0)
+		goto done;
+
+	if (in_len == 0) {
+		rc = actual;
+		goto done;
+	}
+
+	rc = usb_bulk_msg(ch341->usb_dev,
+			  usb_rcvbulkpipe(ch341->usb_dev, ch341->ep_in),
+			  dev->gpio_buf, SEG_SIZE, &actual, DEFAULT_TIMEOUT);
+
+	if (rc == 0)
+		rc = actual;
+
+done:
+	mutex_unlock(&ch341->usb_lock);
+
+	return rc;
+}
+
+/* Read the GPIO line status. */
+static int read_inputs(struct ch341_gpio *dev)
+{
+	int result;
+
+	mutex_lock(&dev->gpio_lock);
+
+	dev->gpio_buf[0] = CH341_PARA_CMD_STS;
+
+	result = 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 (result == 6)
+		dev->gpio_last_read = le16_to_cpu(*(__le16 *)dev->gpio_buf);
+
+	mutex_unlock(&dev->gpio_lock);
+
+	return (result != 6) ? result : 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)) ? 1 : 0;
+}
+
+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);
+
+	dev->gpio_buf[0] = CH341_CMD_UIO_STREAM;
+	dev->gpio_buf[1] = CH341_CMD_UIO_STM_DIR | dev->gpio_dir;
+	dev->gpio_buf[2] = CH341_CMD_UIO_STM_OUT | (dev->gpio_last_written & dev->gpio_dir);
+	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 &= ~*mask;
+	dev->gpio_last_written |= (*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)) ? 0 : 1;
+}
+
+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) {
+		/* 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.
+		 *
+		 * Something like this (with locking?) could be done,
+		 * but there's nothing to retrieve that info without
+		 * doing another USB read:
+		 *
+		 *   dev->gpio_last_read = be16_to_cpu(*(u16 *)&dev->gpio_buf_intr[1]);
+		 */
+
+		handle_nested_irq(dev->gpio_irq.num);
+
+		rc = usb_submit_urb(dev->gpio_irq.urb, GFP_ATOMIC);
+		if (rc)
+			usb_unanchor_urb(dev->gpio_irq.urb);
+	} else {
+		usb_unanchor_urb(dev->gpio_irq.urb);
+	}
+}
+
+static int ch341_gpio_irq_set_type(struct irq_data *data, u32 type)
+{
+	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
+
+	if (data->irq != dev->gpio_irq.num || 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;
+
+	dev->gpio_irq.enabled = true;
+
+	/* 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->gpio_irq.urb_out, 5000))
+		usb_kill_anchored_urbs(&dev->gpio_irq.urb_out);
+
+	usb_anchor_urb(dev->gpio_irq.urb, &dev->gpio_irq.urb_out);
+	rc = usb_submit_urb(dev->gpio_irq.urb, GFP_ATOMIC);
+	if (rc)
+		usb_unanchor_urb(dev->gpio_irq.urb);
+}
+
+static void ch341_gpio_irq_disable(struct irq_data *data)
+{
+	struct ch341_gpio *dev = irq_data_get_irq_chip_data(data);
+
+	dev->gpio_irq.enabled = false;
+	usb_unlink_urb(dev->gpio_irq.urb);
+}
+
+/* Convert the GPIO index to the IRQ number */
+static int ch341_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ch341_gpio *dev = gpiochip_get_data(chip);
+
+	if (offset != CH341_GPIO_INT_LINE)
+		return -ENXIO;
+
+	return dev->gpio_irq.num;
+}
+
+static int ch341_gpio_remove(struct platform_device *pdev)
+{
+	struct ch341_gpio *dev = platform_get_drvdata(pdev);
+
+	usb_kill_anchored_urbs(&dev->gpio_irq.urb_out);
+	gpiochip_remove(&dev->gpio);
+	usb_free_urb(dev->gpio_irq.urb);
+
+	return 0;
+}
+
+static int ch341_gpio_probe(struct platform_device *pdev)
+{
+	struct ch341_device *ch341 = dev_get_drvdata(pdev->dev.parent);
+	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;
+
+	snprintf(dev->gpio_irq.name, sizeof(dev->gpio_irq.name),
+		 "ch341-%s-gpio", dev_name(&ch341->usb_dev->dev));
+	dev->gpio_irq.name[sizeof(dev->gpio_irq.name) - 1] = 0;
+
+	gpio = &dev->gpio;
+	gpio->label = "ch341";
+	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->dbg_show = ch341_gpio_dbg_show;
+	gpio->base = -1;
+	gpio->ngpio = CH341_GPIO_NUM_PINS;
+	gpio->can_sleep = true;
+	gpio->to_irq = ch341_gpio_to_irq;
+
+	dev->gpio_dir = 0;	/* All pins as input */
+
+	mutex_init(&dev->gpio_lock);
+
+	/* Allocate a software driven IRQ, for GPIO 10 */
+	dev->gpio_irq.irq.name = dev->gpio_irq.name;
+	dev->gpio_irq.irq.irq_set_type = ch341_gpio_irq_set_type;
+	dev->gpio_irq.irq.irq_enable = ch341_gpio_irq_enable;
+	dev->gpio_irq.irq.irq_disable = ch341_gpio_irq_disable;
+
+	rc = devm_irq_alloc_desc(&pdev->dev, 0);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "Cannot allocate an IRQ desc");
+		return rc;
+	}
+
+	dev->gpio_irq.num = rc;
+	dev->gpio_irq.enabled = false;
+
+	irq_set_chip_data(dev->gpio_irq.num, dev);
+	irq_set_chip_and_handler(dev->gpio_irq.num, &dev->gpio_irq.irq,
+				 handle_simple_irq);
+
+	/* Create an URB for handling interrupt */
+	dev->gpio_irq.urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!dev->gpio_irq.urb) {
+		dev_err(&pdev->dev, "Cannot allocate the int URB");
+		return -ENOMEM;
+	}
+
+	usb_fill_int_urb(dev->gpio_irq.urb, ch341->usb_dev,
+			 usb_rcvintpipe(ch341->usb_dev, ch341->ep_intr),
+			 dev->gpio_irq.buf, CH341_USB_MAX_INTR_SIZE,
+			 ch341_complete_intr_urb, dev, ch341->ep_intr_interval);
+
+	init_usb_anchor(&dev->gpio_irq.urb_out);
+
+	rc = gpiochip_add_data(gpio, dev);
+	if (rc) {
+		dev_err(&pdev->dev, "Could not add GPIO\n");
+		goto release_urb;
+	}
+
+	return 0;
+
+release_urb:
+	usb_free_urb(dev->gpio_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 v2");
+MODULE_ALIAS("platform:ch341-gpio");
diff --git a/drivers/mfd/ch341-core.c b/drivers/mfd/ch341-core.c
index fbb2897cd956..85e0ae812098 100644
--- a/drivers/mfd/ch341-core.c
+++ b/drivers/mfd/ch341-core.c
@@ -19,6 +19,9 @@
 #include <linux/mfd/ch341.h>
 
 static const struct mfd_cell ch341_devs[] = {
+	{
+		.name = "ch341-gpio",
+	},
 };
 
 static int ch341_usb_probe(struct usb_interface *iface,
-- 
2.32.0

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

[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 | 325 +++++++++++++++++++++++++++++++++
 drivers/mfd/ch341-core.c       |   3 +
 5 files changed, 340 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ch341.c

diff --git a/MAINTAINERS b/MAINTAINERS
index fdff76a5d9b0..ba367013b463 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -20850,6 +20850,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 8a6c6ee28556..987645220d20 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1178,6 +1178,16 @@ config I2C_RCAR
 
 comment "External I2C/SMBus adapter drivers"
 
+config I2C_CH341
+	tristate "CH341 USB adapter in GPIO/I2C/SPI mode"
+	depends 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/Makefile b/drivers/i2c/busses/Makefile
index 1d00dce77098..bca529c325b8 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -123,6 +123,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..f1fbc6e349fa
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ch341.c
@@ -0,0 +1,325 @@
+// 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/i2c.h>
+#include <linux/platform_device.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
+
+#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);
+	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);
+
+		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);
+	mutex_unlock(&ch341->usb_lock);
+	if (rc < 0) {
+		dev_err(&pdev->dev, "Cannot set I2C speed\n");
+		return rc;
+	}
+
+	return devm_i2c_add_adapter(&pdev->dev, &ch341_i2c->adapter);
+}
+
+static struct platform_driver ch341_i2c_driver = {
+	.driver.name	= "ch341-i2c",
+	.driver.owner	= THIS_MODULE,
+	.probe		= ch341_i2c_probe,
+};
+
+module_platform_driver(ch341_i2c_driver);
+
+MODULE_AUTHOR("Various");
+MODULE_DESCRIPTION("CH341 USB to I2C");
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("platform:ch341-i2c");
diff --git a/drivers/mfd/ch341-core.c b/drivers/mfd/ch341-core.c
index 85e0ae812098..400e44bd71ef 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 v4 1/3] mfd: ch341: add core driver for the WCH CH341 in I2C/GPIO mode

[Andy Shevchenko]

On Mon, Mar 21, 2022 at 11:17 AM frank zago <frank@zago.net> 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.

...

> +The driver doesn't support detection of I2C device present on the

devices

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

...

>     spear-pcie-gadget
>     uacce
>     xilinx_sdfec
> +   ch341

Seems you broke the order.

...

> +config MFD_CH341

> +       tristate "WinChipHead CH341 in I2C/SPI/GPIO mode"

(1)

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

"chips running" (no comma needed)

> +         The chip's SPI mode is not supported.

Maybe no need to include SPI in the (1) along with dropping this line?

> +         This driver can also be built as a module.  If so, the
> +         module will be called ch341-core.

...

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>

(2)

> +#include <linux/mfd/core.h>

> +
> +#include <linux/mfd/ch341.h>

Moving these two to (2) ?

...

> +       dev = kzalloc(sizeof(*dev), GFP_KERNEL);

devm_kzalloc() ?

> +       if (!dev)
> +               return -ENOMEM;

...

> +       rc = mfd_add_hotplug_devices(&iface->dev, ch341_devs,
> +                                    ARRAY_SIZE(ch341_devs));

> +       if (rc) {

> +               dev_err(&iface->dev, "Failed to add mfd devices to core.");
> +               goto free_dev;

return dev_err_probe(...); ?

> +       }

...

> +       usb_set_intfdata(dev->iface, NULL);

This has been done by device driver core  for the past 10+ years.

...

> +static const struct usb_device_id ch341_usb_table[] = {
> +       { USB_DEVICE(0x1a86, 0x5512) },
> +       { }
> +};

> +

Redundant blank line.

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

Keep a comma to avoid unneeded churn in the future.

> +};

> +

Redundant blank line.

> +module_usb_driver(ch341_usb_driver);


> +/*
> + * Definitions for CH341 MFD driver
> + */

One line?

...

> +#include <linux/usb.h>

No users of this header. Use forward declarations.

> +#include <linux/mutex.h>

Missed types.h.

...

> +#define DEFAULT_TIMEOUT 1000   /* 1s USB requests timeout */

Use proper suffix, i.e. _MS.

-- 
With Best Regards,
Andy Shevchenko

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

[Andy Shevchenko]

On Mon, Mar 21, 2022 at 4:13 PM frank zago <frank@zago.net> wrote:
>
> The GPIO interface offers 16 GPIOs. 6 are read/write, and 10 are
> read-only.

We use terminology of output-only and input-only. Is it what you are
telling us? If it's something else, you have to elaborate much better
on what's going on with these GPIO lines.

...

> +config GPIO_CH341
> +       tristate "CH341 USB adapter in GPIO/I2C/SPI mode"

How is this driver related to either SPI or I²C modes?

> +       depends on MFD_CH341

Can't be compile tested?

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

...

> +/* Notes.

Keep the proper (not network) style for multi-line comments.


> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/gpio.h>

> +#include <linux/mfd/ch341.h>

If I got your intention with groups of headers, I would see rather

...ordered headers...
blank line
#include <linux/gpio.h>

But more importantly that gpio.h is the wrong header and must be
replaced with the appropriate one from the include/gpio/ folder.

Also you have missed headers, like types.h.

...

> +#define CH341_PARA_CMD_STS          0xA0  /* Get pins status */
> +#define CH341_CMD_UIO_STREAM        0xAB  /* UIO stream command */
> +
> +#define CH341_CMD_UIO_STM_OUT       0x80  /* UIO interface OUT command (D0~D5) */
> +#define CH341_CMD_UIO_STM_DIR       0x40  /* UIO interface DIR command (D0~D5) */
> +#define CH341_CMD_UIO_STM_END       0x20  /* UIO interface END command */

What does UIO mean here? If it is Userspace I/O in terms of Linux
kernel, it's no-go we want this. Otherwise needs to be explained
somewhere.

...

> +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 */
> +       u8 gpio_buf[SEG_SIZE];
> +
> +       struct {
> +               char name[32];
> +               bool enabled;
> +               struct irq_chip irq;
> +               int num;
> +               struct urb *urb;
> +               struct usb_anchor urb_out;
> +               u8 buf[CH341_USB_MAX_INTR_SIZE];
> +       } gpio_irq;

We have a specific GPIO IRQ chip structure, what is the purpose of
semi-duplication of it?

> +
> +       struct ch341_device *ch341;
> +};

...

> +static void ch341_gpio_dbg_show(struct seq_file *s, struct gpio_chip *chip)
> +{
> +       struct ch341_gpio *dev = gpiochip_get_data(chip);
> +
> +       seq_printf(s, "pin config  : %04x  (0=IN, 1=OUT)\n", dev->gpio_dir);
> +       seq_printf(s, "last read   : %04x\n", dev->gpio_last_read);
> +       seq_printf(s, "last written: %04x\n", dev->gpio_last_written);

Multi-line debug output is quite non-standard among GPIO drivers.

> +}

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

> +       if (rc < 0)
> +               goto done;
> +
> +       if (in_len == 0) {
> +               rc = actual;
> +               goto done;
> +       }

You may do it better. See below.

> +       rc = usb_bulk_msg(ch341->usb_dev,
> +                         usb_rcvbulkpipe(ch341->usb_dev, ch341->ep_in),
> +                         dev->gpio_buf, SEG_SIZE, &actual, DEFAULT_TIMEOUT);
> +
> +       if (rc == 0)
> +               rc = actual;

> +done:

out_unlock: sounds better.

> +       mutex_unlock(&ch341->usb_lock);

> +       return rc;

if (rc < 0)
  return rc;

return actual;

> +}

...

> +       int result;

rc / result / etc... Please, become consistent in naming the return
code variable.

...

> +       if (result == 6)
> +               dev->gpio_last_read = le16_to_cpu(*(__le16 *)dev->gpio_buf);

So, it means you have the wrong type of gpio_but. Also you missed the
pointer versions of leXX_to_cpu() helpers.

...

> +       return (result != 6) ? result : 0;

Besides redundant parentheses, this can be optimized. I will leave it
for your homework (the hint is given at the top part of the review).

...

> +       return (dev->gpio_last_read & BIT(offset)) ? 1 : 0;

!! can be used. But it's up to you and maintainers, the compiler will
do its job anyway.

...

> +       dev->gpio_last_written &= ~*mask;
> +       dev->gpio_last_written |= (*bits & *mask);

Can be done in one line as it's a well established pattern in Linux
kernel for drivers.

...

> +       return (dev->gpio_dir & BIT(offset)) ? 0 : 1;

! will do the job.

...

> +       if (!(pin_can_output & mask))
> +               return -EINVAL;

I don't remember if we have a valid mask for this case.

...

> +       if (!urb->status) {

Will be much better to simply do:

if (urb_status) {
 ...
 return;
}

> +       } else {
> +               usb_unanchor_urb(dev->gpio_irq.urb);
> +       }

...

> +       if (data->irq != dev->gpio_irq.num || type != IRQ_TYPE_EDGE_RISING)
> +               return -EINVAL;

Usually we lock the handler type here while in ->probe() we assign a
bad handler by default in order to filter out spurious interrupts.

...

> +       dev->gpio_irq.enabled = true;

What is the purpose of this flag? Note there is a patch to add a
specific flag to the descriptor to do exactly this.

...

> +/* Convert the GPIO index to the IRQ number */
> +static int ch341_gpio_to_irq(struct gpio_chip *chip, unsigned int offset)
> +{
> +       struct ch341_gpio *dev = gpiochip_get_data(chip);
> +
> +       if (offset != CH341_GPIO_INT_LINE)
> +               return -ENXIO;
> +
> +       return dev->gpio_irq.num;

In the new code we will have the special field that limits the GPIO
IRQ lines (can be different to the ngpio).

> +}

...

> +       snprintf(dev->gpio_irq.name, sizeof(dev->gpio_irq.name),
> +                "ch341-%s-gpio", dev_name(&ch341->usb_dev->dev));

> +       dev->gpio_irq.name[sizeof(dev->gpio_irq.name) - 1] = 0;

This is redundant. Have you read the manual page on snprintf()?

...

> +       rc = devm_irq_alloc_desc(&pdev->dev, 0);
> +       if (rc < 0) {

> +               dev_err(&pdev->dev, "Cannot allocate an IRQ desc");
> +               return rc;

return dev_err_probe();

> +       }
> +
> +       dev->gpio_irq.num = rc;
> +       dev->gpio_irq.enabled = false;
> +
> +       irq_set_chip_data(dev->gpio_irq.num, dev);
> +       irq_set_chip_and_handler(dev->gpio_irq.num, &dev->gpio_irq.irq,
> +                                handle_simple_irq);

Oh là là. Can you use the latest and greatest approach of
instantiating the GPIO IRQ chip?

...

> +               dev_err(&pdev->dev, "Cannot allocate the int URB");
> +               return -ENOMEM;

return dev_err_probe();

...

> +       rc = gpiochip_add_data(gpio, dev);

Why not devm?

> +       if (rc) {
> +               dev_err(&pdev->dev, "Could not add GPIO\n");
> +               goto release_urb;

return dev_err_probe();

> +       }

...

> +static struct platform_driver ch341_gpio_driver = {
> +       .driver.name    = "ch341-gpio",
> +       .probe          = ch341_gpio_probe,
> +       .remove         = ch341_gpio_remove,
> +};
> +

Redundant blank line.

> +module_platform_driver(ch341_gpio_driver);

-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

Hi Frank,

thanks for your patch!

I see you already got a bunch of homework from Andy, I will do a more
thorough review on the next iteration, just a few things:

On Mon, Mar 21, 2022 at 5:21 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>
(...)
> +config GPIO_CH341
> +       tristate "CH341 USB adapter in GPIO/I2C/SPI mode"
> +       depends on MFD_CH341

I would add
default MFD_CD341

This way it gets selected automatically if the MFD module gets
selected. (I suspect you should do the same with the I2C module).

> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/platform_device.h>
> +#include <linux/mfd/core.h>
> +#include <linux/gpio.h>

Use <linux/gpio/driver.h>

Yours,
Linus Walleij