Dear Lee, Thank you for reviewing, Lee Jones <lee@xxxxxxxxxx> 於 2025年3月7日 週五 上午9:15寫道: > > On Tue, 25 Feb 2025, Ming Yu wrote: > > > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips, > > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC, > > PWM, and RTC. > > This needs to go into the Kconfig help passage. > Okay, I will move these to Kconfig in the next patch. > > 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. > > 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 <a0282524688@xxxxxxxxx> > > Why aren't you signing off with your work address? > Fix it in v9. > > --- > > 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@xxxxxxxxxxx> > > +L: linux-kernel@xxxxxxxxxxxxxxx > > This is the default list. You shouldn't need to add that here. > Remove it in v9. > > +S: Supported > > +F: drivers/mfd/nct6694.c > > +F: include/linux/mfd/nct6694.h > > + > > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER > > M: Antonino Daplas <adaplas@xxxxxxxxx> > > L: linux-fbdev@xxxxxxxxxxxxxxx > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > index 6b0682af6e32..c97a2bdcea0b 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -1045,6 +1045,24 @@ config MFD_MENF21BMC > > This driver can also be built as a module. If so the module > > will be called menf21bmc. > > > > +config MFD_NCT6694 > > + tristate "Nuvoton NCT6694 support" > > + select MFD_CORE > > + depends on USB > > + help > > + This enables support for the Nuvoton USB device NCT6694, which shares > > + peripherals. > > + > > + This driver provides core APIs to access the NCT6694 hardware > > + monitoring and control features. > > + > > + The NCT6694 is a versatile multi-function device that supports > > Please drop the term multi-function device and replace it what a proper > description of the devices. > Fix it in v9. > > + functionalities such as GPIO, I2C, CAN, WDT, HWMON, and RTC > > + management. > > All of these line breaks should be removed. > Fix these in v9. > > + Additional drivers must be enabled to utilize the specific > > + functionalities of the device. > > + > > config MFD_OCELOT > > tristate "Microsemi Ocelot External Control Support" > > depends on SPI_MASTER > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 9220eaf7cf12..7725b732e265 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -121,6 +121,8 @@ obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o > > obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o > > obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o > > > > +obj-$(CONFIG_MFD_NCT6694) += nct6694.o > > + > > obj-$(CONFIG_MFD_CORE) += mfd-core.o > > > > ocelot-soc-objs := ocelot-core.o ocelot-spi.o > > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c > > new file mode 100644 > > index 000000000000..c82457679ca6 > > --- /dev/null > > +++ b/drivers/mfd/nct6694.c > > @@ -0,0 +1,378 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Nuvoton NCT6694 core driver using USB interface to provide > > + * access to the NCT6694 hardware monitoring and control features. > > + * > > + * The NCT6694 is a versatile multi-function device that supports > > Here too. > Fix it in v9. > > + * functionalities such as GPIO, I2C, CAN, WDT, HWMON and RTC > > + * management. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > This goes at the top. > Fix it in v9. > > + */ > > + > > +#include <linux/bits.h> > > +#include <linux/interrupt.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/slab.h> > > +#include <linux/usb.h> > > + > > +static const struct mfd_cell nct6694_dev[] = { > > + MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x0), > > "-gpio" usually goes on the end. > Fix it in v9. > > + MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x1), > > IDs are usually given in base-10. > Fix it in v9. > 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. > > + 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), > > + > > + 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" ? > > + 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()? > > +}; > > + > > +static int nct6694_response_err_handling(struct nct6694 *nct6694, > > > +{ > > + switch (err_status) { > > + case NCT6694_NO_ERROR: > > + return err_status; > > This is odd since you already know this will be 0. > Fix it in v9. > > + case NCT6694_NOT_SUPPORT_ERROR: > > + dev_warn(nct6694->dev, "Command is not supported!\n"); > > Why not dev_err()? > Fix it in v9. > > + 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_warn(nct6694->dev, "Command is pending!\n"); > > Is this an error? > Yes, I will fix it to dev_err() in the next patch. > > + break; > > + default: > > + return -EINVAL; > > + } > > + > > + return -EIO; > > +} > > + > > +int nct6694_read_msg(struct nct6694 *nct6694, const struct nct6694_cmd_header *cmd_hd, void *buf) > > +{ > > + union nct6694_usb_msg *msg = nct6694->usb_msg; > > + int tx_len, rx_len, ret; > > + > > + guard(mutex)(&nct6694->access_lock); > > + > > + /* Send command packet to USB device */ > > This doesn't really describe the next 2 lines. > > Move it down? > Fix it in v9. > > + memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd)); > > + msg->cmd_header.hctrl = NCT6694_HCTRL_GET; > > + > > + ret = usb_bulk_msg(nct6694->udev, > > Since you use nct6694->udev a bunch - sometimes twice in the same call, > it might be nicer to pull it into it's own variable instead of > dereferencing it all the time. > Fix it in v9. > > + usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP), > > + &msg->cmd_header, sizeof(*msg), &tx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + /* Receive response packet from USB device */ > > + ret = usb_bulk_msg(nct6694->udev, > > + usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP), > > + &msg->response_header, sizeof(*msg), &rx_len, > > How can you read sizeof(*msg) Bytes (22?) into the smaller > response_header (16?) attribute? > The size of (*msg) is the same as command_header and response_header (8 bytes). > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + /* Receive data packet from USB device */ > > + ret = usb_bulk_msg(nct6694->udev, > > + usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP), > > + buf, le16_to_cpu(cmd_hd->len), &rx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + if (rx_len != le16_to_cpu(cmd_hd->len)) { > > + dev_err(nct6694->dev, "Expected received length %d, but got %d\n", > > + le16_to_cpu(cmd_hd->len), rx_len); > > + return -EIO; > > + } > > + > > + return nct6694_response_err_handling(nct6694, msg->response_header.sts); > > +} > > +EXPORT_SYMBOL(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; > > + int tx_len, rx_len, ret; > > + > > + guard(mutex)(&nct6694->access_lock); > > + > > + /* Send command packet to USB device */ > > + memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd)); > > + msg->cmd_header.hctrl = NCT6694_HCTRL_SET; > > + > > + ret = usb_bulk_msg(nct6694->udev, > > + usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP), > > + &msg->cmd_header, sizeof(*msg), &tx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + /* Send data packet to USB device */ > > + ret = usb_bulk_msg(nct6694->udev, > > + usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP), > > + buf, le16_to_cpu(cmd_hd->len), &tx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + /* Receive response packet from USB device */ > > + ret = usb_bulk_msg(nct6694->udev, > > + usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP), > > + &msg->response_header, sizeof(*msg), &rx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + /* Receive data packet from USB device */ > > + ret = usb_bulk_msg(nct6694->udev, > > + usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP), > > + buf, le16_to_cpu(cmd_hd->len), &rx_len, > > + nct6694->timeout); > > + if (ret) > > + return ret; > > + > > + if (rx_len != le16_to_cpu(cmd_hd->len)) { > > + dev_err(nct6694->dev, "Expected transmitted length %d, but got %d\n", > > + le16_to_cpu(cmd_hd->len), rx_len); > > + return -EIO; > > + } > > + > > + return nct6694_response_err_handling(nct6694, msg->response_header.sts); > > +} > > +EXPORT_SYMBOL(nct6694_write_msg); > > + > > +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()? > > + __func__, ERR_PTR(ret)); > > Remove the __func__ part. > Understood. > > +} > > + ... > > +static int nct6694_usb_probe(struct usb_interface *iface, > > + const struct usb_device_id *id) > > +{ > > + 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 *nct6694; > > + int pipe, maxp; > > + int ret; > > + > > + nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL); > > + if (!nct6694) > > + return -ENOMEM; > > + > > + pipe = usb_rcvintpipe(udev, NCT6694_INT_IN_EP); > > + maxp = usb_maxpacket(udev, pipe); > > + > > + nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg), GFP_KERNEL); > > + if (!nct6694->usb_msg) > > + return -ENOMEM; > > + > > + nct6694->int_buffer = devm_kzalloc(dev, maxp, GFP_KERNEL); > > + if (!nct6694->int_buffer) > > + return -ENOMEM; > > + > > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > > + if (!nct6694->int_in_urb) > > + return -ENOMEM; > > + > > + nct6694->domain = irq_domain_add_simple(NULL, NCT6694_NR_IRQS, 0, > > + &nct6694_irq_domain_ops, > > + nct6694); > > + if (!nct6694->domain) { > > + ret = -ENODEV; > > + goto err_urb; > > + } > > + > > + nct6694->dev = dev; > > + nct6694->udev = udev; > > + nct6694->timeout = NCT6694_URB_TIMEOUT; /* Wait until URB completes */ > > No need to save this known value. > Understood, I will drop it in the next patch. > > + ret = devm_mutex_init(dev, &nct6694->access_lock); > > + if (ret) > > + goto err_urb; > > + > > + ret = devm_mutex_init(dev, &nct6694->irq_lock); > > + 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; > > + goto err_urb; > > + } > > + usb_fill_int_urb(nct6694->int_in_urb, udev, pipe, > > + nct6694->int_buffer, maxp, usb_int_callback, > > + nct6694, int_endpoint->bInterval); > > + ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL); > > + if (ret) > > + goto err_urb; > > Please unsquash these calls - space them out. > Fix these in v9. > > + > > + usb_set_intfdata(iface, nct6694); > > + > > + ret = mfd_add_hotplug_devices(dev, nct6694_dev, ARRAY_SIZE(nct6694_dev)); > > + if (ret) > > + goto err_mfd; > > + > > + return 0; > > + > > +err_mfd: > > + usb_kill_urb(nct6694->int_in_urb); > > +err_urb: > > + usb_free_urb(nct6694->int_in_urb); > > + return ret; > > +} > > + > > +static void nct6694_usb_disconnect(struct usb_interface *iface) > > +{ > > + struct nct6694 *nct6694 = usb_get_intfdata(iface); > > + > > + mfd_remove_devices(nct6694->dev); > > + usb_kill_urb(nct6694->int_in_urb); > > + usb_free_urb(nct6694->int_in_urb); > > +} > > + > > +static const struct usb_device_id nct6694_ids[] = { > > + { USB_DEVICE_AND_INTERFACE_INFO(NCT6694_VENDOR_ID, > > + NCT6694_PRODUCT_ID, > > + 0xFF, 0x00, 0x00)}, > > This should fit on one line. You can use up to 100-chars. > Fix it in v9. > > + {} > > +}; > > +MODULE_DEVICE_TABLE(usb, nct6694_ids); > > + > > +static struct usb_driver nct6694_usb_driver = { > > + .name = "nct6694", > > Odd spaces. > Fix it in v9. > > + .id_table = nct6694_ids, > > + .probe = nct6694_usb_probe, > > + .disconnect = nct6694_usb_disconnect, > > +}; > > + > > Remove this line. > Fix it in v9. > > +module_usb_driver(nct6694_usb_driver); > > + > > +MODULE_DESCRIPTION("USB core driver for NCT6694"); > > This is not a USB driver. > Fix it in v9. > > +MODULE_AUTHOR("Ming Yu <tmyu0@xxxxxxxxxxx>"); > > Different to SoB. > Fix it in v9. > > +MODULE_LICENSE("GPL"); > > diff --git a/include/linux/mfd/nct6694.h b/include/linux/mfd/nct6694.h > > new file mode 100644 > > index 000000000000..8171f975761e > > --- /dev/null > > +++ b/include/linux/mfd/nct6694.h > > @@ -0,0 +1,102 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Nuvoton NCT6694 USB transaction and data structure. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > At the top. > Fix it in v9. > > + */ > > + Best regards, Ming