[v1,1/9] mfd: Add core driver for Nuvoton NCT6694

Commit Message

Ming Yu Oct. 24, 2024, 8:59 a.m. UTC

The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
PWM, and RTC.

This driver implements USB device functionality and shares the
chip's peripherals as a child device.

Each child device can use the USB functions nct6694_read_msg()
and nct6694_write_msg() to issue a command. They can also register
a handler function that will be called when the USB device receives
its interrupt pipe.

Signed-off-by: Ming Yu <[e-mail]>
---
 MAINTAINERS                 |   7 +
 drivers/mfd/Kconfig         |  10 +
 drivers/mfd/Makefile        |   2 +
 drivers/mfd/nct6694.c       | 394 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/nct6694.h | 168 +++++++++++++++
 5 files changed, 581 insertions(+)
 create mode 100644 drivers/mfd/nct6694.c
 create mode 100644 include/linux/mfd/nct6694.h

Comments

Marc Kleine-Budde Oct. 24, 2024, 9:03 a.m. UTC | #1

On 24.10.2024 16:59:14, Ming Yu wrote:
> The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> PWM, and RTC.
> 
> This driver implements USB device functionality and shares the
> chip's peripherals as a child device.
> 
> Each child device can use the USB functions nct6694_read_msg()
> and nct6694_write_msg() to issue a command. They can also register
> a handler function that will be called when the USB device receives
> its interrupt pipe.
> 
> Signed-off-by: Ming Yu <[e-mail]>
> ---
>  MAINTAINERS                 |   7 +
>  drivers/mfd/Kconfig         |  10 +
>  drivers/mfd/Makefile        |   2 +
>  drivers/mfd/nct6694.c       | 394 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/nct6694.h | 168 +++++++++++++++
>  5 files changed, 581 insertions(+)
>  create mode 100644 drivers/mfd/nct6694.c
>  create mode 100644 include/linux/mfd/nct6694.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index e9659a5a7fb3..30157ca95cf3 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16434,6 +16434,13 @@ F:	drivers/nubus/
>  F:	include/linux/nubus.h
>  F:	include/uapi/linux/nubus.h
>  
> +NUVOTON NCT6694 MFD DRIVER
> +M:	Ming Yu <[e-mail]>
> +L:	[e-mail]
> +S:	Supported
> +F:	drivers/mfd/nct6694.c
> +F:	include/linux/mfd/nct6694.h
> +
>  NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
>  M:	Antonino Daplas <[e-mail]>
>  L:	[e-mail]
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index f9325bcce1b9..da2600958697 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -546,6 +546,16 @@ config MFD_MX25_TSADC
>  	  i.MX25 processors. They consist of a conversion queue for general
>  	  purpose ADC and a queue for Touchscreens.
>  
> +config MFD_NCT6694
> +	tristate "Nuvoton NCT6694 support"
> +	select MFD_CORE
> +	depends on USB
> +	help
> +	  This adds support for Nuvoton USB device NCT6694 sharing peripherals
> +	  This includes the USB devcie driver and core APIs.
> +	  Additional drivers must be enabled in order to use the functionality
> +	  of the device.
> +
>  config MFD_HI6421_PMIC
>  	tristate "HiSilicon Hi6421 PMU/Codec IC"
>  	depends on OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 2a9f91e81af8..2cf816d67d03 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -116,6 +116,8 @@ obj-$(CONFIG_TWL6040_CORE)	+= twl6040.o
>  
>  obj-$(CONFIG_MFD_MX25_TSADC)	+= fsl-imx25-tsadc.o
>  
> +obj-$(CONFIG_MFD_NCT6694)	+= nct6694.o
> +
>  obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
>  obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
>  obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> new file mode 100644
> index 000000000000..9838c7be0b98
> --- /dev/null
> +++ b/drivers/mfd/nct6694.c
> @@ -0,0 +1,394 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Nuvoton NCT6694 MFD driver based on USB interface.
> + *
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/io.h>
> +#include <linux/usb.h>
> +#include <linux/slab.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/nct6694.h>
> +
> +#define DRVNAME "nct6694-usb_mfd"
> +
> +#define MFD_DEV_SIMPLE(_name)		\
> +{					\
> +	.name = NCT6694_DEV_##_name,	\
> +}					\
> +
> +#define MFD_DEV_WITH_ID(_name, _id)	\
> +{					\
> +	.name = NCT6694_DEV_##_name,	\
> +	.id = _id,			\
> +}
> +
> +/* MFD device resources */
> +static const struct mfd_cell nct6694_dev[] = {
> +	MFD_DEV_WITH_ID(GPIO, 0x0),
> +	MFD_DEV_WITH_ID(GPIO, 0x1),
> +	MFD_DEV_WITH_ID(GPIO, 0x2),
> +	MFD_DEV_WITH_ID(GPIO, 0x3),
> +	MFD_DEV_WITH_ID(GPIO, 0x4),
> +	MFD_DEV_WITH_ID(GPIO, 0x5),
> +	MFD_DEV_WITH_ID(GPIO, 0x6),
> +	MFD_DEV_WITH_ID(GPIO, 0x7),
> +	MFD_DEV_WITH_ID(GPIO, 0x8),
> +	MFD_DEV_WITH_ID(GPIO, 0x9),
> +	MFD_DEV_WITH_ID(GPIO, 0xA),
> +	MFD_DEV_WITH_ID(GPIO, 0xB),
> +	MFD_DEV_WITH_ID(GPIO, 0xC),
> +	MFD_DEV_WITH_ID(GPIO, 0xD),
> +	MFD_DEV_WITH_ID(GPIO, 0xE),
> +	MFD_DEV_WITH_ID(GPIO, 0xF),
> +
> +	MFD_DEV_WITH_ID(I2C, 0x0),
> +	MFD_DEV_WITH_ID(I2C, 0x1),
> +	MFD_DEV_WITH_ID(I2C, 0x2),
> +	MFD_DEV_WITH_ID(I2C, 0x3),
> +	MFD_DEV_WITH_ID(I2C, 0x4),
> +	MFD_DEV_WITH_ID(I2C, 0x5),
> +
> +	MFD_DEV_WITH_ID(CAN, 0x0),
> +	MFD_DEV_WITH_ID(CAN, 0x1),
> +
> +	MFD_DEV_WITH_ID(WDT, 0x0),
> +	MFD_DEV_WITH_ID(WDT, 0x1),
> +
> +	MFD_DEV_SIMPLE(IIO),
> +	MFD_DEV_SIMPLE(HWMON),
> +	MFD_DEV_SIMPLE(PWM),
> +	MFD_DEV_SIMPLE(RTC),
> +};
> +
> +int nct6694_register_handler(struct nct6694 *nct6694, int irq_bit,
> +			     void (*handler)(void *), void *private_data)
> +{
> +	struct nct6694_handler_entry *entry;
> +	unsigned long flags;
> +
> +	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> +	if (!entry)
> +		return -ENOMEM;
> +
> +	entry->irq_bit = irq_bit;
> +	entry->handler = handler;
> +	entry->private_data = private_data;
> +
> +	spin_lock_irqsave(&nct6694->lock, flags);
> +	list_add_tail(&entry->list, &nct6694->handler_list);
> +	spin_unlock_irqrestore(&nct6694->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(nct6694_register_handler);

Where's the corresponding nct6694_free_handler() function?

Marc

Marc Kleine-Budde Oct. 24, 2024, 9:57 a.m. UTC | #2

On 24.10.2024 16:59:14, Ming Yu wrote:
> +static int nct6694_usb_probe(struct usb_interface *iface,
> +			     const struct usb_device_id *id)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct device *dev = &udev->dev;
> +	struct usb_host_interface *interface;
> +	struct usb_endpoint_descriptor *int_endpoint;
> +	struct nct6694 *nct6694;
> +	int pipe, maxp, bulk_pipe;
> +	int ret = EINVAL;
> +
> +	interface = iface->cur_altsetting;
> +	/* Binding interface class : 0xFF */
> +	if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> +	    interface->desc.bInterfaceSubClass != 0x00 ||
> +	    interface->desc.bInterfaceProtocol != 0x00)
> +		return -ENODEV;

I think you can use USB_DEVICE_INFO() and remove this manual check

https://elixir.bootlin.com/linux/v6.11.5/source/include/linux/usb.h#L1056

[...]

> +
> +static const struct usb_device_id nct6694_ids[] = {
> +	{ USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(usb, nct6694_ids);

regards,
Marc

Marc Kleine-Budde Oct. 24, 2024, 3:20 p.m. UTC | #3

On 24.10.2024 16:59:14, Ming Yu wrote:
> The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> PWM, and RTC.
> 
> This driver implements USB device functionality and shares the
> chip's peripherals as a child device.
> 
> Each child device can use the USB functions nct6694_read_msg()
> and nct6694_write_msg() to issue a command. They can also register
> a handler function that will be called when the USB device receives
> its interrupt pipe.

[...]

> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> new file mode 100644
> index 000000000000..9838c7be0b98
> --- /dev/null
> +++ b/drivers/mfd/nct6694.c

[...]

> +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> +		     u8 rd_idx, u8 rd_len, unsigned char *buf)

why not make buf a void *?

> +{
> +	struct usb_device *udev = nct6694->udev;
> +	unsigned char err_status;
> +	int len, packet_len, tx_len, rx_len;
> +	int i, ret;
> +
> +	mutex_lock(&nct6694->access_lock);
> +
> +	/* Send command packet to USB device */
> +	nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> +	nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> +	nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> +	nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> +	nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> +	nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> +
> +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> +			   nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		goto err;
> +
> +	/* Receive response packet from USB device */
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> +			   nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		goto err;
> +
> +	err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> +
> +	/*
> +	 * Segmented reception of messages that exceed the size of USB bulk
> +	 * pipe packets.
> +	 */

The Linux USB stack can receive bulk messages longer than the max packet size.

> +	for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> +		if (len > nct6694->maxp)
> +			packet_len = nct6694->maxp;
> +		else
> +			packet_len = len;
> +
> +		ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> +				   nct6694->rx_buffer + nct6694->maxp * i,
> +				   packet_len, &rx_len, nct6694->timeout);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	for (i = 0; i < rd_len; i++)
> +		buf[i] = nct6694->rx_buffer[i + rd_idx];

memcpy()?

Or why don't you directly receive data into the provided buffer? Copying
of the data doesn't make it faster.

On the other hand, receiving directly into the target buffer means the
target buffer must not live on the stack.

> +
> +	if (err_status) {
> +		pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
> +		ret = -EIO;
> +	}
> +
> +err:
> +	mutex_unlock(&nct6694->access_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(nct6694_read_msg);
> +
> +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> +		      u16 length, unsigned char *buf)
> +{
> +	struct usb_device *udev = nct6694->udev;
> +	unsigned char err_status;
> +	int len, packet_len, tx_len, rx_len;
> +	int i, ret;
> +
> +	mutex_lock(&nct6694->access_lock);
> +
> +	/* Send command packet to USB device  */
> +	nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> +	nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> +	nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> +	nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_SET;
> +	nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> +	nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;

What about creating a struct that describes the cmd_buffer layout?

> +
> +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> +			   nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		goto err;
> +
> +	/*
> +	 * Segmented transmission of messages that exceed the size of USB bulk
> +	 * pipe packets.
> +	 */

same as above

> +	for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> +		if (len > nct6694->maxp)
> +			packet_len = nct6694->maxp;
> +		else
> +			packet_len = len;
> +
> +		memcpy(nct6694->tx_buffer + nct6694->maxp * i,
> +		       buf + nct6694->maxp * i, packet_len);
> +
> +		ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> +				   nct6694->tx_buffer + nct6694->maxp * i,
> +				   packet_len, &tx_len, nct6694->timeout);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	/* Receive response packet from USB device */
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> +			   nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		goto err;
> +
> +	err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> +
> +	/*
> +	 * Segmented reception of messages that exceed the size of USB bulk
> +	 * pipe packets.
> +	 */

same as above

> +	for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> +		if (len > nct6694->maxp)
> +			packet_len = nct6694->maxp;
> +		else
> +			packet_len = len;
> +
> +		ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> +				   nct6694->rx_buffer + nct6694->maxp * i,
> +				   packet_len, &rx_len, nct6694->timeout);
> +		if (ret)
> +			goto err;
> +	}
> +
> +	memcpy(buf, nct6694->rx_buffer, length);

why not rx into the destination buffer directly?

> +
> +	if (err_status) {
> +		pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
> +		ret = -EIO;
> +	}
> +
> +err:
> +	mutex_unlock(&nct6694->access_lock);
> +	return ret;
> +}
> +EXPORT_SYMBOL(nct6694_write_msg);

[...]

> +static int nct6694_usb_probe(struct usb_interface *iface,
> +			     const struct usb_device_id *id)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct device *dev = &udev->dev;
> +	struct usb_host_interface *interface;
> +	struct usb_endpoint_descriptor *int_endpoint;
> +	struct nct6694 *nct6694;
> +	int pipe, maxp, bulk_pipe;
> +	int ret = EINVAL;
> +
> +	interface = iface->cur_altsetting;
> +	/* Binding interface class : 0xFF */
> +	if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> +	    interface->desc.bInterfaceSubClass != 0x00 ||
> +	    interface->desc.bInterfaceProtocol != 0x00)
> +		return -ENODEV;
> +
> +	int_endpoint = &interface->endpoint[0].desc;
> +	if (!usb_endpoint_is_int_in(int_endpoint))
> +		return -ENODEV;
> +
> +	nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL);
> +	if (!nct6694)
> +		return -ENOMEM;
> +
> +	pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT);
> +	maxp = usb_maxpacket(udev, pipe);

better move these 2 down to the usb_fill_int_urb().

> +
> +	nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> +					   sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->cmd_buffer)
> +		return -ENOMEM;
> +	nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> +					  sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->rx_buffer)
> +		return -ENOMEM;
> +	nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> +					  sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->tx_buffer)
> +		return -ENOMEM;
> +	nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> +					   sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->int_buffer)
> +		return -ENOMEM;
> +
> +	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!nct6694->int_in_urb) {
> +		dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Bulk pipe maximum packet for each transaction */
> +	bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> +	nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> +
> +	mutex_init(&nct6694->access_lock);
> +	nct6694->udev = udev;
> +	nct6694->timeout = URB_TIMEOUT;	/* Wait until urb complete */
> +
> +	INIT_LIST_HEAD(&nct6694->handler_list);
> +	spin_lock_init(&nct6694->lock);
> +
> +	usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> +			 nct6694->int_buffer, maxp, usb_int_callback,
> +			 nct6694, int_endpoint->bInterval);
> +	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> +	if (ret)
> +		goto err_urb;
> +
> +	dev_set_drvdata(&udev->dev, nct6694);
> +	usb_set_intfdata(iface, nct6694);
> +
> +	ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> +				      ARRAY_SIZE(nct6694_dev));
> +	if (ret) {
> +		dev_err(&udev->dev, "Failed to add mfd's child device\n");
> +		goto err_mfd;
> +	}
> +
> +	nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);

Where is the async_workqueue used?

> +
> +	dev_info(&udev->dev, "Probed device: (%04X:%04X)\n",
> +		 id->idVendor, id->idProduct);
> +	return 0;
> +
> +err_mfd:
> +	usb_kill_urb(nct6694->int_in_urb);
> +err_urb:
> +	usb_free_urb(nct6694->int_in_urb);
> +	return ret;
> +}
> +
> +static void nct6694_usb_disconnect(struct usb_interface *iface)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct nct6694 *nct6694 = usb_get_intfdata(iface);
> +
> +	mfd_remove_devices(&udev->dev);
> +	flush_workqueue(nct6694->async_workqueue);
> +	destroy_workqueue(nct6694->async_workqueue);
> +	usb_set_intfdata(iface, NULL);

I think this is not needed.

> +	usb_kill_urb(nct6694->int_in_urb);
> +	usb_free_urb(nct6694->int_in_urb);
> +}
> +
> +static const struct usb_device_id nct6694_ids[] = {
> +	{ USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(usb, nct6694_ids);
> +
> +static struct usb_driver nct6694_usb_driver = {
> +	.name	= DRVNAME,
> +	.id_table = nct6694_ids,
> +	.probe = nct6694_usb_probe,
> +	.disconnect = nct6694_usb_disconnect,
> +};
> +
> +module_usb_driver(nct6694_usb_driver);
> +
> +MODULE_DESCRIPTION("USB-MFD driver for NCT6694");
> +MODULE_AUTHOR("Ming Yu <[e-mail]>");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> new file mode 100644
> index 000000000000..0797564363be
> --- /dev/null
> +++ b/include/linux/mfd/nct6694.h
> @@ -0,0 +1,168 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Nuvoton NCT6694 USB transaction and data structure.
> + *
> + * Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#ifndef __MFD_NCT6694_H
> +#define __MFD_NCT6694_H
> +
> +#define NCT6694_DEV_GPIO		"nct6694-gpio"
> +#define NCT6694_DEV_I2C			"nct6694-i2c"
> +#define NCT6694_DEV_CAN			"nct6694-can"
> +#define NCT6694_DEV_WDT			"nct6694-wdt"
> +#define NCT6694_DEV_IIO			"nct6694-iio"
> +#define NCT6694_DEV_HWMON		"nct6694-hwmon"
> +#define NCT6694_DEV_PWM			"nct6694-pwm"
> +#define NCT6694_DEV_RTC			"nct6694-rtc"
> +
> +#define NCT6694_VENDOR_ID		0x0416
> +#define NCT6694_PRODUCT_ID		0x200B
> +#define INT_IN_ENDPOINT			0x81
> +#define BULK_IN_ENDPOINT		0x82

In Linux we don't add the 0x80 for IN endpoint, the framework does this
for you.

> +#define BULK_OUT_ENDPOINT		0x03
> +#define MAX_PACKET_SZ			0x100
> +
> +#define CMD_PACKET_SZ			0x8
> +#define HCTRL_SET			0x40
> +#define HCTRL_GET			0x80
> +
> +#define REQUEST_MOD_IDX			0x01
> +#define REQUEST_CMD_IDX			0x02
> +#define REQUEST_SEL_IDX			0x03
> +#define REQUEST_HCTRL_IDX		0x04
> +#define REQUEST_LEN_L_IDX		0x06
> +#define REQUEST_LEN_H_IDX		0x07
> +
> +#define RESPONSE_STS_IDX		0x01
> +
> +#define INT_IN_IRQ_IDX			0x00
> +#define GPIO_IRQ_STATUS			BIT(0)
> +#define CAN_IRQ_STATUS			BIT(2)
> +#define RTC_IRQ_STATUS			BIT(3)
> +
> +#define URB_TIMEOUT			1000
> +
> +/*
> + * struct nct6694 - Nuvoton NCT6694 structure
> + *
> + * @udev: Pointer to the USB device
> + * @int_in_urb: Interrupt pipe urb
> + * @access_lock: USB transaction lock
> + * @handler_list: List of registered handlers
> + * @async_workqueue: Workqueue of processing asynchronous work
> + * @tx_buffer: USB write message buffer
> + * @rx_buffer: USB read message buffer
> + * @cmd_buffer: USB send command message buffer
> + * @int_buffer: USB receive interrupt message buffer
> + * @lock: Handlers lock
> + * @timeout: URB timeout
> + * @maxp: Maximum packet of bulk pipe
> + */
> +struct nct6694 {
> +	struct usb_device *udev;
> +	struct urb *int_in_urb;
> +	struct list_head handler_list;
> +	struct workqueue_struct *async_workqueue;
> +
> +	/* Make sure that every USB transaction is not interrupted */
> +	struct mutex access_lock;
> +
> +	unsigned char *tx_buffer;
> +	unsigned char *rx_buffer;
> +	unsigned char *cmd_buffer;
> +	unsigned char *int_buffer;
> +
> +	/* Prevent races within handlers */
> +	spinlock_t lock;
> +
> +	/* time in msec to wait for the urb to the complete */
> +	long timeout;
> +
> +	/* Bulk pipe maximum packet for each transaction */
> +	int maxp;
> +};
> +
> +/*
> + * struct nct6694_handler_entry - Stores the interrupt handling information
> + * for each registered peripheral
> + *
> + * @irq_bit: The bit in irq_status[INT_IN_IRQ_IDX] representing interrupt
                    ^^^

I think this comment could be more precise, you can emphasize, that it's
not the bit number but the bit mask. 

> + * @handler: Function pointer to the interrupt handler of the peripheral
> + * @private_data: Private data specific to the peripheral driver
> + * @list: Node used to link to the handler_list
> + */
> +struct nct6694_handler_entry {
> +	int irq_bit;

the int_status you compare against in the IRQ callback ist a unsigned
char. Better make all a u8.

> +	void (*handler)(void *private_data);
> +	void *private_data;
> +	struct list_head list;
> +};

regards,
Marc

Marc Kleine-Budde Oct. 24, 2024, 3:34 p.m. UTC | #4

On 24.10.2024 17:20:57, Marc Kleine-Budde wrote:

[...]

> > +	nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> > +					   sizeof(unsigned char), GFP_KERNEL);
> > +	if (!nct6694->cmd_buffer)
> > +		return -ENOMEM;
> > +	nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +					  sizeof(unsigned char), GFP_KERNEL);
> > +	if (!nct6694->rx_buffer)
> > +		return -ENOMEM;
> > +	nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +					  sizeof(unsigned char), GFP_KERNEL);
> > +	if (!nct6694->tx_buffer)
> > +		return -ENOMEM;
> > +	nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +					   sizeof(unsigned char), GFP_KERNEL);
> > +	if (!nct6694->int_buffer)
> > +		return -ENOMEM;
> > +
> > +	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > +	if (!nct6694->int_in_urb) {
> > +		dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	/* Bulk pipe maximum packet for each transaction */
> > +	bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> > +	nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> > +
> > +	mutex_init(&nct6694->access_lock);
> > +	nct6694->udev = udev;
> > +	nct6694->timeout = URB_TIMEOUT;	/* Wait until urb complete */
> > +
> > +	INIT_LIST_HEAD(&nct6694->handler_list);
> > +	spin_lock_init(&nct6694->lock);
> > +
> > +	usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> > +			 nct6694->int_buffer, maxp, usb_int_callback,
> > +			 nct6694, int_endpoint->bInterval);
> > +	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> > +	if (ret)
> > +		goto err_urb;
> > +
> > +	dev_set_drvdata(&udev->dev, nct6694);
> > +	usb_set_intfdata(iface, nct6694);
> > +
> > +	ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> > +				      ARRAY_SIZE(nct6694_dev));
> > +	if (ret) {
> > +		dev_err(&udev->dev, "Failed to add mfd's child device\n");
> > +		goto err_mfd;
> > +	}
> > +
> > +	nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
> 
> Where is the async_workqueue used?

Sorry - it's used in the driver, which live in separate directories -
you can ignore this comment.

But then the question comes up, it looks racy to _first_ add the devices
and _then_ the workqueue.

regards,
Marc

Ming Yu Oct. 25, 2024, 8 a.m. UTC | #5

Sorry, resending this email in plain text format.

Dear Marc,

Thank you for your comments.
I'll add the nct6694_free_handler() function in the next patch.

Marc Kleine-Budde <[e-mail]> 於 2024年10月24日 週四 下午5:12寫道：
>
> On 24.10.2024 16:59:14, Ming Yu wrote:
> > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> > PWM, and RTC.
> >
> > This driver implements USB device functionality and shares the
> > chip's peripherals as a child device.
> >
> > Each child device can use the USB functions nct6694_read_msg()
> > and nct6694_write_msg() to issue a command. They can also register
> > a handler function that will be called when the USB device receives
> > its interrupt pipe.
> >
> > Signed-off-by: Ming Yu <[e-mail]>
> > ---
> >  MAINTAINERS                 |   7 +
> >  drivers/mfd/Kconfig         |  10 +
> >  drivers/mfd/Makefile        |   2 +
> >  drivers/mfd/nct6694.c       | 394 ++++++++++++++++++++++++++++++++++++
> >  include/linux/mfd/nct6694.h | 168 +++++++++++++++
> >  5 files changed, 581 insertions(+)
> >  create mode 100644 drivers/mfd/nct6694.c
> >  create mode 100644 include/linux/mfd/nct6694.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index e9659a5a7fb3..30157ca95cf3 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -16434,6 +16434,13 @@ F:   drivers/nubus/
> >  F:   include/linux/nubus.h
> >  F:   include/uapi/linux/nubus.h
> >
> > +NUVOTON NCT6694 MFD DRIVER
> > +M:   Ming Yu <[e-mail]>
> > +L:   [e-mail]
> > +S:   Supported
> > +F:   drivers/mfd/nct6694.c
> > +F:   include/linux/mfd/nct6694.h
> > +
> >  NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
> >  M:   Antonino Daplas <[e-mail]>
> >  L:   [e-mail]
> > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> > index f9325bcce1b9..da2600958697 100644
> > --- a/drivers/mfd/Kconfig
> > +++ b/drivers/mfd/Kconfig
> > @@ -546,6 +546,16 @@ config MFD_MX25_TSADC
> >         i.MX25 processors. They consist of a conversion queue for general
> >         purpose ADC and a queue for Touchscreens.
> >
> > +config MFD_NCT6694
> > +     tristate "Nuvoton NCT6694 support"
> > +     select MFD_CORE
> > +     depends on USB
> > +     help
> > +       This adds support for Nuvoton USB device NCT6694 sharing peripherals
> > +       This includes the USB devcie driver and core APIs.
> > +       Additional drivers must be enabled in order to use the functionality
> > +       of the device.
> > +
> >  config MFD_HI6421_PMIC
> >       tristate "HiSilicon Hi6421 PMU/Codec IC"
> >       depends on OF
> > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> > index 2a9f91e81af8..2cf816d67d03 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -116,6 +116,8 @@ obj-$(CONFIG_TWL6040_CORE)        += twl6040.o
> >
> >  obj-$(CONFIG_MFD_MX25_TSADC) += fsl-imx25-tsadc.o
> >
> > +obj-$(CONFIG_MFD_NCT6694)    += nct6694.o
> > +
> >  obj-$(CONFIG_MFD_MC13XXX)    += mc13xxx-core.o
> >  obj-$(CONFIG_MFD_MC13XXX_SPI)        += mc13xxx-spi.o
> >  obj-$(CONFIG_MFD_MC13XXX_I2C)        += mc13xxx-i2c.o
> > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> > new file mode 100644
> > index 000000000000..9838c7be0b98
> > --- /dev/null
> > +++ b/drivers/mfd/nct6694.c
> > @@ -0,0 +1,394 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nuvoton NCT6694 MFD driver based on USB interface.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
> > +
> > +#include <linux/io.h>
> > +#include <linux/usb.h>
> > +#include <linux/slab.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/mfd/core.h>
> > +#include <linux/mfd/nct6694.h>
> > +
> > +#define DRVNAME "nct6694-usb_mfd"
> > +
> > +#define MFD_DEV_SIMPLE(_name)                \
> > +{                                    \
> > +     .name = NCT6694_DEV_##_name,    \
> > +}                                    \
> > +
> > +#define MFD_DEV_WITH_ID(_name, _id)  \
> > +{                                    \
> > +     .name = NCT6694_DEV_##_name,    \
> > +     .id = _id,                      \
> > +}
> > +
> > +/* MFD device resources */
> > +static const struct mfd_cell nct6694_dev[] = {
> > +     MFD_DEV_WITH_ID(GPIO, 0x0),
> > +     MFD_DEV_WITH_ID(GPIO, 0x1),
> > +     MFD_DEV_WITH_ID(GPIO, 0x2),
> > +     MFD_DEV_WITH_ID(GPIO, 0x3),
> > +     MFD_DEV_WITH_ID(GPIO, 0x4),
> > +     MFD_DEV_WITH_ID(GPIO, 0x5),
> > +     MFD_DEV_WITH_ID(GPIO, 0x6),
> > +     MFD_DEV_WITH_ID(GPIO, 0x7),
> > +     MFD_DEV_WITH_ID(GPIO, 0x8),
> > +     MFD_DEV_WITH_ID(GPIO, 0x9),
> > +     MFD_DEV_WITH_ID(GPIO, 0xA),
> > +     MFD_DEV_WITH_ID(GPIO, 0xB),
> > +     MFD_DEV_WITH_ID(GPIO, 0xC),
> > +     MFD_DEV_WITH_ID(GPIO, 0xD),
> > +     MFD_DEV_WITH_ID(GPIO, 0xE),
> > +     MFD_DEV_WITH_ID(GPIO, 0xF),
> > +
> > +     MFD_DEV_WITH_ID(I2C, 0x0),
> > +     MFD_DEV_WITH_ID(I2C, 0x1),
> > +     MFD_DEV_WITH_ID(I2C, 0x2),
> > +     MFD_DEV_WITH_ID(I2C, 0x3),
> > +     MFD_DEV_WITH_ID(I2C, 0x4),
> > +     MFD_DEV_WITH_ID(I2C, 0x5),
> > +
> > +     MFD_DEV_WITH_ID(CAN, 0x0),
> > +     MFD_DEV_WITH_ID(CAN, 0x1),
> > +
> > +     MFD_DEV_WITH_ID(WDT, 0x0),
> > +     MFD_DEV_WITH_ID(WDT, 0x1),
> > +
> > +     MFD_DEV_SIMPLE(IIO),
> > +     MFD_DEV_SIMPLE(HWMON),
> > +     MFD_DEV_SIMPLE(PWM),
> > +     MFD_DEV_SIMPLE(RTC),
> > +};
> > +
> > +int nct6694_register_handler(struct nct6694 *nct6694, int irq_bit,
> > +                          void (*handler)(void *), void *private_data)
> > +{
> > +     struct nct6694_handler_entry *entry;
> > +     unsigned long flags;
> > +
> > +     entry = kmalloc(sizeof(*entry), GFP_KERNEL);
> > +     if (!entry)
> > +             return -ENOMEM;
> > +
> > +     entry->irq_bit = irq_bit;
> > +     entry->handler = handler;
> > +     entry->private_data = private_data;
> > +
> > +     spin_lock_irqsave(&nct6694->lock, flags);
> > +     list_add_tail(&entry->list, &nct6694->handler_list);
> > +     spin_unlock_irqrestore(&nct6694->lock, flags);
> > +
> > +     return 0;
> > +}
> > +EXPORT_SYMBOL(nct6694_register_handler);
>
> Where's the corresponding nct6694_free_handler() function?
>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Ming Yu Oct. 25, 2024, 8:02 a.m. UTC | #6

Sorry, resending this email in plain text format.

Marc Kleine-Budde <[e-mail]> 於 2024年10月24日 週四 下午5:57寫道：
>
> On 24.10.2024 16:59:14, Ming Yu wrote:
> > +static int nct6694_usb_probe(struct usb_interface *iface,
> > +                          const struct usb_device_id *id)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct device *dev = &udev->dev;
> > +     struct usb_host_interface *interface;
> > +     struct usb_endpoint_descriptor *int_endpoint;
> > +     struct nct6694 *nct6694;
> > +     int pipe, maxp, bulk_pipe;
> > +     int ret = EINVAL;
> > +
> > +     interface = iface->cur_altsetting;
> > +     /* Binding interface class : 0xFF */
> > +     if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> > +         interface->desc.bInterfaceSubClass != 0x00 ||
> > +         interface->desc.bInterfaceProtocol != 0x00)
> > +             return -ENODEV;
>
> I think you can use USB_DEVICE_INFO() and remove this manual check
>
> https://elixir.bootlin.com/linux/v6.11.5/source/include/linux/usb.h#L1056
>
> [...]

[Ming] Okay! I'll remove it and change USB_DEVICE()
to USB_DEVICE_AND_INTERFACE_INFO().

>
> > +
> > +static const struct usb_device_id nct6694_ids[] = {
> > +     { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(usb, nct6694_ids);
>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Ming Yu Oct. 25, 2024, 8:08 a.m. UTC | #7

Sorry, resending this email in plain text format.

Marc Kleine-Budde <[e-mail]> 於 2024年10月24日 週四 下午11:21寫道：
>
> On 24.10.2024 16:59:14, Ming Yu wrote:
> > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> > PWM, and RTC.
> >
> > This driver implements USB device functionality and shares the
> > chip's peripherals as a child device.
> >
> > Each child device can use the USB functions nct6694_read_msg()
> > and nct6694_write_msg() to issue a command. They can also register
> > a handler function that will be called when the USB device receives
> > its interrupt pipe.
>
> [...]
>
> > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> > new file mode 100644
> > index 000000000000..9838c7be0b98
> > --- /dev/null
> > +++ b/drivers/mfd/nct6694.c
>
> [...]
>
> > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
>
> why not make buf a void *?

[Ming] I'll change the type in the next patch.

>
> > +{
> > +     struct usb_device *udev = nct6694->udev;
> > +     unsigned char err_status;
> > +     int len, packet_len, tx_len, rx_len;
> > +     int i, ret;
> > +
> > +     mutex_lock(&nct6694->access_lock);
> > +
> > +     /* Send command packet to USB device */
> > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > +
> > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > +
> > +     /*
> > +      * Segmented reception of messages that exceed the size of USB bulk
> > +      * pipe packets.
> > +      */
>
> The Linux USB stack can receive bulk messages longer than the max packet size.

[Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
The core will divide packet 256 bytes for high speed USB device, but
it is exceeds
the hardware limitation, so I am dividing it manually.

>
> > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > +             if (len > nct6694->maxp)
> > +                     packet_len = nct6694->maxp;
> > +             else
> > +                     packet_len = len;
> > +
> > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > +                                packet_len, &rx_len, nct6694->timeout);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     for (i = 0; i < rd_len; i++)
> > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
>
> memcpy()?
>
> Or why don't you directly receive data into the provided buffer? Copying
> of the data doesn't make it faster.
>
> On the other hand, receiving directly into the target buffer means the
> target buffer must not live on the stack.

[Ming] Okay! I'll change it to memcpy().
This is my perspective: the data is uniformly received by the rx_bffer held
by the MFD device. does it need to be changed?

>
> > +
> > +     if (err_status) {
> > +             pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
> > +             ret = -EIO;
> > +     }
> > +
> > +err:
> > +     mutex_unlock(&nct6694->access_lock);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(nct6694_read_msg);
> > +
> > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> > +                   u16 length, unsigned char *buf)
> > +{
> > +     struct usb_device *udev = nct6694->udev;
> > +     unsigned char err_status;
> > +     int len, packet_len, tx_len, rx_len;
> > +     int i, ret;
> > +
> > +     mutex_lock(&nct6694->access_lock);
> > +
> > +     /* Send command packet to USB device  */
> > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_SET;
> > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
>
> What about creating a struct that describes the cmd_buffer layout?

[Ming] I've thought about this before, thanks for your comments.

>
> > +
> > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     /*
> > +      * Segmented transmission of messages that exceed the size of USB bulk
> > +      * pipe packets.
> > +      */
>
> same as above
>
> > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > +             if (len > nct6694->maxp)
> > +                     packet_len = nct6694->maxp;
> > +             else
> > +                     packet_len = len;
> > +
> > +             memcpy(nct6694->tx_buffer + nct6694->maxp * i,
> > +                    buf + nct6694->maxp * i, packet_len);
> > +
> > +             ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > +                                nct6694->tx_buffer + nct6694->maxp * i,
> > +                                packet_len, &tx_len, nct6694->timeout);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             goto err;
> > +
> > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > +
> > +     /*
> > +      * Segmented reception of messages that exceed the size of USB bulk
> > +      * pipe packets.
> > +      */
>
> same as above
>
> > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > +             if (len > nct6694->maxp)
> > +                     packet_len = nct6694->maxp;
> > +             else
> > +                     packet_len = len;
> > +
> > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > +                                packet_len, &rx_len, nct6694->timeout);
> > +             if (ret)
> > +                     goto err;
> > +     }
> > +
> > +     memcpy(buf, nct6694->rx_buffer, length);
>
> why not rx into the destination buffer directly?
>
> > +
> > +     if (err_status) {
> > +             pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
> > +             ret = -EIO;
> > +     }
> > +
> > +err:
> > +     mutex_unlock(&nct6694->access_lock);
> > +     return ret;
> > +}
> > +EXPORT_SYMBOL(nct6694_write_msg);
>
> [...]
>
> > +static int nct6694_usb_probe(struct usb_interface *iface,
> > +                          const struct usb_device_id *id)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct device *dev = &udev->dev;
> > +     struct usb_host_interface *interface;
> > +     struct usb_endpoint_descriptor *int_endpoint;
> > +     struct nct6694 *nct6694;
> > +     int pipe, maxp, bulk_pipe;
> > +     int ret = EINVAL;
> > +
> > +     interface = iface->cur_altsetting;
> > +     /* Binding interface class : 0xFF */
> > +     if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> > +         interface->desc.bInterfaceSubClass != 0x00 ||
> > +         interface->desc.bInterfaceProtocol != 0x00)
> > +             return -ENODEV;
> > +
> > +     int_endpoint = &interface->endpoint[0].desc;
> > +     if (!usb_endpoint_is_int_in(int_endpoint))
> > +             return -ENODEV;
> > +
> > +     nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL);
> > +     if (!nct6694)
> > +             return -ENOMEM;
> > +
> > +     pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT);
> > +     maxp = usb_maxpacket(udev, pipe);
>
> better move these 2 down to the usb_fill_int_urb().

[Ming] Okay! I'll move these in the next patch.

>
> > +
> > +     nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> > +                                        sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->cmd_buffer)
> > +             return -ENOMEM;
> > +     nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                       sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->rx_buffer)
> > +             return -ENOMEM;
> > +     nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                       sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->tx_buffer)
> > +             return -ENOMEM;
> > +     nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                        sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->int_buffer)
> > +             return -ENOMEM;
> > +
> > +     nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > +     if (!nct6694->int_in_urb) {
> > +             dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     /* Bulk pipe maximum packet for each transaction */
> > +     bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> > +     nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> > +
> > +     mutex_init(&nct6694->access_lock);
> > +     nct6694->udev = udev;
> > +     nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */
> > +
> > +     INIT_LIST_HEAD(&nct6694->handler_list);
> > +     spin_lock_init(&nct6694->lock);
> > +
> > +     usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> > +                      nct6694->int_buffer, maxp, usb_int_callback,
> > +                      nct6694, int_endpoint->bInterval);
> > +     ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> > +     if (ret)
> > +             goto err_urb;
> > +
> > +     dev_set_drvdata(&udev->dev, nct6694);
> > +     usb_set_intfdata(iface, nct6694);
> > +
> > +     ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> > +                                   ARRAY_SIZE(nct6694_dev));
> > +     if (ret) {
> > +             dev_err(&udev->dev, "Failed to add mfd's child device\n");
> > +             goto err_mfd;
> > +     }
> > +
> > +     nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
>
> Where is the async_workqueue used?
>
> > +
> > +     dev_info(&udev->dev, "Probed device: (%04X:%04X)\n",
> > +              id->idVendor, id->idProduct);
> > +     return 0;
> > +
> > +err_mfd:
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +err_urb:
> > +     usb_free_urb(nct6694->int_in_urb);
> > +     return ret;
> > +}
> > +
> > +static void nct6694_usb_disconnect(struct usb_interface *iface)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct nct6694 *nct6694 = usb_get_intfdata(iface);
> > +
> > +     mfd_remove_devices(&udev->dev);
> > +     flush_workqueue(nct6694->async_workqueue);
> > +     destroy_workqueue(nct6694->async_workqueue);
> > +     usb_set_intfdata(iface, NULL);
>
> I think this is not needed.

[Ming] I'll remove it in the next patch.

>
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +     usb_free_urb(nct6694->int_in_urb);
> > +}
> > +
> > +static const struct usb_device_id nct6694_ids[] = {
> > +     { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
> > +     {},
> > +};
> > +MODULE_DEVICE_TABLE(usb, nct6694_ids);
> > +
> > +static struct usb_driver nct6694_usb_driver = {
> > +     .name   = DRVNAME,
> > +     .id_table = nct6694_ids,
> > +     .probe = nct6694_usb_probe,
> > +     .disconnect = nct6694_usb_disconnect,
> > +};
> > +
> > +module_usb_driver(nct6694_usb_driver);
> > +
> > +MODULE_DESCRIPTION("USB-MFD driver for NCT6694");
> > +MODULE_AUTHOR("Ming Yu <[e-mail]>");
> > +MODULE_LICENSE("GPL");
> > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> > new file mode 100644
> > index 000000000000..0797564363be
> > --- /dev/null
> > +++ b/include/linux/mfd/nct6694.h
> > @@ -0,0 +1,168 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +/*
> > + * Nuvoton NCT6694 USB transaction and data structure.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
> > +
> > +#ifndef __MFD_NCT6694_H
> > +#define __MFD_NCT6694_H
> > +
> > +#define NCT6694_DEV_GPIO             "nct6694-gpio"
> > +#define NCT6694_DEV_I2C                      "nct6694-i2c"
> > +#define NCT6694_DEV_CAN                      "nct6694-can"
> > +#define NCT6694_DEV_WDT                      "nct6694-wdt"
> > +#define NCT6694_DEV_IIO                      "nct6694-iio"
> > +#define NCT6694_DEV_HWMON            "nct6694-hwmon"
> > +#define NCT6694_DEV_PWM                      "nct6694-pwm"
> > +#define NCT6694_DEV_RTC                      "nct6694-rtc"
> > +
> > +#define NCT6694_VENDOR_ID            0x0416
> > +#define NCT6694_PRODUCT_ID           0x200B
> > +#define INT_IN_ENDPOINT                      0x81
> > +#define BULK_IN_ENDPOINT             0x82
>
> In Linux we don't add the 0x80 for IN endpoint, the framework does this
> for you.

[Ming] I'll change it in the next patch.

>
> > +#define BULK_OUT_ENDPOINT            0x03
> > +#define MAX_PACKET_SZ                        0x100
> > +
> > +#define CMD_PACKET_SZ                        0x8
> > +#define HCTRL_SET                    0x40
> > +#define HCTRL_GET                    0x80
> > +
> > +#define REQUEST_MOD_IDX                      0x01
> > +#define REQUEST_CMD_IDX                      0x02
> > +#define REQUEST_SEL_IDX                      0x03
> > +#define REQUEST_HCTRL_IDX            0x04
> > +#define REQUEST_LEN_L_IDX            0x06
> > +#define REQUEST_LEN_H_IDX            0x07
> > +
> > +#define RESPONSE_STS_IDX             0x01
> > +
> > +#define INT_IN_IRQ_IDX                       0x00
> > +#define GPIO_IRQ_STATUS                      BIT(0)
> > +#define CAN_IRQ_STATUS                       BIT(2)
> > +#define RTC_IRQ_STATUS                       BIT(3)
> > +
> > +#define URB_TIMEOUT                  1000
> > +
> > +/*
> > + * struct nct6694 - Nuvoton NCT6694 structure
> > + *
> > + * @udev: Pointer to the USB device
> > + * @int_in_urb: Interrupt pipe urb
> > + * @access_lock: USB transaction lock
> > + * @handler_list: List of registered handlers
> > + * @async_workqueue: Workqueue of processing asynchronous work
> > + * @tx_buffer: USB write message buffer
> > + * @rx_buffer: USB read message buffer
> > + * @cmd_buffer: USB send command message buffer
> > + * @int_buffer: USB receive interrupt message buffer
> > + * @lock: Handlers lock
> > + * @timeout: URB timeout
> > + * @maxp: Maximum packet of bulk pipe
> > + */
> > +struct nct6694 {
> > +     struct usb_device *udev;
> > +     struct urb *int_in_urb;
> > +     struct list_head handler_list;
> > +     struct workqueue_struct *async_workqueue;
> > +
> > +     /* Make sure that every USB transaction is not interrupted */
> > +     struct mutex access_lock;
> > +
> > +     unsigned char *tx_buffer;
> > +     unsigned char *rx_buffer;
> > +     unsigned char *cmd_buffer;
> > +     unsigned char *int_buffer;
> > +
> > +     /* Prevent races within handlers */
> > +     spinlock_t lock;
> > +
> > +     /* time in msec to wait for the urb to the complete */
> > +     long timeout;
> > +
> > +     /* Bulk pipe maximum packet for each transaction */
> > +     int maxp;
> > +};
> > +
> > +/*
> > + * struct nct6694_handler_entry - Stores the interrupt handling information
> > + * for each registered peripheral
> > + *
> > + * @irq_bit: The bit in irq_status[INT_IN_IRQ_IDX] representing interrupt
>                     ^^^
>
> I think this comment could be more precise, you can emphasize, that it's
> not the bit number but the bit mask.

[Ming] Okay! I'll change it in the next patch.

>
> > + * @handler: Function pointer to the interrupt handler of the peripheral
> > + * @private_data: Private data specific to the peripheral driver
> > + * @list: Node used to link to the handler_list
> > + */
> > +struct nct6694_handler_entry {
> > +     int irq_bit;
>
> the int_status you compare against in the IRQ callback ist a unsigned
> char. Better make all a u8.

[Ming] Okay! I'll change it in the next patch.

>
> > +     void (*handler)(void *private_data);
> > +     void *private_data;
> > +     struct list_head list;
> > +};
>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Ming Yu Oct. 25, 2024, 8:14 a.m. UTC | #8

Dear Marc,

Excuse me, I'm a bit confused. Is there anything I need to
improve on?

Thanks,
Ming

Marc Kleine-Budde <[e-mail]> 於 2024年10月24日 週四 下午11:34寫道：
>
> On 24.10.2024 17:20:57, Marc Kleine-Budde wrote:
>
> [...]
>
> > > +   nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> > > +                                      sizeof(unsigned char), GFP_KERNEL);
> > > +   if (!nct6694->cmd_buffer)
> > > +           return -ENOMEM;
> > > +   nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > > +                                     sizeof(unsigned char), GFP_KERNEL);
> > > +   if (!nct6694->rx_buffer)
> > > +           return -ENOMEM;
> > > +   nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > > +                                     sizeof(unsigned char), GFP_KERNEL);
> > > +   if (!nct6694->tx_buffer)
> > > +           return -ENOMEM;
> > > +   nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > > +                                      sizeof(unsigned char), GFP_KERNEL);
> > > +   if (!nct6694->int_buffer)
> > > +           return -ENOMEM;
> > > +
> > > +   nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > > +   if (!nct6694->int_in_urb) {
> > > +           dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> > > +           return -ENOMEM;
> > > +   }
> > > +
> > > +   /* Bulk pipe maximum packet for each transaction */
> > > +   bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> > > +   nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> > > +
> > > +   mutex_init(&nct6694->access_lock);
> > > +   nct6694->udev = udev;
> > > +   nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */
> > > +
> > > +   INIT_LIST_HEAD(&nct6694->handler_list);
> > > +   spin_lock_init(&nct6694->lock);
> > > +
> > > +   usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> > > +                    nct6694->int_buffer, maxp, usb_int_callback,
> > > +                    nct6694, int_endpoint->bInterval);
> > > +   ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> > > +   if (ret)
> > > +           goto err_urb;
> > > +
> > > +   dev_set_drvdata(&udev->dev, nct6694);
> > > +   usb_set_intfdata(iface, nct6694);
> > > +
> > > +   ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> > > +                                 ARRAY_SIZE(nct6694_dev));
> > > +   if (ret) {
> > > +           dev_err(&udev->dev, "Failed to add mfd's child device\n");
> > > +           goto err_mfd;
> > > +   }
> > > +
> > > +   nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
> >
> > Where is the async_workqueue used?
>
> Sorry - it's used in the driver, which live in separate directories -
> you can ignore this comment.
>
> But then the question comes up, it looks racy to _first_ add the devices
> and _then_ the workqueue.
>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Marc Kleine-Budde Oct. 25, 2024, 8:35 a.m. UTC | #9

On 25.10.2024 16:14:03, Ming Yu wrote:
> Marc Kleine-Budde <[e-mail]> 於 2024年10月24日 週四 下午11:34寫道：
> >
> > On 24.10.2024 17:20:57, Marc Kleine-Budde wrote:
> >
> > [...]
> >
> > > > +   nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> > > > +                                      sizeof(unsigned char), GFP_KERNEL);
> > > > +   if (!nct6694->cmd_buffer)
> > > > +           return -ENOMEM;
> > > > +   nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > > > +                                     sizeof(unsigned char), GFP_KERNEL);
> > > > +   if (!nct6694->rx_buffer)
> > > > +           return -ENOMEM;
> > > > +   nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > > > +                                     sizeof(unsigned char), GFP_KERNEL);
> > > > +   if (!nct6694->tx_buffer)
> > > > +           return -ENOMEM;
> > > > +   nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > > > +                                      sizeof(unsigned char), GFP_KERNEL);
> > > > +   if (!nct6694->int_buffer)
> > > > +           return -ENOMEM;
> > > > +
> > > > +   nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > > > +   if (!nct6694->int_in_urb) {
> > > > +           dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> > > > +           return -ENOMEM;
> > > > +   }
> > > > +
> > > > +   /* Bulk pipe maximum packet for each transaction */
> > > > +   bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> > > > +   nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> > > > +
> > > > +   mutex_init(&nct6694->access_lock);
> > > > +   nct6694->udev = udev;
> > > > +   nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */
> > > > +
> > > > +   INIT_LIST_HEAD(&nct6694->handler_list);
> > > > +   spin_lock_init(&nct6694->lock);
> > > > +
> > > > +   usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> > > > +                    nct6694->int_buffer, maxp, usb_int_callback,
> > > > +                    nct6694, int_endpoint->bInterval);
> > > > +   ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> > > > +   if (ret)
> > > > +           goto err_urb;
> > > > +
> > > > +   dev_set_drvdata(&udev->dev, nct6694);
> > > > +   usb_set_intfdata(iface, nct6694);
> > > > +
> > > > +   ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> > > > +                                 ARRAY_SIZE(nct6694_dev));
> > > > +   if (ret) {
> > > > +           dev_err(&udev->dev, "Failed to add mfd's child device\n");
> > > > +           goto err_mfd;
> > > > +   }
> > > > +
> > > > +   nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
> > >
> > > Where is the async_workqueue used?
> >
> > Sorry - it's used in the driver, which live in separate directories -
> > you can ignore this comment.
> >
> > But then the question comes up, it looks racy to _first_ add the devices
> > and _then_ the workqueue.

> Excuse me, I'm a bit confused. Is there anything I need to
> improve on?

It looks racy to _first_ add the devices and _then_ the workqueue.

So the obvious solution is to allocate the worklist first and then add
the devices.

regards,
Marc

Marc Kleine-Budde Oct. 25, 2024, 9:02 a.m. UTC | #10

On 25.10.2024 10:35:35, Marc Kleine-Budde wrote:
> > Excuse me, I'm a bit confused. Is there anything I need to
> > improve on?
> 
> It looks racy to _first_ add the devices and _then_ the workqueue.
> 
> So the obvious solution is to allocate the worklist first and then add
                                             workqueue
> the devices.

Marc

Marc Kleine-Budde Oct. 25, 2024, 10:08 a.m. UTC | #11

On 25.10.2024 16:08:10, Ming Yu wrote:
> > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> >
> > why not make buf a void *?
> 
> [Ming] I'll change the type in the next patch.
> 
> >
> > > +{
> > > +     struct usb_device *udev = nct6694->udev;
> > > +     unsigned char err_status;
> > > +     int len, packet_len, tx_len, rx_len;
> > > +     int i, ret;
> > > +
> > > +     mutex_lock(&nct6694->access_lock);
> > > +
> > > +     /* Send command packet to USB device */
> > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > +
> > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > +                        nct6694->timeout);
> > > +     if (ret)
> > > +             goto err;
> > > +
> > > +     /* Receive response packet from USB device */
> > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > +                        nct6694->timeout);
> > > +     if (ret)
> > > +             goto err;
> > > +
> > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > +
> > > +     /*
> > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > +      * pipe packets.
> > > +      */
> >
> > The Linux USB stack can receive bulk messages longer than the max packet size.
> 
> [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> The core will divide packet 256 bytes for high speed USB device, but
> it is exceeds
> the hardware limitation, so I am dividing it manually.

You say the endpoint descriptor is correctly reporting it's max packet
size of 128, but the Linux USB will send packets of 256 bytes?

> 
> >
> > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > +             if (len > nct6694->maxp)
> > > +                     packet_len = nct6694->maxp;
> > > +             else
> > > +                     packet_len = len;
> > > +
> > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > +                                packet_len, &rx_len, nct6694->timeout);
> > > +             if (ret)
> > > +                     goto err;
> > > +     }
> > > +
> > > +     for (i = 0; i < rd_len; i++)
> > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> >
> > memcpy()?
> >
> > Or why don't you directly receive data into the provided buffer? Copying
> > of the data doesn't make it faster.
> >
> > On the other hand, receiving directly into the target buffer means the
> > target buffer must not live on the stack.
> 
> [Ming] Okay! I'll change it to memcpy().

fine!

> This is my perspective: the data is uniformly received by the rx_bffer held
> by the MFD device. does it need to be changed?

My question is: Why do you first receive into the nct6694->rx_buffer and
then memcpy() to the buffer provided by the caller, why don't you
directly receive into the memory provided by the caller?

Marc

Ming Yu Oct. 25, 2024, 10:22 a.m. UTC | #12

Got it! I'll make the change in the next patch.

Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午5:02寫道：
>
> On 25.10.2024 10:35:35, Marc Kleine-Budde wrote:
> > > Excuse me, I'm a bit confused. Is there anything I need to
> > > improve on?
> >
> > It looks racy to _first_ add the devices and _then_ the workqueue.
> >
> > So the obvious solution is to allocate the worklist first and then add
>                                              workqueue
> > the devices.
>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Ming Yu Oct. 25, 2024, 11:03 a.m. UTC | #13

Oh! I'm sorry about that I confused the packet size.
The NCT6694 bulk maximum packet size is 256 bytes,
and USB High speed bulk maximum packet size is 512 bytes.

Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午6:08寫道：
>
> On 25.10.2024 16:08:10, Ming Yu wrote:
> > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> > >
> > > why not make buf a void *?
> >
> > [Ming] I'll change the type in the next patch.
> >
> > >
> > > > +{
> > > > +     struct usb_device *udev = nct6694->udev;
> > > > +     unsigned char err_status;
> > > > +     int len, packet_len, tx_len, rx_len;
> > > > +     int i, ret;
> > > > +
> > > > +     mutex_lock(&nct6694->access_lock);
> > > > +
> > > > +     /* Send command packet to USB device */
> > > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > > +
> > > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > > +                        nct6694->timeout);
> > > > +     if (ret)
> > > > +             goto err;
> > > > +
> > > > +     /* Receive response packet from USB device */
> > > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > > +                        nct6694->timeout);
> > > > +     if (ret)
> > > > +             goto err;
> > > > +
> > > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > > +
> > > > +     /*
> > > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > > +      * pipe packets.
> > > > +      */
> > >
> > > The Linux USB stack can receive bulk messages longer than the max packet size.
> >
> > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > The core will divide packet 256 bytes for high speed USB device, but
> > it is exceeds
> > the hardware limitation, so I am dividing it manually.
>
> You say the endpoint descriptor is correctly reporting it's max packet
> size of 128, but the Linux USB will send packets of 256 bytes?

[Ming] The endpoint descriptor is correctly reporting it's max packet
size of 256, but the Linux USB may send more than 256 (max is 512)
https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446

>
> >
> > >
> > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > +             if (len > nct6694->maxp)
> > > > +                     packet_len = nct6694->maxp;
> > > > +             else
> > > > +                     packet_len = len;
> > > > +
> > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > +             if (ret)
> > > > +                     goto err;
> > > > +     }
> > > > +
> > > > +     for (i = 0; i < rd_len; i++)
> > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > >
> > > memcpy()?
> > >
> > > Or why don't you directly receive data into the provided buffer? Copying
> > > of the data doesn't make it faster.
> > >
> > > On the other hand, receiving directly into the target buffer means the
> > > target buffer must not live on the stack.
> >
> > [Ming] Okay! I'll change it to memcpy().
>
> fine!
>
> > This is my perspective: the data is uniformly received by the rx_bffer held
> > by the MFD device. does it need to be changed?
>
> My question is: Why do you first receive into the nct6694->rx_buffer and
> then memcpy() to the buffer provided by the caller, why don't you
> directly receive into the memory provided by the caller?

[Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
using the MFD'd dynamically allocated buffer to submit URBs will better
manage USB-related operations

>
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Marc Kleine-Budde Oct. 25, 2024, 12:23 p.m. UTC | #14

On 25.10.2024 19:03:55, Ming Yu wrote:
> Oh! I'm sorry about that I confused the packet size.
> The NCT6694 bulk maximum packet size is 256 bytes,
> and USB High speed bulk maximum packet size is 512 bytes.
> 
> Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午6:08寫道：
> >
> > On 25.10.2024 16:08:10, Ming Yu wrote:
> > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> > > >
> > > > why not make buf a void *?
> > >
> > > [Ming] I'll change the type in the next patch.
> > >
> > > >
> > > > > +{
> > > > > +     struct usb_device *udev = nct6694->udev;
> > > > > +     unsigned char err_status;
> > > > > +     int len, packet_len, tx_len, rx_len;
> > > > > +     int i, ret;
> > > > > +
> > > > > +     mutex_lock(&nct6694->access_lock);
> > > > > +
> > > > > +     /* Send command packet to USB device */
> > > > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > > > +
> > > > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > > > +                        nct6694->timeout);
> > > > > +     if (ret)
> > > > > +             goto err;
> > > > > +
> > > > > +     /* Receive response packet from USB device */
> > > > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > > > +                        nct6694->timeout);
> > > > > +     if (ret)
> > > > > +             goto err;
> > > > > +
> > > > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > > > +
> > > > > +     /*
> > > > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > > > +      * pipe packets.
> > > > > +      */
> > > >
> > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > >
> > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > The core will divide packet 256 bytes for high speed USB device, but
> > > it is exceeds
> > > the hardware limitation, so I am dividing it manually.
> >
> > You say the endpoint descriptor is correctly reporting it's max packet
> > size of 128, but the Linux USB will send packets of 256 bytes?
> 
> [Ming] The endpoint descriptor is correctly reporting it's max packet
> size of 256, but the Linux USB may send more than 256 (max is 512)
> https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446

AFAIK according to the USB-2.0 spec the maximum packet size for
high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
your device is a non-compliant USB device?

> > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > +             if (len > nct6694->maxp)
> > > > > +                     packet_len = nct6694->maxp;
> > > > > +             else
> > > > > +                     packet_len = len;
> > > > > +
> > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > +             if (ret)
> > > > > +                     goto err;
> > > > > +     }
> > > > > +
> > > > > +     for (i = 0; i < rd_len; i++)
> > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > >
> > > > memcpy()?
> > > >
> > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > of the data doesn't make it faster.
> > > >
> > > > On the other hand, receiving directly into the target buffer means the
> > > > target buffer must not live on the stack.
> > >
> > > [Ming] Okay! I'll change it to memcpy().
> >
> > fine!
> >
> > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > by the MFD device. does it need to be changed?
> >
> > My question is: Why do you first receive into the nct6694->rx_buffer and
> > then memcpy() to the buffer provided by the caller, why don't you
> > directly receive into the memory provided by the caller?
> 
> [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> using the MFD'd dynamically allocated buffer to submit URBs will better
> manage USB-related operations

The non-compliant max packet size limitation is unrelated to the
question which RX or TX buffer to use.

regards,
Marc

Christophe JAILLET Oct. 26, 2024, 2:58 p.m. UTC | #15

Le 24/10/2024 à 10:59, Ming Yu a écrit :
> The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> PWM, and RTC.
> 
> This driver implements USB device functionality and shares the
> chip's peripherals as a child device.
> 
> Each child device can use the USB functions nct6694_read_msg()
> and nct6694_write_msg() to issue a command. They can also register
> a handler function that will be called when the USB device receives
> its interrupt pipe.
> 
> Signed-off-by: Ming Yu <[e-mail]>
> ---

...

> +static int nct6694_usb_probe(struct usb_interface *iface,
> +			     const struct usb_device_id *id)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct device *dev = &udev->dev;
> +	struct usb_host_interface *interface;
> +	struct usb_endpoint_descriptor *int_endpoint;
> +	struct nct6694 *nct6694;
> +	int pipe, maxp, bulk_pipe;
> +	int ret = EINVAL;

Nitpick: no need to init

> +
> +	interface = iface->cur_altsetting;
> +	/* Binding interface class : 0xFF */
> +	if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> +	    interface->desc.bInterfaceSubClass != 0x00 ||
> +	    interface->desc.bInterfaceProtocol != 0x00)
> +		return -ENODEV;
> +
> +	int_endpoint = &interface->endpoint[0].desc;
> +	if (!usb_endpoint_is_int_in(int_endpoint))
> +		return -ENODEV;
> +
> +	nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL);
> +	if (!nct6694)
> +		return -ENOMEM;
> +
> +	pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT);
> +	maxp = usb_maxpacket(udev, pipe);
> +
> +	nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> +					   sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->cmd_buffer)
> +		return -ENOMEM;
> +	nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> +					  sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->rx_buffer)
> +		return -ENOMEM;
> +	nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> +					  sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->tx_buffer)
> +		return -ENOMEM;
> +	nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> +					   sizeof(unsigned char), GFP_KERNEL);
> +	if (!nct6694->int_buffer)
> +		return -ENOMEM;
> +
> +	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!nct6694->int_in_urb) {
> +		dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> +		return -ENOMEM;
> +	}
> +
> +	/* Bulk pipe maximum packet for each transaction */
> +	bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> +	nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> +
> +	mutex_init(&nct6694->access_lock);
> +	nct6694->udev = udev;
> +	nct6694->timeout = URB_TIMEOUT;	/* Wait until urb complete */
> +
> +	INIT_LIST_HEAD(&nct6694->handler_list);
> +	spin_lock_init(&nct6694->lock);
> +
> +	usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> +			 nct6694->int_buffer, maxp, usb_int_callback,
> +			 nct6694, int_endpoint->bInterval);
> +	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> +	if (ret)
> +		goto err_urb;
> +
> +	dev_set_drvdata(&udev->dev, nct6694);
> +	usb_set_intfdata(iface, nct6694);
> +
> +	ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> +				      ARRAY_SIZE(nct6694_dev));
> +	if (ret) {
> +		dev_err(&udev->dev, "Failed to add mfd's child device\n");
> +		goto err_mfd;
> +	}
> +
> +	nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);

Missing error handling.

> +
> +	dev_info(&udev->dev, "Probed device: (%04X:%04X)\n",
> +		 id->idVendor, id->idProduct);
> +	return 0;
> +
> +err_mfd:
> +	usb_kill_urb(nct6694->int_in_urb);
> +err_urb:
> +	usb_free_urb(nct6694->int_in_urb);
> +	return ret;
> +}
> +
> +static void nct6694_usb_disconnect(struct usb_interface *iface)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct nct6694 *nct6694 = usb_get_intfdata(iface);
> +
> +	mfd_remove_devices(&udev->dev);
> +	flush_workqueue(nct6694->async_workqueue);
> +	destroy_workqueue(nct6694->async_workqueue);
> +	usb_set_intfdata(iface, NULL);
> +	usb_kill_urb(nct6694->int_in_urb);
> +	usb_free_urb(nct6694->int_in_urb);
> +}
> +
> +static const struct usb_device_id nct6694_ids[] = {
> +	{ USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},

Nitpick: space missing before the ending }

> +	{},

Nitpick: usually, no comma is added after a {} terminator.

> +};
> +MODULE_DEVICE_TABLE(usb, nct6694_ids);

...

CJ

Ming Yu Oct. 28, 2024, 7:33 a.m. UTC | #16

Dear Marc,

Thank you for your comments,

Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午8:24寫道：
>
> On 25.10.2024 19:03:55, Ming Yu wrote:
> > Oh! I'm sorry about that I confused the packet size.
> > The NCT6694 bulk maximum packet size is 256 bytes,
> > and USB High speed bulk maximum packet size is 512 bytes.
> >
> > Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午6:08寫道：
> > >
> > > On 25.10.2024 16:08:10, Ming Yu wrote:
> > > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > > > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> > > > >
> > > > > why not make buf a void *?
> > > >
> > > > [Ming] I'll change the type in the next patch.
> > > >
> > > > >
> > > > > > +{
> > > > > > +     struct usb_device *udev = nct6694->udev;
> > > > > > +     unsigned char err_status;
> > > > > > +     int len, packet_len, tx_len, rx_len;
> > > > > > +     int i, ret;
> > > > > > +
> > > > > > +     mutex_lock(&nct6694->access_lock);
> > > > > > +
> > > > > > +     /* Send command packet to USB device */
> > > > > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > > > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > > > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > > > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > > > > +
> > > > > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > > > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > > > > +                        nct6694->timeout);
> > > > > > +     if (ret)
> > > > > > +             goto err;
> > > > > > +
> > > > > > +     /* Receive response packet from USB device */
> > > > > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > > > > +                        nct6694->timeout);
> > > > > > +     if (ret)
> > > > > > +             goto err;
> > > > > > +
> > > > > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > > > > +
> > > > > > +     /*
> > > > > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > > > > +      * pipe packets.
> > > > > > +      */
> > > > >
> > > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > > >
> > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > > The core will divide packet 256 bytes for high speed USB device, but
> > > > it is exceeds
> > > > the hardware limitation, so I am dividing it manually.
> > >
> > > You say the endpoint descriptor is correctly reporting it's max packet
> > > size of 128, but the Linux USB will send packets of 256 bytes?
> >
> > [Ming] The endpoint descriptor is correctly reporting it's max packet
> > size of 256, but the Linux USB may send more than 256 (max is 512)
> > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446
>
> AFAIK according to the USB-2.0 spec the maximum packet size for
> high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
> your device is a non-compliant USB device?

We will reduce the endpoint size of other interfaces to ensure that MFD device
meets the USB2.0 spec. In other words, I will remove the code for manual
unpacking in the next patch.

>
> > > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > > +             if (len > nct6694->maxp)
> > > > > > +                     packet_len = nct6694->maxp;
> > > > > > +             else
> > > > > > +                     packet_len = len;
> > > > > > +
> > > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > > +             if (ret)
> > > > > > +                     goto err;
> > > > > > +     }
> > > > > > +
> > > > > > +     for (i = 0; i < rd_len; i++)
> > > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > > >
> > > > > memcpy()?
> > > > >
> > > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > > of the data doesn't make it faster.
> > > > >
> > > > > On the other hand, receiving directly into the target buffer means the
> > > > > target buffer must not live on the stack.
> > > >
> > > > [Ming] Okay! I'll change it to memcpy().
> > >
> > > fine!
> > >
> > > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > > by the MFD device. does it need to be changed?
> > >
> > > My question is: Why do you first receive into the nct6694->rx_buffer and
> > > then memcpy() to the buffer provided by the caller, why don't you
> > > directly receive into the memory provided by the caller?
> >
> > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> > using the MFD'd dynamically allocated buffer to submit URBs will better
> > manage USB-related operations
>
> The non-compliant max packet size limitation is unrelated to the
> question which RX or TX buffer to use.

I think these two USB functions can be easily called using the buffer
dynamically
allocated by the MFD. However, if they transfer data directly to the
target buffer,
they must ensure that it is not located on the stack.

>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Ming

Ming Yu Oct. 28, 2024, 7:37 a.m. UTC | #17

Hi Christophe

Thank you for your comments,
I will update the code in the next patch.

Best regards,
Ming

Christophe JAILLET <[e-mail]> 於 2024年10月26日 週六 下午10:58寫道：
>
> Le 24/10/2024 à 10:59, Ming Yu a écrit :
> > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips,
> > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC,
> > PWM, and RTC.
> >
> > This driver implements USB device functionality and shares the
> > chip's peripherals as a child device.
> >
> > Each child device can use the USB functions nct6694_read_msg()
> > and nct6694_write_msg() to issue a command. They can also register
> > a handler function that will be called when the USB device receives
> > its interrupt pipe.
> >
> > Signed-off-by: Ming Yu <[e-mail]>
> > ---
>
> ...
>
> > +static int nct6694_usb_probe(struct usb_interface *iface,
> > +                          const struct usb_device_id *id)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct device *dev = &udev->dev;
> > +     struct usb_host_interface *interface;
> > +     struct usb_endpoint_descriptor *int_endpoint;
> > +     struct nct6694 *nct6694;
> > +     int pipe, maxp, bulk_pipe;
> > +     int ret = EINVAL;
>
> Nitpick: no need to init
>
> > +
> > +     interface = iface->cur_altsetting;
> > +     /* Binding interface class : 0xFF */
> > +     if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
> > +         interface->desc.bInterfaceSubClass != 0x00 ||
> > +         interface->desc.bInterfaceProtocol != 0x00)
> > +             return -ENODEV;
> > +
> > +     int_endpoint = &interface->endpoint[0].desc;
> > +     if (!usb_endpoint_is_int_in(int_endpoint))
> > +             return -ENODEV;
> > +
> > +     nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL);
> > +     if (!nct6694)
> > +             return -ENOMEM;
> > +
> > +     pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT);
> > +     maxp = usb_maxpacket(udev, pipe);
> > +
> > +     nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
> > +                                        sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->cmd_buffer)
> > +             return -ENOMEM;
> > +     nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                       sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->rx_buffer)
> > +             return -ENOMEM;
> > +     nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                       sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->tx_buffer)
> > +             return -ENOMEM;
> > +     nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
> > +                                        sizeof(unsigned char), GFP_KERNEL);
> > +     if (!nct6694->int_buffer)
> > +             return -ENOMEM;
> > +
> > +     nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> > +     if (!nct6694->int_in_urb) {
> > +             dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
> > +             return -ENOMEM;
> > +     }
> > +
> > +     /* Bulk pipe maximum packet for each transaction */
> > +     bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
> > +     nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
> > +
> > +     mutex_init(&nct6694->access_lock);
> > +     nct6694->udev = udev;
> > +     nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */
> > +
> > +     INIT_LIST_HEAD(&nct6694->handler_list);
> > +     spin_lock_init(&nct6694->lock);
> > +
> > +     usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
> > +                      nct6694->int_buffer, maxp, usb_int_callback,
> > +                      nct6694, int_endpoint->bInterval);
> > +     ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> > +     if (ret)
> > +             goto err_urb;
> > +
> > +     dev_set_drvdata(&udev->dev, nct6694);
> > +     usb_set_intfdata(iface, nct6694);
> > +
> > +     ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
> > +                                   ARRAY_SIZE(nct6694_dev));
> > +     if (ret) {
> > +             dev_err(&udev->dev, "Failed to add mfd's child device\n");
> > +             goto err_mfd;
> > +     }
> > +
> > +     nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
>
> Missing error handling.
>
> > +
> > +     dev_info(&udev->dev, "Probed device: (%04X:%04X)\n",
> > +              id->idVendor, id->idProduct);
> > +     return 0;
> > +
> > +err_mfd:
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +err_urb:
> > +     usb_free_urb(nct6694->int_in_urb);
> > +     return ret;
> > +}
> > +
> > +static void nct6694_usb_disconnect(struct usb_interface *iface)
> > +{
> > +     struct usb_device *udev = interface_to_usbdev(iface);
> > +     struct nct6694 *nct6694 = usb_get_intfdata(iface);
> > +
> > +     mfd_remove_devices(&udev->dev);
> > +     flush_workqueue(nct6694->async_workqueue);
> > +     destroy_workqueue(nct6694->async_workqueue);
> > +     usb_set_intfdata(iface, NULL);
> > +     usb_kill_urb(nct6694->int_in_urb);
> > +     usb_free_urb(nct6694->int_in_urb);
> > +}
> > +
> > +static const struct usb_device_id nct6694_ids[] = {
> > +     { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
>
> Nitpick: space missing before the ending }
>
> > +     {},
>
> Nitpick: usually, no comma is added after a {} terminator.
>
> > +};
> > +MODULE_DEVICE_TABLE(usb, nct6694_ids);
>
> ...
>
> CJ

Marc Kleine-Budde Oct. 28, 2024, 7:52 a.m. UTC | #18

On 28.10.2024 15:33:08, Ming Yu wrote:
> Dear Marc,
> 
> Thank you for your comments,
> 
> Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午8:24寫道：
> >
> > On 25.10.2024 19:03:55, Ming Yu wrote:
> > > Oh! I'm sorry about that I confused the packet size.
> > > The NCT6694 bulk maximum packet size is 256 bytes,
> > > and USB High speed bulk maximum packet size is 512 bytes.
> > >
> > > Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午6:08寫道：
> > > >
> > > > On 25.10.2024 16:08:10, Ming Yu wrote:
> > > > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > > > > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> > > > > >
> > > > > > why not make buf a void *?
> > > > >
> > > > > [Ming] I'll change the type in the next patch.
> > > > >
> > > > > >
> > > > > > > +{
> > > > > > > +     struct usb_device *udev = nct6694->udev;
> > > > > > > +     unsigned char err_status;
> > > > > > > +     int len, packet_len, tx_len, rx_len;
> > > > > > > +     int i, ret;
> > > > > > > +
> > > > > > > +     mutex_lock(&nct6694->access_lock);
> > > > > > > +
> > > > > > > +     /* Send command packet to USB device */
> > > > > > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > > > > > +
> > > > > > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > > > > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > > > > > +                        nct6694->timeout);
> > > > > > > +     if (ret)
> > > > > > > +             goto err;
> > > > > > > +
> > > > > > > +     /* Receive response packet from USB device */
> > > > > > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > > > > > +                        nct6694->timeout);
> > > > > > > +     if (ret)
> > > > > > > +             goto err;
> > > > > > > +
> > > > > > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > > > > > +
> > > > > > > +     /*
> > > > > > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > > > > > +      * pipe packets.
> > > > > > > +      */
> > > > > >
> > > > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > > > >
> > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > > > The core will divide packet 256 bytes for high speed USB device, but
> > > > > it is exceeds
> > > > > the hardware limitation, so I am dividing it manually.
> > > >
> > > > You say the endpoint descriptor is correctly reporting it's max packet
> > > > size of 128, but the Linux USB will send packets of 256 bytes?
> > >
> > > [Ming] The endpoint descriptor is correctly reporting it's max packet
> > > size of 256, but the Linux USB may send more than 256 (max is 512)
> > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446
> >
> > AFAIK according to the USB-2.0 spec the maximum packet size for
> > high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
> > your device is a non-compliant USB device?
> 
> We will reduce the endpoint size of other interfaces to ensure that MFD device
> meets the USB2.0 spec. In other words, I will remove the code for manual
> unpacking in the next patch.

I was not talking about the driver, but your USB device. According to
the USB2.0 spec, the packet size is fixed to 512 for high-speed bulk
transfers. So your device must be able to handle 512 byte transfers or
it's a non-compliant USB device.

> > > > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > > > +             if (len > nct6694->maxp)
> > > > > > > +                     packet_len = nct6694->maxp;
> > > > > > > +             else
> > > > > > > +                     packet_len = len;
> > > > > > > +
> > > > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > > > +             if (ret)
> > > > > > > +                     goto err;
> > > > > > > +     }
> > > > > > > +
> > > > > > > +     for (i = 0; i < rd_len; i++)
> > > > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > > > >
> > > > > > memcpy()?
> > > > > >
> > > > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > > > of the data doesn't make it faster.
> > > > > >
> > > > > > On the other hand, receiving directly into the target buffer means the
> > > > > > target buffer must not live on the stack.
> > > > >
> > > > > [Ming] Okay! I'll change it to memcpy().
> > > >
> > > > fine!
> > > >
> > > > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > > > by the MFD device. does it need to be changed?
> > > >
> > > > My question is: Why do you first receive into the nct6694->rx_buffer and
> > > > then memcpy() to the buffer provided by the caller, why don't you
> > > > directly receive into the memory provided by the caller?
> > >
> > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> > > using the MFD'd dynamically allocated buffer to submit URBs will better
> > > manage USB-related operations
> >
> > The non-compliant max packet size limitation is unrelated to the
> > question which RX or TX buffer to use.
> 
> I think these two USB functions can be easily called using the buffer
> dynamically
> allocated by the MFD. However, if they transfer data directly to the
> target buffer,
> they must ensure that it is not located on the stack.

You have a high coupling between the MFD driver and the individual
drivers anyways, so why not directly use the dynamically allocated
buffer provided by the caller and get rid of the memcpy()?

regards,
Marc

Ming Yu Oct. 28, 2024, 8:31 a.m. UTC | #19

Marc Kleine-Budde <[e-mail]> 於 2024年10月28日 週一 下午3:52寫道：
>
> On 28.10.2024 15:33:08, Ming Yu wrote:
> > Dear Marc,
> >
> > Thank you for your comments,
> >
> > Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午8:24寫道：
> > >
> > > On 25.10.2024 19:03:55, Ming Yu wrote:
> > > > Oh! I'm sorry about that I confused the packet size.
> > > > The NCT6694 bulk maximum packet size is 256 bytes,
> > > > and USB High speed bulk maximum packet size is 512 bytes.
> > > >
> > > > Marc Kleine-Budde <[e-mail]> 於 2024年10月25日 週五 下午6:08寫道：
> > > > >
> > > > > On 25.10.2024 16:08:10, Ming Yu wrote:
> > > > > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
> > > > > > > > +                  u8 rd_idx, u8 rd_len, unsigned char *buf)
> > > > > > >
> > > > > > > why not make buf a void *?
> > > > > >
> > > > > > [Ming] I'll change the type in the next patch.
> > > > > >
> > > > > > >
> > > > > > > > +{
> > > > > > > > +     struct usb_device *udev = nct6694->udev;
> > > > > > > > +     unsigned char err_status;
> > > > > > > > +     int len, packet_len, tx_len, rx_len;
> > > > > > > > +     int i, ret;
> > > > > > > > +
> > > > > > > > +     mutex_lock(&nct6694->access_lock);
> > > > > > > > +
> > > > > > > > +     /* Send command packet to USB device */
> > > > > > > > +     nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
> > > > > > > > +     nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
> > > > > > > > +     nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
> > > > > > > > +     nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
> > > > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
> > > > > > > > +     nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
> > > > > > > > +
> > > > > > > > +     ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
> > > > > > > > +                        nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
> > > > > > > > +                        nct6694->timeout);
> > > > > > > > +     if (ret)
> > > > > > > > +             goto err;
> > > > > > > > +
> > > > > > > > +     /* Receive response packet from USB device */
> > > > > > > > +     ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > > +                        nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
> > > > > > > > +                        nct6694->timeout);
> > > > > > > > +     if (ret)
> > > > > > > > +             goto err;
> > > > > > > > +
> > > > > > > > +     err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
> > > > > > > > +
> > > > > > > > +     /*
> > > > > > > > +      * Segmented reception of messages that exceed the size of USB bulk
> > > > > > > > +      * pipe packets.
> > > > > > > > +      */
> > > > > > >
> > > > > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > > > > >
> > > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > > > > The core will divide packet 256 bytes for high speed USB device, but
> > > > > > it is exceeds
> > > > > > the hardware limitation, so I am dividing it manually.
> > > > >
> > > > > You say the endpoint descriptor is correctly reporting it's max packet
> > > > > size of 128, but the Linux USB will send packets of 256 bytes?
> > > >
> > > > [Ming] The endpoint descriptor is correctly reporting it's max packet
> > > > size of 256, but the Linux USB may send more than 256 (max is 512)
> > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446
> > >
> > > AFAIK according to the USB-2.0 spec the maximum packet size for
> > > high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
> > > your device is a non-compliant USB device?
> >
> > We will reduce the endpoint size of other interfaces to ensure that MFD device
> > meets the USB2.0 spec. In other words, I will remove the code for manual
> > unpacking in the next patch.
>
> I was not talking about the driver, but your USB device. According to
> the USB2.0 spec, the packet size is fixed to 512 for high-speed bulk
> transfers. So your device must be able to handle 512 byte transfers or
> it's a non-compliant USB device.

I understand. Therefore, the USB device's firmware will be modified to support
bulk pipe size of 512 bytes to comply with the USB 2.0 spec.

>
> > > > > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > > > > +             if (len > nct6694->maxp)
> > > > > > > > +                     packet_len = nct6694->maxp;
> > > > > > > > +             else
> > > > > > > > +                     packet_len = len;
> > > > > > > > +
> > > > > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > > > > +             if (ret)
> > > > > > > > +                     goto err;
> > > > > > > > +     }
> > > > > > > > +
> > > > > > > > +     for (i = 0; i < rd_len; i++)
> > > > > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > > > > >
> > > > > > > memcpy()?
> > > > > > >
> > > > > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > > > > of the data doesn't make it faster.
> > > > > > >
> > > > > > > On the other hand, receiving directly into the target buffer means the
> > > > > > > target buffer must not live on the stack.
> > > > > >
> > > > > > [Ming] Okay! I'll change it to memcpy().
> > > > >
> > > > > fine!
> > > > >
> > > > > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > > > > by the MFD device. does it need to be changed?
> > > > >
> > > > > My question is: Why do you first receive into the nct6694->rx_buffer and
> > > > > then memcpy() to the buffer provided by the caller, why don't you
> > > > > directly receive into the memory provided by the caller?
> > > >
> > > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> > > > using the MFD'd dynamically allocated buffer to submit URBs will better
> > > > manage USB-related operations
> > >
> > > The non-compliant max packet size limitation is unrelated to the
> > > question which RX or TX buffer to use.
> >
> > I think these two USB functions can be easily called using the buffer
> > dynamically
> > allocated by the MFD. However, if they transfer data directly to the
> > target buffer,
> > they must ensure that it is not located on the stack.
>
> You have a high coupling between the MFD driver and the individual
> drivers anyways, so why not directly use the dynamically allocated
> buffer provided by the caller and get rid of the memcpy()?

Okay! I will provide a function to request and free buffer for child devices,
and update the caller's variables to use these two functions in the next patch.

>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Thanks,
Ming

Marc Kleine-Budde Oct. 28, 2024, 2:06 p.m. UTC | #20

On 28.10.2024 16:31:25, Ming Yu wrote:
> > > > > > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > > > > > >
> > > > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > > > > > The core will divide packet 256 bytes for high speed USB device, but
> > > > > > > it is exceeds
> > > > > > > the hardware limitation, so I am dividing it manually.
> > > > > >
> > > > > > You say the endpoint descriptor is correctly reporting it's max packet
> > > > > > size of 128, but the Linux USB will send packets of 256 bytes?
> > > > >
> > > > > [Ming] The endpoint descriptor is correctly reporting it's max packet
> > > > > size of 256, but the Linux USB may send more than 256 (max is 512)
> > > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446
> > > >
> > > > AFAIK according to the USB-2.0 spec the maximum packet size for
> > > > high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
> > > > your device is a non-compliant USB device?
> > >
> > > We will reduce the endpoint size of other interfaces to ensure that MFD device
> > > meets the USB2.0 spec. In other words, I will remove the code for manual
> > > unpacking in the next patch.
> >
> > I was not talking about the driver, but your USB device. According to
> > the USB2.0 spec, the packet size is fixed to 512 for high-speed bulk
> > transfers. So your device must be able to handle 512 byte transfers or
> > it's a non-compliant USB device.
> 
> I understand. Therefore, the USB device's firmware will be modified to support
> bulk pipe size of 512 bytes to comply with the USB 2.0 spec.

Then you don't need manual segmentation of bulk transfers anymore!

> > > > > > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > > > > > +             if (len > nct6694->maxp)
> > > > > > > > > +                     packet_len = nct6694->maxp;
> > > > > > > > > +             else
> > > > > > > > > +                     packet_len = len;
> > > > > > > > > +
> > > > > > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > > > > > +             if (ret)
> > > > > > > > > +                     goto err;
> > > > > > > > > +     }
> > > > > > > > > +
> > > > > > > > > +     for (i = 0; i < rd_len; i++)
> > > > > > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > > > > > >
> > > > > > > > memcpy()?
> > > > > > > >
> > > > > > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > > > > > of the data doesn't make it faster.
> > > > > > > >
> > > > > > > > On the other hand, receiving directly into the target buffer means the
> > > > > > > > target buffer must not live on the stack.
> > > > > > >
> > > > > > > [Ming] Okay! I'll change it to memcpy().
> > > > > >
> > > > > > fine!
> > > > > >
> > > > > > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > > > > > by the MFD device. does it need to be changed?
> > > > > >
> > > > > > My question is: Why do you first receive into the nct6694->rx_buffer and
> > > > > > then memcpy() to the buffer provided by the caller, why don't you
> > > > > > directly receive into the memory provided by the caller?
> > > > >
> > > > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> > > > > using the MFD'd dynamically allocated buffer to submit URBs will better
> > > > > manage USB-related operations
> > > >
> > > > The non-compliant max packet size limitation is unrelated to the
> > > > question which RX or TX buffer to use.
> > >
> > > I think these two USB functions can be easily called using the buffer
> > > dynamically
> > > allocated by the MFD. However, if they transfer data directly to the
> > > target buffer,
> > > they must ensure that it is not located on the stack.
> >
> > You have a high coupling between the MFD driver and the individual
> > drivers anyways, so why not directly use the dynamically allocated
> > buffer provided by the caller and get rid of the memcpy()?
> 
> Okay! I will provide a function to request and free buffer for child devices,
> and update the caller's variables to use these two functions in the next patch.

I don't see a need to provide dedicated function to allocate and free
the buffers. The caller can allocate them as part of their private data,
or allocate them during probe().

regards,
Marc

Ming Yu Oct. 29, 2024, 3:45 a.m. UTC | #21

Marc Kleine-Budde <[e-mail]> 於 2024年10月28日 週一 下午10:06寫道：
>
> On 28.10.2024 16:31:25, Ming Yu wrote:
> > > > > > > > > The Linux USB stack can receive bulk messages longer than the max packet size.
> > > > > > > >
> > > > > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device.
> > > > > > > > The core will divide packet 256 bytes for high speed USB device, but
> > > > > > > > it is exceeds
> > > > > > > > the hardware limitation, so I am dividing it manually.
> > > > > > >
> > > > > > > You say the endpoint descriptor is correctly reporting it's max packet
> > > > > > > size of 128, but the Linux USB will send packets of 256 bytes?
> > > > > >
> > > > > > [Ming] The endpoint descriptor is correctly reporting it's max packet
> > > > > > size of 256, but the Linux USB may send more than 256 (max is 512)
> > > > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446
> > > > >
> > > > > AFAIK according to the USB-2.0 spec the maximum packet size for
> > > > > high-speed bulk transfers is fixed set to 512 bytes. Does this mean that
> > > > > your device is a non-compliant USB device?
> > > >
> > > > We will reduce the endpoint size of other interfaces to ensure that MFD device
> > > > meets the USB2.0 spec. In other words, I will remove the code for manual
> > > > unpacking in the next patch.
> > >
> > > I was not talking about the driver, but your USB device. According to
> > > the USB2.0 spec, the packet size is fixed to 512 for high-speed bulk
> > > transfers. So your device must be able to handle 512 byte transfers or
> > > it's a non-compliant USB device.
> >
> > I understand. Therefore, the USB device's firmware will be modified to support
> > bulk pipe size of 512 bytes to comply with the USB 2.0 spec.
>
> Then you don't need manual segmentation of bulk transfers anymore!

Understood, thank you very much.

>
> > > > > > > > > > +     for (i = 0, len = length; len > 0; i++, len -= packet_len) {
> > > > > > > > > > +             if (len > nct6694->maxp)
> > > > > > > > > > +                     packet_len = nct6694->maxp;
> > > > > > > > > > +             else
> > > > > > > > > > +                     packet_len = len;
> > > > > > > > > > +
> > > > > > > > > > +             ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
> > > > > > > > > > +                                nct6694->rx_buffer + nct6694->maxp * i,
> > > > > > > > > > +                                packet_len, &rx_len, nct6694->timeout);
> > > > > > > > > > +             if (ret)
> > > > > > > > > > +                     goto err;
> > > > > > > > > > +     }
> > > > > > > > > > +
> > > > > > > > > > +     for (i = 0; i < rd_len; i++)
> > > > > > > > > > +             buf[i] = nct6694->rx_buffer[i + rd_idx];
> > > > > > > > >
> > > > > > > > > memcpy()?
> > > > > > > > >
> > > > > > > > > Or why don't you directly receive data into the provided buffer? Copying
> > > > > > > > > of the data doesn't make it faster.
> > > > > > > > >
> > > > > > > > > On the other hand, receiving directly into the target buffer means the
> > > > > > > > > target buffer must not live on the stack.
> > > > > > > >
> > > > > > > > [Ming] Okay! I'll change it to memcpy().
> > > > > > >
> > > > > > > fine!
> > > > > > >
> > > > > > > > This is my perspective: the data is uniformly received by the rx_bffer held
> > > > > > > > by the MFD device. does it need to be changed?
> > > > > > >
> > > > > > > My question is: Why do you first receive into the nct6694->rx_buffer and
> > > > > > > then memcpy() to the buffer provided by the caller, why don't you
> > > > > > > directly receive into the memory provided by the caller?
> > > > > >
> > > > > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently
> > > > > > using the MFD'd dynamically allocated buffer to submit URBs will better
> > > > > > manage USB-related operations
> > > > >
> > > > > The non-compliant max packet size limitation is unrelated to the
> > > > > question which RX or TX buffer to use.
> > > >
> > > > I think these two USB functions can be easily called using the buffer
> > > > dynamically
> > > > allocated by the MFD. However, if they transfer data directly to the
> > > > target buffer,
> > > > they must ensure that it is not located on the stack.
> > >
> > > You have a high coupling between the MFD driver and the individual
> > > drivers anyways, so why not directly use the dynamically allocated
> > > buffer provided by the caller and get rid of the memcpy()?
> >
> > Okay! I will provide a function to request and free buffer for child devices,
> > and update the caller's variables to use these two functions in the next patch.
>
> I don't see a need to provide dedicated function to allocate and free
> the buffers. The caller can allocate them as part of their private data,
> or allocate them during probe().

Okay, so each child device may allocate a buffer like this during probe():
priv->xmit_buf = devm_kcalloc(dev, MAX_PACKET_SZ, sizeof(unsigned char),
GFP_KERNEL), right?

>
> regards,
> Marc
>
> --
> Pengutronix e.K.                 | Marc Kleine-Budde          |
> Embedded Linux                   | https://www.pengutronix.de |
> Vertretung Nürnberg              | Phone: [phone] |
> Amtsgericht Hildesheim, HRA 2686 | Fax:   [phone]   |

Thanks,
Ming

Marc Kleine-Budde Oct. 29, 2024, 8:14 a.m. UTC | #22

On 29.10.2024 11:45:30, Ming Yu wrote:
> > > > You have a high coupling between the MFD driver and the individual
> > > > drivers anyways, so why not directly use the dynamically allocated
> > > > buffer provided by the caller and get rid of the memcpy()?
> > >
> > > Okay! I will provide a function to request and free buffer for child devices,
> > > and update the caller's variables to use these two functions in the next patch.
> >
> > I don't see a need to provide dedicated function to allocate and free
> > the buffers. The caller can allocate them as part of their private data,
> > or allocate them during probe().
> 
> Okay, so each child device may allocate a buffer like this during probe():
> priv->xmit_buf = devm_kcalloc(dev, MAX_PACKET_SZ, sizeof(unsigned char),
> GFP_KERNEL), right?

basically yes, probably devm_kzalloc() or embed it into the priv struct
directly with ____cacheline_aligned:

| https://elixir.bootlin.com/linux/v6.11.5/source/drivers/net/can/spi/mcp251xfd/mcp251xfd.h#L498

The size of the driver's RX and TX buffers depend on what they want to
send and expect to receive. The next step would be to create structs the
describe the RX and TX buffers for each driver. If you have a common
header between each driver, create that first.

regards,
Marc

Ming Yu Nov. 1, 2024, 5:35 a.m. UTC | #23

> > Okay, so each child device may allocate a buffer like this during probe():
> > priv->xmit_buf = devm_kcalloc(dev, MAX_PACKET_SZ, sizeof(unsigned char),
> > GFP_KERNEL), right?
>
> basically yes, probably devm_kzalloc() or embed it into the priv struct
> directly with ____cacheline_aligned:
>
> | https://elixir.bootlin.com/linux/v6.11.5/source/drivers/net/can/spi/mcp251xfd/mcp251xfd.h#L498
>
> The size of the driver's RX and TX buffers depend on what they want to
> send and expect to receive. The next step would be to create structs the
> describe the RX and TX buffers for each driver. If you have a common
> header between each driver, create that first.
>
> regards,
> Marc
>

Understood, I'll make these modifications in the next patch.

Thanks,
Ming

Patch

diff --git a/MAINTAINERS b/MAINTAINERS
index e9659a5a7fb3..30157ca95cf3 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -16434,6 +16434,13 @@  F:	drivers/nubus/
 F:	include/linux/nubus.h
 F:	include/uapi/linux/nubus.h
 
+NUVOTON NCT6694 MFD DRIVER
+M:	Ming Yu <[e-mail]>
+L:	[e-mail]
+S:	Supported
+F:	drivers/mfd/nct6694.c
+F:	include/linux/mfd/nct6694.h
+
 NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
 M:	Antonino Daplas <[e-mail]>
 L:	[e-mail]
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index f9325bcce1b9..da2600958697 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -546,6 +546,16 @@  config MFD_MX25_TSADC
 	  i.MX25 processors. They consist of a conversion queue for general
 	  purpose ADC and a queue for Touchscreens.
 
+config MFD_NCT6694
+	tristate "Nuvoton NCT6694 support"
+	select MFD_CORE
+	depends on USB
+	help
+	  This adds support for Nuvoton USB device NCT6694 sharing peripherals
+	  This includes the USB devcie driver and core APIs.
+	  Additional drivers must be enabled in order to use the functionality
+	  of the device.
+
 config MFD_HI6421_PMIC
 	tristate "HiSilicon Hi6421 PMU/Codec IC"
 	depends on OF
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 2a9f91e81af8..2cf816d67d03 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -116,6 +116,8 @@  obj-$(CONFIG_TWL6040_CORE)	+= twl6040.o
 
 obj-$(CONFIG_MFD_MX25_TSADC)	+= fsl-imx25-tsadc.o
 
+obj-$(CONFIG_MFD_NCT6694)	+= nct6694.o
+
 obj-$(CONFIG_MFD_MC13XXX)	+= mc13xxx-core.o
 obj-$(CONFIG_MFD_MC13XXX_SPI)	+= mc13xxx-spi.o
 obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
new file mode 100644
index 000000000000..9838c7be0b98
--- /dev/null
+++ b/drivers/mfd/nct6694.c
@@ -0,0 +1,394 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Nuvoton NCT6694 MFD driver based on USB interface.
+ *
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ */
+
+#include <linux/io.h>
+#include <linux/usb.h>
+#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/nct6694.h>
+
+#define DRVNAME "nct6694-usb_mfd"
+
+#define MFD_DEV_SIMPLE(_name)		\
+{					\
+	.name = NCT6694_DEV_##_name,	\
+}					\
+
+#define MFD_DEV_WITH_ID(_name, _id)	\
+{					\
+	.name = NCT6694_DEV_##_name,	\
+	.id = _id,			\
+}
+
+/* MFD device resources */
+static const struct mfd_cell nct6694_dev[] = {
+	MFD_DEV_WITH_ID(GPIO, 0x0),
+	MFD_DEV_WITH_ID(GPIO, 0x1),
+	MFD_DEV_WITH_ID(GPIO, 0x2),
+	MFD_DEV_WITH_ID(GPIO, 0x3),
+	MFD_DEV_WITH_ID(GPIO, 0x4),
+	MFD_DEV_WITH_ID(GPIO, 0x5),
+	MFD_DEV_WITH_ID(GPIO, 0x6),
+	MFD_DEV_WITH_ID(GPIO, 0x7),
+	MFD_DEV_WITH_ID(GPIO, 0x8),
+	MFD_DEV_WITH_ID(GPIO, 0x9),
+	MFD_DEV_WITH_ID(GPIO, 0xA),
+	MFD_DEV_WITH_ID(GPIO, 0xB),
+	MFD_DEV_WITH_ID(GPIO, 0xC),
+	MFD_DEV_WITH_ID(GPIO, 0xD),
+	MFD_DEV_WITH_ID(GPIO, 0xE),
+	MFD_DEV_WITH_ID(GPIO, 0xF),
+
+	MFD_DEV_WITH_ID(I2C, 0x0),
+	MFD_DEV_WITH_ID(I2C, 0x1),
+	MFD_DEV_WITH_ID(I2C, 0x2),
+	MFD_DEV_WITH_ID(I2C, 0x3),
+	MFD_DEV_WITH_ID(I2C, 0x4),
+	MFD_DEV_WITH_ID(I2C, 0x5),
+
+	MFD_DEV_WITH_ID(CAN, 0x0),
+	MFD_DEV_WITH_ID(CAN, 0x1),
+
+	MFD_DEV_WITH_ID(WDT, 0x0),
+	MFD_DEV_WITH_ID(WDT, 0x1),
+
+	MFD_DEV_SIMPLE(IIO),
+	MFD_DEV_SIMPLE(HWMON),
+	MFD_DEV_SIMPLE(PWM),
+	MFD_DEV_SIMPLE(RTC),
+};
+
+int nct6694_register_handler(struct nct6694 *nct6694, int irq_bit,
+			     void (*handler)(void *), void *private_data)
+{
+	struct nct6694_handler_entry *entry;
+	unsigned long flags;
+
+	entry = kmalloc(sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	entry->irq_bit = irq_bit;
+	entry->handler = handler;
+	entry->private_data = private_data;
+
+	spin_lock_irqsave(&nct6694->lock, flags);
+	list_add_tail(&entry->list, &nct6694->handler_list);
+	spin_unlock_irqrestore(&nct6694->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL(nct6694_register_handler);
+
+int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length,
+		     u8 rd_idx, u8 rd_len, unsigned char *buf)
+{
+	struct usb_device *udev = nct6694->udev;
+	unsigned char err_status;
+	int len, packet_len, tx_len, rx_len;
+	int i, ret;
+
+	mutex_lock(&nct6694->access_lock);
+
+	/* Send command packet to USB device */
+	nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
+	nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
+	nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
+	nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET;
+	nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
+	nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
+
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
+			   nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
+			   nct6694->timeout);
+	if (ret)
+		goto err;
+
+	/* Receive response packet from USB device */
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
+			   nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
+			   nct6694->timeout);
+	if (ret)
+		goto err;
+
+	err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
+
+	/*
+	 * Segmented reception of messages that exceed the size of USB bulk
+	 * pipe packets.
+	 */
+	for (i = 0, len = length; len > 0; i++, len -= packet_len) {
+		if (len > nct6694->maxp)
+			packet_len = nct6694->maxp;
+		else
+			packet_len = len;
+
+		ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
+				   nct6694->rx_buffer + nct6694->maxp * i,
+				   packet_len, &rx_len, nct6694->timeout);
+		if (ret)
+			goto err;
+	}
+
+	for (i = 0; i < rd_len; i++)
+		buf[i] = nct6694->rx_buffer[i + rd_idx];
+
+	if (err_status) {
+		pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
+		ret = -EIO;
+	}
+
+err:
+	mutex_unlock(&nct6694->access_lock);
+	return ret;
+}
+EXPORT_SYMBOL(nct6694_read_msg);
+
+int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
+		      u16 length, unsigned char *buf)
+{
+	struct usb_device *udev = nct6694->udev;
+	unsigned char err_status;
+	int len, packet_len, tx_len, rx_len;
+	int i, ret;
+
+	mutex_lock(&nct6694->access_lock);
+
+	/* Send command packet to USB device  */
+	nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod;
+	nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF;
+	nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF;
+	nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_SET;
+	nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF;
+	nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF;
+
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
+			   nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len,
+			   nct6694->timeout);
+	if (ret)
+		goto err;
+
+	/*
+	 * Segmented transmission of messages that exceed the size of USB bulk
+	 * pipe packets.
+	 */
+	for (i = 0, len = length; len > 0; i++, len -= packet_len) {
+		if (len > nct6694->maxp)
+			packet_len = nct6694->maxp;
+		else
+			packet_len = len;
+
+		memcpy(nct6694->tx_buffer + nct6694->maxp * i,
+		       buf + nct6694->maxp * i, packet_len);
+
+		ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT),
+				   nct6694->tx_buffer + nct6694->maxp * i,
+				   packet_len, &tx_len, nct6694->timeout);
+		if (ret)
+			goto err;
+	}
+
+	/* Receive response packet from USB device */
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
+			   nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len,
+			   nct6694->timeout);
+	if (ret)
+		goto err;
+
+	err_status = nct6694->rx_buffer[RESPONSE_STS_IDX];
+
+	/*
+	 * Segmented reception of messages that exceed the size of USB bulk
+	 * pipe packets.
+	 */
+	for (i = 0, len = length; len > 0; i++, len -= packet_len) {
+		if (len > nct6694->maxp)
+			packet_len = nct6694->maxp;
+		else
+			packet_len = len;
+
+		ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT),
+				   nct6694->rx_buffer + nct6694->maxp * i,
+				   packet_len, &rx_len, nct6694->timeout);
+		if (ret)
+			goto err;
+	}
+
+	memcpy(buf, nct6694->rx_buffer, length);
+
+	if (err_status) {
+		pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status);
+		ret = -EIO;
+	}
+
+err:
+	mutex_unlock(&nct6694->access_lock);
+	return ret;
+}
+EXPORT_SYMBOL(nct6694_write_msg);
+
+static void usb_int_callback(struct urb *urb)
+{
+	unsigned char *int_status = urb->transfer_buffer;
+	struct nct6694 *nct6694 = urb->context;
+	struct nct6694_handler_entry *entry;
+	unsigned long flags;
+	int ret;
+
+	switch (urb->status) {
+	case 0:
+		break;
+	case -ECONNRESET:
+	case -ENOENT:
+	case -ESHUTDOWN:
+		return;
+	default:
+		goto resubmit;
+	}
+
+	spin_lock_irqsave(&nct6694->lock, flags);
+
+	list_for_each_entry(entry, &nct6694->handler_list, list) {
+		if (int_status[INT_IN_IRQ_IDX] & entry->irq_bit)
+			entry->handler(entry->private_data);
+	}
+
+	spin_unlock_irqrestore(&nct6694->lock, flags);
+
+resubmit:
+	ret = usb_submit_urb(urb, GFP_ATOMIC);
+	if (ret)
+		pr_debug("%s: Failed to resubmit urb, status %d",
+			 __func__, ret);
+}
+
+static int nct6694_usb_probe(struct usb_interface *iface,
+			     const struct usb_device_id *id)
+{
+	struct usb_device *udev = interface_to_usbdev(iface);
+	struct device *dev = &udev->dev;
+	struct usb_host_interface *interface;
+	struct usb_endpoint_descriptor *int_endpoint;
+	struct nct6694 *nct6694;
+	int pipe, maxp, bulk_pipe;
+	int ret = EINVAL;
+
+	interface = iface->cur_altsetting;
+	/* Binding interface class : 0xFF */
+	if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC ||
+	    interface->desc.bInterfaceSubClass != 0x00 ||
+	    interface->desc.bInterfaceProtocol != 0x00)
+		return -ENODEV;
+
+	int_endpoint = &interface->endpoint[0].desc;
+	if (!usb_endpoint_is_int_in(int_endpoint))
+		return -ENODEV;
+
+	nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL);
+	if (!nct6694)
+		return -ENOMEM;
+
+	pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT);
+	maxp = usb_maxpacket(udev, pipe);
+
+	nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ,
+					   sizeof(unsigned char), GFP_KERNEL);
+	if (!nct6694->cmd_buffer)
+		return -ENOMEM;
+	nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
+					  sizeof(unsigned char), GFP_KERNEL);
+	if (!nct6694->rx_buffer)
+		return -ENOMEM;
+	nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
+					  sizeof(unsigned char), GFP_KERNEL);
+	if (!nct6694->tx_buffer)
+		return -ENOMEM;
+	nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ,
+					   sizeof(unsigned char), GFP_KERNEL);
+	if (!nct6694->int_buffer)
+		return -ENOMEM;
+
+	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!nct6694->int_in_urb) {
+		dev_err(&udev->dev, "Failed to allocate INT-in urb!\n");
+		return -ENOMEM;
+	}
+
+	/* Bulk pipe maximum packet for each transaction */
+	bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT);
+	nct6694->maxp = usb_maxpacket(udev, bulk_pipe);
+
+	mutex_init(&nct6694->access_lock);
+	nct6694->udev = udev;
+	nct6694->timeout = URB_TIMEOUT;	/* Wait until urb complete */
+
+	INIT_LIST_HEAD(&nct6694->handler_list);
+	spin_lock_init(&nct6694->lock);
+
+	usb_fill_int_urb(nct6694->int_in_urb, udev, pipe,
+			 nct6694->int_buffer, maxp, usb_int_callback,
+			 nct6694, int_endpoint->bInterval);
+	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
+	if (ret)
+		goto err_urb;
+
+	dev_set_drvdata(&udev->dev, nct6694);
+	usb_set_intfdata(iface, nct6694);
+
+	ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev,
+				      ARRAY_SIZE(nct6694_dev));
+	if (ret) {
+		dev_err(&udev->dev, "Failed to add mfd's child device\n");
+		goto err_mfd;
+	}
+
+	nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0);
+
+	dev_info(&udev->dev, "Probed device: (%04X:%04X)\n",
+		 id->idVendor, id->idProduct);
+	return 0;
+
+err_mfd:
+	usb_kill_urb(nct6694->int_in_urb);
+err_urb:
+	usb_free_urb(nct6694->int_in_urb);
+	return ret;
+}
+
+static void nct6694_usb_disconnect(struct usb_interface *iface)
+{
+	struct usb_device *udev = interface_to_usbdev(iface);
+	struct nct6694 *nct6694 = usb_get_intfdata(iface);
+
+	mfd_remove_devices(&udev->dev);
+	flush_workqueue(nct6694->async_workqueue);
+	destroy_workqueue(nct6694->async_workqueue);
+	usb_set_intfdata(iface, NULL);
+	usb_kill_urb(nct6694->int_in_urb);
+	usb_free_urb(nct6694->int_in_urb);
+}
+
+static const struct usb_device_id nct6694_ids[] = {
+	{ USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)},
+	{},
+};
+MODULE_DEVICE_TABLE(usb, nct6694_ids);
+
+static struct usb_driver nct6694_usb_driver = {
+	.name	= DRVNAME,
+	.id_table = nct6694_ids,
+	.probe = nct6694_usb_probe,
+	.disconnect = nct6694_usb_disconnect,
+};
+
+module_usb_driver(nct6694_usb_driver);
+
+MODULE_DESCRIPTION("USB-MFD driver for NCT6694");
+MODULE_AUTHOR("Ming Yu <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
new file mode 100644
index 000000000000..0797564363be
--- /dev/null
+++ b/include/linux/mfd/nct6694.h
@@ -0,0 +1,168 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Nuvoton NCT6694 USB transaction and data structure.
+ *
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ */
+
+#ifndef __MFD_NCT6694_H
+#define __MFD_NCT6694_H
+
+#define NCT6694_DEV_GPIO		"nct6694-gpio"
+#define NCT6694_DEV_I2C			"nct6694-i2c"
+#define NCT6694_DEV_CAN			"nct6694-can"
+#define NCT6694_DEV_WDT			"nct6694-wdt"
+#define NCT6694_DEV_IIO			"nct6694-iio"
+#define NCT6694_DEV_HWMON		"nct6694-hwmon"
+#define NCT6694_DEV_PWM			"nct6694-pwm"
+#define NCT6694_DEV_RTC			"nct6694-rtc"
+
+#define NCT6694_VENDOR_ID		0x0416
+#define NCT6694_PRODUCT_ID		0x200B
+#define INT_IN_ENDPOINT			0x81
+#define BULK_IN_ENDPOINT		0x82
+#define BULK_OUT_ENDPOINT		0x03
+#define MAX_PACKET_SZ			0x100
+
+#define CMD_PACKET_SZ			0x8
+#define HCTRL_SET			0x40
+#define HCTRL_GET			0x80
+
+#define REQUEST_MOD_IDX			0x01
+#define REQUEST_CMD_IDX			0x02
+#define REQUEST_SEL_IDX			0x03
+#define REQUEST_HCTRL_IDX		0x04
+#define REQUEST_LEN_L_IDX		0x06
+#define REQUEST_LEN_H_IDX		0x07
+
+#define RESPONSE_STS_IDX		0x01
+
+#define INT_IN_IRQ_IDX			0x00
+#define GPIO_IRQ_STATUS			BIT(0)
+#define CAN_IRQ_STATUS			BIT(2)
+#define RTC_IRQ_STATUS			BIT(3)
+
+#define URB_TIMEOUT			1000
+
+/*
+ * struct nct6694 - Nuvoton NCT6694 structure
+ *
+ * @udev: Pointer to the USB device
+ * @int_in_urb: Interrupt pipe urb
+ * @access_lock: USB transaction lock
+ * @handler_list: List of registered handlers
+ * @async_workqueue: Workqueue of processing asynchronous work
+ * @tx_buffer: USB write message buffer
+ * @rx_buffer: USB read message buffer
+ * @cmd_buffer: USB send command message buffer
+ * @int_buffer: USB receive interrupt message buffer
+ * @lock: Handlers lock
+ * @timeout: URB timeout
+ * @maxp: Maximum packet of bulk pipe
+ */
+struct nct6694 {
+	struct usb_device *udev;
+	struct urb *int_in_urb;
+	struct list_head handler_list;
+	struct workqueue_struct *async_workqueue;
+
+	/* Make sure that every USB transaction is not interrupted */
+	struct mutex access_lock;
+
+	unsigned char *tx_buffer;
+	unsigned char *rx_buffer;
+	unsigned char *cmd_buffer;
+	unsigned char *int_buffer;
+
+	/* Prevent races within handlers */
+	spinlock_t lock;
+
+	/* time in msec to wait for the urb to the complete */
+	long timeout;
+
+	/* Bulk pipe maximum packet for each transaction */
+	int maxp;
+};
+
+/*
+ * struct nct6694_handler_entry - Stores the interrupt handling information
+ * for each registered peripheral
+ *
+ * @irq_bit: The bit in irq_status[INT_IN_IRQ_IDX] representing interrupt
+ * @handler: Function pointer to the interrupt handler of the peripheral
+ * @private_data: Private data specific to the peripheral driver
+ * @list: Node used to link to the handler_list
+ */
+struct nct6694_handler_entry {
+	int irq_bit;
+	void (*handler)(void *private_data);
+	void *private_data;
+	struct list_head list;
+};
+
+/*
+ * nct6694_register_handler - Register a handler with private data for
+ * interrupt pipe irq event
+ *
+ * @nct6694 - Nuvoton NCT6694 structure
+ * @irq_bit - The irq for which to register a handler
+ * @handler - The handler function
+ * @private_data - Private data for which to register a handler
+ *
+ * This function is called when peripherals need to register a handler
+ * for receiving interrupt pipe.
+ *
+ * Don't use the wait_for_completion function in handler function, as
+ * it is in interrupt context.
+ */
+int nct6694_register_handler(struct nct6694 *nct6694, int irq_bit,
+			     void (*handler)(void *),
+			     void *private_data);
+
+/*
+ * nct6694_read_msg - Receive data from NCT6694 USB device
+ *
+ * @nct6694 - Nuvoton NCT6694 structure
+ * @mod - Module byte
+ * @offset - Offset byte or (Select byte | Command byte)
+ * @length - Length byte
+ * @rd_idx - Read data from rx buffer at index
+ * @rd_len - Read length from rx buffer
+ * @buf - Read data from rx buffer
+ *
+ * USB Transaction format:
+ *
+ *	OUT	|RSV|MOD|CMD|SEL|HCTL|RSV|LEN_L|LEN_H|
+ *	OUT	|SEQ|STS|RSV|RSV|RSV|RSV||LEN_L|LEN_H|
+ *	IN	|-------D------A------D------A-------|
+ *	IN			......
+ *	IN	|-------D------A------D------A-------|
+ */
+int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
+		     u16 length, u8 rd_idx, u8 rd_len,
+		     unsigned char *buf);
+
+/*
+ * nct6694_read_msg - Transmit data to NCT6694 USB device
+ *
+ * @nct6694 - Nuvoton NCT6694 structure
+ * @mod - Module byte
+ * @offset - Offset byte or (Select byte | Command byte)
+ * @length - Length byte
+ * @buf - Write data to tx buffer
+ *
+ * USB Transaction format:
+ *
+ *	OUT	|RSV|MOD|CMD|SEL|HCTL|RSV|LEN_L|LEN_H|
+ *	OUT	|-------D------A------D------A-------|
+ *	OUT			......
+ *	OUT	|-------D------A------D------A-------|
+ *	IN	|SEQ|STS|RSV|RSV|RSV|RSV||LEN_L|LEN_H|
+ *	IN	|-------D------A------D------A-------|
+ *	IN			......
+ *	IN	|-------D------A------D------A-------|
+ */
+int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
+		      u16 length, unsigned char *buf);
+
+#endif