[PATCH v19 0/4] Add Intel LJCA device driver

[PATCH v19 0/4] Add Intel LJCA device driver

[Wentong Wu]

Add driver for Intel La Jolla Cove Adapter (LJCA) device. This
IO-expander adds additional functions to the host system such
as GPIO, I2C and SPI with USB host interface. We add 4 drivers
to support this device: a USB driver, a GPIO chip driver, a I2C
controller driver and a SPI controller driver.

---
v19:
 - add v17's change which v18 doesn't apply

v18:
 - rebase patch set on top of Linus' master branch (57d88e8a5974644039fbc47806bac7bb12025636)

v17:
 - change valid_pins type to __le32 and access valid_pins with get_unaligned_le32
 - remove COMPILE_TEST for USB_LJCA Kconfig

v16:
 - drop all void * and use real types in the exported apis and internal ljca_send()
 - remove #ifdef in usb-ljca.c file
 - add documentation in ljca.h for the public structures
 - add error message in ljca_handle_cmd_ack() if error happens and remove blank line
 - use the functionality in cleanup.h for spinlock to make function much simpler
 - change the type of ex_buf in struct ljca_adapter to u8 *

v15:
 - enhance disconnect() of usb-ljca driver
 - change memchr to strchr in ljca_match_device_ids() of usb-ljca driver

v14:
 - fix build error: implicit declaration of function 'acpi_dev_clear_dependencies'

v13:
 - make ljca-usb more robust with the help of Hans de Goede
 - call acpi_dev_clear_dependencies() to mark _DEP ACPI dependencies on the I2C controller as satisfied, and patch is from Hans de Goede

v12:
 - switch dev_err to dev_dbg for i2c-ljca driver
 - avoid err printing because of calling usb_kill_urb when attempts to resubmit the rx urb

v11:
 - switch dev_err to dev_dbg for i2c-ljca driver
 - remove message length check because of defined quirk structure
 - remove I2C_FUNC_SMBUS_EMUL support

v10:
 - remove ljca_i2c_format_slave_addr
 - remove memset before write write w_packet
 - make ljca_i2c_stop void and print err message in case failure
 - use dev_err_probe in ljca_i2c_probe function

v9:
 - overhaul usb-ljca driver to make it more structured and easy understand
 - fix memory leak issue for usb-ljca driver
 - add spinlock to protect tx_buf and ex_buf
 - change exported APIs for usb-ljca driver
 - unify prefix for structures and functions for i2c-ljca driver
 - unify prefix for structures and functions for spi-ljca driver
 - unify prefix for structures and functions for gpio-ljca driver
 - update gpio-ljca, i2c-ljca and spi-ljca drivers according to usb-ljca's changes

Wentong Wu (4):
  usb: Add support for Intel LJCA device
  i2c: Add support for Intel LJCA USB I2C driver
  spi: Add support for Intel LJCA USB SPI driver
  gpio: update Intel LJCA USB GPIO driver

 drivers/gpio/Kconfig          |   4 +-
 drivers/gpio/gpio-ljca.c      | 246 +++++++------
 drivers/i2c/busses/Kconfig    |  11 +
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ljca.c | 342 +++++++++++++++++
 drivers/spi/Kconfig           |  11 +
 drivers/spi/Makefile          |   1 +
 drivers/spi/spi-ljca.c        | 297 +++++++++++++++
 drivers/usb/misc/Kconfig      |  13 +
 drivers/usb/misc/Makefile     |   1 +
 drivers/usb/misc/usb-ljca.c   | 837 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/ljca.h      | 145 ++++++++
 12 files changed, 1804 insertions(+), 105 deletions(-)
 create mode 100644 drivers/i2c/busses/i2c-ljca.c
 create mode 100644 drivers/spi/spi-ljca.c
 create mode 100644 drivers/usb/misc/usb-ljca.c
 create mode 100644 include/linux/usb/ljca.h

-- 
2.7.4

[PATCH v19 1/4] usb: Add support for Intel LJCA device

[Wentong Wu]

Implements the USB part of Intel USB-I2C/GPIO/SPI adapter device
named "La Jolla Cove Adapter" (LJCA).

The communication between the various LJCA module drivers and the
hardware will be muxed/demuxed by this driver. Three modules (
I2C, GPIO, and SPI) are supported currently.

Each sub-module of LJCA device is identified by type field within
the LJCA message header.

The sub-modules of LJCA can use ljca_transfer() to issue a transfer
between host and hardware. And ljca_register_event_cb is exported
to LJCA sub-module drivers for hardware event subscription.

The minimum code in ASL that covers this board is
Scope (\_SB.PCI0.DWC3.RHUB.HS01)
    {
        Device (GPIO)
        {
            Name (_ADR, Zero)
            Name (_STA, 0x0F)
        }

        Device (I2C)
        {
            Name (_ADR, One)
            Name (_STA, 0x0F)
        }

        Device (SPI)
        {
            Name (_ADR, 0x02)
            Name (_STA, 0x0F)
        }
    }

Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/usb/misc/Kconfig    |  13 +
 drivers/usb/misc/Makefile   |   1 +
 drivers/usb/misc/usb-ljca.c | 837 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/usb/ljca.h    | 145 ++++++++
 4 files changed, 996 insertions(+)
 create mode 100644 drivers/usb/misc/usb-ljca.c
 create mode 100644 include/linux/usb/ljca.h

diff --git a/drivers/usb/misc/Kconfig b/drivers/usb/misc/Kconfig
index 99b15b7..c510af7 100644
--- a/drivers/usb/misc/Kconfig
+++ b/drivers/usb/misc/Kconfig
@@ -165,6 +165,19 @@ config APPLE_MFI_FASTCHARGE
 
 	  It is safe to say M here.
 
+config USB_LJCA
+	tristate "Intel La Jolla Cove Adapter support"
+	select AUXILIARY_BUS
+	depends on USB && ACPI
+	help
+	  This adds support for Intel La Jolla Cove USB-I2C/SPI/GPIO
+	  Master Adapter (LJCA). Additional drivers such as I2C_LJCA,
+	  GPIO_LJCA and SPI_LJCA must be enabled in order to use the
+	  functionality of the device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called usb-ljca.
+
 source "drivers/usb/misc/sisusbvga/Kconfig"
 
 config USB_LD
diff --git a/drivers/usb/misc/Makefile b/drivers/usb/misc/Makefile
index 1992cc2..0bc732bc 100644
--- a/drivers/usb/misc/Makefile
+++ b/drivers/usb/misc/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_USB_EMI26)			+= emi26.o
 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_IDMOUSE)		+= idmouse.o
 obj-$(CONFIG_USB_IOWARRIOR)		+= iowarrior.o
 obj-$(CONFIG_USB_ISIGHTFW)		+= isight_firmware.o
diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
new file mode 100644
index 0000000..673aa2f
--- /dev/null
+++ b/drivers/usb/misc/usb-ljca.c
@@ -0,0 +1,837 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel La Jolla Cove Adapter USB driver
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/cleanup.h>
+#include <linux/dev_printk.h>
+#include <linux/kernel.h>
+#include <linux/mod_devicetable.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+#include <linux/usb.h>
+#include <linux/usb/ljca.h>
+
+#include <asm/unaligned.h>
+
+/* command flags */
+#define LJCA_ACK_FLAG			BIT(0)
+#define LJCA_RESP_FLAG			BIT(1)
+#define LJCA_CMPL_FLAG			BIT(2)
+
+#define LJCA_MAX_PACKET_SIZE		64u
+#define LJCA_MAX_PAYLOAD_SIZE		\
+		(LJCA_MAX_PACKET_SIZE - sizeof(struct ljca_msg))
+
+#define LJCA_WRITE_TIMEOUT_MS		200
+#define LJCA_WRITE_ACK_TIMEOUT_MS	500
+#define LJCA_ENUM_CLIENT_TIMEOUT_MS	20
+
+/* ljca client type */
+enum ljca_client_type {
+	LJCA_CLIENT_MNG = 1,
+	LJCA_CLIENT_GPIO = 3,
+	LJCA_CLIENT_I2C = 4,
+	LJCA_CLIENT_SPI = 5,
+};
+
+/* MNG client commands */
+enum ljca_mng_cmd {
+	LJCA_MNG_RESET = 2,
+	LJCA_MNG_ENUM_GPIO = 4,
+	LJCA_MNG_ENUM_I2C = 5,
+	LJCA_MNG_ENUM_SPI = 8,
+};
+
+/* ljca client acpi _ADR */
+enum ljca_client_acpi_adr {
+	LJCA_GPIO_ACPI_ADR,
+	LJCA_I2C1_ACPI_ADR,
+	LJCA_I2C2_ACPI_ADR,
+	LJCA_SPI1_ACPI_ADR,
+	LJCA_SPI2_ACPI_ADR,
+	LJCA_CLIENT_ACPI_ADR_MAX,
+};
+
+/* ljca cmd message structure */
+struct ljca_msg {
+	u8 type;
+	u8 cmd;
+	u8 flags;
+	u8 len;
+	u8 data[];
+} __packed;
+
+struct ljca_i2c_ctr_info {
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+} __packed;
+
+struct ljca_i2c_descriptor {
+	u8 num;
+	struct ljca_i2c_ctr_info info[];
+} __packed;
+
+struct ljca_spi_ctr_info {
+	u8 id;
+	u8 capacity;
+} __packed;
+
+struct ljca_spi_descriptor {
+	u8 num;
+	struct ljca_spi_ctr_info info[];
+} __packed;
+
+struct ljca_bank_descriptor {
+	u8 bank_id;
+	u8 pin_num;
+
+	/* 1 bit for each gpio, 1 means valid */
+	__le32 valid_pins;
+} __packed;
+
+struct ljca_gpio_descriptor {
+	u8 pins_per_bank;
+	u8 bank_num;
+	struct ljca_bank_descriptor bank_desc[];
+} __packed;
+
+struct ljca_adapter {
+	struct usb_interface *intf;
+	struct usb_device *usb_dev;
+	struct device *dev;
+
+	unsigned int rx_pipe;
+	unsigned int tx_pipe;
+
+	/* urb for recv */
+	struct urb *rx_urb;
+	/* buffer for recv */
+	void *rx_buf;
+	unsigned int rx_len;
+
+	/* external buffer for recv */
+	u8 *ex_buf;
+	unsigned int ex_buf_len;
+	/* actual data length copied to external buffer */
+	unsigned int actual_length;
+
+	/* buffer for send */
+	void *tx_buf;
+	unsigned int tx_buf_len;
+
+	/* lock to protect tx_buf and ex_buf */
+	spinlock_t lock;
+
+	struct completion cmd_completion;
+
+	/* mutex to protect command download */
+	struct mutex mutex;
+
+	/* client device list */
+	struct list_head client_list;
+
+	/* disconnect ongoing or not */
+	bool disconnect;
+
+	/* used to reset firmware */
+	u32 reset_id;
+};
+
+struct ljca_match_ids_walk_data {
+	const struct acpi_device_id *ids;
+	const char *uid;
+	struct acpi_device *adev;
+};
+
+static const struct acpi_device_id ljca_gpio_hids[] = {
+	{ "INTC1074" },
+	{ "INTC1096" },
+	{ "INTC100B" },
+	{ "INTC10D1" },
+	{},
+};
+
+static const struct acpi_device_id ljca_i2c_hids[] = {
+	{ "INTC1075" },
+	{ "INTC1097" },
+	{ "INTC100C" },
+	{ "INTC10D2" },
+	{},
+};
+
+static const struct acpi_device_id ljca_spi_hids[] = {
+	{ "INTC1091" },
+	{ "INTC1098" },
+	{ "INTC100D" },
+	{ "INTC10D3" },
+	{},
+};
+
+static void ljca_handle_event(struct ljca_adapter *adap,
+			      struct ljca_msg *header)
+{
+	struct ljca_client *client;
+
+	list_for_each_entry(client, &adap->client_list, link) {
+		/*
+		 * FIXME: currently only GPIO register event callback.
+		 * firmware message structure should include id when
+		 * multiple same type clients register event callback.
+		 */
+		if (client->type == header->type) {
+			scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
+				client->event_cb(client->context, header->cmd,
+						 header->data, header->len);
+			}
+
+			break;
+		}
+	}
+}
+
+/* process command ack */
+static void ljca_handle_cmd_ack(struct ljca_adapter *adap,
+				struct ljca_msg *header)
+{
+	struct ljca_msg *tx_header = adap->tx_buf;
+	unsigned int actual_len = 0;
+	unsigned int ibuf_len;
+	unsigned long flags;
+	u8 *ibuf;
+
+	spin_lock_irqsave(&adap->lock, flags);
+
+	if (tx_header->type != header->type || tx_header->cmd != header->cmd) {
+		dev_err(adap->dev, "cmd ack mismatch error\n");
+		spin_unlock_irqrestore(&adap->lock, flags);
+		return;
+	}
+
+	ibuf_len = adap->ex_buf_len;
+	ibuf = adap->ex_buf;
+
+	if (ibuf && ibuf_len) {
+		actual_len = min_t(unsigned int, header->len, ibuf_len);
+
+		/* copy received data to external buffer */
+		memcpy(ibuf, header->data, actual_len);
+	}
+	/* update copied data length */
+	adap->actual_length = actual_len;
+
+	spin_unlock_irqrestore(&adap->lock, flags);
+
+	complete(&adap->cmd_completion);
+}
+
+static void ljca_recv(struct urb *urb)
+{
+	struct ljca_msg *header = urb->transfer_buffer;
+	struct ljca_adapter *adap = urb->context;
+	int ret;
+
+	if (urb->status) {
+		/* sync/async unlink faults aren't errors */
+		if (urb->status == -ENOENT) {
+			/*
+			 * directly complete the possible ongoing transfer
+			 * during disconnect
+			 */
+			if (adap->disconnect)
+				complete(&adap->cmd_completion);
+			return;
+		}
+
+		if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN)
+			return;
+
+		dev_err(adap->dev, "recv urb error: %d\n", urb->status);
+		goto resubmit;
+	}
+
+	if (header->len + sizeof(*header) != urb->actual_length)
+		goto resubmit;
+
+	if (header->flags & LJCA_ACK_FLAG)
+		ljca_handle_cmd_ack(adap, header);
+	else
+		ljca_handle_event(adap, header);
+
+resubmit:
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret && ret != -EPERM)
+		dev_err(adap->dev, "resubmit recv urb error %d\n", ret);
+}
+
+static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
+		     const u8 *obuf, unsigned int obuf_len, u8 *ibuf,
+		     unsigned int ibuf_len, bool ack, unsigned long timeout)
+{
+	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
+	struct ljca_msg *header = adap->tx_buf;
+	unsigned int actual;
+	int ret = 0;
+
+	if (adap->disconnect)
+		return -ENODEV;
+
+	if (msg_len > adap->tx_buf_len)
+		return -EINVAL;
+
+	mutex_lock(&adap->mutex);
+
+	scoped_guard(spinlock_irqsave, &adap->lock) {
+		header->type = type;
+		header->cmd = cmd;
+		header->len = obuf_len;
+		if (obuf)
+			memcpy(header->data, obuf, obuf_len);
+		header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
+
+		adap->ex_buf = ibuf;
+		adap->ex_buf_len = ibuf_len;
+		adap->actual_length = 0;
+	}
+
+	reinit_completion(&adap->cmd_completion);
+
+	usb_autopm_get_interface(adap->intf);
+
+	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
+			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
+
+	usb_autopm_put_interface(adap->intf);
+
+	if (!ret && ack) {
+		ret = wait_for_completion_timeout(&adap->cmd_completion,
+						  timeout);
+		if (ret < 0) {
+			goto out;
+		} if (!ret) {
+			ret = -ETIMEDOUT;
+			goto out;
+		}
+	}
+	ret = adap->actual_length;
+
+out:
+	scoped_guard(spinlock_irqsave, &adap->lock) {
+		adap->ex_buf = NULL;
+		adap->ex_buf_len = 0;
+		memset(header, 0, sizeof(*header));
+	}
+
+	mutex_unlock(&adap->mutex);
+
+	return ret;
+}
+
+int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
+		  unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len)
+{
+	return ljca_send(client->adapter, client->type, cmd,
+			 obuf, obuf_len, ibuf, ibuf_len, true,
+			 LJCA_WRITE_ACK_TIMEOUT_MS);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
+
+int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
+			unsigned int obuf_len)
+{
+	return ljca_send(client->adapter, client->type, cmd, obuf,
+			 obuf_len, NULL, 0, false, LJCA_WRITE_ACK_TIMEOUT_MS);
+}
+EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
+
+int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb,
+			   void *context)
+{
+	unsigned long flags;
+
+	if (!event_cb)
+		return -EINVAL;
+
+	spin_lock_irqsave(&client->event_cb_lock, flags);
+
+	if (client->event_cb) {
+		spin_unlock_irqrestore(&client->event_cb_lock, flags);
+		return -EALREADY;
+	}
+
+	client->event_cb = event_cb;
+	client->context = context;
+
+	spin_unlock_irqrestore(&client->event_cb_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
+
+void ljca_unregister_event_cb(struct ljca_client *client)
+{
+	scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
+		client->event_cb = NULL;
+		client->context = NULL;
+	}
+}
+EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
+
+static int ljca_match_device_ids(struct acpi_device *adev, void *data)
+{
+	struct ljca_match_ids_walk_data *wd = data;
+	const char *uid = acpi_device_uid(adev);
+
+	if (acpi_match_device_ids(adev, wd->ids))
+		return 0;
+
+	if (!wd->uid)
+		goto match;
+
+	if (!uid)
+		uid = "0";
+	else
+		uid = strchr(uid, wd->uid[0]);
+
+	if (!uid || strcmp(uid, wd->uid))
+		return 0;
+
+match:
+	wd->adev = adev;
+
+	return 1;
+}
+
+/* bind auxiliary device to acpi device */
+static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
+				  struct auxiliary_device *auxdev,
+				  u64 adr, u8 id)
+{
+	struct ljca_match_ids_walk_data wd = { 0 };
+	struct acpi_device *parent, *adev;
+	struct device *dev = adap->dev;
+	char uid[4];
+
+	parent = ACPI_COMPANION(dev);
+	if (!parent)
+		return;
+
+	/*
+	 * get auxdev ACPI handle from the ACPI device directly
+	 * under the parent that matches _ADR.
+	 */
+	adev = acpi_find_child_device(parent, adr, false);
+	if (adev) {
+		ACPI_COMPANION_SET(&auxdev->dev, adev);
+		return;
+	}
+
+	/*
+	 * _ADR is a grey area in the ACPI specification, some
+	 * platforms use _HID to distinguish children devices.
+	 */
+	switch (adr) {
+	case LJCA_GPIO_ACPI_ADR:
+		wd.ids = ljca_gpio_hids;
+		break;
+	case LJCA_I2C1_ACPI_ADR:
+	case LJCA_I2C2_ACPI_ADR:
+		snprintf(uid, sizeof(uid), "%d", id);
+		wd.uid = uid;
+		wd.ids = ljca_i2c_hids;
+		break;
+	case LJCA_SPI1_ACPI_ADR:
+	case LJCA_SPI2_ACPI_ADR:
+		wd.ids = ljca_spi_hids;
+		break;
+	default:
+		dev_warn(dev, "unsupported _ADR\n");
+		return;
+	}
+
+	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
+	if (wd.adev) {
+		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
+		return;
+	}
+
+	parent = ACPI_COMPANION(dev->parent->parent);
+	if (!parent)
+		return;
+
+	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
+	if (wd.adev)
+		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
+}
+
+static void ljca_auxdev_release(struct device *dev)
+{
+	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
+
+	kfree(auxdev->dev.platform_data);
+}
+
+static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
+				  char *name, void *data, u64 adr)
+{
+	struct auxiliary_device *auxdev;
+	struct ljca_client *client;
+	int ret;
+
+	client = kzalloc(sizeof *client, GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->type = type;
+	client->id = id;
+	client->adapter = adap;
+	spin_lock_init(&client->event_cb_lock);
+
+	auxdev = &client->auxdev;
+	auxdev->name = name;
+	auxdev->id = id;
+
+	auxdev->dev.parent = adap->dev;
+	auxdev->dev.platform_data = data;
+	auxdev->dev.release = ljca_auxdev_release;
+
+	ret = auxiliary_device_init(auxdev);
+	if (ret)
+		goto err_free;
+
+	ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
+
+	ret = auxiliary_device_add(auxdev);
+	if (ret)
+		goto err_uninit;
+
+	list_add_tail(&client->link, &adap->client_list);
+
+	return 0;
+
+err_uninit:
+	auxiliary_device_uninit(auxdev);
+
+err_free:
+	kfree(client);
+
+	return ret;
+}
+
+static int ljca_enumerate_gpio(struct ljca_adapter *adap)
+{
+	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
+	struct ljca_gpio_descriptor *desc;
+	struct ljca_gpio_info *gpio_info;
+	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
+	int ret, gpio_num;
+	unsigned int i;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, NULL, 0, buf,
+			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	/* check firmware response */
+	desc = (struct ljca_gpio_descriptor *)buf;
+	if (ret != struct_size(desc, bank_desc, desc->bank_num))
+		return -EINVAL;
+
+	gpio_num = desc->pins_per_bank * desc->bank_num;
+	if (gpio_num > LJCA_MAX_GPIO_NUM)
+		return -EINVAL;
+
+	/* construct platform data */
+	gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL);
+	if (!gpio_info)
+		return -ENOMEM;
+	gpio_info->num = gpio_num;
+
+	for (i = 0; i < desc->bank_num; i++)
+		valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins);
+	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
+
+	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
+				     gpio_info, LJCA_GPIO_ACPI_ADR);
+	if (ret)
+		kfree(gpio_info);
+
+	return ret;
+}
+
+static int ljca_enumerate_i2c(struct ljca_adapter *adap)
+{
+	struct ljca_i2c_descriptor *desc;
+	struct ljca_i2c_info *i2c_info;
+	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
+	unsigned int i;
+	int ret;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, 0, buf,
+			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	/* check firmware response */
+	desc = (struct ljca_i2c_descriptor *)buf;
+	if (ret != struct_size(desc, info, desc->num))
+		return -EINVAL;
+
+	for (i = 0; i < desc->num; i++) {
+		/* construct platform data */
+		i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL);
+		if (!i2c_info)
+			return -ENOMEM;
+
+		i2c_info->id = desc->info[i].id;
+		i2c_info->capacity = desc->info[i].capacity;
+		i2c_info->intr_pin = desc->info[i].intr_pin;
+
+		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
+					     "ljca-i2c", i2c_info,
+					     LJCA_I2C1_ACPI_ADR + i);
+		if (ret) {
+			kfree(i2c_info);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ljca_enumerate_spi(struct ljca_adapter *adap)
+{
+	struct ljca_spi_descriptor *desc;
+	struct ljca_spi_info *spi_info;
+	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
+	unsigned int i;
+	int ret;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
+			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	desc = (struct ljca_spi_descriptor *)buf;
+	for (i = 0; i < desc->num; i++) {
+		/* construct platform data */
+		spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL);
+		if (!spi_info)
+			return -ENOMEM;
+
+		spi_info->id = desc->info[i].id;
+		spi_info->capacity = desc->info[i].capacity;
+
+		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
+					     "ljca-spi", spi_info,
+					     LJCA_SPI1_ACPI_ADR + i);
+		if (ret) {
+			kfree(spi_info);
+			return ret;
+		}
+	}
+
+	return 0;
+}
+
+static int ljca_reset_handshake(struct ljca_adapter *adap)
+{
+	__le32 reset_id = cpu_to_le32(adap->reset_id);
+	__le32 reset_id_ret = 0;
+	int ret;
+
+	adap->reset_id++;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_RESET, (u8 *)&reset_id,
+			sizeof(__le32), (u8 *)&reset_id_ret, sizeof(__le32),
+			true, LJCA_WRITE_ACK_TIMEOUT_MS);
+	if (ret < 0)
+		return ret;
+
+	if (reset_id_ret != reset_id)
+		return -EINVAL;
+
+	return 0;
+}
+
+static int ljca_enumerate_clients(struct ljca_adapter *adap)
+{
+	int ret;
+
+	ret = ljca_reset_handshake(adap);
+	if (ret)
+		return ret;
+
+	ret = ljca_enumerate_gpio(adap);
+	if (ret)
+		dev_warn(adap->dev, "enumerate GPIO error\n");
+
+	ret = ljca_enumerate_i2c(adap);
+	if (ret)
+		dev_warn(adap->dev, "enumerate I2C error\n");
+
+	ret = ljca_enumerate_spi(adap);
+	if (ret)
+		dev_warn(adap->dev, "enumerate SPI error\n");
+
+	return 0;
+}
+
+static int ljca_probe(struct usb_interface *interface,
+		      const struct usb_device_id *id)
+{
+	struct usb_device *usb_dev = interface_to_usbdev(interface);
+	struct usb_host_interface *alt = interface->cur_altsetting;
+	struct usb_endpoint_descriptor *ep_in, *ep_out;
+	struct device *dev = &interface->dev;
+	struct ljca_adapter *adap;
+	int ret;
+
+	adap = devm_kzalloc(dev, sizeof(*adap), GFP_KERNEL);
+	if (!adap)
+		return -ENOMEM;
+
+	/* separate tx buffer allocation for alignment */
+	adap->tx_buf = devm_kzalloc(dev, LJCA_MAX_PACKET_SIZE, GFP_KERNEL);
+	if (!adap->tx_buf)
+		return -ENOMEM;
+	adap->tx_buf_len = LJCA_MAX_PACKET_SIZE;
+
+	mutex_init(&adap->mutex);
+	spin_lock_init(&adap->lock);
+	init_completion(&adap->cmd_completion);
+	INIT_LIST_HEAD(&adap->client_list);
+
+	adap->intf = usb_get_intf(interface);
+	adap->usb_dev = usb_dev;
+	adap->dev = dev;
+
+	/*
+	 * find the first bulk in and out endpoints.
+	 * ignore any others.
+	 */
+	ret = usb_find_common_endpoints(alt, &ep_in, &ep_out, NULL, NULL);
+	if (ret) {
+		dev_err(dev, "bulk endpoints not found\n");
+		goto err;
+	}
+	adap->rx_pipe = usb_rcvbulkpipe(usb_dev, usb_endpoint_num(ep_in));
+	adap->tx_pipe = usb_sndbulkpipe(usb_dev, usb_endpoint_num(ep_out));
+
+	/* setup rx buffer */
+	adap->rx_len = usb_endpoint_maxp(ep_in);
+	adap->rx_buf = devm_kzalloc(dev, adap->rx_len, GFP_KERNEL);
+	if (!adap->rx_buf) {
+		ret = -ENOMEM;
+		goto err;
+	}
+
+	/* alloc rx urb */
+	adap->rx_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!adap->rx_urb) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	usb_fill_bulk_urb(adap->rx_urb, usb_dev, adap->rx_pipe,
+			  adap->rx_buf, adap->rx_len, ljca_recv, adap);
+
+	usb_set_intfdata(interface, adap);
+
+	/* submit rx urb before enumerate clients */
+	ret = usb_submit_urb(adap->rx_urb, GFP_KERNEL);
+	if (ret) {
+		dev_err(dev, "submit rx urb failed: %d\n", ret);
+		goto err_free;
+	}
+
+	ret = ljca_enumerate_clients(adap);
+	if (ret)
+		goto err_kill;
+
+	usb_enable_autosuspend(usb_dev);
+
+	return 0;
+
+err_kill:
+	usb_kill_urb(adap->rx_urb);
+
+err_free:
+	usb_free_urb(adap->rx_urb);
+
+err:
+	usb_put_intf(adap->intf);
+	mutex_destroy(&adap->mutex);
+
+	return ret;
+}
+
+static void ljca_disconnect(struct usb_interface *interface)
+{
+	struct ljca_adapter *adap = usb_get_intfdata(interface);
+	struct ljca_client *client, *next;
+
+	adap->disconnect = true;
+
+	usb_kill_urb(adap->rx_urb);
+
+	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
+		auxiliary_device_delete(&client->auxdev);
+		auxiliary_device_uninit(&client->auxdev);
+
+		list_del_init(&client->link);
+		kfree(client);
+	}
+
+	usb_free_urb(adap->rx_urb);
+
+	usb_put_intf(adap->intf);
+
+	mutex_destroy(&adap->mutex);
+}
+
+static int ljca_suspend(struct usb_interface *interface, pm_message_t message)
+{
+	struct ljca_adapter *adap = usb_get_intfdata(interface);
+
+	usb_kill_urb(adap->rx_urb);
+
+	return 0;
+}
+
+static int ljca_resume(struct usb_interface *interface)
+{
+	struct ljca_adapter *adap = usb_get_intfdata(interface);
+
+	return usb_submit_urb(adap->rx_urb, GFP_KERNEL);
+}
+
+static const struct usb_device_id ljca_table[] = {
+	{ USB_DEVICE(0x8086, 0x0b63) },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(usb, ljca_table);
+
+static struct usb_driver ljca_driver = {
+	.name = "ljca",
+	.id_table = ljca_table,
+	.probe = ljca_probe,
+	.disconnect = ljca_disconnect,
+	.suspend = ljca_suspend,
+	.resume = ljca_resume,
+	.supports_autosuspend = 1,
+};
+module_usb_driver(ljca_driver);
+
+MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>");
+MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>");
+MODULE_AUTHOR("Lixu Zhang <lixu.zhang@intel.com>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB driver");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/usb/ljca.h b/include/linux/usb/ljca.h
new file mode 100644
index 0000000..502fc8b
--- /dev/null
+++ b/include/linux/usb/ljca.h
@@ -0,0 +1,145 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (c) 2023, Intel Corporation. All rights reserved.
+ */
+#ifndef _LINUX_USB_LJCA_H_
+#define _LINUX_USB_LJCA_H_
+
+#include <linux/auxiliary_bus.h>
+#include <linux/list.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
+#define LJCA_MAX_GPIO_NUM 64
+
+#define auxiliary_dev_to_ljca_client(auxiliary_dev)			\
+		container_of(auxiliary_dev, struct ljca_client, auxdev)
+
+struct ljca_adapter;
+
+/**
+ * typedef ljca_event_cb_t - event callback function signature
+ *
+ * @context: the execution context of who registered this callback
+ * @cmd: the command from device for this event
+ * @evt_data: the event data payload
+ * @len: the event data payload length
+ *
+ * The callback function is called in interrupt context and the data payload is
+ * only valid during the call. If the user needs later access of the data, it
+ * must copy it.
+ */
+typedef void (*ljca_event_cb_t)(void *context, u8 cmd, const void *evt_data, int len);
+
+/**
+ * struct ljca_client - represent a ljca client device
+ *
+ * @type: ljca client type
+ * @id: ljca client id within same client type
+ * @link: ljca client on the same ljca adapter
+ * @auxdev: auxiliary device object
+ * @adapter: ljca adapter the ljca client sit on
+ * @context: the execution context of the event callback
+ * @event_cb: ljca client driver register this callback to get
+ *	firmware asynchronous rx buffer pending notifications
+ * @event_cb_lock: spinlock to protect event callback
+ */
+struct ljca_client {
+	u8 type;
+	u8 id;
+	struct list_head link;
+	struct auxiliary_device auxdev;
+	struct ljca_adapter *adapter;
+
+	void *context;
+	ljca_event_cb_t event_cb;
+	/* lock to protect event_cb */
+	spinlock_t event_cb_lock;
+};
+
+/**
+ * struct ljca_gpio_info - ljca gpio client device info
+ *
+ * @num: ljca gpio client device pin number
+ * @valid_pin_map: ljca gpio client device valid pin mapping
+ */
+struct ljca_gpio_info {
+	unsigned int num;
+	DECLARE_BITMAP(valid_pin_map, LJCA_MAX_GPIO_NUM);
+};
+
+/**
+ * struct ljca_i2c_info - ljca i2c client device info
+ *
+ * @id: ljca i2c client device identification number
+ * @capacity: ljca i2c client device capacity
+ * @intr_pin: ljca i2c client device interrupt pin number if exists
+ */
+struct ljca_i2c_info {
+	u8 id;
+	u8 capacity;
+	u8 intr_pin;
+};
+
+/**
+ * struct ljca_spi_info - ljca spi client device info
+ *
+ * @id: ljca spi client device identification number
+ * @capacity: ljca spi client device capacity
+ */
+struct ljca_spi_info {
+	u8 id;
+	u8 capacity;
+};
+
+/**
+ * ljca_register_event_cb - register a callback function to receive events
+ *
+ * @client: ljca client device
+ * @event_cb: callback function
+ * @context: execution context of event callback
+ *
+ * Return: 0 in case of success, negative value in case of error
+ */
+int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb, void *context);
+
+/**
+ * ljca_unregister_event_cb - unregister the callback function for an event
+ *
+ * @client: ljca client device
+ */
+void ljca_unregister_event_cb(struct ljca_client *client);
+
+/**
+ * ljca_transfer - issue a LJCA command and wait for a response
+ *
+ * @client: ljca client device
+ * @cmd: the command to be sent to the device
+ * @obuf: the buffer to be sent to the device; it can be NULL if the user
+ *	doesn't need to transmit data with this command
+ * @obuf_len: the size of the buffer to be sent to the device; it should
+ *	be 0 when obuf is NULL
+ * @ibuf: any data associated with the response will be copied here; it can be
+ *	NULL if the user doesn't need the response data
+ * @ibuf_len: must be initialized to the input buffer size
+ *
+ * Return: the actual length data transferred for success, negative value for errors
+ */
+int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
+		  unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len);
+
+/**
+ * ljca_transfer_noack - issue a LJCA command without a response
+ *
+ * @client: ljca client device
+ * @cmd: the command to be sent to the device
+ * @obuf: the buffer to be sent to the device; it can be NULL if the user
+ *	doesn't need to transmit data with this command
+ * @obuf_len: the size of the buffer to be sent to the device
+ *
+ * Return: 0 for success, negative value for errors
+ */
+int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
+			unsigned int obuf_len);
+
+#endif
-- 
2.7.4

[PATCH v19 2/4] i2c: Add support for Intel LJCA USB I2C driver

[Wentong Wu]

Implements the I2C function of Intel USB-I2C/GPIO/SPI adapter device
named "La Jolla Cove Adapter" (LJCA). It communicate with LJCA I2C
module with specific protocol through interfaces exported by LJCA
USB driver.

Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/i2c/busses/Kconfig    |  11 ++
 drivers/i2c/busses/Makefile   |   1 +
 drivers/i2c/busses/i2c-ljca.c | 342 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 354 insertions(+)
 create mode 100644 drivers/i2c/busses/i2c-ljca.c

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 169607e..035b7f5 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1264,6 +1264,17 @@ config I2C_DLN2
 	 This driver can also be built as a module.  If so, the module
 	 will be called i2c-dln2.
 
+config I2C_LJCA
+	tristate "I2C functionality of Intel La Jolla Cove Adapter"
+	depends on USB_LJCA
+	default USB_LJCA
+	help
+	  If you say yes to this option, I2C functionality support of Intel
+	  La Jolla Cove Adapter (LJCA) will be included.
+
+	  This driver can also be built as a module.  If so, the module
+	  will be called i2c-ljca.
+
 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 af56fe2..3757b93 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -133,6 +133,7 @@ obj-$(CONFIG_I2C_GXP)		+= i2c-gxp.o
 # External I2C/SMBus adapter drivers
 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_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-ljca.c b/drivers/i2c/busses/i2c-ljca.c
new file mode 100644
index 0000000..9c544bb
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ljca.c
@@ -0,0 +1,342 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel La Jolla Cove Adapter USB-I2C driver
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ */
+
+#include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/usb/ljca.h>
+
+/* I2C init flags */
+#define LJCA_I2C_INIT_FLAG_MODE			BIT(0)
+#define LJCA_I2C_INIT_FLAG_MODE_POLLING		FIELD_PREP(LJCA_I2C_INIT_FLAG_MODE, 0)
+#define LJCA_I2C_INIT_FLAG_MODE_INTERRUPT	FIELD_PREP(LJCA_I2C_INIT_FLAG_MODE, 1)
+
+#define LJCA_I2C_INIT_FLAG_ADDR_16BIT		BIT(0)
+
+#define LJCA_I2C_INIT_FLAG_FREQ			GENMASK(2, 1)
+#define LJCA_I2C_INIT_FLAG_FREQ_100K		FIELD_PREP(LJCA_I2C_INIT_FLAG_FREQ, 0)
+#define LJCA_I2C_INIT_FLAG_FREQ_400K		FIELD_PREP(LJCA_I2C_INIT_FLAG_FREQ, 1)
+#define LJCA_I2C_INIT_FLAG_FREQ_1M		FIELD_PREP(LJCA_I2C_INIT_FLAG_FREQ, 2)
+
+#define LJCA_I2C_BUF_SIZE			60u
+#define LJCA_I2C_MAX_XFER_SIZE			(LJCA_I2C_BUF_SIZE - sizeof(struct ljca_i2c_rw_packet))
+
+/* I2C commands */
+enum ljca_i2c_cmd {
+	LJCA_I2C_INIT = 1,
+	LJCA_I2C_XFER,
+	LJCA_I2C_START,
+	LJCA_I2C_STOP,
+	LJCA_I2C_READ,
+	LJCA_I2C_WRITE,
+};
+
+enum ljca_xfer_type {
+	LJCA_I2C_WRITE_XFER_TYPE,
+	LJCA_I2C_READ_XFER_TYPE,
+};
+
+/* I2C raw commands: Init/Start/Read/Write/Stop */
+struct ljca_i2c_rw_packet {
+	u8 id;
+	__le16 len;
+	u8 data[];
+} __packed;
+
+struct ljca_i2c_dev {
+	struct ljca_client *ljca;
+	struct ljca_i2c_info *i2c_info;
+	struct i2c_adapter adap;
+
+	u8 obuf[LJCA_I2C_BUF_SIZE];
+	u8 ibuf[LJCA_I2C_BUF_SIZE];
+};
+
+static int ljca_i2c_init(struct ljca_i2c_dev *ljca_i2c, u8 id)
+{
+	struct ljca_i2c_rw_packet *w_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->obuf;
+	int ret;
+
+	w_packet->id = id;
+	w_packet->len = cpu_to_le16(sizeof(*w_packet->data));
+	w_packet->data[0] = LJCA_I2C_INIT_FLAG_FREQ_400K;
+
+	ret = ljca_transfer(ljca_i2c->ljca, LJCA_I2C_INIT, (u8 *)w_packet,
+			    struct_size(w_packet, data, 1), NULL, 0);
+
+	return ret < 0 ? ret : 0;
+}
+
+static int ljca_i2c_start(struct ljca_i2c_dev *ljca_i2c, u8 slave_addr,
+			  enum ljca_xfer_type type)
+{
+	struct ljca_i2c_rw_packet *w_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->obuf;
+	struct ljca_i2c_rw_packet *r_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->ibuf;
+	s16 rp_len;
+	int ret;
+
+	w_packet->id = ljca_i2c->i2c_info->id;
+	w_packet->len = cpu_to_le16(sizeof(*w_packet->data));
+	w_packet->data[0] = (slave_addr << 1) | type;
+
+	ret = ljca_transfer(ljca_i2c->ljca, LJCA_I2C_START, (u8 *)w_packet,
+			    struct_size(w_packet, data, 1), (u8 *)r_packet,
+			    LJCA_I2C_BUF_SIZE);
+	if (ret < 0 || ret < sizeof(*r_packet))
+		return ret < 0 ? ret : -EIO;
+
+	rp_len = le16_to_cpu(r_packet->len);
+	if (rp_len < 0 || r_packet->id != w_packet->id) {
+		dev_dbg(&ljca_i2c->adap.dev,
+			"i2c start failed len: %d id: %d %d\n",
+			rp_len, r_packet->id, w_packet->id);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static void ljca_i2c_stop(struct ljca_i2c_dev *ljca_i2c, u8 slave_addr)
+{
+	struct ljca_i2c_rw_packet *w_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->obuf;
+	struct ljca_i2c_rw_packet *r_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->ibuf;
+	s16 rp_len;
+	int ret;
+
+	w_packet->id = ljca_i2c->i2c_info->id;
+	w_packet->len = cpu_to_le16(sizeof(*w_packet->data));
+	w_packet->data[0] = 0;
+
+	ret = ljca_transfer(ljca_i2c->ljca, LJCA_I2C_STOP, (u8 *)w_packet,
+			    struct_size(w_packet, data, 1), (u8 *)r_packet,
+			    LJCA_I2C_BUF_SIZE);
+	if (ret < 0 || ret < sizeof(*r_packet)) {
+		dev_dbg(&ljca_i2c->adap.dev,
+			"i2c stop failed ret: %d id: %d\n",
+			ret, w_packet->id);
+		return;
+	}
+
+	rp_len = le16_to_cpu(r_packet->len);
+	if (rp_len < 0 || r_packet->id != w_packet->id)
+		dev_dbg(&ljca_i2c->adap.dev,
+			"i2c stop failed len: %d id: %d %d\n",
+			rp_len, r_packet->id, w_packet->id);
+}
+
+static int ljca_i2c_pure_read(struct ljca_i2c_dev *ljca_i2c, u8 *data, u8 len)
+{
+	struct ljca_i2c_rw_packet *w_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->obuf;
+	struct ljca_i2c_rw_packet *r_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->ibuf;
+	s16 rp_len;
+	int ret;
+
+	w_packet->id = ljca_i2c->i2c_info->id;
+	w_packet->len = cpu_to_le16(len);
+	w_packet->data[0] = 0;
+
+	ret = ljca_transfer(ljca_i2c->ljca, LJCA_I2C_READ, (u8 *)w_packet,
+			    struct_size(w_packet, data, 1), (u8 *)r_packet,
+			    LJCA_I2C_BUF_SIZE);
+	if (ret < 0 || ret < sizeof(*r_packet))
+		return ret < 0 ? ret : -EIO;
+
+	rp_len = le16_to_cpu(r_packet->len);
+	if (rp_len != len || r_packet->id != w_packet->id) {
+		dev_dbg(&ljca_i2c->adap.dev,
+			"i2c raw read failed len: %d id: %d %d\n",
+			rp_len, r_packet->id, w_packet->id);
+		return -EIO;
+	}
+
+	memcpy(data, r_packet->data, len);
+
+	return 0;
+}
+
+static int ljca_i2c_read(struct ljca_i2c_dev *ljca_i2c, u8 slave_addr, u8 *data,
+			 u8 len)
+{
+	int ret;
+
+	ret = ljca_i2c_start(ljca_i2c, slave_addr, LJCA_I2C_READ_XFER_TYPE);
+	if (!ret)
+		ret = ljca_i2c_pure_read(ljca_i2c, data, len);
+
+	ljca_i2c_stop(ljca_i2c, slave_addr);
+
+	return ret;
+}
+
+static int ljca_i2c_pure_write(struct ljca_i2c_dev *ljca_i2c, u8 *data, u8 len)
+{
+	struct ljca_i2c_rw_packet *w_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->obuf;
+	struct ljca_i2c_rw_packet *r_packet =
+			(struct ljca_i2c_rw_packet *)ljca_i2c->ibuf;
+	s16 rplen;
+	int ret;
+
+	w_packet->id = ljca_i2c->i2c_info->id;
+	w_packet->len = cpu_to_le16(len);
+	memcpy(w_packet->data, data, len);
+
+	ret = ljca_transfer(ljca_i2c->ljca, LJCA_I2C_WRITE, (u8 *)w_packet,
+			    struct_size(w_packet, data, len), (u8 *)r_packet,
+			    LJCA_I2C_BUF_SIZE);
+	if (ret < 0 || ret < sizeof(*r_packet))
+		return ret < 0 ? ret : -EIO;
+
+	rplen = le16_to_cpu(r_packet->len);
+	if (rplen != len || r_packet->id != w_packet->id) {
+		dev_dbg(&ljca_i2c->adap.dev,
+			"i2c write failed len: %d id: %d/%d\n",
+			rplen, r_packet->id, w_packet->id);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int ljca_i2c_write(struct ljca_i2c_dev *ljca_i2c, u8 slave_addr,
+			  u8 *data, u8 len)
+{
+	int ret;
+
+	ret = ljca_i2c_start(ljca_i2c, slave_addr, LJCA_I2C_WRITE_XFER_TYPE);
+	if (!ret)
+		ret = ljca_i2c_pure_write(ljca_i2c, data, len);
+
+	ljca_i2c_stop(ljca_i2c, slave_addr);
+
+	return ret;
+}
+
+static int ljca_i2c_xfer(struct i2c_adapter *adapter, struct i2c_msg *msg,
+			 int num)
+{
+	struct ljca_i2c_dev *ljca_i2c;
+	struct i2c_msg *cur_msg;
+	int i, ret;
+
+	ljca_i2c = i2c_get_adapdata(adapter);
+	if (!ljca_i2c)
+		return -EINVAL;
+
+	for (i = 0; i < num; i++) {
+		cur_msg = &msg[i];
+		if (cur_msg->flags & I2C_M_RD)
+			ret = ljca_i2c_read(ljca_i2c, cur_msg->addr,
+					    cur_msg->buf, cur_msg->len);
+		else
+			ret = ljca_i2c_write(ljca_i2c, cur_msg->addr,
+					     cur_msg->buf, cur_msg->len);
+
+		if (ret)
+			return ret;
+	}
+
+	return num;
+}
+
+static u32 ljca_i2c_func(struct i2c_adapter *adap)
+{
+	return I2C_FUNC_I2C;
+}
+
+static const struct i2c_adapter_quirks ljca_i2c_quirks = {
+	.max_read_len = LJCA_I2C_MAX_XFER_SIZE,
+	.max_write_len = LJCA_I2C_MAX_XFER_SIZE,
+};
+
+static const struct i2c_algorithm ljca_i2c_algo = {
+	.master_xfer = ljca_i2c_xfer,
+	.functionality = ljca_i2c_func,
+};
+
+static int ljca_i2c_probe(struct auxiliary_device *auxdev,
+			  const struct auxiliary_device_id *aux_dev_id)
+{
+	struct ljca_client *ljca = auxiliary_dev_to_ljca_client(auxdev);
+	struct ljca_i2c_dev *ljca_i2c;
+	int ret;
+
+	ljca_i2c = devm_kzalloc(&auxdev->dev, sizeof(*ljca_i2c), GFP_KERNEL);
+	if (!ljca_i2c)
+		return -ENOMEM;
+
+	ljca_i2c->ljca = ljca;
+	ljca_i2c->i2c_info = dev_get_platdata(&auxdev->dev);
+
+	ljca_i2c->adap.owner = THIS_MODULE;
+	ljca_i2c->adap.class = I2C_CLASS_HWMON;
+	ljca_i2c->adap.algo = &ljca_i2c_algo;
+	ljca_i2c->adap.quirks = &ljca_i2c_quirks;
+	ljca_i2c->adap.dev.parent = &auxdev->dev;
+
+	snprintf(ljca_i2c->adap.name, sizeof(ljca_i2c->adap.name), "%s-%s-%d",
+		 dev_name(&auxdev->dev), dev_name(auxdev->dev.parent),
+		 ljca_i2c->i2c_info->id);
+
+	device_set_node(&ljca_i2c->adap.dev, dev_fwnode(&auxdev->dev));
+
+	i2c_set_adapdata(&ljca_i2c->adap, ljca_i2c);
+	auxiliary_set_drvdata(auxdev, ljca_i2c);
+
+	ret = ljca_i2c_init(ljca_i2c, ljca_i2c->i2c_info->id);
+	if (ret)
+		return dev_err_probe(&auxdev->dev, -EIO,
+				     "i2c init failed id: %d\n",
+				     ljca_i2c->i2c_info->id);
+
+	ret = devm_i2c_add_adapter(&auxdev->dev, &ljca_i2c->adap);
+	if (ret)
+		return ret;
+
+	if (has_acpi_companion(&ljca_i2c->adap.dev))
+		acpi_dev_clear_dependencies(ACPI_COMPANION(&ljca_i2c->adap.dev));
+
+	return 0;
+}
+
+static void ljca_i2c_remove(struct auxiliary_device *auxdev)
+{
+	struct ljca_i2c_dev *ljca_i2c = auxiliary_get_drvdata(auxdev);
+
+	i2c_del_adapter(&ljca_i2c->adap);
+}
+
+static const struct auxiliary_device_id ljca_i2c_id_table[] = {
+	{ "usb_ljca.ljca-i2c", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(auxiliary, ljca_i2c_id_table);
+
+static struct auxiliary_driver ljca_i2c_driver = {
+	.probe = ljca_i2c_probe,
+	.remove = ljca_i2c_remove,
+	.id_table = ljca_i2c_id_table,
+};
+module_auxiliary_driver(ljca_i2c_driver);
+
+MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>");
+MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>");
+MODULE_AUTHOR("Lixu Zhang <lixu.zhang@intel.com>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB-I2C driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LJCA);
-- 
2.7.4

[PATCH v19 3/4] spi: Add support for Intel LJCA USB SPI driver

[Wentong Wu]

Implements the SPI function of Intel USB-I2C/GPIO/SPI adapter device
named "La Jolla Cove Adapter" (LJCA). It communicate with LJCA SPI
module with specific protocol through interfaces exported by LJCA USB
driver.

Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Reviewed-by: Andi Shyti <andi.shyti@linux.intel.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/spi/Kconfig    |  11 ++
 drivers/spi/Makefile   |   1 +
 drivers/spi/spi-ljca.c | 297 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 309 insertions(+)
 create mode 100644 drivers/spi/spi-ljca.c

diff --git a/drivers/spi/Kconfig b/drivers/spi/Kconfig
index 2c21d5b..32d9ea6 100644
--- a/drivers/spi/Kconfig
+++ b/drivers/spi/Kconfig
@@ -616,6 +616,17 @@ config SPI_FSL_ESPI
 	  From MPC8536, 85xx platform uses the controller, and all P10xx,
 	  P20xx, P30xx,P40xx, P50xx uses this controller.
 
+config SPI_LJCA
+	tristate "Intel La Jolla Cove Adapter SPI support"
+	depends on USB_LJCA
+	default USB_LJCA
+	help
+	  Select this option to enable SPI driver for the Intel
+	  La Jolla Cove Adapter (LJCA) board.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called spi-ljca.
+
 config SPI_MESON_SPICC
 	tristate "Amlogic Meson SPICC controller"
 	depends on COMMON_CLK
diff --git a/drivers/spi/Makefile b/drivers/spi/Makefile
index 6af5484..4ff8d72 100644
--- a/drivers/spi/Makefile
+++ b/drivers/spi/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_SPI_INTEL_PCI)		+= spi-intel-pci.o
 obj-$(CONFIG_SPI_INTEL_PLATFORM)	+= spi-intel-platform.o
 obj-$(CONFIG_SPI_LANTIQ_SSC)		+= spi-lantiq-ssc.o
 obj-$(CONFIG_SPI_JCORE)			+= spi-jcore.o
+obj-$(CONFIG_SPI_LJCA)			+= spi-ljca.o
 obj-$(CONFIG_SPI_LM70_LLP)		+= spi-lm70llp.o
 obj-$(CONFIG_SPI_LOONGSON_CORE)		+= spi-loongson-core.o
 obj-$(CONFIG_SPI_LOONGSON_PCI)		+= spi-loongson-pci.o
diff --git a/drivers/spi/spi-ljca.c b/drivers/spi/spi-ljca.c
new file mode 100644
index 0000000..6b6d000d
--- /dev/null
+++ b/drivers/spi/spi-ljca.c
@@ -0,0 +1,297 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Intel La Jolla Cove Adapter USB-SPI driver
+ *
+ * Copyright (c) 2023, Intel Corporation.
+ */
+
+#include <linux/auxiliary_bus.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/dev_printk.h>
+#include <linux/module.h>
+#include <linux/spi/spi.h>
+#include <linux/usb/ljca.h>
+
+#define LJCA_SPI_BUS_MAX_HZ		48000000
+
+#define LJCA_SPI_BUF_SIZE		60u
+#define LJCA_SPI_MAX_XFER_SIZE		\
+	(LJCA_SPI_BUF_SIZE - sizeof(struct ljca_spi_xfer_packet))
+
+#define LJCA_SPI_CLK_MODE_POLARITY	BIT(0)
+#define LJCA_SPI_CLK_MODE_PHASE		BIT(1)
+
+#define LJCA_SPI_XFER_INDICATOR_ID	GENMASK(5, 0)
+#define LJCA_SPI_XFER_INDICATOR_CMPL	BIT(6)
+#define LJCA_SPI_XFER_INDICATOR_INDEX	BIT(7)
+
+/* SPI commands */
+enum ljca_spi_cmd {
+	LJCA_SPI_INIT = 1,
+	LJCA_SPI_READ,
+	LJCA_SPI_WRITE,
+	LJCA_SPI_WRITEREAD,
+	LJCA_SPI_DEINIT,
+};
+
+enum {
+	LJCA_SPI_BUS_SPEED_24M,
+	LJCA_SPI_BUS_SPEED_12M,
+	LJCA_SPI_BUS_SPEED_8M,
+	LJCA_SPI_BUS_SPEED_6M,
+	LJCA_SPI_BUS_SPEED_4_8M, /*4.8MHz*/
+	LJCA_SPI_BUS_SPEED_MIN = LJCA_SPI_BUS_SPEED_4_8M,
+};
+
+enum {
+	LJCA_SPI_CLOCK_LOW_POLARITY,
+	LJCA_SPI_CLOCK_HIGH_POLARITY,
+};
+
+enum {
+	LJCA_SPI_CLOCK_FIRST_PHASE,
+	LJCA_SPI_CLOCK_SECOND_PHASE,
+};
+
+struct ljca_spi_init_packet {
+	u8 index;
+	u8 speed;
+	u8 mode;
+} __packed;
+
+struct ljca_spi_xfer_packet {
+	u8 indicator;
+	u8 len;
+	u8 data[];
+} __packed;
+
+struct ljca_spi_dev {
+	struct ljca_client *ljca;
+	struct spi_controller *controller;
+	struct ljca_spi_info *spi_info;
+	u8 speed;
+	u8 mode;
+
+	u8 obuf[LJCA_SPI_BUF_SIZE];
+	u8 ibuf[LJCA_SPI_BUF_SIZE];
+};
+
+static int ljca_spi_read_write(struct ljca_spi_dev *ljca_spi, const u8 *w_data,
+			       u8 *r_data, int len, int id, int complete,
+			       int cmd)
+{
+	struct ljca_spi_xfer_packet *w_packet =
+			(struct ljca_spi_xfer_packet *)ljca_spi->obuf;
+	struct ljca_spi_xfer_packet *r_packet =
+			(struct ljca_spi_xfer_packet *)ljca_spi->ibuf;
+	int ret;
+
+	w_packet->indicator = FIELD_PREP(LJCA_SPI_XFER_INDICATOR_ID, id) |
+			      FIELD_PREP(LJCA_SPI_XFER_INDICATOR_CMPL, complete) |
+			      FIELD_PREP(LJCA_SPI_XFER_INDICATOR_INDEX,
+					 ljca_spi->spi_info->id);
+
+	if (cmd == LJCA_SPI_READ) {
+		w_packet->len = sizeof(u16);
+		*(__le16 *)&w_packet->data[0] = cpu_to_le16(len);
+	} else {
+		w_packet->len = len;
+		memcpy(w_packet->data, w_data, len);
+	}
+
+	ret = ljca_transfer(ljca_spi->ljca, cmd, (u8 *)w_packet,
+			    struct_size(w_packet, data, w_packet->len),
+			    (u8 *)r_packet, LJCA_SPI_BUF_SIZE);
+	if (ret < 0)
+		return ret;
+	else if (ret < sizeof(*r_packet) || r_packet->len <= 0)
+		return -EIO;
+
+	if (r_data)
+		memcpy(r_data, r_packet->data, r_packet->len);
+
+	return 0;
+}
+
+static int ljca_spi_init(struct ljca_spi_dev *ljca_spi, u8 div, u8 mode)
+{
+	struct ljca_spi_init_packet w_packet = {};
+	int ret;
+
+	if (ljca_spi->mode == mode && ljca_spi->speed == div)
+		return 0;
+
+	w_packet.index = ljca_spi->spi_info->id;
+	w_packet.speed = div;
+	w_packet.mode = FIELD_PREP(LJCA_SPI_CLK_MODE_POLARITY,
+				   (mode & SPI_CPOL) ? LJCA_SPI_CLOCK_HIGH_POLARITY :
+						       LJCA_SPI_CLOCK_LOW_POLARITY) |
+			FIELD_PREP(LJCA_SPI_CLK_MODE_PHASE,
+				   (mode & SPI_CPHA) ? LJCA_SPI_CLOCK_SECOND_PHASE :
+						       LJCA_SPI_CLOCK_FIRST_PHASE);
+
+	ret = ljca_transfer(ljca_spi->ljca, LJCA_SPI_INIT, (u8 *)&w_packet,
+			    sizeof(w_packet), NULL, 0);
+	if (ret < 0)
+		return ret;
+
+	ljca_spi->mode = mode;
+	ljca_spi->speed = div;
+
+	return 0;
+}
+
+static int ljca_spi_deinit(struct ljca_spi_dev *ljca_spi)
+{
+	struct ljca_spi_init_packet w_packet = {};
+	int ret;
+
+	w_packet.index = ljca_spi->spi_info->id;
+
+	ret = ljca_transfer(ljca_spi->ljca, LJCA_SPI_DEINIT, (u8 *)&w_packet,
+			    sizeof(w_packet), NULL, 0);
+
+	return ret < 0 ? ret : 0;
+}
+
+static inline int ljca_spi_transfer(struct ljca_spi_dev *ljca_spi,
+				    const u8 *tx_data, u8 *rx_data, u16 len)
+{
+	int complete, cur_len;
+	int remaining = len;
+	int cmd, ret, i;
+	int offset = 0;
+
+	if (tx_data && rx_data)
+		cmd = LJCA_SPI_WRITEREAD;
+	else if (tx_data)
+		cmd = LJCA_SPI_WRITE;
+	else if (rx_data)
+		cmd = LJCA_SPI_READ;
+	else
+		return -EINVAL;
+
+	for (i = 0; remaining > 0; i++) {
+		cur_len = min_t(unsigned int, remaining, LJCA_SPI_MAX_XFER_SIZE);
+		complete = (cur_len == remaining);
+
+		ret = ljca_spi_read_write(ljca_spi,
+					  tx_data ? tx_data + offset : NULL,
+					  rx_data ? rx_data + offset : NULL,
+					  cur_len, i, complete, cmd);
+		if (ret)
+			return ret;
+
+		offset += cur_len;
+		remaining -= cur_len;
+	}
+
+	return 0;
+}
+
+static int ljca_spi_transfer_one(struct spi_controller *controller,
+				 struct spi_device *spi,
+				 struct spi_transfer *xfer)
+{
+	u8 div = DIV_ROUND_UP(controller->max_speed_hz, xfer->speed_hz) / 2 - 1;
+	struct ljca_spi_dev *ljca_spi = spi_controller_get_devdata(controller);
+	int ret;
+
+	div = min_t(u8, LJCA_SPI_BUS_SPEED_MIN, div);
+
+	ret = ljca_spi_init(ljca_spi, div, spi->mode);
+	if (ret) {
+		dev_err(&ljca_spi->ljca->auxdev.dev,
+			"cannot initialize transfer ret %d\n", ret);
+		return ret;
+	}
+
+	ret = ljca_spi_transfer(ljca_spi, xfer->tx_buf, xfer->rx_buf, xfer->len);
+	if (ret)
+		dev_err(&ljca_spi->ljca->auxdev.dev,
+			"transfer failed len: %d\n", xfer->len);
+
+	return ret;
+}
+
+static int ljca_spi_probe(struct auxiliary_device *auxdev,
+			  const struct auxiliary_device_id *aux_dev_id)
+{
+	struct ljca_client *ljca = auxiliary_dev_to_ljca_client(auxdev);
+	struct spi_controller *controller;
+	struct ljca_spi_dev *ljca_spi;
+	int ret;
+
+	controller = devm_spi_alloc_master(&auxdev->dev, sizeof(*ljca_spi));
+	if (!controller)
+		return -ENOMEM;
+
+	ljca_spi = spi_controller_get_devdata(controller);
+	ljca_spi->ljca = ljca;
+	ljca_spi->spi_info = dev_get_platdata(&auxdev->dev);
+	ljca_spi->controller = controller;
+
+	controller->bus_num = -1;
+	controller->mode_bits = SPI_CPHA | SPI_CPOL;
+	controller->transfer_one = ljca_spi_transfer_one;
+	controller->auto_runtime_pm = false;
+	controller->max_speed_hz = LJCA_SPI_BUS_MAX_HZ;
+
+	device_set_node(&ljca_spi->controller->dev, dev_fwnode(&auxdev->dev));
+	auxiliary_set_drvdata(auxdev, controller);
+
+	ret = spi_register_controller(controller);
+	if (ret)
+		dev_err(&auxdev->dev, "Failed to register controller\n");
+
+	return ret;
+}
+
+static void ljca_spi_dev_remove(struct auxiliary_device *auxdev)
+{
+	struct spi_controller *controller = auxiliary_get_drvdata(auxdev);
+	struct ljca_spi_dev *ljca_spi = spi_controller_get_devdata(controller);
+
+	spi_unregister_controller(controller);
+	ljca_spi_deinit(ljca_spi);
+}
+
+static int ljca_spi_dev_suspend(struct device *dev)
+{
+	struct spi_controller *controller = dev_get_drvdata(dev);
+
+	return spi_controller_suspend(controller);
+}
+
+static int ljca_spi_dev_resume(struct device *dev)
+{
+	struct spi_controller *controller = dev_get_drvdata(dev);
+
+	return spi_controller_resume(controller);
+}
+
+static const struct dev_pm_ops ljca_spi_pm = {
+	SYSTEM_SLEEP_PM_OPS(ljca_spi_dev_suspend, ljca_spi_dev_resume)
+};
+
+static const struct auxiliary_device_id ljca_spi_id_table[] = {
+	{ "usb_ljca.ljca-spi", 0 },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(auxiliary, ljca_spi_id_table);
+
+static struct auxiliary_driver ljca_spi_driver = {
+	.driver.pm	= &ljca_spi_pm,
+	.probe		= ljca_spi_probe,
+	.remove		= ljca_spi_dev_remove,
+	.id_table	= ljca_spi_id_table,
+};
+module_auxiliary_driver(ljca_spi_driver);
+
+MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>");
+MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>");
+MODULE_AUTHOR("Lixu Zhang <lixu.zhang@intel.com>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB-SPI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LJCA);
-- 
2.7.4

[PATCH v19 4/4] gpio: update Intel LJCA USB GPIO driver

[Wentong Wu]

This driver communicate with LJCA GPIO module with specific
protocol through interfaces exported by LJCA USB driver.
Update the driver according to LJCA USB driver's changes.

Signed-off-by: Wentong Wu <wentong.wu@intel.com>
Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
Acked-by: Linus Walleij <linus.walleij@linaro.org>
Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
Tested-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/gpio/Kconfig     |   4 +-
 drivers/gpio/gpio-ljca.c | 246 +++++++++++++++++++++++++++--------------------
 2 files changed, 145 insertions(+), 105 deletions(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 673bafb..8d5b6c3 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -1312,9 +1312,9 @@ config GPIO_KEMPLD
 
 config GPIO_LJCA
 	tristate "INTEL La Jolla Cove Adapter GPIO support"
-	depends on MFD_LJCA
+	depends on USB_LJCA
 	select GPIOLIB_IRQCHIP
-	default MFD_LJCA
+	default USB_LJCA
 	help
 	  Select this option to enable GPIO driver for the INTEL
 	  La Jolla Cove Adapter (LJCA) board.
diff --git a/drivers/gpio/gpio-ljca.c b/drivers/gpio/gpio-ljca.c
index 87863f0..7fae26d 100644
--- a/drivers/gpio/gpio-ljca.c
+++ b/drivers/gpio/gpio-ljca.c
@@ -6,6 +6,7 @@
  */
 
 #include <linux/acpi.h>
+#include <linux/auxiliary_bus.h>
 #include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/dev_printk.h>
@@ -13,19 +14,18 @@
 #include <linux/irq.h>
 #include <linux/kernel.h>
 #include <linux/kref.h>
-#include <linux/mfd/ljca.h>
 #include <linux/module.h>
-#include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#include <linux/usb/ljca.h>
 
 /* GPIO commands */
-#define LJCA_GPIO_CONFIG	1
-#define LJCA_GPIO_READ		2
-#define LJCA_GPIO_WRITE		3
-#define LJCA_GPIO_INT_EVENT	4
-#define LJCA_GPIO_INT_MASK	5
-#define LJCA_GPIO_INT_UNMASK	6
+#define LJCA_GPIO_CONFIG		1
+#define LJCA_GPIO_READ			2
+#define LJCA_GPIO_WRITE			3
+#define LJCA_GPIO_INT_EVENT		4
+#define LJCA_GPIO_INT_MASK		5
+#define LJCA_GPIO_INT_UNMASK		6
 
 #define LJCA_GPIO_CONF_DISABLE		BIT(0)
 #define LJCA_GPIO_CONF_INPUT		BIT(1)
@@ -36,45 +36,49 @@
 #define LJCA_GPIO_CONF_INTERRUPT	BIT(6)
 #define LJCA_GPIO_INT_TYPE		BIT(7)
 
-#define LJCA_GPIO_CONF_EDGE	FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
-#define LJCA_GPIO_CONF_LEVEL	FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
+#define LJCA_GPIO_CONF_EDGE		FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
+#define LJCA_GPIO_CONF_LEVEL		FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
 
 /* Intentional overlap with PULLUP / PULLDOWN */
-#define LJCA_GPIO_CONF_SET	BIT(3)
-#define LJCA_GPIO_CONF_CLR	BIT(4)
+#define LJCA_GPIO_CONF_SET		BIT(3)
+#define LJCA_GPIO_CONF_CLR		BIT(4)
 
-struct gpio_op {
+#define LJCA_GPIO_BUF_SIZE		60u
+
+struct ljca_gpio_op {
 	u8 index;
 	u8 value;
 } __packed;
 
-struct gpio_packet {
+struct ljca_gpio_packet {
 	u8 num;
-	struct gpio_op item[];
+	struct ljca_gpio_op item[];
 } __packed;
 
-#define LJCA_GPIO_BUF_SIZE 60
 struct ljca_gpio_dev {
-	struct platform_device *pdev;
+	struct ljca_client *ljca;
 	struct gpio_chip gc;
 	struct ljca_gpio_info *gpio_info;
 	DECLARE_BITMAP(unmasked_irqs, LJCA_MAX_GPIO_NUM);
 	DECLARE_BITMAP(enabled_irqs, LJCA_MAX_GPIO_NUM);
 	DECLARE_BITMAP(reenable_irqs, LJCA_MAX_GPIO_NUM);
+	DECLARE_BITMAP(output_enabled, LJCA_MAX_GPIO_NUM);
 	u8 *connect_mode;
-	/* mutex to protect irq bus */
+	/* protect irq bus */
 	struct mutex irq_lock;
 	struct work_struct work;
-	/* lock to protect package transfer to Hardware */
+	/* protect package transfer to hardware */
 	struct mutex trans_lock;
 
 	u8 obuf[LJCA_GPIO_BUF_SIZE];
 	u8 ibuf[LJCA_GPIO_BUF_SIZE];
 };
 
-static int gpio_config(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id, u8 config)
+static int ljca_gpio_config(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id,
+			    u8 config)
 {
-	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
+	struct ljca_gpio_packet *packet =
+				(struct ljca_gpio_packet *)ljca_gpio->obuf;
 	int ret;
 
 	mutex_lock(&ljca_gpio->trans_lock);
@@ -82,43 +86,43 @@ static int gpio_config(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id, u8 config)
 	packet->item[0].value = config | ljca_gpio->connect_mode[gpio_id];
 	packet->num = 1;
 
-	ret = ljca_transfer(ljca_gpio->gpio_info->ljca, LJCA_GPIO_CONFIG, packet,
-			    struct_size(packet, item, packet->num), NULL, NULL);
+	ret = ljca_transfer(ljca_gpio->ljca, LJCA_GPIO_CONFIG, (u8 *)packet,
+			    struct_size(packet, item, packet->num), NULL, 0);
 	mutex_unlock(&ljca_gpio->trans_lock);
-	return ret;
+
+	return ret < 0 ? ret : 0;
 }
 
 static int ljca_gpio_read(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id)
 {
-	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
-	struct gpio_packet *ack_packet = (struct gpio_packet *)ljca_gpio->ibuf;
-	unsigned int ibuf_len = LJCA_GPIO_BUF_SIZE;
+	struct ljca_gpio_packet *ack_packet =
+				(struct ljca_gpio_packet *)ljca_gpio->ibuf;
+	struct ljca_gpio_packet *packet =
+				(struct ljca_gpio_packet *)ljca_gpio->obuf;
 	int ret;
 
 	mutex_lock(&ljca_gpio->trans_lock);
 	packet->num = 1;
 	packet->item[0].index = gpio_id;
-	ret = ljca_transfer(ljca_gpio->gpio_info->ljca, LJCA_GPIO_READ, packet,
-			    struct_size(packet, item, packet->num), ljca_gpio->ibuf, &ibuf_len);
-	if (ret)
-		goto out_unlock;
-
-	if (!ibuf_len || ack_packet->num != packet->num) {
-		dev_err(&ljca_gpio->pdev->dev, "failed gpio_id:%u %u", gpio_id, ack_packet->num);
-		ret = -EIO;
+	ret = ljca_transfer(ljca_gpio->ljca, LJCA_GPIO_READ, (u8 *)packet,
+			    struct_size(packet, item, packet->num),
+			    ljca_gpio->ibuf, LJCA_GPIO_BUF_SIZE);
+
+	if (ret <= 0 || ack_packet->num != packet->num) {
+		dev_err(&ljca_gpio->ljca->auxdev.dev,
+			"read package error, gpio_id: %u num: %u ret: %d\n",
+			gpio_id, ack_packet->num, ret);
+		ret = ret < 0 ? ret : -EIO;
 	}
-
-out_unlock:
 	mutex_unlock(&ljca_gpio->trans_lock);
-	if (ret)
-		return ret;
-	return ack_packet->item[0].value > 0;
+
+	return ret < 0 ? ret : ack_packet->item[0].value > 0;
 }
 
-static int ljca_gpio_write(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id,
-			   int value)
+static int ljca_gpio_write(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id, int value)
 {
-	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
+	struct ljca_gpio_packet *packet =
+			(struct ljca_gpio_packet *)ljca_gpio->obuf;
 	int ret;
 
 	mutex_lock(&ljca_gpio->trans_lock);
@@ -126,10 +130,11 @@ static int ljca_gpio_write(struct ljca_gpio_dev *ljca_gpio, u8 gpio_id,
 	packet->item[0].index = gpio_id;
 	packet->item[0].value = value & 1;
 
-	ret = ljca_transfer(ljca_gpio->gpio_info->ljca, LJCA_GPIO_WRITE, packet,
-			    struct_size(packet, item, packet->num), NULL, NULL);
+	ret = ljca_transfer(ljca_gpio->ljca, LJCA_GPIO_WRITE, (u8 *)packet,
+			    struct_size(packet, item, packet->num), NULL, 0);
 	mutex_unlock(&ljca_gpio->trans_lock);
-	return ret;
+
+	return ret < 0 ? ret : 0;
 }
 
 static int ljca_gpio_get_value(struct gpio_chip *chip, unsigned int offset)
@@ -147,16 +152,24 @@ static void ljca_gpio_set_value(struct gpio_chip *chip, unsigned int offset,
 
 	ret = ljca_gpio_write(ljca_gpio, offset, val);
 	if (ret)
-		dev_err(chip->parent, "offset:%u val:%d set value failed %d\n", offset, val, ret);
+		dev_err(chip->parent,
+			"set value failed offset: %u val: %d ret: %d\n",
+			offset, val, ret);
 }
 
-static int ljca_gpio_direction_input(struct gpio_chip *chip,
-				     unsigned int offset)
+static int ljca_gpio_direction_input(struct gpio_chip *chip, unsigned int offset)
 {
 	struct ljca_gpio_dev *ljca_gpio = gpiochip_get_data(chip);
 	u8 config = LJCA_GPIO_CONF_INPUT | LJCA_GPIO_CONF_CLR;
+	int ret;
 
-	return gpio_config(ljca_gpio, offset, config);
+	ret = ljca_gpio_config(ljca_gpio, offset, config);
+	if (ret)
+		return ret;
+
+	clear_bit(offset, ljca_gpio->output_enabled);
+
+	return 0;
 }
 
 static int ljca_gpio_direction_output(struct gpio_chip *chip,
@@ -166,14 +179,26 @@ static int ljca_gpio_direction_output(struct gpio_chip *chip,
 	u8 config = LJCA_GPIO_CONF_OUTPUT | LJCA_GPIO_CONF_CLR;
 	int ret;
 
-	ret = gpio_config(ljca_gpio, offset, config);
+	ret = ljca_gpio_config(ljca_gpio, offset, config);
 	if (ret)
 		return ret;
 
 	ljca_gpio_set_value(chip, offset, val);
+	set_bit(offset, ljca_gpio->output_enabled);
+
 	return 0;
 }
 
+static int ljca_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
+{
+	struct ljca_gpio_dev *ljca_gpio = gpiochip_get_data(chip);
+
+	if (test_bit(offset, ljca_gpio->output_enabled))
+		return GPIO_LINE_DIRECTION_OUT;
+
+	return GPIO_LINE_DIRECTION_IN;
+}
+
 static int ljca_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 				unsigned long config)
 {
@@ -197,7 +222,8 @@ static int ljca_gpio_set_config(struct gpio_chip *chip, unsigned int offset,
 	return 0;
 }
 
-static int ljca_gpio_init_valid_mask(struct gpio_chip *chip, unsigned long *valid_mask,
+static int ljca_gpio_init_valid_mask(struct gpio_chip *chip,
+				     unsigned long *valid_mask,
 				     unsigned int ngpios)
 {
 	struct ljca_gpio_dev *ljca_gpio = gpiochip_get_data(chip);
@@ -208,15 +234,18 @@ static int ljca_gpio_init_valid_mask(struct gpio_chip *chip, unsigned long *vali
 	return 0;
 }
 
-static void ljca_gpio_irq_init_valid_mask(struct gpio_chip *chip, unsigned long *valid_mask,
+static void ljca_gpio_irq_init_valid_mask(struct gpio_chip *chip,
+					  unsigned long *valid_mask,
 					  unsigned int ngpios)
 {
 	ljca_gpio_init_valid_mask(chip, valid_mask, ngpios);
 }
 
-static int ljca_enable_irq(struct ljca_gpio_dev *ljca_gpio, int gpio_id, bool enable)
+static int ljca_enable_irq(struct ljca_gpio_dev *ljca_gpio, int gpio_id,
+			   bool enable)
 {
-	struct gpio_packet *packet = (struct gpio_packet *)ljca_gpio->obuf;
+	struct ljca_gpio_packet *packet =
+			(struct ljca_gpio_packet *)ljca_gpio->obuf;
 	int ret;
 
 	mutex_lock(&ljca_gpio->trans_lock);
@@ -224,18 +253,20 @@ static int ljca_enable_irq(struct ljca_gpio_dev *ljca_gpio, int gpio_id, bool en
 	packet->item[0].index = gpio_id;
 	packet->item[0].value = 0;
 
-	ret = ljca_transfer(ljca_gpio->gpio_info->ljca,
-			    enable ? LJCA_GPIO_INT_UNMASK : LJCA_GPIO_INT_MASK, packet,
-			    struct_size(packet, item, packet->num), NULL, NULL);
+	ret = ljca_transfer(ljca_gpio->ljca,
+			    enable ? LJCA_GPIO_INT_UNMASK : LJCA_GPIO_INT_MASK,
+			    (u8 *)packet, struct_size(packet, item, packet->num),
+			    NULL, 0);
 	mutex_unlock(&ljca_gpio->trans_lock);
-	return ret;
+
+	return ret < 0 ? ret : 0;
 }
 
 static void ljca_gpio_async(struct work_struct *work)
 {
-	struct ljca_gpio_dev *ljca_gpio = container_of(work, struct ljca_gpio_dev, work);
-	int gpio_id;
-	int unmasked;
+	struct ljca_gpio_dev *ljca_gpio =
+			container_of(work, struct ljca_gpio_dev, work);
+	int gpio_id, unmasked;
 
 	for_each_set_bit(gpio_id, ljca_gpio->reenable_irqs, ljca_gpio->gc.ngpio) {
 		clear_bit(gpio_id, ljca_gpio->reenable_irqs);
@@ -245,20 +276,22 @@ static void ljca_gpio_async(struct work_struct *work)
 	}
 }
 
-static void ljca_gpio_event_cb(void *context, u8 cmd, const void *evt_data, int len)
+static void ljca_gpio_event_cb(void *context, u8 cmd, const void *evt_data,
+			       int len)
 {
-	const struct gpio_packet *packet = evt_data;
+	const struct ljca_gpio_packet *packet = evt_data;
 	struct ljca_gpio_dev *ljca_gpio = context;
-	int i;
-	int irq;
+	int i, irq;
 
 	if (cmd != LJCA_GPIO_INT_EVENT)
 		return;
 
 	for (i = 0; i < packet->num; i++) {
-		irq = irq_find_mapping(ljca_gpio->gc.irq.domain, packet->item[i].index);
+		irq = irq_find_mapping(ljca_gpio->gc.irq.domain,
+				       packet->item[i].index);
 		if (!irq) {
-			dev_err(ljca_gpio->gc.parent, "gpio_id %u does not mapped to IRQ yet\n",
+			dev_err(ljca_gpio->gc.parent,
+				"gpio_id %u does not mapped to IRQ yet\n",
 				packet->item[i].index);
 			return;
 		}
@@ -299,18 +332,22 @@ static int ljca_irq_set_type(struct irq_data *irqd, unsigned int type)
 	ljca_gpio->connect_mode[gpio_id] = LJCA_GPIO_CONF_INTERRUPT;
 	switch (type) {
 	case IRQ_TYPE_LEVEL_HIGH:
-		ljca_gpio->connect_mode[gpio_id] |= (LJCA_GPIO_CONF_LEVEL | LJCA_GPIO_CONF_PULLUP);
+		ljca_gpio->connect_mode[gpio_id] |=
+			(LJCA_GPIO_CONF_LEVEL | LJCA_GPIO_CONF_PULLUP);
 		break;
 	case IRQ_TYPE_LEVEL_LOW:
-		ljca_gpio->connect_mode[gpio_id] |= (LJCA_GPIO_CONF_LEVEL | LJCA_GPIO_CONF_PULLDOWN);
+		ljca_gpio->connect_mode[gpio_id] |=
+			(LJCA_GPIO_CONF_LEVEL | LJCA_GPIO_CONF_PULLDOWN);
 		break;
 	case IRQ_TYPE_EDGE_BOTH:
 		break;
 	case IRQ_TYPE_EDGE_RISING:
-		ljca_gpio->connect_mode[gpio_id] |= (LJCA_GPIO_CONF_EDGE | LJCA_GPIO_CONF_PULLUP);
+		ljca_gpio->connect_mode[gpio_id] |=
+			(LJCA_GPIO_CONF_EDGE | LJCA_GPIO_CONF_PULLUP);
 		break;
 	case IRQ_TYPE_EDGE_FALLING:
-		ljca_gpio->connect_mode[gpio_id] |= (LJCA_GPIO_CONF_EDGE | LJCA_GPIO_CONF_PULLDOWN);
+		ljca_gpio->connect_mode[gpio_id] |=
+			(LJCA_GPIO_CONF_EDGE | LJCA_GPIO_CONF_PULLDOWN);
 		break;
 	default:
 		return -EINVAL;
@@ -332,15 +369,14 @@ static void ljca_irq_bus_unlock(struct irq_data *irqd)
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(irqd);
 	struct ljca_gpio_dev *ljca_gpio = gpiochip_get_data(gc);
 	int gpio_id = irqd_to_hwirq(irqd);
-	int enabled;
-	int unmasked;
+	int enabled, unmasked;
 
 	enabled = test_bit(gpio_id, ljca_gpio->enabled_irqs);
 	unmasked = test_bit(gpio_id, ljca_gpio->unmasked_irqs);
 
 	if (enabled != unmasked) {
 		if (unmasked) {
-			gpio_config(ljca_gpio, gpio_id, 0);
+			ljca_gpio_config(ljca_gpio, gpio_id, 0);
 			ljca_enable_irq(ljca_gpio, gpio_id, true);
 			set_bit(gpio_id, ljca_gpio->enabled_irqs);
 		} else {
@@ -363,43 +399,48 @@ static const struct irq_chip ljca_gpio_irqchip = {
 	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
 
-static int ljca_gpio_probe(struct platform_device *pdev)
+static int ljca_gpio_probe(struct auxiliary_device *auxdev,
+			   const struct auxiliary_device_id *aux_dev_id)
 {
+	struct ljca_client *ljca = auxiliary_dev_to_ljca_client(auxdev);
 	struct ljca_gpio_dev *ljca_gpio;
 	struct gpio_irq_chip *girq;
 	int ret;
 
-	ljca_gpio = devm_kzalloc(&pdev->dev, sizeof(*ljca_gpio), GFP_KERNEL);
+	ljca_gpio = devm_kzalloc(&auxdev->dev, sizeof(*ljca_gpio), GFP_KERNEL);
 	if (!ljca_gpio)
 		return -ENOMEM;
 
-	ljca_gpio->gpio_info = dev_get_platdata(&pdev->dev);
-	ljca_gpio->connect_mode = devm_kcalloc(&pdev->dev, ljca_gpio->gpio_info->num,
-					       sizeof(*ljca_gpio->connect_mode), GFP_KERNEL);
+	ljca_gpio->ljca = ljca;
+	ljca_gpio->gpio_info = dev_get_platdata(&auxdev->dev);
+	ljca_gpio->connect_mode = devm_kcalloc(&auxdev->dev,
+					       ljca_gpio->gpio_info->num,
+					       sizeof(*ljca_gpio->connect_mode),
+					       GFP_KERNEL);
 	if (!ljca_gpio->connect_mode)
 		return -ENOMEM;
 
 	mutex_init(&ljca_gpio->irq_lock);
 	mutex_init(&ljca_gpio->trans_lock);
-	ljca_gpio->pdev = pdev;
 	ljca_gpio->gc.direction_input = ljca_gpio_direction_input;
 	ljca_gpio->gc.direction_output = ljca_gpio_direction_output;
+	ljca_gpio->gc.get_direction = ljca_gpio_get_direction;
 	ljca_gpio->gc.get = ljca_gpio_get_value;
 	ljca_gpio->gc.set = ljca_gpio_set_value;
 	ljca_gpio->gc.set_config = ljca_gpio_set_config;
 	ljca_gpio->gc.init_valid_mask = ljca_gpio_init_valid_mask;
 	ljca_gpio->gc.can_sleep = true;
-	ljca_gpio->gc.parent = &pdev->dev;
+	ljca_gpio->gc.parent = &auxdev->dev;
 
 	ljca_gpio->gc.base = -1;
 	ljca_gpio->gc.ngpio = ljca_gpio->gpio_info->num;
-	ljca_gpio->gc.label = ACPI_COMPANION(&pdev->dev) ?
-			      acpi_dev_name(ACPI_COMPANION(&pdev->dev)) :
-			      dev_name(&pdev->dev);
+	ljca_gpio->gc.label = ACPI_COMPANION(&auxdev->dev) ?
+			      acpi_dev_name(ACPI_COMPANION(&auxdev->dev)) :
+			      dev_name(&auxdev->dev);
 	ljca_gpio->gc.owner = THIS_MODULE;
 
-	platform_set_drvdata(pdev, ljca_gpio);
-	ljca_register_event_cb(ljca_gpio->gpio_info->ljca, ljca_gpio_event_cb, ljca_gpio);
+	auxiliary_set_drvdata(auxdev, ljca_gpio);
+	ljca_register_event_cb(ljca, ljca_gpio_event_cb, ljca_gpio);
 
 	girq = &ljca_gpio->gc.irq;
 	gpio_irq_chip_set_chip(girq, &ljca_gpio_irqchip);
@@ -413,7 +454,7 @@ static int ljca_gpio_probe(struct platform_device *pdev)
 	INIT_WORK(&ljca_gpio->work, ljca_gpio_async);
 	ret = gpiochip_add_data(&ljca_gpio->gc, ljca_gpio);
 	if (ret) {
-		ljca_unregister_event_cb(ljca_gpio->gpio_info->ljca);
+		ljca_unregister_event_cb(ljca);
 		mutex_destroy(&ljca_gpio->irq_lock);
 		mutex_destroy(&ljca_gpio->trans_lock);
 	}
@@ -421,34 +462,33 @@ static int ljca_gpio_probe(struct platform_device *pdev)
 	return ret;
 }
 
-static int ljca_gpio_remove(struct platform_device *pdev)
+static void ljca_gpio_remove(struct auxiliary_device *auxdev)
 {
-	struct ljca_gpio_dev *ljca_gpio = platform_get_drvdata(pdev);
+	struct ljca_gpio_dev *ljca_gpio = auxiliary_get_drvdata(auxdev);
 
 	gpiochip_remove(&ljca_gpio->gc);
-	ljca_unregister_event_cb(ljca_gpio->gpio_info->ljca);
+	ljca_unregister_event_cb(ljca_gpio->ljca);
+	cancel_work_sync(&ljca_gpio->work);
 	mutex_destroy(&ljca_gpio->irq_lock);
 	mutex_destroy(&ljca_gpio->trans_lock);
-	return 0;
 }
 
-#define LJCA_GPIO_DRV_NAME "ljca-gpio"
-static const struct platform_device_id ljca_gpio_id[] = {
-	{ LJCA_GPIO_DRV_NAME, 0 },
-	{ /* sentinel */ }
+static const struct auxiliary_device_id ljca_gpio_id_table[] = {
+	{ "usb_ljca.ljca-gpio", 0 },
+	{ /* sentinel */ },
 };
-MODULE_DEVICE_TABLE(platform, ljca_gpio_id);
+MODULE_DEVICE_TABLE(auxiliary, ljca_gpio_id_table);
 
-static struct platform_driver ljca_gpio_driver = {
-	.driver.name = LJCA_GPIO_DRV_NAME,
+static struct auxiliary_driver ljca_gpio_driver = {
 	.probe = ljca_gpio_probe,
 	.remove = ljca_gpio_remove,
+	.id_table = ljca_gpio_id_table,
 };
-module_platform_driver(ljca_gpio_driver);
+module_auxiliary_driver(ljca_gpio_driver);
 
-MODULE_AUTHOR("Ye Xiang <xiang.ye@intel.com>");
-MODULE_AUTHOR("Wang Zhifeng <zhifeng.wang@intel.com>");
-MODULE_AUTHOR("Zhang Lixu <lixu.zhang@intel.com>");
+MODULE_AUTHOR("Wentong Wu <wentong.wu@intel.com>");
+MODULE_AUTHOR("Zhifeng Wang <zhifeng.wang@intel.com>");
+MODULE_AUTHOR("Lixu Zhang <lixu.zhang@intel.com>");
 MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB-GPIO driver");
 MODULE_LICENSE("GPL");
 MODULE_IMPORT_NS(LJCA);
-- 
2.7.4

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Marc Zyngier]

On 2023-09-16 19:53, Wentong Wu wrote:
> Add driver for Intel La Jolla Cove Adapter (LJCA) device. This
> IO-expander adds additional functions to the host system such
> as GPIO, I2C and SPI with USB host interface. We add 4 drivers
> to support this device: a USB driver, a GPIO chip driver, a I2C
> controller driver and a SPI controller driver.

Can I be a pain and ask you to limit the rate at which you
repost this series? You send it every other day, hence
actively discouraging people from reviewing it (why would
they, there's another version coming).

Once a week is a perfectly good rate, and would probably
lead to better results.

Alternatively, if you decide that you really want to keep
sending it that often, please drop me from the Cc list.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Greg KH]

On Sun, Sep 17, 2023 at 02:53:32AM +0800, Wentong Wu wrote:
> Add driver for Intel La Jolla Cove Adapter (LJCA) device. This
> IO-expander adds additional functions to the host system such
> as GPIO, I2C and SPI with USB host interface. We add 4 drivers
> to support this device: a USB driver, a GPIO chip driver, a I2C
> controller driver and a SPI controller driver.
> 
> ---
> v19:
>  - add v17's change which v18 doesn't apply

I don't understand this changelog line at all, what do you mean?

> v18:
>  - rebase patch set on top of Linus' master branch (57d88e8a5974644039fbc47806bac7bb12025636)

Why?  What required this?

As Marc says, you are sending this way too often.  I'm going to move
this to the bottom of my pile and get to it in a week or so as with the
constant resends, there are way more changes that were sent before yours
that need to be reviewed.

thanks,

greg k-h

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Hans de Goede]

Hi Marc, Greg,

On 9/17/23 12:42, Greg KH wrote:
> On Sun, Sep 17, 2023 at 02:53:32AM +0800, Wentong Wu wrote:
>> Add driver for Intel La Jolla Cove Adapter (LJCA) device. This
>> IO-expander adds additional functions to the host system such
>> as GPIO, I2C and SPI with USB host interface. We add 4 drivers
>> to support this device: a USB driver, a GPIO chip driver, a I2C
>> controller driver and a SPI controller driver.
>>
>> ---
>> v19:
>>  - add v17's change which v18 doesn't apply
> 
> I don't understand this changelog line at all, what do you mean?
> 
>> v18:
>>  - rebase patch set on top of Linus' master branch (57d88e8a5974644039fbc47806bac7bb12025636)
> 
> Why?  What required this?

I noticed that this did not apply cleanly (the SPI patch did not apply
cleanly), so I asked for the next version to be based on 6.6-rc1 or
Linus' master branch.

Note I did not ask for a new version to be send right away, but
I'm afraid there has been a bit of miscommunication and instead
of rebasing the next version based on further review Wentong has
send out a new rebased version immediately, sorry about that.

Regards,

Hans

RE: [PATCH v19 0/4] Add Intel LJCA device driver

[Wu, Wentong]

Hi Greg and Marc,

It's my bad. I'm so sorry for this, and apologize for it here.

And I thought you have lots of patches to review, so I just try to address all
the issues currently I know before you start to review, so that my patch
doesn't take over your bandwidth much.

And now I understand your pain here, I will follow your advise going forward. Thanks

Also thank @Hans de Goede for the help. Thanks

BR,
Wentong

> From: Greg KH
> 
> On Sun, Sep 17, 2023 at 02:53:32AM +0800, Wentong Wu wrote:
> > Add driver for Intel La Jolla Cove Adapter (LJCA) device. This
> > IO-expander adds additional functions to the host system such as GPIO,
> > I2C and SPI with USB host interface. We add 4 drivers to support this
> > device: a USB driver, a GPIO chip driver, a I2C controller driver and
> > a SPI controller driver.
> >
> > ---
> > v19:
> >  - add v17's change which v18 doesn't apply
> 
> I don't understand this changelog line at all, what do you mean?

In v18, I forgot to include v17's change, so resend with v19 immediately, sorry
for this again.

> 
> > v18:
> >  - rebase patch set on top of Linus' master branch
> > (57d88e8a5974644039fbc47806bac7bb12025636)
> 
> Why?  What required this?

This is just a rebase for you and other viewers review and apply the patch easily.
Sorry again.

BR,
Wentong
> 
> As Marc says, you are sending this way too often.  I'm going to move this to the
> bottom of my pile and get to it in a week or so as with the constant resends,
> there are way more changes that were sent before yours that need to be
> reviewed.
> 
> thanks,
> 
> greg k-h

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Oliver Neukum]

On 17.09.23 13:26, Hans de Goede wrote:
  
> Note I did not ask for a new version to be send right away, but
> I'm afraid there has been a bit of miscommunication and instead
> of rebasing the next version based on further review Wentong has
> send out a new rebased version immediately, sorry about that.

Hi,

what to do now? It's been ten days.
I am sure this driver has been very thoroughly reviewed by now.
We are dragging this out. Do we want the developer to do another release
or do we ask Greg to take it as is?
This is becoming almost comical, but that is not what we want driver
submission to be.

As far as I am concerned on the USB side everything is fine now.
Hans? Greg?

	Regards
		Oliver

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Hans de Goede]

Hi,

On 9/28/23 12:18, Oliver Neukum wrote:
> On 17.09.23 13:26, Hans de Goede wrote:
>  
>> Note I did not ask for a new version to be send right away, but
>> I'm afraid there has been a bit of miscommunication and instead
>> of rebasing the next version based on further review Wentong has
>> send out a new rebased version immediately, sorry about that.
> 
> Hi,
> 
> what to do now? It's been ten days.
> I am sure this driver has been very thoroughly reviewed by now.
> We are dragging this out. Do we want the developer to do another release
> or do we ask Greg to take it as is?
> This is becoming almost comical, but that is not what we want driver
> submission to be.
> 
> As far as I am concerned on the USB side everything is fine now.
> Hans? Greg?

Note I have been mostly involved in testing these patches I have
*not* thoroughly reviewed them. I have taken a quick(ish) look
which did not find anything obviously wrong.

I agree that at least patch 1/4 is ready for merging. I'm
not sure if Greg should pick-up the entire series or if
the rest should be merged through there relevant subsystems
to also give the relevant subsys maintainer tree.

For the series:

Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Greg KH]

On Thu, Sep 28, 2023 at 02:20:04PM +0200, Hans de Goede wrote:
> Hi,
> 
> On 9/28/23 12:18, Oliver Neukum wrote:
> > On 17.09.23 13:26, Hans de Goede wrote:
> >  
> >> Note I did not ask for a new version to be send right away, but
> >> I'm afraid there has been a bit of miscommunication and instead
> >> of rebasing the next version based on further review Wentong has
> >> send out a new rebased version immediately, sorry about that.
> > 
> > Hi,
> > 
> > what to do now? It's been ten days.
> > I am sure this driver has been very thoroughly reviewed by now.
> > We are dragging this out. Do we want the developer to do another release
> > or do we ask Greg to take it as is?
> > This is becoming almost comical, but that is not what we want driver
> > submission to be.
> > 
> > As far as I am concerned on the USB side everything is fine now.
> > Hans? Greg?
> 
> Note I have been mostly involved in testing these patches I have
> *not* thoroughly reviewed them. I have taken a quick(ish) look
> which did not find anything obviously wrong.
> 
> I agree that at least patch 1/4 is ready for merging. I'm
> not sure if Greg should pick-up the entire series or if
> the rest should be merged through there relevant subsystems
> to also give the relevant subsys maintainer tree.
> 
> For the series:
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com>

Ok, I'll take a look at these again next week when I return home and
catch up on my pending patch review queue.

thanks for the review!

greg k-h

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Andi Shyti]

Hi,

On Thu, Sep 28, 2023 at 12:18:50PM +0200, Oliver Neukum wrote:
> On 17.09.23 13:26, Hans de Goede wrote:
> > Note I did not ask for a new version to be send right away, but
> > I'm afraid there has been a bit of miscommunication and instead
> > of rebasing the next version based on further review Wentong has
> > send out a new rebased version immediately, sorry about that.
> 
> Hi,
> 
> what to do now? It's been ten days.
> I am sure this driver has been very thoroughly reviewed by now.
> We are dragging this out. Do we want the developer to do another release
> or do we ask Greg to take it as is?
> This is becoming almost comical, but that is not what we want driver
> submission to be.
> 
> As far as I am concerned on the USB side everything is fine now.
> Hans? Greg?

i2c is also good to go and the rest looks good, as well. I have
some concerns on patch 4 that looks like a mixture of many random
things.

Andi

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Mark Brown]

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

On Thu, Sep 28, 2023 at 02:20:04PM +0200, Hans de Goede wrote:

> I agree that at least patch 1/4 is ready for merging. I'm
> not sure if Greg should pick-up the entire series or if
> the rest should be merged through there relevant subsystems
> to also give the relevant subsys maintainer tree.

I stopped paying attention when it was obvious that the USB stuff was
struggling badly and getting lots of resends, I've not checked the SPI 
stuff in a while.

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

Re: [PATCH v19 0/4] Add Intel LJCA device driver

[Bartosz Golaszewski]

On Thu, Sep 28, 2023 at 2:29 PM Andi Shyti <andi.shyti@linux.intel.com> wrote:
>
> Hi,
>
> On Thu, Sep 28, 2023 at 12:18:50PM +0200, Oliver Neukum wrote:
> > On 17.09.23 13:26, Hans de Goede wrote:
> > > Note I did not ask for a new version to be send right away, but
> > > I'm afraid there has been a bit of miscommunication and instead
> > > of rebasing the next version based on further review Wentong has
> > > send out a new rebased version immediately, sorry about that.
> >
> > Hi,
> >
> > what to do now? It's been ten days.
> > I am sure this driver has been very thoroughly reviewed by now.
> > We are dragging this out. Do we want the developer to do another release
> > or do we ask Greg to take it as is?
> > This is becoming almost comical, but that is not what we want driver
> > submission to be.
> >
> > As far as I am concerned on the USB side everything is fine now.
> > Hans? Greg?
>
> i2c is also good to go and the rest looks good, as well. I have
> some concerns on patch 4 that looks like a mixture of many random
> things.
>
> Andi

It's got a lot of coding style fixes ninja-packed in there that are
not mentioned by the commit message. But as it's been reviewed by
Linus, acked by Andy (and myself) and tested by Hans, I'm ready to let
it slide if that saves me from seeing ten additional versions of this
series in my inbox.

Bart

Re: [PATCH v19 4/4] gpio: update Intel LJCA USB GPIO driver

[Greg KH]

On Sun, Sep 17, 2023 at 02:53:36AM +0800, Wentong Wu wrote:
> This driver communicate with LJCA GPIO module with specific
> protocol through interfaces exported by LJCA USB driver.
> Update the driver according to LJCA USB driver's changes.
> 
> Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> Acked-by: Linus Walleij <linus.walleij@linaro.org>
> Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> Tested-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/gpio/Kconfig     |   4 +-
>  drivers/gpio/gpio-ljca.c | 246 +++++++++++++++++++++++++++--------------------
>  2 files changed, 145 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
> index 673bafb..8d5b6c3 100644
> --- a/drivers/gpio/Kconfig
> +++ b/drivers/gpio/Kconfig
> @@ -1312,9 +1312,9 @@ config GPIO_KEMPLD
>  
>  config GPIO_LJCA
>  	tristate "INTEL La Jolla Cove Adapter GPIO support"
> -	depends on MFD_LJCA
> +	depends on USB_LJCA
>  	select GPIOLIB_IRQCHIP
> -	default MFD_LJCA
> +	default USB_LJCA
>  	help
>  	  Select this option to enable GPIO driver for the INTEL
>  	  La Jolla Cove Adapter (LJCA) board.
> diff --git a/drivers/gpio/gpio-ljca.c b/drivers/gpio/gpio-ljca.c
> index 87863f0..7fae26d 100644
> --- a/drivers/gpio/gpio-ljca.c
> +++ b/drivers/gpio/gpio-ljca.c
> @@ -6,6 +6,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <linux/auxiliary_bus.h>
>  #include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/dev_printk.h>
> @@ -13,19 +14,18 @@
>  #include <linux/irq.h>
>  #include <linux/kernel.h>
>  #include <linux/kref.h>
> -#include <linux/mfd/ljca.h>
>  #include <linux/module.h>
> -#include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/types.h>
> +#include <linux/usb/ljca.h>
>  
>  /* GPIO commands */
> -#define LJCA_GPIO_CONFIG	1
> -#define LJCA_GPIO_READ		2
> -#define LJCA_GPIO_WRITE		3
> -#define LJCA_GPIO_INT_EVENT	4
> -#define LJCA_GPIO_INT_MASK	5
> -#define LJCA_GPIO_INT_UNMASK	6
> +#define LJCA_GPIO_CONFIG		1
> +#define LJCA_GPIO_READ			2
> +#define LJCA_GPIO_WRITE			3
> +#define LJCA_GPIO_INT_EVENT		4
> +#define LJCA_GPIO_INT_MASK		5
> +#define LJCA_GPIO_INT_UNMASK		6

Why are you changing whitespace for no good reason?

Please don't do that, it makes finding the actual changes in this driver
impossible to notice and review.



>  
>  #define LJCA_GPIO_CONF_DISABLE		BIT(0)
>  #define LJCA_GPIO_CONF_INPUT		BIT(1)
> @@ -36,45 +36,49 @@
>  #define LJCA_GPIO_CONF_INTERRUPT	BIT(6)
>  #define LJCA_GPIO_INT_TYPE		BIT(7)
>  
> -#define LJCA_GPIO_CONF_EDGE	FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
> -#define LJCA_GPIO_CONF_LEVEL	FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
> +#define LJCA_GPIO_CONF_EDGE		FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
> +#define LJCA_GPIO_CONF_LEVEL		FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
>  
>  /* Intentional overlap with PULLUP / PULLDOWN */
> -#define LJCA_GPIO_CONF_SET	BIT(3)
> -#define LJCA_GPIO_CONF_CLR	BIT(4)
> +#define LJCA_GPIO_CONF_SET		BIT(3)
> +#define LJCA_GPIO_CONF_CLR		BIT(4)
>  
> -struct gpio_op {
> +#define LJCA_GPIO_BUF_SIZE		60u

Why "u"?  What requires that?

Odd, sorry, I know people are just getting tired of the constant churn
here, but really, you know better than making changes that are not
needed, or not documented.

greg k-h

Re: [PATCH v19 1/4] usb: Add support for Intel LJCA device

[Greg KH]

On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote:
> +/* ljca cmd message structure */
> +struct ljca_msg {
> +	u8 type;
> +	u8 cmd;
> +	u8 flags;
> +	u8 len;
> +	u8 data[];

Shouldn't you be using the __counted_by attributes for all of these []
arrays so that the compiler knows what to do and how to check that you
don't overrun the buffer?

Adding that now will save you having to add it later, AND it might catch
bugs you already have so please add that.

> +
> +struct ljca_adapter {
> +	struct usb_interface *intf;
> +	struct usb_device *usb_dev;
> +	struct device *dev;
> +
> +	unsigned int rx_pipe;
> +	unsigned int tx_pipe;
> +
> +	/* urb for recv */
> +	struct urb *rx_urb;
> +	/* buffer for recv */
> +	void *rx_buf;
> +	unsigned int rx_len;
> +
> +	/* external buffer for recv */
> +	u8 *ex_buf;
> +	unsigned int ex_buf_len;
> +	/* actual data length copied to external buffer */
> +	unsigned int actual_length;
> +
> +	/* buffer for send */
> +	void *tx_buf;
> +	unsigned int tx_buf_len;
> +
> +	/* lock to protect tx_buf and ex_buf */
> +	spinlock_t lock;
> +
> +	struct completion cmd_completion;
> +
> +	/* mutex to protect command download */
> +	struct mutex mutex;
> +
> +	/* client device list */
> +	struct list_head client_list;
> +
> +	/* disconnect ongoing or not */
> +	bool disconnect;
> +
> +	/* used to reset firmware */

Can you use proper kernel doc for this structure instead of inline
comments?

> +	u32 reset_id;
> +};
> +
> +struct ljca_match_ids_walk_data {
> +	const struct acpi_device_id *ids;
> +	const char *uid;
> +	struct acpi_device *adev;
> +};
> +
> +static const struct acpi_device_id ljca_gpio_hids[] = {
> +	{ "INTC1074" },
> +	{ "INTC1096" },
> +	{ "INTC100B" },
> +	{ "INTC10D1" },
> +	{},
> +};
> +
> +static const struct acpi_device_id ljca_i2c_hids[] = {
> +	{ "INTC1075" },
> +	{ "INTC1097" },
> +	{ "INTC100C" },
> +	{ "INTC10D2" },
> +	{},
> +};
> +
> +static const struct acpi_device_id ljca_spi_hids[] = {
> +	{ "INTC1091" },
> +	{ "INTC1098" },
> +	{ "INTC100D" },
> +	{ "INTC10D3" },
> +	{},
> +};
> +
> +static void ljca_handle_event(struct ljca_adapter *adap,
> +			      struct ljca_msg *header)
> +{
> +	struct ljca_client *client;
> +
> +	list_for_each_entry(client, &adap->client_list, link) {
> +		/*
> +		 * FIXME: currently only GPIO register event callback.
> +		 * firmware message structure should include id when
> +		 * multiple same type clients register event callback.
> +		 */

When will this be fixed?

If not now, why not?

> +		if (client->type == header->type) {
> +			scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
> +				client->event_cb(client->context, header->cmd,
> +						 header->data, header->len);
> +			}
> +
> +			break;
> +		}
> +	}
> +}
> +
> +/* process command ack */
> +static void ljca_handle_cmd_ack(struct ljca_adapter *adap,
> +				struct ljca_msg *header)

We can use 100 columns...

> +{
> +	struct ljca_msg *tx_header = adap->tx_buf;
> +	unsigned int actual_len = 0;
> +	unsigned int ibuf_len;
> +	unsigned long flags;
> +	u8 *ibuf;
> +
> +	spin_lock_irqsave(&adap->lock, flags);
> +
> +	if (tx_header->type != header->type || tx_header->cmd != header->cmd) {
> +		dev_err(adap->dev, "cmd ack mismatch error\n");

When you print error messages like this, who can do something about it?
And why print with interrupts disabled?

> +		spin_unlock_irqrestore(&adap->lock, flags);
> +		return;
> +	}
> +
> +	ibuf_len = adap->ex_buf_len;
> +	ibuf = adap->ex_buf;
> +
> +	if (ibuf && ibuf_len) {
> +		actual_len = min_t(unsigned int, header->len, ibuf_len);

You control both of these types, why aren't they the same type to start
with?  Why the force cast?

> +
> +		/* copy received data to external buffer */
> +		memcpy(ibuf, header->data, actual_len);
> +	}
> +	/* update copied data length */
> +	adap->actual_length = actual_len;

Wait, what happens if you don't actually copy the data?  Why lie and say
you did?  Where is the error handling?

> +
> +	spin_unlock_irqrestore(&adap->lock, flags);
> +
> +	complete(&adap->cmd_completion);
> +}
> +
> +static void ljca_recv(struct urb *urb)

No error handling?

> +{
> +	struct ljca_msg *header = urb->transfer_buffer;
> +	struct ljca_adapter *adap = urb->context;
> +	int ret;
> +
> +	if (urb->status) {
> +		/* sync/async unlink faults aren't errors */
> +		if (urb->status == -ENOENT) {
> +			/*
> +			 * directly complete the possible ongoing transfer
> +			 * during disconnect
> +			 */
> +			if (adap->disconnect)
> +				complete(&adap->cmd_completion);
> +			return;
> +		}
> +
> +		if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN)
> +			return;
> +
> +		dev_err(adap->dev, "recv urb error: %d\n", urb->status);
> +		goto resubmit;

You have an error, yet you don't report it you just spam the kernel log?
Why?  Doesn't this happen when a device is removed?

> +	}
> +
> +	if (header->len + sizeof(*header) != urb->actual_length)
> +		goto resubmit;
> +
> +	if (header->flags & LJCA_ACK_FLAG)
> +		ljca_handle_cmd_ack(adap, header);
> +	else
> +		ljca_handle_event(adap, header);
> +
> +resubmit:
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);

Why atomic, is this in a urb callback?

> +	if (ret && ret != -EPERM)
> +		dev_err(adap->dev, "resubmit recv urb error %d\n", ret);

What happens if ret is an error here?

> +}
> +
> +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> +		     const u8 *obuf, unsigned int obuf_len, u8 *ibuf,
> +		     unsigned int ibuf_len, bool ack, unsigned long timeout)
> +{
> +	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
> +	struct ljca_msg *header = adap->tx_buf;
> +	unsigned int actual;
> +	int ret = 0;
> +
> +	if (adap->disconnect)
> +		return -ENODEV;
> +
> +	if (msg_len > adap->tx_buf_len)
> +		return -EINVAL;
> +
> +	mutex_lock(&adap->mutex);
> +
> +	scoped_guard(spinlock_irqsave, &adap->lock) {
> +		header->type = type;
> +		header->cmd = cmd;
> +		header->len = obuf_len;
> +		if (obuf)
> +			memcpy(header->data, obuf, obuf_len);
> +		header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
> +
> +		adap->ex_buf = ibuf;
> +		adap->ex_buf_len = ibuf_len;
> +		adap->actual_length = 0;
> +	}

Do you really need a scoped guard when you can not fail out of the
block?

Why do you have both a mutex and spinlock grabed?

> +
> +	reinit_completion(&adap->cmd_completion);
> +
> +	usb_autopm_get_interface(adap->intf);

This can fail.  Why aren't you checking that?

> +
> +	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> +			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
> +
> +	usb_autopm_put_interface(adap->intf);
> +
> +	if (!ret && ack) {
> +		ret = wait_for_completion_timeout(&adap->cmd_completion,
> +						  timeout);
> +		if (ret < 0) {
> +			goto out;
> +		} if (!ret) {
> +			ret = -ETIMEDOUT;
> +			goto out;
> +		}
> +	}
> +	ret = adap->actual_length;

Why are you not verifying that you sent what you wanted to send?

When you call this function, sometimes you check that the function sent
the proper amount of data, but in many places you do not, and you assume
that the full buffer was sent, which is not correct.  So please change
_this_ function to check that you sent the proper amount and then the
caller logic will be much simpler and actually work like you are using
it in many places (some places you got it right, some wrong, which is a
HUGE indication that the API is wrong because you wrote this code, and
if you can't get it right...)

> +out:
> +	scoped_guard(spinlock_irqsave, &adap->lock) {
> +		adap->ex_buf = NULL;
> +		adap->ex_buf_len = 0;
> +		memset(header, 0, sizeof(*header));
> +	}

Again, why a scoped guard for something like this that does not need it?

> +
> +	mutex_unlock(&adap->mutex);
> +
> +	return ret;
> +}
> +
> +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
> +		  unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len)
> +{
> +	return ljca_send(client->adapter, client->type, cmd,
> +			 obuf, obuf_len, ibuf, ibuf_len, true,
> +			 LJCA_WRITE_ACK_TIMEOUT_MS);
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
> +
> +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
> +			unsigned int obuf_len)
> +{
> +	return ljca_send(client->adapter, client->type, cmd, obuf,
> +			 obuf_len, NULL, 0, false, LJCA_WRITE_ACK_TIMEOUT_MS);
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
> +
> +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb,
> +			   void *context)
> +{
> +	unsigned long flags;
> +
> +	if (!event_cb)
> +		return -EINVAL;
> +
> +	spin_lock_irqsave(&client->event_cb_lock, flags);
> +
> +	if (client->event_cb) {
> +		spin_unlock_irqrestore(&client->event_cb_lock, flags);
> +		return -EALREADY;
> +	}
> +
> +	client->event_cb = event_cb;
> +	client->context = context;
> +
> +	spin_unlock_irqrestore(&client->event_cb_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
> +
> +void ljca_unregister_event_cb(struct ljca_client *client)
> +{
> +	scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
> +		client->event_cb = NULL;
> +		client->context = NULL;
> +	}
> +}
> +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
> +
> +static int ljca_match_device_ids(struct acpi_device *adev, void *data)
> +{
> +	struct ljca_match_ids_walk_data *wd = data;
> +	const char *uid = acpi_device_uid(adev);
> +
> +	if (acpi_match_device_ids(adev, wd->ids))
> +		return 0;
> +
> +	if (!wd->uid)
> +		goto match;
> +
> +	if (!uid)
> +		uid = "0";

Are you sure this is a valid uid to use?  If so, why?  What happens if
this gets set to "0" for multiple ones?  Don't underestimate broken
firmware tables, but also don't paper over problems in ways that will be
impossible to notice and can cause problems.

Or am I mis-reading this function wrong, how can this ever be a valid
UID to compare against?

> +	else
> +		uid = strchr(uid, wd->uid[0]);
> +
> +	if (!uid || strcmp(uid, wd->uid))
> +		return 0;
> +
> +match:
> +	wd->adev = adev;
> +
> +	return 1;
> +}
> +
> +/* bind auxiliary device to acpi device */
> +static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> +				  struct auxiliary_device *auxdev,
> +				  u64 adr, u8 id)
> +{
> +	struct ljca_match_ids_walk_data wd = { 0 };
> +	struct acpi_device *parent, *adev;
> +	struct device *dev = adap->dev;
> +	char uid[4];
> +
> +	parent = ACPI_COMPANION(dev);
> +	if (!parent)
> +		return;
> +
> +	/*
> +	 * get auxdev ACPI handle from the ACPI device directly
> +	 * under the parent that matches _ADR.
> +	 */
> +	adev = acpi_find_child_device(parent, adr, false);
> +	if (adev) {
> +		ACPI_COMPANION_SET(&auxdev->dev, adev);
> +		return;
> +	}
> +
> +	/*
> +	 * _ADR is a grey area in the ACPI specification, some
> +	 * platforms use _HID to distinguish children devices.
> +	 */
> +	switch (adr) {
> +	case LJCA_GPIO_ACPI_ADR:
> +		wd.ids = ljca_gpio_hids;
> +		break;
> +	case LJCA_I2C1_ACPI_ADR:
> +	case LJCA_I2C2_ACPI_ADR:
> +		snprintf(uid, sizeof(uid), "%d", id);
> +		wd.uid = uid;
> +		wd.ids = ljca_i2c_hids;
> +		break;
> +	case LJCA_SPI1_ACPI_ADR:
> +	case LJCA_SPI2_ACPI_ADR:
> +		wd.ids = ljca_spi_hids;
> +		break;
> +	default:
> +		dev_warn(dev, "unsupported _ADR\n");
> +		return;
> +	}
> +
> +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> +	if (wd.adev) {
> +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
> +		return;
> +	}
> +
> +	parent = ACPI_COMPANION(dev->parent->parent);
> +	if (!parent)
> +		return;
> +
> +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> +	if (wd.adev)
> +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
> +}
> +
> +static void ljca_auxdev_release(struct device *dev)
> +{
> +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> +
> +	kfree(auxdev->dev.platform_data);
> +}
> +
> +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
> +				  char *name, void *data, u64 adr)
> +{
> +	struct auxiliary_device *auxdev;
> +	struct ljca_client *client;
> +	int ret;
> +
> +	client = kzalloc(sizeof *client, GFP_KERNEL);
> +	if (!client)
> +		return -ENOMEM;
> +
> +	client->type = type;
> +	client->id = id;
> +	client->adapter = adap;
> +	spin_lock_init(&client->event_cb_lock);
> +
> +	auxdev = &client->auxdev;
> +	auxdev->name = name;
> +	auxdev->id = id;
> +
> +	auxdev->dev.parent = adap->dev;
> +	auxdev->dev.platform_data = data;
> +	auxdev->dev.release = ljca_auxdev_release;
> +
> +	ret = auxiliary_device_init(auxdev);
> +	if (ret)
> +		goto err_free;
> +
> +	ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
> +
> +	ret = auxiliary_device_add(auxdev);
> +	if (ret)
> +		goto err_uninit;
> +
> +	list_add_tail(&client->link, &adap->client_list);
> +
> +	return 0;
> +
> +err_uninit:
> +	auxiliary_device_uninit(auxdev);
> +
> +err_free:
> +	kfree(client);
> +
> +	return ret;
> +}
> +
> +static int ljca_enumerate_gpio(struct ljca_adapter *adap)
> +{
> +	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
> +	struct ljca_gpio_descriptor *desc;
> +	struct ljca_gpio_info *gpio_info;
> +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> +	int ret, gpio_num;
> +	unsigned int i;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO, NULL, 0, buf,
> +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* check firmware response */
> +	desc = (struct ljca_gpio_descriptor *)buf;
> +	if (ret != struct_size(desc, bank_desc, desc->bank_num))
> +		return -EINVAL;
> +
> +	gpio_num = desc->pins_per_bank * desc->bank_num;
> +	if (gpio_num > LJCA_MAX_GPIO_NUM)
> +		return -EINVAL;
> +
> +	/* construct platform data */
> +	gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL);
> +	if (!gpio_info)
> +		return -ENOMEM;
> +	gpio_info->num = gpio_num;
> +
> +	for (i = 0; i < desc->bank_num; i++)
> +		valid_pin[i] = get_unaligned_le32(&desc->bank_desc[i].valid_pins);
> +	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
> +
> +	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> +				     gpio_info, LJCA_GPIO_ACPI_ADR);
> +	if (ret)
> +		kfree(gpio_info);
> +
> +	return ret;
> +}
> +
> +static int ljca_enumerate_i2c(struct ljca_adapter *adap)
> +{
> +	struct ljca_i2c_descriptor *desc;
> +	struct ljca_i2c_info *i2c_info;
> +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> +	unsigned int i;
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL, 0, buf,
> +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* check firmware response */
> +	desc = (struct ljca_i2c_descriptor *)buf;
> +	if (ret != struct_size(desc, info, desc->num))
> +		return -EINVAL;
> +
> +	for (i = 0; i < desc->num; i++) {
> +		/* construct platform data */
> +		i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL);
> +		if (!i2c_info)
> +			return -ENOMEM;
> +
> +		i2c_info->id = desc->info[i].id;
> +		i2c_info->capacity = desc->info[i].capacity;
> +		i2c_info->intr_pin = desc->info[i].intr_pin;
> +
> +		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> +					     "ljca-i2c", i2c_info,
> +					     LJCA_I2C1_ACPI_ADR + i);
> +		if (ret) {
> +			kfree(i2c_info);
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ljca_enumerate_spi(struct ljca_adapter *adap)
> +{
> +	struct ljca_spi_descriptor *desc;
> +	struct ljca_spi_info *spi_info;
> +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> +	unsigned int i;
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
> +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	desc = (struct ljca_spi_descriptor *)buf;
> +	for (i = 0; i < desc->num; i++) {
> +		/* construct platform data */
> +		spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL);
> +		if (!spi_info)
> +			return -ENOMEM;
> +
> +		spi_info->id = desc->info[i].id;
> +		spi_info->capacity = desc->info[i].capacity;
> +
> +		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> +					     "ljca-spi", spi_info,
> +					     LJCA_SPI1_ACPI_ADR + i);
> +		if (ret) {
> +			kfree(spi_info);
> +			return ret;

What about the other objects that you created here, don't you need to
unwind the new ones if one in the chain fails?  And if not, why stop at
the first failure?


> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ljca_reset_handshake(struct ljca_adapter *adap)
> +{
> +	__le32 reset_id = cpu_to_le32(adap->reset_id);
> +	__le32 reset_id_ret = 0;
> +	int ret;
> +
> +	adap->reset_id++;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_RESET, (u8 *)&reset_id,
> +			sizeof(__le32), (u8 *)&reset_id_ret, sizeof(__le32),
> +			true, LJCA_WRITE_ACK_TIMEOUT_MS);
> +	if (ret < 0)
> +		return ret;
> +
> +	if (reset_id_ret != reset_id)
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int ljca_enumerate_clients(struct ljca_adapter *adap)
> +{
> +	int ret;
> +
> +	ret = ljca_reset_handshake(adap);
> +	if (ret)
> +		return ret;
> +
> +	ret = ljca_enumerate_gpio(adap);
> +	if (ret)
> +		dev_warn(adap->dev, "enumerate GPIO error\n");
> +
> +	ret = ljca_enumerate_i2c(adap);
> +	if (ret)
> +		dev_warn(adap->dev, "enumerate I2C error\n");
> +
> +	ret = ljca_enumerate_spi(adap);
> +	if (ret)
> +		dev_warn(adap->dev, "enumerate SPI error\n");

So none of these "errors" are actually errors:

> +	return 0;

You return success?  Why?  Are they not actually problems?

thanks,

greg k-h

Re: [PATCH v19 2/4] i2c: Add support for Intel LJCA USB I2C driver

[Wolfram Sang]

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


> +static u32 ljca_i2c_func(struct i2c_adapter *adap)
> +{
> +	return I2C_FUNC_I2C;

You could use this here to make the driver way more useful:

return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);

Except I2C_FUNC_SMBUS_QUICK, emulated SMBUS calls work out of the box.

For I2C_FUNC_SMBUS_QUICK, you need to test zero-length transfers indeed
('i2cdetect <busnum>' will use it and is a good testcase). Which would
be good anyway, because if it is not supporting zero-length, we need to
add an adapter->quirk flag as well.

We could add this incrementally, but it will be better to have this
correct right away. i2cdetect is a good and simple testcase.


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

RE: [PATCH v19 1/4] usb: Add support for Intel LJCA device

[Wu, Wentong]

Hi Greg,

Thanks for your review

> From: Greg KH <gregkh@linuxfoundation.org>
> On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote:
> > +/* ljca cmd message structure */
> > +struct ljca_msg {
> > +	u8 type;
> > +	u8 cmd;
> > +	u8 flags;
> > +	u8 len;
> > +	u8 data[];
> 
> Shouldn't you be using the __counted_by attributes for all of these [] arrays so
> that the compiler knows what to do and how to check that you don't overrun
> the buffer?

In this structure, len is not for data length, instead it's the length of header and data.

> 
> Adding that now will save you having to add it later, AND it might catch bugs you
> already have so please add that.

But, I will add the __counted_by attribute for others like below:

struct ljca_i2c_descriptor {
	u8 num;
	struct ljca_i2c_ctr_info info[] __counted_by(num);
} __packed;

struct ljca_spi_descriptor {
	u8 num;
	struct ljca_spi_ctr_info info[] __counted_by(num);
} __packed;

struct ljca_gpio_descriptor {
	u8 pins_per_bank;
	u8 bank_num;
	struct ljca_bank_descriptor bank_desc[] __counted_by(bank_num);
} __packed;

> 
> > +
> > +struct ljca_adapter {
> > +	struct usb_interface *intf;
> > +	struct usb_device *usb_dev;
> > +	struct device *dev;
> > +
> > +	unsigned int rx_pipe;
> > +	unsigned int tx_pipe;
> > +
> > +	/* urb for recv */
> > +	struct urb *rx_urb;
> > +	/* buffer for recv */
> > +	void *rx_buf;
> > +	unsigned int rx_len;
> > +
> > +	/* external buffer for recv */
> > +	u8 *ex_buf;
> > +	unsigned int ex_buf_len;
> > +	/* actual data length copied to external buffer */
> > +	unsigned int actual_length;
> > +
> > +	/* buffer for send */
> > +	void *tx_buf;
> > +	unsigned int tx_buf_len;
> > +
> > +	/* lock to protect tx_buf and ex_buf */
> > +	spinlock_t lock;
> > +
> > +	struct completion cmd_completion;
> > +
> > +	/* mutex to protect command download */
> > +	struct mutex mutex;
> > +
> > +	/* client device list */
> > +	struct list_head client_list;
> > +
> > +	/* disconnect ongoing or not */
> > +	bool disconnect;
> > +
> > +	/* used to reset firmware */
> 
> Can you use proper kernel doc for this structure instead of inline comments?

Ack, thanks

> 
> > +	u32 reset_id;
> > +};
> > +
> > +struct ljca_match_ids_walk_data {
> > +	const struct acpi_device_id *ids;
> > +	const char *uid;
> > +	struct acpi_device *adev;
> > +};
> > +
> > +static const struct acpi_device_id ljca_gpio_hids[] = {
> > +	{ "INTC1074" },
> > +	{ "INTC1096" },
> > +	{ "INTC100B" },
> > +	{ "INTC10D1" },
> > +	{},
> > +};
> > +
> > +static const struct acpi_device_id ljca_i2c_hids[] = {
> > +	{ "INTC1075" },
> > +	{ "INTC1097" },
> > +	{ "INTC100C" },
> > +	{ "INTC10D2" },
> > +	{},
> > +};
> > +
> > +static const struct acpi_device_id ljca_spi_hids[] = {
> > +	{ "INTC1091" },
> > +	{ "INTC1098" },
> > +	{ "INTC100D" },
> > +	{ "INTC10D3" },
> > +	{},
> > +};
> > +
> > +static void ljca_handle_event(struct ljca_adapter *adap,
> > +			      struct ljca_msg *header)
> > +{
> > +	struct ljca_client *client;
> > +
> > +	list_for_each_entry(client, &adap->client_list, link) {
> > +		/*
> > +		 * FIXME: currently only GPIO register event callback.
> > +		 * firmware message structure should include id when
> > +		 * multiple same type clients register event callback.
> > +		 */
> 
> When will this be fixed?
> 
> If not now, why not?

Actually this doesn't impact current functionality because only GPIO register
event callback, but from coding perspective it should add the id in the message
structure. This fix should be done by firmware first, but many products have
already been running the firmware, it's not easy to update the firmware.

Can I just remove the 'FIXME' and leave the comment here?
 
> 
> > +		if (client->type == header->type) {
> > +			scoped_guard(spinlock_irqsave, &client-
> >event_cb_lock) {
> > +				client->event_cb(client->context, header->cmd,
> > +						 header->data, header->len);
> > +			}
> > +
> > +			break;
> > +		}
> > +	}
> > +}
> > +
> > +/* process command ack */
> > +static void ljca_handle_cmd_ack(struct ljca_adapter *adap,
> > +				struct ljca_msg *header)
> 
> We can use 100 columns...

Ok, I will make this in one row.

> 
> > +{
> > +	struct ljca_msg *tx_header = adap->tx_buf;
> > +	unsigned int actual_len = 0;
> > +	unsigned int ibuf_len;
> > +	unsigned long flags;
> > +	u8 *ibuf;
> > +
> > +	spin_lock_irqsave(&adap->lock, flags);
> > +
> > +	if (tx_header->type != header->type || tx_header->cmd != header->cmd)
> {
> > +		dev_err(adap->dev, "cmd ack mismatch error\n");
> 
> When you print error messages like this, who can do something about it?
> And why print with interrupts disabled?

Thanks, this will be like below: 

	if (tx_header->type != header->type || tx_header->cmd != header->cmd) {
		spin_unlock_irqrestore(&adap->lock, flags);
		dev_err(adap->dev, "cmd ack mismatch error\n");
		return;
	}

> 
> > +		spin_unlock_irqrestore(&adap->lock, flags);
> > +		return;
> > +	}
> > +
> > +	ibuf_len = adap->ex_buf_len;
> > +	ibuf = adap->ex_buf;
> > +
> > +	if (ibuf && ibuf_len) {
> > +		actual_len = min_t(unsigned int, header->len, ibuf_len);
> 
> You control both of these types, why aren't they the same type to start with?
> Why the force cast?

The len of header is defined by firmware, it should be u8 type. But the ex_buf_len
is passed by API users, I don't want users to do the cast if their buffer is large than
256.

> 
> > +
> > +		/* copy received data to external buffer */
> > +		memcpy(ibuf, header->data, actual_len);
> > +	}
> > +	/* update copied data length */
> > +	adap->actual_length = actual_len;
> 
> Wait, what happens if you don't actually copy the data?

This actual_length is the actual length of data copied to external buffer
where is to save the command response. The API callers should check
the response length according to the command you passed to this API.

> Why lie and say you did? Where is the error handling?

As I said, the API callers should have the error handing because they know
the exact response format, and actually I already the error handling in this
patch set.

> 
> > +
> > +	spin_unlock_irqrestore(&adap->lock, flags);
> > +
> > +	complete(&adap->cmd_completion);
> > +}
> > +
> > +static void ljca_recv(struct urb *urb)
> 
> No error handling?

Actually I have, I re-structure this function as below to make it more clear,

static void ljca_recv(struct urb *urb)
{
	struct ljca_msg *header = urb->transfer_buffer;
	struct ljca_adapter *adap = urb->context;
	int ret;

	switch (urb->status) {
	case 0:
		/* success */
		break;
	case -ENOENT:
		/*
		   * directly complete the possible ongoing transfer
		   * during disconnect
		   */
		if (adap->disconnect)
			complete(&adap->cmd_completion);
		return;
	case -ECONNRESET:
	case -ESHUTDOWN:
	case -EPIPE:
		/* rx urb is terminated */
		dev_dbg(adap->dev, "rx urb terminated with status: %d\n",
			urb->status);
		return;
	default:
		dev_dbg(adap->dev, "rx urb error: %d\n", urb->status);
		goto resubmit;
	}

	if (header->len + sizeof(*header) != urb->actual_length)
		goto resubmit;

	if (header->flags & LJCA_ACK_FLAG)
		ljca_handle_cmd_ack(adap, header);
	else
		ljca_handle_event(adap, header);

resubmit:
	ret = usb_submit_urb(urb, GFP_ATOMIC);
 	if (ret && ret != -EPERM)
		dev_err(adap->dev, "resubmit rx urb error %d\n", ret);
} 

> 
> > +{
> > +	struct ljca_msg *header = urb->transfer_buffer;
> > +	struct ljca_adapter *adap = urb->context;
> > +	int ret;
> > +
> > +	if (urb->status) {
> > +		/* sync/async unlink faults aren't errors */
> > +		if (urb->status == -ENOENT) {
> > +			/*
> > +			 * directly complete the possible ongoing transfer
> > +			 * during disconnect
> > +			 */
> > +			if (adap->disconnect)
> > +				complete(&adap->cmd_completion);
> > +			return;
> > +		}
> > +
> > +		if (urb->status == -ECONNRESET || urb->status == -
> ESHUTDOWN)
> > +			return;
> > +
> > +		dev_err(adap->dev, "recv urb error: %d\n", urb->status);
> > +		goto resubmit;
> 
> You have an error, yet you don't report it you just spam the kernel log?
> Why?  Doesn't this happen when a device is removed?

When device is removed, it will be covered by ESHUTDOWN case in the above
'if (urb->status == -ECONNRESET || urb->status == -ESHUTDOWN) '

Most of the command error cases have been handled by the above
'if (usb->status == ', for others it will print error log, In the re-structured code,
I changed it to dev_dbg to avoid spam.

> 
> > +	}
> > +
> > +	if (header->len + sizeof(*header) != urb->actual_length)
> > +		goto resubmit;
> > +
> > +	if (header->flags & LJCA_ACK_FLAG)
> > +		ljca_handle_cmd_ack(adap, header);
> > +	else
> > +		ljca_handle_event(adap, header);
> > +
> > +resubmit:
> > +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> 
> Why atomic, is this in a urb callback?

Yes,

> 
> > +	if (ret && ret != -EPERM)
> > +		dev_err(adap->dev, "resubmit recv urb error %d\n", ret);
> 
> What happens if ret is an error here?

No command response, and there will be command timeout.

> 
> > +}
> > +
> > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> > +		     const u8 *obuf, unsigned int obuf_len, u8 *ibuf,
> > +		     unsigned int ibuf_len, bool ack, unsigned long timeout) {
> > +	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
> > +	struct ljca_msg *header = adap->tx_buf;
> > +	unsigned int actual;
> > +	int ret = 0;
> > +
> > +	if (adap->disconnect)
> > +		return -ENODEV;
> > +
> > +	if (msg_len > adap->tx_buf_len)
> > +		return -EINVAL;
> > +
> > +	mutex_lock(&adap->mutex);
> > +
> > +	scoped_guard(spinlock_irqsave, &adap->lock) {
> > +		header->type = type;
> > +		header->cmd = cmd;
> > +		header->len = obuf_len;
> > +		if (obuf)
> > +			memcpy(header->data, obuf, obuf_len);
> > +		header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
> > +
> > +		adap->ex_buf = ibuf;
> > +		adap->ex_buf_len = ibuf_len;
> > +		adap->actual_length = 0;
> > +	}
> 
> Do you really need a scoped guard when you can not fail out of the block?

The scoped_guard is required by you with "Why not use the functionality in
cleanup.h for this lock? Makes this function much simpler." If I understand
correctly, so I use the scoped_guard where I can to make things simpler.

> 
> Why do you have both a mutex and spinlock grabed?

The mutex is to avoid command download concurrently

The spinlock is to protect tx_buf and ex_buf, which may be accessed by tx and rx
at the same time.

> 
> > +
> > +	reinit_completion(&adap->cmd_completion);
> > +
> > +	usb_autopm_get_interface(adap->intf);
> 
> This can fail.  Why aren't you checking that?

Ack, thanks, and it will be like below:

	ret = usb_autopm_get_interface(adap->intf);
	if (ret < 0)
		goto out;

> 
> > +
> > +	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> > +			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
> > +
> > +	usb_autopm_put_interface(adap->intf);
> > +
> > +	if (!ret && ack) {
> > +		ret = wait_for_completion_timeout(&adap->cmd_completion,
> > +						  timeout);
> > +		if (ret < 0) {
> > +			goto out;
> > +		} if (!ret) {
> > +			ret = -ETIMEDOUT;
> > +			goto out;
> > +		}
> > +	}
> > +	ret = adap->actual_length;
> 
> Why are you not verifying that you sent what you wanted to send?

As I said, the actual_length is the actual length of data copied to user buffer instead
of the length of what we want to send.

And even for verifying the length of what we want to send, the max length of the
message is sizeof(struct ljca_msg) + LJCA_MAX_PACKET_SIZE which is less than
the endpoint's max packet size, so I don't check the actual sent length in above
usb_bulk_msg().

> 
> When you call this function, sometimes you check that the function sent the
> proper amount of data, but in many places you do not, and you assume that the
> full buffer was sent, which is not correct.  So please change _this_ function to
> check that you sent the proper amount and then the caller logic will be much
> simpler and actually work like you are using it in many places (some places you
> got it right, some wrong, which is a HUGE indication that the API is wrong
> because you wrote this code, and if you can't get it right...)

As I said, the return value of this function is the response data length instead of
sent data length. And in this patch set, every caller has verified if the response
length matched with the sent command. 

> 
> > +out:
> > +	scoped_guard(spinlock_irqsave, &adap->lock) {
> > +		adap->ex_buf = NULL;
> > +		adap->ex_buf_len = 0;
> > +		memset(header, 0, sizeof(*header));
> > +	}
> 
> Again, why a scoped guard for something like this that does not need it?

As I said above, this is to address your last time review comment if I understand
correctly, I can switch to the original one if we don't need this scoped guard.

> 
> > +
> > +	mutex_unlock(&adap->mutex);
> > +
> > +	return ret;
> > +}
> > +
> > +int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
> > +		  unsigned int obuf_len, u8 *ibuf, unsigned int ibuf_len) {
> > +	return ljca_send(client->adapter, client->type, cmd,
> > +			 obuf, obuf_len, ibuf, ibuf_len, true,
> > +			 LJCA_WRITE_ACK_TIMEOUT_MS);
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ljca_transfer, LJCA);
> > +
> > +int ljca_transfer_noack(struct ljca_client *client, u8 cmd, const u8 *obuf,
> > +			unsigned int obuf_len)
> > +{
> > +	return ljca_send(client->adapter, client->type, cmd, obuf,
> > +			 obuf_len, NULL, 0, false,
> LJCA_WRITE_ACK_TIMEOUT_MS); }
> > +EXPORT_SYMBOL_NS_GPL(ljca_transfer_noack, LJCA);
> > +
> > +int ljca_register_event_cb(struct ljca_client *client, ljca_event_cb_t event_cb,
> > +			   void *context)
> > +{
> > +	unsigned long flags;
> > +
> > +	if (!event_cb)
> > +		return -EINVAL;
> > +
> > +	spin_lock_irqsave(&client->event_cb_lock, flags);
> > +
> > +	if (client->event_cb) {
> > +		spin_unlock_irqrestore(&client->event_cb_lock, flags);
> > +		return -EALREADY;
> > +	}
> > +
> > +	client->event_cb = event_cb;
> > +	client->context = context;
> > +
> > +	spin_unlock_irqrestore(&client->event_cb_lock, flags);
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ljca_register_event_cb, LJCA);
> > +
> > +void ljca_unregister_event_cb(struct ljca_client *client) {
> > +	scoped_guard(spinlock_irqsave, &client->event_cb_lock) {
> > +		client->event_cb = NULL;
> > +		client->context = NULL;
> > +	}
> > +}
> > +EXPORT_SYMBOL_NS_GPL(ljca_unregister_event_cb, LJCA);
> > +
> > +static int ljca_match_device_ids(struct acpi_device *adev, void
> > +*data) {
> > +	struct ljca_match_ids_walk_data *wd = data;
> > +	const char *uid = acpi_device_uid(adev);
> > +
> > +	if (acpi_match_device_ids(adev, wd->ids))
> > +		return 0;
> > +
> > +	if (!wd->uid)
> > +		goto match;
> > +
> > +	if (!uid)
> > +		uid = "0";
> 
> Are you sure this is a valid uid to use?  If so, why?  What happens if this gets set
> to "0" for multiple ones?  Don't underestimate broken firmware tables, but also
> don't paper over problems in ways that will be impossible to notice and can
> cause problems.

This actually has been discussed in previous email as bellow: 

some DSDTs have only 1 ACPI companion for the 2 I2C controllers
and these don't set a UID at all. On these models only the first I2C
controller is used. So if a HID match has no UID use "0" for the HID.
assigning the ACPI companion to the first I2C controller.
An example device with this setup is the Dell Latitude 9420.

> 
> Or am I mis-reading this function wrong, how can this ever be a valid UID to
> compare against?
> 
> > +	else
> > +		uid = strchr(uid, wd->uid[0]);
> > +
> > +	if (!uid || strcmp(uid, wd->uid))
> > +		return 0;
> > +
> > +match:
> > +	wd->adev = adev;
> > +
> > +	return 1;
> > +}
> > +
> > +/* bind auxiliary device to acpi device */ static void
> > +ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
> > +				  struct auxiliary_device *auxdev,
> > +				  u64 adr, u8 id)
> > +{
> > +	struct ljca_match_ids_walk_data wd = { 0 };
> > +	struct acpi_device *parent, *adev;
> > +	struct device *dev = adap->dev;
> > +	char uid[4];
> > +
> > +	parent = ACPI_COMPANION(dev);
> > +	if (!parent)
> > +		return;
> > +
> > +	/*
> > +	 * get auxdev ACPI handle from the ACPI device directly
> > +	 * under the parent that matches _ADR.
> > +	 */
> > +	adev = acpi_find_child_device(parent, adr, false);
> > +	if (adev) {
> > +		ACPI_COMPANION_SET(&auxdev->dev, adev);
> > +		return;
> > +	}
> > +
> > +	/*
> > +	 * _ADR is a grey area in the ACPI specification, some
> > +	 * platforms use _HID to distinguish children devices.
> > +	 */
> > +	switch (adr) {
> > +	case LJCA_GPIO_ACPI_ADR:
> > +		wd.ids = ljca_gpio_hids;
> > +		break;
> > +	case LJCA_I2C1_ACPI_ADR:
> > +	case LJCA_I2C2_ACPI_ADR:
> > +		snprintf(uid, sizeof(uid), "%d", id);
> > +		wd.uid = uid;
> > +		wd.ids = ljca_i2c_hids;
> > +		break;
> > +	case LJCA_SPI1_ACPI_ADR:
> > +	case LJCA_SPI2_ACPI_ADR:
> > +		wd.ids = ljca_spi_hids;
> > +		break;
> > +	default:
> > +		dev_warn(dev, "unsupported _ADR\n");
> > +		return;
> > +	}
> > +
> > +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> > +	if (wd.adev) {
> > +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev);
> > +		return;
> > +	}
> > +
> > +	parent = ACPI_COMPANION(dev->parent->parent);
> > +	if (!parent)
> > +		return;
> > +
> > +	acpi_dev_for_each_child(parent, ljca_match_device_ids, &wd);
> > +	if (wd.adev)
> > +		ACPI_COMPANION_SET(&auxdev->dev, wd.adev); }
> > +
> > +static void ljca_auxdev_release(struct device *dev) {
> > +	struct auxiliary_device *auxdev = to_auxiliary_dev(dev);
> > +
> > +	kfree(auxdev->dev.platform_data);
> > +}
> > +
> > +static int ljca_new_client_device(struct ljca_adapter *adap, u8 type, u8 id,
> > +				  char *name, void *data, u64 adr) {
> > +	struct auxiliary_device *auxdev;
> > +	struct ljca_client *client;
> > +	int ret;
> > +
> > +	client = kzalloc(sizeof *client, GFP_KERNEL);
> > +	if (!client)
> > +		return -ENOMEM;
> > +
> > +	client->type = type;
> > +	client->id = id;
> > +	client->adapter = adap;
> > +	spin_lock_init(&client->event_cb_lock);
> > +
> > +	auxdev = &client->auxdev;
> > +	auxdev->name = name;
> > +	auxdev->id = id;
> > +
> > +	auxdev->dev.parent = adap->dev;
> > +	auxdev->dev.platform_data = data;
> > +	auxdev->dev.release = ljca_auxdev_release;
> > +
> > +	ret = auxiliary_device_init(auxdev);
> > +	if (ret)
> > +		goto err_free;
> > +
> > +	ljca_auxdev_acpi_bind(adap, auxdev, adr, id);
> > +
> > +	ret = auxiliary_device_add(auxdev);
> > +	if (ret)
> > +		goto err_uninit;
> > +
> > +	list_add_tail(&client->link, &adap->client_list);
> > +
> > +	return 0;
> > +
> > +err_uninit:
> > +	auxiliary_device_uninit(auxdev);
> > +
> > +err_free:
> > +	kfree(client);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ljca_enumerate_gpio(struct ljca_adapter *adap) {
> > +	u32 valid_pin[LJCA_MAX_GPIO_NUM / BITS_PER_TYPE(u32)];
> > +	struct ljca_gpio_descriptor *desc;
> > +	struct ljca_gpio_info *gpio_info;
> > +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> > +	int ret, gpio_num;
> > +	unsigned int i;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_GPIO,
> NULL, 0, buf,
> > +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* check firmware response */
> > +	desc = (struct ljca_gpio_descriptor *)buf;
> > +	if (ret != struct_size(desc, bank_desc, desc->bank_num))
> > +		return -EINVAL;
> > +
> > +	gpio_num = desc->pins_per_bank * desc->bank_num;
> > +	if (gpio_num > LJCA_MAX_GPIO_NUM)
> > +		return -EINVAL;
> > +
> > +	/* construct platform data */
> > +	gpio_info = kzalloc(sizeof *gpio_info, GFP_KERNEL);
> > +	if (!gpio_info)
> > +		return -ENOMEM;
> > +	gpio_info->num = gpio_num;
> > +
> > +	for (i = 0; i < desc->bank_num; i++)
> > +		valid_pin[i] = get_unaligned_le32(&desc-
> >bank_desc[i].valid_pins);
> > +	bitmap_from_arr32(gpio_info->valid_pin_map, valid_pin, gpio_num);
> > +
> > +	ret = ljca_new_client_device(adap, LJCA_CLIENT_GPIO, 0, "ljca-gpio",
> > +				     gpio_info, LJCA_GPIO_ACPI_ADR);
> > +	if (ret)
> > +		kfree(gpio_info);
> > +
> > +	return ret;
> > +}
> > +
> > +static int ljca_enumerate_i2c(struct ljca_adapter *adap) {
> > +	struct ljca_i2c_descriptor *desc;
> > +	struct ljca_i2c_info *i2c_info;
> > +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_I2C, NULL,
> 0, buf,
> > +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	/* check firmware response */
> > +	desc = (struct ljca_i2c_descriptor *)buf;
> > +	if (ret != struct_size(desc, info, desc->num))
> > +		return -EINVAL;
> > +
> > +	for (i = 0; i < desc->num; i++) {
> > +		/* construct platform data */
> > +		i2c_info = kzalloc(sizeof *i2c_info, GFP_KERNEL);
> > +		if (!i2c_info)
> > +			return -ENOMEM;
> > +
> > +		i2c_info->id = desc->info[i].id;
> > +		i2c_info->capacity = desc->info[i].capacity;
> > +		i2c_info->intr_pin = desc->info[i].intr_pin;
> > +
> > +		ret = ljca_new_client_device(adap, LJCA_CLIENT_I2C, i,
> > +					     "ljca-i2c", i2c_info,
> > +					     LJCA_I2C1_ACPI_ADR + i);
> > +		if (ret) {
> > +			kfree(i2c_info);
> > +			return ret;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int ljca_enumerate_spi(struct ljca_adapter *adap) {
> > +	struct ljca_spi_descriptor *desc;
> > +	struct ljca_spi_info *spi_info;
> > +	u8 buf[LJCA_MAX_PAYLOAD_SIZE];
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL,
> 0, buf,
> > +			sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> > +	if (ret < 0)
> > +		return ret;
> > +
> > +	desc = (struct ljca_spi_descriptor *)buf;
> > +	for (i = 0; i < desc->num; i++) {
> > +		/* construct platform data */
> > +		spi_info = kzalloc(sizeof *spi_info, GFP_KERNEL);
> > +		if (!spi_info)
> > +			return -ENOMEM;
> > +
> > +		spi_info->id = desc->info[i].id;
> > +		spi_info->capacity = desc->info[i].capacity;
> > +
> > +		ret = ljca_new_client_device(adap, LJCA_CLIENT_SPI, i,
> > +					     "ljca-spi", spi_info,
> > +					     LJCA_SPI1_ACPI_ADR + i);
> > +		if (ret) {
> > +			kfree(spi_info);
> > +			return ret;
> 
> What about the other objects that you created here, don't you need to unwind
> the new ones if one in the chain fails?  And if not, why stop at the first failure?
> 
> 
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +
> > +static int ljca_enumerate_clients(struct ljca_adapter *adap) {
> > +	int ret;
> > +
> > +	ret = ljca_reset_handshake(adap);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = ljca_enumerate_gpio(adap);
> > +	if (ret)
> > +		dev_warn(adap->dev, "enumerate GPIO error\n");
> > +
> > +	ret = ljca_enumerate_i2c(adap);
> > +	if (ret)
> > +		dev_warn(adap->dev, "enumerate I2C error\n");
> > +
> > +	ret = ljca_enumerate_spi(adap);
> > +	if (ret)
> > +		dev_warn(adap->dev, "enumerate SPI error\n");
> 
> So none of these "errors" are actually errors:
> 
> > +	return 0;
> 
> You return success?  Why?  Are they not actually problems?

This is to be compatible with old version FW which does not support
full USB2xxx functions, so it just warn here as this is.
To make things more clear, I re-structure this as below, hope that
helps, thanks

static int ljca_enumerate_clients(struct ljca_adapter *adap)
{
	struct ljca_client *client, *next;
	int ret;

	ret = ljca_reset_handshake(adap);
	if (ret)
		return ret;

	ret = ljca_enumerate_gpio(adap);
	if (ret)
		goto err_free;

	ret = ljca_enumerate_i2c(adap);
	if (ret)
		goto err_free;

	ret = ljca_enumerate_spi(adap);
	if (ret)
		goto err_free;

	return 0;

err_free:
	adap->disconnect = true;

	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
		auxiliary_device_delete(&client->auxdev);
		auxiliary_device_uninit(&client->auxdev);

		list_del_init(&client->link);
		kfree(client);
	}

	return ret;
}

And accordingly, the ljca_enumerate_xxx() has slightly change to cover the
unsupported case as below( take spi as an example)

@@ -617,6 +633,11 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap)
 
        ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
                        sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
+       if (ret == -ETIMEDOUT) {
+               dev_warn(adap->dev, "doesn't support SPI function\n");
+               return 0;
+       }
+
        if (ret < 0)
                return ret;

Thanks
Wentong
> 
> thanks,
> 
> greg k-h

RE: [PATCH v19 4/4] gpio: update Intel LJCA USB GPIO driver

[Wu, Wentong]

> From: Greg KH
> On Sun, Sep 17, 2023 at 02:53:36AM +0800, Wentong Wu wrote:
> > This driver communicate with LJCA GPIO module with specific protocol
> > through interfaces exported by LJCA USB driver.
> > Update the driver according to LJCA USB driver's changes.
> >
> > Signed-off-by: Wentong Wu <wentong.wu@intel.com>
> > Reviewed-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > Acked-by: Linus Walleij <linus.walleij@linaro.org>
> > Acked-by: Bartosz Golaszewski <bartosz.golaszewski@linaro.org>
> > Tested-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/gpio/Kconfig     |   4 +-
> >  drivers/gpio/gpio-ljca.c | 246
> > +++++++++++++++++++++++++++--------------------
> >  2 files changed, 145 insertions(+), 105 deletions(-)
> >
> > diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig index
> > 673bafb..8d5b6c3 100644
> > --- a/drivers/gpio/Kconfig
> > +++ b/drivers/gpio/Kconfig
> > @@ -1312,9 +1312,9 @@ config GPIO_KEMPLD
> >
> >  config GPIO_LJCA
> >  	tristate "INTEL La Jolla Cove Adapter GPIO support"
> > -	depends on MFD_LJCA
> > +	depends on USB_LJCA
> >  	select GPIOLIB_IRQCHIP
> > -	default MFD_LJCA
> > +	default USB_LJCA
> >  	help
> >  	  Select this option to enable GPIO driver for the INTEL
> >  	  La Jolla Cove Adapter (LJCA) board.
> > diff --git a/drivers/gpio/gpio-ljca.c b/drivers/gpio/gpio-ljca.c index
> > 87863f0..7fae26d 100644
> > --- a/drivers/gpio/gpio-ljca.c
> > +++ b/drivers/gpio/gpio-ljca.c
> > @@ -6,6 +6,7 @@
> >   */
> >
> >  #include <linux/acpi.h>
> > +#include <linux/auxiliary_bus.h>
> >  #include <linux/bitfield.h>
> >  #include <linux/bitops.h>
> >  #include <linux/dev_printk.h>
> > @@ -13,19 +14,18 @@
> >  #include <linux/irq.h>
> >  #include <linux/kernel.h>
> >  #include <linux/kref.h>
> > -#include <linux/mfd/ljca.h>
> >  #include <linux/module.h>
> > -#include <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#include <linux/usb/ljca.h>
> >
> >  /* GPIO commands */
> > -#define LJCA_GPIO_CONFIG	1
> > -#define LJCA_GPIO_READ		2
> > -#define LJCA_GPIO_WRITE		3
> > -#define LJCA_GPIO_INT_EVENT	4
> > -#define LJCA_GPIO_INT_MASK	5
> > -#define LJCA_GPIO_INT_UNMASK	6
> > +#define LJCA_GPIO_CONFIG		1
> > +#define LJCA_GPIO_READ			2
> > +#define LJCA_GPIO_WRITE			3
> > +#define LJCA_GPIO_INT_EVENT		4
> > +#define LJCA_GPIO_INT_MASK		5
> > +#define LJCA_GPIO_INT_UNMASK		6
> 
> Why are you changing whitespace for no good reason?

This is to make all the macro value in the same column to
make them look better.

> 
> Please don't do that, it makes finding the actual changes in this driver impossible
> to notice and review.

Understand, I'll follow this going forward. Thanks

Thanks
Wentong
> 
> 
> 
> >
> >  #define LJCA_GPIO_CONF_DISABLE		BIT(0)
> >  #define LJCA_GPIO_CONF_INPUT		BIT(1)
> > @@ -36,45 +36,49 @@
> >  #define LJCA_GPIO_CONF_INTERRUPT	BIT(6)
> >  #define LJCA_GPIO_INT_TYPE		BIT(7)
> >
> > -#define LJCA_GPIO_CONF_EDGE	FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
> > -#define LJCA_GPIO_CONF_LEVEL	FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
> > +#define LJCA_GPIO_CONF_EDGE
> 	FIELD_PREP(LJCA_GPIO_INT_TYPE, 1)
> > +#define LJCA_GPIO_CONF_LEVEL
> 	FIELD_PREP(LJCA_GPIO_INT_TYPE, 0)
> >
> >  /* Intentional overlap with PULLUP / PULLDOWN */
> > -#define LJCA_GPIO_CONF_SET	BIT(3)
> > -#define LJCA_GPIO_CONF_CLR	BIT(4)
> > +#define LJCA_GPIO_CONF_SET		BIT(3)
> > +#define LJCA_GPIO_CONF_CLR		BIT(4)
> >
> > -struct gpio_op {
> > +#define LJCA_GPIO_BUF_SIZE		60u
> 
> Why "u"?  What requires that?
> 
> Odd, sorry, I know people are just getting tired of the constant churn here, but
> really, you know better than making changes that are not needed, or not
> documented.
> 
> greg k-h

RE: [PATCH v19 2/4] i2c: Add support for Intel LJCA USB I2C driver

[Wu, Wentong]

> From: Wolfram Sang <wsa@kernel.org>
> 
> > +static u32 ljca_i2c_func(struct i2c_adapter *adap) {
> > +	return I2C_FUNC_I2C;
> 
> You could use this here to make the driver way more useful:
> 
> return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL &
> ~I2C_FUNC_SMBUS_QUICK);
> 
> Except I2C_FUNC_SMBUS_QUICK, emulated SMBUS calls work out of the box.
> 
> For I2C_FUNC_SMBUS_QUICK, you need to test zero-length transfers indeed
> ('i2cdetect <busnum>' will use it and is a good testcase). Which would be good
> anyway, because if it is not supporting zero-length, we need to add an adapter-
> >quirk flag as well.

Thanks.

And with i2cdetect -q busnum, I see error on driver side which enable
I2C_FUNC_SMBUS_EMUL and not disable I2C_FUNC_SMBUS_QUICK, I think
it means we don't support zero-length transfer if I understand correctly.

> 
> We could add this incrementally, but it will be better to have this correct right
> away. i2cdetect is a good and simple testcase.

So the code will be like below, please help take a look, thanks.

static u32 ljca_i2c_func(struct i2c_adapter *adap)
{
	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
}

static const struct i2c_adapter_quirks ljca_i2c_quirks = {
	.flags = I2C_AQ_NO_ZERO_LEN,
	.max_read_len = LJCA_I2C_MAX_XFER_SIZE,
	.max_write_len = LJCA_I2C_MAX_XFER_SIZE,
};

BR,
Wentong

Re: [PATCH v19 2/4] i2c: Add support for Intel LJCA USB I2C driver

[Wolfram Sang]

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


> And with i2cdetect -q busnum, I see error on driver side which enable
> I2C_FUNC_SMBUS_EMUL and not disable I2C_FUNC_SMBUS_QUICK, I think
> it means we don't support zero-length transfer if I understand correctly.

At least, not currently. Sometimes the driver just needs updates,
sometimes the HW simply cannot do it. If it is a software issue, we can
fix it incrementally.

> static u32 ljca_i2c_func(struct i2c_adapter *adap)
> {
> 	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> }

Yes.

> static const struct i2c_adapter_quirks ljca_i2c_quirks = {
> 	.flags = I2C_AQ_NO_ZERO_LEN,
> 	.max_read_len = LJCA_I2C_MAX_XFER_SIZE,
> 	.max_write_len = LJCA_I2C_MAX_XFER_SIZE,
> };

Yes.

I think the I2C driver is good then.


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

Re: [PATCH v19 1/4] usb: Add support for Intel LJCA device

[Greg KH]

On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote:
> > From: Greg KH <gregkh@linuxfoundation.org>
> > On Sun, Sep 17, 2023 at 02:53:33AM +0800, Wentong Wu wrote:
> > > +static void ljca_handle_event(struct ljca_adapter *adap,
> > > +			      struct ljca_msg *header)
> > > +{
> > > +	struct ljca_client *client;
> > > +
> > > +	list_for_each_entry(client, &adap->client_list, link) {
> > > +		/*
> > > +		 * FIXME: currently only GPIO register event callback.
> > > +		 * firmware message structure should include id when
> > > +		 * multiple same type clients register event callback.
> > > +		 */
> > 
> > When will this be fixed?
> > 
> > If not now, why not?
> 
> Actually this doesn't impact current functionality because only GPIO register
> event callback, but from coding perspective it should add the id in the message
> structure. This fix should be done by firmware first, but many products have
> already been running the firmware, it's not easy to update the firmware.
> 
> Can I just remove the 'FIXME' and leave the comment here?

If you are never going to fix it, it does not need a FIXME, right?  :)

> > > +		spin_unlock_irqrestore(&adap->lock, flags);
> > > +		return;
> > > +	}
> > > +
> > > +	ibuf_len = adap->ex_buf_len;
> > > +	ibuf = adap->ex_buf;
> > > +
> > > +	if (ibuf && ibuf_len) {
> > > +		actual_len = min_t(unsigned int, header->len, ibuf_len);
> > 
> > You control both of these types, why aren't they the same type to start with?
> > Why the force cast?
> 
> The len of header is defined by firmware, it should be u8 type. But the ex_buf_len
> is passed by API users, I don't want users to do the cast if their buffer is large than
> 256.

Then fix the api to use a u8 as obviously you can not handle more data
then that for now.

> > > +
> > > +		/* copy received data to external buffer */
> > > +		memcpy(ibuf, header->data, actual_len);
> > > +	}
> > > +	/* update copied data length */
> > > +	adap->actual_length = actual_len;
> > 
> > Wait, what happens if you don't actually copy the data?
> 
> This actual_length is the actual length of data copied to external buffer
> where is to save the command response. The API callers should check
> the response length according to the command you passed to this API.

But they aren't checking it as I pointed out elsewhere.

> > > +}
> > > +
> > > +static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> > > +		     const u8 *obuf, unsigned int obuf_len, u8 *ibuf,
> > > +		     unsigned int ibuf_len, bool ack, unsigned long timeout) {
> > > +	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
> > > +	struct ljca_msg *header = adap->tx_buf;
> > > +	unsigned int actual;
> > > +	int ret = 0;
> > > +
> > > +	if (adap->disconnect)
> > > +		return -ENODEV;
> > > +
> > > +	if (msg_len > adap->tx_buf_len)
> > > +		return -EINVAL;
> > > +
> > > +	mutex_lock(&adap->mutex);
> > > +
> > > +	scoped_guard(spinlock_irqsave, &adap->lock) {
> > > +		header->type = type;
> > > +		header->cmd = cmd;
> > > +		header->len = obuf_len;
> > > +		if (obuf)
> > > +			memcpy(header->data, obuf, obuf_len);
> > > +		header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
> > > +
> > > +		adap->ex_buf = ibuf;
> > > +		adap->ex_buf_len = ibuf_len;
> > > +		adap->actual_length = 0;
> > > +	}
> > 
> > Do you really need a scoped guard when you can not fail out of the block?
> 
> The scoped_guard is required by you with "Why not use the functionality in
> cleanup.h for this lock? Makes this function much simpler." If I understand
> correctly, so I use the scoped_guard where I can to make things simpler.

But that's not making anything simpler here, cleanup.h works well when
you have error paths that would be more complex without it.  You do not
have that here at all now (maybe you did before?)

> > Why do you have both a mutex and spinlock grabed?
> 
> The mutex is to avoid command download concurrently
> 
> The spinlock is to protect tx_buf and ex_buf, which may be accessed by tx and rx
> at the same time.

Please document this somewhere.

> > > +	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> > > +			   msg_len, &actual, LJCA_WRITE_TIMEOUT_MS);
> > > +
> > > +	usb_autopm_put_interface(adap->intf);
> > > +
> > > +	if (!ret && ack) {
> > > +		ret = wait_for_completion_timeout(&adap->cmd_completion,
> > > +						  timeout);
> > > +		if (ret < 0) {
> > > +			goto out;
> > > +		} if (!ret) {
> > > +			ret = -ETIMEDOUT;
> > > +			goto out;
> > > +		}
> > > +	}
> > > +	ret = adap->actual_length;
> > 
> > Why are you not verifying that you sent what you wanted to send?
> 
> As I said, the actual_length is the actual length of data copied to user buffer instead
> of the length of what we want to send.
> 
> And even for verifying the length of what we want to send, the max length of the
> message is sizeof(struct ljca_msg) + LJCA_MAX_PACKET_SIZE which is less than
> the endpoint's max packet size, so I don't check the actual sent length in above
> usb_bulk_msg().

But you need to.

> > When you call this function, sometimes you check that the function sent the
> > proper amount of data, but in many places you do not, and you assume that the
> > full buffer was sent, which is not correct.  So please change _this_ function to
> > check that you sent the proper amount and then the caller logic will be much
> > simpler and actually work like you are using it in many places (some places you
> > got it right, some wrong, which is a HUGE indication that the API is wrong
> > because you wrote this code, and if you can't get it right...)
> 
> As I said, the return value of this function is the response data length instead of
> sent data length. And in this patch set, every caller has verified if the response
> length matched with the sent command. 

No, I found many users that did not do this.  Please make the api easy
to use, right now it's not.

> > > +static int ljca_match_device_ids(struct acpi_device *adev, void
> > > +*data) {
> > > +	struct ljca_match_ids_walk_data *wd = data;
> > > +	const char *uid = acpi_device_uid(adev);
> > > +
> > > +	if (acpi_match_device_ids(adev, wd->ids))
> > > +		return 0;
> > > +
> > > +	if (!wd->uid)
> > > +		goto match;
> > > +
> > > +	if (!uid)
> > > +		uid = "0";
> > 
> > Are you sure this is a valid uid to use?  If so, why?  What happens if this gets set
> > to "0" for multiple ones?  Don't underestimate broken firmware tables, but also
> > don't paper over problems in ways that will be impossible to notice and can
> > cause problems.
> 
> This actually has been discussed in previous email as bellow: 
> 
> some DSDTs have only 1 ACPI companion for the 2 I2C controllers
> and these don't set a UID at all. On these models only the first I2C
> controller is used. So if a HID match has no UID use "0" for the HID.
> assigning the ACPI companion to the first I2C controller.
> An example device with this setup is the Dell Latitude 9420.

Then document this really really well in the code itself, otherwise it
looks broken.

> > > +static int ljca_enumerate_clients(struct ljca_adapter *adap) {
> > > +	int ret;
> > > +
> > > +	ret = ljca_reset_handshake(adap);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = ljca_enumerate_gpio(adap);
> > > +	if (ret)
> > > +		dev_warn(adap->dev, "enumerate GPIO error\n");
> > > +
> > > +	ret = ljca_enumerate_i2c(adap);
> > > +	if (ret)
> > > +		dev_warn(adap->dev, "enumerate I2C error\n");
> > > +
> > > +	ret = ljca_enumerate_spi(adap);
> > > +	if (ret)
> > > +		dev_warn(adap->dev, "enumerate SPI error\n");
> > 
> > So none of these "errors" are actually errors:
> > 
> > > +	return 0;
> > 
> > You return success?  Why?  Are they not actually problems?
> 
> This is to be compatible with old version FW which does not support
> full USB2xxx functions, so it just warn here as this is.

Why do you have to support obsolete and broken firmware?  Can't it be
updated?

> To make things more clear, I re-structure this as below, hope that
> helps, thanks
> 
> static int ljca_enumerate_clients(struct ljca_adapter *adap)
> {
> 	struct ljca_client *client, *next;
> 	int ret;
> 
> 	ret = ljca_reset_handshake(adap);
> 	if (ret)
> 		return ret;
> 
> 	ret = ljca_enumerate_gpio(adap);
> 	if (ret)
> 		goto err_free;
> 
> 	ret = ljca_enumerate_i2c(adap);
> 	if (ret)
> 		goto err_free;
> 
> 	ret = ljca_enumerate_spi(adap);
> 	if (ret)
> 		goto err_free;
> 
> 	return 0;
> 
> err_free:
> 	adap->disconnect = true;
> 
> 	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
> 		auxiliary_device_delete(&client->auxdev);
> 		auxiliary_device_uninit(&client->auxdev);
> 
> 		list_del_init(&client->link);
> 		kfree(client);
> 	}
> 
> 	return ret;
> }
> 
> And accordingly, the ljca_enumerate_xxx() has slightly change to cover the
> unsupported case as below( take spi as an example)
> 
> @@ -617,6 +633,11 @@ static int ljca_enumerate_spi(struct ljca_adapter *adap)
>  
>         ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_ENUM_SPI, NULL, 0, buf,
>                         sizeof(buf), true, LJCA_ENUM_CLIENT_TIMEOUT_MS);
> +       if (ret == -ETIMEDOUT) {
> +               dev_warn(adap->dev, "doesn't support SPI function\n");
> +               return 0;

You warn, yet return success?  Again, that doesn't work well as you
never know if you need to unwind it or not.

Either report an error and handle it, or don't, but what you have here
looks broken as-is.

thanks,

greg k-h

RE: [PATCH v19 1/4] usb: Add support for Intel LJCA device

[Wu, Wentong]

> From: Greg KH <gregkh@linuxfoundation.org>
> On Fri, Sep 29, 2023 at 11:31:21AM +0000, Wu, Wentong wrote:
> > > From: Greg KH <gregkh@linuxfoundation.org> On Sun, Sep 17, 2023 at
> > > 02:53:33AM +0800, Wentong Wu wrote:
> > > > +static void ljca_handle_event(struct ljca_adapter *adap,
> > > > +			      struct ljca_msg *header) {
> > > > +	struct ljca_client *client;
> > > > +
> > > > +	list_for_each_entry(client, &adap->client_list, link) {
> > > > +		/*
> > > > +		 * FIXME: currently only GPIO register event callback.
> > > > +		 * firmware message structure should include id when
> > > > +		 * multiple same type clients register event callback.
> > > > +		 */
> > >
> > > When will this be fixed?
> > >
> > > If not now, why not?
> >
> > Actually this doesn't impact current functionality because only GPIO
> > register event callback, but from coding perspective it should add the
> > id in the message structure. This fix should be done by firmware
> > first, but many products have already been running the firmware, it's not easy
> to update the firmware.
> >
> > Can I just remove the 'FIXME' and leave the comment here?
> 
> If you are never going to fix it, it does not need a FIXME, right?  :)

Thanks, I will remove 'FIXME' here.

> > > You control both of these types, why aren't they the same type to start with?
> > > Why the force cast?
> >
> > The len of header is defined by firmware, it should be u8 type. But
> > the ex_buf_len is passed by API users, I don't want users to do the
> > cast if their buffer is large than 256.
> 
> Then fix the api to use a u8 as obviously you can not handle more data then that
> for now.

Ack, thanks
 
> > > Do you really need a scoped guard when you can not fail out of the block?
> >
> > The scoped_guard is required by you with "Why not use the
> > functionality in cleanup.h for this lock? Makes this function much
> > simpler." If I understand correctly, so I use the scoped_guard where I can to
> make things simpler.
> 
> But that's not making anything simpler here, cleanup.h works well when you
> have error paths that would be more complex without it.  You do not have that
> here at all now (maybe you did before?)

I'll remove scoped guard

> 
> > > Why do you have both a mutex and spinlock grabed?
> >
> > The mutex is to avoid command download concurrently
> >
> > The spinlock is to protect tx_buf and ex_buf, which may be accessed by
> > tx and rx at the same time.
> 
> Please document this somewhere.

Ack, thanks. Below is the kernel doc for struct ljca_adapter where we have
spinlock and mutex document.

/**
 * struct ljca_adapter - represent a ljca adapter
 *
 * @intf: the usb interface for this ljca adapter
 * @usb_dev: the usb device for this ljca adapter
 * @dev: the specific device info of the usb interface
 * @rx_pipe: bulk in pipe for receive data from firmware
 * @tx_pipe: bulk out pipe for send data to firmware
 * @rx_urb: urb used for the bulk in pipe
 * @rx_buf: buffer used to receive command response and event
 * @rx_len: length of rx buffer
 * @ex_buf: external buffer to save command response
 * @ex_buf_len: length of external buffer
 * @actual_length: actual length of data copied to external buffer
 * @tx_buf: buffer used to download command to firmware
 * @tx_buf_len: length of tx buffer
 * @lock: spinlock to protect tx_buf and ex_buf
 * @cmd_completion: completion object as the command receives ack
 * @mutex: mutex to avoid command download concurrently
 * @client_list: client device list
 * @disconnect: usb disconnect ongoing or not
 * @reset_id: used to reset firmware
 */
struct ljca_adapter {
	struct usb_interface *intf;
	struct usb_device *usb_dev;
	struct device *dev;

	unsigned int rx_pipe;
	unsigned int tx_pipe;

	struct urb *rx_urb;
	void *rx_buf;
	unsigned int rx_len;

	u8 *ex_buf;
	u8 ex_buf_len;
	u8 actual_length;

	void *tx_buf;
	u8 tx_buf_len;

	spinlock_t lock;

	struct completion cmd_completion;
	struct mutex mutex;

	struct list_head client_list;

	bool disconnect;

	u32 reset_id;
};
 
> > > Why are you not verifying that you sent what you wanted to send?
> >
> > As I said, the actual_length is the actual length of data copied to
> > user buffer instead of the length of what we want to send.
> >
> > And even for verifying the length of what we want to send, the max
> > length of the message is sizeof(struct ljca_msg) +
> > LJCA_MAX_PACKET_SIZE which is less than the endpoint's max packet
> > size, so I don't check the actual sent length in above usb_bulk_msg().
> 
> But you need to.

Ack, thanks.

> 
> > > When you call this function, sometimes you check that the function
> > > sent the proper amount of data, but in many places you do not, and
> > > you assume that the full buffer was sent, which is not correct.  So
> > > please change _this_ function to check that you sent the proper
> > > amount and then the caller logic will be much simpler and actually
> > > work like you are using it in many places (some places you got it
> > > right, some wrong, which is a HUGE indication that the API is wrong
> > > because you wrote this code, and if you can't get it right...)
> >
> > As I said, the return value of this function is the response data
> > length instead of sent data length. And in this patch set, every
> > caller has verified if the response length matched with the sent command.
> 
> No, I found many users that did not do this.  

Ack, I will check more, thanks

>Please make the api easy to use, right now it's not.

I post the new ljca_send() here to try to address several comments.

First it changes the type of buffer length to u8, second it checks the actual sent
of length usb_bulk_msg(). It removes the scoped guard as well.

And below gives an explanation of the parameters and return value.

The parameters(type, cmd, obuf_len) are used to construct message header,
obuf is used for message data. And ibuf and ibuf_len are for response buffer
and buffer length passed by API users. ack indicates if the command
need an ack from firmware, timeout is timeout value to wait command ack.

And the return value is the response copied to response buffer passed by API
users, normally the users know how large buffer they have to pass to this API,
but from coding perspective we should check the passed response buffer
length(ibuf_len) and return the actual copied data length to the buffer.

Hope that helps, thanks

static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
		      const u8 *obuf, u8 obuf_len, u8 *ibuf, u8 ibuf_len,
		      bool ack, unsigned long timeout)
{
	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
	struct ljca_msg *header = adap->tx_buf;
	unsigned int transferred;
	unsigned long flags;
	int ret;

	if (adap->disconnect)
		return -ENODEV;

	if (msg_len > adap->tx_buf_len)
		return -EINVAL;

	mutex_lock(&adap->mutex);

	spin_lock_irqsave(&adap->lock, flags);

	header->type = type;
	header->cmd = cmd;
	header->len = obuf_len;
	if (obuf)
		memcpy(header->data, obuf, obuf_len);

	header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);

	adap->ex_buf = ibuf;
	adap->ex_buf_len = ibuf_len;
	adap->actual_length = 0;

	spin_unlock_irqrestore(&adap->lock, flags);

 	reinit_completion(&adap->cmd_completion);

	ret = usb_autopm_get_interface(adap->intf);
	if (ret < 0)
		goto out;

	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
			    msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);

	usb_autopm_put_interface(adap->intf);

	if (ret < 0)
		goto out;
	if (transferred != msg_len) {
		ret = -EIO;
		goto out;
	}

	if (ack) {
		ret = wait_for_completion_timeout(&adap->cmd_completion,
						       timeout);
		if (!ret) {
			ret = -ETIMEDOUT;
			goto out;
		}
	}
	ret = adap->actual_length;

out:
	spin_lock_irqsave(&adap->lock, flags);
	adap->ex_buf = NULL;
	adap->ex_buf_len = 0;

	memset(header, 0, sizeof(*header));
	spin_unlock_irqrestore(&adap->lock, flags);

	mutex_unlock(&adap->mutex);

	return ret;
}

> > > Are you sure this is a valid uid to use?  If so, why?  What happens
> > > if this gets set to "0" for multiple ones?  Don't underestimate
> > > broken firmware tables, but also don't paper over problems in ways
> > > that will be impossible to notice and can cause problems.
> >
> > This actually has been discussed in previous email as bellow:
> >
> > some DSDTs have only 1 ACPI companion for the 2 I2C controllers and
> > these don't set a UID at all. On these models only the first I2C
> > controller is used. So if a HID match has no UID use "0" for the HID.
> > assigning the ACPI companion to the first I2C controller.
> > An example device with this setup is the Dell Latitude 9420.
> 
> Then document this really really well in the code itself, otherwise it looks broken.

Ack, thanks, I post the new ljca_match_device_ids() here, hope that helps, thanks.
 
static int ljca_match_device_ids(struct acpi_device *adev, void *data)
{
	struct ljca_match_ids_walk_data *wd = data;
	const char *uid = acpi_device_uid(adev);

	if (acpi_match_device_ids(adev, wd->ids))
		return 0;

	if (!wd->uid)
		goto match;

	if (!uid)
		/*
		 * Some DSDTs have only one ACPI companion for the two I2C
		 * controllers and they don't set a UID at all (e.g. Dell
 		 * Latitude 9420). On these platforms only the first I2C
		 * controller is used, so if a HID match has no UID we use
		 * "0" as the UID and assign ACPI companion to the first
		 * I2C controller.
		 */ 
		uid = "0";
	else
		uid = strchr(uid, wd->uid[0]);

	if (!uid || strcmp(uid, wd->uid))
		return 0;

match:
	wd->adev = adev;

	return 1;
}

> > This is to be compatible with old version FW which does not support
> > full USB2xxx functions, so it just warn here as this is.
> 
> Why do you have to support obsolete and broken firmware?  Can't it be updated?

make sense, and probably they have to update, thanks

> You warn, yet return success?  Again, that doesn't work well as you never know
> if you need to unwind it or not.
> 
> Either report an error and handle it, or don't, but what you have here looks
> broken as-is.

Ack, it should report error and handle it. Thanks, the function will be like below:

static int ljca_enumerate_clients(struct ljca_adapter *adap)
{
	struct ljca_client *client, *next;
	int ret;

	ret = ljca_reset_handshake(adap);
	if (ret)
		goto err_kill;

	ret = ljca_enumerate_gpio(adap);
	if (ret) {
		dev_err(adap->dev, "enumerate GPIO error\n");
		goto err_kill;
        	}

	ret = ljca_enumerate_i2c(adap);
	if (ret) {
		dev_err(adap->dev, "enumerate I2C error\n");
		goto err_kill;
	}

	ret = ljca_enumerate_spi(adap);
	if (ret) {
		dev_err(adap->dev, "enumerate SPI error\n");
		goto err_kill;
	}

	return 0;

err_kill:
	adap->disconnect = true;

	usb_kill_urb(adap->rx_urb);

	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
		auxiliary_device_delete(&client->auxdev);
		auxiliary_device_uninit(&client->auxdev);

		list_del_init(&client->link);
		kfree(client);
	}

	return ret;
}

Thanks,

Wentong

> 
> thanks,
> 
> greg k-h

RE: [PATCH v19 1/4] usb: Add support for Intel LJCA device

[Wu, Wentong]

> From: Wu, Wentong
> > From: Greg KH <gregkh@linuxfoundation.org> On Fri, Sep 29, 2023 at
> > 11:31:21AM +0000, Wu, Wentong wrote:
> > > > From: Greg KH <gregkh@linuxfoundation.org> On Sun, Sep 17, 2023 at
> > > > 02:53:33AM +0800, Wentong Wu wrote:
> > > > > +static void ljca_handle_event(struct ljca_adapter *adap,
> > > > > +			      struct ljca_msg *header) {
> > > > > +	struct ljca_client *client;
> > > > > +
> > > > > +	list_for_each_entry(client, &adap->client_list, link) {
> > > > > +		/*
> > > > > +		 * FIXME: currently only GPIO register event callback.
> > > > > +		 * firmware message structure should include id when
> > > > > +		 * multiple same type clients register event callback.
> > > > > +		 */
> > > >
> > > > When will this be fixed?
> > > >
> > > > If not now, why not?
> > >
> > > Actually this doesn't impact current functionality because only GPIO
> > > register event callback, but from coding perspective it should add
> > > the id in the message structure. This fix should be done by firmware
> > > first, but many products have already been running the firmware,
> > > it's not easy
> > to update the firmware.
> > >
> > > Can I just remove the 'FIXME' and leave the comment here?
> >
> > If you are never going to fix it, it does not need a FIXME, right?  :)
> 
> Thanks, I will remove 'FIXME' here.
> 
> > > > You control both of these types, why aren't they the same type to start
> with?
> > > > Why the force cast?
> > >
> > > The len of header is defined by firmware, it should be u8 type. But
> > > the ex_buf_len is passed by API users, I don't want users to do the
> > > cast if their buffer is large than 256.
> >
> > Then fix the api to use a u8 as obviously you can not handle more data
> > then that for now.
> 
> Ack, thanks
> 
> > > > Do you really need a scoped guard when you can not fail out of the block?
> > >
> > > The scoped_guard is required by you with "Why not use the
> > > functionality in cleanup.h for this lock? Makes this function much
> > > simpler." If I understand correctly, so I use the scoped_guard where
> > > I can to
> > make things simpler.
> >
> > But that's not making anything simpler here, cleanup.h works well when
> > you have error paths that would be more complex without it.  You do
> > not have that here at all now (maybe you did before?)
> 
> I'll remove scoped guard
> 
> >
> > > > Why do you have both a mutex and spinlock grabed?
> > >
> > > The mutex is to avoid command download concurrently
> > >
> > > The spinlock is to protect tx_buf and ex_buf, which may be accessed
> > > by tx and rx at the same time.
> >
> > Please document this somewhere.
> 
> Ack, thanks. Below is the kernel doc for struct ljca_adapter where we have
> spinlock and mutex document.
> 
> /**
>  * struct ljca_adapter - represent a ljca adapter
>  *
>  * @intf: the usb interface for this ljca adapter
>  * @usb_dev: the usb device for this ljca adapter
>  * @dev: the specific device info of the usb interface
>  * @rx_pipe: bulk in pipe for receive data from firmware
>  * @tx_pipe: bulk out pipe for send data to firmware
>  * @rx_urb: urb used for the bulk in pipe
>  * @rx_buf: buffer used to receive command response and event
>  * @rx_len: length of rx buffer
>  * @ex_buf: external buffer to save command response
>  * @ex_buf_len: length of external buffer
>  * @actual_length: actual length of data copied to external buffer
>  * @tx_buf: buffer used to download command to firmware
>  * @tx_buf_len: length of tx buffer
>  * @lock: spinlock to protect tx_buf and ex_buf
>  * @cmd_completion: completion object as the command receives ack
>  * @mutex: mutex to avoid command download concurrently
>  * @client_list: client device list
>  * @disconnect: usb disconnect ongoing or not
>  * @reset_id: used to reset firmware
>  */
> struct ljca_adapter {
> 	struct usb_interface *intf;
> 	struct usb_device *usb_dev;
> 	struct device *dev;
> 
> 	unsigned int rx_pipe;
> 	unsigned int tx_pipe;
> 
> 	struct urb *rx_urb;
> 	void *rx_buf;
> 	unsigned int rx_len;
> 
> 	u8 *ex_buf;
> 	u8 ex_buf_len;
> 	u8 actual_length;
> 
> 	void *tx_buf;
> 	u8 tx_buf_len;
> 
> 	spinlock_t lock;
> 
> 	struct completion cmd_completion;
> 	struct mutex mutex;
> 
> 	struct list_head client_list;
> 
> 	bool disconnect;
> 
> 	u32 reset_id;
> };
> 
> > > > Why are you not verifying that you sent what you wanted to send?
> > >
> > > As I said, the actual_length is the actual length of data copied to
> > > user buffer instead of the length of what we want to send.
> > >
> > > And even for verifying the length of what we want to send, the max
> > > length of the message is sizeof(struct ljca_msg) +
> > > LJCA_MAX_PACKET_SIZE which is less than the endpoint's max packet
> > > size, so I don't check the actual sent length in above usb_bulk_msg().
> >
> > But you need to.
> 
> Ack, thanks.
> 
> >
> > > > When you call this function, sometimes you check that the function
> > > > sent the proper amount of data, but in many places you do not, and
> > > > you assume that the full buffer was sent, which is not correct.
> > > > So please change _this_ function to check that you sent the proper
> > > > amount and then the caller logic will be much simpler and actually
> > > > work like you are using it in many places (some places you got it
> > > > right, some wrong, which is a HUGE indication that the API is
> > > > wrong because you wrote this code, and if you can't get it
> > > > right...)
> > >
> > > As I said, the return value of this function is the response data
> > > length instead of sent data length. And in this patch set, every
> > > caller has verified if the response length matched with the sent command.
> >
> > No, I found many users that did not do this.
> 
> Ack, I will check more, thanks
> 
> >Please make the api easy to use, right now it's not.
> 
> I post the new ljca_send() here to try to address several comments.
> 
> First it changes the type of buffer length to u8, second it checks the actual sent of
> length usb_bulk_msg(). It removes the scoped guard as well.
> 
> And below gives an explanation of the parameters and return value.
> 
> The parameters(type, cmd, obuf_len) are used to construct message header, obuf
> is used for message data. And ibuf and ibuf_len are for response buffer and buffer
> length passed by API users. ack indicates if the command need an ack from
> firmware, timeout is timeout value to wait command ack.
> 
> And the return value is the response copied to response buffer passed by API
> users, normally the users know how large buffer they have to pass to this API, but
> from coding perspective we should check the passed response buffer
> length(ibuf_len) and return the actual copied data length to the buffer.
> 
> Hope that helps, thanks
> 
> static int ljca_send(struct ljca_adapter *adap, u8 type, u8 cmd,
> 		      const u8 *obuf, u8 obuf_len, u8 *ibuf, u8 ibuf_len,
> 		      bool ack, unsigned long timeout) {
> 	unsigned int msg_len = sizeof(struct ljca_msg) + obuf_len;
> 	struct ljca_msg *header = adap->tx_buf;
> 	unsigned int transferred;
> 	unsigned long flags;
> 	int ret;
> 
> 	if (adap->disconnect)
> 		return -ENODEV;
> 
> 	if (msg_len > adap->tx_buf_len)
> 		return -EINVAL;
> 
> 	mutex_lock(&adap->mutex);
> 
> 	spin_lock_irqsave(&adap->lock, flags);
> 
> 	header->type = type;
> 	header->cmd = cmd;
> 	header->len = obuf_len;
> 	if (obuf)
> 		memcpy(header->data, obuf, obuf_len);
> 
> 	header->flags = LJCA_CMPL_FLAG | (ack ? LJCA_ACK_FLAG : 0);
> 
> 	adap->ex_buf = ibuf;
> 	adap->ex_buf_len = ibuf_len;
> 	adap->actual_length = 0;
> 
> 	spin_unlock_irqrestore(&adap->lock, flags);
> 
>  	reinit_completion(&adap->cmd_completion);
> 
> 	ret = usb_autopm_get_interface(adap->intf);
> 	if (ret < 0)
> 		goto out;
> 
> 	ret = usb_bulk_msg(adap->usb_dev, adap->tx_pipe, header,
> 			    msg_len, &transferred, LJCA_WRITE_TIMEOUT_MS);
> 
> 	usb_autopm_put_interface(adap->intf);
> 
> 	if (ret < 0)
> 		goto out;
> 	if (transferred != msg_len) {
> 		ret = -EIO;
> 		goto out;
> 	}
> 
> 	if (ack) {
> 		ret = wait_for_completion_timeout(&adap->cmd_completion,
> 						       timeout);
> 		if (!ret) {
> 			ret = -ETIMEDOUT;
> 			goto out;
> 		}
> 	}
> 	ret = adap->actual_length;
> 
> out:
> 	spin_lock_irqsave(&adap->lock, flags);
> 	adap->ex_buf = NULL;
> 	adap->ex_buf_len = 0;
> 
> 	memset(header, 0, sizeof(*header));
> 	spin_unlock_irqrestore(&adap->lock, flags);
> 
> 	mutex_unlock(&adap->mutex);
> 
> 	return ret;
> }
> 
> > > > Are you sure this is a valid uid to use?  If so, why?  What
> > > > happens if this gets set to "0" for multiple ones?  Don't
> > > > underestimate broken firmware tables, but also don't paper over
> > > > problems in ways that will be impossible to notice and can cause problems.
> > >
> > > This actually has been discussed in previous email as bellow:
> > >
> > > some DSDTs have only 1 ACPI companion for the 2 I2C controllers and
> > > these don't set a UID at all. On these models only the first I2C
> > > controller is used. So if a HID match has no UID use "0" for the HID.
> > > assigning the ACPI companion to the first I2C controller.
> > > An example device with this setup is the Dell Latitude 9420.
> >
> > Then document this really really well in the code itself, otherwise it looks
> broken.
> 
> Ack, thanks, I post the new ljca_match_device_ids() here, hope that helps, thanks.
> 
> static int ljca_match_device_ids(struct acpi_device *adev, void *data) {
> 	struct ljca_match_ids_walk_data *wd = data;
> 	const char *uid = acpi_device_uid(adev);
> 
> 	if (acpi_match_device_ids(adev, wd->ids))
> 		return 0;
> 
> 	if (!wd->uid)
> 		goto match;
> 
> 	if (!uid)
> 		/*
> 		 * Some DSDTs have only one ACPI companion for the two I2C
> 		 * controllers and they don't set a UID at all (e.g. Dell
>  		 * Latitude 9420). On these platforms only the first I2C
> 		 * controller is used, so if a HID match has no UID we use
> 		 * "0" as the UID and assign ACPI companion to the first
> 		 * I2C controller.
> 		 */
> 		uid = "0";
> 	else
> 		uid = strchr(uid, wd->uid[0]);
> 
> 	if (!uid || strcmp(uid, wd->uid))
> 		return 0;
> 
> match:
> 	wd->adev = adev;
> 
> 	return 1;
> }
> 
> > > This is to be compatible with old version FW which does not support
> > > full USB2xxx functions, so it just warn here as this is.
> >
> > Why do you have to support obsolete and broken firmware?  Can't it be
> updated?
> 
> make sense, and probably they have to update, thanks
> 
> > You warn, yet return success?  Again, that doesn't work well as you
> > never know if you need to unwind it or not.
> >
> > Either report an error and handle it, or don't, but what you have here
> > looks broken as-is.
> 
> Ack, it should report error and handle it. Thanks, the function will be like below:
> 
> static int ljca_enumerate_clients(struct ljca_adapter *adap) {
> 	struct ljca_client *client, *next;
> 	int ret;
> 
> 	ret = ljca_reset_handshake(adap);
> 	if (ret)
> 		goto err_kill;
> 
> 	ret = ljca_enumerate_gpio(adap);
> 	if (ret) {
> 		dev_err(adap->dev, "enumerate GPIO error\n");
> 		goto err_kill;
>         	}
> 
> 	ret = ljca_enumerate_i2c(adap);
> 	if (ret) {
> 		dev_err(adap->dev, "enumerate I2C error\n");
> 		goto err_kill;
> 	}
> 
> 	ret = ljca_enumerate_spi(adap);
> 	if (ret) {
> 		dev_err(adap->dev, "enumerate SPI error\n");
> 		goto err_kill;
> 	}
> 
> 	return 0;
> 
> err_kill:
> 	adap->disconnect = true;
> 
> 	usb_kill_urb(adap->rx_urb);
> 
> 	list_for_each_entry_safe_reverse(client, next, &adap->client_list, link) {
> 		auxiliary_device_delete(&client->auxdev);
> 		auxiliary_device_uninit(&client->auxdev);
> 
> 		list_del_init(&client->link);
> 		kfree(client);
> 	}
> 
> 	return ret;
> }
> 

Hi Greg,

Could you please share your comment about this? If no more comment, I will
sent out next version patch set. Thanks

BR,
Wentong

> 
> >
> > thanks,
> >
> > greg k-h