[PATCH 0/3] usb/gpio/i2c: Add Intel USBIO USB IO-expander drivers

[PATCH 0/3] usb/gpio/i2c: Add Intel USBIO USB IO-expander drivers

[Hans de Goede]

Hi All,

This patch series adds support for the Intel USBIO USB IO-expander used by
the MIPI cameras on various new (Meteor Lake and later) Intel laptops.

The first patch adds an USB bridge driver which registers auxbus children
for the GPIO and I2C functions of the USBIO chip.

The second and third patch add a GPIO resp. an I2C driver for the
auxbus children using the IO functions exported by the USB bridge driver.

The second and third patch depend on the IO functions exported by
the first patch. So to merge this we will need either an immutable tag on
the USB tree, or all 3 patches can be merged through the USB tree with
acks from the GPIO and I2C subsystem maintainers.

Regards,

Hans


Israel Cepeda (3):
  usb: misc: Add Intel USBIO bridge driver
  gpio: Add Intel USBIO GPIO driver
  i2c: Add Intel USBIO I2C driver

 MAINTAINERS                    |   9 +
 drivers/gpio/Kconfig           |  11 +
 drivers/gpio/Makefile          |   1 +
 drivers/gpio/gpio-usbio.c      | 258 ++++++++++++
 drivers/i2c/busses/Kconfig     |  11 +
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-usbio.c | 344 ++++++++++++++++
 drivers/usb/misc/Kconfig       |  14 +
 drivers/usb/misc/Makefile      |   1 +
 drivers/usb/misc/usbio.c       | 693 +++++++++++++++++++++++++++++++++
 include/linux/usb/usbio.h      | 168 ++++++++
 11 files changed, 1511 insertions(+)
 create mode 100644 drivers/gpio/gpio-usbio.c
 create mode 100644 drivers/i2c/busses/i2c-usbio.c
 create mode 100644 drivers/usb/misc/usbio.c
 create mode 100644 include/linux/usb/usbio.h

-- 
2.49.0

[PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Hans de Goede]

From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>

Add a driver for the Intel USBIO USB IO-expander used by the MIPI cameras
on various new (Meteor Lake and later) Intel laptops.

This is an USB bridge driver which adds auxbus child devices for the GPIO,
I2C and SPI functions of the USBIO chip and which exports IO-functions for
the drivers for the auxbus child devices to communicate with the USBIO
device's firmware.

Co-developed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
---
 MAINTAINERS               |   7 +
 drivers/usb/misc/Kconfig  |  14 +
 drivers/usb/misc/Makefile |   1 +
 drivers/usb/misc/usbio.c  | 693 ++++++++++++++++++++++++++++++++++++++
 include/linux/usb/usbio.h | 168 +++++++++
 5 files changed, 883 insertions(+)
 create mode 100644 drivers/usb/misc/usbio.c
 create mode 100644 include/linux/usb/usbio.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 0618351510ad..1208efe41f9f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12518,6 +12518,13 @@ S:	Maintained
 F:	Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
 F:	drivers/platform/x86/intel/uncore-frequency/
 
+INTEL USBIO USB I/O EXPANDER DRIVERS
+M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
+M:	Hans de Goede <hansg@kernel.org>
+S:	Maintained
+F:	drivers/usb/misc/usbio.c
+F:	include/linux/usb/usbio.h
+
 INTEL VENDOR SPECIFIC EXTENDED CAPABILITIES DRIVER
 M:	David E. Box <david.e.box@linux.intel.com>
 S:	Supported
diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 6497c4e81e95..bfe08e15d051 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -178,6 +178,20 @@ config USB_LJCA
 	  This driver can also be built as a module. If so, the module
 	  will be called usb-ljca.
 
+config USB_USBIO
+	tristate "Intel USBIO Bridge support"
+	depends on USB && ACPI
+	select AUXILIARY_BUS
+	help
+	  This adds support for Intel USBIO drivers.
+	  This enables the USBIO bridge driver module in charge to talk
+	  to the USB device. Additional drivers such as GPIO_USBIO and
+	  I2C_USBIO must be enabled in order to use the device's full
+	  functionality.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called usbio.
+
 source "drivers/usb/misc/sisusbvga/Kconfig"
 
 config USB_LD
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 0cd5bc8f52fe..494ab0377f35 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_USB_EMI62)			+= emi62.o
 obj-$(CONFIG_USB_EZUSB_FX2)		+= ezusb.o
 obj-$(CONFIG_APPLE_MFI_FASTCHARGE)	+= apple-mfi-fastcharge.o
 obj-$(CONFIG_USB_LJCA)			+= usb-ljca.o
+obj-$(CONFIG_USB_USBIO)			+= usbio.o
 obj-$(CONFIG_USB_IDMOUSE)		+= idmouse.o
 obj-$(CONFIG_USB_IOWARRIOR)		+= iowarrior.o
 obj-$(CONFIG_USB_ISIGHTFW)		+= isight_firmware.o
diff --git a/drivers/usb/misc/usbio.c b/drivers/usb/misc/usbio.c
new file mode 100644
index 000000000000..88197092f39a
--- /dev/null
+++ b/drivers/usb/misc/usbio.c
@@ -0,0 +1,693 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Intel USBIO Bridge driver
+ *
+ * Copyright (c) 2025 Intel Corporation.
+ * Copyright (c) 2025 Red Hat, Inc.
+ */
+
+#include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/completion.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/lockdep.h>
+#include <linux/mutex.h>
+#include <linux/string.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+#include <linux/usb/usbio.h>
+
+/*************************************
+ * USBIO Bridge Protocol Definitions *
+ *************************************/
+
+/* USBIO Control Commands */
+#define USBIO_CTRLCMD_PROTVER	0
+#define USBIO_CTRLCMD_FWVER	1
+#define USBIO_CTRLCMD_HS	2
+#define USBIO_CTRLCMD_ENUMGPIO	16
+#define USBIO_CTRLCMD_ENUMI2C	17
+#define USBIO_CTRLCMD_ENUMSPI	18
+
+/* USBIO Packet Flags */
+#define USBIO_PKTFLAG_ACK	BIT(0)
+#define USBIO_PKTFLAG_RSP	BIT(1)
+#define USBIO_PKTFLAG_CMP	BIT(2)
+#define USBIO_PKTFLAG_ERR	BIT(3)
+
+#define USBIO_PKTFLAGS_REQRESP	(USBIO_PKTFLAG_CMP | USBIO_PKTFLAG_ACK)
+
+#define USBIO_CTRLXFER_TIMEOUT 0
+#define USBIO_BULKXFER_TIMEOUT 100
+
+struct usbio_protver {
+	uint8_t ver;
+} __packed;
+
+struct usbio_fwver {
+	uint8_t major;
+	uint8_t minor;
+	uint16_t patch;
+	uint16_t build;
+} __packed;
+
+#define USBIO_SPI_BUS_WIRE_CAP_3W	BIT(3) /* 3 wires support */
+#define USBIO_SPI_BUS_ADDR_CAP_MASK	0x3
+#define USBIO_SPI_BUS_ADDR_CAP_CS0	0 /* CS0 */
+#define USBIO_SPI_BUS_ADDR_CAP_CS1	1 /* CS0/1 */
+#define USBIO_SPI_BUS_ADDR_CAP_CS2	2 /* CS0/1/2 */
+#define USBIO_SPI_BUS_ADDR_CAP_CS3	3 /* CS0/1/2/3 */
+
+struct usbio_spi_bus_desc {
+	uint8_t id;
+	uint8_t caps;
+} __packed;
+
+/***********************************
+ * USBIO Bridge Device Definitions *
+ ***********************************/
+struct usbio_dev_info {
+	struct usbio_protver protver;
+	struct usbio_fwver fwver;
+};
+
+#define MAX_SPIBUSES	5
+
+/**
+ * struct usbio_device - the usb device exposing IOs
+ *
+ * @dev: the device in the usb interface
+ * @udev: the detected usb device
+ * @intf: the usb interface
+ * @ctrl_pipe: the control transfer pipe
+ * @ctrlbuf_len: the size of the control transfer pipe
+ * @ctrlbuf: the buffer used for control transfers
+ * @tx_pipe: the bulk out pipe
+ * @txbuf_len: the size of the bulk out pipe
+ * @txbuf: the buffer used for bulk out transfers
+ * @urb: the urb to read bulk pipe
+ * @rx_pipe: the bulk in pipe
+ * @rxbuf_len: the size of the bulk in pipe
+ * @rxdat_len: the data length at rx buffer
+ * @rxbuf: the buffer used for bulk in transfers
+ * @info: the device's protocol and firmware information
+ * @done: completion object as request is done
+ * @mutex: protection against access concurrency
+ * @cli_list: device's client list
+ */
+struct usbio_device {
+	struct device *dev;
+	struct usb_device *udev;
+	struct usb_interface *intf;
+
+	unsigned int ctrl_pipe;
+	u16 ctrlbuf_len;
+	void *ctrlbuf;
+
+	unsigned int tx_pipe;
+	u16 txbuf_len;
+	void *txbuf;
+
+	struct urb *urb;
+	unsigned int rx_pipe;
+	u16 rxbuf_len;
+	u16 rxdat_len;
+	void *rxbuf;
+
+	struct usbio_dev_info info;
+
+	struct completion done;
+	struct mutex mutex;
+
+	struct list_head cli_list;
+
+	unsigned int nr_gpio_banks;
+	struct usbio_gpio_bank_desc gpios[USBIO_MAX_GPIOBANKS];
+
+	unsigned int nr_i2c_buses;
+	struct usbio_i2c_bus_desc i2cs[USBIO_MAX_I2CBUSES];
+
+	unsigned int nr_spi_buses;
+	struct usbio_spi_bus_desc spis[MAX_SPIBUSES];
+};
+
+/**
+ * struct usbio_client - represents a usbio client
+ *
+ * @adev: auxiliary device object
+ * @bridge: usbio bridge who service the client
+ * @link: usbio bridge clients list member
+ */
+struct usbio_client {
+	struct auxiliary_device adev;
+	struct usbio_device *bridge;
+	struct list_head link;
+};
+
+#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
+
+static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
+			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
+{
+	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
+	struct usbio_ctrl_packet *cpkt;
+	unsigned int pipe;
+	u16 cpkt_len;
+	int ret;
+
+	lockdep_assert_held(&usbio->mutex);
+
+	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
+	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
+		return -EMSGSIZE;
+
+	/* Prepare Control Packet Header */
+	cpkt = usbio->ctrlbuf;
+	cpkt->header.type = type;
+	cpkt->header.cmd = cmd;
+	if (type == USBIO_PKTTYPE_CTRL || ibuf_len)
+		cpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
+	else
+		cpkt->header.flags = USBIO_PKTFLAG_CMP;
+	cpkt->len = obuf_len;
+
+	/* Copy the data */
+	memcpy(cpkt->data, obuf, obuf_len);
+
+	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
+	cpkt_len = sizeof(*cpkt) + obuf_len;
+	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
+			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
+	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
+		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
+
+	if (ret != cpkt_len) {
+		dev_err(usbio->dev, "USB control out failed: %d\n", ret);
+		return (ret < 0) ? ret : -EPROTO;
+	}
+
+	if (!(cpkt->header.flags & USBIO_PKTFLAG_ACK))
+		return 0;
+
+	pipe = usb_rcvctrlpipe(usbio->udev, usbio->ctrl_pipe);
+	cpkt_len = sizeof(*cpkt) + ibuf_len;
+	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
+			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
+	dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
+		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
+
+	if (ret < sizeof(*cpkt)) {
+		dev_err(usbio->dev, "USB control in failed: %d\n", ret);
+		return (ret < 0) ? ret : -EPROTO;
+	}
+
+	if (cpkt->header.type != type || cpkt->header.cmd != cmd ||
+	    !(cpkt->header.flags & USBIO_PKTFLAG_RSP)) {
+		dev_err(usbio->dev, "Unexpected reply type: %u, cmd: %u, flags: %u\n",
+			cpkt->header.type, cpkt->header.cmd, cpkt->header.flags);
+		return -EPROTO;
+	}
+
+	if (cpkt->header.flags & USBIO_PKTFLAG_ERR)
+		return -EREMOTEIO;
+
+	if (ibuf_len < cpkt->len)
+		return -ENOSPC;
+
+	memcpy(ibuf, cpkt->data, cpkt->len);
+	return cpkt->len;
+}
+
+int usbio_control_msg(struct auxiliary_device *adev, u8 type, u8 cmd,
+		      const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
+{
+	struct usbio_client *client = adev_to_client(adev);
+	int ret;
+
+	ret = usbio_acquire(adev);
+	if (ret)
+		return ret;
+
+	ret = usbio_ctrl_msg(client->bridge, type, cmd, obuf, obuf_len, ibuf, ibuf_len);
+
+	usbio_release(adev);
+
+	return ret;
+}
+EXPORT_SYMBOL_NS_GPL(usbio_control_msg, "USBIO");
+
+static void usbio_bulk_recv(struct urb *urb)
+{
+	struct usbio_bulk_packet *bpkt = urb->transfer_buffer;
+	struct usbio_device *usbio = urb->context;
+
+	if (!urb->status) {
+		if (bpkt->header.flags & USBIO_PKTFLAG_RSP) {
+			usbio->rxdat_len = urb->actual_length;
+			complete(&usbio->done);
+		}
+	} else if (urb->status != -ENOENT) {
+		dev_err(usbio->dev, "Bulk in error %d\n", urb->status);
+	}
+
+	usb_submit_urb(usbio->urb, GFP_ATOMIC);
+}
+
+int usbio_bulk_msg(struct auxiliary_device *adev, u8 type, u8 cmd, bool last,
+		   const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
+{
+	struct usbio_device *usbio = adev_to_client(adev)->bridge;
+	struct usbio_bulk_packet *bpkt;
+	int ret, act;
+	u16 bpkt_len;
+
+	lockdep_assert_held(&usbio->mutex);
+
+	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
+	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
+		return -EMSGSIZE;
+
+	if (ibuf_len)
+		reinit_completion(&usbio->done);
+
+	/* If no data to send, skip to read */
+	if (!obuf_len)
+		goto read;
+
+	/* Prepare Bulk Packet Header */
+	bpkt = usbio->txbuf;
+	bpkt->header.type = type;
+	bpkt->header.cmd = cmd;
+	if (!last)
+		bpkt->header.flags = 0;
+	else if (ibuf_len)
+		bpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
+	else
+		bpkt->header.flags = USBIO_PKTFLAG_CMP;
+	bpkt->len = obuf_len;
+
+	/* Copy the data */
+	memcpy(bpkt->data, obuf, obuf_len);
+
+	bpkt_len = sizeof(*bpkt) + obuf_len;
+	ret = usb_bulk_msg(usbio->udev, usbio->tx_pipe, bpkt, bpkt_len, &act,
+			   USBIO_BULKXFER_TIMEOUT);
+	dev_dbg(usbio->dev, "bulk out %d hdr %*phN data %*phN\n", act,
+		(int)sizeof(*bpkt), bpkt, (int)bpkt->len, bpkt->data);
+
+	if (ret || act != bpkt_len) {
+		dev_err(usbio->dev, "Bulk out failed: %d\n", ret);
+		return ret ?: -EPROTO;
+	}
+
+	if (!(bpkt->header.flags & USBIO_PKTFLAG_ACK))
+		return obuf_len;
+
+read:
+	ret = wait_for_completion_timeout(&usbio->done, USBIO_BULKXFER_TIMEOUT);
+	if (ret <= 0) {
+		dev_err(usbio->dev, "Bulk in wait failed: %d\n", ret);
+		return ret ?: -ETIMEDOUT;
+	}
+
+	act = usbio->rxdat_len;
+	bpkt = usbio->rxbuf;
+	dev_dbg(usbio->dev, "bulk in %d hdr %*phN data %*phN\n", act,
+		(int)sizeof(*bpkt), bpkt, (int)bpkt->len, bpkt->data);
+
+	/*
+	 * Unsupported bulk commands get only an usbio_packet_header with
+	 * the error flag set as reply. Return -EPIPE for this case.
+	 */
+	if (act == sizeof(struct usbio_packet_header) &&
+	    (bpkt->header.flags & USBIO_PKTFLAG_ERR))
+		return -EPIPE;
+
+	if (act < sizeof(*bpkt)) {
+		dev_err(usbio->dev, "Bulk in short read: %d\n", act);
+		return -EPROTO;
+	}
+
+	if (bpkt->header.type != type || bpkt->header.cmd != cmd ||
+	    !(bpkt->header.flags & USBIO_PKTFLAG_RSP)) {
+		dev_err(usbio->dev,
+			"Unexpected bulk in type 0x%02x cmd 0x%02x flags 0x%02x\n",
+			bpkt->header.type, bpkt->header.cmd, bpkt->header.flags);
+		return -EPROTO;
+	}
+
+	if (bpkt->header.flags & USBIO_PKTFLAG_ERR)
+		return -EREMOTEIO;
+
+	if (ibuf_len < bpkt->len)
+		return -ENOSPC;
+
+	memcpy(ibuf, bpkt->data, bpkt->len);
+	return bpkt->len;
+}
+EXPORT_SYMBOL_NS_GPL(usbio_bulk_msg, "USBIO");
+
+static int usbio_get_and_lock(struct usbio_device *usbio)
+{
+	int ret;
+
+	ret = usb_autopm_get_interface(usbio->intf);
+	if (ret)
+		return ret;
+
+	mutex_lock(&usbio->mutex);
+	return 0;
+}
+
+static void usbio_unlock_and_put(struct usbio_device *usbio)
+{
+	mutex_unlock(&usbio->mutex);
+	usb_autopm_put_interface(usbio->intf);
+}
+
+int usbio_acquire(struct auxiliary_device *adev)
+{
+	return usbio_get_and_lock(adev_to_client(adev)->bridge);
+}
+EXPORT_SYMBOL_NS_GPL(usbio_acquire, "USBIO");
+
+void usbio_release(struct auxiliary_device *adev)
+{
+	usbio_unlock_and_put(adev_to_client(adev)->bridge);
+}
+EXPORT_SYMBOL_NS_GPL(usbio_release, "USBIO");
+
+void usbio_get_txrxbuf_len(struct auxiliary_device *adev, u16 *txbuf_len, u16 *rxbuf_len)
+{
+	struct usbio_device *usbio = adev_to_client(adev)->bridge;
+
+	*txbuf_len = usbio->txbuf_len;
+	*rxbuf_len = usbio->rxbuf_len;
+}
+EXPORT_SYMBOL_NS_GPL(usbio_get_txrxbuf_len, "USBIO");
+
+static void usbio_auxdev_release(struct device *dev)
+{
+	struct auxiliary_device *adev = to_auxiliary_dev(dev);
+	struct usbio_client *client = adev_to_client(adev);
+
+	kfree(client);
+}
+
+static int usbio_add_client(struct usbio_device *usbio, char *name, u8 id, void *data)
+{
+	struct usbio_client *client;
+	struct auxiliary_device *adev;
+	int ret;
+
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->bridge = usbio;
+	adev = &client->adev;
+	adev->name = name;
+	adev->id = id;
+
+	adev->dev.parent = usbio->dev;
+	adev->dev.platform_data = data;
+	adev->dev.release = usbio_auxdev_release;
+
+	ret = auxiliary_device_init(adev);
+	if (!ret) {
+		if (auxiliary_device_add(adev))
+			auxiliary_device_uninit(adev);
+	}
+
+	if (ret) {
+		dev_err(usbio->dev, "Client %s.%d add failed: %d\n", name, id, ret);
+		kfree(client);
+		return ret;
+	}
+
+	list_add_tail(&client->link, &usbio->cli_list);
+	return 0;
+}
+
+static int usbio_ctrl_enumgpios(struct usbio_device *usbio)
+{
+	struct usbio_gpio_bank_desc *gpio = usbio->gpios;
+	int ret, i;
+
+	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMGPIO, NULL, 0,
+			     gpio, sizeof(usbio->gpios));
+	if (ret < 0 || ret % sizeof(*gpio)) {
+		dev_err(usbio->dev, "CTRL ENUMGPIO failed: %d\n", ret);
+		return ret;
+	}
+
+	usbio->nr_gpio_banks = ret / sizeof(*gpio);
+	dev_dbg(usbio->dev, "GPIO Banks: %d\n", usbio->nr_gpio_banks);
+	for (i = 0; i < usbio->nr_gpio_banks; i++)
+		dev_dbg(usbio->dev, "\tBank%d[%d] map: %#08x\n",
+			gpio[i].id, gpio[i].pins, gpio[i].bmap);
+
+	usbio_add_client(usbio, USBIO_GPIO_CLIENT, 0, gpio);
+	return 0;
+}
+
+static int usbio_ctrl_enumi2cs(struct usbio_device *usbio)
+{
+	struct usbio_i2c_bus_desc *i2c = usbio->i2cs;
+	int ret, i;
+
+	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMI2C, NULL, 0,
+			     i2c, sizeof(usbio->i2cs));
+	if (ret < 0 || ret % sizeof(*i2c)) {
+		dev_err(usbio->dev, "CTRL ENUMI2C failed: %d\n", ret);
+		return ret;
+	}
+
+	usbio->nr_i2c_buses = ret / sizeof(*i2c);
+	dev_dbg(usbio->dev, "I2C Busses: %d\n", usbio->nr_i2c_buses);
+	for (i = 0; i < usbio->nr_i2c_buses; i++) {
+		dev_dbg(usbio->dev, "\tBus%d caps: %#02x\n", i2c[i].id, i2c[i].caps);
+		usbio_add_client(usbio, USBIO_I2C_CLIENT, i, &i2c[i]);
+	}
+
+	return 0;
+}
+
+static int usbio_ctrl_enumspis(struct usbio_device *usbio)
+{
+	struct usbio_spi_bus_desc *spi = usbio->spis;
+	int ret, i;
+
+	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMSPI, NULL, 0,
+			     spi, sizeof(usbio->spis));
+	if (ret < 0 || ret % sizeof(*spi)) {
+		dev_err(usbio->dev, "CTRL ENUMSPI failed: %d\n", ret);
+		return ret;
+	}
+
+	usbio->nr_spi_buses = ret / sizeof(*spi);
+	dev_dbg(usbio->dev, "SPI Busses: %d\n", usbio->nr_spi_buses);
+	for (i = 0; i < usbio->nr_spi_buses; i++)
+		dev_dbg(usbio->dev, "\tBus%d caps: %#02x\n", spi[i].id, spi[i].caps);
+
+	return 0;
+}
+
+static int usbio_suspend(struct usb_interface *intf, pm_message_t msg)
+{
+	struct usbio_device *usbio = usb_get_intfdata(intf);
+
+	usb_kill_urb(usbio->urb);
+
+	return 0;
+}
+
+static int usbio_resume(struct usb_interface *intf)
+{
+	struct usbio_device *usbio = usb_get_intfdata(intf);
+
+	return usb_submit_urb(usbio->urb, GFP_KERNEL);
+}
+
+static void usbio_disconnect(struct usb_interface *intf)
+{
+	struct usbio_device *usbio = usb_get_intfdata(intf);
+	struct usbio_client *client, *prev;
+
+	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
+		auxiliary_device_delete(&client->adev);
+		list_del_init(&client->link);
+		auxiliary_device_uninit(&client->adev);
+	}
+
+	usb_kill_urb(usbio->urb);
+	usb_free_urb(usbio->urb);
+}
+
+static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
+{
+	struct usb_device *udev = interface_to_usbdev(intf);
+	struct usb_endpoint_descriptor *ep_in, *ep_out;
+	struct device *dev = &intf->dev;
+	struct usbio_device *usbio;
+	int ret;
+
+	usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);
+	if (!usbio)
+		return -ENOMEM;
+
+	ret = devm_mutex_init(dev, &usbio->mutex);
+	if (ret)
+		return ret;
+
+	usbio->dev = dev;
+	usbio->udev = udev;
+	usbio->intf = intf;
+	init_completion(&usbio->done);
+	INIT_LIST_HEAD(&usbio->cli_list);
+	usb_set_intfdata(intf, usbio);
+
+	usbio->ctrl_pipe = usb_endpoint_num(&udev->ep0.desc);
+	usbio->ctrlbuf_len = usb_maxpacket(udev, usbio->ctrl_pipe);
+	usbio->ctrlbuf = devm_kzalloc(dev, usbio->ctrlbuf_len, GFP_KERNEL);
+	if (!usbio->ctrlbuf)
+		return -ENOMEM;
+
+	/* Find the first bulk-in and bulk-out endpoints */
+	ret = usb_find_common_endpoints(intf->cur_altsetting, &ep_in, &ep_out,
+					NULL, NULL);
+	if (ret) {
+		dev_err(dev, "Cannot find bulk endpoints: %d\n", ret);
+		return ret;
+	}
+
+	usbio->tx_pipe = usb_sndbulkpipe(udev, usb_endpoint_num(ep_out));
+	usbio->txbuf_len = usb_endpoint_maxp(ep_out);
+	usbio->txbuf = devm_kzalloc(dev, usbio->txbuf_len, GFP_KERNEL);
+	if (!usbio->txbuf)
+		return -ENOMEM;
+
+	usbio->rx_pipe = usb_rcvbulkpipe(udev, usb_endpoint_num(ep_in));
+	usbio->rxbuf_len = usb_endpoint_maxp(ep_in);
+	usbio->rxbuf = devm_kzalloc(dev, usbio->rxbuf_len, GFP_KERNEL);
+	if (!usbio->rxbuf)
+		return -ENOMEM;
+
+	usbio->urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!usbio->urb)
+		return -ENOMEM;
+
+	usb_fill_bulk_urb(usbio->urb, udev, usbio->rx_pipe, usbio->rxbuf,
+			  usbio->rxbuf_len, usbio_bulk_recv, usbio);
+	ret = usb_submit_urb(usbio->urb, GFP_KERNEL);
+	if (ret)
+		return dev_err_probe(dev, ret, "Submitting usb urb\n");
+
+	ret = usbio_get_and_lock(usbio);
+	if (ret)
+		return ret;
+
+	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_HS, NULL, 0, NULL, 0);
+	if (ret < 0)
+		goto error;
+
+	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_PROTVER, NULL, 0,
+			     &usbio->info.protver, sizeof(struct usbio_protver));
+	if (ret < 0)
+		goto error;
+
+	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_FWVER, NULL, 0,
+			     &usbio->info.fwver, sizeof(struct usbio_fwver));
+	if (ret < 0)
+		goto error;
+
+	dev_dbg(dev, "ProtVer(BCD): %02x FwVer: %d.%d.%d.%d\n",
+		usbio->info.protver.ver, usbio->info.fwver.major,
+		usbio->info.fwver.minor, usbio->info.fwver.patch,
+		usbio->info.fwver.build);
+
+	usbio_ctrl_enumgpios(usbio);
+	usbio_ctrl_enumi2cs(usbio);
+	usbio_ctrl_enumspis(usbio);
+	usbio_unlock_and_put(usbio);
+	return 0;
+
+error:
+	usbio_unlock_and_put(usbio);
+	usbio_disconnect(intf);
+	return ret;
+}
+
+static const struct usb_device_id usbio_table[] = {
+	{ USB_DEVICE(0x2AC1, 0x20C1) }, /* Lattice NX40 */
+	{ USB_DEVICE(0x2AC1, 0x20C9) }, /* Lattice NX33 */
+	{ USB_DEVICE(0x2AC1, 0x20CB) }, /* Lattice NX33U */
+	{ USB_DEVICE(0x06CB, 0x0701) }, /* Synaptics Sabre */
+	{ }
+};
+MODULE_DEVICE_TABLE(usb, usbio_table);
+
+static struct usb_driver usbio_driver = {
+	.name = "usbio-bridge",
+	.probe = usbio_probe,
+	.disconnect = usbio_disconnect,
+	.suspend = usbio_suspend,
+	.resume = usbio_resume,
+	.id_table = usbio_table,
+	.supports_autosuspend = 1
+};
+module_usb_driver(usbio_driver);
+
+struct usbio_match_ids_walk_data {
+	struct acpi_device *adev;
+	const struct acpi_device_id *hids;
+	const u8 id;
+};
+
+static int usbio_match_device_ids(struct acpi_device *adev, void *data)
+{
+	struct usbio_match_ids_walk_data *wd = data;
+	unsigned int id = 0;
+	char *uid;
+
+	if (!acpi_match_device_ids(adev, wd->hids)) {
+		uid = acpi_device_uid(adev);
+		if (uid)
+			for (int i = 0; i < strlen(uid); i++)
+				if (!kstrtouint(&uid[i], 10, &id))
+					break;
+
+		if (!uid || (uid && wd->id == (u8)id)) {
+			wd->adev = adev;
+			return 1;
+		}
+	}
+
+	return 0;
+}
+
+void usbio_acpi_bind(struct auxiliary_device *adev, const struct acpi_device_id *hids)
+{
+	struct device *dev = &adev->dev;
+	struct acpi_device *parent;
+	struct usbio_match_ids_walk_data wd = {
+		.adev = NULL,
+		.hids = hids,
+		.id = adev->id,
+	};
+
+	parent = ACPI_COMPANION(dev->parent);
+	if (!parent)
+		return;
+
+	acpi_dev_for_each_child(parent, usbio_match_device_ids, &wd);
+	if (wd.adev)
+		ACPI_COMPANION_SET(dev, wd.adev);
+}
+EXPORT_SYMBOL_NS_GPL(usbio_acpi_bind, "USBIO");
+
+MODULE_DESCRIPTION("Intel USBIO Bridge driver");
+MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/usbio.h b/include/linux/usb/usbio.h
new file mode 100644
index 000000000000..9c34b157664f
--- /dev/null
+++ b/include/linux/usb/usbio.h
@@ -0,0 +1,168 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (c) 2025 Intel Corporation.
+ *
+ */
+
+#ifndef _LINUX_USBIO_H_
+#define _LINUX_USBIO_H_
+
+#include <linux/auxiliary_bus.h>
+#include <linux/list.h>
+#include <linux/types.h>
+
+/***********************
+ * USBIO Clients Names *
+ ***********************/
+#define USBIO_GPIO_CLIENT	"usbio-gpio"
+#define USBIO_I2C_CLIENT	"usbio-i2c"
+#define USBIO_SPI_CLIENT	"usbio-spi"
+
+/**************************
+ * USBIO Type Definitions *
+ **************************/
+
+/* USBIO Packet Type */
+#define USBIO_PKTTYPE_CTRL	1
+#define USBIO_PKTTYPE_DBG	2
+#define USBIO_PKTTYPE_GPIO	3
+#define USBIO_PKTTYPE_I2C	4
+#define USBIO_PKTTYPE_SPI	5
+
+/* USBIO Packet Header */
+struct usbio_packet_header {
+	uint8_t type;
+	uint8_t cmd;
+	uint8_t flags;
+} __packed;
+
+/* USBIO Control Transfer Packet */
+struct usbio_ctrl_packet {
+	struct usbio_packet_header header;
+	uint8_t len;
+	uint8_t data[] __counted_by(len);
+} __packed;
+
+/* USBIO Bulk Transfer Packet */
+struct usbio_bulk_packet {
+	struct usbio_packet_header header;
+	uint16_t len;
+	uint8_t data[] __counted_by(len);
+} __packed;
+
+/* USBIO GPIO commands */
+enum usbio_gpio_cmd {
+	USBIO_GPIOCMD_DEINIT,
+	USBIO_GPIOCMD_INIT,
+	USBIO_GPIOCMD_READ,
+	USBIO_GPIOCMD_WRITE,
+	USBIO_GPIOCMD_END
+};
+
+/* USBIO GPIO config */
+enum usbio_gpio_pincfg {
+	USBIO_GPIO_PINCFG_DEFAULT,
+	USBIO_GPIO_PINCFG_PULLUP,
+	USBIO_GPIO_PINCFG_PULLDOWN,
+	USBIO_GPIO_PINCFG_PUSHPULL
+};
+
+#define USBIO_GPIO_PINCFG_SHIFT 2
+#define USBIO_GPIO_PINCFG_MASK (0x3 << USBIO_GPIO_PINCFG_SHIFT)
+#define USBIO_GPIO_SET_PINCFG(pincfg) \
+	(((pincfg) << USBIO_GPIO_PINCFG_SHIFT) & USBIO_GPIO_PINCFG_MASK)
+
+enum usbio_gpio_pinmode {
+	USBIO_GPIO_PINMOD_INVAL,
+	USBIO_GPIO_PINMOD_INPUT,
+	USBIO_GPIO_PINMOD_OUTPUT,
+	USBIO_GPIO_PINMOD_MAXVAL
+};
+
+#define USBIO_GPIO_PINMOD_MASK 0x3
+#define USBIO_GPIO_SET_PINMOD(pin) (pin & USBIO_GPIO_PINMOD_MASK)
+
+/*************************
+ * USBIO GPIO Controller *
+ *************************/
+
+#define USBIO_MAX_GPIOBANKS	5
+#define USBIO_GPIOSPERBANK	32
+
+struct usbio_gpio_bank_desc {
+	uint8_t id;
+	uint8_t pins;
+	uint32_t bmap;
+} __packed;
+
+struct usbio_gpio_init {
+	u8 bankid;
+	u8 config;
+	u8 pincount;
+	u8 pin;
+} __packed;
+
+struct usbio_gpio_rw {
+	u8 bankid;
+	u8 pincount;
+	u8 pin;
+	u32 value;
+} __packed;
+
+/* USBIO I2C commands */
+enum usbio_i2c_cmd {
+	USBIO_I2CCMD_UNINIT,
+	USBIO_I2CCMD_INIT,
+	USBIO_I2CCMD_READ,
+	USBIO_I2CCMD_WRITE,
+	USBIO_I2CCMD_END
+};
+
+/************************
+ * USBIO I2C Controller *
+ ************************/
+
+#define USBIO_MAX_I2CBUSES 5
+
+#define USBIO_I2C_BUS_ADDR_CAP_10B	BIT(3) /* 10bit address support */
+#define USBIO_I2C_BUS_MODE_CAP_MASK	0x3
+#define USBIO_I2C_BUS_MODE_CAP_SM	0 /* Standard Mode */
+#define USBIO_I2C_BUS_MODE_CAP_FM	1 /* Fast Mode */
+#define USBIO_I2C_BUS_MODE_CAP_FMP	2 /* Fast Mode+ */
+#define USBIO_I2C_BUS_MODE_CAP_HSM	3 /* High-Speed Mode */
+
+struct usbio_i2c_bus_desc {
+	uint8_t id;
+	uint8_t caps;
+} __packed;
+
+struct usbio_i2c_uninit {
+	u8 busid;
+	u16 config;
+} __packed;
+
+struct usbio_i2c_init {
+	u8 busid;
+	u16 config;
+	u32 speed;
+} __packed;
+
+struct usbio_i2c_rw {
+	u8 busid;
+	u16 config;
+	u16 size;
+	u8 data[] __counted_by(size);
+} __packed;
+
+int usbio_control_msg(struct auxiliary_device *adev, u8 type, u8 cmd,
+		      const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len);
+
+int usbio_bulk_msg(struct auxiliary_device *adev, u8 type, u8 cmd, bool last,
+		   const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len);
+
+int usbio_acquire(struct auxiliary_device *adev);
+void usbio_release(struct auxiliary_device *adev);
+void usbio_get_txrxbuf_len(struct auxiliary_device *adev, u16 *txbuf_len, u16 *rxbuf_len);
+void usbio_acpi_bind(struct auxiliary_device *adev, const struct acpi_device_id *hids);
+
+#endif
-- 
2.49.0

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Greg Kroah-Hartman]

On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
> +struct usbio_protver {
> +	uint8_t ver;

Nit, but you do this everywhere.  Kernel types are "u8", not "uint8_t",
that's a userspace C type.  Please use the correct ones when writing
kernel code.


> +} __packed;
> +
> +struct usbio_fwver {
> +	uint8_t major;
> +	uint8_t minor;
> +	uint16_t patch;
> +	uint16_t build;

What is the endian of these u16 variables?

> +/* USBIO Packet Header */
> +struct usbio_packet_header {
> +	uint8_t type;
> +	uint8_t cmd;
> +	uint8_t flags;

Are these crossing the user/kernel boundry?  I think so (same with
above), and so shouldn't they use the proper types (__u8)?

> +} __packed;
> +
> +/* USBIO Control Transfer Packet */
> +struct usbio_ctrl_packet {
> +	struct usbio_packet_header header;
> +	uint8_t len;
> +	uint8_t data[] __counted_by(len);

Same here.

> +} __packed;
> +
> +/* USBIO Bulk Transfer Packet */
> +struct usbio_bulk_packet {
> +	struct usbio_packet_header header;
> +	uint16_t len;

Endian-ness of len?

> +	uint8_t data[] __counted_by(len);
> +} __packed;
> +
> +/* USBIO GPIO commands */
> +enum usbio_gpio_cmd {
> +	USBIO_GPIOCMD_DEINIT,
> +	USBIO_GPIOCMD_INIT,
> +	USBIO_GPIOCMD_READ,
> +	USBIO_GPIOCMD_WRITE,
> +	USBIO_GPIOCMD_END

No specific value of these enums?

> +};
> +
> +/* USBIO GPIO config */
> +enum usbio_gpio_pincfg {
> +	USBIO_GPIO_PINCFG_DEFAULT,
> +	USBIO_GPIO_PINCFG_PULLUP,
> +	USBIO_GPIO_PINCFG_PULLDOWN,
> +	USBIO_GPIO_PINCFG_PUSHPULL

Same here, no specific values?

> +};
> +
> +#define USBIO_GPIO_PINCFG_SHIFT 2
> +#define USBIO_GPIO_PINCFG_MASK (0x3 << USBIO_GPIO_PINCFG_SHIFT)
> +#define USBIO_GPIO_SET_PINCFG(pincfg) \
> +	(((pincfg) << USBIO_GPIO_PINCFG_SHIFT) & USBIO_GPIO_PINCFG_MASK)
> +
> +enum usbio_gpio_pinmode {
> +	USBIO_GPIO_PINMOD_INVAL,
> +	USBIO_GPIO_PINMOD_INPUT,
> +	USBIO_GPIO_PINMOD_OUTPUT,
> +	USBIO_GPIO_PINMOD_MAXVAL

And here?

> +};
> +
> +#define USBIO_GPIO_PINMOD_MASK 0x3
> +#define USBIO_GPIO_SET_PINMOD(pin) (pin & USBIO_GPIO_PINMOD_MASK)
> +
> +/*************************
> + * USBIO GPIO Controller *
> + *************************/
> +
> +#define USBIO_MAX_GPIOBANKS	5
> +#define USBIO_GPIOSPERBANK	32
> +
> +struct usbio_gpio_bank_desc {
> +	uint8_t id;
> +	uint8_t pins;
> +	uint32_t bmap;

endian?

> +} __packed;
> +
> +struct usbio_gpio_init {
> +	u8 bankid;
> +	u8 config;
> +	u8 pincount;
> +	u8 pin;

Now you use "u8"  :)

> +} __packed;
> +
> +struct usbio_gpio_rw {
> +	u8 bankid;
> +	u8 pincount;
> +	u8 pin;
> +	u32 value;

endian?

> +} __packed;
> +
> +/* USBIO I2C commands */
> +enum usbio_i2c_cmd {
> +	USBIO_I2CCMD_UNINIT,
> +	USBIO_I2CCMD_INIT,
> +	USBIO_I2CCMD_READ,
> +	USBIO_I2CCMD_WRITE,
> +	USBIO_I2CCMD_END

No specific values?

> +};
> +
> +/************************
> + * USBIO I2C Controller *
> + ************************/
> +
> +#define USBIO_MAX_I2CBUSES 5
> +
> +#define USBIO_I2C_BUS_ADDR_CAP_10B	BIT(3) /* 10bit address support */
> +#define USBIO_I2C_BUS_MODE_CAP_MASK	0x3
> +#define USBIO_I2C_BUS_MODE_CAP_SM	0 /* Standard Mode */
> +#define USBIO_I2C_BUS_MODE_CAP_FM	1 /* Fast Mode */
> +#define USBIO_I2C_BUS_MODE_CAP_FMP	2 /* Fast Mode+ */
> +#define USBIO_I2C_BUS_MODE_CAP_HSM	3 /* High-Speed Mode */
> +
> +struct usbio_i2c_bus_desc {
> +	uint8_t id;
> +	uint8_t caps;
> +} __packed;
> +
> +struct usbio_i2c_uninit {
> +	u8 busid;
> +	u16 config;
> +} __packed;

You are using both types, again, please make everything "u8" and
friends.

And again, endian?  Same for the rest of this .h file.

thanks,

greg k-h

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Hans de Goede]

Hi Greg,

On 9-Aug-25 4:28 PM, Greg Kroah-Hartman wrote:
> On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
>> +struct usbio_protver {
>> +	uint8_t ver;
> 
> Nit, but you do this everywhere.  Kernel types are "u8", not "uint8_t",
> that's a userspace C type.  Please use the correct ones when writing
> kernel code.

Ack and also ack for the mission endianness on
the bigger word sizes.

I'll fix this all for the next version.

>> +} __packed;
>> +
>> +struct usbio_fwver {
>> +	uint8_t major;
>> +	uint8_t minor;
>> +	uint16_t patch;
>> +	uint16_t build;
> 
> What is the endian of these u16 variables?
> 
>> +/* USBIO Packet Header */
>> +struct usbio_packet_header {
>> +	uint8_t type;
>> +	uint8_t cmd;
>> +	uint8_t flags;
> 
> Are these crossing the user/kernel boundry?  I think so (same with
> above), and so shouldn't they use the proper types (__u8)?

No these only cross the kernel <-> hw boundary. This is not
uapi. I'll switch all these (everywhere in this driver) to plain
u8 / __le16 / __le32 for the next version and use 
le16_to_cpu, etc. to access the bigger word sizes.

Regards,

Hans

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus brgl/gpio/for-next andi-shyti/i2c/i2c-host linus/master v6.16 next-20250808]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/usb-misc-Add-Intel-USBIO-bridge-driver/20250809-182506
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20250809102326.6032-2-hansg%40kernel.org
patch subject: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver
config: i386-allmodconfig (https://download.01.org/0day-ci/archive/20250809/202508092329.U0OxBRAh-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14+deb12u1) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250809/202508092329.U0OxBRAh-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508092329.U0OxBRAh-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: drivers/usb/misc/usbio.c:132 struct member 'nr_gpio_banks' not described in 'usbio_device'
>> Warning: drivers/usb/misc/usbio.c:132 struct member 'gpios' not described in 'usbio_device'
>> Warning: drivers/usb/misc/usbio.c:132 struct member 'nr_i2c_buses' not described in 'usbio_device'
>> Warning: drivers/usb/misc/usbio.c:132 struct member 'i2cs' not described in 'usbio_device'
>> Warning: drivers/usb/misc/usbio.c:132 struct member 'nr_spi_buses' not described in 'usbio_device'
>> Warning: drivers/usb/misc/usbio.c:132 struct member 'spis' not described in 'usbio_device'

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

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[kernel test robot]

Hi Hans,

kernel test robot noticed the following build warnings:

[auto build test WARNING on usb/usb-testing]
[also build test WARNING on usb/usb-next usb/usb-linus brgl/gpio/for-next andi-shyti/i2c/i2c-host linus/master v6.16 next-20250808]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Hans-de-Goede/usb-misc-Add-Intel-USBIO-bridge-driver/20250809-182506
base:   https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git usb-testing
patch link:    https://lore.kernel.org/r/20250809102326.6032-2-hansg%40kernel.org
patch subject: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver
config: loongarch-randconfig-r052-20250810 (https://download.01.org/0day-ci/archive/20250810/202508100706.TBcpUB9u-lkp@intel.com/config)
compiler: clang version 18.1.8 (https://github.com/llvm/llvm-project 3b5b5c1ec4a3095ab096dd780e84d7ab81f3d7ff)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202508100706.TBcpUB9u-lkp@intel.com/

cocci warnings: (new ones prefixed by >>)
>> drivers/usb/misc/usbio.c:661:19-21: WARNING !A || A && B is equivalent to !A || B

vim +661 drivers/usb/misc/usbio.c

   647	
   648	static int usbio_match_device_ids(struct acpi_device *adev, void *data)
   649	{
   650		struct usbio_match_ids_walk_data *wd = data;
   651		unsigned int id = 0;
   652		char *uid;
   653	
   654		if (!acpi_match_device_ids(adev, wd->hids)) {
   655			uid = acpi_device_uid(adev);
   656			if (uid)
   657				for (int i = 0; i < strlen(uid); i++)
   658					if (!kstrtouint(&uid[i], 10, &id))
   659						break;
   660	
 > 661			if (!uid || (uid && wd->id == (u8)id)) {
   662				wd->adev = adev;
   663				return 1;
   664			}
   665		}
   666	
   667		return 0;
   668	}
   669	

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

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Sakari Ailus]

Hi Hans,

Thanks for posting this. Some comments below...

On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> 
> Add a driver for the Intel USBIO USB IO-expander used by the MIPI cameras
> on various new (Meteor Lake and later) Intel laptops.
> 
> This is an USB bridge driver which adds auxbus child devices for the GPIO,
> I2C and SPI functions of the USBIO chip and which exports IO-functions for
> the drivers for the auxbus child devices to communicate with the USBIO
> device's firmware.
> 
> Co-developed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> ---
>  MAINTAINERS               |   7 +
>  drivers/usb/misc/Kconfig  |  14 +
>  drivers/usb/misc/Makefile |   1 +
>  drivers/usb/misc/usbio.c  | 693 ++++++++++++++++++++++++++++++++++++++
>  include/linux/usb/usbio.h | 168 +++++++++
>  5 files changed, 883 insertions(+)
>  create mode 100644 drivers/usb/misc/usbio.c
>  create mode 100644 include/linux/usb/usbio.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0618351510ad..1208efe41f9f 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12518,6 +12518,13 @@ S:	Maintained
>  F:	Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
>  F:	drivers/platform/x86/intel/uncore-frequency/
>  
> +INTEL USBIO USB I/O EXPANDER DRIVERS
> +M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> +M:	Hans de Goede <hansg@kernel.org>

Could you add:

R:	Sakari Ailus <sakari.ailus@linux.intel.com>

?

> +S:	Maintained
> +F:	drivers/usb/misc/usbio.c
> +F:	include/linux/usb/usbio.h
> +
>  INTEL VENDOR SPECIFIC EXTENDED CAPABILITIES DRIVER
>  M:	David E. Box <david.e.box@linux.intel.com>
>  S:	Supported
> diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
> index 6497c4e81e95..bfe08e15d051 100644
> --- a/drivers/usb/misc/Kconfig
> +++ b/drivers/usb/misc/Kconfig
> @@ -178,6 +178,20 @@ config USB_LJCA
>  	  This driver can also be built as a module. If so, the module
>  	  will be called usb-ljca.
>  
> +config USB_USBIO
> +	tristate "Intel USBIO Bridge support"
> +	depends on USB && ACPI
> +	select AUXILIARY_BUS
> +	help
> +	  This adds support for Intel USBIO drivers.
> +	  This enables the USBIO bridge driver module in charge to talk
> +	  to the USB device. Additional drivers such as GPIO_USBIO and
> +	  I2C_USBIO must be enabled in order to use the device's full
> +	  functionality.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called usbio.
> +
>  source "drivers/usb/misc/sisusbvga/Kconfig"
>  
>  config USB_LD
> diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
> index 0cd5bc8f52fe..494ab0377f35 100644
> --- a/drivers/usb/misc/Makefile
> +++ b/drivers/usb/misc/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_USB_EMI62)			+= emi62.o
>  obj-$(CONFIG_USB_EZUSB_FX2)		+= ezusb.o
>  obj-$(CONFIG_APPLE_MFI_FASTCHARGE)	+= apple-mfi-fastcharge.o
>  obj-$(CONFIG_USB_LJCA)			+= usb-ljca.o
> +obj-$(CONFIG_USB_USBIO)			+= usbio.o
>  obj-$(CONFIG_USB_IDMOUSE)		+= idmouse.o
>  obj-$(CONFIG_USB_IOWARRIOR)		+= iowarrior.o
>  obj-$(CONFIG_USB_ISIGHTFW)		+= isight_firmware.o
> diff --git a/drivers/usb/misc/usbio.c b/drivers/usb/misc/usbio.c
> new file mode 100644
> index 000000000000..88197092f39a
> --- /dev/null
> +++ b/drivers/usb/misc/usbio.c
> @@ -0,0 +1,693 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Intel USBIO Bridge driver
> + *
> + * Copyright (c) 2025 Intel Corporation.
> + * Copyright (c) 2025 Red Hat, Inc.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/completion.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/lockdep.h>
> +#include <linux/mutex.h>
> +#include <linux/string.h>
> +#include <linux/types.h>
> +#include <linux/usb.h>
> +#include <linux/usb/usbio.h>
> +
> +/*************************************
> + * USBIO Bridge Protocol Definitions *
> + *************************************/
> +
> +/* USBIO Control Commands */
> +#define USBIO_CTRLCMD_PROTVER	0
> +#define USBIO_CTRLCMD_FWVER	1
> +#define USBIO_CTRLCMD_HS	2
> +#define USBIO_CTRLCMD_ENUMGPIO	16
> +#define USBIO_CTRLCMD_ENUMI2C	17
> +#define USBIO_CTRLCMD_ENUMSPI	18
> +
> +/* USBIO Packet Flags */
> +#define USBIO_PKTFLAG_ACK	BIT(0)
> +#define USBIO_PKTFLAG_RSP	BIT(1)
> +#define USBIO_PKTFLAG_CMP	BIT(2)
> +#define USBIO_PKTFLAG_ERR	BIT(3)
> +
> +#define USBIO_PKTFLAGS_REQRESP	(USBIO_PKTFLAG_CMP | USBIO_PKTFLAG_ACK)
> +
> +#define USBIO_CTRLXFER_TIMEOUT 0
> +#define USBIO_BULKXFER_TIMEOUT 100
> +
> +struct usbio_protver {
> +	uint8_t ver;
> +} __packed;
> +
> +struct usbio_fwver {
> +	uint8_t major;
> +	uint8_t minor;
> +	uint16_t patch;
> +	uint16_t build;
> +} __packed;
> +
> +#define USBIO_SPI_BUS_WIRE_CAP_3W	BIT(3) /* 3 wires support */
> +#define USBIO_SPI_BUS_ADDR_CAP_MASK	0x3
> +#define USBIO_SPI_BUS_ADDR_CAP_CS0	0 /* CS0 */
> +#define USBIO_SPI_BUS_ADDR_CAP_CS1	1 /* CS0/1 */
> +#define USBIO_SPI_BUS_ADDR_CAP_CS2	2 /* CS0/1/2 */
> +#define USBIO_SPI_BUS_ADDR_CAP_CS3	3 /* CS0/1/2/3 */
> +
> +struct usbio_spi_bus_desc {
> +	uint8_t id;
> +	uint8_t caps;
> +} __packed;
> +
> +/***********************************
> + * USBIO Bridge Device Definitions *
> + ***********************************/
> +struct usbio_dev_info {
> +	struct usbio_protver protver;
> +	struct usbio_fwver fwver;
> +};
> +
> +#define MAX_SPIBUSES	5
> +
> +/**
> + * struct usbio_device - the usb device exposing IOs
> + *
> + * @dev: the device in the usb interface
> + * @udev: the detected usb device
> + * @intf: the usb interface
> + * @ctrl_pipe: the control transfer pipe
> + * @ctrlbuf_len: the size of the control transfer pipe
> + * @ctrlbuf: the buffer used for control transfers
> + * @tx_pipe: the bulk out pipe
> + * @txbuf_len: the size of the bulk out pipe
> + * @txbuf: the buffer used for bulk out transfers
> + * @urb: the urb to read bulk pipe
> + * @rx_pipe: the bulk in pipe
> + * @rxbuf_len: the size of the bulk in pipe
> + * @rxdat_len: the data length at rx buffer
> + * @rxbuf: the buffer used for bulk in transfers
> + * @info: the device's protocol and firmware information
> + * @done: completion object as request is done
> + * @mutex: protection against access concurrency
> + * @cli_list: device's client list
> + */
> +struct usbio_device {
> +	struct device *dev;
> +	struct usb_device *udev;
> +	struct usb_interface *intf;
> +
> +	unsigned int ctrl_pipe;
> +	u16 ctrlbuf_len;
> +	void *ctrlbuf;
> +
> +	unsigned int tx_pipe;
> +	u16 txbuf_len;
> +	void *txbuf;
> +
> +	struct urb *urb;
> +	unsigned int rx_pipe;
> +	u16 rxbuf_len;
> +	u16 rxdat_len;
> +	void *rxbuf;
> +
> +	struct usbio_dev_info info;
> +
> +	struct completion done;
> +	struct mutex mutex;
> +
> +	struct list_head cli_list;
> +
> +	unsigned int nr_gpio_banks;
> +	struct usbio_gpio_bank_desc gpios[USBIO_MAX_GPIOBANKS];
> +
> +	unsigned int nr_i2c_buses;
> +	struct usbio_i2c_bus_desc i2cs[USBIO_MAX_I2CBUSES];
> +
> +	unsigned int nr_spi_buses;
> +	struct usbio_spi_bus_desc spis[MAX_SPIBUSES];
> +};
> +
> +/**
> + * struct usbio_client - represents a usbio client
> + *
> + * @adev: auxiliary device object
> + * @bridge: usbio bridge who service the client
> + * @link: usbio bridge clients list member
> + */
> +struct usbio_client {
> +	struct auxiliary_device adev;
> +	struct usbio_device *bridge;
> +	struct list_head link;
> +};
> +
> +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)

Please use a different name than "adev" for the argument, which is also the
struct field of interest.

> +
> +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
> +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> +{
> +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> +	struct usbio_ctrl_packet *cpkt;
> +	unsigned int pipe;
> +	u16 cpkt_len;
> +	int ret;
> +
> +	lockdep_assert_held(&usbio->mutex);
> +
> +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
> +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))

You can (and should) remove all parentheses except the outer ones here.

> +		return -EMSGSIZE;
> +
> +	/* Prepare Control Packet Header */
> +	cpkt = usbio->ctrlbuf;
> +	cpkt->header.type = type;
> +	cpkt->header.cmd = cmd;
> +	if (type == USBIO_PKTTYPE_CTRL || ibuf_len)
> +		cpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
> +	else
> +		cpkt->header.flags = USBIO_PKTFLAG_CMP;
> +	cpkt->len = obuf_len;
> +
> +	/* Copy the data */
> +	memcpy(cpkt->data, obuf, obuf_len);
> +
> +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
> +	cpkt_len = sizeof(*cpkt) + obuf_len;
> +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
> +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
> +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);

Instead of casting, how about using %zu for printing a size_t?

> +
> +	if (ret != cpkt_len) {
> +		dev_err(usbio->dev, "USB control out failed: %d\n", ret);
> +		return (ret < 0) ? ret : -EPROTO;

Redundant parentheses.

> +	}
> +
> +	if (!(cpkt->header.flags & USBIO_PKTFLAG_ACK))
> +		return 0;
> +
> +	pipe = usb_rcvctrlpipe(usbio->udev, usbio->ctrl_pipe);
> +	cpkt_len = sizeof(*cpkt) + ibuf_len;
> +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
> +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> +	dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
> +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> +
> +	if (ret < sizeof(*cpkt)) {
> +		dev_err(usbio->dev, "USB control in failed: %d\n", ret);
> +		return (ret < 0) ? ret : -EPROTO;

Remove parentheses? There's no need to try to comply with MISRA C, is
there? :-) I'm not commenting anymore on further instances of this.

> +	}
> +
> +	if (cpkt->header.type != type || cpkt->header.cmd != cmd ||
> +	    !(cpkt->header.flags & USBIO_PKTFLAG_RSP)) {
> +		dev_err(usbio->dev, "Unexpected reply type: %u, cmd: %u, flags: %u\n",
> +			cpkt->header.type, cpkt->header.cmd, cpkt->header.flags);
> +		return -EPROTO;
> +	}
> +
> +	if (cpkt->header.flags & USBIO_PKTFLAG_ERR)
> +		return -EREMOTEIO;
> +
> +	if (ibuf_len < cpkt->len)
> +		return -ENOSPC;
> +
> +	memcpy(ibuf, cpkt->data, cpkt->len);

It'd be nice to have one more newline here.

> +	return cpkt->len;
> +}
> +
> +int usbio_control_msg(struct auxiliary_device *adev, u8 type, u8 cmd,
> +		      const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> +{
> +	struct usbio_client *client = adev_to_client(adev);
> +	int ret;
> +
> +	ret = usbio_acquire(adev);
> +	if (ret)
> +		return ret;
> +
> +	ret = usbio_ctrl_msg(client->bridge, type, cmd, obuf, obuf_len, ibuf, ibuf_len);
> +
> +	usbio_release(adev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_NS_GPL(usbio_control_msg, "USBIO");
> +
> +static void usbio_bulk_recv(struct urb *urb)
> +{
> +	struct usbio_bulk_packet *bpkt = urb->transfer_buffer;
> +	struct usbio_device *usbio = urb->context;
> +
> +	if (!urb->status) {
> +		if (bpkt->header.flags & USBIO_PKTFLAG_RSP) {
> +			usbio->rxdat_len = urb->actual_length;
> +			complete(&usbio->done);
> +		}
> +	} else if (urb->status != -ENOENT) {
> +		dev_err(usbio->dev, "Bulk in error %d\n", urb->status);
> +	}
> +
> +	usb_submit_urb(usbio->urb, GFP_ATOMIC);
> +}
> +
> +int usbio_bulk_msg(struct auxiliary_device *adev, u8 type, u8 cmd, bool last,
> +		   const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> +{
> +	struct usbio_device *usbio = adev_to_client(adev)->bridge;
> +	struct usbio_bulk_packet *bpkt;
> +	int ret, act;
> +	u16 bpkt_len;
> +
> +	lockdep_assert_held(&usbio->mutex);
> +
> +	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
> +	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
> +		return -EMSGSIZE;
> +
> +	if (ibuf_len)
> +		reinit_completion(&usbio->done);
> +
> +	/* If no data to send, skip to read */
> +	if (!obuf_len)
> +		goto read;
> +
> +	/* Prepare Bulk Packet Header */
> +	bpkt = usbio->txbuf;
> +	bpkt->header.type = type;
> +	bpkt->header.cmd = cmd;
> +	if (!last)
> +		bpkt->header.flags = 0;
> +	else if (ibuf_len)
> +		bpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
> +	else
> +		bpkt->header.flags = USBIO_PKTFLAG_CMP;
> +	bpkt->len = obuf_len;
> +
> +	/* Copy the data */
> +	memcpy(bpkt->data, obuf, obuf_len);
> +
> +	bpkt_len = sizeof(*bpkt) + obuf_len;
> +	ret = usb_bulk_msg(usbio->udev, usbio->tx_pipe, bpkt, bpkt_len, &act,
> +			   USBIO_BULKXFER_TIMEOUT);
> +	dev_dbg(usbio->dev, "bulk out %d hdr %*phN data %*phN\n", act,
> +		(int)sizeof(*bpkt), bpkt, (int)bpkt->len, bpkt->data);
> +
> +	if (ret || act != bpkt_len) {
> +		dev_err(usbio->dev, "Bulk out failed: %d\n", ret);
> +		return ret ?: -EPROTO;
> +	}
> +
> +	if (!(bpkt->header.flags & USBIO_PKTFLAG_ACK))
> +		return obuf_len;
> +
> +read:
> +	ret = wait_for_completion_timeout(&usbio->done, USBIO_BULKXFER_TIMEOUT);
> +	if (ret <= 0) {
> +		dev_err(usbio->dev, "Bulk in wait failed: %d\n", ret);
> +		return ret ?: -ETIMEDOUT;
> +	}
> +
> +	act = usbio->rxdat_len;
> +	bpkt = usbio->rxbuf;
> +	dev_dbg(usbio->dev, "bulk in %d hdr %*phN data %*phN\n", act,
> +		(int)sizeof(*bpkt), bpkt, (int)bpkt->len, bpkt->data);

%zu, please.

> +
> +	/*
> +	 * Unsupported bulk commands get only an usbio_packet_header with
> +	 * the error flag set as reply. Return -EPIPE for this case.
> +	 */
> +	if (act == sizeof(struct usbio_packet_header) &&
> +	    (bpkt->header.flags & USBIO_PKTFLAG_ERR))
> +		return -EPIPE;
> +
> +	if (act < sizeof(*bpkt)) {
> +		dev_err(usbio->dev, "Bulk in short read: %d\n", act);
> +		return -EPROTO;
> +	}
> +
> +	if (bpkt->header.type != type || bpkt->header.cmd != cmd ||
> +	    !(bpkt->header.flags & USBIO_PKTFLAG_RSP)) {
> +		dev_err(usbio->dev,
> +			"Unexpected bulk in type 0x%02x cmd 0x%02x flags 0x%02x\n",
> +			bpkt->header.type, bpkt->header.cmd, bpkt->header.flags);
> +		return -EPROTO;
> +	}
> +
> +	if (bpkt->header.flags & USBIO_PKTFLAG_ERR)
> +		return -EREMOTEIO;
> +
> +	if (ibuf_len < bpkt->len)
> +		return -ENOSPC;
> +
> +	memcpy(ibuf, bpkt->data, bpkt->len);

One more newline, perhaps?

> +	return bpkt->len;
> +}
> +EXPORT_SYMBOL_NS_GPL(usbio_bulk_msg, "USBIO");
> +
> +static int usbio_get_and_lock(struct usbio_device *usbio)
> +{
> +	int ret;
> +
> +	ret = usb_autopm_get_interface(usbio->intf);
> +	if (ret)
> +		return ret;
> +
> +	mutex_lock(&usbio->mutex);
> +	return 0;
> +}
> +
> +static void usbio_unlock_and_put(struct usbio_device *usbio)
> +{
> +	mutex_unlock(&usbio->mutex);
> +	usb_autopm_put_interface(usbio->intf);
> +}
> +
> +int usbio_acquire(struct auxiliary_device *adev)
> +{
> +	return usbio_get_and_lock(adev_to_client(adev)->bridge);
> +}
> +EXPORT_SYMBOL_NS_GPL(usbio_acquire, "USBIO");
> +
> +void usbio_release(struct auxiliary_device *adev)
> +{
> +	usbio_unlock_and_put(adev_to_client(adev)->bridge);
> +}
> +EXPORT_SYMBOL_NS_GPL(usbio_release, "USBIO");
> +
> +void usbio_get_txrxbuf_len(struct auxiliary_device *adev, u16 *txbuf_len, u16 *rxbuf_len)
> +{
> +	struct usbio_device *usbio = adev_to_client(adev)->bridge;
> +
> +	*txbuf_len = usbio->txbuf_len;
> +	*rxbuf_len = usbio->rxbuf_len;
> +}
> +EXPORT_SYMBOL_NS_GPL(usbio_get_txrxbuf_len, "USBIO");
> +
> +static void usbio_auxdev_release(struct device *dev)
> +{
> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
> +	struct usbio_client *client = adev_to_client(adev);
> +
> +	kfree(client);
> +}
> +
> +static int usbio_add_client(struct usbio_device *usbio, char *name, u8 id, void *data)
> +{
> +	struct usbio_client *client;
> +	struct auxiliary_device *adev;
> +	int ret;
> +
> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->bridge = usbio;
> +	adev = &client->adev;
> +	adev->name = name;
> +	adev->id = id;
> +
> +	adev->dev.parent = usbio->dev;
> +	adev->dev.platform_data = data;
> +	adev->dev.release = usbio_auxdev_release;
> +
> +	ret = auxiliary_device_init(adev);
> +	if (!ret) {
> +		if (auxiliary_device_add(adev))
> +			auxiliary_device_uninit(adev);

This isn't enough for error handling if auxiliary_device_add() fails.

> +	}
> +
> +	if (ret) {
> +		dev_err(usbio->dev, "Client %s.%d add failed: %d\n", name, id, ret);
> +		kfree(client);
> +		return ret;
> +	}
> +
> +	list_add_tail(&client->link, &usbio->cli_list);

A newline, please?

> +	return 0;
> +}
> +
> +static int usbio_ctrl_enumgpios(struct usbio_device *usbio)
> +{
> +	struct usbio_gpio_bank_desc *gpio = usbio->gpios;
> +	int ret, i;

unsigned int i, please.

> +
> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMGPIO, NULL, 0,
> +			     gpio, sizeof(usbio->gpios));
> +	if (ret < 0 || ret % sizeof(*gpio)) {
> +		dev_err(usbio->dev, "CTRL ENUMGPIO failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usbio->nr_gpio_banks = ret / sizeof(*gpio);
> +	dev_dbg(usbio->dev, "GPIO Banks: %d\n", usbio->nr_gpio_banks);
> +	for (i = 0; i < usbio->nr_gpio_banks; i++)
> +		dev_dbg(usbio->dev, "\tBank%d[%d] map: %#08x\n",
> +			gpio[i].id, gpio[i].pins, gpio[i].bmap);
> +
> +	usbio_add_client(usbio, USBIO_GPIO_CLIENT, 0, gpio);

One more newline?

> +	return 0;
> +}
> +
> +static int usbio_ctrl_enumi2cs(struct usbio_device *usbio)
> +{
> +	struct usbio_i2c_bus_desc *i2c = usbio->i2cs;
> +	int ret, i;

unsigned int i, please.

> +
> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMI2C, NULL, 0,
> +			     i2c, sizeof(usbio->i2cs));
> +	if (ret < 0 || ret % sizeof(*i2c)) {
> +		dev_err(usbio->dev, "CTRL ENUMI2C failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usbio->nr_i2c_buses = ret / sizeof(*i2c);
> +	dev_dbg(usbio->dev, "I2C Busses: %d\n", usbio->nr_i2c_buses);
> +	for (i = 0; i < usbio->nr_i2c_buses; i++) {
> +		dev_dbg(usbio->dev, "\tBus%d caps: %#02x\n", i2c[i].id, i2c[i].caps);
> +		usbio_add_client(usbio, USBIO_I2C_CLIENT, i, &i2c[i]);
> +	}
> +
> +	return 0;
> +}
> +
> +static int usbio_ctrl_enumspis(struct usbio_device *usbio)
> +{
> +	struct usbio_spi_bus_desc *spi = usbio->spis;
> +	int ret, i;

Ditto.

> +
> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMSPI, NULL, 0,
> +			     spi, sizeof(usbio->spis));
> +	if (ret < 0 || ret % sizeof(*spi)) {
> +		dev_err(usbio->dev, "CTRL ENUMSPI failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usbio->nr_spi_buses = ret / sizeof(*spi);
> +	dev_dbg(usbio->dev, "SPI Busses: %d\n", usbio->nr_spi_buses);
> +	for (i = 0; i < usbio->nr_spi_buses; i++)
> +		dev_dbg(usbio->dev, "\tBus%d caps: %#02x\n", spi[i].id, spi[i].caps);
> +
> +	return 0;
> +}
> +
> +static int usbio_suspend(struct usb_interface *intf, pm_message_t msg)
> +{
> +	struct usbio_device *usbio = usb_get_intfdata(intf);
> +
> +	usb_kill_urb(usbio->urb);
> +
> +	return 0;
> +}
> +
> +static int usbio_resume(struct usb_interface *intf)
> +{
> +	struct usbio_device *usbio = usb_get_intfdata(intf);
> +
> +	return usb_submit_urb(usbio->urb, GFP_KERNEL);
> +}
> +
> +static void usbio_disconnect(struct usb_interface *intf)
> +{
> +	struct usbio_device *usbio = usb_get_intfdata(intf);
> +	struct usbio_client *client, *prev;
> +
> +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> +		auxiliary_device_delete(&client->adev);
> +		list_del_init(&client->link);
> +		auxiliary_device_uninit(&client->adev);
> +	}
> +
> +	usb_kill_urb(usbio->urb);
> +	usb_free_urb(usbio->urb);

What will happen on client drivers if they're working with the bridge while
disconnect happens?

One easy solution to this could be to use an rw_semaphore where client
acquire it for readingin conjunction (in a helper that also checks the
interface status) and disconnect callback for writing.

> +}
> +
> +static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
> +{
> +	struct usb_device *udev = interface_to_usbdev(intf);
> +	struct usb_endpoint_descriptor *ep_in, *ep_out;
> +	struct device *dev = &intf->dev;
> +	struct usbio_device *usbio;
> +	int ret;
> +
> +	usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);

usbio will be released at the exit from usbio_disconnect(). I think you'll
need to use a release callback in struct device to release usbio once all
clients are gone.

> +	if (!usbio)
> +		return -ENOMEM;
> +
> +	ret = devm_mutex_init(dev, &usbio->mutex);
> +	if (ret)
> +		return ret;
> +
> +	usbio->dev = dev;
> +	usbio->udev = udev;
> +	usbio->intf = intf;
> +	init_completion(&usbio->done);
> +	INIT_LIST_HEAD(&usbio->cli_list);
> +	usb_set_intfdata(intf, usbio);
> +
> +	usbio->ctrl_pipe = usb_endpoint_num(&udev->ep0.desc);
> +	usbio->ctrlbuf_len = usb_maxpacket(udev, usbio->ctrl_pipe);
> +	usbio->ctrlbuf = devm_kzalloc(dev, usbio->ctrlbuf_len, GFP_KERNEL);
> +	if (!usbio->ctrlbuf)
> +		return -ENOMEM;
> +
> +	/* Find the first bulk-in and bulk-out endpoints */
> +	ret = usb_find_common_endpoints(intf->cur_altsetting, &ep_in, &ep_out,
> +					NULL, NULL);
> +	if (ret) {
> +		dev_err(dev, "Cannot find bulk endpoints: %d\n", ret);
> +		return ret;
> +	}
> +
> +	usbio->tx_pipe = usb_sndbulkpipe(udev, usb_endpoint_num(ep_out));
> +	usbio->txbuf_len = usb_endpoint_maxp(ep_out);
> +	usbio->txbuf = devm_kzalloc(dev, usbio->txbuf_len, GFP_KERNEL);
> +	if (!usbio->txbuf)
> +		return -ENOMEM;
> +
> +	usbio->rx_pipe = usb_rcvbulkpipe(udev, usb_endpoint_num(ep_in));
> +	usbio->rxbuf_len = usb_endpoint_maxp(ep_in);
> +	usbio->rxbuf = devm_kzalloc(dev, usbio->rxbuf_len, GFP_KERNEL);
> +	if (!usbio->rxbuf)
> +		return -ENOMEM;
> +
> +	usbio->urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!usbio->urb)
> +		return -ENOMEM;
> +
> +	usb_fill_bulk_urb(usbio->urb, udev, usbio->rx_pipe, usbio->rxbuf,
> +			  usbio->rxbuf_len, usbio_bulk_recv, usbio);
> +	ret = usb_submit_urb(usbio->urb, GFP_KERNEL);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Submitting usb urb\n");
> +
> +	ret = usbio_get_and_lock(usbio);
> +	if (ret)
> +		return ret;
> +
> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_HS, NULL, 0, NULL, 0);
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_PROTVER, NULL, 0,
> +			     &usbio->info.protver, sizeof(struct usbio_protver));
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_FWVER, NULL, 0,
> +			     &usbio->info.fwver, sizeof(struct usbio_fwver));
> +	if (ret < 0)
> +		goto error;
> +
> +	dev_dbg(dev, "ProtVer(BCD): %02x FwVer: %d.%d.%d.%d\n",
> +		usbio->info.protver.ver, usbio->info.fwver.major,
> +		usbio->info.fwver.minor, usbio->info.fwver.patch,
> +		usbio->info.fwver.build);
> +
> +	usbio_ctrl_enumgpios(usbio);
> +	usbio_ctrl_enumi2cs(usbio);
> +	usbio_ctrl_enumspis(usbio);
> +	usbio_unlock_and_put(usbio);

A newline here?

> +	return 0;
> +
> +error:
> +	usbio_unlock_and_put(usbio);
> +	usbio_disconnect(intf);

Ditto.

> +	return ret;
> +}
> +
> +static const struct usb_device_id usbio_table[] = {
> +	{ USB_DEVICE(0x2AC1, 0x20C1) }, /* Lattice NX40 */
> +	{ USB_DEVICE(0x2AC1, 0x20C9) }, /* Lattice NX33 */
> +	{ USB_DEVICE(0x2AC1, 0x20CB) }, /* Lattice NX33U */
> +	{ USB_DEVICE(0x06CB, 0x0701) }, /* Synaptics Sabre */

Lower case hexadecimals would be nice.

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(usb, usbio_table);
> +
> +static struct usb_driver usbio_driver = {
> +	.name = "usbio-bridge",
> +	.probe = usbio_probe,
> +	.disconnect = usbio_disconnect,
> +	.suspend = usbio_suspend,
> +	.resume = usbio_resume,
> +	.id_table = usbio_table,
> +	.supports_autosuspend = 1

Add the leading comma perhaps?

> +};
> +module_usb_driver(usbio_driver);
> +
> +struct usbio_match_ids_walk_data {
> +	struct acpi_device *adev;
> +	const struct acpi_device_id *hids;
> +	const u8 id;
> +};
> +
> +static int usbio_match_device_ids(struct acpi_device *adev, void *data)
> +{
> +	struct usbio_match_ids_walk_data *wd = data;
> +	unsigned int id = 0;
> +	char *uid;
> +
> +	if (!acpi_match_device_ids(adev, wd->hids)) {

You could return here if acpi_match_device_ids() returns non-zero and
unindent the rest.

> +		uid = acpi_device_uid(adev);
> +		if (uid)
> +			for (int i = 0; i < strlen(uid); i++)

size_t i?

> +				if (!kstrtouint(&uid[i], 10, &id))
> +					break;
> +
> +		if (!uid || (uid && wd->id == (u8)id)) {
> +			wd->adev = adev;
> +			return 1;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +void usbio_acpi_bind(struct auxiliary_device *adev, const struct acpi_device_id *hids)
> +{
> +	struct device *dev = &adev->dev;
> +	struct acpi_device *parent;
> +	struct usbio_match_ids_walk_data wd = {
> +		.adev = NULL,
> +		.hids = hids,
> +		.id = adev->id,
> +	};
> +
> +	parent = ACPI_COMPANION(dev->parent);
> +	if (!parent)
> +		return;
> +
> +	acpi_dev_for_each_child(parent, usbio_match_device_ids, &wd);
> +	if (wd.adev)
> +		ACPI_COMPANION_SET(dev, wd.adev);
> +}
> +EXPORT_SYMBOL_NS_GPL(usbio_acpi_bind, "USBIO");
> +
> +MODULE_DESCRIPTION("Intel USBIO Bridge driver");
> +MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/usb/usbio.h b/include/linux/usb/usbio.h
> new file mode 100644
> index 000000000000..9c34b157664f
> --- /dev/null
> +++ b/include/linux/usb/usbio.h
> @@ -0,0 +1,168 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (c) 2025 Intel Corporation.
> + *
> + */
> +
> +#ifndef _LINUX_USBIO_H_
> +#define _LINUX_USBIO_H_
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/list.h>
> +#include <linux/types.h>
> +
> +/***********************
> + * USBIO Clients Names *
> + ***********************/
> +#define USBIO_GPIO_CLIENT	"usbio-gpio"
> +#define USBIO_I2C_CLIENT	"usbio-i2c"
> +#define USBIO_SPI_CLIENT	"usbio-spi"
> +
> +/**************************
> + * USBIO Type Definitions *
> + **************************/
> +
> +/* USBIO Packet Type */
> +#define USBIO_PKTTYPE_CTRL	1
> +#define USBIO_PKTTYPE_DBG	2
> +#define USBIO_PKTTYPE_GPIO	3
> +#define USBIO_PKTTYPE_I2C	4
> +#define USBIO_PKTTYPE_SPI	5
> +
> +/* USBIO Packet Header */
> +struct usbio_packet_header {
> +	uint8_t type;
> +	uint8_t cmd;
> +	uint8_t flags;
> +} __packed;
> +
> +/* USBIO Control Transfer Packet */
> +struct usbio_ctrl_packet {
> +	struct usbio_packet_header header;
> +	uint8_t len;
> +	uint8_t data[] __counted_by(len);
> +} __packed;
> +
> +/* USBIO Bulk Transfer Packet */
> +struct usbio_bulk_packet {
> +	struct usbio_packet_header header;
> +	uint16_t len;
> +	uint8_t data[] __counted_by(len);
> +} __packed;
> +
> +/* USBIO GPIO commands */
> +enum usbio_gpio_cmd {
> +	USBIO_GPIOCMD_DEINIT,
> +	USBIO_GPIOCMD_INIT,
> +	USBIO_GPIOCMD_READ,
> +	USBIO_GPIOCMD_WRITE,
> +	USBIO_GPIOCMD_END
> +};
> +
> +/* USBIO GPIO config */
> +enum usbio_gpio_pincfg {
> +	USBIO_GPIO_PINCFG_DEFAULT,
> +	USBIO_GPIO_PINCFG_PULLUP,
> +	USBIO_GPIO_PINCFG_PULLDOWN,
> +	USBIO_GPIO_PINCFG_PUSHPULL
> +};
> +
> +#define USBIO_GPIO_PINCFG_SHIFT 2
> +#define USBIO_GPIO_PINCFG_MASK (0x3 << USBIO_GPIO_PINCFG_SHIFT)
> +#define USBIO_GPIO_SET_PINCFG(pincfg) \
> +	(((pincfg) << USBIO_GPIO_PINCFG_SHIFT) & USBIO_GPIO_PINCFG_MASK)
> +
> +enum usbio_gpio_pinmode {
> +	USBIO_GPIO_PINMOD_INVAL,
> +	USBIO_GPIO_PINMOD_INPUT,
> +	USBIO_GPIO_PINMOD_OUTPUT,
> +	USBIO_GPIO_PINMOD_MAXVAL
> +};
> +
> +#define USBIO_GPIO_PINMOD_MASK 0x3
> +#define USBIO_GPIO_SET_PINMOD(pin) (pin & USBIO_GPIO_PINMOD_MASK)
> +
> +/*************************
> + * USBIO GPIO Controller *
> + *************************/
> +
> +#define USBIO_MAX_GPIOBANKS	5
> +#define USBIO_GPIOSPERBANK	32
> +
> +struct usbio_gpio_bank_desc {
> +	uint8_t id;
> +	uint8_t pins;
> +	uint32_t bmap;
> +} __packed;
> +
> +struct usbio_gpio_init {
> +	u8 bankid;
> +	u8 config;
> +	u8 pincount;
> +	u8 pin;
> +} __packed;
> +
> +struct usbio_gpio_rw {
> +	u8 bankid;
> +	u8 pincount;
> +	u8 pin;
> +	u32 value;
> +} __packed;
> +
> +/* USBIO I2C commands */
> +enum usbio_i2c_cmd {
> +	USBIO_I2CCMD_UNINIT,
> +	USBIO_I2CCMD_INIT,
> +	USBIO_I2CCMD_READ,
> +	USBIO_I2CCMD_WRITE,
> +	USBIO_I2CCMD_END
> +};
> +
> +/************************
> + * USBIO I2C Controller *
> + ************************/
> +
> +#define USBIO_MAX_I2CBUSES 5
> +
> +#define USBIO_I2C_BUS_ADDR_CAP_10B	BIT(3) /* 10bit address support */
> +#define USBIO_I2C_BUS_MODE_CAP_MASK	0x3
> +#define USBIO_I2C_BUS_MODE_CAP_SM	0 /* Standard Mode */
> +#define USBIO_I2C_BUS_MODE_CAP_FM	1 /* Fast Mode */
> +#define USBIO_I2C_BUS_MODE_CAP_FMP	2 /* Fast Mode+ */
> +#define USBIO_I2C_BUS_MODE_CAP_HSM	3 /* High-Speed Mode */
> +
> +struct usbio_i2c_bus_desc {
> +	uint8_t id;
> +	uint8_t caps;
> +} __packed;
> +
> +struct usbio_i2c_uninit {
> +	u8 busid;
> +	u16 config;
> +} __packed;
> +
> +struct usbio_i2c_init {
> +	u8 busid;
> +	u16 config;
> +	u32 speed;
> +} __packed;
> +
> +struct usbio_i2c_rw {
> +	u8 busid;
> +	u16 config;
> +	u16 size;
> +	u8 data[] __counted_by(size);
> +} __packed;
> +
> +int usbio_control_msg(struct auxiliary_device *adev, u8 type, u8 cmd,
> +		      const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len);
> +
> +int usbio_bulk_msg(struct auxiliary_device *adev, u8 type, u8 cmd, bool last,
> +		   const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len);
> +
> +int usbio_acquire(struct auxiliary_device *adev);
> +void usbio_release(struct auxiliary_device *adev);
> +void usbio_get_txrxbuf_len(struct auxiliary_device *adev, u16 *txbuf_len, u16 *rxbuf_len);
> +void usbio_acpi_bind(struct auxiliary_device *adev, const struct acpi_device_id *hids);
> +
> +#endif

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 06:51:03AM +0000, Sakari Ailus wrote:
> > +/**
> > + * struct usbio_client - represents a usbio client
> > + *
> > + * @adev: auxiliary device object
> > + * @bridge: usbio bridge who service the client
> > + * @link: usbio bridge clients list member
> > + */
> > +struct usbio_client {
> > +	struct auxiliary_device adev;
> > +	struct usbio_device *bridge;
> > +	struct list_head link;
> > +};
> > +
> > +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> 
> Please use a different name than "adev" for the argument, which is also the
> struct field of interest.

Why?  That's a very common way of doing this.  My only complaint is that
it really should be "container_of_const()" instead of just
"container_of()"


> > +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
> > +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> > +{
> > +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> > +	struct usbio_ctrl_packet *cpkt;
> > +	unsigned int pipe;
> > +	u16 cpkt_len;
> > +	int ret;
> > +
> > +	lockdep_assert_held(&usbio->mutex);
> > +
> > +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
> > +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> 
> You can (and should) remove all parentheses except the outer ones here.

No, don't do that.  If you do that you will have to manually go and try
to remember the order of operations every time you read this code.

Remember, we write code for people first, compilers second.  Make it
totally obvious what is going on here as you want PEOPLE to catch your
issues.

The statement is fine as-is.

> > +		return -EMSGSIZE;
> > +
> > +	/* Prepare Control Packet Header */
> > +	cpkt = usbio->ctrlbuf;
> > +	cpkt->header.type = type;
> > +	cpkt->header.cmd = cmd;
> > +	if (type == USBIO_PKTTYPE_CTRL || ibuf_len)
> > +		cpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
> > +	else
> > +		cpkt->header.flags = USBIO_PKTFLAG_CMP;
> > +	cpkt->len = obuf_len;
> > +
> > +	/* Copy the data */
> > +	memcpy(cpkt->data, obuf, obuf_len);
> > +
> > +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
> > +	cpkt_len = sizeof(*cpkt) + obuf_len;
> > +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
> > +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> > +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
> > +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> 
> Instead of casting, how about using %zu for printing a size_t?
> 
> > +
> > +	if (ret != cpkt_len) {
> > +		dev_err(usbio->dev, "USB control out failed: %d\n", ret);
> > +		return (ret < 0) ? ret : -EPROTO;
> 
> Redundant parentheses.

Nope!  Again, please mat it obvioue.

Actually, I hate ? : lines, this should be:
	if (ret < 0)
		return ret
	else
		return -EPROTO

by spelling it all out.

> > +static int usbio_ctrl_enumgpios(struct usbio_device *usbio)
> > +{
> > +	struct usbio_gpio_bank_desc *gpio = usbio->gpios;
> > +	int ret, i;
> 
> unsigned int i, please.

Nope, 'int' is just fine.  Please see Dan's many rants about why this is
acceptable for loops.

> > +static int usbio_ctrl_enumi2cs(struct usbio_device *usbio)
> > +{
> > +	struct usbio_i2c_bus_desc *i2c = usbio->i2cs;
> > +	int ret, i;
> 
> unsigned int i, please.

Nope, 'int' is fine.

> > +static int usbio_ctrl_enumspis(struct usbio_device *usbio)
> > +{
> > +	struct usbio_spi_bus_desc *spi = usbio->spis;
> > +	int ret, i;
> 
> Ditto.

Nope :)

> > +static void usbio_disconnect(struct usb_interface *intf)
> > +{
> > +	struct usbio_device *usbio = usb_get_intfdata(intf);
> > +	struct usbio_client *client, *prev;
> > +
> > +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> > +		auxiliary_device_delete(&client->adev);
> > +		list_del_init(&client->link);
> > +		auxiliary_device_uninit(&client->adev);
> > +	}
> > +
> > +	usb_kill_urb(usbio->urb);
> > +	usb_free_urb(usbio->urb);
> 
> What will happen on client drivers if they're working with the bridge while
> disconnect happens?
> 
> One easy solution to this could be to use an rw_semaphore where client
> acquire it for readingin conjunction (in a helper that also checks the
> interface status) and disconnect callback for writing.

How is that going to change anything?  And how can a disconnect happen?
Isn't this an onboard device?

> > +}
> > +
> > +static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > +{
> > +	struct usb_device *udev = interface_to_usbdev(intf);
> > +	struct usb_endpoint_descriptor *ep_in, *ep_out;
> > +	struct device *dev = &intf->dev;
> > +	struct usbio_device *usbio;
> > +	int ret;
> > +
> > +	usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);
> 
> usbio will be released at the exit from usbio_disconnect(). I think you'll
> need to use a release callback in struct device to release usbio once all
> clients are gone.

Which clients?  The disconnect will cause that to happen, it can not
happen at the same time probe is called.

> > +		uid = acpi_device_uid(adev);
> > +		if (uid)
> > +			for (int i = 0; i < strlen(uid); i++)
> 
> size_t i?

Ick, no, "int" is fine.

thanks,

greg k-h

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Sakari Ailus]

Hi Greg,

On Mon, Aug 11, 2025 at 09:12:36AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 11, 2025 at 06:51:03AM +0000, Sakari Ailus wrote:
> > > +/**
> > > + * struct usbio_client - represents a usbio client
> > > + *
> > > + * @adev: auxiliary device object
> > > + * @bridge: usbio bridge who service the client
> > > + * @link: usbio bridge clients list member
> > > + */
> > > +struct usbio_client {
> > > +	struct auxiliary_device adev;
> > > +	struct usbio_device *bridge;
> > > +	struct list_head link;
> > > +};
> > > +
> > > +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> > 
> > Please use a different name than "adev" for the argument, which is also the
> > struct field of interest.
> 
> Why?  That's a very common way of doing this.  My only complaint is that
> it really should be "container_of_const()" instead of just
> "container_of()"

Because the struct field has the same name. The macro isn't intended for
obtaining the container struct based on any field in the struct, only the
field called "adev".

I'll post a patch to add the container_of() check to checkpatch.pl.

> 
> 
> > > +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
> > > +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> > > +{
> > > +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> > > +	struct usbio_ctrl_packet *cpkt;
> > > +	unsigned int pipe;
> > > +	u16 cpkt_len;
> > > +	int ret;
> > > +
> > > +	lockdep_assert_held(&usbio->mutex);
> > > +
> > > +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
> > > +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> > 
> > You can (and should) remove all parentheses except the outer ones here.
> 
> No, don't do that.  If you do that you will have to manually go and try
> to remember the order of operations every time you read this code.

I presume kernel developers in general do.

But if in doubt: <URL:https://users.ece.utexas.edu/~adnan/c-refcard.pdf>.

> 
> Remember, we write code for people first, compilers second.  Make it
> totally obvious what is going on here as you want PEOPLE to catch your
> issues.

I guess people can have differing opinions on this; I find the above much
easier to read without the extra parentheses.

GCC actually nowadays generates warnings for code for which the evaluation
order is perfectly well defined (namely mixing bitwise and logical
operations AFAIR, for relying on the evaluation order between && and || I
can somehow understand that).

> 
> The statement is fine as-is.
> 
> > > +		return -EMSGSIZE;
> > > +
> > > +	/* Prepare Control Packet Header */
> > > +	cpkt = usbio->ctrlbuf;
> > > +	cpkt->header.type = type;
> > > +	cpkt->header.cmd = cmd;
> > > +	if (type == USBIO_PKTTYPE_CTRL || ibuf_len)
> > > +		cpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
> > > +	else
> > > +		cpkt->header.flags = USBIO_PKTFLAG_CMP;
> > > +	cpkt->len = obuf_len;
> > > +
> > > +	/* Copy the data */
> > > +	memcpy(cpkt->data, obuf, obuf_len);
> > > +
> > > +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
> > > +	cpkt_len = sizeof(*cpkt) + obuf_len;
> > > +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
> > > +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
> > > +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
> > > +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> > 
> > Instead of casting, how about using %zu for printing a size_t?
> > 
> > > +
> > > +	if (ret != cpkt_len) {
> > > +		dev_err(usbio->dev, "USB control out failed: %d\n", ret);
> > > +		return (ret < 0) ? ret : -EPROTO;
> > 
> > Redundant parentheses.
> 
> Nope!  Again, please mat it obvioue.
> 
> Actually, I hate ? : lines, this should be:
> 	if (ret < 0)
> 		return ret
> 	else
> 		return -EPROTO
> 
> by spelling it all out.
> 
> > > +static int usbio_ctrl_enumgpios(struct usbio_device *usbio)
> > > +{
> > > +	struct usbio_gpio_bank_desc *gpio = usbio->gpios;
> > > +	int ret, i;
> > 
> > unsigned int i, please.
> 
> Nope, 'int' is just fine.  Please see Dan's many rants about why this is
> acceptable for loops.
> 
> > > +static int usbio_ctrl_enumi2cs(struct usbio_device *usbio)
> > > +{
> > > +	struct usbio_i2c_bus_desc *i2c = usbio->i2cs;
> > > +	int ret, i;
> > 
> > unsigned int i, please.
> 
> Nope, 'int' is fine.
> 
> > > +static int usbio_ctrl_enumspis(struct usbio_device *usbio)
> > > +{
> > > +	struct usbio_spi_bus_desc *spi = usbio->spis;
> > > +	int ret, i;
> > 
> > Ditto.
> 
> Nope :)
> 
> > > +static void usbio_disconnect(struct usb_interface *intf)
> > > +{
> > > +	struct usbio_device *usbio = usb_get_intfdata(intf);
> > > +	struct usbio_client *client, *prev;
> > > +
> > > +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> > > +		auxiliary_device_delete(&client->adev);
> > > +		list_del_init(&client->link);
> > > +		auxiliary_device_uninit(&client->adev);
> > > +	}
> > > +
> > > +	usb_kill_urb(usbio->urb);
> > > +	usb_free_urb(usbio->urb);
> > 
> > What will happen on client drivers if they're working with the bridge while
> > disconnect happens?
> > 
> > One easy solution to this could be to use an rw_semaphore where client
> > acquire it for readingin conjunction (in a helper that also checks the
> > interface status) and disconnect callback for writing.
> 
> How is that going to change anything?  And how can a disconnect happen?
> Isn't this an onboard device?

It is, but the device firmware is known to crash occasionally.

The documantation says you can't access USB interfaces once disconnect has
returned. I'm not sure if there are checks to safeguard against ongoing or
additional accesses in the USB stack but on many other buses this may
simply lead to a system crash.

> 
> > > +}
> > > +
> > > +static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
> > > +{
> > > +	struct usb_device *udev = interface_to_usbdev(intf);
> > > +	struct usb_endpoint_descriptor *ep_in, *ep_out;
> > > +	struct device *dev = &intf->dev;
> > > +	struct usbio_device *usbio;
> > > +	int ret;
> > > +
> > > +	usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);
> > 
> > usbio will be released at the exit from usbio_disconnect(). I think you'll
> > need to use a release callback in struct device to release usbio once all
> > clients are gone.
> 
> Which clients?  The disconnect will cause that to happen, it can not
> happen at the same time probe is called.

Not probe but disconnect. See the two other patches in the set for client
drivers.

> 
> > > +		uid = acpi_device_uid(adev);
> > > +		if (uid)
> > > +			for (int i = 0; i < strlen(uid); i++)
> > 
> > size_t i?
> 
> Ick, no, "int" is fine.

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Greg Kroah-Hartman]

On Mon, Aug 11, 2025 at 07:29:37AM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Mon, Aug 11, 2025 at 09:12:36AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Aug 11, 2025 at 06:51:03AM +0000, Sakari Ailus wrote:
> > > > +/**
> > > > + * struct usbio_client - represents a usbio client
> > > > + *
> > > > + * @adev: auxiliary device object
> > > > + * @bridge: usbio bridge who service the client
> > > > + * @link: usbio bridge clients list member
> > > > + */
> > > > +struct usbio_client {
> > > > +	struct auxiliary_device adev;
> > > > +	struct usbio_device *bridge;
> > > > +	struct list_head link;
> > > > +};
> > > > +
> > > > +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> > > 
> > > Please use a different name than "adev" for the argument, which is also the
> > > struct field of interest.
> > 
> > Why?  That's a very common way of doing this.  My only complaint is that
> > it really should be "container_of_const()" instead of just
> > "container_of()"
> 
> Because the struct field has the same name. The macro isn't intended for
> obtaining the container struct based on any field in the struct, only the
> field called "adev".

And that's fine, the macro works like this, so all should be ok.

> I'll post a patch to add the container_of() check to checkpatch.pl.

Patch to add it to do what?

> > > > +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
> > > > +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> > > > +{
> > > > +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> > > > +	struct usbio_ctrl_packet *cpkt;
> > > > +	unsigned int pipe;
> > > > +	u16 cpkt_len;
> > > > +	int ret;
> > > > +
> > > > +	lockdep_assert_held(&usbio->mutex);
> > > > +
> > > > +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
> > > > +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> > > 
> > > You can (and should) remove all parentheses except the outer ones here.
> > 
> > No, don't do that.  If you do that you will have to manually go and try
> > to remember the order of operations every time you read this code.
> 
> I presume kernel developers in general do.
> 
> But if in doubt: <URL:https://users.ece.utexas.edu/~adnan/c-refcard.pdf>.

Don't force me to look it up all the time, use () to make it obvious
please.  That's the biggest thing I hate about that checkpatch "rule",
please do not follow it for any code that I am a maintainer for.

> > > > +static void usbio_disconnect(struct usb_interface *intf)
> > > > +{
> > > > +	struct usbio_device *usbio = usb_get_intfdata(intf);
> > > > +	struct usbio_client *client, *prev;
> > > > +
> > > > +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> > > > +		auxiliary_device_delete(&client->adev);
> > > > +		list_del_init(&client->link);
> > > > +		auxiliary_device_uninit(&client->adev);
> > > > +	}
> > > > +
> > > > +	usb_kill_urb(usbio->urb);
> > > > +	usb_free_urb(usbio->urb);
> > > 
> > > What will happen on client drivers if they're working with the bridge while
> > > disconnect happens?
> > > 
> > > One easy solution to this could be to use an rw_semaphore where client
> > > acquire it for readingin conjunction (in a helper that also checks the
> > > interface status) and disconnect callback for writing.
> > 
> > How is that going to change anything?  And how can a disconnect happen?
> > Isn't this an onboard device?
> 
> It is, but the device firmware is known to crash occasionally.

Then fix the firmware :)

> The documantation says you can't access USB interfaces once disconnect has
> returned. I'm not sure if there are checks to safeguard against ongoing or
> additional accesses in the USB stack but on many other buses this may
> simply lead to a system crash.

How can you access the USB interface after disconnect has returned on
these codepaths?  The child devices should all be cleaned up properly
after disconnect happens so there should not be a pointer to even use
anymore.

thanks,

greg k-h

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Sakari Ailus]

Hi Greg,

On Mon, Aug 11, 2025 at 10:31:22AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Aug 11, 2025 at 07:29:37AM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Mon, Aug 11, 2025 at 09:12:36AM +0200, Greg Kroah-Hartman wrote:
> > > On Mon, Aug 11, 2025 at 06:51:03AM +0000, Sakari Ailus wrote:
> > > > > +/**
> > > > > + * struct usbio_client - represents a usbio client
> > > > > + *
> > > > > + * @adev: auxiliary device object
> > > > > + * @bridge: usbio bridge who service the client
> > > > > + * @link: usbio bridge clients list member
> > > > > + */
> > > > > +struct usbio_client {
> > > > > +	struct auxiliary_device adev;
> > > > > +	struct usbio_device *bridge;
> > > > > +	struct list_head link;
> > > > > +};
> > > > > +
> > > > > +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> > > > 
> > > > Please use a different name than "adev" for the argument, which is also the
> > > > struct field of interest.
> > > 
> > > Why?  That's a very common way of doing this.  My only complaint is that
> > > it really should be "container_of_const()" instead of just
> > > "container_of()"
> > 
> > Because the struct field has the same name. The macro isn't intended for
> > obtaining the container struct based on any field in the struct, only the
> > field called "adev".
> 
> And that's fine, the macro works like this, so all should be ok.
> 
> > I'll post a patch to add the container_of() check to checkpatch.pl.
> 
> Patch to add it to do what?

You're cc'd. :-)

> 
> > > > > +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
> > > > > +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
> > > > > +{
> > > > > +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
> > > > > +	struct usbio_ctrl_packet *cpkt;
> > > > > +	unsigned int pipe;
> > > > > +	u16 cpkt_len;
> > > > > +	int ret;
> > > > > +
> > > > > +	lockdep_assert_held(&usbio->mutex);
> > > > > +
> > > > > +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
> > > > > +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> > > > 
> > > > You can (and should) remove all parentheses except the outer ones here.
> > > 
> > > No, don't do that.  If you do that you will have to manually go and try
> > > to remember the order of operations every time you read this code.
> > 
> > I presume kernel developers in general do.
> > 
> > But if in doubt: <URL:https://users.ece.utexas.edu/~adnan/c-refcard.pdf>.
> 
> Don't force me to look it up all the time, use () to make it obvious
> please.  That's the biggest thing I hate about that checkpatch "rule",
> please do not follow it for any code that I am a maintainer for.
> 
> > > > > +static void usbio_disconnect(struct usb_interface *intf)
> > > > > +{
> > > > > +	struct usbio_device *usbio = usb_get_intfdata(intf);
> > > > > +	struct usbio_client *client, *prev;
> > > > > +
> > > > > +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> > > > > +		auxiliary_device_delete(&client->adev);
> > > > > +		list_del_init(&client->link);
> > > > > +		auxiliary_device_uninit(&client->adev);
> > > > > +	}
> > > > > +
> > > > > +	usb_kill_urb(usbio->urb);
> > > > > +	usb_free_urb(usbio->urb);
> > > > 
> > > > What will happen on client drivers if they're working with the bridge while
> > > > disconnect happens?
> > > > 
> > > > One easy solution to this could be to use an rw_semaphore where client
> > > > acquire it for readingin conjunction (in a helper that also checks the
> > > > interface status) and disconnect callback for writing.
> > > 
> > > How is that going to change anything?  And how can a disconnect happen?
> > > Isn't this an onboard device?
> > 
> > It is, but the device firmware is known to crash occasionally.
> 
> Then fix the firmware :)

In practice this depends on device vendor and we all know the quality of
firmware from random sources. These are not Intel-implemented devices.

> 
> > The documantation says you can't access USB interfaces once disconnect has
> > returned. I'm not sure if there are checks to safeguard against ongoing or
> > additional accesses in the USB stack but on many other buses this may
> > simply lead to a system crash.
> 
> How can you access the USB interface after disconnect has returned on
> these codepaths?  The child devices should all be cleaned up properly
> after disconnect happens so there should not be a pointer to even use
> anymore.

See functions exported by the main USBIO driver.

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Hans de Goede]

Hi,

On 11-Aug-25 11:23 AM, Sakari Ailus wrote:

...

>>> The documantation says you can't access USB interfaces once disconnect has
>>> returned. I'm not sure if there are checks to safeguard against ongoing or
>>> additional accesses in the USB stack but on many other buses this may
>>> simply lead to a system crash.
>>
>> How can you access the USB interface after disconnect has returned on
>> these codepaths?  The child devices should all be cleaned up properly
>> after disconnect happens so there should not be a pointer to even use
>> anymore.
> 
> See functions exported by the main USBIO driver.

As Greg says the rules for USB devices are clear no usb_device access
in any form is allowed after the disconnect() callback completes.

It is up to the USBIO driver to ensure that this does not happen,
also see my proposed disconnect() changes in reply to your review.

Note those changes are to fix races between the auxdev child/client
drivers and disconnect() the auxdev client drivers also should no
longer make any calls to the USBIO functions after their remove()
callback has completed.

Regards,

Hans

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Hans de Goede]

Hi Sakari,

On 11-Aug-25 8:51 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for posting this. Some comments below...
> 
> On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
>> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>>
>> Add a driver for the Intel USBIO USB IO-expander used by the MIPI cameras
>> on various new (Meteor Lake and later) Intel laptops.
>>
>> This is an USB bridge driver which adds auxbus child devices for the GPIO,
>> I2C and SPI functions of the USBIO chip and which exports IO-functions for
>> the drivers for the auxbus child devices to communicate with the USBIO
>> device's firmware.
>>
>> Co-developed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>> ---
>>  MAINTAINERS               |   7 +
>>  drivers/usb/misc/Kconfig  |  14 +
>>  drivers/usb/misc/Makefile |   1 +
>>  drivers/usb/misc/usbio.c  | 693 ++++++++++++++++++++++++++++++++++++++
>>  include/linux/usb/usbio.h | 168 +++++++++
>>  5 files changed, 883 insertions(+)
>>  create mode 100644 drivers/usb/misc/usbio.c
>>  create mode 100644 include/linux/usb/usbio.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 0618351510ad..1208efe41f9f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -12518,6 +12518,13 @@ S:	Maintained
>>  F:	Documentation/admin-guide/pm/intel_uncore_frequency_scaling.rst
>>  F:	drivers/platform/x86/intel/uncore-frequency/
>>  
>> +INTEL USBIO USB I/O EXPANDER DRIVERS
>> +M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>> +M:	Hans de Goede <hansg@kernel.org>
> 
> Could you add:
> 
> R:	Sakari Ailus <sakari.ailus@linux.intel.com>

Ack.

> 
> ?
> 
>> +S:	Maintained
>> +F:	drivers/usb/misc/usbio.c
>> +F:	include/linux/usb/usbio.h
>> +
>>  INTEL VENDOR SPECIFIC EXTENDED CAPABILITIES DRIVER
>>  M:	David E. Box <david.e.box@linux.intel.com>
>>  S:	Supported

...

>> diff --git a/drivers/usb/misc/usbio.c b/drivers/usb/misc/usbio.c
>> new file mode 100644
>> index 000000000000..88197092f39a
>> --- /dev/null
>> +++ b/drivers/usb/misc/usbio.c

...

>> +/**
>> + * struct usbio_client - represents a usbio client
>> + *
>> + * @adev: auxiliary device object
>> + * @bridge: usbio bridge who service the client
>> + * @link: usbio bridge clients list member
>> + */
>> +struct usbio_client {
>> +	struct auxiliary_device adev;
>> +	struct usbio_device *bridge;
>> +	struct list_head link;
>> +};
>> +
>> +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> 
> Please use a different name than "adev" for the argument, which is also the
> struct field of interest.

As gkh mentioned doing things this way is quite normal.

>> +
>> +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
>> +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
>> +{
>> +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
>> +	struct usbio_ctrl_packet *cpkt;
>> +	unsigned int pipe;
>> +	u16 cpkt_len;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&usbio->mutex);
>> +
>> +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
>> +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> 
> You can (and should) remove all parentheses except the outer ones here.

I disagree, even not very experienced C-programmer can read the above,
wile it takes a really experienced programmer to read it without
parentheses. Even I after C-programming for 30 years still need to
look up the operator order sometimes...

> 
>> +		return -EMSGSIZE;
>> +
>> +	/* Prepare Control Packet Header */
>> +	cpkt = usbio->ctrlbuf;
>> +	cpkt->header.type = type;
>> +	cpkt->header.cmd = cmd;
>> +	if (type == USBIO_PKTTYPE_CTRL || ibuf_len)
>> +		cpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
>> +	else
>> +		cpkt->header.flags = USBIO_PKTFLAG_CMP;
>> +	cpkt->len = obuf_len;
>> +
>> +	/* Copy the data */
>> +	memcpy(cpkt->data, obuf, obuf_len);
>> +
>> +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
>> +	cpkt_len = sizeof(*cpkt) + obuf_len;
>> +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
>> +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
>> +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
>> +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> 
> Instead of casting, how about using %zu for printing a size_t?

Ack.

>> +
>> +	if (ret != cpkt_len) {
>> +		dev_err(usbio->dev, "USB control out failed: %d\n", ret);
>> +		return (ret < 0) ? ret : -EPROTO;
> 
> Redundant parentheses.

Disagree, see above.

> 
>> +	}
>> +
>> +	if (!(cpkt->header.flags & USBIO_PKTFLAG_ACK))
>> +		return 0;
>> +
>> +	pipe = usb_rcvctrlpipe(usbio->udev, usbio->ctrl_pipe);
>> +	cpkt_len = sizeof(*cpkt) + ibuf_len;
>> +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_IN, 0, 0,
>> +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
>> +	dev_dbg(usbio->dev, "control in %d hdr %*phN data %*phN\n", ret,
>> +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
>> +
>> +	if (ret < sizeof(*cpkt)) {
>> +		dev_err(usbio->dev, "USB control in failed: %d\n", ret);
>> +		return (ret < 0) ? ret : -EPROTO;
> 
> Remove parentheses? There's no need to try to comply with MISRA C, is
> there? :-) I'm not commenting anymore on further instances of this.
> 
>> +	}
>> +
>> +	if (cpkt->header.type != type || cpkt->header.cmd != cmd ||
>> +	    !(cpkt->header.flags & USBIO_PKTFLAG_RSP)) {
>> +		dev_err(usbio->dev, "Unexpected reply type: %u, cmd: %u, flags: %u\n",
>> +			cpkt->header.type, cpkt->header.cmd, cpkt->header.flags);
>> +		return -EPROTO;
>> +	}
>> +
>> +	if (cpkt->header.flags & USBIO_PKTFLAG_ERR)
>> +		return -EREMOTEIO;
>> +
>> +	if (ibuf_len < cpkt->len)
>> +		return -ENOSPC;
>> +
>> +	memcpy(ibuf, cpkt->data, cpkt->len);
> 
> It'd be nice to have one more newline here.

Ack.

>> +	return cpkt->len;
>> +}
>> +
>> +int usbio_control_msg(struct auxiliary_device *adev, u8 type, u8 cmd,
>> +		      const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
>> +{
>> +	struct usbio_client *client = adev_to_client(adev);
>> +	int ret;
>> +
>> +	ret = usbio_acquire(adev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = usbio_ctrl_msg(client->bridge, type, cmd, obuf, obuf_len, ibuf, ibuf_len);
>> +
>> +	usbio_release(adev);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(usbio_control_msg, "USBIO");
>> +
>> +static void usbio_bulk_recv(struct urb *urb)
>> +{
>> +	struct usbio_bulk_packet *bpkt = urb->transfer_buffer;
>> +	struct usbio_device *usbio = urb->context;
>> +
>> +	if (!urb->status) {
>> +		if (bpkt->header.flags & USBIO_PKTFLAG_RSP) {
>> +			usbio->rxdat_len = urb->actual_length;
>> +			complete(&usbio->done);
>> +		}
>> +	} else if (urb->status != -ENOENT) {
>> +		dev_err(usbio->dev, "Bulk in error %d\n", urb->status);
>> +	}
>> +
>> +	usb_submit_urb(usbio->urb, GFP_ATOMIC);
>> +}
>> +
>> +int usbio_bulk_msg(struct auxiliary_device *adev, u8 type, u8 cmd, bool last,
>> +		   const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
>> +{
>> +	struct usbio_device *usbio = adev_to_client(adev)->bridge;
>> +	struct usbio_bulk_packet *bpkt;
>> +	int ret, act;
>> +	u16 bpkt_len;
>> +
>> +	lockdep_assert_held(&usbio->mutex);
>> +
>> +	if ((obuf_len > (usbio->txbuf_len - sizeof(*bpkt))) ||
>> +	    (ibuf_len > (usbio->txbuf_len - sizeof(*bpkt))))
>> +		return -EMSGSIZE;
>> +
>> +	if (ibuf_len)
>> +		reinit_completion(&usbio->done);
>> +
>> +	/* If no data to send, skip to read */
>> +	if (!obuf_len)
>> +		goto read;
>> +
>> +	/* Prepare Bulk Packet Header */
>> +	bpkt = usbio->txbuf;
>> +	bpkt->header.type = type;
>> +	bpkt->header.cmd = cmd;
>> +	if (!last)
>> +		bpkt->header.flags = 0;
>> +	else if (ibuf_len)
>> +		bpkt->header.flags = USBIO_PKTFLAGS_REQRESP;
>> +	else
>> +		bpkt->header.flags = USBIO_PKTFLAG_CMP;
>> +	bpkt->len = obuf_len;
>> +
>> +	/* Copy the data */
>> +	memcpy(bpkt->data, obuf, obuf_len);
>> +
>> +	bpkt_len = sizeof(*bpkt) + obuf_len;
>> +	ret = usb_bulk_msg(usbio->udev, usbio->tx_pipe, bpkt, bpkt_len, &act,
>> +			   USBIO_BULKXFER_TIMEOUT);
>> +	dev_dbg(usbio->dev, "bulk out %d hdr %*phN data %*phN\n", act,
>> +		(int)sizeof(*bpkt), bpkt, (int)bpkt->len, bpkt->data);
>> +
>> +	if (ret || act != bpkt_len) {
>> +		dev_err(usbio->dev, "Bulk out failed: %d\n", ret);
>> +		return ret ?: -EPROTO;
>> +	}
>> +
>> +	if (!(bpkt->header.flags & USBIO_PKTFLAG_ACK))
>> +		return obuf_len;
>> +
>> +read:
>> +	ret = wait_for_completion_timeout(&usbio->done, USBIO_BULKXFER_TIMEOUT);
>> +	if (ret <= 0) {
>> +		dev_err(usbio->dev, "Bulk in wait failed: %d\n", ret);
>> +		return ret ?: -ETIMEDOUT;
>> +	}
>> +
>> +	act = usbio->rxdat_len;
>> +	bpkt = usbio->rxbuf;
>> +	dev_dbg(usbio->dev, "bulk in %d hdr %*phN data %*phN\n", act,
>> +		(int)sizeof(*bpkt), bpkt, (int)bpkt->len, bpkt->data);
> 
> %zu, please.

Ack.

>> +
>> +	/*
>> +	 * Unsupported bulk commands get only an usbio_packet_header with
>> +	 * the error flag set as reply. Return -EPIPE for this case.
>> +	 */
>> +	if (act == sizeof(struct usbio_packet_header) &&
>> +	    (bpkt->header.flags & USBIO_PKTFLAG_ERR))
>> +		return -EPIPE;
>> +
>> +	if (act < sizeof(*bpkt)) {
>> +		dev_err(usbio->dev, "Bulk in short read: %d\n", act);
>> +		return -EPROTO;
>> +	}
>> +
>> +	if (bpkt->header.type != type || bpkt->header.cmd != cmd ||
>> +	    !(bpkt->header.flags & USBIO_PKTFLAG_RSP)) {
>> +		dev_err(usbio->dev,
>> +			"Unexpected bulk in type 0x%02x cmd 0x%02x flags 0x%02x\n",
>> +			bpkt->header.type, bpkt->header.cmd, bpkt->header.flags);
>> +		return -EPROTO;
>> +	}
>> +
>> +	if (bpkt->header.flags & USBIO_PKTFLAG_ERR)
>> +		return -EREMOTEIO;
>> +
>> +	if (ibuf_len < bpkt->len)
>> +		return -ENOSPC;
>> +
>> +	memcpy(ibuf, bpkt->data, bpkt->len);
> 
> One more newline, perhaps?

Ack.

>> +	return bpkt->len;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(usbio_bulk_msg, "USBIO");
>> +
>> +static int usbio_get_and_lock(struct usbio_device *usbio)
>> +{
>> +	int ret;
>> +
>> +	ret = usb_autopm_get_interface(usbio->intf);
>> +	if (ret)
>> +		return ret;
>> +
>> +	mutex_lock(&usbio->mutex);
>> +	return 0;
>> +}
>> +
>> +static void usbio_unlock_and_put(struct usbio_device *usbio)
>> +{
>> +	mutex_unlock(&usbio->mutex);
>> +	usb_autopm_put_interface(usbio->intf);
>> +}
>> +
>> +int usbio_acquire(struct auxiliary_device *adev)
>> +{
>> +	return usbio_get_and_lock(adev_to_client(adev)->bridge);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(usbio_acquire, "USBIO");
>> +
>> +void usbio_release(struct auxiliary_device *adev)
>> +{
>> +	usbio_unlock_and_put(adev_to_client(adev)->bridge);
>> +}
>> +EXPORT_SYMBOL_NS_GPL(usbio_release, "USBIO");
>> +
>> +void usbio_get_txrxbuf_len(struct auxiliary_device *adev, u16 *txbuf_len, u16 *rxbuf_len)
>> +{
>> +	struct usbio_device *usbio = adev_to_client(adev)->bridge;
>> +
>> +	*txbuf_len = usbio->txbuf_len;
>> +	*rxbuf_len = usbio->rxbuf_len;
>> +}
>> +EXPORT_SYMBOL_NS_GPL(usbio_get_txrxbuf_len, "USBIO");
>> +
>> +static void usbio_auxdev_release(struct device *dev)
>> +{
>> +	struct auxiliary_device *adev = to_auxiliary_dev(dev);
>> +	struct usbio_client *client = adev_to_client(adev);
>> +
>> +	kfree(client);
>> +}
>> +
>> +static int usbio_add_client(struct usbio_device *usbio, char *name, u8 id, void *data)
>> +{
>> +	struct usbio_client *client;
>> +	struct auxiliary_device *adev;
>> +	int ret;
>> +
>> +	client = kzalloc(sizeof(*client), GFP_KERNEL);
>> +	if (!client)
>> +		return -ENOMEM;
>> +
>> +	client->bridge = usbio;
>> +	adev = &client->adev;
>> +	adev->name = name;
>> +	adev->id = id;
>> +
>> +	adev->dev.parent = usbio->dev;
>> +	adev->dev.platform_data = data;
>> +	adev->dev.release = usbio_auxdev_release;
>> +
>> +	ret = auxiliary_device_init(adev);
>> +	if (!ret) {
>> +		if (auxiliary_device_add(adev))
>> +			auxiliary_device_uninit(adev);
> 
> This isn't enough for error handling if auxiliary_device_add() fails.

Ack, I spotted this myself already but I somehow forgot to
make a note to fix this, so this slipped through.

> 
>> +	}
>> +
>> +	if (ret) {
>> +		dev_err(usbio->dev, "Client %s.%d add failed: %d\n", name, id, ret);
>> +		kfree(client);
>> +		return ret;
>> +	}
>> +
>> +	list_add_tail(&client->link, &usbio->cli_list);
> 
> A newline, please?

Ack.

>> +	return 0;
>> +}
>> +
>> +static int usbio_ctrl_enumgpios(struct usbio_device *usbio)
>> +{
>> +	struct usbio_gpio_bank_desc *gpio = usbio->gpios;
>> +	int ret, i;
> 
> unsigned int i, please.

Since there are conflicting opinions on this I'm going to
keep this as is.

>> +
>> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMGPIO, NULL, 0,
>> +			     gpio, sizeof(usbio->gpios));
>> +	if (ret < 0 || ret % sizeof(*gpio)) {
>> +		dev_err(usbio->dev, "CTRL ENUMGPIO failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usbio->nr_gpio_banks = ret / sizeof(*gpio);
>> +	dev_dbg(usbio->dev, "GPIO Banks: %d\n", usbio->nr_gpio_banks);
>> +	for (i = 0; i < usbio->nr_gpio_banks; i++)
>> +		dev_dbg(usbio->dev, "\tBank%d[%d] map: %#08x\n",
>> +			gpio[i].id, gpio[i].pins, gpio[i].bmap);
>> +
>> +	usbio_add_client(usbio, USBIO_GPIO_CLIENT, 0, gpio);
> 
> One more newline?

Sure.

>> +	return 0;
>> +}
>> +
>> +static int usbio_ctrl_enumi2cs(struct usbio_device *usbio)
>> +{
>> +	struct usbio_i2c_bus_desc *i2c = usbio->i2cs;
>> +	int ret, i;
> 
> unsigned int i, please.
> 
>> +
>> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMI2C, NULL, 0,
>> +			     i2c, sizeof(usbio->i2cs));
>> +	if (ret < 0 || ret % sizeof(*i2c)) {
>> +		dev_err(usbio->dev, "CTRL ENUMI2C failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usbio->nr_i2c_buses = ret / sizeof(*i2c);
>> +	dev_dbg(usbio->dev, "I2C Busses: %d\n", usbio->nr_i2c_buses);
>> +	for (i = 0; i < usbio->nr_i2c_buses; i++) {
>> +		dev_dbg(usbio->dev, "\tBus%d caps: %#02x\n", i2c[i].id, i2c[i].caps);
>> +		usbio_add_client(usbio, USBIO_I2C_CLIENT, i, &i2c[i]);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int usbio_ctrl_enumspis(struct usbio_device *usbio)
>> +{
>> +	struct usbio_spi_bus_desc *spi = usbio->spis;
>> +	int ret, i;
> 
> Ditto.
> 
>> +
>> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_ENUMSPI, NULL, 0,
>> +			     spi, sizeof(usbio->spis));
>> +	if (ret < 0 || ret % sizeof(*spi)) {
>> +		dev_err(usbio->dev, "CTRL ENUMSPI failed: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usbio->nr_spi_buses = ret / sizeof(*spi);
>> +	dev_dbg(usbio->dev, "SPI Busses: %d\n", usbio->nr_spi_buses);
>> +	for (i = 0; i < usbio->nr_spi_buses; i++)
>> +		dev_dbg(usbio->dev, "\tBus%d caps: %#02x\n", spi[i].id, spi[i].caps);
>> +
>> +	return 0;
>> +}
>> +
>> +static int usbio_suspend(struct usb_interface *intf, pm_message_t msg)
>> +{
>> +	struct usbio_device *usbio = usb_get_intfdata(intf);
>> +
>> +	usb_kill_urb(usbio->urb);
>> +
>> +	return 0;
>> +}
>> +
>> +static int usbio_resume(struct usb_interface *intf)
>> +{
>> +	struct usbio_device *usbio = usb_get_intfdata(intf);
>> +
>> +	return usb_submit_urb(usbio->urb, GFP_KERNEL);
>> +}
>> +
>> +static void usbio_disconnect(struct usb_interface *intf)
>> +{
>> +	struct usbio_device *usbio = usb_get_intfdata(intf);
>> +	struct usbio_client *client, *prev;
>> +
>> +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
>> +		auxiliary_device_delete(&client->adev);
>> +		list_del_init(&client->link);
>> +		auxiliary_device_uninit(&client->adev);
>> +	}
>> +
>> +	usb_kill_urb(usbio->urb);
>> +	usb_free_urb(usbio->urb);
> 
> What will happen on client drivers if they're working with the bridge while
> disconnect happens?

All clients will have their remove() function called. If clients
somehow need to have some userspace-api related bits stick around
longer they will need to handle that themselves. Clients are no longer
allowed to call an usbio_*() functions after having their remove()
function called.

> One easy solution to this could be to use an rw_semaphore where client
> acquire it for readingin conjunction (in a helper that also checks the
> interface status) and disconnect callback for writing.

That is not necessary.

One thing which should be done here is set a disconnected flag to make new
usbio_*() calls fail immediately and signal the "done" completion to wakeup
any waiters, something like this:

static void usbio_disconnect(struct usb_interface *intf)
{
	struct usbio_device *usbio = usb_get_intfdata(intf);
	struct usbio_client *client, *prev;

	list_for_each_entry(client, &usbio->cli_list, link)
		client->disconnected = true;

	complete(&usbio->done);

	/* Ensure all clients see the disconnect flag */
	mutex_lock(&usbio->mutex);
	mutex_unlock(&usbio->mutex);

	/* From here on clients will no longer touch struct usbio_device */
	usb_kill_urb(usbio->urb);
	usb_free_urb(usbio->urb);

	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
		auxiliary_device_delete(&client->adev);
		list_del_init(&client->link);
		auxiliary_device_uninit(&client->adev);
	}
}

Combined with checking client->disconnected in entry points
and after waiting for the completion.

This will even allow the client to keep the struct auxdev
around by keeping a reference and it will even be safe for
the client to make usbio_*() calls on the auxdev since as
long as the reference is there the memory of the usbio_client
which embeds the auxdev will stick around and the disconnected
flag will ensure this is safe. Note client drivers really should
not do that though!





	

> 
>> +}
>> +
>> +static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
>> +{
>> +	struct usb_device *udev = interface_to_usbdev(intf);
>> +	struct usb_endpoint_descriptor *ep_in, *ep_out;
>> +	struct device *dev = &intf->dev;
>> +	struct usbio_device *usbio;
>> +	int ret;
>> +
>> +	usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);
> 
> usbio will be released at the exit from usbio_disconnect(). I think you'll
> need to use a release callback in struct device to release usbio once all
> clients are gone.

This is fine, see above it is up to the client drivers to no longer
call any usbio_*() also for v2 I'll make all the public usbio_*()
functions test client->disconnected and then they will never deref
client->bridge.

> 
>> +	if (!usbio)
>> +		return -ENOMEM;
>> +
>> +	ret = devm_mutex_init(dev, &usbio->mutex);
>> +	if (ret)
>> +		return ret;
>> +
>> +	usbio->dev = dev;
>> +	usbio->udev = udev;
>> +	usbio->intf = intf;
>> +	init_completion(&usbio->done);
>> +	INIT_LIST_HEAD(&usbio->cli_list);
>> +	usb_set_intfdata(intf, usbio);
>> +
>> +	usbio->ctrl_pipe = usb_endpoint_num(&udev->ep0.desc);
>> +	usbio->ctrlbuf_len = usb_maxpacket(udev, usbio->ctrl_pipe);
>> +	usbio->ctrlbuf = devm_kzalloc(dev, usbio->ctrlbuf_len, GFP_KERNEL);
>> +	if (!usbio->ctrlbuf)
>> +		return -ENOMEM;
>> +
>> +	/* Find the first bulk-in and bulk-out endpoints */
>> +	ret = usb_find_common_endpoints(intf->cur_altsetting, &ep_in, &ep_out,
>> +					NULL, NULL);
>> +	if (ret) {
>> +		dev_err(dev, "Cannot find bulk endpoints: %d\n", ret);
>> +		return ret;
>> +	}
>> +
>> +	usbio->tx_pipe = usb_sndbulkpipe(udev, usb_endpoint_num(ep_out));
>> +	usbio->txbuf_len = usb_endpoint_maxp(ep_out);
>> +	usbio->txbuf = devm_kzalloc(dev, usbio->txbuf_len, GFP_KERNEL);
>> +	if (!usbio->txbuf)
>> +		return -ENOMEM;
>> +
>> +	usbio->rx_pipe = usb_rcvbulkpipe(udev, usb_endpoint_num(ep_in));
>> +	usbio->rxbuf_len = usb_endpoint_maxp(ep_in);
>> +	usbio->rxbuf = devm_kzalloc(dev, usbio->rxbuf_len, GFP_KERNEL);
>> +	if (!usbio->rxbuf)
>> +		return -ENOMEM;
>> +
>> +	usbio->urb = usb_alloc_urb(0, GFP_KERNEL);
>> +	if (!usbio->urb)
>> +		return -ENOMEM;
>> +
>> +	usb_fill_bulk_urb(usbio->urb, udev, usbio->rx_pipe, usbio->rxbuf,
>> +			  usbio->rxbuf_len, usbio_bulk_recv, usbio);
>> +	ret = usb_submit_urb(usbio->urb, GFP_KERNEL);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Submitting usb urb\n");
>> +
>> +	ret = usbio_get_and_lock(usbio);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_HS, NULL, 0, NULL, 0);
>> +	if (ret < 0)
>> +		goto error;
>> +
>> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_PROTVER, NULL, 0,
>> +			     &usbio->info.protver, sizeof(struct usbio_protver));
>> +	if (ret < 0)
>> +		goto error;
>> +
>> +	ret = usbio_ctrl_msg(usbio, USBIO_PKTTYPE_CTRL, USBIO_CTRLCMD_FWVER, NULL, 0,
>> +			     &usbio->info.fwver, sizeof(struct usbio_fwver));
>> +	if (ret < 0)
>> +		goto error;
>> +
>> +	dev_dbg(dev, "ProtVer(BCD): %02x FwVer: %d.%d.%d.%d\n",
>> +		usbio->info.protver.ver, usbio->info.fwver.major,
>> +		usbio->info.fwver.minor, usbio->info.fwver.patch,
>> +		usbio->info.fwver.build);
>> +
>> +	usbio_ctrl_enumgpios(usbio);
>> +	usbio_ctrl_enumi2cs(usbio);
>> +	usbio_ctrl_enumspis(usbio);
>> +	usbio_unlock_and_put(usbio);
> 
> A newline here?

Sure.

> 
>> +	return 0;
>> +
>> +error:
>> +	usbio_unlock_and_put(usbio);
>> +	usbio_disconnect(intf);
> 
> Ditto.

Sure.

> 
>> +	return ret;
>> +}
>> +
>> +static const struct usb_device_id usbio_table[] = {
>> +	{ USB_DEVICE(0x2AC1, 0x20C1) }, /* Lattice NX40 */
>> +	{ USB_DEVICE(0x2AC1, 0x20C9) }, /* Lattice NX33 */
>> +	{ USB_DEVICE(0x2AC1, 0x20CB) }, /* Lattice NX33U */
>> +	{ USB_DEVICE(0x06CB, 0x0701) }, /* Synaptics Sabre */
> 
> Lower case hexadecimals would be nice.

Ack.

> 
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(usb, usbio_table);
>> +
>> +static struct usb_driver usbio_driver = {
>> +	.name = "usbio-bridge",
>> +	.probe = usbio_probe,
>> +	.disconnect = usbio_disconnect,
>> +	.suspend = usbio_suspend,
>> +	.resume = usbio_resume,
>> +	.id_table = usbio_table,
>> +	.supports_autosuspend = 1
> 
> Add the leading comma perhaps?

Ack.

> 
>> +};
>> +module_usb_driver(usbio_driver);
>> +
>> +struct usbio_match_ids_walk_data {
>> +	struct acpi_device *adev;
>> +	const struct acpi_device_id *hids;
>> +	const u8 id;
>> +};
>> +
>> +static int usbio_match_device_ids(struct acpi_device *adev, void *data)
>> +{
>> +	struct usbio_match_ids_walk_data *wd = data;
>> +	unsigned int id = 0;
>> +	char *uid;
>> +
>> +	if (!acpi_match_device_ids(adev, wd->hids)) {
> 
> You could return here if acpi_match_device_ids() returns non-zero and
> unindent the rest.

Ack.

> 
>> +		uid = acpi_device_uid(adev);
>> +		if (uid)
>> +			for (int i = 0; i < strlen(uid); i++)
> 
> size_t i?

No, see other discussion about this.

>> +				if (!kstrtouint(&uid[i], 10, &id))
>> +					break;
>> +
>> +		if (!uid || (uid && wd->id == (u8)id)) {
>> +			wd->adev = adev;
>> +			return 1;
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +

...

Regards,

Hans

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Sakari Ailus]

Hi Hans,

On Mon, Aug 11, 2025 at 11:13:16AM +0200, Hans de Goede wrote:
> >> +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> > 
> > Please use a different name than "adev" for the argument, which is also the
> > struct field of interest.
> 
> As gkh mentioned doing things this way is quite normal.

Isn't this exactly what people have been warning against because it'll lead
to hard-to-find bugs and requesting static inline functions being used
instead? Why not now?

...

> >> +static void usbio_disconnect(struct usb_interface *intf)
> >> +{
> >> +	struct usbio_device *usbio = usb_get_intfdata(intf);
> >> +	struct usbio_client *client, *prev;
> >> +
> >> +	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> >> +		auxiliary_device_delete(&client->adev);
> >> +		list_del_init(&client->link);
> >> +		auxiliary_device_uninit(&client->adev);
> >> +	}
> >> +
> >> +	usb_kill_urb(usbio->urb);
> >> +	usb_free_urb(usbio->urb);
> > 
> > What will happen on client drivers if they're working with the bridge while
> > disconnect happens?
> 
> All clients will have their remove() function called. If clients
> somehow need to have some userspace-api related bits stick around
> longer they will need to handle that themselves. Clients are no longer
> allowed to call an usbio_*() functions after having their remove()
> function called.
> 
> > One easy solution to this could be to use an rw_semaphore where client
> > acquire it for readingin conjunction (in a helper that also checks the
> > interface status) and disconnect callback for writing.
> 
> That is not necessary.
> 
> One thing which should be done here is set a disconnected flag to make new
> usbio_*() calls fail immediately and signal the "done" completion to wakeup
> any waiters, something like this:
> 
> static void usbio_disconnect(struct usb_interface *intf)
> {
> 	struct usbio_device *usbio = usb_get_intfdata(intf);
> 	struct usbio_client *client, *prev;
> 
> 	list_for_each_entry(client, &usbio->cli_list, link)
> 		client->disconnected = true;
> 
> 	complete(&usbio->done);
> 
> 	/* Ensure all clients see the disconnect flag */
> 	mutex_lock(&usbio->mutex);
> 	mutex_unlock(&usbio->mutex);
> 
> 	/* From here on clients will no longer touch struct usbio_device */
> 	usb_kill_urb(usbio->urb);
> 	usb_free_urb(usbio->urb);
> 
> 	list_for_each_entry_safe_reverse(client, prev, &usbio->cli_list, link) {
> 		auxiliary_device_delete(&client->adev);
> 		list_del_init(&client->link);
> 		auxiliary_device_uninit(&client->adev);
> 	}
> }
> 
> Combined with checking client->disconnected in entry points
> and after waiting for the completion.
> 
> This will even allow the client to keep the struct auxdev
> around by keeping a reference and it will even be safe for
> the client to make usbio_*() calls on the auxdev since as
> long as the reference is there the memory of the usbio_client
> which embeds the auxdev will stick around and the disconnected
> flag will ensure this is safe. Note client drivers really should
> not do that though!

Ack, sounds good. I'll wait for v2 then.

> 
> 
> 
> 
> 
> 	
> 
> > 
> >> +}
> >> +
> >> +static int usbio_probe(struct usb_interface *intf, const struct usb_device_id *id)
> >> +{
> >> +	struct usb_device *udev = interface_to_usbdev(intf);
> >> +	struct usb_endpoint_descriptor *ep_in, *ep_out;
> >> +	struct device *dev = &intf->dev;
> >> +	struct usbio_device *usbio;
> >> +	int ret;
> >> +
> >> +	usbio = devm_kzalloc(dev, sizeof(*usbio), GFP_KERNEL);
> > 
> > usbio will be released at the exit from usbio_disconnect(). I think you'll
> > need to use a release callback in struct device to release usbio once all
> > clients are gone.
> 
> This is fine, see above it is up to the client drivers to no longer
> call any usbio_*() also for v2 I'll make all the public usbio_*()
> functions test client->disconnected and then they will never deref
> client->bridge.

Yes, keeping this information on the client side should be equally good.

-- 
Regards,

Sakari Ailus

Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

[Hans de Goede]

Hi Sakari,

On 11-Aug-25 8:51 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for posting this. Some comments below...
> 
> On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
>> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>>
>> Add a driver for the Intel USBIO USB IO-expander used by the MIPI cameras
>> on various new (Meteor Lake and later) Intel laptops.
>>
>> This is an USB bridge driver which adds auxbus child devices for the GPIO,
>> I2C and SPI functions of the USBIO chip and which exports IO-functions for
>> the drivers for the auxbus child devices to communicate with the USBIO
>> device's firmware.
>>
>> Co-developed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>

Once more thank you for the review.

I'm replying here to correct some of my previous replies to your
review. Anything not mentioned below I've addressed as discussed
before.

...

>> diff --git a/drivers/usb/misc/usbio.c b/drivers/usb/misc/usbio.c
>> new file mode 100644
>> index 000000000000..88197092f39a
>> --- /dev/null
>> +++ b/drivers/usb/misc/usbio.c
>> @@ -0,0 +1,693 @@

...

>> +#define adev_to_client(adev) container_of(adev, struct usbio_client, adev)
> 
> Please use a different name than "adev" for the argument, which is also the
> struct field of interest.

Ignore my previous comment on this remark. I've decided to
clean this up for v2 and use a different names for the 2
container_of() arguments.

>> +
>> +static int usbio_ctrl_msg(struct usbio_device *usbio, u8 type, u8 cmd,
>> +			  const void *obuf, u16 obuf_len, void *ibuf, u16 ibuf_len)
>> +{
>> +	u8 request = USB_TYPE_VENDOR | USB_RECIP_DEVICE;
>> +	struct usbio_ctrl_packet *cpkt;
>> +	unsigned int pipe;
>> +	u16 cpkt_len;
>> +	int ret;
>> +
>> +	lockdep_assert_held(&usbio->mutex);
>> +
>> +	if ((obuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))) ||
>> +	    (ibuf_len > (usbio->ctrlbuf_len - sizeof(*cpkt))))
> 
> You can (and should) remove all parentheses except the outer ones here.

As mentioned by Greg parentheses help to keep the code readable, so
I'm going to keep these as well as those in other places where you've
asked to drop them.

...

>> +	pipe = usb_sndctrlpipe(usbio->udev, usbio->ctrl_pipe);
>> +	cpkt_len = sizeof(*cpkt) + obuf_len;
>> +	ret = usb_control_msg(usbio->udev, pipe, 0, request | USB_DIR_OUT, 0, 0,
>> +			      cpkt, cpkt_len, USBIO_CTRLXFER_TIMEOUT);
>> +	dev_dbg(usbio->dev, "control out %d hdr %*phN data %*phN\n", ret,
>> +		(int)sizeof(*cpkt), cpkt, (int)cpkt->len, cpkt->data);
> 
> Instead of casting, how about using %zu for printing a size_t?

This is not printing the size, this is filling the * field-width
parameter for the %*phN dumping of the header. Using an int when
setting field-width with * is mandatory. The same applies to
your other %zu review remarks.

...

>> +	if (ibuf_len < cpkt->len)
>> +		return -ENOSPC;
>> +
>> +	memcpy(ibuf, cpkt->data, cpkt->len);
> 
> It'd be nice to have one more newline here.

As already mentioned I've fixed for v2, as well as all the other
requests for extra newlines.

> 
>> +	return cpkt->len;
>> +}

...

Regards,

Hans

[PATCH 2/3] gpio: Add Intel USBIO GPIO driver

[Hans de Goede]

From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>

Add a a driver for the GPIO auxbus child device of the Intel USBIO USB
IO-expander used by the MIPI cameras on various new (Meteor Lake and
later) Intel laptops.

Co-developed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
---
 MAINTAINERS               |   1 +
 drivers/gpio/Kconfig      |  11 ++
 drivers/gpio/Makefile     |   1 +
 drivers/gpio/gpio-usbio.c | 258 ++++++++++++++++++++++++++++++++++++++
 4 files changed, 271 insertions(+)
 create mode 100644 drivers/gpio/gpio-usbio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 1208efe41f9f..81db1457e9d1 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12522,6 +12522,7 @@ INTEL USBIO USB I/O EXPANDER DRIVERS
 M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
 M:	Hans de Goede <hansg@kernel.org>
 S:	Maintained
+F:	drivers/gpio/gpio-usbio.c
 F:	drivers/usb/misc/usbio.c
 F:	include/linux/usb/usbio.h
 
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 44f922e10db2..f3f7b3b782a3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1436,6 +1436,17 @@ config GPIO_LJCA
 	  This driver can also be built as a module. If so, the module
 	  will be called gpio-ljca.
 
+config GPIO_USBIO
+	tristate "Intel USBIO GPIO support"
+	depends on USB_USBIO
+	default USB_USBIO
+	help
+	  Select this option to enable GPIO driver for the INTEL
+	  USBIO driver stack.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called gpio_usbio.
+
 config GPIO_LP3943
 	tristate "TI/National Semiconductor LP3943 GPIO expander"
 	depends on MFD_LP3943
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 88dedd298256..a2c054ea2d4c 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -90,6 +90,7 @@ obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
 obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
 obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
 obj-$(CONFIG_GPIO_LJCA) 		+= gpio-ljca.o
+obj-$(CONFIG_GPIO_USBIO) 		+= gpio-usbio.o
 obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
 obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
 obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
diff --git a/drivers/gpio/gpio-usbio.c b/drivers/gpio/gpio-usbio.c
new file mode 100644
index 000000000000..08a1219153f4
--- /dev/null
+++ b/drivers/gpio/gpio-usbio.c
@@ -0,0 +1,258 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Intel Corporation.
+ * Copyright (c) 2025 Red Hat, Inc.
+ */
+
+#include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/gpio/driver.h>
+#include <linux/types.h>
+#include <linux/usb/usbio.h>
+
+struct usbio_gpio_bank {
+	u8 config[USBIO_GPIOSPERBANK];
+	u32 bitmap;
+};
+
+struct usbio_gpio {
+	struct usbio_gpio_bank banks[USBIO_MAX_GPIOBANKS];
+	struct gpio_chip gc;
+	struct auxiliary_device *adev;
+};
+
+static const struct acpi_device_id usbio_gpio_acpi_hids[] = {
+	{ "INTC1007" }, /* MTL */
+	{ "INTC10B2" }, /* ARL */
+	{ "INTC10B5" }, /* LNL */
+	{ "INTC10E2" }, /* PTL */
+	{ }
+};
+
+static bool usbio_gpio_get_bank_and_pin(struct gpio_chip *gc, unsigned int offset,
+					struct usbio_gpio_bank **bank_ret, int *pin_ret)
+{
+	struct usbio_gpio *gpio = gpiochip_get_data(gc);
+	struct device *dev = &gpio->adev->dev;
+	struct usbio_gpio_bank *bank;
+	int pin;
+
+	if (offset >= gc->ngpio)
+		return false;
+
+	bank = &gpio->banks[offset / USBIO_GPIOSPERBANK];
+	pin = offset % USBIO_GPIOSPERBANK;
+	if (~bank->bitmap & BIT(pin)) {
+		/* The FW bitmap sometimes is invalid, warn and continue */
+		dev_warn_once(dev, FW_BUG "GPIO %u is not in FW pins bitmap\n", offset);
+	}
+
+	*bank_ret = bank;
+	*pin_ret = pin;
+	return true;
+}
+
+static int usbio_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usbio_gpio_bank *bank;
+	int pin;
+	u8 cfg;
+
+	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
+		return -EINVAL;
+
+	cfg = bank->config[pin] & USBIO_GPIO_PINMOD_MASK;
+	if (cfg == USBIO_GPIO_PINMOD_OUTPUT)
+		return GPIO_LINE_DIRECTION_OUT;
+	else
+		return GPIO_LINE_DIRECTION_IN;
+}
+
+static int usbio_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	struct usbio_gpio *gpio = gpiochip_get_data(gc);
+	struct usbio_gpio_bank *bank;
+	struct usbio_gpio_rw gbuf;
+	int pin, ret;
+
+	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
+		return -EINVAL;
+
+	gbuf.bankid = offset / USBIO_GPIOSPERBANK;
+	gbuf.pincount  = 1;
+	gbuf.pin = pin;
+
+	ret = usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_READ,
+				&gbuf, sizeof(gbuf) - sizeof(gbuf.value),
+				&gbuf, sizeof(gbuf));
+	if (ret != sizeof(gbuf))
+		return (ret < 0) ? ret : -EPROTO;
+
+	return (gbuf.value >> pin) & 1;
+}
+
+static void usbio_gpio_set(struct gpio_chip *gc, unsigned int offset,
+		int value)
+{
+	struct usbio_gpio *gpio = gpiochip_get_data(gc);
+	struct usbio_gpio_bank *bank;
+	struct usbio_gpio_rw gbuf;
+	int pin;
+
+	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
+		return;
+
+	gbuf.bankid = offset / USBIO_GPIOSPERBANK;
+	gbuf.pincount  = 1;
+	gbuf.pin = pin;
+	gbuf.value = value << pin;
+
+	usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_WRITE,
+			  &gbuf, sizeof(gbuf), NULL, 0);
+}
+
+static int usbio_gpio_update_config(struct gpio_chip *gc, unsigned int offset,
+				    u8 mask, u8 value)
+{
+	struct usbio_gpio *gpio = gpiochip_get_data(gc);
+	struct usbio_gpio_bank *bank;
+	struct usbio_gpio_init gbuf;
+	int pin;
+
+	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
+		return -EINVAL;
+
+	bank->config[pin] &= ~mask;
+	bank->config[pin] |= value;
+
+	gbuf.bankid = offset / USBIO_GPIOSPERBANK;
+	gbuf.config = bank->config[pin];
+	gbuf.pincount  = 1;
+	gbuf.pin = pin;
+
+	return usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_INIT,
+				 &gbuf, sizeof(gbuf), NULL, 0);
+}
+
+static int usbio_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
+{
+	return usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINMOD_MASK,
+					USBIO_GPIO_SET_PINMOD(USBIO_GPIO_PINMOD_INPUT));
+}
+
+static int usbio_gpio_direction_output(struct gpio_chip *gc,
+		unsigned int offset, int value)
+{
+	int ret;
+
+	ret = usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINMOD_MASK,
+				       USBIO_GPIO_SET_PINMOD(USBIO_GPIO_PINMOD_OUTPUT));
+	if (ret)
+		return ret;
+
+	usbio_gpio_set(gc, offset, value);
+	return 0;
+}
+
+static int usbio_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
+		unsigned long config)
+{
+	u8 value;
+
+	switch (pinconf_to_config_param(config)) {
+	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
+		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_DEFAULT);
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PULLUP);
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PULLDOWN);
+		break;
+	case PIN_CONFIG_DRIVE_PUSH_PULL:
+		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PUSHPULL);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	return usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINCFG_MASK, value);
+}
+
+static int usbio_gpio_probe(struct auxiliary_device *adev,
+		const struct auxiliary_device_id *adev_id)
+{
+	struct usbio_gpio_bank_desc *bank_desc;
+	struct device *dev = &adev->dev;
+	struct usbio_gpio *gpio;
+	int bank, ret;
+
+	bank_desc = dev_get_platdata(dev);
+	if (!bank_desc)
+		return -EINVAL;
+
+	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
+	if (!gpio)
+		return -ENOMEM;
+
+	gpio->adev = adev;
+
+	usbio_acpi_bind(gpio->adev, usbio_gpio_acpi_hids);
+
+	for (bank = 0; bank < USBIO_MAX_GPIOBANKS && bank_desc[bank].bmap; bank++)
+		gpio->banks[bank].bitmap = bank_desc[bank].bmap;
+
+	gpio->gc.label = ACPI_COMPANION(dev) ?
+					acpi_dev_name(ACPI_COMPANION(dev)) : dev_name(dev);
+	gpio->gc.parent = dev;
+	gpio->gc.owner = THIS_MODULE;
+	gpio->gc.get_direction = usbio_gpio_get_direction;
+	gpio->gc.direction_input = usbio_gpio_direction_input;
+	gpio->gc.direction_output = usbio_gpio_direction_output;
+	gpio->gc.get = usbio_gpio_get;
+	gpio->gc.set = usbio_gpio_set;
+	gpio->gc.set_config = usbio_gpio_set_config;
+	gpio->gc.base = -1;
+	gpio->gc.ngpio = bank * USBIO_GPIOSPERBANK;
+	gpio->gc.can_sleep = true;
+
+	auxiliary_set_drvdata(adev, gpio);
+
+	ret = gpiochip_add_data(&gpio->gc, gpio);
+	if (ret)
+		return ret;
+
+	if (has_acpi_companion(dev))
+		acpi_dev_clear_dependencies(ACPI_COMPANION(dev));
+
+	return 0;
+}
+
+static void usbio_gpio_remove(struct auxiliary_device *adev)
+{
+	struct usbio_gpio *gpio = auxiliary_get_drvdata(adev);
+
+	gpiochip_remove(&gpio->gc);
+}
+
+static const struct auxiliary_device_id usbio_gpio_id_table[] = {
+	{ "usbio.usbio-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(auxiliary, usbio_gpio_id_table);
+
+static struct auxiliary_driver usbio_gpio_driver = {
+	.name = USBIO_GPIO_CLIENT,
+	.probe = usbio_gpio_probe,
+	.remove = usbio_gpio_remove,
+	.id_table = usbio_gpio_id_table
+};
+module_auxiliary_driver(usbio_gpio_driver);
+
+MODULE_DESCRIPTION("Intel USBIO GPIO driver");
+MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("USBIO");
-- 
2.49.0

Re: [PATCH 2/3] gpio: Add Intel USBIO GPIO driver

[Sakari Ailus]

Hi Hans, Israel,

On Sat, Aug 09, 2025 at 12:23:25PM +0200, Hans de Goede wrote:
> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> 
> Add a a driver for the GPIO auxbus child device of the Intel USBIO USB
> IO-expander used by the MIPI cameras on various new (Meteor Lake and
> later) Intel laptops.
> 
> Co-developed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> ---
>  MAINTAINERS               |   1 +
>  drivers/gpio/Kconfig      |  11 ++
>  drivers/gpio/Makefile     |   1 +
>  drivers/gpio/gpio-usbio.c | 258 ++++++++++++++++++++++++++++++++++++++
>  4 files changed, 271 insertions(+)
>  create mode 100644 drivers/gpio/gpio-usbio.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 1208efe41f9f..81db1457e9d1 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12522,6 +12522,7 @@ INTEL USBIO USB I/O EXPANDER DRIVERS
>  M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>  M:	Hans de Goede <hansg@kernel.org>
>  S:	Maintained
> +F:	drivers/gpio/gpio-usbio.c
>  F:	drivers/usb/misc/usbio.c
>  F:	include/linux/usb/usbio.h
>  
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 44f922e10db2..f3f7b3b782a3 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1436,6 +1436,17 @@ config GPIO_LJCA
>  	  This driver can also be built as a module. If so, the module
>  	  will be called gpio-ljca.
>  
> +config GPIO_USBIO
> +	tristate "Intel USBIO GPIO support"
> +	depends on USB_USBIO
> +	default USB_USBIO
> +	help
> +	  Select this option to enable GPIO driver for the INTEL
> +	  USBIO driver stack.
> +
> +	  This driver can also be built as a module. If so, the module
> +	  will be called gpio_usbio.
> +
>  config GPIO_LP3943
>  	tristate "TI/National Semiconductor LP3943 GPIO expander"
>  	depends on MFD_LP3943
> diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
> index 88dedd298256..a2c054ea2d4c 100644
> --- a/drivers/gpio/Makefile
> +++ b/drivers/gpio/Makefile
> @@ -90,6 +90,7 @@ obj-$(CONFIG_GPIO_JANZ_TTL)		+= gpio-janz-ttl.o
>  obj-$(CONFIG_GPIO_KEMPLD)		+= gpio-kempld.o
>  obj-$(CONFIG_GPIO_LATCH)		+= gpio-latch.o
>  obj-$(CONFIG_GPIO_LJCA) 		+= gpio-ljca.o
> +obj-$(CONFIG_GPIO_USBIO) 		+= gpio-usbio.o
>  obj-$(CONFIG_GPIO_LOGICVC)		+= gpio-logicvc.o
>  obj-$(CONFIG_GPIO_LOONGSON1)		+= gpio-loongson1.o
>  obj-$(CONFIG_GPIO_LOONGSON)		+= gpio-loongson.o
> diff --git a/drivers/gpio/gpio-usbio.c b/drivers/gpio/gpio-usbio.c
> new file mode 100644
> index 000000000000..08a1219153f4
> --- /dev/null
> +++ b/drivers/gpio/gpio-usbio.c
> @@ -0,0 +1,258 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Intel Corporation.
> + * Copyright (c) 2025 Red Hat, Inc.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/auxiliary_bus.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/gpio/driver.h>
> +#include <linux/types.h>
> +#include <linux/usb/usbio.h>
> +
> +struct usbio_gpio_bank {
> +	u8 config[USBIO_GPIOSPERBANK];
> +	u32 bitmap;
> +};
> +
> +struct usbio_gpio {
> +	struct usbio_gpio_bank banks[USBIO_MAX_GPIOBANKS];
> +	struct gpio_chip gc;
> +	struct auxiliary_device *adev;
> +};
> +
> +static const struct acpi_device_id usbio_gpio_acpi_hids[] = {
> +	{ "INTC1007" }, /* MTL */
> +	{ "INTC10B2" }, /* ARL */
> +	{ "INTC10B5" }, /* LNL */
> +	{ "INTC10E2" }, /* PTL */
> +	{ }
> +};
> +
> +static bool usbio_gpio_get_bank_and_pin(struct gpio_chip *gc, unsigned int offset,
> +					struct usbio_gpio_bank **bank_ret, int *pin_ret)
> +{
> +	struct usbio_gpio *gpio = gpiochip_get_data(gc);
> +	struct device *dev = &gpio->adev->dev;
> +	struct usbio_gpio_bank *bank;
> +	int pin;

unsigned int pin and for pin_ret?

> +
> +	if (offset >= gc->ngpio)
> +		return false;
> +
> +	bank = &gpio->banks[offset / USBIO_GPIOSPERBANK];
> +	pin = offset % USBIO_GPIOSPERBANK;
> +	if (~bank->bitmap & BIT(pin)) {
> +		/* The FW bitmap sometimes is invalid, warn and continue */
> +		dev_warn_once(dev, FW_BUG "GPIO %u is not in FW pins bitmap\n", offset);
> +	}
> +
> +	*bank_ret = bank;
> +	*pin_ret = pin;
> +	return true;
> +}
> +
> +static int usbio_gpio_get_direction(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct usbio_gpio_bank *bank;
> +	int pin;
> +	u8 cfg;
> +
> +	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> +		return -EINVAL;
> +
> +	cfg = bank->config[pin] & USBIO_GPIO_PINMOD_MASK;
> +	if (cfg == USBIO_GPIO_PINMOD_OUTPUT)
> +		return GPIO_LINE_DIRECTION_OUT;
> +	else
> +		return GPIO_LINE_DIRECTION_IN;

The else branch is not useful. But I think you could do instead:

	return cfg == USBIO_GPIO_PINMOD_OUTPUT ?
		GPIO_LINE_DIRECTION_OUT : GPIO_LINE_DIRECTION_IN;

> +}
> +
> +static int usbio_gpio_get(struct gpio_chip *gc, unsigned int offset)
> +{
> +	struct usbio_gpio *gpio = gpiochip_get_data(gc);
> +	struct usbio_gpio_bank *bank;
> +	struct usbio_gpio_rw gbuf;
> +	int pin, ret;
> +
> +	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> +		return -EINVAL;
> +
> +	gbuf.bankid = offset / USBIO_GPIOSPERBANK;
> +	gbuf.pincount  = 1;
> +	gbuf.pin = pin;
> +
> +	ret = usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_READ,
> +				&gbuf, sizeof(gbuf) - sizeof(gbuf.value),
> +				&gbuf, sizeof(gbuf));
> +	if (ret != sizeof(gbuf))
> +		return (ret < 0) ? ret : -EPROTO;

Unneeded parentheses.

> +
> +	return (gbuf.value >> pin) & 1;
> +}
> +
> +static void usbio_gpio_set(struct gpio_chip *gc, unsigned int offset,
> +		int value)
> +{
> +	struct usbio_gpio *gpio = gpiochip_get_data(gc);
> +	struct usbio_gpio_bank *bank;
> +	struct usbio_gpio_rw gbuf;
> +	int pin;
> +
> +	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> +		return;
> +
> +	gbuf.bankid = offset / USBIO_GPIOSPERBANK;
> +	gbuf.pincount  = 1;
> +	gbuf.pin = pin;
> +	gbuf.value = value << pin;
> +
> +	usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_WRITE,
> +			  &gbuf, sizeof(gbuf), NULL, 0);
> +}
> +
> +static int usbio_gpio_update_config(struct gpio_chip *gc, unsigned int offset,
> +				    u8 mask, u8 value)
> +{
> +	struct usbio_gpio *gpio = gpiochip_get_data(gc);
> +	struct usbio_gpio_bank *bank;
> +	struct usbio_gpio_init gbuf;
> +	int pin;
> +
> +	if (!usbio_gpio_get_bank_and_pin(gc, offset, &bank, &pin))
> +		return -EINVAL;
> +
> +	bank->config[pin] &= ~mask;
> +	bank->config[pin] |= value;
> +

You could declare gbuf here and assign the fields in initialisation. There
are no additional, uninitialised fields in the struct but still I'd do
that.

> +	gbuf.bankid = offset / USBIO_GPIOSPERBANK;
> +	gbuf.config = bank->config[pin];
> +	gbuf.pincount  = 1;
> +	gbuf.pin = pin;
> +
> +	return usbio_control_msg(gpio->adev, USBIO_PKTTYPE_GPIO, USBIO_GPIOCMD_INIT,
> +				 &gbuf, sizeof(gbuf), NULL, 0);
> +}
> +
> +static int usbio_gpio_direction_input(struct gpio_chip *gc, unsigned int offset)
> +{
> +	return usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINMOD_MASK,
> +					USBIO_GPIO_SET_PINMOD(USBIO_GPIO_PINMOD_INPUT));
> +}
> +
> +static int usbio_gpio_direction_output(struct gpio_chip *gc,
> +		unsigned int offset, int value)
> +{
> +	int ret;
> +
> +	ret = usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINMOD_MASK,
> +				       USBIO_GPIO_SET_PINMOD(USBIO_GPIO_PINMOD_OUTPUT));
> +	if (ret)
> +		return ret;
> +
> +	usbio_gpio_set(gc, offset, value);

Please check the return value (after switching to set_rv(); see below).

> +	return 0;
> +}
> +
> +static int usbio_gpio_set_config(struct gpio_chip *gc, unsigned int offset,
> +		unsigned long config)
> +{
> +	u8 value;
> +
> +	switch (pinconf_to_config_param(config)) {
> +	case PIN_CONFIG_BIAS_PULL_PIN_DEFAULT:
> +		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_DEFAULT);
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_UP:
> +		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PULLUP);
> +		break;
> +	case PIN_CONFIG_BIAS_PULL_DOWN:
> +		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PULLDOWN);
> +		break;
> +	case PIN_CONFIG_DRIVE_PUSH_PULL:
> +		value = USBIO_GPIO_SET_PINCFG(USBIO_GPIO_PINCFG_PUSHPULL);
> +		break;
> +	default:
> +		return -ENOTSUPP;
> +	}
> +
> +	return usbio_gpio_update_config(gc, offset, USBIO_GPIO_PINCFG_MASK, value);
> +}
> +
> +static int usbio_gpio_probe(struct auxiliary_device *adev,
> +		const struct auxiliary_device_id *adev_id)
> +{
> +	struct usbio_gpio_bank_desc *bank_desc;
> +	struct device *dev = &adev->dev;
> +	struct usbio_gpio *gpio;
> +	int bank, ret;
> +
> +	bank_desc = dev_get_platdata(dev);
> +	if (!bank_desc)
> +		return -EINVAL;
> +
> +	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> +	if (!gpio)
> +		return -ENOMEM;

All devm-allocated memory is released once the device is gone but you may
still have users. Could you release this in the adev's release callback
instead?

> +
> +	gpio->adev = adev;
> +
> +	usbio_acpi_bind(gpio->adev, usbio_gpio_acpi_hids);
> +
> +	for (bank = 0; bank < USBIO_MAX_GPIOBANKS && bank_desc[bank].bmap; bank++)
> +		gpio->banks[bank].bitmap = bank_desc[bank].bmap;
> +
> +	gpio->gc.label = ACPI_COMPANION(dev) ?
> +					acpi_dev_name(ACPI_COMPANION(dev)) : dev_name(dev);
> +	gpio->gc.parent = dev;
> +	gpio->gc.owner = THIS_MODULE;
> +	gpio->gc.get_direction = usbio_gpio_get_direction;
> +	gpio->gc.direction_input = usbio_gpio_direction_input;
> +	gpio->gc.direction_output = usbio_gpio_direction_output;
> +	gpio->gc.get = usbio_gpio_get;
> +	gpio->gc.set = usbio_gpio_set;

Please use set_rv() callback; it can return an error. The set callback is
also deprecated nowadays.

> +	gpio->gc.set_config = usbio_gpio_set_config;
> +	gpio->gc.base = -1;
> +	gpio->gc.ngpio = bank * USBIO_GPIOSPERBANK;
> +	gpio->gc.can_sleep = true;
> +
> +	auxiliary_set_drvdata(adev, gpio);
> +
> +	ret = gpiochip_add_data(&gpio->gc, gpio);
> +	if (ret)
> +		return ret;
> +
> +	if (has_acpi_companion(dev))
> +		acpi_dev_clear_dependencies(ACPI_COMPANION(dev));
> +
> +	return 0;
> +}
> +
> +static void usbio_gpio_remove(struct auxiliary_device *adev)
> +{
> +	struct usbio_gpio *gpio = auxiliary_get_drvdata(adev);
> +
> +	gpiochip_remove(&gpio->gc);
> +}
> +
> +static const struct auxiliary_device_id usbio_gpio_id_table[] = {
> +	{ "usbio.usbio-gpio" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, usbio_gpio_id_table);
> +
> +static struct auxiliary_driver usbio_gpio_driver = {
> +	.name = USBIO_GPIO_CLIENT,
> +	.probe = usbio_gpio_probe,
> +	.remove = usbio_gpio_remove,
> +	.id_table = usbio_gpio_id_table
> +};
> +module_auxiliary_driver(usbio_gpio_driver);
> +
> +MODULE_DESCRIPTION("Intel USBIO GPIO driver");
> +MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("USBIO");

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 2/3] gpio: Add Intel USBIO GPIO driver

[Hans de Goede]

Hi Sakari,

Thank you for your review, note this
is not a full reply.

On 11-Aug-25 9:07 AM, Sakari Ailus wrote:

...

>> +static int usbio_gpio_probe(struct auxiliary_device *adev,
>> +		const struct auxiliary_device_id *adev_id)
>> +{
>> +	struct usbio_gpio_bank_desc *bank_desc;
>> +	struct device *dev = &adev->dev;
>> +	struct usbio_gpio *gpio;
>> +	int bank, ret;
>> +
>> +	bank_desc = dev_get_platdata(dev);
>> +	if (!bank_desc)
>> +		return -EINVAL;
>> +
>> +	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
>> +	if (!gpio)
>> +		return -ENOMEM;
> 
> All devm-allocated memory is released once the device is gone but you may
> still have users. Could you release this in the adev's release callback
> instead?

All GPIO users go through gpio_descriptors which are seperately
refcounted. The GPIO core can handle GPIO controllers being
hot-unplugged since the end of 2023 and guarantees that no
calls will be made to the gpiochip driver after calling
gpiochip_remove().

Regards,

Hans

Re: [PATCH 2/3] gpio: Add Intel USBIO GPIO driver

[Sakari Ailus]

Hi Hans,

On Mon, Aug 11, 2025 at 11:23:50AM +0200, Hans de Goede wrote:
> Hi Sakari,
> 
> Thank you for your review, note this
> is not a full reply.
> 
> On 11-Aug-25 9:07 AM, Sakari Ailus wrote:
> 
> ...
> 
> >> +static int usbio_gpio_probe(struct auxiliary_device *adev,
> >> +		const struct auxiliary_device_id *adev_id)
> >> +{
> >> +	struct usbio_gpio_bank_desc *bank_desc;
> >> +	struct device *dev = &adev->dev;
> >> +	struct usbio_gpio *gpio;
> >> +	int bank, ret;
> >> +
> >> +	bank_desc = dev_get_platdata(dev);
> >> +	if (!bank_desc)
> >> +		return -EINVAL;
> >> +
> >> +	gpio = devm_kzalloc(dev, sizeof(*gpio), GFP_KERNEL);
> >> +	if (!gpio)
> >> +		return -ENOMEM;
> > 
> > All devm-allocated memory is released once the device is gone but you may
> > still have users. Could you release this in the adev's release callback
> > instead?
> 
> All GPIO users go through gpio_descriptors which are seperately
> refcounted. The GPIO core can handle GPIO controllers being
> hot-unplugged since the end of 2023 and guarantees that no
> calls will be made to the gpiochip driver after calling
> gpiochip_remove().

Thanks, I wasn't aware of that. This simplifies the client driver
implementation nicely.

-- 
Regards,

Sakari Ailus

[PATCH 3/3] i2c: Add Intel USBIO I2C driver

[Hans de Goede]

From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>

Add a a driver for the I2C auxbus child device of the Intel USBIO USB
IO-expander used by the MIPI cameras on various new (Meteor Lake and
later) Intel laptops.

Co-developed-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Hans de Goede <hansg@kernel.org>
Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
---
 MAINTAINERS                    |   1 +
 drivers/i2c/busses/Kconfig     |  11 ++
 drivers/i2c/busses/Makefile    |   1 +
 drivers/i2c/busses/i2c-usbio.c | 344 +++++++++++++++++++++++++++++++++
 4 files changed, 357 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-usbio.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 81db1457e9d1..9e8875e3dabf 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12523,6 +12523,7 @@ M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
 M:	Hans de Goede <hansg@kernel.org>
 S:	Maintained
 F:	drivers/gpio/gpio-usbio.c
+F:	drivers/i2c/busses/i2c-usbio.c
 F:	drivers/usb/misc/usbio.c
 F:	include/linux/usb/usbio.h
 
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index c8d115b58e44..bf6a624f5ff6 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1358,6 +1358,17 @@ config I2C_LJCA
 	  This driver can also be built as a module.  If so, the module
 	  will be called i2c-ljca.
 
+config I2C_USBIO
+	tristate "Intel USBIO I2C Adapter support"
+	depends on USB_USBIO
+	default USB_USBIO
+	help
+	  Select this option to enable I2C driver for the INTEL
+	  USBIO driver stack.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c_usbio.
+
 config I2C_CP2615
 	tristate "Silicon Labs CP2615 USB sound card and I2C adapter"
 	depends on USB
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 04db855fdfd6..401a79c9767e 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -135,6 +135,7 @@ obj-$(CONFIG_I2C_GXP)		+= i2c-gxp.o
 obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
 obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
 obj-$(CONFIG_I2C_LJCA)		+= i2c-ljca.o
+obj-$(CONFIG_I2C_USBIO)		+= i2c-usbio.o
 obj-$(CONFIG_I2C_CP2615) += i2c-cp2615.o
 obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
 obj-$(CONFIG_I2C_PCI1XXXX)	+= i2c-mchp-pci1xxxx.o
diff --git a/drivers/i2c/busses/i2c-usbio.c b/drivers/i2c/busses/i2c-usbio.c
new file mode 100644
index 000000000000..82c4769852f8
--- /dev/null
+++ b/drivers/i2c/busses/i2c-usbio.c
@@ -0,0 +1,344 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2025 Intel Corporation.
+ * Copyright (c) 2025 Red Hat, Inc.
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/dev_printk.h>
+#include <linux/device.h>
+#include <linux/i2c.h>
+#include <linux/types.h>
+#include <linux/usb/usbio.h>
+
+#define I2C_RW_OVERHEAD (sizeof(struct usbio_bulk_packet) + sizeof(struct usbio_i2c_rw))
+
+struct usbio_i2c {
+	u32 speed;
+	struct i2c_adapter adap;
+	struct auxiliary_device *adev;
+	struct usbio_i2c_rw *rwbuf;
+	bool init_supports_ack_flag;
+	u16 txbuf_len;
+	u16 rxbuf_len;
+};
+
+static const struct acpi_device_id usbio_i2c_acpi_hids[] = {
+	{ "INTC1008" }, /* MTL */
+	{ "INTC10B3" }, /* ARL */
+	{ "INTC10B6" }, /* LNL */
+	{ "INTC10E3" }, /* PTL */
+	{ }
+};
+
+static const u32 usbio_i2c_speeds[] = {
+	I2C_MAX_STANDARD_MODE_FREQ,
+	I2C_MAX_FAST_MODE_FREQ,
+	I2C_MAX_FAST_MODE_PLUS_FREQ,
+	I2C_MAX_HIGH_SPEED_MODE_FREQ
+};
+
+static void usbio_i2c_uninit(struct i2c_adapter *adap, struct i2c_msg *msg)
+{
+	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
+	struct usbio_i2c_uninit ubuf;
+
+	ubuf.busid = i2c->adev->id;
+	ubuf.config = msg->addr;
+
+	usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_UNINIT, true,
+		       &ubuf, sizeof(ubuf), NULL, 0);
+}
+
+static int usbio_i2c_init(struct i2c_adapter *adap, struct i2c_msg *msg)
+{
+	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
+	struct usbio_i2c_init ibuf;
+	void *reply_buf;
+	u16 reply_len;
+	int ret;
+
+	ibuf.busid = i2c->adev->id;
+	ibuf.config = msg->addr;
+	ibuf.speed = i2c->speed;
+
+	if (i2c->init_supports_ack_flag) {
+		reply_buf = &ibuf;
+		reply_len = sizeof(ibuf);
+	} else {
+		reply_buf = NULL;
+		reply_len = 0;
+	}
+	ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_INIT, true,
+			     &ibuf, sizeof(ibuf), reply_buf, reply_len);
+	if (ret != sizeof(ibuf))
+		return (ret < 0) ? ret : -EIO;
+
+	return 0;
+}
+
+static int usbio_i2c_read(struct i2c_adapter *adap, struct i2c_msg *msg)
+{
+	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
+	u16 rxchunk = i2c->rxbuf_len - I2C_RW_OVERHEAD;
+	struct usbio_i2c_rw *rbuf = i2c->rwbuf;
+	int ret;
+
+	rbuf->busid = i2c->adev->id;
+	rbuf->config = msg->addr;
+	rbuf->size = msg->len;
+
+	if (msg->len > rxchunk) {
+		/* Need to split the input buffer */
+		u16 len = 0;
+
+		do {
+			if (msg->len - len < rxchunk)
+				rxchunk = msg->len - len;
+
+			ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C,
+					     USBIO_I2CCMD_READ, true,
+					     rbuf, len == 0 ? sizeof(*rbuf) : 0,
+					     rbuf, sizeof(*rbuf) + rxchunk);
+			if (ret < 0)
+				return ret;
+
+			memcpy(&msg->buf[len], rbuf->data, rxchunk);
+			len += rxchunk;
+		} while (msg->len > len);
+
+		goto out_log;
+	}
+
+	ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_READ, true,
+			     rbuf, sizeof(*rbuf), rbuf, sizeof(*rbuf) + msg->len);
+	if (ret != sizeof(*rbuf) + msg->len)
+		return (ret < 0) ? ret : -EIO;
+
+	memcpy(msg->buf, rbuf->data, msg->len);
+out_log:
+	dev_dbg(adap->dev.parent, "RD[%d]:%*phN\n", msg->len, msg->len, msg->buf);
+	return 0;
+}
+
+static int usbio_i2c_write(struct i2c_adapter *adap, struct i2c_msg *msg)
+{
+	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
+	u16 txchunk = i2c->txbuf_len - I2C_RW_OVERHEAD;
+	struct usbio_i2c_rw *wbuf = i2c->rwbuf;
+	int ret;
+
+	dev_dbg(adap->dev.parent, "WR[%d]:%*phN\n", msg->len, msg->len, msg->buf);
+
+	if (msg->len > txchunk) {
+		/* Need to split the output buffer */
+		u16 len = 0;
+
+		do {
+			wbuf->busid = i2c->adev->id;
+			wbuf->config = msg->addr;
+			wbuf->size = msg->len;
+
+			memcpy(wbuf->data, &msg->buf[len], txchunk);
+			len += txchunk;
+
+			ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C,
+					     USBIO_I2CCMD_WRITE, msg->len == len,
+					     wbuf, sizeof(*wbuf) + txchunk,
+					     wbuf, sizeof(*wbuf));
+			if (ret < 0)
+				return ret;
+
+			if (msg->len - len < txchunk)
+				txchunk = msg->len - len;
+		} while (msg->len > len);
+
+		return 0;
+	}
+
+	wbuf->busid = i2c->adev->id;
+	wbuf->config = msg->addr;
+	wbuf->size = msg->len;
+	memcpy(wbuf->data, msg->buf, msg->len);
+
+	ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_WRITE, true,
+			     wbuf, sizeof(*wbuf) + msg->len, wbuf, sizeof(*wbuf));
+	if (ret != sizeof(*wbuf) || wbuf->size != msg->len)
+		return (ret < 0) ? ret : -EIO;
+
+	return 0;
+}
+
+static int usbio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
+{
+	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
+	int ret;
+
+	usbio_acquire(i2c->adev);
+
+	ret = usbio_i2c_init(adap, msgs);
+	if (ret)
+		goto out_release;
+
+	for (int i = 0; i < num; ret = ++i) {
+		if (msgs[i].flags & I2C_M_RD)
+			ret = usbio_i2c_read(adap, &msgs[i]);
+		else
+			ret = usbio_i2c_write(adap, &msgs[i]);
+
+		if (ret)
+			break;
+	}
+
+	usbio_i2c_uninit(adap, msgs);
+
+out_release:
+	usbio_release(i2c->adev);
+	return ret;
+}
+
+static u32 usbio_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
+}
+
+static const struct i2c_adapter_quirks usbio_i2c_quirks = {
+	.flags = I2C_AQ_NO_REP_START,
+	.max_read_len = SZ_4K,
+	.max_write_len = SZ_4K,
+};
+
+/* Use smaller max_*_len settings for chips which do not support split bulk transfers */
+static const struct i2c_adapter_quirks usbio_i2c_no_split_transfers_quirks = {
+	.flags = I2C_AQ_NO_REP_START,
+	.max_read_len = 63 - I2C_RW_OVERHEAD,
+	.max_write_len = 63 -  I2C_RW_OVERHEAD,
+};
+
+static const struct i2c_algorithm usbio_i2c_algo = {
+	.master_xfer = usbio_i2c_xfer,
+	.functionality = usbio_i2c_func,
+};
+
+static int usbio_i2c_probe(struct auxiliary_device *adev,
+		const struct auxiliary_device_id *adev_id)
+{
+	struct usbio_i2c_bus_desc *i2c_desc;
+	struct device *dev = &adev->dev;
+	u8 dummy_read_buf;
+	struct i2c_msg dummy_read = {
+		.addr = 0x08,
+		.flags = I2C_M_RD,
+		.len = 1,
+		.buf = &dummy_read_buf,
+	};
+	struct usbio_i2c *i2c;
+	u32 max_speed;
+	int ret;
+
+	i2c_desc = dev_get_platdata(dev);
+	if (!i2c_desc)
+		return -EINVAL;
+
+	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
+	if (!i2c_desc->caps)
+		max_speed = I2C_MAX_FAST_MODE_FREQ;
+	else
+		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
+
+	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return -ENOMEM;
+
+	i2c->adev = adev;
+
+	usbio_acpi_bind(i2c->adev, usbio_i2c_acpi_hids);
+	usbio_get_txrxbuf_len(i2c->adev, &i2c->txbuf_len, &i2c->rxbuf_len);
+
+	i2c->rwbuf = devm_kzalloc(dev, max(i2c->txbuf_len, i2c->rxbuf_len), GFP_KERNEL);
+	if (!i2c->rwbuf)
+		return -ENOMEM;
+
+	i2c->speed = i2c_acpi_find_bus_speed(dev);
+	if (!i2c->speed)
+		i2c->speed = I2C_MAX_STANDARD_MODE_FREQ;
+	else if (i2c->speed > max_speed) {
+		dev_warn(dev, "Invalid speed %u adjusting to bus max %u\n",
+			 i2c->speed, max_speed);
+		i2c->speed = max_speed;
+	}
+
+	i2c->adap.owner = THIS_MODULE;
+	i2c->adap.class = I2C_CLASS_HWMON;
+	i2c->adap.dev.parent = dev;
+	i2c->adap.algo = &usbio_i2c_algo;
+	i2c->adap.quirks = &usbio_i2c_quirks;
+
+	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "%s.%d",
+		 USBIO_I2C_CLIENT, i2c->adev->id);
+
+	device_set_node(&i2c->adap.dev, dev_fwnode(&adev->dev));
+
+	auxiliary_set_drvdata(adev, i2c);
+	i2c_set_adapdata(&i2c->adap, i2c);
+
+	/*
+	 * Test if USBIO_I2CCMD_INIT commands with the USBIO_PKTFLAG_ACK flag
+	 * are supported.
+	 */
+	usbio_acquire(i2c->adev);
+	i2c->init_supports_ack_flag = true;
+	ret = usbio_i2c_init(&i2c->adap, &dummy_read);
+	if (ret) {
+		if (ret != -EPIPE) {
+			usbio_release(i2c->adev);
+			return ret; /* Unexpected error */
+		}
+
+		dev_info(dev, "i2c-init command does not support ack flag\n");
+		i2c->init_supports_ack_flag = false;
+		/* Chips with this quirk also do not support split bulk transfers */
+		i2c->adap.quirks = &usbio_i2c_no_split_transfers_quirks;
+	} else {
+		/* Continue dummy read to not confuse the USBIO chip */
+		usbio_i2c_read(&i2c->adap, &dummy_read);
+	}
+	usbio_i2c_uninit(&i2c->adap, &dummy_read);
+	usbio_release(i2c->adev);
+
+	ret = i2c_add_adapter(&i2c->adap);
+	if (ret)
+		return ret;
+
+	if (has_acpi_companion(&i2c->adap.dev))
+		acpi_dev_clear_dependencies(ACPI_COMPANION(&i2c->adap.dev));
+
+	return 0;
+}
+
+static void usbio_i2c_remove(struct auxiliary_device *adev)
+{
+	struct usbio_i2c *i2c = auxiliary_get_drvdata(adev);
+
+	i2c_del_adapter(&i2c->adap);
+}
+
+static const struct auxiliary_device_id usbio_i2c_id_table[] = {
+	{ "usbio.usbio-i2c" },
+	{ }
+};
+MODULE_DEVICE_TABLE(auxiliary, usbio_i2c_id_table);
+
+static struct auxiliary_driver usbio_i2c_driver = {
+	.name = USBIO_I2C_CLIENT,
+	.probe = usbio_i2c_probe,
+	.remove = usbio_i2c_remove,
+	.id_table = usbio_i2c_id_table
+};
+module_auxiliary_driver(usbio_i2c_driver);
+
+MODULE_DESCRIPTION("Intel USBIO I2C driver");
+MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
+MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS("USBIO");
-- 
2.49.0

Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver

[Sakari Ailus]

Hi Hans, Israel,

My comments on newlines and parentheses apply to this one, too; I'm not
making new ones in similar locations on that subject anymore for this
patch.

On Sat, Aug 09, 2025 at 12:23:26PM +0200, Hans de Goede wrote:
> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> 
> Add a a driver for the I2C auxbus child device of the Intel USBIO USB
> IO-expander used by the MIPI cameras on various new (Meteor Lake and
> later) Intel laptops.
> 
> Co-developed-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Hans de Goede <hansg@kernel.org>
> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
> ---
>  MAINTAINERS                    |   1 +
>  drivers/i2c/busses/Kconfig     |  11 ++
>  drivers/i2c/busses/Makefile    |   1 +
>  drivers/i2c/busses/i2c-usbio.c | 344 +++++++++++++++++++++++++++++++++
>  4 files changed, 357 insertions(+)
>  create mode 100644 drivers/i2c/busses/i2c-usbio.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 81db1457e9d1..9e8875e3dabf 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -12523,6 +12523,7 @@ M:	Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>  M:	Hans de Goede <hansg@kernel.org>
>  S:	Maintained
>  F:	drivers/gpio/gpio-usbio.c
> +F:	drivers/i2c/busses/i2c-usbio.c
>  F:	drivers/usb/misc/usbio.c
>  F:	include/linux/usb/usbio.h
>  
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index c8d115b58e44..bf6a624f5ff6 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1358,6 +1358,17 @@ config I2C_LJCA
>  	  This driver can also be built as a module.  If so, the module
>  	  will be called i2c-ljca.
>  
> +config I2C_USBIO
> +	tristate "Intel USBIO I2C Adapter support"
> +	depends on USB_USBIO
> +	default USB_USBIO
> +	help
> +	  Select this option to enable I2C driver for the INTEL
> +	  USBIO driver stack.
> +
> +	  This driver can also be built as a module.  If so, the module
> +	  will be called i2c_usbio.
> +
>  config I2C_CP2615
>  	tristate "Silicon Labs CP2615 USB sound card and I2C adapter"
>  	depends on USB
> diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
> index 04db855fdfd6..401a79c9767e 100644
> --- a/drivers/i2c/busses/Makefile
> +++ b/drivers/i2c/busses/Makefile
> @@ -135,6 +135,7 @@ obj-$(CONFIG_I2C_GXP)		+= i2c-gxp.o
>  obj-$(CONFIG_I2C_DIOLAN_U2C)	+= i2c-diolan-u2c.o
>  obj-$(CONFIG_I2C_DLN2)		+= i2c-dln2.o
>  obj-$(CONFIG_I2C_LJCA)		+= i2c-ljca.o
> +obj-$(CONFIG_I2C_USBIO)		+= i2c-usbio.o
>  obj-$(CONFIG_I2C_CP2615) += i2c-cp2615.o
>  obj-$(CONFIG_I2C_PARPORT)	+= i2c-parport.o
>  obj-$(CONFIG_I2C_PCI1XXXX)	+= i2c-mchp-pci1xxxx.o
> diff --git a/drivers/i2c/busses/i2c-usbio.c b/drivers/i2c/busses/i2c-usbio.c
> new file mode 100644
> index 000000000000..82c4769852f8
> --- /dev/null
> +++ b/drivers/i2c/busses/i2c-usbio.c
> @@ -0,0 +1,344 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2025 Intel Corporation.
> + * Copyright (c) 2025 Red Hat, Inc.
> + */
> +
> +#include <linux/auxiliary_bus.h>
> +#include <linux/dev_printk.h>
> +#include <linux/device.h>
> +#include <linux/i2c.h>
> +#include <linux/types.h>
> +#include <linux/usb/usbio.h>
> +
> +#define I2C_RW_OVERHEAD (sizeof(struct usbio_bulk_packet) + sizeof(struct usbio_i2c_rw))
> +
> +struct usbio_i2c {
> +	u32 speed;

You could declare this with the smaller fields for better struct packing.

> +	struct i2c_adapter adap;
> +	struct auxiliary_device *adev;
> +	struct usbio_i2c_rw *rwbuf;
> +	bool init_supports_ack_flag;
> +	u16 txbuf_len;
> +	u16 rxbuf_len;
> +};
> +
> +static const struct acpi_device_id usbio_i2c_acpi_hids[] = {
> +	{ "INTC1008" }, /* MTL */
> +	{ "INTC10B3" }, /* ARL */
> +	{ "INTC10B6" }, /* LNL */
> +	{ "INTC10E3" }, /* PTL */
> +	{ }
> +};
> +
> +static const u32 usbio_i2c_speeds[] = {
> +	I2C_MAX_STANDARD_MODE_FREQ,
> +	I2C_MAX_FAST_MODE_FREQ,
> +	I2C_MAX_FAST_MODE_PLUS_FREQ,
> +	I2C_MAX_HIGH_SPEED_MODE_FREQ
> +};
> +
> +static void usbio_i2c_uninit(struct i2c_adapter *adap, struct i2c_msg *msg)
> +{
> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
> +	struct usbio_i2c_uninit ubuf;
> +
> +	ubuf.busid = i2c->adev->id;
> +	ubuf.config = msg->addr;

You can initialise this in declaration.

> +
> +	usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_UNINIT, true,
> +		       &ubuf, sizeof(ubuf), NULL, 0);
> +}
> +
> +static int usbio_i2c_init(struct i2c_adapter *adap, struct i2c_msg *msg)
> +{
> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
> +	struct usbio_i2c_init ibuf;
> +	void *reply_buf;
> +	u16 reply_len;
> +	int ret;
> +
> +	ibuf.busid = i2c->adev->id;
> +	ibuf.config = msg->addr;
> +	ibuf.speed = i2c->speed;

Ditto.

> +
> +	if (i2c->init_supports_ack_flag) {
> +		reply_buf = &ibuf;
> +		reply_len = sizeof(ibuf);
> +	} else {
> +		reply_buf = NULL;
> +		reply_len = 0;
> +	}
> +	ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_INIT, true,
> +			     &ibuf, sizeof(ibuf), reply_buf, reply_len);
> +	if (ret != sizeof(ibuf))
> +		return (ret < 0) ? ret : -EIO;
> +
> +	return 0;
> +}
> +
> +static int usbio_i2c_read(struct i2c_adapter *adap, struct i2c_msg *msg)
> +{
> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
> +	u16 rxchunk = i2c->rxbuf_len - I2C_RW_OVERHEAD;
> +	struct usbio_i2c_rw *rbuf = i2c->rwbuf;
> +	int ret;
> +
> +	rbuf->busid = i2c->adev->id;
> +	rbuf->config = msg->addr;
> +	rbuf->size = msg->len;
> +
> +	if (msg->len > rxchunk) {
> +		/* Need to split the input buffer */
> +		u16 len = 0;
> +
> +		do {
> +			if (msg->len - len < rxchunk)
> +				rxchunk = msg->len - len;
> +
> +			ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C,
> +					     USBIO_I2CCMD_READ, true,
> +					     rbuf, len == 0 ? sizeof(*rbuf) : 0,
> +					     rbuf, sizeof(*rbuf) + rxchunk);
> +			if (ret < 0)
> +				return ret;
> +
> +			memcpy(&msg->buf[len], rbuf->data, rxchunk);
> +			len += rxchunk;
> +		} while (msg->len > len);
> +
> +		goto out_log;
> +	}
> +
> +	ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_READ, true,
> +			     rbuf, sizeof(*rbuf), rbuf, sizeof(*rbuf) + msg->len);
> +	if (ret != sizeof(*rbuf) + msg->len)
> +		return (ret < 0) ? ret : -EIO;
> +
> +	memcpy(msg->buf, rbuf->data, msg->len);

An extra newline here?

> +out_log:
> +	dev_dbg(adap->dev.parent, "RD[%d]:%*phN\n", msg->len, msg->len, msg->buf);
> +	return 0;
> +}
> +
> +static int usbio_i2c_write(struct i2c_adapter *adap, struct i2c_msg *msg)
> +{
> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
> +	u16 txchunk = i2c->txbuf_len - I2C_RW_OVERHEAD;
> +	struct usbio_i2c_rw *wbuf = i2c->rwbuf;
> +	int ret;
> +
> +	dev_dbg(adap->dev.parent, "WR[%d]:%*phN\n", msg->len, msg->len, msg->buf);
> +
> +	if (msg->len > txchunk) {
> +		/* Need to split the output buffer */
> +		u16 len = 0;
> +
> +		do {
> +			wbuf->busid = i2c->adev->id;
> +			wbuf->config = msg->addr;
> +			wbuf->size = msg->len;
> +
> +			memcpy(wbuf->data, &msg->buf[len], txchunk);
> +			len += txchunk;
> +
> +			ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C,
> +					     USBIO_I2CCMD_WRITE, msg->len == len,
> +					     wbuf, sizeof(*wbuf) + txchunk,
> +					     wbuf, sizeof(*wbuf));
> +			if (ret < 0)
> +				return ret;
> +
> +			if (msg->len - len < txchunk)
> +				txchunk = msg->len - len;
> +		} while (msg->len > len);
> +
> +		return 0;
> +	}
> +
> +	wbuf->busid = i2c->adev->id;
> +	wbuf->config = msg->addr;
> +	wbuf->size = msg->len;
> +	memcpy(wbuf->data, msg->buf, msg->len);
> +
> +	ret = usbio_bulk_msg(i2c->adev, USBIO_PKTTYPE_I2C, USBIO_I2CCMD_WRITE, true,
> +			     wbuf, sizeof(*wbuf) + msg->len, wbuf, sizeof(*wbuf));
> +	if (ret != sizeof(*wbuf) || wbuf->size != msg->len)
> +		return (ret < 0) ? ret : -EIO;
> +
> +	return 0;
> +}
> +
> +static int usbio_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int num)
> +{
> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
> +	int ret;
> +
> +	usbio_acquire(i2c->adev);
> +
> +	ret = usbio_i2c_init(adap, msgs);
> +	if (ret)
> +		goto out_release;
> +
> +	for (int i = 0; i < num; ret = ++i) {
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = usbio_i2c_read(adap, &msgs[i]);
> +		else
> +			ret = usbio_i2c_write(adap, &msgs[i]);
> +
> +		if (ret)
> +			break;
> +	}
> +
> +	usbio_i2c_uninit(adap, msgs);
> +
> +out_release:
> +	usbio_release(i2c->adev);
> +	return ret;
> +}
> +
> +static u32 usbio_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static const struct i2c_adapter_quirks usbio_i2c_quirks = {
> +	.flags = I2C_AQ_NO_REP_START,
> +	.max_read_len = SZ_4K,
> +	.max_write_len = SZ_4K,
> +};
> +
> +/* Use smaller max_*_len settings for chips which do not support split bulk transfers */
> +static const struct i2c_adapter_quirks usbio_i2c_no_split_transfers_quirks = {
> +	.flags = I2C_AQ_NO_REP_START,
> +	.max_read_len = 63 - I2C_RW_OVERHEAD,
> +	.max_write_len = 63 -  I2C_RW_OVERHEAD,
> +};
> +
> +static const struct i2c_algorithm usbio_i2c_algo = {
> +	.master_xfer = usbio_i2c_xfer,
> +	.functionality = usbio_i2c_func,
> +};
> +
> +static int usbio_i2c_probe(struct auxiliary_device *adev,
> +		const struct auxiliary_device_id *adev_id)
> +{
> +	struct usbio_i2c_bus_desc *i2c_desc;
> +	struct device *dev = &adev->dev;
> +	u8 dummy_read_buf;
> +	struct i2c_msg dummy_read = {
> +		.addr = 0x08,
> +		.flags = I2C_M_RD,
> +		.len = 1,
> +		.buf = &dummy_read_buf,
> +	};
> +	struct usbio_i2c *i2c;
> +	u32 max_speed;
> +	int ret;
> +
> +	i2c_desc = dev_get_platdata(dev);
> +	if (!i2c_desc)
> +		return -EINVAL;
> +
> +	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
> +	if (!i2c_desc->caps)
> +		max_speed = I2C_MAX_FAST_MODE_FREQ;
> +	else
> +		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
> +
> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
> +	if (!i2c)
> +		return -ENOMEM;

Same comment on devm memory allocation than on the GPIO driver: I think you
need to use the release callback of struct device here.

> +
> +	i2c->adev = adev;
> +
> +	usbio_acpi_bind(i2c->adev, usbio_i2c_acpi_hids);
> +	usbio_get_txrxbuf_len(i2c->adev, &i2c->txbuf_len, &i2c->rxbuf_len);
> +
> +	i2c->rwbuf = devm_kzalloc(dev, max(i2c->txbuf_len, i2c->rxbuf_len), GFP_KERNEL);
> +	if (!i2c->rwbuf)
> +		return -ENOMEM;

Ditto.

> +
> +	i2c->speed = i2c_acpi_find_bus_speed(dev);
> +	if (!i2c->speed)
> +		i2c->speed = I2C_MAX_STANDARD_MODE_FREQ;
> +	else if (i2c->speed > max_speed) {
> +		dev_warn(dev, "Invalid speed %u adjusting to bus max %u\n",
> +			 i2c->speed, max_speed);
> +		i2c->speed = max_speed;
> +	}
> +
> +	i2c->adap.owner = THIS_MODULE;
> +	i2c->adap.class = I2C_CLASS_HWMON;
> +	i2c->adap.dev.parent = dev;
> +	i2c->adap.algo = &usbio_i2c_algo;
> +	i2c->adap.quirks = &usbio_i2c_quirks;
> +
> +	snprintf(i2c->adap.name, sizeof(i2c->adap.name), "%s.%d",
> +		 USBIO_I2C_CLIENT, i2c->adev->id);
> +
> +	device_set_node(&i2c->adap.dev, dev_fwnode(&adev->dev));
> +
> +	auxiliary_set_drvdata(adev, i2c);
> +	i2c_set_adapdata(&i2c->adap, i2c);
> +
> +	/*
> +	 * Test if USBIO_I2CCMD_INIT commands with the USBIO_PKTFLAG_ACK flag
> +	 * are supported.
> +	 */
> +	usbio_acquire(i2c->adev);
> +	i2c->init_supports_ack_flag = true;
> +	ret = usbio_i2c_init(&i2c->adap, &dummy_read);
> +	if (ret) {
> +		if (ret != -EPIPE) {
> +			usbio_release(i2c->adev);
> +			return ret; /* Unexpected error */
> +		}
> +
> +		dev_info(dev, "i2c-init command does not support ack flag\n");
> +		i2c->init_supports_ack_flag = false;
> +		/* Chips with this quirk also do not support split bulk transfers */
> +		i2c->adap.quirks = &usbio_i2c_no_split_transfers_quirks;
> +	} else {
> +		/* Continue dummy read to not confuse the USBIO chip */
> +		usbio_i2c_read(&i2c->adap, &dummy_read);
> +	}
> +	usbio_i2c_uninit(&i2c->adap, &dummy_read);
> +	usbio_release(i2c->adev);
> +
> +	ret = i2c_add_adapter(&i2c->adap);
> +	if (ret)
> +		return ret;
> +
> +	if (has_acpi_companion(&i2c->adap.dev))
> +		acpi_dev_clear_dependencies(ACPI_COMPANION(&i2c->adap.dev));
> +
> +	return 0;
> +}
> +
> +static void usbio_i2c_remove(struct auxiliary_device *adev)
> +{
> +	struct usbio_i2c *i2c = auxiliary_get_drvdata(adev);
> +
> +	i2c_del_adapter(&i2c->adap);
> +}
> +
> +static const struct auxiliary_device_id usbio_i2c_id_table[] = {
> +	{ "usbio.usbio-i2c" },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(auxiliary, usbio_i2c_id_table);
> +
> +static struct auxiliary_driver usbio_i2c_driver = {
> +	.name = USBIO_I2C_CLIENT,
> +	.probe = usbio_i2c_probe,
> +	.remove = usbio_i2c_remove,
> +	.id_table = usbio_i2c_id_table
> +};
> +module_auxiliary_driver(usbio_i2c_driver);
> +
> +MODULE_DESCRIPTION("Intel USBIO I2C driver");
> +MODULE_AUTHOR("Israel Cepeda <israel.a.cepeda.lopez@intel.com>");
> +MODULE_AUTHOR("Hans de Goede <hansg@kernel.org>");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS("USBIO");

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver

[Hans de Goede]

Hi Sakari,

Thank you for your review, note this
is not a full reply.

On 11-Aug-25 9:16 AM, Sakari Ailus wrote:

<snip>

>> +static int usbio_i2c_probe(struct auxiliary_device *adev,
>> +		const struct auxiliary_device_id *adev_id)
>> +{
>> +	struct usbio_i2c_bus_desc *i2c_desc;
>> +	struct device *dev = &adev->dev;
>> +	u8 dummy_read_buf;
>> +	struct i2c_msg dummy_read = {
>> +		.addr = 0x08,
>> +		.flags = I2C_M_RD,
>> +		.len = 1,
>> +		.buf = &dummy_read_buf,
>> +	};
>> +	struct usbio_i2c *i2c;
>> +	u32 max_speed;
>> +	int ret;
>> +
>> +	i2c_desc = dev_get_platdata(dev);
>> +	if (!i2c_desc)
>> +		return -EINVAL;
>> +
>> +	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
>> +	if (!i2c_desc->caps)
>> +		max_speed = I2C_MAX_FAST_MODE_FREQ;
>> +	else
>> +		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
>> +
>> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>> +	if (!i2c)
>> +		return -ENOMEM;
> 
> Same comment on devm memory allocation than on the GPIO driver: I think you
> need to use the release callback of struct device here.

And more or less the same reply, i2c_del_adapter() ensures
all i2c_clients on the adapters bus are unregistered so
it guarantees that after i2c_del_adapter() none of the adapter
functions will get called.

So freeing the struct containing the adapter after remove()
has run is fine.

I know that the media subsystem does not handle v4l2-subdevs
(which the i2c-clients are) going away very well.

Richard mentioned that after a fw-update the usbio chip will
not come back until a reboot. And I've noticed that after crashing
the usbio fw it will not come back until a full power-cycle.

So we do not need to worry about somehow slotting new i2c-clients
into the media-controller graph after a disconnect + reconnect
since the reconnect will never happen during the current boot.

We do need to somehow make sure that trying to access the v4l2-subdev
after disconnect does not cause oopses or worse.

We will likely need to somehow keep the memory for the v4l2-subdev
around and add something like a disconnected / dead flag to it.

Regards,

Hans

Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver

[Sakari Ailus]

Hi Hans,

On Mon, Aug 11, 2025 at 11:49:29AM +0200, Hans de Goede wrote:
> Hi Sakari,
> 
> Thank you for your review, note this
> is not a full reply.
> 
> On 11-Aug-25 9:16 AM, Sakari Ailus wrote:
> 
> <snip>
> 
> >> +static int usbio_i2c_probe(struct auxiliary_device *adev,
> >> +		const struct auxiliary_device_id *adev_id)
> >> +{
> >> +	struct usbio_i2c_bus_desc *i2c_desc;
> >> +	struct device *dev = &adev->dev;
> >> +	u8 dummy_read_buf;
> >> +	struct i2c_msg dummy_read = {
> >> +		.addr = 0x08,
> >> +		.flags = I2C_M_RD,
> >> +		.len = 1,
> >> +		.buf = &dummy_read_buf,
> >> +	};
> >> +	struct usbio_i2c *i2c;
> >> +	u32 max_speed;
> >> +	int ret;
> >> +
> >> +	i2c_desc = dev_get_platdata(dev);
> >> +	if (!i2c_desc)
> >> +		return -EINVAL;
> >> +
> >> +	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
> >> +	if (!i2c_desc->caps)
> >> +		max_speed = I2C_MAX_FAST_MODE_FREQ;
> >> +	else
> >> +		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
> >> +
> >> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
> >> +	if (!i2c)
> >> +		return -ENOMEM;
> > 
> > Same comment on devm memory allocation than on the GPIO driver: I think you
> > need to use the release callback of struct device here.
> 
> And more or less the same reply, i2c_del_adapter() ensures
> all i2c_clients on the adapters bus are unregistered so
> it guarantees that after i2c_del_adapter() none of the adapter
> functions will get called.
> 
> So freeing the struct containing the adapter after remove()
> has run is fine.
> 
> I know that the media subsystem does not handle v4l2-subdevs
> (which the i2c-clients are) going away very well.
> 
> Richard mentioned that after a fw-update the usbio chip will
> not come back until a reboot. And I've noticed that after crashing
> the usbio fw it will not come back until a full power-cycle.

Ouch. I guess this also depends on the implementation. Some presumably
behave better and with firmware upgrades the behaviour could (at least
theoretically) change as well.

> 
> So we do not need to worry about somehow slotting new i2c-clients
> into the media-controller graph after a disconnect + reconnect
> since the reconnect will never happen during the current boot.

Ack.

> 
> We do need to somehow make sure that trying to access the v4l2-subdev
> after disconnect does not cause oopses or worse.
> 
> We will likely need to somehow keep the memory for the v4l2-subdev
> around and add something like a disconnected / dead flag to it.

For this exact reason. V4L2 sub-devices can't be reliably removed for the
time being. (The same goes for the Media device.) I presume the sub-devices
will remain, but just won't actually be functional in this case?

-- 
Kind regards,

Sakari Ailus

Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver

[Hans de Goede]

Hi,

On 11-Aug-25 9:16 AM, Sakari Ailus wrote:
> Hi Hans, Israel,
> 
> My comments on newlines and parentheses apply to this one, too; I'm not
> making new ones in similar locations on that subject anymore for this
> patch.

Ack I've added the necessary new-line changes,
thank you for the review.

> On Sat, Aug 09, 2025 at 12:23:26PM +0200, Hans de Goede wrote:
>> From: Israel Cepeda <israel.a.cepeda.lopez@intel.com>
>>
>> Add a a driver for the I2C auxbus child device of the Intel USBIO USB
>> IO-expander used by the MIPI cameras on various new (Meteor Lake and
>> later) Intel laptops.
>>
>> Co-developed-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Hans de Goede <hansg@kernel.org>
>> Signed-off-by: Israel Cepeda <israel.a.cepeda.lopez@intel.com>

...

>> diff --git a/drivers/i2c/busses/i2c-usbio.c b/drivers/i2c/busses/i2c-usbio.c
>> new file mode 100644
>> index 000000000000..82c4769852f8
>> --- /dev/null
>> +++ b/drivers/i2c/busses/i2c-usbio.c
>> @@ -0,0 +1,344 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Copyright (c) 2025 Intel Corporation.
>> + * Copyright (c) 2025 Red Hat, Inc.
>> + */
>> +
>> +#include <linux/auxiliary_bus.h>
>> +#include <linux/dev_printk.h>
>> +#include <linux/device.h>
>> +#include <linux/i2c.h>
>> +#include <linux/types.h>
>> +#include <linux/usb/usbio.h>
>> +
>> +#define I2C_RW_OVERHEAD (sizeof(struct usbio_bulk_packet) + sizeof(struct usbio_i2c_rw))
>> +
>> +struct usbio_i2c {
>> +	u32 speed;
> 
> You could declare this with the smaller fields for better struct packing.

Ack, fixed for v2.

>> +	struct i2c_adapter adap;
>> +	struct auxiliary_device *adev;
>> +	struct usbio_i2c_rw *rwbuf;
>> +	bool init_supports_ack_flag;
>> +	u16 txbuf_len;
>> +	u16 rxbuf_len;
>> +};

...

>> +static void usbio_i2c_uninit(struct i2c_adapter *adap, struct i2c_msg *msg)
>> +{
>> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
>> +	struct usbio_i2c_uninit ubuf;
>> +
>> +	ubuf.busid = i2c->adev->id;
>> +	ubuf.config = msg->addr;
> 
> You can initialise this in declaration.

With the changes to use __le16 and _le32 for multi-byte words
which Greg rightfully asked for this now looks like this:

        ubuf.busid = i2c->adev->id;
        ubuf.config = cpu_to_le16(msg->addr);

having a function call in a struct initializer looks weird / wrong,
so I'm going to keep these one as well as the other places as-is.

Also in the usbio_i2c_read() / write() cases the struct may
get re-initialized several times if needing to split the
i2c-transfer into multiple bulk transfers.

Using struct initilization for the first one in that case
feels rather inconsistent.

...

>> +static int usbio_i2c_probe(struct auxiliary_device *adev,
>> +		const struct auxiliary_device_id *adev_id)
>> +{
>> +	struct usbio_i2c_bus_desc *i2c_desc;
>> +	struct device *dev = &adev->dev;
>> +	u8 dummy_read_buf;
>> +	struct i2c_msg dummy_read = {
>> +		.addr = 0x08,
>> +		.flags = I2C_M_RD,
>> +		.len = 1,
>> +		.buf = &dummy_read_buf,
>> +	};
>> +	struct usbio_i2c *i2c;
>> +	u32 max_speed;
>> +	int ret;
>> +
>> +	i2c_desc = dev_get_platdata(dev);
>> +	if (!i2c_desc)
>> +		return -EINVAL;
>> +
>> +	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
>> +	if (!i2c_desc->caps)
>> +		max_speed = I2C_MAX_FAST_MODE_FREQ;
>> +	else
>> +		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
>> +
>> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
>> +	if (!i2c)
>> +		return -ENOMEM;
> 
> Same comment on devm memory allocation than on the GPIO driver: I think you
> need to use the release callback of struct device here.

We unregister the adapter in remove() after that no callbacks into
the driver can be made, so using devm() managed memory here is fine.

Regards,

Hans

Re: [PATCH 3/3] i2c: Add Intel USBIO I2C driver

[Sakari Ailus]

Hi Hans,

On Fri, Sep 05, 2025 at 08:50:29PM +0200, Hans de Goede wrote:
> >> +static void usbio_i2c_uninit(struct i2c_adapter *adap, struct i2c_msg *msg)
> >> +{
> >> +	struct usbio_i2c *i2c = i2c_get_adapdata(adap);
> >> +	struct usbio_i2c_uninit ubuf;
> >> +
> >> +	ubuf.busid = i2c->adev->id;
> >> +	ubuf.config = msg->addr;
> > 
> > You can initialise this in declaration.
> 
> With the changes to use __le16 and _le32 for multi-byte words
> which Greg rightfully asked for this now looks like this:
> 
>         ubuf.busid = i2c->adev->id;
>         ubuf.config = cpu_to_le16(msg->addr);
> 
> having a function call in a struct initializer looks weird / wrong,
> so I'm going to keep these one as well as the other places as-is.

There's nothing wrong with initialising a struct field based on a function
return value.

> 
> Also in the usbio_i2c_read() / write() cases the struct may
> get re-initialized several times if needing to split the
> i2c-transfer into multiple bulk transfers.
> 
> Using struct initilization for the first one in that case
> feels rather inconsistent.

I'm fine with either.

> 
> ...
> 
> >> +static int usbio_i2c_probe(struct auxiliary_device *adev,
> >> +		const struct auxiliary_device_id *adev_id)
> >> +{
> >> +	struct usbio_i2c_bus_desc *i2c_desc;
> >> +	struct device *dev = &adev->dev;
> >> +	u8 dummy_read_buf;
> >> +	struct i2c_msg dummy_read = {
> >> +		.addr = 0x08,
> >> +		.flags = I2C_M_RD,
> >> +		.len = 1,
> >> +		.buf = &dummy_read_buf,
> >> +	};
> >> +	struct usbio_i2c *i2c;
> >> +	u32 max_speed;
> >> +	int ret;
> >> +
> >> +	i2c_desc = dev_get_platdata(dev);
> >> +	if (!i2c_desc)
> >> +		return -EINVAL;
> >> +
> >> +	/* Some USBIO chips have caps set to 0, but all chips can do 400KHz */
> >> +	if (!i2c_desc->caps)
> >> +		max_speed = I2C_MAX_FAST_MODE_FREQ;
> >> +	else
> >> +		max_speed = usbio_i2c_speeds[i2c_desc->caps & USBIO_I2C_BUS_MODE_CAP_MASK];
> >> +
> >> +	i2c = devm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
> >> +	if (!i2c)
> >> +		return -ENOMEM;
> > 
> > Same comment on devm memory allocation than on the GPIO driver: I think you
> > need to use the release callback of struct device here.
> 
> We unregister the adapter in remove() after that no callbacks into
> the driver can be made, so using devm() managed memory here is fine.

Ack.

-- 
Kind regards,

Sakari Ailus