[PATCH v7 01/10] mfd: nct6694: Move module type macros to shared header

[PATCH v7 01/10] mfd: nct6694: Move module type macros to shared header

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Move NCT6694_XXX_MOD macro definitions from individual sub-device
drivers into the shared header include/linux/mfd/nct6694.h.

This is a prerequisite for supporting multiple transport interfaces
(USB, HIF) without duplicating these definitions.

No functional change.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:

Changes in v6:

Changes in v5:
- Split from the monolithic v4 patch to follow the single logical change
  principle.

 drivers/gpio/gpio-nct6694.c         |  7 -------
 drivers/hwmon/nct6694-hwmon.c       | 21 ---------------------
 drivers/i2c/busses/i2c-nct6694.c    |  7 -------
 drivers/net/can/usb/nct6694_canfd.c |  6 ------
 drivers/rtc/rtc-nct6694.c           |  7 -------
 drivers/watchdog/nct6694_wdt.c      |  7 -------
 include/linux/mfd/nct6694.h         |  9 +++++++++
 7 files changed, 9 insertions(+), 55 deletions(-)

diff --git a/drivers/gpio/gpio-nct6694.c b/drivers/gpio/gpio-nct6694.c
index a8607f0d9915..53bfc5983648 100644
--- a/drivers/gpio/gpio-nct6694.c
+++ b/drivers/gpio/gpio-nct6694.c
@@ -13,13 +13,6 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-/*
- * USB command module type for NCT6694 GPIO controller.
- * This defines the module type used for communication with the NCT6694
- * GPIO controller over the USB interface.
- */
-#define NCT6694_GPIO_MOD	0xFF
-
 #define NCT6694_GPIO_VER	0x90
 #define NCT6694_GPIO_VALID	0x110
 #define NCT6694_GPI_DATA	0x120
diff --git a/drivers/hwmon/nct6694-hwmon.c b/drivers/hwmon/nct6694-hwmon.c
index 6dcf22ca5018..581451875f2c 100644
--- a/drivers/hwmon/nct6694-hwmon.c
+++ b/drivers/hwmon/nct6694-hwmon.c
@@ -15,13 +15,6 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
-/*
- * USB command module type for NCT6694 report channel
- * This defines the module type used for communication with the NCT6694
- * report channel over the USB interface.
- */
-#define NCT6694_RPT_MOD			0xFF
-
 /* Report channel */
 /*
  * The report channel is used to report the status of the hardware monitor
@@ -38,13 +31,6 @@
 #define NCT6694_TIN_STS(x)		(0x6A + (x))
 #define NCT6694_FIN_STS(x)		(0x6E + (x))
 
-/*
- * USB command module type for NCT6694 HWMON controller.
- * This defines the module type used for communication with the NCT6694
- * HWMON controller over the USB interface.
- */
-#define NCT6694_HWMON_MOD		0x00
-
 /* Command 00h - Hardware Monitor Control */
 #define NCT6694_HWMON_CONTROL		0x00
 #define NCT6694_HWMON_CONTROL_SEL	0x00
@@ -53,13 +39,6 @@
 #define NCT6694_HWMON_ALARM		0x02
 #define NCT6694_HWMON_ALARM_SEL		0x00
 
-/*
- * USB command module type for NCT6694 PWM controller.
- * This defines the module type used for communication with the NCT6694
- * PWM controller over the USB interface.
- */
-#define NCT6694_PWM_MOD			0x01
-
 /* PWM Command - Manual Control */
 #define NCT6694_PWM_CONTROL		0x01
 #define NCT6694_PWM_CONTROL_SEL		0x00
diff --git a/drivers/i2c/busses/i2c-nct6694.c b/drivers/i2c/busses/i2c-nct6694.c
index 1413ab6f9462..ef3329f34246 100644
--- a/drivers/i2c/busses/i2c-nct6694.c
+++ b/drivers/i2c/busses/i2c-nct6694.c
@@ -12,13 +12,6 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 
-/*
- * USB command module type for NCT6694 I2C controller.
- * This defines the module type used for communication with the NCT6694
- * I2C controller over the USB interface.
- */
-#define NCT6694_I2C_MOD			0x03
-
 /* Command 00h - I2C Deliver */
 #define NCT6694_I2C_DELIVER		0x00
 #define NCT6694_I2C_DELIVER_SEL		0x00
diff --git a/drivers/net/can/usb/nct6694_canfd.c b/drivers/net/can/usb/nct6694_canfd.c
index e5f7f8849a73..262b4c26c9d4 100644
--- a/drivers/net/can/usb/nct6694_canfd.c
+++ b/drivers/net/can/usb/nct6694_canfd.c
@@ -18,12 +18,6 @@
 
 #define DEVICE_NAME "nct6694-canfd"
 
-/* USB command module type for NCT6694 CANfd controller.
- * This defines the module type used for communication with the NCT6694
- * CANfd controller over the USB interface.
- */
-#define NCT6694_CANFD_MOD			0x05
-
 /* Command 00h - CAN Setting and Initialization */
 #define NCT6694_CANFD_SETTING			0x00
 #define NCT6694_CANFD_SETTING_ACTIVE_CTRL1	BIT(0)
diff --git a/drivers/rtc/rtc-nct6694.c b/drivers/rtc/rtc-nct6694.c
index 35401a0d9cf5..c06902f150c9 100644
--- a/drivers/rtc/rtc-nct6694.c
+++ b/drivers/rtc/rtc-nct6694.c
@@ -14,13 +14,6 @@
 #include <linux/rtc.h>
 #include <linux/slab.h>
 
-/*
- * USB command module type for NCT6694 RTC controller.
- * This defines the module type used for communication with the NCT6694
- * RTC controller over the USB interface.
- */
-#define NCT6694_RTC_MOD		0x08
-
 /* Command 00h - RTC Time */
 #define NCT6694_RTC_TIME	0x0000
 #define NCT6694_RTC_TIME_SEL	0x00
diff --git a/drivers/watchdog/nct6694_wdt.c b/drivers/watchdog/nct6694_wdt.c
index bc3689bd4b6b..4c06ac105562 100644
--- a/drivers/watchdog/nct6694_wdt.c
+++ b/drivers/watchdog/nct6694_wdt.c
@@ -20,13 +20,6 @@
 
 #define NCT6694_WDT_MAX_DEVS		2
 
-/*
- * USB command module type for NCT6694 WDT controller.
- * This defines the module type used for communication with the NCT6694
- * WDT controller over the USB interface.
- */
-#define NCT6694_WDT_MOD			0x07
-
 /* Command 00h - WDT Setup */
 #define NCT6694_WDT_SETUP		0x00
 #define NCT6694_WDT_SETUP_SEL(idx)	(idx ? 0x01 : 0x00)
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
index 6eb9be2cd4a0..3c683e317aa3 100644
--- a/include/linux/mfd/nct6694.h
+++ b/include/linux/mfd/nct6694.h
@@ -8,6 +8,15 @@
 #ifndef __MFD_NCT6694_H
 #define __MFD_NCT6694_H
 
+#define NCT6694_HWMON_MOD	0x00
+#define NCT6694_PWM_MOD		0x01
+#define NCT6694_I2C_MOD		0x03
+#define NCT6694_CANFD_MOD	0x05
+#define NCT6694_WDT_MOD		0x07
+#define NCT6694_RTC_MOD		0x08
+#define NCT6694_RPT_MOD		0xFF
+#define NCT6694_GPIO_MOD	NCT6694_RPT_MOD
+
 #define NCT6694_VENDOR_ID	0x0416
 #define NCT6694_PRODUCT_ID	0x200B
 #define NCT6694_INT_IN_EP	0x81
-- 
2.34.1

Re: [PATCH v7 01/10] mfd: nct6694: Move module type macros to shared header

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Moving driver-specific macros into a global header that facilitates bespoke parent-child accessors.
--

--- Patch [1]: [PATCH v7 01/10] mfd: nct6694: Move module type macros to shared header ---
commit ddd165427cd793b4dafe2d040a37ea0bf7d0d5fc
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Move module type macros to shared header

This commit moves module type macros from individual sub-device drivers into the
global MFD header include/linux/mfd/nct6694.h. The commit message states this
is a prerequisite for supporting multiple transport interfaces without
duplicating these definitions.

> This is a prerequisite for supporting multiple transport interfaces
> (USB, HIF) without duplicating these definitions.

[Severity: Medium]
Is it accurate that multiple transport interfaces use these definitions?

Since these module ID macros appear to be used exclusively by their respective
child drivers, shouldn't they remain local to those drivers rather than being
centralized to prevent duplicated definitions?

[ ... ]

> diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> index 6eb9be2cd4a01..3c683e317aa32 100644
> --- a/include/linux/mfd/nct6694.h
> +++ b/include/linux/mfd/nct6694.h
> @@ -8,6 +8,15 @@
>  #ifndef __MFD_NCT6694_H
>  #define __MFD_NCT6694_H
>  
> +#define NCT6694_HWMON_MOD	0x00
> +#define NCT6694_PWM_MOD		0x01
> +#define NCT6694_I2C_MOD		0x03
> +#define NCT6694_CANFD_MOD	0x05
> +#define NCT6694_WDT_MOD		0x07
> +#define NCT6694_RTC_MOD		0x08
> +#define NCT6694_RPT_MOD		0xFF
> +#define NCT6694_GPIO_MOD	NCT6694_RPT_MOD

[Severity: Medium]
Does centralizing these macros here violate the MFD subsystem guidelines?

The guidelines state that driver-specific definitions should not be placed in
the global include/linux/mfd/ directory if they are only used by the parent
and its immediate children. Keeping them local to drivers/mfd/ or the
individual sub-devices is preferred.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=1

[PATCH v7 02/10] mfd: nct6694: Refactor USB-specific data into nct6694_usb_data

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Separate USB transport-specific fields from the core nct6694 structure
into a new nct6694_usb_data structure. This decouples the shared MFD
core from the USB transport layer, preparing the driver for potential
support of alternative transport backends in the future.

The following fields are moved into nct6694_usb_data:
  - access_lock
  - int_in_urb
  - udev
  - usb_msg
  - int_buffer

The core nct6694 structure now holds a void *priv pointer to reference
the transport-specific data. USB-only definitions (vendor/product IDs,
endpoint addresses, URB timeout, and the USB message union) are also
moved from the shared header into the USB driver source file.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:

Changes in v6:

Changes in v5:
- Split from the monolithic v4 patch to follow the single logical change
  principle.

 drivers/mfd/nct6694.c       | 76 ++++++++++++++++++++++++++-----------
 include/linux/mfd/nct6694.h | 27 ++++---------
 2 files changed, 62 insertions(+), 41 deletions(-)

diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
index 308b2fda3055..58c1cbcbe3f2 100644
--- a/drivers/mfd/nct6694.c
+++ b/drivers/mfd/nct6694.c
@@ -10,8 +10,8 @@
  */
 
 #include <linux/bits.h>
-#include <linux/interrupt.h>
 #include <linux/idr.h>
+#include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/kernel.h>
@@ -22,6 +22,27 @@
 #include <linux/spinlock.h>
 #include <linux/usb.h>
 
+#define NCT6694_VENDOR_ID	0x0416
+#define NCT6694_PRODUCT_ID	0x200B
+#define NCT6694_INT_IN_EP	0x81
+#define NCT6694_BULK_IN_EP	0x02
+#define NCT6694_BULK_OUT_EP	0x03
+
+#define NCT6694_URB_TIMEOUT	1000
+
+union __packed nct6694_usb_msg {
+	struct nct6694_cmd_header cmd_header;
+	struct nct6694_response_header response_header;
+};
+
+struct nct6694_usb_data {
+	struct mutex access_lock;
+	struct urb *int_in_urb;
+	struct usb_device *udev;
+	union nct6694_usb_msg *usb_msg;
+	__le32 *int_buffer;
+};
+
 static const struct mfd_cell nct6694_devs[] = {
 	MFD_CELL_NAME("nct6694-gpio"),
 	MFD_CELL_NAME("nct6694-gpio"),
@@ -96,11 +117,12 @@ static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char
  */
 int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
 {
-	union nct6694_usb_msg *msg = nct6694->usb_msg;
-	struct usb_device *udev = nct6694->udev;
+	struct nct6694_usb_data *udata = nct6694->priv;
+	union nct6694_usb_msg *msg = udata->usb_msg;
+	struct usb_device *udev = udata->udev;
 	int tx_len, rx_len, ret;
 
-	guard(mutex)(&nct6694->access_lock);
+	guard(mutex)(&udata->access_lock);
 
 	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
 	msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
@@ -146,11 +168,12 @@ EXPORT_SYMBOL_GPL(nct6694_read_msg);
  */
 int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
 {
-	union nct6694_usb_msg *msg = nct6694->usb_msg;
-	struct usb_device *udev = nct6694->udev;
+	struct nct6694_usb_data *udata = nct6694->priv;
+	union nct6694_usb_msg *msg = udata->usb_msg;
+	struct usb_device *udev = udata->udev;
 	int tx_len, rx_len, ret;
 
-	guard(mutex)(&nct6694->access_lock);
+	guard(mutex)(&udata->access_lock);
 
 	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
 	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
@@ -277,6 +300,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	struct usb_endpoint_descriptor *int_endpoint;
 	struct usb_host_interface *interface;
 	struct device *dev = &iface->dev;
+	struct nct6694_usb_data *udata;
 	struct nct6694 *nct6694;
 	int ret;
 
@@ -284,18 +308,26 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	if (!nct6694)
 		return -ENOMEM;
 
-	nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg), GFP_KERNEL);
-	if (!nct6694->usb_msg)
+	udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
+	if (!udata)
+		return -ENOMEM;
+
+	udata->usb_msg = devm_kzalloc(dev, sizeof(*udata->usb_msg), GFP_KERNEL);
+	if (!udata->usb_msg)
 		return -ENOMEM;
 
-	nct6694->int_buffer = devm_kzalloc(dev, sizeof(*nct6694->int_buffer), GFP_KERNEL);
-	if (!nct6694->int_buffer)
+	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
+	if (!udata->int_buffer)
 		return -ENOMEM;
 
-	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
-	if (!nct6694->int_in_urb)
+	udata->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
+	if (!udata->int_in_urb)
 		return -ENOMEM;
 
+	udata->udev = udev;
+
+	nct6694->priv = udata;
+
 	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
 						   &nct6694_irq_domain_ops,
 						   nct6694);
@@ -305,7 +337,6 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	}
 
 	nct6694->dev = dev;
-	nct6694->udev = udev;
 
 	ida_init(&nct6694->gpio_ida);
 	ida_init(&nct6694->i2c_ida);
@@ -314,7 +345,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 
 	spin_lock_init(&nct6694->irq_lock);
 
-	ret = devm_mutex_init(dev, &nct6694->access_lock);
+	ret = devm_mutex_init(dev, &udata->access_lock);
 	if (ret)
 		goto err_ida;
 
@@ -326,11 +357,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 		goto err_ida;
 	}
 
-	usb_fill_int_urb(nct6694->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
-			 nct6694->int_buffer, sizeof(*nct6694->int_buffer), usb_int_callback,
+	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
+			 udata->int_buffer, sizeof(*udata->int_buffer), usb_int_callback,
 			 nct6694, int_endpoint->bInterval);
 
-	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
+	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
 	if (ret)
 		goto err_ida;
 
@@ -343,7 +374,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	return 0;
 
 err_mfd:
-	usb_kill_urb(nct6694->int_in_urb);
+	usb_kill_urb(udata->int_in_urb);
 err_ida:
 	ida_destroy(&nct6694->wdt_ida);
 	ida_destroy(&nct6694->canfd_ida);
@@ -351,22 +382,23 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	ida_destroy(&nct6694->gpio_ida);
 	irq_domain_remove(nct6694->domain);
 err_urb:
-	usb_free_urb(nct6694->int_in_urb);
+	usb_free_urb(udata->int_in_urb);
 	return ret;
 }
 
 static void nct6694_usb_disconnect(struct usb_interface *iface)
 {
 	struct nct6694 *nct6694 = usb_get_intfdata(iface);
+	struct nct6694_usb_data *udata = nct6694->priv;
 
 	mfd_remove_devices(nct6694->dev);
-	usb_kill_urb(nct6694->int_in_urb);
+	usb_kill_urb(udata->int_in_urb);
 	ida_destroy(&nct6694->wdt_ida);
 	ida_destroy(&nct6694->canfd_ida);
 	ida_destroy(&nct6694->i2c_ida);
 	ida_destroy(&nct6694->gpio_ida);
 	irq_domain_remove(nct6694->domain);
-	usb_free_urb(nct6694->int_in_urb);
+	usb_free_urb(udata->int_in_urb);
 }
 
 static const struct usb_device_id nct6694_ids[] = {
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
index 3c683e317aa3..3f5dd53f38de 100644
--- a/include/linux/mfd/nct6694.h
+++ b/include/linux/mfd/nct6694.h
@@ -2,12 +2,18 @@
 /*
  * Copyright (C) 2025 Nuvoton Technology Corp.
  *
- * Nuvoton NCT6694 USB transaction and data structure.
+ * Nuvoton NCT6694 core definitions shared by all transport drivers
+ * and sub-device drivers.
  */
 
 #ifndef __MFD_NCT6694_H
 #define __MFD_NCT6694_H
 
+#include <linux/idr.h>
+#include <linux/mutex.h>
+#include <linux/spinlock.h>
+#include <linux/types.h>
+
 #define NCT6694_HWMON_MOD	0x00
 #define NCT6694_PWM_MOD		0x01
 #define NCT6694_I2C_MOD		0x03
@@ -17,17 +23,9 @@
 #define NCT6694_RPT_MOD		0xFF
 #define NCT6694_GPIO_MOD	NCT6694_RPT_MOD
 
-#define NCT6694_VENDOR_ID	0x0416
-#define NCT6694_PRODUCT_ID	0x200B
-#define NCT6694_INT_IN_EP	0x81
-#define NCT6694_BULK_IN_EP	0x02
-#define NCT6694_BULK_OUT_EP	0x03
-
 #define NCT6694_HCTRL_SET	0x40
 #define NCT6694_HCTRL_GET	0x80
 
-#define NCT6694_URB_TIMEOUT	1000
-
 enum nct6694_irq_id {
 	NCT6694_IRQ_GPIO0 = 0,
 	NCT6694_IRQ_GPIO1,
@@ -84,11 +82,6 @@ struct __packed nct6694_response_header {
 	__le16 len;
 };
 
-union __packed nct6694_usb_msg {
-	struct nct6694_cmd_header cmd_header;
-	struct nct6694_response_header response_header;
-};
-
 struct nct6694 {
 	struct device *dev;
 	struct ida gpio_ida;
@@ -96,13 +89,9 @@ struct nct6694 {
 	struct ida canfd_ida;
 	struct ida wdt_ida;
 	struct irq_domain *domain;
-	struct mutex access_lock;
 	spinlock_t irq_lock;
-	struct urb *int_in_urb;
-	struct usb_device *udev;
-	union nct6694_usb_msg *usb_msg;
-	__le32 *int_buffer;
 	unsigned int irq_enable;
+	void *priv;
 };
 
 int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf);
-- 
2.34.1

Re: [PATCH v7 02/10] mfd: nct6694: Refactor USB-specific data into nct6694_usb_data

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] GPIO chip is registered with `can_sleep = false` despite its callbacks performing sleeping operations.
- [High] The USB interrupt callback does not check `urb->actual_length`, leading to stale data reads and interrupt storms.
- [High] The USB interrupt callback fails to validate the return value of `irq_find_mapping()`, allowing arbitrary interrupt injection.
--

--- Patch [2]: [PATCH v7 02/10] mfd: nct6694: Refactor USB-specific data into nct6694_usb_data ---
commit e3cdeafa7e419309614b6129c5ba029024c522a3
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Refactor USB-specific data into nct6694_usb_data

This commit separates USB transport-specific fields from the core nct6694
structure into a new nct6694_usb_data structure. It also adds a void *priv
pointer to the core structure to decouple the shared MFD core from the USB
transport layer.

> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 308b2fda3055c..58c1cbcbe3f22 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c
[ ... ]
> @@ -326,11 +357,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  		goto err_ida;
>  	}
>  
> -	usb_fill_int_urb(nct6694->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> -			 nct6694->int_buffer, sizeof(*nct6694->int_buffer), usb_int_callback,
> +	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> +			 udata->int_buffer, sizeof(*udata->int_buffer), usb_int_callback,
>  			 nct6694, int_endpoint->bInterval);

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
usb_int_callback() function referenced here.

drivers/mfd/nct6694.c:usb_int_callback() {
    ...
    int_status = le32_to_cpu(*status_le);
    ...
}

The driver parses the URB payload as interrupt status without verifying
urb->actual_length. If a malfunctioning USB device sends an interrupt IN
packet smaller than 4 bytes, the buffer will retain its previous payload
from the USB core.

Could this cause the driver to process the same interrupts continuously and
create an interrupt storm?

[Severity: High]
This is another pre-existing issue, but I wanted to ask about the IRQ
mapping in the same callback.

drivers/mfd/nct6694.c:usb_int_callback() {
    ...
    generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
    ...
}

The return value of irq_find_mapping() is passed directly to the generic
IRQ handler without validation. If the USB device sends a payload with bits
set for unmapped or out-of-bounds IRQs, irq_find_mapping() could return 0.

Could passing a 0 virq to generic_handle_irq_safe() lead to arbitrary
system interrupt injection, such as invoking important system timers on
some architectures?

>  
> -	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> +	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
>  	if (ret)
>  		goto err_ida;
[ ... ]

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but
there appears to be an issue in the related GPIO driver registration.

drivers/gpio/gpio-nct6694.c:nct6694_gpio_probe() {
    ...
    data->gpio.can_sleep = false;
    ...
}

The driver registers the GPIO chip with can_sleep set to false. However,
the accessors like nct6694_get_value and nct6694_set_value acquire a mutex
and perform USB bulk transfers that sleep.

Could this lead to a scheduling while atomic bug if the GPIO subsystem or
another caller attempts to toggle the GPIOs from an atomic context?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=2

Re: [PATCH v7 02/10] mfd: nct6694: Refactor USB-specific data into nct6694_usb_data

[Lee Jones]

On Fri, 21 Aug 2026, a0282524688@gmail.com wrote:

> From: Ming Yu <a0282524688@gmail.com>
> 
> Separate USB transport-specific fields from the core nct6694 structure
> into a new nct6694_usb_data structure. This decouples the shared MFD
> core from the USB transport layer, preparing the driver for potential
> support of alternative transport backends in the future.
> 
> The following fields are moved into nct6694_usb_data:
>   - access_lock
>   - int_in_urb
>   - udev
>   - usb_msg
>   - int_buffer
> 
> The core nct6694 structure now holds a void *priv pointer to reference

priv is a terrible name.  Please improve it.

Even 'transport_data' is better, but I'm sure you can improve on it still.

> the transport-specific data. USB-only definitions (vendor/product IDs,
> endpoint addresses, URB timeout, and the USB message union) are also
> moved from the shared header into the USB driver source file.
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>
> ---
> Changes in v7:
> 
> Changes in v6:
> 
> Changes in v5:
> - Split from the monolithic v4 patch to follow the single logical change
>   principle.
> 
>  drivers/mfd/nct6694.c       | 76 ++++++++++++++++++++++++++-----------
>  include/linux/mfd/nct6694.h | 27 ++++---------
>  2 files changed, 62 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
> index 308b2fda3055..58c1cbcbe3f2 100644
> --- a/drivers/mfd/nct6694.c
> +++ b/drivers/mfd/nct6694.c
> @@ -10,8 +10,8 @@
>   */
>  
>  #include <linux/bits.h>
> -#include <linux/interrupt.h>
>  #include <linux/idr.h>
> +#include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
>  #include <linux/kernel.h>
> @@ -22,6 +22,27 @@
>  #include <linux/spinlock.h>
>  #include <linux/usb.h>
>  
> +#define NCT6694_VENDOR_ID	0x0416
> +#define NCT6694_PRODUCT_ID	0x200B
> +#define NCT6694_INT_IN_EP	0x81
> +#define NCT6694_BULK_IN_EP	0x02
> +#define NCT6694_BULK_OUT_EP	0x03
> +
> +#define NCT6694_URB_TIMEOUT	1000
> +
> +union __packed nct6694_usb_msg {
> +	struct nct6694_cmd_header cmd_header;
> +	struct nct6694_response_header response_header;
> +};
> +
> +struct nct6694_usb_data {
> +	struct mutex access_lock;
> +	struct urb *int_in_urb;
> +	struct usb_device *udev;

'udev' is ambiguous in Linux IMHO.  Can you use 'usb_dev' instead?

> +	union nct6694_usb_msg *usb_msg;
> +	__le32 *int_buffer;
> +};
> +
>  static const struct mfd_cell nct6694_devs[] = {
>  	MFD_CELL_NAME("nct6694-gpio"),
>  	MFD_CELL_NAME("nct6694-gpio"),
> @@ -96,11 +117,12 @@ static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char
>   */
>  int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
>  {
> -	union nct6694_usb_msg *msg = nct6694->usb_msg;
> -	struct usb_device *udev = nct6694->udev;
> +	struct nct6694_usb_data *udata = nct6694->priv;

No one knows what udata is.  Be explicit, 'usb_data'?

Then we have; usb_dev, usb_msg and usb_data - which makes my OCD happy!

> +	union nct6694_usb_msg *msg = udata->usb_msg;
> +	struct usb_device *udev = udata->udev;
>  	int tx_len, rx_len, ret;
>  
> -	guard(mutex)(&nct6694->access_lock);
> +	guard(mutex)(&udata->access_lock);
>  
>  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
>  	msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
> @@ -146,11 +168,12 @@ EXPORT_SYMBOL_GPL(nct6694_read_msg);
>   */
>  int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
>  {
> -	union nct6694_usb_msg *msg = nct6694->usb_msg;
> -	struct usb_device *udev = nct6694->udev;
> +	struct nct6694_usb_data *udata = nct6694->priv;
> +	union nct6694_usb_msg *msg = udata->usb_msg;
> +	struct usb_device *udev = udata->udev;
>  	int tx_len, rx_len, ret;
>  
> -	guard(mutex)(&nct6694->access_lock);
> +	guard(mutex)(&udata->access_lock);
>  
>  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
>  	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> @@ -277,6 +300,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	struct usb_endpoint_descriptor *int_endpoint;
>  	struct usb_host_interface *interface;
>  	struct device *dev = &iface->dev;
> +	struct nct6694_usb_data *udata;
>  	struct nct6694 *nct6694;
>  	int ret;
>  
> @@ -284,18 +308,26 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	if (!nct6694)
>  		return -ENOMEM;
>  
> -	nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg), GFP_KERNEL);
> -	if (!nct6694->usb_msg)
> +	udata = devm_kzalloc(dev, sizeof(*udata), GFP_KERNEL);
> +	if (!udata)
> +		return -ENOMEM;
> +
> +	udata->usb_msg = devm_kzalloc(dev, sizeof(*udata->usb_msg), GFP_KERNEL);
> +	if (!udata->usb_msg)
>  		return -ENOMEM;
>  
> -	nct6694->int_buffer = devm_kzalloc(dev, sizeof(*nct6694->int_buffer), GFP_KERNEL);
> -	if (!nct6694->int_buffer)
> +	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
> +	if (!udata->int_buffer)
>  		return -ENOMEM;
>  
> -	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> -	if (!nct6694->int_in_urb)
> +	udata->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!udata->int_in_urb)
>  		return -ENOMEM;
>  
> +	udata->udev = udev;
> +
> +	nct6694->priv = udata;
> +
>  	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
>  						   &nct6694_irq_domain_ops,
>  						   nct6694);
> @@ -305,7 +337,6 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	}
>  
>  	nct6694->dev = dev;
> -	nct6694->udev = udev;
>  
>  	ida_init(&nct6694->gpio_ida);
>  	ida_init(&nct6694->i2c_ida);
> @@ -314,7 +345,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  
>  	spin_lock_init(&nct6694->irq_lock);
>  
> -	ret = devm_mutex_init(dev, &nct6694->access_lock);
> +	ret = devm_mutex_init(dev, &udata->access_lock);
>  	if (ret)
>  		goto err_ida;
>  
> @@ -326,11 +357,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  		goto err_ida;
>  	}
>  
> -	usb_fill_int_urb(nct6694->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> -			 nct6694->int_buffer, sizeof(*nct6694->int_buffer), usb_int_callback,
> +	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> +			 udata->int_buffer, sizeof(*udata->int_buffer), usb_int_callback,
>  			 nct6694, int_endpoint->bInterval);
>  
> -	ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL);
> +	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
>  	if (ret)
>  		goto err_ida;
>  
> @@ -343,7 +374,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	return 0;
>  
>  err_mfd:
> -	usb_kill_urb(nct6694->int_in_urb);
> +	usb_kill_urb(udata->int_in_urb);
>  err_ida:
>  	ida_destroy(&nct6694->wdt_ida);
>  	ida_destroy(&nct6694->canfd_ida);
> @@ -351,22 +382,23 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	ida_destroy(&nct6694->gpio_ida);
>  	irq_domain_remove(nct6694->domain);
>  err_urb:
> -	usb_free_urb(nct6694->int_in_urb);
> +	usb_free_urb(udata->int_in_urb);
>  	return ret;
>  }
>  
>  static void nct6694_usb_disconnect(struct usb_interface *iface)
>  {
>  	struct nct6694 *nct6694 = usb_get_intfdata(iface);
> +	struct nct6694_usb_data *udata = nct6694->priv;
>  
>  	mfd_remove_devices(nct6694->dev);
> -	usb_kill_urb(nct6694->int_in_urb);
> +	usb_kill_urb(udata->int_in_urb);
>  	ida_destroy(&nct6694->wdt_ida);
>  	ida_destroy(&nct6694->canfd_ida);
>  	ida_destroy(&nct6694->i2c_ida);
>  	ida_destroy(&nct6694->gpio_ida);
>  	irq_domain_remove(nct6694->domain);
> -	usb_free_urb(nct6694->int_in_urb);
> +	usb_free_urb(udata->int_in_urb);
>  }
>  
>  static const struct usb_device_id nct6694_ids[] = {
> diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> index 3c683e317aa3..3f5dd53f38de 100644
> --- a/include/linux/mfd/nct6694.h
> +++ b/include/linux/mfd/nct6694.h
> @@ -2,12 +2,18 @@
>  /*
>   * Copyright (C) 2025 Nuvoton Technology Corp.
>   *
> - * Nuvoton NCT6694 USB transaction and data structure.
> + * Nuvoton NCT6694 core definitions shared by all transport drivers
> + * and sub-device drivers.
>   */
>  
>  #ifndef __MFD_NCT6694_H
>  #define __MFD_NCT6694_H
>  
> +#include <linux/idr.h>
> +#include <linux/mutex.h>
> +#include <linux/spinlock.h>
> +#include <linux/types.h>
> +
>  #define NCT6694_HWMON_MOD	0x00
>  #define NCT6694_PWM_MOD		0x01
>  #define NCT6694_I2C_MOD		0x03
> @@ -17,17 +23,9 @@
>  #define NCT6694_RPT_MOD		0xFF
>  #define NCT6694_GPIO_MOD	NCT6694_RPT_MOD
>  
> -#define NCT6694_VENDOR_ID	0x0416
> -#define NCT6694_PRODUCT_ID	0x200B
> -#define NCT6694_INT_IN_EP	0x81
> -#define NCT6694_BULK_IN_EP	0x02
> -#define NCT6694_BULK_OUT_EP	0x03
> -
>  #define NCT6694_HCTRL_SET	0x40
>  #define NCT6694_HCTRL_GET	0x80
>  
> -#define NCT6694_URB_TIMEOUT	1000
> -
>  enum nct6694_irq_id {
>  	NCT6694_IRQ_GPIO0 = 0,
>  	NCT6694_IRQ_GPIO1,
> @@ -84,11 +82,6 @@ struct __packed nct6694_response_header {
>  	__le16 len;
>  };
>  
> -union __packed nct6694_usb_msg {
> -	struct nct6694_cmd_header cmd_header;
> -	struct nct6694_response_header response_header;
> -};
> -
>  struct nct6694 {
>  	struct device *dev;
>  	struct ida gpio_ida;
> @@ -96,13 +89,9 @@ struct nct6694 {
>  	struct ida canfd_ida;
>  	struct ida wdt_ida;
>  	struct irq_domain *domain;
> -	struct mutex access_lock;
>  	spinlock_t irq_lock;
> -	struct urb *int_in_urb;
> -	struct usb_device *udev;
> -	union nct6694_usb_msg *usb_msg;
> -	__le32 *int_buffer;
>  	unsigned int irq_enable;
> +	void *priv;
>  };
>  
>  int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf);
> -- 
> 2.34.1
> 

-- 
Lee Jones

[PATCH v7 03/10] mfd: nct6694: Rename USB transport functions with _usb_ prefix

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Rename nct6694_{read,write,err_handling}_msg() to carry an `_usb_`
prefix, marking them as USB transport-specific. This prepares for
the core layer and HIF (eSPI) transport added in later patches.

Add transitional static inline wrappers nct6694_{read,write}_msg()
in the shared header so sub-device drivers remain untouched in
this commit; they will be replaced by the transport dispatch layer
in a subsequent patch.

No functional change.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:

Changes in v6:
- New patch replacing the v5 function-pointer transport abstraction:
  rename the exported I/O functions with an _usb_ prefix and add
  transitional inline nct6694_{read,write}_msg() wrappers in the shared
  header so sub-device drivers stay untouched in this commit.

 drivers/mfd/nct6694.c       | 40 ++++++++++++++++++++-----------------
 include/linux/mfd/nct6694.h | 22 ++++++++++++++++++--
 2 files changed, 42 insertions(+), 20 deletions(-)

diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c
index 58c1cbcbe3f2..7c6b986db7f7 100644
--- a/drivers/mfd/nct6694.c
+++ b/drivers/mfd/nct6694.c
@@ -43,7 +43,7 @@ struct nct6694_usb_data {
 	__le32 *int_buffer;
 };
 
-static const struct mfd_cell nct6694_devs[] = {
+static const struct mfd_cell nct6694_usb_devs[] = {
 	MFD_CELL_NAME("nct6694-gpio"),
 	MFD_CELL_NAME("nct6694-gpio"),
 	MFD_CELL_NAME("nct6694-gpio"),
@@ -79,7 +79,7 @@ static const struct mfd_cell nct6694_devs[] = {
 	MFD_CELL_NAME("nct6694-rtc"),
 };
 
-static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char err_status)
+static int nct6694_usb_err_handling(struct nct6694 *nct6694, unsigned char err_status)
 {
 	switch (err_status) {
 	case NCT6694_NO_ERROR:
@@ -104,7 +104,7 @@ static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char
 }
 
 /**
- * nct6694_read_msg() - Read message from NCT6694 device
+ * nct6694_usb_read_msg() - Read message from NCT6694 device
  * @nct6694: NCT6694 device pointer
  * @cmd_hd: command header structure
  * @buf: buffer to store the response data
@@ -115,7 +115,9 @@ static int nct6694_response_err_handling(struct nct6694 *nct6694, unsigned char
  *
  * Return: Negative value on error or 0 on success.
  */
-int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
+int nct6694_usb_read_msg(struct nct6694 *nct6694,
+			 const struct nct6694_cmd_header *cmd_hd,
+			 void *buf)
 {
 	struct nct6694_usb_data *udata = nct6694->priv;
 	union nct6694_usb_msg *msg = udata->usb_msg;
@@ -151,12 +153,12 @@ int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *c
 		return -EIO;
 	}
 
-	return nct6694_response_err_handling(nct6694, msg->response_header.sts);
+	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
 }
-EXPORT_SYMBOL_GPL(nct6694_read_msg);
+EXPORT_SYMBOL_GPL(nct6694_usb_read_msg);
 
 /**
- * nct6694_write_msg() - Write message to NCT6694 device
+ * nct6694_usb_write_msg() - Write message to NCT6694 device
  * @nct6694: NCT6694 device pointer
  * @cmd_hd: command header structure
  * @buf: buffer containing the data to be sent
@@ -166,7 +168,9 @@ EXPORT_SYMBOL_GPL(nct6694_read_msg);
  *
  * Return: Negative value on error or 0 on success.
  */
-int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf)
+int nct6694_usb_write_msg(struct nct6694 *nct6694,
+			  const struct nct6694_cmd_header *cmd_hd,
+			  void *buf)
 {
 	struct nct6694_usb_data *udata = nct6694->priv;
 	union nct6694_usb_msg *msg = udata->usb_msg;
@@ -208,11 +212,11 @@ int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *
 		return -EIO;
 	}
 
-	return nct6694_response_err_handling(nct6694, msg->response_header.sts);
+	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
 }
-EXPORT_SYMBOL_GPL(nct6694_write_msg);
+EXPORT_SYMBOL_GPL(nct6694_usb_write_msg);
 
-static void usb_int_callback(struct urb *urb)
+static void nct6694_usb_int_callback(struct urb *urb)
 {
 	struct nct6694 *nct6694 = urb->context;
 	__le32 *status_le = urb->transfer_buffer;
@@ -358,7 +362,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	}
 
 	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
-			 udata->int_buffer, sizeof(*udata->int_buffer), usb_int_callback,
+			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
 			 nct6694, int_endpoint->bInterval);
 
 	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
@@ -367,7 +371,7 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 
 	usb_set_intfdata(iface, nct6694);
 
-	ret = mfd_add_hotplug_devices(dev, nct6694_devs, ARRAY_SIZE(nct6694_devs));
+	ret = mfd_add_hotplug_devices(dev, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
 	if (ret)
 		goto err_mfd;
 
@@ -401,20 +405,20 @@ static void nct6694_usb_disconnect(struct usb_interface *iface)
 	usb_free_urb(udata->int_in_urb);
 }
 
-static const struct usb_device_id nct6694_ids[] = {
+static const struct usb_device_id nct6694_usb_ids[] = {
 	{ USB_DEVICE_AND_INTERFACE_INFO(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID, 0xFF, 0x00, 0x00) },
 	{ }
 };
-MODULE_DEVICE_TABLE(usb, nct6694_ids);
+MODULE_DEVICE_TABLE(usb, nct6694_usb_ids);
 
 static struct usb_driver nct6694_usb_driver = {
-	.name		= "nct6694",
-	.id_table	= nct6694_ids,
+	.name		= "nct6694-usb",
+	.id_table	= nct6694_usb_ids,
 	.probe		= nct6694_usb_probe,
 	.disconnect	= nct6694_usb_disconnect,
 };
 module_usb_driver(nct6694_usb_driver);
 
-MODULE_DESCRIPTION("Nuvoton NCT6694 core driver");
+MODULE_DESCRIPTION("Nuvoton NCT6694 USB transport driver");
 MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
 MODULE_LICENSE("GPL");
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
index 3f5dd53f38de..43b51f243e8e 100644
--- a/include/linux/mfd/nct6694.h
+++ b/include/linux/mfd/nct6694.h
@@ -94,7 +94,25 @@ struct nct6694 {
 	void *priv;
 };
 
-int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf);
-int nct6694_write_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf);
+int nct6694_usb_read_msg(struct nct6694 *nct6694,
+			 const struct nct6694_cmd_header *cmd_hd,
+			 void *buf);
+int nct6694_usb_write_msg(struct nct6694 *nct6694,
+			  const struct nct6694_cmd_header *cmd_hd,
+			  void *buf);
+
+static inline int nct6694_read_msg(struct nct6694 *nct6694,
+				   const struct nct6694_cmd_header *cmd_hd,
+				   void *buf)
+{
+	return nct6694_usb_read_msg(nct6694, cmd_hd, buf);
+}
+
+static inline int nct6694_write_msg(struct nct6694 *nct6694,
+				    const struct nct6694_cmd_header *cmd_hd,
+				    void *buf)
+{
+	return nct6694_usb_write_msg(nct6694, cmd_hd, buf);
+}
 
 #endif
-- 
2.34.1

Re: [PATCH v7 03/10] mfd: nct6694: Rename USB transport functions with _usb_ prefix

[sashiko-bot]

> From: Ming Yu <a0282524688@gmail.com>
> 
> Rename nct6694_{read,write,err_handling}_msg() to carry an `_usb_`
> prefix, marking them as USB transport-specific. This prepares for
> the core layer and HIF (eSPI) transport added in later patches.
> 
> Add transitional static inline wrappers nct6694_{read,write}_msg()
> in the shared header so sub-device drivers remain untouched in
> this commit; they will be replaced by the transport dispatch layer
> in a subsequent patch.
> 
> No functional change.
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=3

[PATCH v7 04/10] mfd: nct6694: Rename driver to nct6694-usb and update Kconfig

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Rename nct6694.c to nct6694-usb.c to accurately reflect that it
implements the USB transport backend.

Additionally, introduce a new MFD_NCT6694_USB Kconfig option and convert
the existing MFD_NCT6694 into a hidden core symbol. The core symbol is
now automatically selected by the transport drivers.

This Kconfig and naming restructure aligns with standard MFD transport
abstraction practices, paving the way for future interfaces (e.g., HIF)
to be seamlessly integrated.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:

Changes in v6:

Changes in v5:
- Split from the monolithic v4 patch to follow the single logical change
  principle.

 MAINTAINERS                              |  2 +-
 drivers/mfd/Kconfig                      | 31 ++++++++++++++++--------
 drivers/mfd/Makefile                     |  2 +-
 drivers/mfd/{nct6694.c => nct6694-usb.c} |  0
 4 files changed, 23 insertions(+), 12 deletions(-)
 rename drivers/mfd/{nct6694.c => nct6694-usb.c} (100%)

diff --git a/MAINTAINERS b/MAINTAINERS
index 040df7bfb24f..89d7339f3613 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19356,7 +19356,7 @@ S:	Supported
 F:	drivers/gpio/gpio-nct6694.c
 F:	drivers/hwmon/nct6694-hwmon.c
 F:	drivers/i2c/busses/i2c-nct6694.c
-F:	drivers/mfd/nct6694.c
+F:	drivers/mfd/nct6694-usb.c
 F:	drivers/net/can/usb/nct6694_canfd.c
 F:	drivers/rtc/rtc-nct6694.c
 F:	drivers/watchdog/nct6694_wdt.c
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 763ce6a34782..5506a0adf3ec 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1164,19 +1164,30 @@ config MFD_MENF21BMC
 	  will be called menf21bmc.
 
 config MFD_NCT6694
-	tristate "Nuvoton NCT6694 support"
+	tristate
 	select MFD_CORE
+	help
+	  Core MFD support for the Nuvoton NCT6694 peripheral expander.
+	  This provides the common APIs and shared structures used by all
+	  interfaces (USB, HIF) to access the NCT6694 hardware features
+	  such as GPIO, I2C, CAN-FD, Watchdog, ADC, PWM, and RTC.
+
+	  It is selected automatically by the transport interface drivers.
+
+config MFD_NCT6694_USB
+	tristate "Nuvoton NCT6694 USB interface support"
+	select MFD_NCT6694
 	depends on USB
 	help
-	  This enables support for the Nuvoton USB device NCT6694, which shares
-	  peripherals.
-	  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 provides core APIs to access the NCT6694 hardware
-	  monitoring and control features.
-	  Additional drivers must be enabled to utilize the specific
-	  functionalities of the device.
+	  This enables support for the Nuvoton NCT6694 peripheral expander
+	  connected via the USB interface.
+
+	  The transport driver uses USB bulk and interrupt transfers to
+	  communicate with the NCT6694 firmware. Enable this option if you
+	  are using the NCT6694 via a USB connection.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called nct6694-usb.
 
 config MFD_OCELOT
 	tristate "Microsemi Ocelot External Control Support"
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index dd4bb7e77c33..48caac64f3d8 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -124,7 +124,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 
 obj-$(CONFIG_MFD_PF1550)	+= pf1550.o
 
-obj-$(CONFIG_MFD_NCT6694)	+= nct6694.o
+obj-$(CONFIG_MFD_NCT6694_USB)	+= nct6694-usb.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
 
diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694-usb.c
similarity index 100%
rename from drivers/mfd/nct6694.c
rename to drivers/mfd/nct6694-usb.c
-- 
2.34.1

Re: [PATCH v7 04/10] mfd: nct6694: Rename driver to nct6694-usb and update Kconfig

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Renaming the user-visible Kconfig symbol `MFD_NCT6694` to `MFD_NCT6694_USB` without a migration path silently drops the driver from existing kernel configurations.
--

--- Patch [4]: [PATCH v7 04/10] mfd: nct6694: Rename driver to nct6694-usb and update Kconfig ---
commit 215b54ed65ccbd40b44ec465fcadf580136e0e2d
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Rename driver to nct6694-usb and update Kconfig

This commit renames the nct6694 driver to nct6694-usb to reflect its USB
transport backend. It also introduces the MFD_NCT6694_USB option and hides
the original MFD_NCT6694 config, setting it up to be selected by transport
drivers.

> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782b..5506a0adf3ec0 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1164,19 +1164,30 @@ config MFD_MENF21BMC
>  	  will be called menf21bmc.
>  
>  config MFD_NCT6694
> -	tristate "Nuvoton NCT6694 support"
> +	tristate
>  	select MFD_CORE

[Severity: Medium]
Will users upgrading their kernel with an existing .config have the NCT6694
driver silently disabled?

The original symbol loses its prompt here, making it hidden. Because the new
MFD_NCT6694_USB symbol has no default fallback or migration mechanism, it looks
like running `make olddefconfig` with CONFIG_MFD_NCT6694=y will drop the
driver entirely.

Does it make sense to add a default property to MFD_NCT6694_USB to migrate the
previous configuration?

> +	help
> +	  Core MFD support for the Nuvoton NCT6694 peripheral expander.

[ ... ]

> +config MFD_NCT6694_USB
> +	tristate "Nuvoton NCT6694 USB interface support"
> +	select MFD_NCT6694
>  	depends on USB

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=4

Re: [PATCH v7 04/10] mfd: nct6694: Rename driver to nct6694-usb and update Kconfig

[Lee Jones]

On Fri, 21 Aug 2026, a0282524688@gmail.com wrote:

> From: Ming Yu <a0282524688@gmail.com>
> 
> Rename nct6694.c to nct6694-usb.c to accurately reflect that it
> implements the USB transport backend.
> 
> Additionally, introduce a new MFD_NCT6694_USB Kconfig option and convert
> the existing MFD_NCT6694 into a hidden core symbol. The core symbol is
> now automatically selected by the transport drivers.
> 
> This Kconfig and naming restructure aligns with standard MFD transport
> abstraction practices, paving the way for future interfaces (e.g., HIF)
> to be seamlessly integrated.
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>
> ---
> Changes in v7:

No changes or you forgot to populate it?

> Changes in v6:

> - Split from the monolithic v4 patch to follow the single logical change
>   principle.
> 
>  MAINTAINERS                              |  2 +-
>  drivers/mfd/Kconfig                      | 31 ++++++++++++++++--------
>  drivers/mfd/Makefile                     |  2 +-
>  drivers/mfd/{nct6694.c => nct6694-usb.c} |  0
>  4 files changed, 23 insertions(+), 12 deletions(-)
>  rename drivers/mfd/{nct6694.c => nct6694-usb.c} (100%)
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 040df7bfb24f..89d7339f3613 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19356,7 +19356,7 @@ S:	Supported
>  F:	drivers/gpio/gpio-nct6694.c
>  F:	drivers/hwmon/nct6694-hwmon.c
>  F:	drivers/i2c/busses/i2c-nct6694.c
> -F:	drivers/mfd/nct6694.c
> +F:	drivers/mfd/nct6694-usb.c
>  F:	drivers/net/can/usb/nct6694_canfd.c
>  F:	drivers/rtc/rtc-nct6694.c
>  F:	drivers/watchdog/nct6694_wdt.c
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 763ce6a34782..5506a0adf3ec 100644
> --- "a/drivers/mfd/Kconfig"
> +++ "b/drivers/mfd/Kconfig"
> @@ -1164,19 +1164,30 @@ config MFD_MENF21BMC
>  	  will be called menf21bmc.
>  
>  config MFD_NCT6694

So what does this do now?

Will there be another core symbol soon?

> -	tristate "Nuvoton NCT6694 support"
> +	tristate
>  	select MFD_CORE
> +	help
> +	  Core MFD support for the Nuvoton NCT6694 peripheral expander.
> +	  This provides the common APIs and shared structures used by all
> +	  interfaces (USB, HIF) to access the NCT6694 hardware features
> +	  such as GPIO, I2C, CAN-FD, Watchdog, ADC, PWM, and RTC.
> +
> +	  It is selected automatically by the transport interface drivers.
> +
> +config MFD_NCT6694_USB
> +	tristate "Nuvoton NCT6694 USB interface support"
> +	select MFD_NCT6694
>  	depends on USB
>  	help
> -	  This enables support for the Nuvoton USB device NCT6694, which shares
> -	  peripherals.
> -	  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 provides core APIs to access the NCT6694 hardware
> -	  monitoring and control features.
> -	  Additional drivers must be enabled to utilize the specific
> -	  functionalities of the device.
> +	  This enables support for the Nuvoton NCT6694 peripheral expander
> +	  connected via the USB interface.
> +
> +	  The transport driver uses USB bulk and interrupt transfers to
> +	  communicate with the NCT6694 firmware. Enable this option if you
> +	  are using the NCT6694 via a USB connection.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called nct6694-usb.
>  
>  config MFD_OCELOT
>  	tristate "Microsemi Ocelot External Control Support"
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index dd4bb7e77c33..48caac64f3d8 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -124,7 +124,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
>  
>  obj-$(CONFIG_MFD_PF1550)	+= pf1550.o
>  
> -obj-$(CONFIG_MFD_NCT6694)	+= nct6694.o
> +obj-$(CONFIG_MFD_NCT6694_USB)	+= nct6694-usb.o
>  
>  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
>  
> diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694-usb.c
> similarity index 100%
> rename from drivers/mfd/nct6694.c
> rename to drivers/mfd/nct6694-usb.c
> -- 
> 2.34.1
> 

-- 
Lee Jones

Re: [PATCH v7 04/10] mfd: nct6694: Rename driver to nct6694-usb and update Kconfig

[Ming Yu]

Hi Lee,

Thank you for reviewing.

Lee Jones <lee@kernel.org> 於 2026年9月3日週四 下午6:24寫道：
>
> On Fri, 21 Aug 2026, a0282524688@gmail.com wrote:
>
> > From: Ming Yu <a0282524688@gmail.com>
> >
> > Rename nct6694.c to nct6694-usb.c to accurately reflect that it
> > implements the USB transport backend.
> >
> > Additionally, introduce a new MFD_NCT6694_USB Kconfig option and convert
> > the existing MFD_NCT6694 into a hidden core symbol. The core symbol is
> > now automatically selected by the transport drivers.
> >
> > This Kconfig and naming restructure aligns with standard MFD transport
> > abstraction practices, paving the way for future interfaces (e.g., HIF)
> > to be seamlessly integrated.
> >
> > Signed-off-by: Ming Yu <a0282524688@gmail.com>
> > ---
> > Changes in v7:
>
> No changes or you forgot to populate it?
>

There is no functional change in v7 for this patch; I'll mark it
explicitly as "No changes." in v8.

> > Changes in v6:
>
...
> > --- "a/drivers/mfd/Kconfig"
> > +++ "b/drivers/mfd/Kconfig"
> > @@ -1164,19 +1164,30 @@ config MFD_MENF21BMC
> >         will be called menf21bmc.
> >
> >  config MFD_NCT6694
>
> So what does this do now?
>
> Will there be another core symbol soon?
>

MFD_NCT6694 is now a hidden symbol that builds only the
transport-agnostic core (nct6694-core.o, added in 5/10): the IRQ
domain, IDA pools and MFD sub-device registration shared by every
backend. It has no prompt because it is not useful on its own, so it
is  selected automatically by whichever transport driver is enabled.

There won't be a second core symbol. The next addition is another
*transport*, MFD_NCT6694_HIF (eSPI, 10/10), which likewise selects
MFD_NCT6694. So the final shape is a single shared core selected by
one  or more transports:
    MFD_NCT6694 (hidden core)                 -> nct6694-core.o
        ^ select
    MFD_NCT6694_USB (USB transport)    -> nct6694-usb.o
    MFD_NCT6694_HIF (eSPI transport)     -> nct6694-hif.o   [10/10]

I'll clarify this in the commit message for v8.

> > -     tristate "Nuvoton NCT6694 support"
> > +     tristate
> >       select MFD_CORE
> > +     help
> > +       Core MFD support for the Nuvoton NCT6694 peripheral expander.
> > +       This provides the common APIs and shared structures used by all
> > +       interfaces (USB, HIF) to access the NCT6694 hardware features
> > +       such as GPIO, I2C, CAN-FD, Watchdog, ADC, PWM, and RTC.
> > +
...
>
> --
> Lee Jones

Thanks,
Ming

[PATCH v7 05/10] mfd: nct6694: Extract core device management into a separate module

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Extract the transport-agnostic core logic, including IRQ domain setup,
IDA initialization, and MFD sub-device registration, from the USB driver
into a new nct6694-core.c module.

The core routines are exported as nct6694_core_probe() and
nct6694_core_remove() to be consumed by the transport drivers. The USB
driver is updated to pass its specific MFD cells to the core probe
routine.

This completes the transport abstraction, ensuring that the shared
device management logic is cleanly separated from the underlying I/O
implementation, and is fully ready for new transport backends
(e.g., HIF).

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:

Changes in v6:
- Reordered to sit before the transport-abstraction patch; no functional
  change.

Changes in v5:
- Split from the monolithic v4 patch to follow the single logical change
  principle.

 MAINTAINERS                 |   2 +-
 drivers/mfd/Makefile        |   1 +
 drivers/mfd/nct6694-core.c  | 136 ++++++++++++++++++++++++++++++++++++
 drivers/mfd/nct6694-usb.c   |  91 ++----------------------
 include/linux/mfd/nct6694.h |   9 ++-
 5 files changed, 153 insertions(+), 86 deletions(-)
 create mode 100644 drivers/mfd/nct6694-core.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 89d7339f3613..f1894e88ba62 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -19356,7 +19356,7 @@ S:	Supported
 F:	drivers/gpio/gpio-nct6694.c
 F:	drivers/hwmon/nct6694-hwmon.c
 F:	drivers/i2c/busses/i2c-nct6694.c
-F:	drivers/mfd/nct6694-usb.c
+F:	drivers/mfd/nct6694-*.c
 F:	drivers/net/can/usb/nct6694_canfd.c
 F:	drivers/rtc/rtc-nct6694.c
 F:	drivers/watchdog/nct6694_wdt.c
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 48caac64f3d8..10c19a19541e 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -124,6 +124,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 
 obj-$(CONFIG_MFD_PF1550)	+= pf1550.o
 
+obj-$(CONFIG_MFD_NCT6694)	+= nct6694-core.o
 obj-$(CONFIG_MFD_NCT6694_USB)	+= nct6694-usb.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
diff --git a/drivers/mfd/nct6694-core.c b/drivers/mfd/nct6694-core.c
new file mode 100644
index 000000000000..36dfaa2e327f
--- /dev/null
+++ b/drivers/mfd/nct6694-core.c
@@ -0,0 +1,136 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2026 Nuvoton Technology Corp.
+ *
+ * Nuvoton NCT6694 MFD core driver.
+ *
+ * This provides common registration for IRQ domain, IDA pools,
+ * and MFD sub-devices shared by all transport drivers.
+ */
+
+#include <linux/idr.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/nct6694.h>
+#include <linux/module.h>
+#include <linux/spinlock.h>
+
+static void nct6694_irq_enable(struct irq_data *data)
+{
+	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = irqd_to_hwirq(data);
+
+	guard(spinlock_irqsave)(&nct6694->irq_lock);
+
+	nct6694->irq_enable |= BIT(hwirq);
+}
+
+static void nct6694_irq_disable(struct irq_data *data)
+{
+	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
+	irq_hw_number_t hwirq = irqd_to_hwirq(data);
+
+	guard(spinlock_irqsave)(&nct6694->irq_lock);
+
+	nct6694->irq_enable &= ~BIT(hwirq);
+}
+
+static const struct irq_chip nct6694_irq_chip = {
+	.name = "nct6694-irq",
+	.flags = IRQCHIP_SKIP_SET_WAKE,
+	.irq_enable = nct6694_irq_enable,
+	.irq_disable = nct6694_irq_disable,
+};
+
+static int nct6694_irq_domain_map(struct irq_domain *d, unsigned int irq,
+				  irq_hw_number_t hw)
+{
+	struct nct6694 *nct6694 = d->host_data;
+
+	irq_set_chip_data(irq, nct6694);
+	irq_set_chip_and_handler(irq, &nct6694_irq_chip, handle_simple_irq);
+
+	return 0;
+}
+
+static void nct6694_irq_domain_unmap(struct irq_domain *d, unsigned int irq)
+{
+	irq_set_chip_and_handler(irq, NULL, NULL);
+	irq_set_chip_data(irq, NULL);
+}
+
+static const struct irq_domain_ops nct6694_irq_domain_ops = {
+	.map	= nct6694_irq_domain_map,
+	.unmap	= nct6694_irq_domain_unmap,
+};
+
+/**
+ * nct6694_core_probe() - Register IRQ domain, IDAs, and MFD sub-devices
+ * @dev: parent device (USB interface or platform device)
+ * @nct6694: initialized nct6694 structure with its transport regmap set
+ *
+ * This function completes the common probe steps shared by all transport
+ * drivers: IRQ domain creation, IDA initialization, and MFD cell registration.
+ *
+ * The caller must have already set nct6694->dev, nct6694->priv, and
+ * nct6694->regmap before calling this.
+ *
+ * Return: 0 on success or negative errno on failure.
+ */
+int nct6694_core_probe(struct device *dev, struct nct6694 *nct6694,
+		       const struct mfd_cell *cells, int n_cells)
+{
+	int ret;
+
+	spin_lock_init(&nct6694->irq_lock);
+
+	ida_init(&nct6694->gpio_ida);
+	ida_init(&nct6694->i2c_ida);
+	ida_init(&nct6694->canfd_ida);
+	ida_init(&nct6694->wdt_ida);
+
+	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
+						   &nct6694_irq_domain_ops,
+						   nct6694);
+	if (!nct6694->domain) {
+		ret = -ENODEV;
+		goto err_ida;
+	}
+
+	ret = mfd_add_hotplug_devices(dev, cells, n_cells);
+	if (ret)
+		goto err_domain;
+
+	return 0;
+
+err_domain:
+	irq_domain_remove(nct6694->domain);
+err_ida:
+	ida_destroy(&nct6694->wdt_ida);
+	ida_destroy(&nct6694->canfd_ida);
+	ida_destroy(&nct6694->i2c_ida);
+	ida_destroy(&nct6694->gpio_ida);
+	return ret;
+}
+EXPORT_SYMBOL_GPL(nct6694_core_probe);
+
+/**
+ * nct6694_core_remove() - Unregister MFD sub-devices and free core resources
+ * @nct6694: nct6694 structure previously passed to nct6694_core_probe()
+ */
+void nct6694_core_remove(struct nct6694 *nct6694)
+{
+	mfd_remove_devices(nct6694->dev);
+	irq_domain_remove(nct6694->domain);
+	ida_destroy(&nct6694->wdt_ida);
+	ida_destroy(&nct6694->canfd_ida);
+	ida_destroy(&nct6694->i2c_ida);
+	ida_destroy(&nct6694->gpio_ida);
+}
+EXPORT_SYMBOL_GPL(nct6694_core_remove);
+
+MODULE_DESCRIPTION("Nuvoton NCT6694 MFD core driver");
+MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
index 7c6b986db7f7..2289ebfde7fa 100644
--- a/drivers/mfd/nct6694-usb.c
+++ b/drivers/mfd/nct6694-usb.c
@@ -10,7 +10,6 @@
  */
 
 #include <linux/bits.h>
-#include <linux/idr.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
@@ -19,7 +18,6 @@
 #include <linux/mfd/nct6694.h>
 #include <linux/module.h>
 #include <linux/slab.h>
-#include <linux/spinlock.h>
 #include <linux/usb.h>
 
 #define NCT6694_VENDOR_ID	0x0416
@@ -246,57 +244,9 @@ static void nct6694_usb_int_callback(struct urb *urb)
 resubmit:
 	ret = usb_submit_urb(urb, GFP_ATOMIC);
 	if (ret)
-		dev_warn(nct6694->dev, "Failed to resubmit urb, status %pe",  ERR_PTR(ret));
+		dev_warn(nct6694->dev, "Failed to resubmit urb, status %pe", ERR_PTR(ret));
 }
 
-static void nct6694_irq_enable(struct irq_data *data)
-{
-	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
-	irq_hw_number_t hwirq = irqd_to_hwirq(data);
-
-	guard(spinlock_irqsave)(&nct6694->irq_lock);
-
-	nct6694->irq_enable |= BIT(hwirq);
-}
-
-static void nct6694_irq_disable(struct irq_data *data)
-{
-	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
-	irq_hw_number_t hwirq = irqd_to_hwirq(data);
-
-	guard(spinlock_irqsave)(&nct6694->irq_lock);
-
-	nct6694->irq_enable &= ~BIT(hwirq);
-}
-
-static const struct irq_chip nct6694_irq_chip = {
-	.name = "nct6694-irq",
-	.flags = IRQCHIP_SKIP_SET_WAKE,
-	.irq_enable = nct6694_irq_enable,
-	.irq_disable = nct6694_irq_disable,
-};
-
-static int nct6694_irq_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
-{
-	struct nct6694 *nct6694 = d->host_data;
-
-	irq_set_chip_data(irq, nct6694);
-	irq_set_chip_and_handler(irq, &nct6694_irq_chip, handle_simple_irq);
-
-	return 0;
-}
-
-static void nct6694_irq_domain_unmap(struct irq_domain *d, unsigned int irq)
-{
-	irq_set_chip_and_handler(irq, NULL, NULL);
-	irq_set_chip_data(irq, NULL);
-}
-
-static const struct irq_domain_ops nct6694_irq_domain_ops = {
-	.map	= nct6694_irq_domain_map,
-	.unmap	= nct6694_irq_domain_unmap,
-};
-
 static int nct6694_usb_probe(struct usb_interface *iface,
 			     const struct usb_device_id *id)
 {
@@ -330,35 +280,19 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 
 	udata->udev = udev;
 
-	nct6694->priv = udata;
-
-	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
-						   &nct6694_irq_domain_ops,
-						   nct6694);
-	if (!nct6694->domain) {
-		ret = -ENODEV;
-		goto err_urb;
-	}
-
 	nct6694->dev = dev;
-
-	ida_init(&nct6694->gpio_ida);
-	ida_init(&nct6694->i2c_ida);
-	ida_init(&nct6694->canfd_ida);
-	ida_init(&nct6694->wdt_ida);
-
-	spin_lock_init(&nct6694->irq_lock);
+	nct6694->priv = udata;
 
 	ret = devm_mutex_init(dev, &udata->access_lock);
 	if (ret)
-		goto err_ida;
+		goto err_urb;
 
 	interface = iface->cur_altsetting;
 
 	int_endpoint = &interface->endpoint[0].desc;
 	if (!usb_endpoint_is_int_in(int_endpoint)) {
 		ret = -ENODEV;
-		goto err_ida;
+		goto err_urb;
 	}
 
 	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
@@ -367,11 +301,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 
 	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
 	if (ret)
-		goto err_ida;
+		goto err_urb;
 
 	usb_set_intfdata(iface, nct6694);
 
-	ret = mfd_add_hotplug_devices(dev, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
+	ret = nct6694_core_probe(dev, nct6694, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
 	if (ret)
 		goto err_mfd;
 
@@ -379,12 +313,6 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 
 err_mfd:
 	usb_kill_urb(udata->int_in_urb);
-err_ida:
-	ida_destroy(&nct6694->wdt_ida);
-	ida_destroy(&nct6694->canfd_ida);
-	ida_destroy(&nct6694->i2c_ida);
-	ida_destroy(&nct6694->gpio_ida);
-	irq_domain_remove(nct6694->domain);
 err_urb:
 	usb_free_urb(udata->int_in_urb);
 	return ret;
@@ -395,13 +323,8 @@ static void nct6694_usb_disconnect(struct usb_interface *iface)
 	struct nct6694 *nct6694 = usb_get_intfdata(iface);
 	struct nct6694_usb_data *udata = nct6694->priv;
 
-	mfd_remove_devices(nct6694->dev);
+	nct6694_core_remove(nct6694);
 	usb_kill_urb(udata->int_in_urb);
-	ida_destroy(&nct6694->wdt_ida);
-	ida_destroy(&nct6694->canfd_ida);
-	ida_destroy(&nct6694->i2c_ida);
-	ida_destroy(&nct6694->gpio_ida);
-	irq_domain_remove(nct6694->domain);
 	usb_free_urb(udata->int_in_urb);
 }
 
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
index 43b51f243e8e..853b1530755d 100644
--- a/include/linux/mfd/nct6694.h
+++ b/include/linux/mfd/nct6694.h
@@ -10,10 +10,13 @@
 #define __MFD_NCT6694_H
 
 #include <linux/idr.h>
-#include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
+struct device;
+struct irq_domain;
+struct mfd_cell;
+
 #define NCT6694_HWMON_MOD	0x00
 #define NCT6694_PWM_MOD		0x01
 #define NCT6694_I2C_MOD		0x03
@@ -94,6 +97,10 @@ struct nct6694 {
 	void *priv;
 };
 
+int nct6694_core_probe(struct device *dev, struct nct6694 *nct6694,
+		       const struct mfd_cell *cells, int n_cells);
+void nct6694_core_remove(struct nct6694 *nct6694);
+
 int nct6694_usb_read_msg(struct nct6694 *nct6694,
 			 const struct nct6694_cmd_header *cmd_hd,
 			 void *buf);
-- 
2.34.1

Re: [PATCH v7 05/10] mfd: nct6694: Extract core device management into a separate module

[sashiko-bot]

> From: Ming Yu <a0282524688@gmail.com>
> 
> Extract the transport-agnostic core logic, including IRQ domain setup,
> IDA initialization, and MFD sub-device registration, from the USB driver
> into a new nct6694-core.c module.
> 
> The core routines are exported as nct6694_core_probe() and
> nct6694_core_remove() to be consumed by the transport drivers. The USB
> driver is updated to pass its specific MFD cells to the core probe
> routine.
> 
> This completes the transport abstraction, ensuring that the shared
> device management logic is cleanly separated from the underlying I/O
> implementation, and is fully ready for new transport backends
> (e.g., HIF).
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=5

Re: [PATCH v7 05/10] mfd: nct6694: Extract core device management into a separate module

[Lee Jones]

On Fri, 21 Aug 2026, a0282524688@gmail.com wrote:

> From: Ming Yu <a0282524688@gmail.com>
> 
> Extract the transport-agnostic core logic, including IRQ domain setup,
> IDA initialization, and MFD sub-device registration, from the USB driver
> into a new nct6694-core.c module.
> 
> The core routines are exported as nct6694_core_probe() and
> nct6694_core_remove() to be consumed by the transport drivers. The USB
> driver is updated to pass its specific MFD cells to the core probe
> routine.
> 
> This completes the transport abstraction, ensuring that the shared
> device management logic is cleanly separated from the underlying I/O
> implementation, and is fully ready for new transport backends
> (e.g., HIF).
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>
> ---
> Changes in v7:
> 
> Changes in v6:
> - Reordered to sit before the transport-abstraction patch; no functional
>   change.
> 
> Changes in v5:
> - Split from the monolithic v4 patch to follow the single logical change
>   principle.
> 
>  MAINTAINERS                 |   2 +-
>  drivers/mfd/Makefile        |   1 +
>  drivers/mfd/nct6694-core.c  | 136 ++++++++++++++++++++++++++++++++++++
>  drivers/mfd/nct6694-usb.c   |  91 ++----------------------
>  include/linux/mfd/nct6694.h |   9 ++-
>  5 files changed, 153 insertions(+), 86 deletions(-)
>  create mode 100644 drivers/mfd/nct6694-core.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 89d7339f3613..f1894e88ba62 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -19356,7 +19356,7 @@ S:	Supported
>  F:	drivers/gpio/gpio-nct6694.c
>  F:	drivers/hwmon/nct6694-hwmon.c
>  F:	drivers/i2c/busses/i2c-nct6694.c
> -F:	drivers/mfd/nct6694-usb.c
> +F:	drivers/mfd/nct6694-*.c
>  F:	drivers/net/can/usb/nct6694_canfd.c
>  F:	drivers/rtc/rtc-nct6694.c
>  F:	drivers/watchdog/nct6694_wdt.c
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index 48caac64f3d8..10c19a19541e 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -124,6 +124,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
>  
>  obj-$(CONFIG_MFD_PF1550)	+= pf1550.o
>  
> +obj-$(CONFIG_MFD_NCT6694)	+= nct6694-core.o
>  obj-$(CONFIG_MFD_NCT6694_USB)	+= nct6694-usb.o
>  
>  obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
> diff --git a/drivers/mfd/nct6694-core.c b/drivers/mfd/nct6694-core.c
> new file mode 100644
> index 000000000000..36dfaa2e327f
> --- /dev/null
> +++ b/drivers/mfd/nct6694-core.c
> @@ -0,0 +1,136 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2026 Nuvoton Technology Corp.
> + *
> + * Nuvoton NCT6694 MFD core driver.
> + *
> + * This provides common registration for IRQ domain, IDA pools,
> + * and MFD sub-devices shared by all transport drivers.
> + */
> +
> +#include <linux/idr.h>
> +#include <linux/irq.h>
> +#include <linux/irqdomain.h>
> +#include <linux/kernel.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/nct6694.h>
> +#include <linux/module.h>
> +#include <linux/spinlock.h>
> +
> +static void nct6694_irq_enable(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(data);
> +
> +	guard(spinlock_irqsave)(&nct6694->irq_lock);
> +
> +	nct6694->irq_enable |= BIT(hwirq);
> +}
> +
> +static void nct6694_irq_disable(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +
> +	guard(spinlock_irqsave)(&nct6694->irq_lock);
> +
> +	nct6694->irq_enable &= ~BIT(hwirq);
> +}
> +
> +static const struct irq_chip nct6694_irq_chip = {
> +	.name = "nct6694-irq",
> +	.flags = IRQCHIP_SKIP_SET_WAKE,
> +	.irq_enable = nct6694_irq_enable,
> +	.irq_disable = nct6694_irq_disable,
> +};
> +
> +static int nct6694_irq_domain_map(struct irq_domain *d, unsigned int irq,
> +				  irq_hw_number_t hw)
> +{
> +	struct nct6694 *nct6694 = d->host_data;
> +
> +	irq_set_chip_data(irq, nct6694);
> +	irq_set_chip_and_handler(irq, &nct6694_irq_chip, handle_simple_irq);
> +
> +	return 0;
> +}
> +
> +static void nct6694_irq_domain_unmap(struct irq_domain *d, unsigned int irq)
> +{
> +	irq_set_chip_and_handler(irq, NULL, NULL);
> +	irq_set_chip_data(irq, NULL);
> +}
> +
> +static const struct irq_domain_ops nct6694_irq_domain_ops = {
> +	.map	= nct6694_irq_domain_map,
> +	.unmap	= nct6694_irq_domain_unmap,
> +};
> +
> +/**
> + * nct6694_core_probe() - Register IRQ domain, IDAs, and MFD sub-devices
> + * @dev: parent device (USB interface or platform device)
> + * @nct6694: initialized nct6694 structure with its transport regmap set
> + *
> + * This function completes the common probe steps shared by all transport
> + * drivers: IRQ domain creation, IDA initialization, and MFD cell registration.
> + *
> + * The caller must have already set nct6694->dev, nct6694->priv, and
> + * nct6694->regmap before calling this.
> + *
> + * Return: 0 on success or negative errno on failure.
> + */
> +int nct6694_core_probe(struct device *dev, struct nct6694 *nct6694,

You can put nct6694 into the device's private struct attribute which
means that you no longer have to pass it around.

I'm not particularly keen on passing mfd_cells around.  Why not call
mfd_add_*() from the transport's probe?

> +		       const struct mfd_cell *cells, int n_cells)
> +{
> +	int ret;
> +
> +	spin_lock_init(&nct6694->irq_lock);
> +
> +	ida_init(&nct6694->gpio_ida);
> +	ida_init(&nct6694->i2c_ida);
> +	ida_init(&nct6694->canfd_ida);
> +	ida_init(&nct6694->wdt_ida);
> +
> +	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
> +						   &nct6694_irq_domain_ops,
> +						   nct6694);
> +	if (!nct6694->domain) {
> +		ret = -ENODEV;

Should we return '-ENOMEM' here instead of '-ENODEV', as this represents
a resource allocation failure?

> +		goto err_ida;
> +	}
> +
> +	ret = mfd_add_hotplug_devices(dev, cells, n_cells);
> +	if (ret)
> +		goto err_domain;
> +
> +	return 0;
> +
> +err_domain:
> +	irq_domain_remove(nct6694->domain);
> +err_ida:
> +	ida_destroy(&nct6694->wdt_ida);
> +	ida_destroy(&nct6694->canfd_ida);
> +	ida_destroy(&nct6694->i2c_ida);
> +	ida_destroy(&nct6694->gpio_ida);
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(nct6694_core_probe);
> +
> +/**
> + * nct6694_core_remove() - Unregister MFD sub-devices and free core resources
> + * @nct6694: nct6694 structure previously passed to nct6694_core_probe()
> + */
> +void nct6694_core_remove(struct nct6694 *nct6694)

Should we also rename 'nct6694' to 'ddata' here?

> +{
> +	mfd_remove_devices(nct6694->dev);
> +	irq_domain_remove(nct6694->domain);
> +	ida_destroy(&nct6694->wdt_ida);
> +	ida_destroy(&nct6694->canfd_ida);
> +	ida_destroy(&nct6694->i2c_ida);
> +	ida_destroy(&nct6694->gpio_ida);
> +}
> +EXPORT_SYMBOL_GPL(nct6694_core_remove);
> +
> +MODULE_DESCRIPTION("Nuvoton NCT6694 MFD core driver");
> +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index 7c6b986db7f7..2289ebfde7fa 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c
> @@ -10,7 +10,6 @@
>   */
>  
>  #include <linux/bits.h>
> -#include <linux/idr.h>
>  #include <linux/interrupt.h>
>  #include <linux/irq.h>
>  #include <linux/irqdomain.h>
> @@ -19,7 +18,6 @@
>  #include <linux/mfd/nct6694.h>
>  #include <linux/module.h>
>  #include <linux/slab.h>
> -#include <linux/spinlock.h>
>  #include <linux/usb.h>
>  
>  #define NCT6694_VENDOR_ID	0x0416
> @@ -246,57 +244,9 @@ static void nct6694_usb_int_callback(struct urb *urb)
>  resubmit:
>  	ret = usb_submit_urb(urb, GFP_ATOMIC);
>  	if (ret)
> -		dev_warn(nct6694->dev, "Failed to resubmit urb, status %pe",  ERR_PTR(ret));
> +		dev_warn(nct6694->dev, "Failed to resubmit urb, status %pe", ERR_PTR(ret));
>  }
>  
> -static void nct6694_irq_enable(struct irq_data *data)
> -{
> -	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> -	irq_hw_number_t hwirq = irqd_to_hwirq(data);
> -
> -	guard(spinlock_irqsave)(&nct6694->irq_lock);
> -
> -	nct6694->irq_enable |= BIT(hwirq);
> -}
> -
> -static void nct6694_irq_disable(struct irq_data *data)
> -{
> -	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> -	irq_hw_number_t hwirq = irqd_to_hwirq(data);
> -
> -	guard(spinlock_irqsave)(&nct6694->irq_lock);
> -
> -	nct6694->irq_enable &= ~BIT(hwirq);
> -}
> -
> -static const struct irq_chip nct6694_irq_chip = {
> -	.name = "nct6694-irq",
> -	.flags = IRQCHIP_SKIP_SET_WAKE,
> -	.irq_enable = nct6694_irq_enable,
> -	.irq_disable = nct6694_irq_disable,
> -};
> -
> -static int nct6694_irq_domain_map(struct irq_domain *d, unsigned int irq, irq_hw_number_t hw)
> -{
> -	struct nct6694 *nct6694 = d->host_data;
> -
> -	irq_set_chip_data(irq, nct6694);
> -	irq_set_chip_and_handler(irq, &nct6694_irq_chip, handle_simple_irq);
> -
> -	return 0;
> -}
> -
> -static void nct6694_irq_domain_unmap(struct irq_domain *d, unsigned int irq)
> -{
> -	irq_set_chip_and_handler(irq, NULL, NULL);
> -	irq_set_chip_data(irq, NULL);
> -}
> -
> -static const struct irq_domain_ops nct6694_irq_domain_ops = {
> -	.map	= nct6694_irq_domain_map,
> -	.unmap	= nct6694_irq_domain_unmap,
> -};
> -
>  static int nct6694_usb_probe(struct usb_interface *iface,
>  			     const struct usb_device_id *id)
>  {
> @@ -330,35 +280,19 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  
>  	udata->udev = udev;
>  
> -	nct6694->priv = udata;
> -
> -	nct6694->domain = irq_domain_create_simple(NULL, NCT6694_NR_IRQS, 0,
> -						   &nct6694_irq_domain_ops,
> -						   nct6694);
> -	if (!nct6694->domain) {
> -		ret = -ENODEV;
> -		goto err_urb;
> -	}
> -
>  	nct6694->dev = dev;
> -
> -	ida_init(&nct6694->gpio_ida);
> -	ida_init(&nct6694->i2c_ida);
> -	ida_init(&nct6694->canfd_ida);
> -	ida_init(&nct6694->wdt_ida);
> -
> -	spin_lock_init(&nct6694->irq_lock);
> +	nct6694->priv = udata;
>  
>  	ret = devm_mutex_init(dev, &udata->access_lock);
>  	if (ret)
> -		goto err_ida;
> +		goto err_urb;
>  
>  	interface = iface->cur_altsetting;
>  
>  	int_endpoint = &interface->endpoint[0].desc;
>  	if (!usb_endpoint_is_int_in(int_endpoint)) {
>  		ret = -ENODEV;
> -		goto err_ida;
> +		goto err_urb;
>  	}
>  
>  	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> @@ -367,11 +301,11 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  
>  	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
>  	if (ret)
> -		goto err_ida;
> +		goto err_urb;
>  
>  	usb_set_intfdata(iface, nct6694);
>  
> -	ret = mfd_add_hotplug_devices(dev, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
> +	ret = nct6694_core_probe(dev, nct6694, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
>  	if (ret)
>  		goto err_mfd;
>  
> @@ -379,12 +313,6 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  
>  err_mfd:
>  	usb_kill_urb(udata->int_in_urb);
> -err_ida:
> -	ida_destroy(&nct6694->wdt_ida);
> -	ida_destroy(&nct6694->canfd_ida);
> -	ida_destroy(&nct6694->i2c_ida);
> -	ida_destroy(&nct6694->gpio_ida);
> -	irq_domain_remove(nct6694->domain);
>  err_urb:
>  	usb_free_urb(udata->int_in_urb);
>  	return ret;
> @@ -395,13 +323,8 @@ static void nct6694_usb_disconnect(struct usb_interface *iface)
>  	struct nct6694 *nct6694 = usb_get_intfdata(iface);
>  	struct nct6694_usb_data *udata = nct6694->priv;
>  
> -	mfd_remove_devices(nct6694->dev);
> +	nct6694_core_remove(nct6694);
>  	usb_kill_urb(udata->int_in_urb);
> -	ida_destroy(&nct6694->wdt_ida);
> -	ida_destroy(&nct6694->canfd_ida);
> -	ida_destroy(&nct6694->i2c_ida);
> -	ida_destroy(&nct6694->gpio_ida);
> -	irq_domain_remove(nct6694->domain);
>  	usb_free_urb(udata->int_in_urb);
>  }
>  
> diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> index 43b51f243e8e..853b1530755d 100644
> --- a/include/linux/mfd/nct6694.h
> +++ b/include/linux/mfd/nct6694.h
> @@ -10,10 +10,13 @@
>  #define __MFD_NCT6694_H
>  
>  #include <linux/idr.h>
> -#include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/types.h>
>  
> +struct device;
> +struct irq_domain;
> +struct mfd_cell;
> +
>  #define NCT6694_HWMON_MOD	0x00
>  #define NCT6694_PWM_MOD		0x01
>  #define NCT6694_I2C_MOD		0x03
> @@ -94,6 +97,10 @@ struct nct6694 {
>  	void *priv;
>  };
>  
> +int nct6694_core_probe(struct device *dev, struct nct6694 *nct6694,
> +		       const struct mfd_cell *cells, int n_cells);
> +void nct6694_core_remove(struct nct6694 *nct6694);
> +
>  int nct6694_usb_read_msg(struct nct6694 *nct6694,
>  			 const struct nct6694_cmd_header *cmd_hd,
>  			 void *buf);
> -- 
> 2.34.1
> 

-- 
Lee Jones

[PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

The transport hands the caller's payload buffer straight to
usb_bulk_msg(). Sub-device drivers embed those buffers in their private
data structures, so they are neither cacheline aligned nor exclusively
owned by the transfer. On non-coherent architectures, mapping such a
buffer for DMA can corrupt the unrelated fields sharing its cachelines.

Transfer the payload through a buffer owned by the transport instead,
and reject commands exceeding the maximum firmware packet size.

Fixes: 51dad33ede63 ("mfd: Add core driver for Nuvoton NCT6694")
Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:
- New patch. Fixes the DMA-unsafe use of caller-owned payload buffers
  reported on v6 patch 6/7.

 drivers/mfd/nct6694-usb.c   | 38 +++++++++++++++++++++++++++----------
 include/linux/mfd/nct6694.h |  3 +++
 2 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
index 2289ebfde7fa..793ce54c02aa 100644
--- a/drivers/mfd/nct6694-usb.c
+++ b/drivers/mfd/nct6694-usb.c
@@ -38,6 +38,7 @@ struct nct6694_usb_data {
 	struct urb *int_in_urb;
 	struct usb_device *udev;
 	union nct6694_usb_msg *usb_msg;
+	void *xfer_buf;
 	__le32 *int_buffer;
 };
 
@@ -120,8 +121,12 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
 	struct nct6694_usb_data *udata = nct6694->priv;
 	union nct6694_usb_msg *msg = udata->usb_msg;
 	struct usb_device *udev = udata->udev;
+	u16 len = le16_to_cpu(cmd_hd->len);
 	int tx_len, rx_len, ret;
 
+	if (len > NCT6694_MAX_PACKET_SIZE)
+		return -EINVAL;
+
 	guard(mutex)(&udata->access_lock);
 
 	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
@@ -140,17 +145,19 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
 		return ret;
 
 	/* Receive data packet from USB device */
-	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
-			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
+			   len, &rx_len, NCT6694_URB_TIMEOUT);
 	if (ret)
 		return ret;
 
-	if (rx_len != le16_to_cpu(cmd_hd->len)) {
+	if (rx_len != len) {
 		dev_err(nct6694->dev, "Expected received length %d, but got %d\n",
-			le16_to_cpu(cmd_hd->len), rx_len);
+			len, rx_len);
 		return -EIO;
 	}
 
+	memcpy(buf, udata->xfer_buf, len);
+
 	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
 }
 EXPORT_SYMBOL_GPL(nct6694_usb_read_msg);
@@ -173,12 +180,17 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
 	struct nct6694_usb_data *udata = nct6694->priv;
 	union nct6694_usb_msg *msg = udata->usb_msg;
 	struct usb_device *udev = udata->udev;
+	u16 len = le16_to_cpu(cmd_hd->len);
 	int tx_len, rx_len, ret;
 
+	if (len > NCT6694_MAX_PACKET_SIZE)
+		return -EINVAL;
+
 	guard(mutex)(&udata->access_lock);
 
 	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
 	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
+	memcpy(udata->xfer_buf, buf, len);
 
 	/* Send command packet to USB device */
 	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), &msg->cmd_header,
@@ -187,8 +199,8 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
 		return ret;
 
 	/* Send data packet to USB device */
-	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), buf,
-			   le16_to_cpu(cmd_hd->len), &tx_len, NCT6694_URB_TIMEOUT);
+	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), udata->xfer_buf,
+			   len, &tx_len, NCT6694_URB_TIMEOUT);
 	if (ret)
 		return ret;
 
@@ -199,17 +211,19 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
 		return ret;
 
 	/* Receive data packet from USB device */
-	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
-			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
+	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
+			   len, &rx_len, NCT6694_URB_TIMEOUT);
 	if (ret)
 		return ret;
 
-	if (rx_len != le16_to_cpu(cmd_hd->len)) {
+	if (rx_len != len) {
 		dev_err(nct6694->dev, "Expected transmitted length %d, but got %d\n",
-			le16_to_cpu(cmd_hd->len), rx_len);
+			len, rx_len);
 		return -EIO;
 	}
 
+	memcpy(buf, udata->xfer_buf, len);
+
 	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
 }
 EXPORT_SYMBOL_GPL(nct6694_usb_write_msg);
@@ -270,6 +284,10 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	if (!udata->usb_msg)
 		return -ENOMEM;
 
+	udata->xfer_buf = devm_kzalloc(dev, NCT6694_MAX_PACKET_SIZE, GFP_KERNEL);
+	if (!udata->xfer_buf)
+		return -ENOMEM;
+
 	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
 	if (!udata->int_buffer)
 		return -ENOMEM;
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
index 853b1530755d..cb311e58a437 100644
--- a/include/linux/mfd/nct6694.h
+++ b/include/linux/mfd/nct6694.h
@@ -29,6 +29,9 @@ struct mfd_cell;
 #define NCT6694_HCTRL_SET	0x40
 #define NCT6694_HCTRL_GET	0x80
 
+/* Maximum payload length the firmware accepts in a single command */
+#define NCT6694_MAX_PACKET_SIZE	0x3F0
+
 enum nct6694_irq_id {
 	NCT6694_IRQ_GPIO0 = 0,
 	NCT6694_IRQ_GPIO1,
-- 
2.34.1

Re: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The GPIO controller driver incorrectly sets `data->gpio.can_sleep = false`, falsely advertising to the GPIO core that its callbacks can be invoked from atomic contexts.
- [High] Missing check for unmapped IRQs allows a USB device to arbitrarily trigger IRQ 0 handlers.
--

--- Patch [6]: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer ---
commit 20bf57a851a06631ddae8833be2bd95d21bbd709
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

This commit fixes a DMA safety issue on non-coherent architectures by
transferring command payloads through a dedicated buffer owned by the
transport. It also ensures commands exceeding the maximum firmware packet
size are rejected.

> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index 2289ebfde7fab..793ce54c02aad 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c

[ ... ]

[Severity: High]
This is a pre-existing issue, but does this code leave us vulnerable to
spurious IRQ 0 triggers?

Looking at nct6694_usb_int_callback() in drivers/mfd/nct6694-usb.c, the
return value is passed directly into the handler:

    generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));

If a buggy or malicious USB device sends an interrupt URB with bits set
for unmapped IRQs, irq_find_mapping() returns 0. Since 0 is often a valid
system interrupt on some architectures, could this crash the system by
unconditionally passing 0 to generic_handle_irq_safe()?

[Severity: High]
This isn't a bug introduced by this patch, but does the GPIO controller
driver falsely advertise that its callbacks can run in atomic context?

In drivers/gpio/gpio-nct6694.c during nct6694_gpio_probe(), the driver sets:

    data->gpio.can_sleep = false;

However, the GPIO accessors like nct6694_get_value() take a mutex and
perform blocking USB I/O:

    guard(mutex)(&data->lock);
    ret = nct6694_read_msg(data->nct6694, &cmd_hd, &data->reg_val);

Will this cause a kernel panic if a consumer of this GPIO chip calls a
gpiod_* function from within a spinlock or hardirq context, given that the
GPIO core permits atomic usage based on this flag?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=6

Re: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

[Lee Jones]

On Fri, 21 Aug 2026, a0282524688@gmail.com wrote:

> From: Ming Yu <a0282524688@gmail.com>
> 
> The transport hands the caller's payload buffer straight to
> usb_bulk_msg(). Sub-device drivers embed those buffers in their private
> data structures, so they are neither cacheline aligned nor exclusively
> owned by the transfer. On non-coherent architectures, mapping such a
> buffer for DMA can corrupt the unrelated fields sharing its cachelines.
> 
> Transfer the payload through a buffer owned by the transport instead,
> and reject commands exceeding the maximum firmware packet size.
> 
> Fixes: 51dad33ede63 ("mfd: Add core driver for Nuvoton NCT6694")
> Signed-off-by: Ming Yu <a0282524688@gmail.com>
> ---
> Changes in v7:
> - New patch. Fixes the DMA-unsafe use of caller-owned payload buffers
>   reported on v6 patch 6/7.
> 
>  drivers/mfd/nct6694-usb.c   | 38 +++++++++++++++++++++++++++----------
>  include/linux/mfd/nct6694.h |  3 +++
>  2 files changed, 31 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index 2289ebfde7fa..793ce54c02aa 100644
> --- 'a/drivers/mfd/nct6694-usb.c'
> +++ 'b/drivers/mfd/nct6694-usb.c'
> @@ -38,6 +38,7 @@ struct nct6694_usb_data {
>  	struct urb *int_in_urb;
>  	struct usb_device *udev;
>  	union nct6694_usb_msg *usb_msg;
> +	void *xfer_buf;
>  	__le32 *int_buffer;

There are a bunch of buffers used in this file and I'm losing track of
which one is which.  Please consolidate on the nomenclature for them
all.

Ins, outs, payloads, xfers, etc.  Make a decision and stick to it throughout.

>  };
>  
> @@ -120,8 +121,12 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
>  	struct nct6694_usb_data *udata = nct6694->priv;
>  	union nct6694_usb_msg *msg = udata->usb_msg;
>  	struct usb_device *udev = udata->udev;
> +	u16 len = le16_to_cpu(cmd_hd->len);
>  	int tx_len, rx_len, ret;
>  
> +	if (len > NCT6694_MAX_PACKET_SIZE)
> +		return -EINVAL;
> +
>  	guard(mutex)(&udata->access_lock);
>  
>  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> @@ -140,17 +145,19 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
>  		return ret;
>  
>  	/* Receive data packet from USB device */
> -	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> -			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
> +			   len, &rx_len, NCT6694_URB_TIMEOUT);
>  	if (ret)
>  		return ret;
>  
> -	if (rx_len != le16_to_cpu(cmd_hd->len)) {
> +	if (rx_len != len) {
>  		dev_err(nct6694->dev, "Expected received length %d, but got %d\n",
> -			le16_to_cpu(cmd_hd->len), rx_len);
> +			len, rx_len);
>  		return -EIO;
>  	}
>  
> +	memcpy(buf, udata->xfer_buf, len);
> +
>  	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
>  }
>  EXPORT_SYMBOL_GPL(nct6694_usb_read_msg);
> @@ -173,12 +180,17 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
>  	struct nct6694_usb_data *udata = nct6694->priv;
>  	union nct6694_usb_msg *msg = udata->usb_msg;
>  	struct usb_device *udev = udata->udev;
> +	u16 len = le16_to_cpu(cmd_hd->len);
>  	int tx_len, rx_len, ret;
>  
> +	if (len > NCT6694_MAX_PACKET_SIZE)
> +		return -EINVAL;
> +
>  	guard(mutex)(&udata->access_lock);
>  
>  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
>  	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> +	memcpy(udata->xfer_buf, buf, len);
>  
>  	/* Send command packet to USB device */
>  	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), &msg->cmd_header,
> @@ -187,8 +199,8 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
>  		return ret;
>  
>  	/* Send data packet to USB device */
> -	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), buf,
> -			   le16_to_cpu(cmd_hd->len), &tx_len, NCT6694_URB_TIMEOUT);
> +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), udata->xfer_buf,
> +			   len, &tx_len, NCT6694_URB_TIMEOUT);
>  	if (ret)
>  		return ret;
>  
> @@ -199,17 +211,19 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
>  		return ret;
>  
>  	/* Receive data packet from USB device */
> -	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> -			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
> +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
> +			   len, &rx_len, NCT6694_URB_TIMEOUT);
>  	if (ret)
>  		return ret;
>  
> -	if (rx_len != le16_to_cpu(cmd_hd->len)) {
> +	if (rx_len != len) {
>  		dev_err(nct6694->dev, "Expected transmitted length %d, but got %d\n",
> -			le16_to_cpu(cmd_hd->len), rx_len);
> +			len, rx_len);
>  		return -EIO;
>  	}
>  
> +	memcpy(buf, udata->xfer_buf, len);
> +
>  	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
>  }
>  EXPORT_SYMBOL_GPL(nct6694_usb_write_msg);
> @@ -270,6 +284,10 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	if (!udata->usb_msg)
>  		return -ENOMEM;
>  
> +	udata->xfer_buf = devm_kzalloc(dev, NCT6694_MAX_PACKET_SIZE, GFP_KERNEL);
> +	if (!udata->xfer_buf)
> +		return -ENOMEM;
> +
>  	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
>  	if (!udata->int_buffer)
>  		return -ENOMEM;
> diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> index 853b1530755d..cb311e58a437 100644
> --- a/include/linux/mfd/nct6694.h
> +++ b/include/linux/mfd/nct6694.h
> @@ -29,6 +29,9 @@ struct mfd_cell;
>  #define NCT6694_HCTRL_SET	0x40
>  #define NCT6694_HCTRL_GET	0x80
>  
> +/* Maximum payload length the firmware accepts in a single command */
> +#define NCT6694_MAX_PACKET_SIZE	0x3F0
> +
>  enum nct6694_irq_id {
>  	NCT6694_IRQ_GPIO0 = 0,
>  	NCT6694_IRQ_GPIO1,
> -- 
> 2.34.1
> 

-- 
Lee Jones

Re: [PATCH v7 06/10] mfd: nct6694: Transfer command payloads via a dedicated DMA buffer

[Lee Jones]

On Thu, 03 Sep 2026, Lee Jones wrote:

> On Fri, 21 Aug 2026, a0282524688@gmail.com wrote:
> 
> > From: Ming Yu <a0282524688@gmail.com>
> > 
> > The transport hands the caller's payload buffer straight to
> > usb_bulk_msg(). Sub-device drivers embed those buffers in their private
> > data structures, so they are neither cacheline aligned nor exclusively
> > owned by the transfer. On non-coherent architectures, mapping such a
> > buffer for DMA can corrupt the unrelated fields sharing its cachelines.
> > 
> > Transfer the payload through a buffer owned by the transport instead,
> > and reject commands exceeding the maximum firmware packet size.
> > 
> > Fixes: 51dad33ede63 ("mfd: Add core driver for Nuvoton NCT6694")
> > Signed-off-by: Ming Yu <a0282524688@gmail.com>
> > ---
> > Changes in v7:
> > - New patch. Fixes the DMA-unsafe use of caller-owned payload buffers
> >   reported on v6 patch 6/7.
> > 
> >  drivers/mfd/nct6694-usb.c   | 38 +++++++++++++++++++++++++++----------
> >  include/linux/mfd/nct6694.h |  3 +++
> >  2 files changed, 31 insertions(+), 10 deletions(-)
> > 
> > diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> > index 2289ebfde7fa..793ce54c02aa 100644
> > --- 'a/drivers/mfd/nct6694-usb.c'
> > +++ 'b/drivers/mfd/nct6694-usb.c'
> > @@ -38,6 +38,7 @@ struct nct6694_usb_data {
> >  	struct urb *int_in_urb;
> >  	struct usb_device *udev;
> >  	union nct6694_usb_msg *usb_msg;
> > +	void *xfer_buf;
> >  	__le32 *int_buffer;
> 
> There are a bunch of buffers used in this file and I'm losing track of
> which one is which.  Please consolidate on the nomenclature for them
> all.
> 
> Ins, outs, payloads, xfers, etc.  Make a decision and stick to it throughout.

Same with 'len' - label them all please.

> >  };
> >  
> > @@ -120,8 +121,12 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
> >  	struct nct6694_usb_data *udata = nct6694->priv;
> >  	union nct6694_usb_msg *msg = udata->usb_msg;
> >  	struct usb_device *udev = udata->udev;
> > +	u16 len = le16_to_cpu(cmd_hd->len);
> >  	int tx_len, rx_len, ret;
> >  
> > +	if (len > NCT6694_MAX_PACKET_SIZE)
> > +		return -EINVAL;
> > +
> >  	guard(mutex)(&udata->access_lock);
> >  
> >  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> > @@ -140,17 +145,19 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
> >  		return ret;
> >  
> >  	/* Receive data packet from USB device */
> > -	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> > -			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
> > +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
> > +			   len, &rx_len, NCT6694_URB_TIMEOUT);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (rx_len != le16_to_cpu(cmd_hd->len)) {
> > +	if (rx_len != len) {
> >  		dev_err(nct6694->dev, "Expected received length %d, but got %d\n",
> > -			le16_to_cpu(cmd_hd->len), rx_len);
> > +			len, rx_len);
> >  		return -EIO;
> >  	}
> >  
> > +	memcpy(buf, udata->xfer_buf, len);
> > +
> >  	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
> >  }
> >  EXPORT_SYMBOL_GPL(nct6694_usb_read_msg);
> > @@ -173,12 +180,17 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
> >  	struct nct6694_usb_data *udata = nct6694->priv;
> >  	union nct6694_usb_msg *msg = udata->usb_msg;
> >  	struct usb_device *udev = udata->udev;
> > +	u16 len = le16_to_cpu(cmd_hd->len);
> >  	int tx_len, rx_len, ret;
> >  
> > +	if (len > NCT6694_MAX_PACKET_SIZE)
> > +		return -EINVAL;
> > +
> >  	guard(mutex)(&udata->access_lock);
> >  
> >  	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
> >  	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> > +	memcpy(udata->xfer_buf, buf, len);
> >  
> >  	/* Send command packet to USB device */
> >  	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), &msg->cmd_header,
> > @@ -187,8 +199,8 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
> >  		return ret;
> >  
> >  	/* Send data packet to USB device */
> > -	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), buf,
> > -			   le16_to_cpu(cmd_hd->len), &tx_len, NCT6694_URB_TIMEOUT);
> > +	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), udata->xfer_buf,
> > +			   len, &tx_len, NCT6694_URB_TIMEOUT);
> >  	if (ret)
> >  		return ret;
> >  
> > @@ -199,17 +211,19 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
> >  		return ret;
> >  
> >  	/* Receive data packet from USB device */
> > -	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), buf,
> > -			   le16_to_cpu(cmd_hd->len), &rx_len, NCT6694_URB_TIMEOUT);
> > +	ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, NCT6694_BULK_IN_EP), udata->xfer_buf,
> > +			   len, &rx_len, NCT6694_URB_TIMEOUT);
> >  	if (ret)
> >  		return ret;
> >  
> > -	if (rx_len != le16_to_cpu(cmd_hd->len)) {
> > +	if (rx_len != len) {
> >  		dev_err(nct6694->dev, "Expected transmitted length %d, but got %d\n",
> > -			le16_to_cpu(cmd_hd->len), rx_len);
> > +			len, rx_len);
> >  		return -EIO;
> >  	}
> >  
> > +	memcpy(buf, udata->xfer_buf, len);
> > +
> >  	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
> >  }
> >  EXPORT_SYMBOL_GPL(nct6694_usb_write_msg);
> > @@ -270,6 +284,10 @@ static int nct6694_usb_probe(struct usb_interface *iface,
> >  	if (!udata->usb_msg)
> >  		return -ENOMEM;
> >  
> > +	udata->xfer_buf = devm_kzalloc(dev, NCT6694_MAX_PACKET_SIZE, GFP_KERNEL);
> > +	if (!udata->xfer_buf)
> > +		return -ENOMEM;
> > +
> >  	udata->int_buffer = devm_kzalloc(dev, sizeof(*udata->int_buffer), GFP_KERNEL);
> >  	if (!udata->int_buffer)
> >  		return -ENOMEM;
> > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
> > index 853b1530755d..cb311e58a437 100644
> > --- a/include/linux/mfd/nct6694.h
> > +++ b/include/linux/mfd/nct6694.h
> > @@ -29,6 +29,9 @@ struct mfd_cell;
> >  #define NCT6694_HCTRL_SET	0x40
> >  #define NCT6694_HCTRL_GET	0x80
> >  
> > +/* Maximum payload length the firmware accepts in a single command */
> > +#define NCT6694_MAX_PACKET_SIZE	0x3F0
> > +
> >  enum nct6694_irq_id {
> >  	NCT6694_IRQ_GPIO0 = 0,
> >  	NCT6694_IRQ_GPIO1,
> > -- 
> > 2.34.1
> > 
> 
> -- 
> Lee Jones

-- 
Lee Jones

[PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

The probe reads endpoint[0] of the current altsetting without checking
that the interface actually describes any endpoint. A device reporting
zero endpoints makes the driver read past the endpoint array.

Locate the interrupt IN endpoint with usb_find_int_in_endpoint(), which
validates the descriptor before it is used.

Fixes: 51dad33ede63 ("mfd: Add core driver for Nuvoton NCT6694")
Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:
- New patch. Fixes the out-of-bounds endpoint access reported on v6
  patch 6/7.

 drivers/mfd/nct6694-usb.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
index 793ce54c02aa..c8667984df5e 100644
--- a/drivers/mfd/nct6694-usb.c
+++ b/drivers/mfd/nct6694-usb.c
@@ -266,7 +266,6 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 {
 	struct usb_device *udev = interface_to_usbdev(iface);
 	struct usb_endpoint_descriptor *int_endpoint;
-	struct usb_host_interface *interface;
 	struct device *dev = &iface->dev;
 	struct nct6694_usb_data *udata;
 	struct nct6694 *nct6694;
@@ -305,13 +304,9 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	if (ret)
 		goto err_urb;
 
-	interface = iface->cur_altsetting;
-
-	int_endpoint = &interface->endpoint[0].desc;
-	if (!usb_endpoint_is_int_in(int_endpoint)) {
-		ret = -ENODEV;
+	ret = usb_find_int_in_endpoint(iface->cur_altsetting, &int_endpoint);
+	if (ret)
 		goto err_urb;
-	}
 
 	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
 			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
-- 
2.34.1

Re: [PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Unchecked transfer lengths lead to stale data reads.
- [Medium] Mismatched endpoint address used for interrupt pipe creation.
- [Medium] Missing endpoint existence and type validation for required bulk endpoints.
--

--- Patch [7]: [PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint ---
commit 403c0781b839695d774ed394e3d21842e56bd309
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Validate the interrupt IN endpoint

The probe reads endpoint[0] of the current altsetting without checking
that the interface actually describes any endpoint. A device reporting
zero endpoints makes the driver read past the endpoint array.

> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index 793ce54c02aad..c8667984df5eb 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c
> @@ -305,13 +304,9 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  	if (ret)
>  		goto err_urb;
>  
> -	interface = iface->cur_altsetting;
> -
> -	int_endpoint = &interface->endpoint[0].desc;
> -	if (!usb_endpoint_is_int_in(int_endpoint)) {
> -		ret = -ENODEV;
> +	ret = usb_find_int_in_endpoint(iface->cur_altsetting, &int_endpoint);
> +	if (ret)
>  		goto err_urb;

[Severity: Medium]
This is a pre-existing issue, but does this probe function also need to
validate the bulk endpoints?

It appears that nct6694_usb_read_msg() uses usb_bulk_msg() with a hardcoded
NCT6694_BULK_IN_EP. If a malformed device lacks these bulk endpoints or
configures them with the wrong type, could it trigger a 'BOGUS urb xfer'
warning in the USB core?

> -	}
>  
>  	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
>  			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
>  			 nct6694, int_endpoint->bInterval);

[Severity: Medium]
This is a pre-existing issue, but does this mismatch the found endpoint
address?

We dynamically find the interrupt endpoint above and use its bInterval here,
but we still hardcode NCT6694_INT_IN_EP for the pipe creation. If a malformed
device has its first interrupt IN endpoint at a different address, and 0x81 is
used for a different type, could this result in a type mismatch and a kernel
warning?

[Severity: High]
This isn't a bug introduced by this patch, but does the driver handle short or
zero-length packets correctly?

Looking at nct6694_usb_int_callback(), it seems to read the transfer buffer
without checking urb->actual_length:

drivers/mfd/nct6694-usb.c:nct6694_usb_int_callback() {
    ...
	int_status = le32_to_cpu(*status_le);

	while (int_status) {
		int irq = __ffs(int_status);
    ...
}

Similarly, in nct6694_usb_read_msg(), it appears rx_len is not checked before
evaluating msg->response_header.sts. If a device returns short or zero-length
packets, could this cause the driver to read uninitialized memory and
potentially trigger spurious IRQs?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=7

Re: [PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint

[Lee Jones]

On Fri, 21 Aug 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Unchecked transfer lengths lead to stale data reads.
> - [Medium] Mismatched endpoint address used for interrupt pipe creation.
> - [Medium] Missing endpoint existence and type validation for required bulk endpoints.

Since this is a new driver, these are all your issues too, right?

I'm going to stop my review here until Sashiko is clean for this driver.

> --
> 
> --- Patch [7]: [PATCH v7 07/10] mfd: nct6694: Validate the interrupt IN endpoint ---
> commit 403c0781b839695d774ed394e3d21842e56bd309
> Author: Ming Yu <a0282524688@gmail.com>
> 
> mfd: nct6694: Validate the interrupt IN endpoint
> 
> The probe reads endpoint[0] of the current altsetting without checking
> that the interface actually describes any endpoint. A device reporting
> zero endpoints makes the driver read past the endpoint array.
> 
> > diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> > index 793ce54c02aad..c8667984df5eb 100644
> > --- a/drivers/mfd/nct6694-usb.c
> > +++ b/drivers/mfd/nct6694-usb.c
> > @@ -305,13 +304,9 @@ static int nct6694_usb_probe(struct usb_interface *iface,
> >  	if (ret)
> >  		goto err_urb;
> >  
> > -	interface = iface->cur_altsetting;
> > -
> > -	int_endpoint = &interface->endpoint[0].desc;
> > -	if (!usb_endpoint_is_int_in(int_endpoint)) {
> > -		ret = -ENODEV;
> > +	ret = usb_find_int_in_endpoint(iface->cur_altsetting, &int_endpoint);
> > +	if (ret)
> >  		goto err_urb;
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this probe function also need to
> validate the bulk endpoints?
> 
> It appears that nct6694_usb_read_msg() uses usb_bulk_msg() with a hardcoded
> NCT6694_BULK_IN_EP. If a malformed device lacks these bulk endpoints or
> configures them with the wrong type, could it trigger a 'BOGUS urb xfer'
> warning in the USB core?
> 
> > -	}
> >  
> >  	usb_fill_int_urb(udata->int_in_urb, udev, usb_rcvintpipe(udev, NCT6694_INT_IN_EP),
> >  			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
> >  			 nct6694, int_endpoint->bInterval);
> 
> [Severity: Medium]
> This is a pre-existing issue, but does this mismatch the found endpoint
> address?
> 
> We dynamically find the interrupt endpoint above and use its bInterval here,
> but we still hardcode NCT6694_INT_IN_EP for the pipe creation. If a malformed
> device has its first interrupt IN endpoint at a different address, and 0x81 is
> used for a different type, could this result in a type mismatch and a kernel
> warning?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does the driver handle short or
> zero-length packets correctly?
> 
> Looking at nct6694_usb_int_callback(), it seems to read the transfer buffer
> without checking urb->actual_length:
> 
> drivers/mfd/nct6694-usb.c:nct6694_usb_int_callback() {
>     ...
> 	int_status = le32_to_cpu(*status_le);
> 
> 	while (int_status) {
> 		int irq = __ffs(int_status);
>     ...
> }
> 
> Similarly, in nct6694_usb_read_msg(), it appears rx_len is not checked before
> evaluating msg->response_header.sts. If a device returns short or zero-length
> packets, could this cause the driver to read uninitialized memory and
> potentially trigger spurious IRQs?
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=7

-- 
Lee Jones

[PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

The interrupt URB is submitted before nct6694_core_probe() creates the
IRQ domain. A device completing the URB early makes the callback pass a
NULL domain to irq_find_mapping(), which then falls back to the default
domain and dispatches interrupts the device does not own.

Submit the URB once the core is registered, and kill it before the core
is removed so the callback never runs without a domain.

Fixes: 51dad33ede63 ("mfd: Add core driver for Nuvoton NCT6694")
Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:
- New patch. Fixes the URB-before-IRQ-domain race reported on v6
  patch 6/7.

 drivers/mfd/nct6694-usb.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
index c8667984df5e..cc3e7b7b3a0d 100644
--- a/drivers/mfd/nct6694-usb.c
+++ b/drivers/mfd/nct6694-usb.c
@@ -312,20 +312,20 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
 			 nct6694, int_endpoint->bInterval);
 
-	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
-	if (ret)
-		goto err_urb;
-
 	usb_set_intfdata(iface, nct6694);
 
 	ret = nct6694_core_probe(dev, nct6694, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
 	if (ret)
-		goto err_mfd;
+		goto err_urb;
+
+	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
+	if (ret)
+		goto err_core;
 
 	return 0;
 
-err_mfd:
-	usb_kill_urb(udata->int_in_urb);
+err_core:
+	nct6694_core_remove(nct6694);
 err_urb:
 	usb_free_urb(udata->int_in_urb);
 	return ret;
@@ -336,8 +336,8 @@ static void nct6694_usb_disconnect(struct usb_interface *iface)
 	struct nct6694 *nct6694 = usb_get_intfdata(iface);
 	struct nct6694_usb_data *udata = nct6694->priv;
 
-	nct6694_core_remove(nct6694);
 	usb_kill_urb(udata->int_in_urb);
+	nct6694_core_remove(nct6694);
 	usb_free_urb(udata->int_in_urb);
 }
 
-- 
2.34.1

Re: [PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing validation of irq_find_mapping() return value leads to spurious system interrupts (e.g., IRQ 0).
- [High] Missing urb->actual_length validation leads to processing stale memory and spurious interrupt storms.
--

--- Patch [8]: [PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered ---
commit 8c344a9772877b334fe47602ee4be5e3e806ecab
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Submit the interrupt URB after the core is registered

The interrupt URB is submitted before nct6694_core_probe() creates the
IRQ domain. A device completing the URB early makes the callback pass a
NULL domain to irq_find_mapping(), which then falls back to the default
domain and dispatches interrupts the device does not own.

Submit the URB once the core is registered, and kill it before the core
is removed so the callback never runs without a domain.

> diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> index c8667984df5eb..cc3e7b7b3a0dc 100644
> --- a/drivers/mfd/nct6694-usb.c
> +++ b/drivers/mfd/nct6694-usb.c
> @@ -312,20 +312,20 @@ static int nct6694_usb_probe(struct usb_interface *iface,
>  			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,

[Severity: High]
This is a pre-existing issue, but does this callback risk dispatching
interrupts the device does not own if an unmapped IRQ is received?

Looking at the loop inside nct6694_usb_int_callback():

	while (int_status) {
		int irq = __ffs(int_status);

		generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
		int_status &= ~BIT(irq);
	}

If a malfunctioning or malicious USB device sends an interrupt status for an
IRQ bit that has not been mapped, irq_find_mapping() will return 0.

By passing 0 directly to generic_handle_irq_safe(), wouldn't this bypass
validation and incorrectly invoke the system's handler for IRQ 0 (typically
the x86 PIT)?

Would using generic_handle_domain_irq_safe() be a safer approach?

[Severity: High]
This isn't a bug introduced by this patch, but could this callback trigger
an infinite loop of spurious interrupts if the USB device returns a short
packet?

In nct6694_usb_int_callback(), we dereference the transfer buffer:

	int_status = le32_to_cpu(*status_le);

If the USB device sends a 0-byte packet with a successful status,
urb->actual_length would be 0, and the buffer retains its previous state.

Since the bits are only cleared in the local int_status variable and
the URB is resubmitted, wouldn't the exact same interrupts be triggered
again?

Should we verify urb->actual_length is at least 4 before processing?

>  			 nct6694, int_endpoint->bInterval);
>  
> -	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
> -	if (ret)
> -		goto err_urb;
> -
>  	usb_set_intfdata(iface, nct6694);
>  
>  	ret = nct6694_core_probe(dev, nct6694, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
>  	if (ret)
> -		goto err_mfd;
> +		goto err_urb;
> +
> +	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
> +	if (ret)
> +		goto err_core;
>  
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=8

Re: [PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered

[Lee Jones]

On Fri, 21 Aug 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing validation of irq_find_mapping() return value leads to spurious system interrupts (e.g., IRQ 0).
> - [High] Missing urb->actual_length validation leads to processing stale memory and spurious interrupt storms.

Also yours.

> --
> 
> --- Patch [8]: [PATCH v7 08/10] mfd: nct6694: Submit the interrupt URB after the core is registered ---
> commit 8c344a9772877b334fe47602ee4be5e3e806ecab
> Author: Ming Yu <a0282524688@gmail.com>
> 
> mfd: nct6694: Submit the interrupt URB after the core is registered
> 
> The interrupt URB is submitted before nct6694_core_probe() creates the
> IRQ domain. A device completing the URB early makes the callback pass a
> NULL domain to irq_find_mapping(), which then falls back to the default
> domain and dispatches interrupts the device does not own.
> 
> Submit the URB once the core is registered, and kill it before the core
> is removed so the callback never runs without a domain.
> 
> > diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
> > index c8667984df5eb..cc3e7b7b3a0dc 100644
> > --- a/drivers/mfd/nct6694-usb.c
> > +++ b/drivers/mfd/nct6694-usb.c
> > @@ -312,20 +312,20 @@ static int nct6694_usb_probe(struct usb_interface *iface,
> >  			 udata->int_buffer, sizeof(*udata->int_buffer), nct6694_usb_int_callback,
> 
> [Severity: High]
> This is a pre-existing issue, but does this callback risk dispatching
> interrupts the device does not own if an unmapped IRQ is received?
> 
> Looking at the loop inside nct6694_usb_int_callback():
> 
> 	while (int_status) {
> 		int irq = __ffs(int_status);
> 
> 		generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
> 		int_status &= ~BIT(irq);
> 	}
> 
> If a malfunctioning or malicious USB device sends an interrupt status for an
> IRQ bit that has not been mapped, irq_find_mapping() will return 0.
> 
> By passing 0 directly to generic_handle_irq_safe(), wouldn't this bypass
> validation and incorrectly invoke the system's handler for IRQ 0 (typically
> the x86 PIT)?
> 
> Would using generic_handle_domain_irq_safe() be a safer approach?
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but could this callback trigger
> an infinite loop of spurious interrupts if the USB device returns a short
> packet?
> 
> In nct6694_usb_int_callback(), we dereference the transfer buffer:
> 
> 	int_status = le32_to_cpu(*status_le);
> 
> If the USB device sends a 0-byte packet with a successful status,
> urb->actual_length would be 0, and the buffer retains its previous state.
> 
> Since the bits are only cleared in the local int_status variable and
> the URB is resubmitted, wouldn't the exact same interrupts be triggered
> again?
> 
> Should we verify urb->actual_length is at least 4 before processing?
> 
> >  			 nct6694, int_endpoint->bInterval);
> >  
> > -	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
> > -	if (ret)
> > -		goto err_urb;
> > -
> >  	usb_set_intfdata(iface, nct6694);
> >  
> >  	ret = nct6694_core_probe(dev, nct6694, nct6694_usb_devs, ARRAY_SIZE(nct6694_usb_devs));
> >  	if (ret)
> > -		goto err_mfd;
> > +		goto err_urb;
> > +
> > +	ret = usb_submit_urb(udata->int_in_urb, GFP_KERNEL);
> > +	if (ret)
> > +		goto err_core;
> >  
> >  	return 0;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=8

-- 
Lee Jones

[PATCH v7 09/10] mfd: nct6694: Introduce regmap-based transport abstraction

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Sub-device drivers call into the USB transport directly, so a second
transport cannot be added without touching all of them.

Wrap the transport behind a regmap bus and let the sub-device drivers
reach the firmware only through nct6694_{read,write}_msg(). The command
header maps onto the regmap bulk accessors by packing the host control
byte, the module id and the 16-bit offset into a single 32-bit register.

Add nct6694_write_read_msg() for the commands that transmit a request
and read the reply back within the same firmware message, and use it for
the I2C deliver command.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:
- Made the USB transport helpers static and gave nct6694_usb_write_msg()
  separate @tx/@rx buffers, so a SET command only copies the firmware
  reply back when the caller asks for it.
- Documented in the shared header why nct6694_write_read_msg() is
  expressed as a read of a SET register.

Changes in v6:
- New patch. Replaces the v5 function-pointer abstraction with a
  regmap_bus based transport: the firmware command header is packed into
  a single 32-bit regmap register and sub-device drivers use the regmap
  bulk accessors. Adds nct6694_write_read_msg() for request/response
  commands and drops the per-transport access_lock (regmap already
  serialises bus accesses).

 drivers/i2c/busses/i2c-nct6694.c |   2 +-
 drivers/mfd/Kconfig              |   1 +
 drivers/mfd/nct6694-usb.c        | 106 +++++++++++++++++++------------
 include/linux/mfd/nct6694.h      |  62 ++++++++++++++----
 4 files changed, 120 insertions(+), 51 deletions(-)

diff --git a/drivers/i2c/busses/i2c-nct6694.c b/drivers/i2c/busses/i2c-nct6694.c
index ef3329f34246..7e32dab6e759 100644
--- a/drivers/i2c/busses/i2c-nct6694.c
+++ b/drivers/i2c/busses/i2c-nct6694.c
@@ -77,7 +77,7 @@ static int nct6694_i2c_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, int
 		deliver->addr = i2c_8bit_addr_from_msg(msg_temp);
 		if (msg_temp->flags & I2C_M_RD) {
 			deliver->r_cnt = msg_temp->len;
-			ret = nct6694_write_msg(data->nct6694, &cmd_hd, deliver);
+			ret = nct6694_write_read_msg(data->nct6694, &cmd_hd, deliver);
 			if (ret < 0)
 				return ret;
 
diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 5506a0adf3ec..742fc26e6ff7 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1166,6 +1166,7 @@ config MFD_MENF21BMC
 config MFD_NCT6694
 	tristate
 	select MFD_CORE
+	select REGMAP
 	help
 	  Core MFD support for the Nuvoton NCT6694 peripheral expander.
 	  This provides the common APIs and shared structures used by all
diff --git a/drivers/mfd/nct6694-usb.c b/drivers/mfd/nct6694-usb.c
index cc3e7b7b3a0d..979ae1125d16 100644
--- a/drivers/mfd/nct6694-usb.c
+++ b/drivers/mfd/nct6694-usb.c
@@ -9,6 +9,7 @@
  * CAN, WDT, HWMON and RTC management.
  */
 
+#include <linux/bitfield.h>
 #include <linux/bits.h>
 #include <linux/interrupt.h>
 #include <linux/irq.h>
@@ -17,7 +18,9 @@
 #include <linux/mfd/core.h>
 #include <linux/mfd/nct6694.h>
 #include <linux/module.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
+#include <linux/unaligned.h>
 #include <linux/usb.h>
 
 #define NCT6694_VENDOR_ID	0x0416
@@ -34,7 +37,6 @@ union __packed nct6694_usb_msg {
 };
 
 struct nct6694_usb_data {
-	struct mutex access_lock;
 	struct urb *int_in_urb;
 	struct usb_device *udev;
 	union nct6694_usb_msg *usb_msg;
@@ -102,21 +104,9 @@ static int nct6694_usb_err_handling(struct nct6694 *nct6694, unsigned char err_s
 	return -EIO;
 }
 
-/**
- * nct6694_usb_read_msg() - Read message from NCT6694 device
- * @nct6694: NCT6694 device pointer
- * @cmd_hd: command header structure
- * @buf: buffer to store the response data
- *
- * Sends a command to the NCT6694 device and reads the response.
- * The command header is specified in @cmd_hd, and the response
- * data is stored in @buf.
- *
- * Return: Negative value on error or 0 on success.
- */
-int nct6694_usb_read_msg(struct nct6694 *nct6694,
-			 const struct nct6694_cmd_header *cmd_hd,
-			 void *buf)
+static int nct6694_usb_read_msg(struct nct6694 *nct6694,
+				const struct nct6694_cmd_header *cmd_hd,
+				void *buf)
 {
 	struct nct6694_usb_data *udata = nct6694->priv;
 	union nct6694_usb_msg *msg = udata->usb_msg;
@@ -127,8 +117,6 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
 	if (len > NCT6694_MAX_PACKET_SIZE)
 		return -EINVAL;
 
-	guard(mutex)(&udata->access_lock);
-
 	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
 	msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
 
@@ -160,22 +148,15 @@ int nct6694_usb_read_msg(struct nct6694 *nct6694,
 
 	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
 }
-EXPORT_SYMBOL_GPL(nct6694_usb_read_msg);
 
-/**
- * nct6694_usb_write_msg() - Write message to NCT6694 device
- * @nct6694: NCT6694 device pointer
- * @cmd_hd: command header structure
- * @buf: buffer containing the data to be sent
- *
- * Sends a command to the NCT6694 device and writes the data
- * from @buf. The command header is specified in @cmd_hd.
- *
- * Return: Negative value on error or 0 on success.
+/*
+ * @tx is const because regmap_bus->write() hands over the caller's buffer. The
+ * firmware always answers a SET command with a payload of the same length;
+ * callers that need it pass @rx, the others pass NULL.
  */
-int nct6694_usb_write_msg(struct nct6694 *nct6694,
-			  const struct nct6694_cmd_header *cmd_hd,
-			  void *buf)
+static int nct6694_usb_write_msg(struct nct6694 *nct6694,
+				 const struct nct6694_cmd_header *cmd_hd,
+				 const void *tx, void *rx)
 {
 	struct nct6694_usb_data *udata = nct6694->priv;
 	union nct6694_usb_msg *msg = udata->usb_msg;
@@ -186,11 +167,9 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
 	if (len > NCT6694_MAX_PACKET_SIZE)
 		return -EINVAL;
 
-	guard(mutex)(&udata->access_lock);
-
 	memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd));
 	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
-	memcpy(udata->xfer_buf, buf, len);
+	memcpy(udata->xfer_buf, tx, len);
 
 	/* Send command packet to USB device */
 	ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, NCT6694_BULK_OUT_EP), &msg->cmd_header,
@@ -222,11 +201,57 @@ int nct6694_usb_write_msg(struct nct6694 *nct6694,
 		return -EIO;
 	}
 
-	memcpy(buf, udata->xfer_buf, len);
+	if (rx)
+		memcpy(rx, udata->xfer_buf, len);
 
 	return nct6694_usb_err_handling(nct6694, msg->response_header.sts);
 }
-EXPORT_SYMBOL_GPL(nct6694_usb_write_msg);
+
+static int nct6694_usb_regmap_read(void *context, const void *reg_buf,
+				   size_t reg_size, void *val_buf,
+				   size_t val_size)
+{
+	struct nct6694 *nct6694 = context;
+	u32 reg = get_unaligned_be32(reg_buf);
+	const struct nct6694_cmd_header cmd_hd = {
+		.mod = FIELD_GET(NCT6694_REG_MOD, reg),
+		.offset = cpu_to_le16(FIELD_GET(NCT6694_REG_OFFSET, reg)),
+		.len = cpu_to_le16(val_size),
+	};
+
+	if (FIELD_GET(NCT6694_REG_HCTRL, reg) == NCT6694_HCTRL_SET)
+		return nct6694_usb_write_msg(nct6694, &cmd_hd, val_buf, val_buf);
+
+	return nct6694_usb_read_msg(nct6694, &cmd_hd, val_buf);
+}
+
+static int nct6694_usb_regmap_write(void *context, const void *data,
+				    size_t count)
+{
+	struct nct6694 *nct6694 = context;
+	u32 reg = get_unaligned_be32(data);
+	size_t len = count - sizeof(reg);
+	const struct nct6694_cmd_header cmd_hd = {
+		.mod = FIELD_GET(NCT6694_REG_MOD, reg),
+		.offset = cpu_to_le16(FIELD_GET(NCT6694_REG_OFFSET, reg)),
+		.len = cpu_to_le16(len),
+	};
+
+	return nct6694_usb_write_msg(nct6694, &cmd_hd, data + sizeof(reg), NULL);
+}
+
+static const struct regmap_bus nct6694_usb_regmap_bus = {
+	.read = nct6694_usb_regmap_read,
+	.write = nct6694_usb_regmap_write,
+};
+
+static const struct regmap_config nct6694_usb_regmap_config = {
+	.reg_bits = 32,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_raw_read = NCT6694_MAX_PACKET_SIZE,
+	.max_raw_write = NCT6694_MAX_PACKET_SIZE,
+};
 
 static void nct6694_usb_int_callback(struct urb *urb)
 {
@@ -300,9 +325,12 @@ static int nct6694_usb_probe(struct usb_interface *iface,
 	nct6694->dev = dev;
 	nct6694->priv = udata;
 
-	ret = devm_mutex_init(dev, &udata->access_lock);
-	if (ret)
+	nct6694->regmap = devm_regmap_init(dev, &nct6694_usb_regmap_bus, nct6694,
+					   &nct6694_usb_regmap_config);
+	if (IS_ERR(nct6694->regmap)) {
+		ret = PTR_ERR(nct6694->regmap);
 		goto err_urb;
+	}
 
 	ret = usb_find_int_in_endpoint(iface->cur_altsetting, &int_endpoint);
 	if (ret)
diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h
index cb311e58a437..aef0f9bd914c 100644
--- a/include/linux/mfd/nct6694.h
+++ b/include/linux/mfd/nct6694.h
@@ -9,7 +9,9 @@
 #ifndef __MFD_NCT6694_H
 #define __MFD_NCT6694_H
 
+#include <linux/bitfield.h>
 #include <linux/idr.h>
+#include <linux/regmap.h>
 #include <linux/spinlock.h>
 #include <linux/types.h>
 
@@ -90,6 +92,7 @@ struct __packed nct6694_response_header {
 
 struct nct6694 {
 	struct device *dev;
+	struct regmap *regmap;
 	struct ida gpio_ida;
 	struct ida i2c_ida;
 	struct ida canfd_ida;
@@ -100,29 +103,66 @@ struct nct6694 {
 	void *priv;
 };
 
-int nct6694_core_probe(struct device *dev, struct nct6694 *nct6694,
-		       const struct mfd_cell *cells, int n_cells);
-void nct6694_core_remove(struct nct6694 *nct6694);
+/*
+ * Firmware messages are addressed by a module id and a 16-bit offset (a
+ * command/selector pair). Pack them together with the host control byte into a
+ * single 32-bit regmap register, so that sub-device drivers can issue commands
+ * through the regmap bulk accessors while each transport driver only has to
+ * implement a regmap bus.
+ *
+ *   bits [31:24]  host control (NCT6694_HCTRL_GET / NCT6694_HCTRL_SET)
+ *   bits [23:16]  module id
+ *   bits [15:0]   offset (low byte = command, high byte = selector)
+ */
+#define NCT6694_REG_HCTRL	GENMASK(31, 24)
+#define NCT6694_REG_MOD		GENMASK(23, 16)
+#define NCT6694_REG_OFFSET	GENMASK(15, 0)
 
-int nct6694_usb_read_msg(struct nct6694 *nct6694,
-			 const struct nct6694_cmd_header *cmd_hd,
-			 void *buf);
-int nct6694_usb_write_msg(struct nct6694 *nct6694,
-			  const struct nct6694_cmd_header *cmd_hd,
-			  void *buf);
+static inline u32 nct6694_cmd_to_reg(const struct nct6694_cmd_header *cmd_hd,
+				     u8 hctrl)
+{
+	return FIELD_PREP(NCT6694_REG_HCTRL, hctrl) |
+	       FIELD_PREP(NCT6694_REG_MOD, cmd_hd->mod) |
+	       FIELD_PREP(NCT6694_REG_OFFSET, le16_to_cpu(cmd_hd->offset));
+}
 
 static inline int nct6694_read_msg(struct nct6694 *nct6694,
 				   const struct nct6694_cmd_header *cmd_hd,
 				   void *buf)
 {
-	return nct6694_usb_read_msg(nct6694, cmd_hd, buf);
+	return regmap_bulk_read(nct6694->regmap,
+				nct6694_cmd_to_reg(cmd_hd, NCT6694_HCTRL_GET),
+				buf, le16_to_cpu(cmd_hd->len));
 }
 
 static inline int nct6694_write_msg(struct nct6694 *nct6694,
 				    const struct nct6694_cmd_header *cmd_hd,
 				    void *buf)
 {
-	return nct6694_usb_write_msg(nct6694, cmd_hd, buf);
+	return regmap_bulk_write(nct6694->regmap,
+				 nct6694_cmd_to_reg(cmd_hd, NCT6694_HCTRL_SET),
+				 buf, le16_to_cpu(cmd_hd->len));
 }
 
+/*
+ * A few commands, such as the I2C deliver, transmit a request and read the
+ * reply back within the same firmware message. regmap has no accessor for such
+ * an exchange, so express it as a read of a SET register: @buf carries the
+ * request on entry and holds the reply on return. This relies on the transport
+ * bus being handed @buf directly, which holds as long as the regmap is left
+ * uncached and byte sized.
+ */
+static inline int nct6694_write_read_msg(struct nct6694 *nct6694,
+					 const struct nct6694_cmd_header *cmd_hd,
+					 void *buf)
+{
+	return regmap_bulk_read(nct6694->regmap,
+				nct6694_cmd_to_reg(cmd_hd, NCT6694_HCTRL_SET),
+				buf, le16_to_cpu(cmd_hd->len));
+}
+
+int nct6694_core_probe(struct device *dev, struct nct6694 *nct6694,
+		       const struct mfd_cell *cells, int n_cells);
+void nct6694_core_remove(struct nct6694 *nct6694);
+
 #endif
-- 
2.34.1

Re: [PATCH v7 09/10] mfd: nct6694: Introduce regmap-based transport abstraction

[sashiko-bot]

> From: Ming Yu <a0282524688@gmail.com>
> 
> Sub-device drivers call into the USB transport directly, so a second
> transport cannot be added without touching all of them.
> 
> Wrap the transport behind a regmap bus and let the sub-device drivers
> reach the firmware only through nct6694_{read,write}_msg(). The command
> header maps onto the regmap bulk accessors by packing the host control
> byte, the module id and the 16-bit offset into a single 32-bit register.
> 
> Add nct6694_write_read_msg() for the commands that transmit a request
> and read the reply back within the same firmware message, and use it for
> the I2C deliver command.
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>

Sashiko has reviewed this patch and found no issues. It looks great!

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=9

[PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver

[a0282524688]

From: Ming Yu <a0282524688@gmail.com>

Add support for the Host Interface (HIF) transport via eSPI for the
Nuvoton NCT6694 peripheral expander.

This transport driver initializes the Super-I/O to configure the
device's shared memory base address and SIRQ. It provides a regmap_bus
implementation that drives the firmware command/response exchange over
the shared-memory window, plus an internal regmap_mmio for the report
region. The initialized device config is then passed to the MFD core
(nct6694_core_probe) for sub-device registration and IRQ domain setup.

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
Changes in v7:
- Zero the tail of the caller buffer when the firmware answers with
  fewer bytes than requested, and split the transfer helper into
  separate @tx/@rx buffers instead of a shared scratch buffer.
- Request the interrupt before registering the core, so devres frees it
  after the sub-devices are removed. Mask the interrupt and clear a
  stale status in nct6694_hif_irq_init() so the handler cannot run
  before the IRQ domain exists.
- Return IRQ_HANDLED once the interrupt status has been cleared.
- Skip hardware interrupts that have no mapping instead of dispatching
  virq 0.
- Bail out with -ENODEV if the BIOS left the shared memory base address
  unconfigured.
- Program the SIRQ register with the hardware interrupt number rather
  than the Linux virq.

Changes in v6:
- Reworked the transport to implement a regmap_bus instead of the v5
  read_msg/write_msg function pointers.

Changes in v5:
- Split from the monolithic v4 patch.
- Adapted to re-use the newly introduced nct6694_core_probe() and
  abstracted I/O APIs.

Changes since version 3:
- Remove redundant module type macro definitions from sub-device drivers
  that are now provided by the shared header <linux/mfd/nct6694.h>,
  fixing -Wmacro-redefined warnings.

Changes since version 2:
- Restore per-device IDA and mfd_add_hotplug_devices()/PLATFORM_DEVID_AUTO
  to avoid child device ID conflicts with multiple NCT6694 chips.
- Validate irq_find_mapping() return value before dispatching IRQs.
- Check superio_enter() return value in nct6694_irq_init().

Changes since version 1:
- Drop function pointers from Super-I/O access and use static inline
  helpers with proper types.

 drivers/mfd/Kconfig       |  16 ++
 drivers/mfd/Makefile      |   1 +
 drivers/mfd/nct6694-hif.c | 590 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 607 insertions(+)
 create mode 100644 drivers/mfd/nct6694-hif.c

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 742fc26e6ff7..f37acf585671 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -1175,6 +1175,22 @@ config MFD_NCT6694
 
 	  It is selected automatically by the transport interface drivers.
 
+config MFD_NCT6694_HIF
+	tristate "Nuvoton NCT6694 HIF (eSPI) interface support"
+	depends on HAS_IOPORT && ACPI
+	select MFD_NCT6694
+	select REGMAP_MMIO
+	help
+	  This enables support for the Nuvoton NCT6694 peripheral expander
+	  connected via the Host Interface (HIF) using eSPI transport.
+
+	  The transport driver uses Super-I/O mapping and shared memory to
+	  communicate with the NCT6694 firmware. Enable this option if you
+	  are using the NCT6694 over an eSPI interface on an ACPI platform.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called nct6694-hif.
+
 config MFD_NCT6694_USB
 	tristate "Nuvoton NCT6694 USB interface support"
 	select MFD_NCT6694
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
index 10c19a19541e..97924dd3552c 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -125,6 +125,7 @@ obj-$(CONFIG_MFD_MC13XXX_I2C)	+= mc13xxx-i2c.o
 obj-$(CONFIG_MFD_PF1550)	+= pf1550.o
 
 obj-$(CONFIG_MFD_NCT6694)	+= nct6694-core.o
+obj-$(CONFIG_MFD_NCT6694_HIF)	+= nct6694-hif.o
 obj-$(CONFIG_MFD_NCT6694_USB)	+= nct6694-usb.o
 
 obj-$(CONFIG_MFD_CORE)		+= mfd-core.o
diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c
new file mode 100644
index 000000000000..198838d3e7d8
--- /dev/null
+++ b/drivers/mfd/nct6694-hif.c
@@ -0,0 +1,590 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2026 Nuvoton Technology Corp.
+ *
+ * Nuvoton NCT6694 host-interface (eSPI) transport driver.
+ */
+
+#include <linux/acpi.h>
+#include <linux/bitfield.h>
+#include <linux/bits.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/irq.h>
+#include <linux/irqdomain.h>
+#include <linux/kernel.h>
+#include <linux/mfd/core.h>
+#include <linux/mfd/nct6694.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/unaligned.h>
+
+#define DRVNAME "nct6694-hif"
+
+#define NCT6694_POLL_INTERVAL_US	10
+#define NCT6694_POLL_TIMEOUT_US		10000
+
+/*
+ * Super-I/O registers
+ */
+#define SIO_REG_LDSEL		0x07	/* Logical device select */
+#define SIO_REG_DEVID		0x20	/* Device ID (2 bytes) */
+#define SIO_REG_LD_SHM		0x0F	/* Logical device shared memory control */
+
+#define SIO_REG_SHM_ENABLE	0x30	/* Enable shared memory */
+#define SIO_REG_SHM_BASE_ADDR	0x60	/* Shared memory base address (2 bytes) */
+#define SIO_REG_SHM_IRQ_NR	0x70	/* Shared memory interrupt number */
+#define SIO_SHM_IRQ_NR_MAX	15	/* Highest ISA interrupt line */
+
+#define SIO_REG_UNLOCK_KEY	0x87	/* Key to enable Super-I/O */
+#define SIO_REG_LOCK_KEY	0xAA	/* Key to disable Super-I/O */
+
+#define SIO_NCT6694B_ID		0xD029
+#define SIO_NCT6694D_ID		0x5832
+
+/*
+ * Super-I/O Shared Memory Logical Device registers
+ */
+#define NCT6694_SHM_COFS_STS			0x2E
+#define NCT6694_SHM_COFS_STS_COFS4W		BIT(7)
+
+#define NCT6694_SHM_COFS_CTL2			0x3B
+#define NCT6694_SHM_COFS_CTL2_COFS4W_IE		BIT(3)
+
+/* COFS register block [STS..CTL2] is the only SHM range driven via inb/outb */
+#define NCT6694_SHM_COFS_LEN			\
+	(NCT6694_SHM_COFS_CTL2 - NCT6694_SHM_COFS_STS + 1)
+
+#define NCT6694_SHM_INTR_STATUS			0x9C	/* Interrupt status register (4 bytes) */
+
+enum nct6694_chips {
+	NCT6694B = 0,
+	NCT6694D,
+};
+
+struct __packed nct6694_hif_msg {
+	struct nct6694_cmd_header cmd_header;
+	struct nct6694_response_header response_header;
+	unsigned char data[];
+};
+
+struct nct6694_sio_data {
+	enum nct6694_chips chip;
+	int sioreg;	/* Super-I/O index port */
+};
+
+struct nct6694_hif_data {
+	struct regmap *rpt_regmap;
+	struct nct6694_sio_data *sio_data;
+	void __iomem *msg_base;
+	unsigned int shm_base;
+};
+
+static const char * const nct6694_chip_names[] = {
+	[NCT6694B] = "NCT6694B",
+	[NCT6694D] = "NCT6694D",
+};
+
+/*
+ * Super-I/O functions.
+ */
+static inline int superio_enter(struct nct6694_sio_data *sio_data)
+{
+	int ioreg = sio_data->sioreg;
+
+	/*
+	 * Try to reserve <ioreg> and <ioreg + 1> for exclusive access.
+	 */
+	if (!request_muxed_region(ioreg, 2, DRVNAME))
+		return -EBUSY;
+
+	outb(SIO_REG_UNLOCK_KEY, ioreg);
+	outb(SIO_REG_UNLOCK_KEY, ioreg);
+
+	return 0;
+}
+
+static inline void superio_exit(struct nct6694_sio_data *sio_data)
+{
+	int ioreg = sio_data->sioreg;
+
+	outb(SIO_REG_LOCK_KEY, ioreg);
+
+	release_region(ioreg, 2);
+}
+
+static inline void superio_select(struct nct6694_sio_data *sio_data, int ld)
+{
+	int ioreg = sio_data->sioreg;
+
+	outb(SIO_REG_LDSEL, ioreg);
+	outb(ld, ioreg + 1);
+}
+
+static inline int superio_inb(struct nct6694_sio_data *sio_data, int reg)
+{
+	int ioreg = sio_data->sioreg;
+
+	outb(reg, ioreg);
+	return inb(ioreg + 1);
+}
+
+static inline int superio_inw(struct nct6694_sio_data *sio_data, int reg)
+{
+	int ioreg = sio_data->sioreg;
+	int val;
+
+	outb(reg++, ioreg);
+	val = inb(ioreg + 1) << 8;
+	outb(reg, ioreg);
+	val |= inb(ioreg + 1);
+
+	return val;
+}
+
+static inline void superio_outb(struct nct6694_sio_data *sio_data, int reg, u8 val)
+{
+	int ioreg = sio_data->sioreg;
+
+	outb(reg, ioreg);
+	outb(val, ioreg + 1);
+}
+
+static int nct6694_sio_find(struct nct6694_sio_data *sio_data, u8 sioreg)
+{
+	int ret;
+	u16 devid;
+
+	sio_data->sioreg = sioreg;
+
+	ret = superio_enter(sio_data);
+	if (ret)
+		return ret;
+
+	/* Check Chip ID */
+	devid = superio_inw(sio_data, SIO_REG_DEVID);
+	switch (devid) {
+	case SIO_NCT6694B_ID:
+		sio_data->chip = NCT6694B;
+		break;
+	case SIO_NCT6694D_ID:
+		sio_data->chip = NCT6694D;
+		break;
+	default:
+		superio_exit(sio_data);
+		return -ENODEV;
+	}
+
+	superio_exit(sio_data);
+
+	return 0;
+}
+
+static const struct mfd_cell nct6694_hif_devs[] = {
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+	MFD_CELL_NAME("nct6694-gpio"),
+
+	MFD_CELL_NAME("nct6694-i2c"),
+	MFD_CELL_NAME("nct6694-i2c"),
+	MFD_CELL_NAME("nct6694-i2c"),
+	MFD_CELL_NAME("nct6694-i2c"),
+	MFD_CELL_NAME("nct6694-i2c"),
+	MFD_CELL_NAME("nct6694-i2c"),
+
+	MFD_CELL_NAME("nct6694-canfd"),
+	MFD_CELL_NAME("nct6694-canfd"),
+};
+
+static int nct6694_hif_err_handling(struct nct6694 *nct6694, unsigned char err_status)
+{
+	switch (err_status) {
+	case NCT6694_NO_ERROR:
+		return 0;
+	case NCT6694_NOT_SUPPORT_ERROR:
+		dev_err(nct6694->dev, "Command is not supported!\n");
+		break;
+	case NCT6694_NO_RESPONSE_ERROR:
+		dev_warn(nct6694->dev, "Command received no response!\n");
+		break;
+	case NCT6694_TIMEOUT_ERROR:
+		dev_warn(nct6694->dev, "Command timed out!\n");
+		break;
+	case NCT6694_PENDING:
+		dev_err(nct6694->dev, "Command is pending!\n");
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return -EIO;
+}
+
+/*
+ * @tx is const because regmap_bus->write() hands over the caller's buffer. A
+ * GET command carries no request payload and passes @tx as NULL; callers that
+ * need the response pass @rx, the others pass NULL.
+ */
+static int nct6694_hif_xfer_msg(struct nct6694 *nct6694,
+				const struct nct6694_cmd_header *cmd_hd,
+				u8 hctrl, const void *tx, void *rx)
+{
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	void __iomem *hdr = hdata->msg_base + offsetof(struct nct6694_hif_msg, cmd_header);
+	void __iomem *payload = hdata->msg_base + offsetof(struct nct6694_hif_msg, data);
+	struct nct6694_cmd_header cmd = *cmd_hd;
+	struct nct6694_response_header resp;
+	u16 len = le16_to_cpu(cmd.len);
+	u16 resp_len;
+	u8 status;
+	int ret;
+
+	if (len > NCT6694_MAX_PACKET_SIZE)
+		return -EINVAL;
+
+	/* Wait until the previous command is completed */
+	ret = readb_poll_timeout(hdr + offsetof(struct nct6694_cmd_header, hctrl),
+				 status, status == 0, NCT6694_POLL_INTERVAL_US,
+				 NCT6694_POLL_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	/*
+	 * Write cmd header fields, but skip hctrl - writing to it triggers
+	 * firmware command processing and must be deferred until data is ready.
+	 */
+	memcpy_toio(hdr, &cmd, offsetof(struct nct6694_cmd_header, hctrl));
+	memcpy_toio(hdr + offsetof(struct nct6694_cmd_header, rsv2), &cmd.rsv2,
+		    sizeof(cmd) - offsetof(struct nct6694_cmd_header, rsv2));
+
+	if (tx)
+		memcpy_toio(payload, tx, len);
+
+	/* Write hctrl last to trigger command processing */
+	writeb(hctrl, hdr + offsetof(struct nct6694_cmd_header, hctrl));
+
+	ret = readb_poll_timeout(hdr + offsetof(struct nct6694_cmd_header, hctrl),
+				 status, status == 0, NCT6694_POLL_INTERVAL_US,
+				 NCT6694_POLL_TIMEOUT_US);
+	if (ret)
+		return ret;
+
+	memcpy_fromio(&resp, hdata->msg_base + offsetof(struct nct6694_hif_msg, response_header),
+		      sizeof(resp));
+
+	ret = nct6694_hif_err_handling(nct6694, resp.sts);
+	if (ret)
+		return ret;
+
+	if (!rx)
+		return 0;
+
+	/* The firmware may answer with fewer bytes than requested */
+	resp_len = min(len, le16_to_cpu(resp.len));
+	memcpy_fromio(rx, payload, resp_len);
+	memset(rx + resp_len, 0, len - resp_len);
+
+	return 0;
+}
+
+static int nct6694_hif_regmap_read(void *context, const void *reg_buf,
+				   size_t reg_size, void *val_buf,
+				   size_t val_size)
+{
+	struct nct6694 *nct6694 = context;
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	u32 reg = get_unaligned_be32(reg_buf);
+	u8 hctrl = FIELD_GET(NCT6694_REG_HCTRL, reg);
+	const struct nct6694_cmd_header cmd_hd = {
+		.mod = FIELD_GET(NCT6694_REG_MOD, reg),
+		.offset = cpu_to_le16(FIELD_GET(NCT6694_REG_OFFSET, reg)),
+		.len = cpu_to_le16(val_size),
+	};
+
+	if (cmd_hd.mod == NCT6694_RPT_MOD)
+		return regmap_bulk_read(hdata->rpt_regmap,
+					FIELD_GET(NCT6694_REG_OFFSET, reg),
+					val_buf, val_size);
+
+	return nct6694_hif_xfer_msg(nct6694, &cmd_hd, hctrl,
+				    hctrl == NCT6694_HCTRL_SET ? val_buf : NULL,
+				    val_buf);
+}
+
+static int nct6694_hif_regmap_write(void *context, const void *data,
+				    size_t count)
+{
+	struct nct6694 *nct6694 = context;
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	u32 reg = get_unaligned_be32(data);
+	size_t len = count - sizeof(reg);
+	const struct nct6694_cmd_header cmd_hd = {
+		.mod = FIELD_GET(NCT6694_REG_MOD, reg),
+		.offset = cpu_to_le16(FIELD_GET(NCT6694_REG_OFFSET, reg)),
+		.len = cpu_to_le16(len),
+	};
+
+	if (cmd_hd.mod == NCT6694_RPT_MOD)
+		return regmap_bulk_write(hdata->rpt_regmap,
+					 FIELD_GET(NCT6694_REG_OFFSET, reg),
+					 data + sizeof(reg), len);
+
+	return nct6694_hif_xfer_msg(nct6694, &cmd_hd, NCT6694_HCTRL_SET,
+				    data + sizeof(reg), NULL);
+}
+
+static const struct regmap_bus nct6694_hif_regmap_bus = {
+	.read = nct6694_hif_regmap_read,
+	.write = nct6694_hif_regmap_write,
+};
+
+static const struct regmap_config nct6694_hif_msg_regmap_config = {
+	.name = "msg",
+	.reg_bits = 32,
+	.val_bits = 8,
+	.reg_stride = 1,
+	.max_raw_read = NCT6694_MAX_PACKET_SIZE,
+	.max_raw_write = NCT6694_MAX_PACKET_SIZE,
+};
+
+static const struct regmap_config nct6694_hif_rpt_regmap_config = {
+	.name = "rpt",
+	.reg_bits = 8,
+	.val_bits = 8,
+	.reg_stride = 1,
+};
+
+static irqreturn_t nct6694_hif_irq_handler(int irq, void *data)
+{
+	struct nct6694 *nct6694 = data;
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	u8 reg_data[4];
+	u32 intr_status;
+	int ret;
+
+	/* Check interrupt status is set */
+	if (!(inb(hdata->shm_base + NCT6694_SHM_COFS_STS) & NCT6694_SHM_COFS_STS_COFS4W))
+		return IRQ_NONE;
+
+	/* Clear interrupt status */
+	outb(NCT6694_SHM_COFS_STS_COFS4W, hdata->shm_base + NCT6694_SHM_COFS_STS);
+
+	ret = regmap_bulk_read(hdata->rpt_regmap, NCT6694_SHM_INTR_STATUS,
+			       reg_data, ARRAY_SIZE(reg_data));
+	if (ret)
+		return IRQ_HANDLED;
+
+	intr_status = get_unaligned_le32(reg_data);
+
+	while (intr_status) {
+		int hwirq = __ffs(intr_status);
+		unsigned int virq = irq_find_mapping(nct6694->domain, hwirq);
+
+		if (virq)
+			generic_handle_irq_safe(virq);
+
+		intr_status &= ~BIT(hwirq);
+	}
+
+	return IRQ_HANDLED;
+}
+
+static void nct6694_hif_irq_disable(void *data)
+{
+	struct nct6694 *nct6694 = data;
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	u8 cofs_ctl2;
+
+	/* Disable SIRQ interrupt */
+	cofs_ctl2 = inb(hdata->shm_base + NCT6694_SHM_COFS_CTL2);
+	cofs_ctl2 &= ~NCT6694_SHM_COFS_CTL2_COFS4W_IE;
+	outb(cofs_ctl2, hdata->shm_base + NCT6694_SHM_COFS_CTL2);
+}
+
+static void nct6694_hif_irq_enable(struct nct6694 *nct6694)
+{
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	u8 cofs_ctl2;
+
+	/* Enable SIRQ interrupt */
+	cofs_ctl2 = inb(hdata->shm_base + NCT6694_SHM_COFS_CTL2);
+	cofs_ctl2 |= NCT6694_SHM_COFS_CTL2_COFS4W_IE;
+	outb(cofs_ctl2, hdata->shm_base + NCT6694_SHM_COFS_CTL2);
+}
+
+static int nct6694_hif_irq_init(struct nct6694 *nct6694, int irq)
+{
+	struct nct6694_hif_data *hdata = nct6694->priv;
+	struct nct6694_sio_data *sio_data = hdata->sio_data;
+	struct irq_data *irq_data;
+	irq_hw_number_t hwirq;
+	int ret;
+
+	/* The chip is programmed with the SIRQ line, not the Linux irq number */
+	irq_data = irq_get_irq_data(irq);
+	if (!irq_data)
+		return -EINVAL;
+
+	hwirq = irqd_to_hwirq(irq_data);
+	if (hwirq > SIO_SHM_IRQ_NR_MAX)
+		return -ERANGE;
+
+	/* Set SIRQ number */
+	ret = superio_enter(sio_data);
+	if (ret)
+		return ret;
+
+	superio_select(sio_data, SIO_REG_LD_SHM);
+
+	if (!superio_inb(sio_data, SIO_REG_SHM_ENABLE)) {
+		superio_exit(sio_data);
+		return -EIO;
+	}
+
+	hdata->shm_base = superio_inw(sio_data, SIO_REG_SHM_BASE_ADDR);
+	if (!hdata->shm_base) {
+		superio_exit(sio_data);
+		return -ENODEV;
+	}
+
+	superio_outb(sio_data, SIO_REG_SHM_IRQ_NR, hwirq);
+
+	superio_exit(sio_data);
+
+	if (!devm_request_region(nct6694->dev,
+				 hdata->shm_base + NCT6694_SHM_COFS_STS,
+				 NCT6694_SHM_COFS_LEN, DRVNAME))
+		return -EBUSY;
+
+	/* Keep the device quiet until the IRQ domain is ready */
+	nct6694_hif_irq_disable(nct6694);
+	outb(NCT6694_SHM_COFS_STS_COFS4W, hdata->shm_base + NCT6694_SHM_COFS_STS);
+
+	return 0;
+}
+
+static void nct6694_hif_core_remove_action(void *data)
+{
+	struct nct6694 *nct6694 = data;
+
+	nct6694_core_remove(nct6694);
+}
+
+static const u8 sio_addrs[] = { 0x2e, 0x4e };
+
+static int nct6694_hif_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct nct6694_sio_data *sio_data;
+	struct nct6694_hif_data *hdata;
+	struct nct6694 *nct6694;
+	void __iomem *rpt_base, *msg_base;
+	int ret, i, irq;
+
+	rpt_base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(rpt_base))
+		return PTR_ERR(rpt_base);
+
+	msg_base = devm_platform_ioremap_resource(pdev, 1);
+	if (IS_ERR(msg_base))
+		return PTR_ERR(msg_base);
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return irq;
+
+	sio_data = devm_kzalloc(dev, sizeof(*sio_data), GFP_KERNEL);
+	if (!sio_data)
+		return -ENOMEM;
+
+	for (i = 0; i < ARRAY_SIZE(sio_addrs); i++) {
+		if (!nct6694_sio_find(sio_data, sio_addrs[i]))
+			break;
+	}
+	if (i == ARRAY_SIZE(sio_addrs))
+		return -ENODEV;
+
+	dev_dbg(dev, "Found %s at %#x\n", nct6694_chip_names[sio_data->chip], sio_data->sioreg);
+
+	nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL);
+	if (!nct6694)
+		return -ENOMEM;
+
+	hdata = devm_kzalloc(dev, sizeof(*hdata), GFP_KERNEL);
+	if (!hdata)
+		return -ENOMEM;
+
+	hdata->sio_data = sio_data;
+	hdata->msg_base = msg_base;
+	hdata->rpt_regmap = devm_regmap_init_mmio(dev, rpt_base,
+						  &nct6694_hif_rpt_regmap_config);
+	if (IS_ERR(hdata->rpt_regmap))
+		return PTR_ERR(hdata->rpt_regmap);
+
+	nct6694->dev = dev;
+	nct6694->priv = hdata;
+	nct6694->regmap = devm_regmap_init(dev, &nct6694_hif_regmap_bus, nct6694,
+					   &nct6694_hif_msg_regmap_config);
+	if (IS_ERR(nct6694->regmap))
+		return PTR_ERR(nct6694->regmap);
+
+	ret = nct6694_hif_irq_init(nct6694, irq);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, nct6694);
+
+	/* Request the interrupt first so that devres releases it last */
+	ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_hif_irq_handler,
+					IRQF_ONESHOT | IRQF_SHARED,
+					dev_name(dev), nct6694);
+	if (ret)
+		return ret;
+
+	ret = nct6694_core_probe(dev, nct6694, nct6694_hif_devs,
+				 ARRAY_SIZE(nct6694_hif_devs));
+	if (ret)
+		return ret;
+
+	ret = devm_add_action_or_reset(dev, nct6694_hif_core_remove_action, nct6694);
+	if (ret)
+		return ret;
+
+	nct6694_hif_irq_enable(nct6694);
+
+	return devm_add_action_or_reset(dev, nct6694_hif_irq_disable, nct6694);
+}
+
+static const struct acpi_device_id nct6694_hif_acpi_ids[] = {
+	{ "NTN0538", 0 },
+	{}
+};
+MODULE_DEVICE_TABLE(acpi, nct6694_hif_acpi_ids);
+
+static struct platform_driver nct6694_hif_driver = {
+	.driver = {
+		.name = DRVNAME,
+		.acpi_match_table = nct6694_hif_acpi_ids,
+	},
+	.probe	= nct6694_hif_probe,
+};
+module_platform_driver(nct6694_hif_driver);
+
+MODULE_DESCRIPTION("Nuvoton NCT6694 host-interface transport driver");
+MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>");
+MODULE_LICENSE("GPL");
-- 
2.34.1

Re: [PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Requesting a shared, oneshot threaded IRQ without a primary handler will be rejected by the IRQ core and degrade system performance if bypassed.
- [High] The IRQ handler is unregistered after the IRQ domain is destroyed, creating a window for a Use-After-Free of the domain pointer during teardown.
--

--- Patch [10]: [PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver ---
commit fca991565f4ea259d4b5653b4e67f56e5a45bdd9
Author: Ming Yu <a0282524688@gmail.com>

mfd: nct6694: Add Host Interface (HIF) eSPI transport driver

This commit adds support for the Host Interface (HIF) transport via eSPI
for the Nuvoton NCT6694 peripheral expander. It initializes the Super-I/O
and provides a regmap_bus implementation.

> diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c
> new file mode 100644
> index 0000000000000..198838d3e7d87
> --- /dev/null
> +++ b/drivers/mfd/nct6694-hif.c
[ ... ]
> @@ -546,12 +546,12 @@ static int nct6694_hif_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, nct6694);
>  
>  	/* Request the interrupt first so that devres releases it last */
>  	ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_hif_irq_handler,
>  					IRQF_ONESHOT | IRQF_SHARED,
>  					dev_name(dev), nct6694);

[Severity: High]
Will this probe unconditionally fail with -EINVAL?

The IRQ subsystem typically rejects devm_request_threaded_irq() with
IRQF_SHARED | IRQF_ONESHOT if there is no primary handler, because it cannot
determine if the interrupt originated from this device before masking the
shared line.

[Severity: High]
Can this devres ordering lead to a use-after-free of nct6694->domain
during driver unbind?

Because the threaded IRQ is requested before the core is probed, devres will
release it last. During unbind, devres will execute actions in reverse order:

1. nct6694_hif_irq_disable() executes, masking the interrupt but not clearing
   pending status.
2. nct6694_hif_core_remove_action() executes, destroying the IRQ domain:

drivers/mfd/nct6694-core.c:nct6694_core_remove() {
    ...
    irq_domain_remove(nct6694->domain);
    ...
}

If another device sharing this IRQ line triggers an interrupt before devres
finally unregisters this handler, won't nct6694_hif_irq_handler() execute,
see a pending status, and dereference the freed nct6694->domain in
irq_find_mapping()?

>  	if (ret)
>  		return ret;
>  
>  	ret = nct6694_core_probe(dev, nct6694, nct6694_hif_devs,
>  				 ARRAY_SIZE(nct6694_hif_devs));
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=10

Re: [PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver

[Lee Jones]

On Fri, 21 Aug 2026, sashiko-bot@kernel.org wrote:

> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> - [High] Requesting a shared, oneshot threaded IRQ without a primary handler will be rejected by the IRQ core and degrade system performance if bypassed.
> - [High] The IRQ handler is unregistered after the IRQ domain is destroyed, creating a window for a Use-After-Free of the domain pointer during teardown.

These either need addressing or comments from you.

> --
> 
> --- Patch [10]: [PATCH v7 10/10] mfd: nct6694: Add Host Interface (HIF) eSPI transport driver ---
> commit fca991565f4ea259d4b5653b4e67f56e5a45bdd9
> Author: Ming Yu <a0282524688@gmail.com>
> 
> mfd: nct6694: Add Host Interface (HIF) eSPI transport driver
> 
> This commit adds support for the Host Interface (HIF) transport via eSPI
> for the Nuvoton NCT6694 peripheral expander. It initializes the Super-I/O
> and provides a regmap_bus implementation.
> 
> > diff --git a/drivers/mfd/nct6694-hif.c b/drivers/mfd/nct6694-hif.c
> > new file mode 100644
> > index 0000000000000..198838d3e7d87
> > --- /dev/null
> > +++ b/drivers/mfd/nct6694-hif.c
> [ ... ]
> > @@ -546,12 +546,12 @@ static int nct6694_hif_probe(struct platform_device *pdev)
> >  	platform_set_drvdata(pdev, nct6694);
> >  
> >  	/* Request the interrupt first so that devres releases it last */
> >  	ret = devm_request_threaded_irq(dev, irq, NULL, nct6694_hif_irq_handler,
> >  					IRQF_ONESHOT | IRQF_SHARED,
> >  					dev_name(dev), nct6694);
> 
> [Severity: High]
> Will this probe unconditionally fail with -EINVAL?
> 
> The IRQ subsystem typically rejects devm_request_threaded_irq() with
> IRQF_SHARED | IRQF_ONESHOT if there is no primary handler, because it cannot
> determine if the interrupt originated from this device before masking the
> shared line.
> 
> [Severity: High]
> Can this devres ordering lead to a use-after-free of nct6694->domain
> during driver unbind?
> 
> Because the threaded IRQ is requested before the core is probed, devres will
> release it last. During unbind, devres will execute actions in reverse order:
> 
> 1. nct6694_hif_irq_disable() executes, masking the interrupt but not clearing
>    pending status.
> 2. nct6694_hif_core_remove_action() executes, destroying the IRQ domain:
> 
> drivers/mfd/nct6694-core.c:nct6694_core_remove() {
>     ...
>     irq_domain_remove(nct6694->domain);
>     ...
> }
> 
> If another device sharing this IRQ line triggers an interrupt before devres
> finally unregisters this handler, won't nct6694_hif_irq_handler() execute,
> see a pending status, and dereference the freed nct6694->domain in
> irq_find_mapping()?
> 
> >  	if (ret)
> >  		return ret;
> >  
> >  	ret = nct6694_core_probe(dev, nct6694, nct6694_hif_devs,
> >  				 ARRAY_SIZE(nct6694_hif_devs));
> >  	if (ret)
> >  		return ret;
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260821033505.4017901-1-a0282524688@gmail.com?part=10

-- 
Lee Jones