On Mon, 17 Mar 2025, Ming Yu wrote:

Dear Lee,

Thank you for reviewing,

Lee Jones [off-list ref] 於 2025年3月7日 週五 上午9:15寫道：

quoted

On Tue, 25 Feb 2025, Ming Yu wrote:

quoted

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 needs to go into the Kconfig help passage.

Okay, I will move these to Kconfig in the next patch.

quoted

quoted

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

This driver doesn't implement USB functionality.

Fix it in v9.

quoted

quoted

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

Signed-off-by: Ming Yu <redacted>

Why aren't you signing off with your work address?

Fix it in v9.

quoted

quoted

---
 MAINTAINERS                 |   7 +
 drivers/mfd/Kconfig         |  18 ++
 drivers/mfd/Makefile        |   2 +
 drivers/mfd/nct6694.c       | 378 ++++++++++++++++++++++++++++++++++++
 include/linux/mfd/nct6694.h | 102 ++++++++++
 5 files changed, 507 insertions(+)
 create mode 100644 drivers/mfd/nct6694.c
 create mode 100644 include/linux/mfd/nct6694.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 873aa2cce4d7..c700a0b96960 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -16918,6 +16918,13 @@ F:   drivers/nubus/
 F:   include/linux/nubus.h
 F:   include/uapi/linux/nubus.h

+NUVOTON NCT6694 MFD DRIVER
+M:   Ming Yu <tmyu0@nuvoton.com>
+L:   linux-kernel@vger.kernel.org

This is the default list.  You shouldn't need to add that here.

Remove it in v9.

Please snip everything that you agree with.

quoted

quoted

+S:   Supported
+F:   drivers/mfd/nct6694.c
+F:   include/linux/mfd/nct6694.h
+
 NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER
 M:   Antonino Daplas [off-list ref]
 L:   linux-fbdev@vger.kernel.org

[...]

quoted

quoted

+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x1),

IDs are usually given in base-10.

Fix it in v9.

quoted

Why are you manually adding the device IDs?

PLATFORM_DEVID_AUTO doesn't work for you?

I need to manage these IDs to ensure that child devices can be
properly utilized within their respective modules.

How?  Please explain.

This numbering looks sequential and arbitrary.

What does PLATFORM_DEVID_AUTO do differently such that it is not useful?

quoted

quoted

+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x2),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x3),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x4),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x5),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x6),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x7),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x8),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x9),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xA),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xB),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xC),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xD),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xE),
+     MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0xF),

quoted

quoted

+
+     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x0),
+     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x1),
+     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x2),
+     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x3),
+     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x4),
+     MFD_CELL_BASIC("i2c-nct6694", NULL, NULL, 0, 0x5),
+
+     MFD_CELL_BASIC("nct6694_canfd", NULL, NULL, 0, 0x0),

Why has the naming convention changed here?

I originally expected the child devices name to directly match its
driver name. Do you think it would be better to standardize the naming
as "nct6694-xxx" ?

Yes, that is the usual procedure.

quoted

quoted

+     MFD_CELL_BASIC("nct6694_canfd", NULL, NULL, 0, 0x1),
+
+     MFD_CELL_BASIC("nct6694_wdt", NULL, NULL, 0, 0x0),
+     MFD_CELL_BASIC("nct6694_wdt", NULL, NULL, 0, 0x1),
+
+     MFD_CELL_NAME("nct6694-hwmon"),
+     MFD_CELL_NAME("rtc-nct6694"),

There doesn't seem to be any consistency here.

Do you think these two should be changed to use MFD_CELL_BASIC()?

No.  I mean with the device nomenclature.

[...]

quoted

quoted

+static void usb_int_callback(struct urb *urb)
+{
+     struct nct6694 *nct6694 = urb->context;
+     unsigned int *int_status = urb->transfer_buffer;
+     int ret;
+
+     switch (urb->status) {
+     case 0:
+             break;
+     case -ECONNRESET:
+     case -ENOENT:
+     case -ESHUTDOWN:
+             return;
+     default:
+             generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
+             *int_status &= ~BIT(irq);
+     }
+
+resubmit:
+     ret = usb_submit_urb(urb, GFP_ATOMIC);
+     if (ret)
+             dev_dbg(nct6694->dev, "%s: Failed to resubmit urb, status %pe",

Why debug?

Excuse me, do you think it should change to dev_err()?

Probably a dev_warn() since you are not propagating the error.

Is this okay by the way?  Is it okay to fail?

-- 
Lee Jones [李琼斯]