LinuxLists.cc - [PATCH v20 0/4] Add Intel LJCA device driver

[permalink] [raw]

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

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 <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Tested-by: Hans de Goede <[email protected]>
---
drivers/i2c/busses/Kconfig | 11 ++
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-ljca.c | 343 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 355 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..b492762
--- /dev/null
+++ b/drivers/i2c/busses/i2c-ljca.c
@@ -0,0 +1,343 @@
+// 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[] __counted_by(len);
+} __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 | (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,
+};
+
+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 <[email protected]>");
+MODULE_AUTHOR("Zhifeng Wang <[email protected]>");
+MODULE_AUTHOR("Lixu Zhang <[email protected]>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB-I2C driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LJCA);
--
2.7.4

2023-10-09 06:34:36

[permalink] [raw]

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

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 <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Acked-by: Linus Walleij <[email protected]>
Acked-by: Bartosz Golaszewski <[email protected]>
Tested-by: Hans de Goede <[email protected]>
---
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..dfec9fb 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[] __counted_by(num);
} __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 <[email protected]>");
-MODULE_AUTHOR("Wang Zhifeng <[email protected]>");
-MODULE_AUTHOR("Zhang Lixu <[email protected]>");
+MODULE_AUTHOR("Wentong Wu <[email protected]>");
+MODULE_AUTHOR("Zhifeng Wang <[email protected]>");
+MODULE_AUTHOR("Lixu Zhang <[email protected]>");
MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB-GPIO driver");
MODULE_LICENSE("GPL");
MODULE_IMPORT_NS(LJCA);
--
2.7.4

2023-10-09 06:34:46

[permalink] [raw]

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

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 <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Tested-by: Hans de Goede <[email protected]>
---
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..c5a066c
--- /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[] __counted_by(len);
+} __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 <[email protected]>");
+MODULE_AUTHOR("Zhifeng Wang <[email protected]>");
+MODULE_AUTHOR("Lixu Zhang <[email protected]>");
+MODULE_DESCRIPTION("Intel La Jolla Cove Adapter USB-SPI driver");
+MODULE_LICENSE("GPL");
+MODULE_IMPORT_NS(LJCA);
--
2.7.4

2023-10-09 06:35:05

[permalink] [raw]

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

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 <[email protected]>
Reviewed-by: Sakari Ailus <[email protected]>
Reviewed-by: Andi Shyti <[email protected]>
Tested-by: Hans de Goede <[email protected]>
---
drivers/usb/misc/Kconfig | 13 +
drivers/usb/misc/Makefile | 1 +
drivers/usb/misc/usb-ljca.c | 902 ++++++++++++++++++++++++++++++++++++++++++++
include/linux/usb/ljca.h | 145 +++++++
4 files changed, 1061 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..c9decd0
--- /dev/null
+++ b/drivers/usb/misc/usb-ljca.c
@@ -0,0 +1,902 @@
+// 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/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[] __counted_by(len);
+} __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[] __counted_by(num);
+} __packed;
+
+struct ljca_spi_ctr_info {
+ u8 id;
+ u8 capacity;
+ u8 intr_pin;
+} __packed;
+
+struct ljca_spi_descriptor {
+ u8 num;
+ struct ljca_spi_ctr_info info[] __counted_by(num);
+} __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[] __counted_by(bank_num);
+} __packed;
+
+/**
+ * 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;
+};
+
+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) {
+ /*
+ * Currently only GPIO register event callback, but
+ * firmware message structure should include id when
+ * multiple same type clients register event callback.
+ */
+ if (client->type == header->type) {
+ unsigned long flags;
+
+ spin_lock_irqsave(&client->event_cb_lock, flags);
+ client->event_cb(client->context, header->cmd,
+ header->data, header->len);
+ spin_unlock_irqrestore(&client->event_cb_lock, flags);
+
+ break;
+ }
+ }
+}
+
+/* process command ack and received data if available */
+static void ljca_handle_cmd_ack(struct ljca_adapter *adap, struct ljca_msg *header)
+{
+ struct ljca_msg *tx_header = adap->tx_buf;
+ u8 ibuf_len, actual_len = 0;
+ unsigned long flags;
+ u8 *ibuf;
+
+ spin_lock_irqsave(&adap->lock, flags);
+
+ 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;
+ }
+
+ ibuf_len = adap->ex_buf_len;
+ ibuf = adap->ex_buf;
+
+ if (ibuf && ibuf_len) {
+ actual_len = min(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;
+
+ 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);
+}
+
+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;
+}
+
+int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
+ u8 obuf_len, u8 *ibuf, u8 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,
+ u8 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)
+{
+ unsigned long flags;
+
+ spin_lock_irqsave(&client->event_cb_lock, flags);
+
+ client->event_cb = NULL;
+ client->context = NULL;
+
+ spin_unlock_irqrestore(&client->event_cb_lock, flags);
+}
+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)
+ /*
+ * 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;
+}
+
+/* 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;
+
+ /* check firmware response */
+ desc = (struct ljca_spi_descriptor *)buf;
+ if (ret != struct_size(desc, info, desc->num))
+ return -EINVAL;
+
+ 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)
+{
+ 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;
+}
+
+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_put;
+ }
+ 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_put;
+ }
+
+ /* alloc rx urb */
+ adap->rx_urb = usb_alloc_urb(0, GFP_KERNEL);
+ if (!adap->rx_urb) {
+ ret = -ENOMEM;
+ goto err_put;
+ }
+ 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_free;
+
+ usb_enable_autosuspend(usb_dev);
+
+ return 0;
+
+err_free:
+ usb_free_urb(adap->rx_urb);
+
+err_put:
+ 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 <[email protected]>");
+MODULE_AUTHOR("Zhifeng Wang <[email protected]>");
+MODULE_AUTHOR("Lixu Zhang <[email protected]>");
+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..47661fe
--- /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 of response data for success, negative value for errors
+ */
+int ljca_transfer(struct ljca_client *client, u8 cmd, const u8 *obuf,
+ u8 obuf_len, u8 *ibuf, u8 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,
+ u8 obuf_len);
+
+#endif
--
2.7.4

2023-10-09 10:22:41

by Oliver Neukum

[permalink] [raw]

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

On 09.10.23 08:33, Wentong Wu wrote:
> 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 <[email protected]>
> Reviewed-by: Sakari Ailus <[email protected]>
> Reviewed-by: Andi Shyti <[email protected]>
> Tested-by: Hans de Goede <[email protected]>
Reviewed-by: Oliver Neukum <[email protected]>

2023-10-09 12:50:59

by Mark Brown

[permalink] [raw]

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

On Mon, Oct 09, 2023 at 02:33:24PM +0800, Wentong Wu wrote:
> 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.

Reviewed-by: Mark Brown <[email protected]>

Attachments:

(No filename) (335.00 B)
signature.asc (499.00 B)
Download all attachments

2023-10-10 19:26:46

by Wolfram Sang

[permalink] [raw]

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

On Mon, Oct 09, 2023 at 02:33:23PM +0800, Wentong Wu wrote:
> 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 <[email protected]>
> Reviewed-by: Sakari Ailus <[email protected]>
> Reviewed-by: Andi Shyti <[email protected]>
> Tested-by: Hans de Goede <[email protected]>

I2C driver looks good. Waiting for the USB part to be applied.

Reviewed-by: Wolfram Sang <[email protected]>

Attachments:

(No filename) (623.00 B)
signature.asc (849.00 B)
Download all attachments

2023-10-11 10:21:54

[permalink] [raw]

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

On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
> 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)
> }
> }

This commit message is not true anymore, or misleading at bare minimum.
The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
they must NOT be used together for the same device node. So, can you
clarify how the DSDT is organized and update the commit message and
it may require (quite likely) to redesign the architecture of this
driver. Sorry I missed this from previous rounds as I was busy by
something else.

Greg, please do not promote this to the next before above will be clarified.

P.S> Using _ADR and _HID together is an immediate NAK from me.

--
With Best Regards,
Andy Shevchenko

2023-10-11 10:38:53

[permalink] [raw]

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

Hi,

On 10/11/23 12:21, Andy Shevchenko wrote:
> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
>> 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)
>> }
>> }
>
> This commit message is not true anymore, or misleading at bare minimum.
> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
> they must NOT be used together for the same device node. So, can you
> clarify how the DSDT is organized and update the commit message and
> it may require (quite likely) to redesign the architecture of this
> driver. Sorry I missed this from previous rounds as I was busy by
> something else.

This part of the commit message unfortunately is not accurate.
_ADR is not used in either DSDTs of shipping hw; nor in the code.

The code in question parsing the relevant part of the DSDT looks
like this:

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;
}

Notice that it check _UID (for some child devices, only those of
which there may be more then 1 have a UID set in the DSDT) and
that in case of requested but missing UID it assumes UID = "0"
for compatibility with older DSDTs which lack the UID.

And relevant DSDT bits from early hw (TigerLake Dell Latitude 9420)
Note no UID for the I2C node even though the LJCA USB IO expander
has 2 I2C controllers :

Scope (_SB.PC00.XHCI.RHUB.HS06)
{
Device (VGPO)
{
Name (_HID, "INTC1074") // _HID: Hardware ID
Name (_DDN, "Intel UsbGpio Device") // _DDN: DOS Device Name
}

Device (VI2C)
{
Name (_HID, "INTC1075") // _HID: Hardware ID
Name (_DDN, "Intel UsbI2C Device") // _DDN: DOS Device Name
}
}

And for newer hw (Lenovo Thinkpad X1 yoga gen7, alder lake):

Scope (_SB.PC00.XHCI.RHUB.HS08)
{
Device (VGPO)
{
Name (_HID, "INTC1096") // _HID: Hardware ID
Name (_DDN, "Intel UsbGpio Device") // _DDN: DOS Device Name
}

Device (VIC0)
{
Name (_HID, "INTC1097") // _HID: Hardware ID
Name (_DDN, "Intel UsbI2C Device") // _DDN: DOS Device Name
Name (_UID, Zero) // _UID: Unique ID
}

Device (VIC1)
{
Name (_HID, "INTC1097") // _HID: Hardware ID
Name (_DDN, "Intel UsbI2C Device") // _DDN: DOS Device Name
Name (_UID, One) // _UID: Unique ID
}

Device (VSPI)
{
Name (_HID, "INTC1098") // _HID: Hardware ID
Name (_DDN, "Intel UsbSPI Device") // _DDN: DOS Device Name
}
}

Note UIDs are used for the I2C controllers but not for the singleton
SPI and GPIO controllers.

TL;DR: there is nothing to worry about here, but the commit message
should be updated to reflect reality.

Regards,

Hans

2023-10-11 10:44:21

[permalink] [raw]

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

On Wed, Oct 11, 2023 at 12:37:51PM +0200, Hans de Goede wrote:
> On 10/11/23 12:21, Andy Shevchenko wrote:
> > On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:

...

> TL;DR: there is nothing to worry about here, but the commit message
> should be updated to reflect reality.

I have just sent the similar worry, but thanks that you have checked
the code and we don't need to worry too much.

--
With Best Regards,
Andy Shevchenko

2023-10-11 12:51:25

[permalink] [raw]

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

> From: Hans de Goede <hdegoede>
>
> Hi,
>
> On 10/11/23 12:21, Andy Shevchenko wrote:
> > On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
> >> 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)
> >> }
> >> }
> >
> > This commit message is not true anymore, or misleading at bare minimum.
> > The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
> > they must NOT be used together for the same device node. So, can you
> > clarify how the DSDT is organized and update the commit message and it
> > may require (quite likely) to redesign the architecture of this
> > driver. Sorry I missed this from previous rounds as I was busy by
> > something else.
>
> This part of the commit message unfortunately is not accurate.
> _ADR is not used in either DSDTs of shipping hw; nor in the code.

We have covered the _ADR in the code like below, it first try to find the
child device based on _ADR, if not found, it will check the _HID, and there
is clear comment in the function.

/* 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);

>
> The code in question parsing the relevant part of the DSDT looks like this:
>
> 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;
> }
>
> Notice that it check _UID (for some child devices, only those of which there may
> be more then 1 have a UID set in the DSDT) and that in case of requested but
> missing UID it assumes UID = "0"
> for compatibility with older DSDTs which lack the UID.
>
> And relevant DSDT bits from early hw (TigerLake Dell Latitude 9420) Note no UID
> for the I2C node even though the LJCA USB IO expander has 2 I2C controllers :
>
> Scope (_SB.PC00.XHCI.RHUB.HS06)
> {
> Device (VGPO)
> {
> Name (_HID, "INTC1074") // _HID: Hardware ID
> Name (_DDN, "Intel UsbGpio Device") // _DDN: DOS Device Name
> }
>
> Device (VI2C)
> {
> Name (_HID, "INTC1075") // _HID: Hardware ID
> Name (_DDN, "Intel UsbI2C Device") // _DDN: DOS Device Name
> }
> }
>
> And for newer hw (Lenovo Thinkpad X1 yoga gen7, alder lake):
>
> Scope (_SB.PC00.XHCI.RHUB.HS08)
> {
> Device (VGPO)
> {
> Name (_HID, "INTC1096") // _HID: Hardware ID
> Name (_DDN, "Intel UsbGpio Device") // _DDN: DOS Device Name
> }
>
> Device (VIC0)
> {
> Name (_HID, "INTC1097") // _HID: Hardware ID
> Name (_DDN, "Intel UsbI2C Device") // _DDN: DOS Device Name
> Name (_UID, Zero) // _UID: Unique ID
> }
>
> Device (VIC1)
> {
> Name (_HID, "INTC1097") // _HID: Hardware ID
> Name (_DDN, "Intel UsbI2C Device") // _DDN: DOS Device Name
> Name (_UID, One) // _UID: Unique ID
> }
>
> Device (VSPI)
> {
> Name (_HID, "INTC1098") // _HID: Hardware ID
> Name (_DDN, "Intel UsbSPI Device") // _DDN: DOS Device Name
> }
> }
>
> Note UIDs are used for the I2C controllers but not for the singleton SPI and GPIO
> controllers.
>
> TL;DR: there is nothing to worry about here, but the commit message should be
> updated to reflect reality.
>
> Regards,
>
> Hans

2023-10-12 11:16:37

[permalink] [raw]

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

Hi,

On 10/11/23 14:50, Wu, Wentong wrote:
>> From: Hans de Goede <hdegoede>
>>
>> Hi,
>>
>> On 10/11/23 12:21, Andy Shevchenko wrote:
>>> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
>>>> 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)
>>>> }
>>>> }
>>>
>>> This commit message is not true anymore, or misleading at bare minimum.
>>> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
>>> they must NOT be used together for the same device node. So, can you
>>> clarify how the DSDT is organized and update the commit message and it
>>> may require (quite likely) to redesign the architecture of this
>>> driver. Sorry I missed this from previous rounds as I was busy by
>>> something else.
>>
>> This part of the commit message unfortunately is not accurate.
>> _ADR is not used in either DSDTs of shipping hw; nor in the code.
>
> We have covered the _ADR in the code like below, it first try to find the
> child device based on _ADR, if not found, it will check the _HID, and there
> is clear comment in the function.
>
> /* 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);

Ah ok, I see. So the code:

1. First tries to find the matching child acpi_device for the auxdev by ADR

2. If 1. fails then falls back to HID + UID matching

And there are DSDTs which use either:

1. Only use _ADR to identify which child device is which, like the example
DSDT snippet from the commit msg.

2. Only use _HID + _UID like the 2 example DSDT snippets from me email

But there never is a case where both _ADR and _HID are used at
the same time (which would be an ACPI spec violation as Andy said).

So AFAICT there is no issue here since _ADR and _HID are never
user at the same time and the commit message correctly describes
scenario 1. from above, so the commit message is fine too.

So I believe that we can continue with this patch series in
its current v20 form, which has already been staged for
going into -next by Greg.

Andy can you confirm that moving ahead with the current
version is ok ?

Regards,

Hans

2023-10-13 20:06:01

[permalink] [raw]

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

On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> On 10/11/23 14:50, Wu, Wentong wrote:
> >> On 10/11/23 12:21, Andy Shevchenko wrote:
> >>> On Mon, Oct 09, 2023 at 02:33:22PM +0800, Wentong Wu wrote:
> >>>> 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)
> >>>> }
> >>>> }
> >>>
> >>> This commit message is not true anymore, or misleading at bare minimum.
> >>> The ACPI specification is crystal clear about usage _ADR and _HID, i.e.
> >>> they must NOT be used together for the same device node. So, can you
> >>> clarify how the DSDT is organized and update the commit message and it
> >>> may require (quite likely) to redesign the architecture of this
> >>> driver. Sorry I missed this from previous rounds as I was busy by
> >>> something else.
> >>
> >> This part of the commit message unfortunately is not accurate.
> >> _ADR is not used in either DSDTs of shipping hw; nor in the code.
> >
> > We have covered the _ADR in the code like below, it first try to find the
> > child device based on _ADR, if not found, it will check the _HID, and there
> > is clear comment in the function.
> >
> > /* 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);
>
> Ah ok, I see. So the code:
>
> 1. First tries to find the matching child acpi_device for the auxdev by ADR
>
> 2. If 1. fails then falls back to HID + UID matching
>
> And there are DSDTs which use either:
>
> 1. Only use _ADR to identify which child device is which, like the example
> DSDT snippet from the commit msg.
>
> 2. Only use _HID + _UID like the 2 example DSDT snippets from me email
>
> But there never is a case where both _ADR and _HID are used at
> the same time (which would be an ACPI spec violation as Andy said).
>
> So AFAICT there is no issue here since _ADR and _HID are never
> user at the same time and the commit message correctly describes
> scenario 1. from above, so the commit message is fine too.
>
> So I believe that we can continue with this patch series in
> its current v20 form, which has already been staged for
> going into -next by Greg.
>
> Andy can you confirm that moving ahead with the current
> version is ok ?

Yes as we have a few weeks to fix corner cases.

What I'm worrying is that opening door for _ADR that seems never used is kinda
an overkill here (resolving non-existing problem). Looking at the design of the
driver I'm not sure why ACPI HIDs are collected somewhere else than in the
respective drivers. And looking at the ID lists themselves I am not sure why
the firmware of the respective hardware platforms are not using _CID.

--
With Best Regards,
Andy Shevchenko

2023-10-14 10:58:59

[permalink] [raw]

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

Hi Andy,

On 10/13/23 22:05, Shevchenko, Andriy wrote:
> On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:

<snip>

>> Ah ok, I see. So the code:
>>
>> 1. First tries to find the matching child acpi_device for the auxdev by ADR
>>
>> 2. If 1. fails then falls back to HID + UID matching
>>
>> And there are DSDTs which use either:
>>
>> 1. Only use _ADR to identify which child device is which, like the example
>> DSDT snippet from the commit msg.
>>
>> 2. Only use _HID + _UID like the 2 example DSDT snippets from me email
>>
>> But there never is a case where both _ADR and _HID are used at
>> the same time (which would be an ACPI spec violation as Andy said).
>>
>> So AFAICT there is no issue here since _ADR and _HID are never
>> user at the same time and the commit message correctly describes
>> scenario 1. from above, so the commit message is fine too.
>>
>> So I believe that we can continue with this patch series in
>> its current v20 form, which has already been staged for
>> going into -next by Greg.
>>
>> Andy can you confirm that moving ahead with the current
>> version is ok ?
>
> Yes as we have a few weeks to fix corner cases.
>
> What I'm worrying is that opening door for _ADR that seems never used is kinda
> an overkill here (resolving non-existing problem).

I assume that there actually some DSDTs using the _ADR approach
and that this support is not there just for fun.

Wentong, can you confirm that the _ADR using codepaths are
actually used on some hardware / with some DSDTs out there ?

> Looking at the design of the
> driver I'm not sure why ACPI HIDs are collected somewhere else than in the
> respective drivers. And looking at the ID lists themselves I am not sure why
> the firmware of the respective hardware platforms are not using _CID.

This is a USB device which has 4 functions:

1. GPIO controller
2. I2C controller 1
3. I2C controller 2
4. SPI controller

The driver for the main USB interface uses
the new auxbus to create 4 child devices. The _ADR
or if that fails _HID + _UID matching is done to
find the correct acpi_device child of the acpi_device
which is the ACPI-companion of the main USB device.

After looking up the correct acpi_device child
this is then set as the fwnode / ACPI-companion
of the auxbus device created for that function.

Having the correct fwnode is important because other
parts of the DSDT reference this fwnode to specify
GPIO / I2C / SPI resources and if the fwnode of
the aux-device is not set correctly then the resources
for other devices referencing it (typically a camera
sensor) can not be found.

As for why the driver for the auxbus devices / children
do not use HID matching, AFAIK the auxbus has no support
for using ACPI (or DT) matching for aux-devices and these
drivers need to be auxiliary_driver's and bind to the
auxbus device and not to a platform_device instantiated for
the acpi_device since they need the auxbus device to access
the USB device.

Regards,

Hans

2023-10-16 05:52:59

[permalink] [raw]

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

> From: Hans de Goede <hdegoede>
>
> Hi Andy,
>
> On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
>
> <snip>
>
> >> Ah ok, I see. So the code:
> >>
> >> 1. First tries to find the matching child acpi_device for the auxdev
> >> by ADR
> >>
> >> 2. If 1. fails then falls back to HID + UID matching
> >>
> >> And there are DSDTs which use either:
> >>
> >> 1. Only use _ADR to identify which child device is which, like the example
> >> DSDT snippet from the commit msg.
> >>
> >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> >> email
> >>
> >> But there never is a case where both _ADR and _HID are used at the
> >> same time (which would be an ACPI spec violation as Andy said).
> >>
> >> So AFAICT there is no issue here since _ADR and _HID are never user
> >> at the same time and the commit message correctly describes scenario
> >> 1. from above, so the commit message is fine too.
> >>
> >> So I believe that we can continue with this patch series in its
> >> current v20 form, which has already been staged for going into -next
> >> by Greg.
> >>
> >> Andy can you confirm that moving ahead with the current version is ok
> >> ?
> >
> > Yes as we have a few weeks to fix corner cases.
> >
> > What I'm worrying is that opening door for _ADR that seems never used
> > is kinda an overkill here (resolving non-existing problem).
>
> I assume that there actually some DSDTs using the _ADR approach and that this
> support is not there just for fun.

right, it's not for fun, we use _ADR here is to reduce the maintain effort because
currently it defines _HID for every new platform and the drivers have to be updated
accordingly, while _ADR doesn't have that problem.

> Wentong, can you confirm that the _ADR using codepaths are actually used on
> some hardware / with some DSDTs out there ?

what I can share is that we will see.

> > Looking at the design of the
> > driver I'm not sure why ACPI HIDs are collected somewhere else than in
> > the respective drivers.

AFAIK, auxiliary bus doesn't support parsing fwnodes currently. Probably we can
support it for auxiliary bus in another patch.

> > And looking at the ID lists themselves I am
> > not sure why the firmware of the respective hardware platforms are not using
> _CID.

I think firmware can select _CID as well, but the shipped hw doesn't use _CID,
the driver has to make sure the shipped hw working as well. And switching to _CID
for the shipped hw is not easy, and it has to change windows driver as well.

>
> This is a USB device which has 4 functions:
>
> 1. GPIO controller
> 2. I2C controller 1
> 3. I2C controller 2
> 4. SPI controller
>
> The driver for the main USB interface uses the new auxbus to create 4 child
> devices. The _ADR or if that fails _HID + _UID matching is done to find the
> correct acpi_device child of the acpi_device which is the ACPI-companion of the
> main USB device.
>
> After looking up the correct acpi_device child this is then set as the fwnode /
> ACPI-companion of the auxbus device created for that function.
>
> Having the correct fwnode is important because other parts of the DSDT
> reference this fwnode to specify GPIO / I2C / SPI resources and if the fwnode of
> the aux-device is not set correctly then the resources for other devices
> referencing it (typically a camera
> sensor) can not be found.
>
> As for why the driver for the auxbus devices / children do not use HID matching,
> AFAIK the auxbus has no support for using ACPI (or DT) matching for aux-devices
> and these drivers need to be auxiliary_driver's and bind to the auxbus device and
> not to a platform_device instantiated for the acpi_device since they need the
> auxbus device to access the USB device.

Yes, total agree. Thanks

Thanks
Wentong
>
> Regards,
>
> Hans
>

2023-10-16 07:36:45

[permalink] [raw]

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

Hi,

On 10/16/23 07:52, Wu, Wentong wrote:
>> From: Hans de Goede <hdegoede>
>>
>> Hi Andy,
>>
>> On 10/13/23 22:05, Shevchenko, Andriy wrote:
>>> On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
>>
>> <snip>
>>
>>>> Ah ok, I see. So the code:
>>>>
>>>> 1. First tries to find the matching child acpi_device for the auxdev
>>>> by ADR
>>>>
>>>> 2. If 1. fails then falls back to HID + UID matching
>>>>
>>>> And there are DSDTs which use either:
>>>>
>>>> 1. Only use _ADR to identify which child device is which, like the example
>>>> DSDT snippet from the commit msg.
>>>>
>>>> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
>>>> email
>>>>
>>>> But there never is a case where both _ADR and _HID are used at the
>>>> same time (which would be an ACPI spec violation as Andy said).
>>>>
>>>> So AFAICT there is no issue here since _ADR and _HID are never user
>>>> at the same time and the commit message correctly describes scenario
>>>> 1. from above, so the commit message is fine too.
>>>>
>>>> So I believe that we can continue with this patch series in its
>>>> current v20 form, which has already been staged for going into -next
>>>> by Greg.
>>>>
>>>> Andy can you confirm that moving ahead with the current version is ok
>>>> ?
>>>
>>> Yes as we have a few weeks to fix corner cases.
>>>
>>> What I'm worrying is that opening door for _ADR that seems never used
>>> is kinda an overkill here (resolving non-existing problem).
>>
>> I assume that there actually some DSDTs using the _ADR approach and that this
>> support is not there just for fun.
>
> right, it's not for fun, we use _ADR here is to reduce the maintain effort because
> currently it defines _HID for every new platform and the drivers have to be updated
> accordingly, while _ADR doesn't have that problem.

Hmm, this sounds to me like _ADR is currently not actually being used in any
shipping DSDTs ?

Adding support for it to the driver seems a bit premature then IMHO ?

Also HIDs can perfectly be re-used for compatible hardware in
a newer generation so that is really not a good argument to use _ADR
instead.

Regards,

Hans

2023-10-16 07:40:01

[permalink] [raw]

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

On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:

<snip>

> > >> Ah ok, I see. So the code:
> > >>
> > >> 1. First tries to find the matching child acpi_device for the auxdev
> > >> by ADR
> > >>
> > >> 2. If 1. fails then falls back to HID + UID matching
> > >>
> > >> And there are DSDTs which use either:
> > >>
> > >> 1. Only use _ADR to identify which child device is which, like the example
> > >> DSDT snippet from the commit msg.
> > >>
> > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> > >> email
> > >>
> > >> But there never is a case where both _ADR and _HID are used at the
> > >> same time (which would be an ACPI spec violation as Andy said).
> > >>
> > >> So AFAICT there is no issue here since _ADR and _HID are never user
> > >> at the same time and the commit message correctly describes scenario
> > >> 1. from above, so the commit message is fine too.
> > >>
> > >> So I believe that we can continue with this patch series in its
> > >> current v20 form, which has already been staged for going into -next
> > >> by Greg.
> > >>
> > >> Andy can you confirm that moving ahead with the current version is ok
> > >> ?
> > >
> > > Yes as we have a few weeks to fix corner cases.
> > >
> > > What I'm worrying is that opening door for _ADR that seems never used
> > > is kinda an overkill here (resolving non-existing problem).
> >
> > I assume that there actually some DSDTs using the _ADR approach and that this
> > support is not there just for fun.
>
> right, it's not for fun, we use _ADR here is to reduce the maintain effort because
> currently it defines _HID for every new platform and the drivers have to be updated
> accordingly, while _ADR doesn't have that problem.

But this does not confirm if you have such devices. Moreover, My question about
_CID per function stays the same. Why firmware is not using it? In that case
you need only one ID per function in the driver (it might require some IDs in
the _HID, I don't remember that part of the spec by heart, i.e. if _CID can be
only provided with existing _HID or not).

> > Wentong, can you confirm that the _ADR using codepaths are actually used on
> > some hardware / with some DSDTs out there ?
>
> what I can share is that we will see.
>
> > > Looking at the design of the
> > > driver I'm not sure why ACPI HIDs are collected somewhere else than in
> > > the respective drivers.
>
> AFAIK, auxiliary bus doesn't support parsing fwnodes currently. Probably we can
> support it for auxiliary bus in another patch.

This is good idea!

> > > And looking at the ID lists themselves I am
> > > not sure why the firmware of the respective hardware platforms are not using
> > _CID.
>
> I think firmware can select _CID as well, but the shipped hw doesn't use _CID,
> the driver has to make sure the shipped hw working as well. And switching to _CID
> for the shipped hw is not easy, and it has to change windows driver as well.

I understand, but at least you may stop growing list in the driver. And actually
using separate IDs for multifunctional device seems not ideal solution to me.

> > This is a USB device which has 4 functions:

Yes, I understand this part, but thank you for elaboration about auxbus, which
seems lack of needed support. And I would really like to see someone adds it there.

> > 1. GPIO controller
> > 2. I2C controller 1
> > 3. I2C controller 2
> > 4. SPI controller
> >
> > The driver for the main USB interface uses the new auxbus to create 4 child
> > devices. The _ADR or if that fails _HID + _UID matching is done to find the
> > correct acpi_device child of the acpi_device which is the ACPI-companion of the
> > main USB device.
> >
> > After looking up the correct acpi_device child this is then set as the fwnode /
> > ACPI-companion of the auxbus device created for that function.
> >
> > Having the correct fwnode is important because other parts of the DSDT
> > reference this fwnode to specify GPIO / I2C / SPI resources and if the fwnode of
> > the aux-device is not set correctly then the resources for other devices
> > referencing it (typically a camera
> > sensor) can not be found.
> >
> > As for why the driver for the auxbus devices / children do not use HID matching,
> > AFAIK the auxbus has no support for using ACPI (or DT) matching for aux-devices
> > and these drivers need to be auxiliary_driver's and bind to the auxbus device and
> > not to a platform_device instantiated for the acpi_device since they need the
> > auxbus device to access the USB device.
>
> Yes, total agree. Thanks

--
With Best Regards,
Andy Shevchenko

2023-10-16 15:06:26

[permalink] [raw]

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

> From: Shevchenko, Andriy
> On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
>
> <snip>
>
> > > >> Ah ok, I see. So the code:
> > > >>
> > > >> 1. First tries to find the matching child acpi_device for the
> > > >> auxdev by ADR
> > > >>
> > > >> 2. If 1. fails then falls back to HID + UID matching
> > > >>
> > > >> And there are DSDTs which use either:
> > > >>
> > > >> 1. Only use _ADR to identify which child device is which, like the example
> > > >> DSDT snippet from the commit msg.
> > > >>
> > > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> > > >> email
> > > >>
> > > >> But there never is a case where both _ADR and _HID are used at
> > > >> the same time (which would be an ACPI spec violation as Andy said).
> > > >>
> > > >> So AFAICT there is no issue here since _ADR and _HID are never
> > > >> user at the same time and the commit message correctly describes
> > > >> scenario 1. from above, so the commit message is fine too.
> > > >>
> > > >> So I believe that we can continue with this patch series in its
> > > >> current v20 form, which has already been staged for going into
> > > >> -next by Greg.
> > > >>
> > > >> Andy can you confirm that moving ahead with the current version
> > > >> is ok ?
> > > >
> > > > Yes as we have a few weeks to fix corner cases.
> > > >
> > > > What I'm worrying is that opening door for _ADR that seems never
> > > > used is kinda an overkill here (resolving non-existing problem).
> > >
> > > I assume that there actually some DSDTs using the _ADR approach and
> > > that this support is not there just for fun.
> >
> > right, it's not for fun, we use _ADR here is to reduce the maintain
> > effort because currently it defines _HID for every new platform and
> > the drivers have to be updated accordingly, while _ADR doesn't have that
> problem.
>
> But this does not confirm if you have such devices. Moreover, My question
> about _CID per function stays the same. Why firmware is not using it?

Yes, both _ADR and _CID can stop growing list in the driver. And for _ADR, it also
only require one ID per function. I don't know why BIOS team doesn't select _CID,
but I have suggested use _ADR internally, and , to make things moving forward,
the driver adds support for _ADR here first.

But you're right, _CID is another solution as well, we will discuss it with firmware
team more.

> In that case you need only one ID per function in the driver (it might require some
> IDs in the _HID, I don't remember that part of the spec by heart, i.e. if _CID can be
> only provided with existing _HID or not).
>
> > > Wentong, can you confirm that the _ADR using codepaths are actually
> > > used on some hardware / with some DSDTs out there ?
> >
> > what I can share is that we will see.
> >
> > > > Looking at the design of the
> > > > driver I'm not sure why ACPI HIDs are collected somewhere else
> > > > than in the respective drivers.
> >
> > AFAIK, auxiliary bus doesn't support parsing fwnodes currently.
> > Probably we can support it for auxiliary bus in another patch.
>
> This is good idea!
>
>
> > > > And looking at the ID lists themselves I am not sure why the
> > > > firmware of the respective hardware platforms are not using
> > > _CID.
> >
> > I think firmware can select _CID as well, but the shipped hw doesn't
> > use _CID, the driver has to make sure the shipped hw working as well.
> > And switching to _CID for the shipped hw is not easy, and it has to change
> windows driver as well.
>
> I understand, but at least you may stop growing list in the driver.
Yes,

> And actually using separate IDs for multifunctional device seems not ideal
> solution to me.
Agree, I will consider _CID more, but currently to avoid this and also support
shipped hardware, _ADR is at least a choice.

BR,
Wentong

> > > This is a USB device which has 4 functions:
>
> Yes, I understand this part, but thank you for elaboration about auxbus, which
> seems lack of needed support. And I would really like to see someone adds it
> there.
>
> > > 1. GPIO controller
> > > 2. I2C controller 1
> > > 3. I2C controller 2
> > > 4. SPI controller
> > >
> > > The driver for the main USB interface uses the new auxbus to create
> > > 4 child devices. The _ADR or if that fails _HID + _UID matching is
> > > done to find the correct acpi_device child of the acpi_device which
> > > is the ACPI-companion of the main USB device.
> > >
> > > After looking up the correct acpi_device child this is then set as
> > > the fwnode / ACPI-companion of the auxbus device created for that function.
> > >
> > > Having the correct fwnode is important because other parts of the
> > > DSDT reference this fwnode to specify GPIO / I2C / SPI resources and
> > > if the fwnode of the aux-device is not set correctly then the
> > > resources for other devices referencing it (typically a camera
> > > sensor) can not be found.
> > >
> > > As for why the driver for the auxbus devices / children do not use
> > > HID matching, AFAIK the auxbus has no support for using ACPI (or DT)
> > > matching for aux-devices and these drivers need to be
> > > auxiliary_driver's and bind to the auxbus device and not to a
> > > platform_device instantiated for the acpi_device since they need the auxbus
> device to access the USB device.
> >
> > Yes, total agree. Thanks
>
> --
> With Best Regards,
> Andy Shevchenko
>

2023-10-16 15:19:38

by Greg Kroah-Hartman

[permalink] [raw]

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

On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> > From: Shevchenko, Andriy
> > On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > > > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> >
> > <snip>
> >
> > > > >> Ah ok, I see. So the code:
> > > > >>
> > > > >> 1. First tries to find the matching child acpi_device for the
> > > > >> auxdev by ADR
> > > > >>
> > > > >> 2. If 1. fails then falls back to HID + UID matching
> > > > >>
> > > > >> And there are DSDTs which use either:
> > > > >>
> > > > >> 1. Only use _ADR to identify which child device is which, like the example
> > > > >> DSDT snippet from the commit msg.
> > > > >>
> > > > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from me
> > > > >> email
> > > > >>
> > > > >> But there never is a case where both _ADR and _HID are used at
> > > > >> the same time (which would be an ACPI spec violation as Andy said).
> > > > >>
> > > > >> So AFAICT there is no issue here since _ADR and _HID are never
> > > > >> user at the same time and the commit message correctly describes
> > > > >> scenario 1. from above, so the commit message is fine too.
> > > > >>
> > > > >> So I believe that we can continue with this patch series in its
> > > > >> current v20 form, which has already been staged for going into
> > > > >> -next by Greg.
> > > > >>
> > > > >> Andy can you confirm that moving ahead with the current version
> > > > >> is ok ?
> > > > >
> > > > > Yes as we have a few weeks to fix corner cases.
> > > > >
> > > > > What I'm worrying is that opening door for _ADR that seems never
> > > > > used is kinda an overkill here (resolving non-existing problem).
> > > >
> > > > I assume that there actually some DSDTs using the _ADR approach and
> > > > that this support is not there just for fun.
> > >
> > > right, it's not for fun, we use _ADR here is to reduce the maintain
> > > effort because currently it defines _HID for every new platform and
> > > the drivers have to be updated accordingly, while _ADR doesn't have that
> > problem.
> >
> > But this does not confirm if you have such devices. Moreover, My question
> > about _CID per function stays the same. Why firmware is not using it?
>
> Yes, both _ADR and _CID can stop growing list in the driver. And for _ADR, it also
> only require one ID per function. I don't know why BIOS team doesn't select _CID,
> but I have suggested use _ADR internally, and , to make things moving forward,
> the driver adds support for _ADR here first.
>
> But you're right, _CID is another solution as well, we will discuss it with firmware
> team more.

Should I revert this series now until this gets sorted out?

thanks,

greg k-h

2023-10-16 15:44:39

[permalink] [raw]

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

> From: [email protected]
> On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> > > From: Shevchenko, Andriy
> > > On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
> > > > > On 10/13/23 22:05, Shevchenko, Andriy wrote:
> > > > > > On Thu, Oct 12, 2023 at 01:14:23PM +0200, Hans de Goede wrote:
> > >
> > > <snip>
> > >
> > > > > >> Ah ok, I see. So the code:
> > > > > >>
> > > > > >> 1. First tries to find the matching child acpi_device for the
> > > > > >> auxdev by ADR
> > > > > >>
> > > > > >> 2. If 1. fails then falls back to HID + UID matching
> > > > > >>
> > > > > >> And there are DSDTs which use either:
> > > > > >>
> > > > > >> 1. Only use _ADR to identify which child device is which, like the
> example
> > > > > >> DSDT snippet from the commit msg.
> > > > > >>
> > > > > >> 2. Only use _HID + _UID like the 2 example DSDT snippets from
> > > > > >> me email
> > > > > >>
> > > > > >> But there never is a case where both _ADR and _HID are used
> > > > > >> at the same time (which would be an ACPI spec violation as Andy said).
> > > > > >>
> > > > > >> So AFAICT there is no issue here since _ADR and _HID are
> > > > > >> never user at the same time and the commit message correctly
> > > > > >> describes scenario 1. from above, so the commit message is fine too.
> > > > > >>
> > > > > >> So I believe that we can continue with this patch series in
> > > > > >> its current v20 form, which has already been staged for going
> > > > > >> into -next by Greg.
> > > > > >>
> > > > > >> Andy can you confirm that moving ahead with the current
> > > > > >> version is ok ?
> > > > > >
> > > > > > Yes as we have a few weeks to fix corner cases.
> > > > > >
> > > > > > What I'm worrying is that opening door for _ADR that seems
> > > > > > never used is kinda an overkill here (resolving non-existing problem).
> > > > >
> > > > > I assume that there actually some DSDTs using the _ADR approach
> > > > > and that this support is not there just for fun.
> > > >
> > > > right, it's not for fun, we use _ADR here is to reduce the
> > > > maintain effort because currently it defines _HID for every new
> > > > platform and the drivers have to be updated accordingly, while
> > > > _ADR doesn't have that
> > > problem.
> > >
> > > But this does not confirm if you have such devices. Moreover, My
> > > question about _CID per function stays the same. Why firmware is not using
> it?
> >
> > Yes, both _ADR and _CID can stop growing list in the driver. And for
> > _ADR, it also only require one ID per function. I don't know why BIOS
> > team doesn't select _CID, but I have suggested use _ADR internally,
> > and , to make things moving forward, the driver adds support for _ADR here
> first.
> >
> > But you're right, _CID is another solution as well, we will discuss it
> > with firmware team more.
>
> Should I revert this series now until this gets sorted out?

Current _ADR support is a solution, I don't think _CID is better than _ADR to both
stop growing list in driver and support the shipped hardware at the same time.

Andy, what's your idea?

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

2023-10-16 16:45:32

[permalink] [raw]

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

On Mon, Oct 16, 2023 at 06:44:21PM +0300, Wu, Wentong wrote:
> > From: [email protected]
> > On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
> > > > From: Shevchenko, Andriy
> > > > On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:

...

> > > > But this does not confirm if you have such devices. Moreover, My
> > > > question about _CID per function stays the same. Why firmware is not using
> > it?
> > >
> > > Yes, both _ADR and _CID can stop growing list in the driver. And for
> > > _ADR, it also only require one ID per function. I don't know why BIOS
> > > team doesn't select _CID, but I have suggested use _ADR internally,
> > > and , to make things moving forward, the driver adds support for _ADR here
> > first.
> > >
> > > But you're right, _CID is another solution as well, we will discuss it
> > > with firmware team more.
> >
> > Should I revert this series now until this gets sorted out?
>
> Current _ADR support is a solution, I don't think _CID is better than _ADR to both
> stop growing list in driver and support the shipped hardware at the same time.
>
> Andy, what's your idea?

In my opinion if _CID can be made, it's better than _ADR. As using _ADR like
you do is a bit of grey area in the ACPI specification. I.o.w. can you get
a confirmation, let's say, from Microsoft, that they will go your way for other
similar devices?

Btw, Microsoft has their own solution actually using _ADR for the so called
"wired" USB devices. Is it your case? If so, I'm not sure why _HID has been
used from day 1...

Also I suggest to wait for Hans' opinion on the topic.

--
With Best Regards,
Andy Shevchenko

2023-10-16 17:21:26

[permalink] [raw]

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

Hi,

On 10/16/23 18:05, Shevchenko, Andriy wrote:
> On Mon, Oct 16, 2023 at 06:44:21PM +0300, Wu, Wentong wrote:
>>> From: [email protected]
>>> On Mon, Oct 16, 2023 at 03:05:09PM +0000, Wu, Wentong wrote:
>>>>> From: Shevchenko, Andriy
>>>>> On Mon, Oct 16, 2023 at 08:52:28AM +0300, Wu, Wentong wrote:
>
> ...
>
>>>>> But this does not confirm if you have such devices. Moreover, My
>>>>> question about _CID per function stays the same. Why firmware is not using
>>> it?
>>>>
>>>> Yes, both _ADR and _CID can stop growing list in the driver. And for
>>>> _ADR, it also only require one ID per function. I don't know why BIOS
>>>> team doesn't select _CID, but I have suggested use _ADR internally,
>>>> and , to make things moving forward, the driver adds support for _ADR here
>>> first.
>>>>
>>>> But you're right, _CID is another solution as well, we will discuss it
>>>> with firmware team more.
>>>
>>> Should I revert this series now until this gets sorted out?
>>
>> Current _ADR support is a solution, I don't think _CID is better than _ADR to both
>> stop growing list in driver and support the shipped hardware at the same time.
>>
>> Andy, what's your idea?
>
> In my opinion if _CID can be made, it's better than _ADR. As using _ADR like
> you do is a bit of grey area in the ACPI specification. I.o.w. can you get
> a confirmation, let's say, from Microsoft, that they will go your way for other
> similar devices?
>
> Btw, Microsoft has their own solution actually using _ADR for the so called
> "wired" USB devices. Is it your case? If so, I'm not sure why _HID has been
> used from day 1...
>
> Also I suggest to wait for Hans' opinion on the topic.

I definitely don't think we should revert the entire series since this
supports actual hw which has already been shipping for years.

But if the _ADR support is only there to support future hw and
it is not even certain yet that that future hw is actually going
to be using _ADR support then I believe that a follow-up patch
to drop _ADR support for now is in order. We can then re-introduce
it (revert the follow up patch) if future hw actually starts
using _ADR support.

Specifically what I'm suggesting is something like the following:

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index c9decd0396d4..e1bbaf964786 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -457,8 +457,8 @@ static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
u64 adr, u8 id)
{
struct ljca_match_ids_walk_data wd = { 0 };
- struct acpi_device *parent, *adev;
struct device *dev = adap->dev;
+ struct acpi_device *parent;
char uid[4];

parent = ACPI_COMPANION(dev);
@@ -466,17 +466,7 @@ static void ljca_auxdev_acpi_bind(struct ljca_adapter *adap,
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
+ * Currently LJCA hw does not use _ADR instead current
* platforms use _HID to distinguish children devices.
*/
switch (adr) {

As a follow-up patch to the existing series.

Regards,

Hans

2023-10-17 00:46:44