| Message ID | 20250225081644.3524915-2-a0282524688@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD drivers | expand |
On 25.02.2025 16:16:38, Ming Yu wrote: [...] > +static int nct6694_usb_probe(struct usb_interface *iface, > + const struct usb_device_id *id) > +{ > + struct usb_device *udev = interface_to_usbdev(iface); > + struct 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 */ Why do you need this variable? You can directly use NCT6694_URB_TIMEOUT in the usb_bulk_msg() and friends calls. regards, Marc
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. > This driver implements USB device functionality and shares the > chip's peripherals as a child device. This driver doesn't implement USB functionality. > 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@gmail.com> Why aren't you signing off with your work address? > --- > 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. > +S: Supported > +F: drivers/mfd/nct6694.c > +F: include/linux/mfd/nct6694.h > + > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER > M: Antonino Daplas <adaplas@gmail.com> > L: linux-fbdev@vger.kernel.org > 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. > + functionalities such as GPIO, I2C, CAN, WDT, HWMON, and RTC > + management. All of these line breaks should be removed. > + 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. > + * functionalities such as GPIO, I2C, CAN, WDT, HWMON and RTC > + * management. > + * > + * Copyright (C) 2024 Nuvoton Technology Corp. This goes at the top. > + */ > + > +#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. > + MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x1), IDs are usually given in base-10. Why are you manually adding the device IDs? PLATFORM_DEVID_AUTO doesn't work for you? > + 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? > + 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. > +}; > + > +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. > + case NCT6694_NOT_SUPPORT_ERROR: > + dev_warn(nct6694->dev, "Command is not supported!\n"); Why not dev_err()? > + 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? > + 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? > + 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. > + 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? > + 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? > + __func__, ERR_PTR(ret)); Remove the __func__ part. > +} > + > +static void nct6694_irq_lock(struct irq_data *data) > +{ > + struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data); > + > + mutex_lock(&nct6694->irq_lock); > +} > + > +static void nct6694_irq_sync_unlock(struct irq_data *data) > +{ > + struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data); > + > + mutex_unlock(&nct6694->irq_lock); > +} > + > +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); > + > + 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); > + > + nct6694->irq_enable &= ~BIT(hwirq); > +} > + > +static const struct irq_chip nct6694_irq_chip = { > + .name = "nct6694-irq", > + .flags = IRQCHIP_SKIP_SET_WAKE, > + .irq_bus_lock = nct6694_irq_lock, > + .irq_bus_sync_unlock = nct6694_irq_sync_unlock, > + .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) > +{ > + 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. > + 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. > + > + 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. > + {} > +}; > +MODULE_DEVICE_TABLE(usb, nct6694_ids); > + > +static struct usb_driver nct6694_usb_driver = { > + .name = "nct6694", Odd spaces. > + .id_table = nct6694_ids, > + .probe = nct6694_usb_probe, > + .disconnect = nct6694_usb_disconnect, > +}; > + Remove this line. > +module_usb_driver(nct6694_usb_driver); > + > +MODULE_DESCRIPTION("USB core driver for NCT6694"); This is not a USB driver. > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); Different to SoB. > +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. > + */ > + > +#ifndef __MFD_NCT6694_H > +#define __MFD_NCT6694_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_HCTRL_SET 0x40 > +#define NCT6694_HCTRL_GET 0x80 > + > +#define NCT6694_URB_TIMEOUT 1000 > + > +enum nct6694_irq_id { > + NCT6694_IRQ_GPIO0 = 0, > + NCT6694_IRQ_GPIO1, > + NCT6694_IRQ_GPIO2, > + NCT6694_IRQ_GPIO3, > + NCT6694_IRQ_GPIO4, > + NCT6694_IRQ_GPIO5, > + NCT6694_IRQ_GPIO6, > + NCT6694_IRQ_GPIO7, > + NCT6694_IRQ_GPIO8, > + NCT6694_IRQ_GPIO9, > + NCT6694_IRQ_GPIOA, > + NCT6694_IRQ_GPIOB, > + NCT6694_IRQ_GPIOC, > + NCT6694_IRQ_GPIOD, > + NCT6694_IRQ_GPIOE, > + NCT6694_IRQ_GPIOF, > + NCT6694_IRQ_CAN0, > + NCT6694_IRQ_CAN1, > + NCT6694_IRQ_RTC, > + NCT6694_NR_IRQS, > +}; > + > +enum nct6694_response_err_status { > + NCT6694_NO_ERROR = 0, > + NCT6694_FORMAT_ERROR, > + NCT6694_RESERVED1, > + NCT6694_RESERVED2, > + NCT6694_NOT_SUPPORT_ERROR, > + NCT6694_NO_RESPONSE_ERROR, > + NCT6694_TIMEOUT_ERROR, > + NCT6694_PENDING, > +}; > + > +struct __packed nct6694_cmd_header { > + u8 rsv1; > + u8 mod; > + union __packed { > + __le16 offset; > + struct __packed { > + u8 cmd; > + u8 sel; > + }; > + }; > + u8 hctrl; > + u8 rsv2; > + __le16 len; > +}; > + > +struct __packed nct6694_response_header { > + u8 sequence_id; > + u8 sts; > + u8 reserved[4]; > + __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 irq_domain *domain; > + /* Mutex to protect access to the device */ > + struct mutex access_lock; > + /* Mutex to protect access to the IRQ */ > + struct mutex irq_lock; > + struct urb *int_in_urb; > + struct usb_device *udev; > + union nct6694_usb_msg *usb_msg; > + unsigned char *int_buffer; > + unsigned int irq_enable; > + /* Time in msec to wait for the URB to the complete */ > + long timeout; timeout_ms > +}; > + > +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); > + > +#endif > -- > 2.34.1 >
Dear Marc, Thank you for reviewing, Marc Kleine-Budde <mkl@pengutronix.de> 於 2025年2月28日 週五 下午4:52寫道： > > > +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 */ > > Why do you need this variable? You can directly use NCT6694_URB_TIMEOUT > in the usb_bulk_msg() and friends calls. > Okay, I will make the modifications in the next patch. Best regards, Ming
On Mon, 17 Mar 2025, Ming Yu wrote: > Dear Lee, > > Thank you for reviewing, > > Lee Jones <lee@kernel.org> 於 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@gmail.com> > > > > 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@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. > > > +S: Supported > > > +F: drivers/mfd/nct6694.c > > > +F: include/linux/mfd/nct6694.h > > > + > > > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER > > > M: Antonino Daplas <adaplas@gmail.com> > > > L: linux-fbdev@vger.kernel.org [...] > > > + 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. How? Please explain. This numbering looks sequential and arbitrary. What does PLATFORM_DEVID_AUTO do differently such that it is not useful? > > > > + 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" ? Yes, that is the usual procedure. > > > + 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. [...] > > > +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 <lee@kernel.org> 於 2025年3月20日 週四 下午10:50寫道： > ... > > > > + 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. > > How? Please explain. > > This numbering looks sequential and arbitrary. > > What does PLATFORM_DEVID_AUTO do differently such that it is not useful? > As far as I know, PLATFORM_DEVID_AUTO assigns dynamic IDs to devices, but I need fixed IDs. For example, the GPIO driver relies on these IDs to determine the group, allowing the firmware to identify which GPIO group to operate on through the API. > > > > > > + 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), Thanks, Ming
On Wed, 26 Mar 2025, Ming Yu wrote: > Lee Jones <lee@kernel.org> 於 2025年3月20日 週四 下午10:50寫道： > > > ... > > > > > + 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. > > > > How? Please explain. > > > > This numbering looks sequential and arbitrary. > > > > What does PLATFORM_DEVID_AUTO do differently such that it is not useful? > > > > As far as I know, PLATFORM_DEVID_AUTO assigns dynamic IDs to devices, > but I need fixed IDs. > For example, the GPIO driver relies on these IDs to determine the > group, allowing the firmware to identify which GPIO group to operate > on through the API. PLATFORM_DEVID_AUTO will allocate IDs 0 through 16, the same as you've done here. These lines do not have any differentiating attributes, so either way we are not allocating specific IDs to specific pieces of the H/W. I still do not understand why you need to allocate them manually. > > > > > + 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), > > > Thanks, > Ming
On Mon, 07 Apr 2025, Ming Yu wrote: > Lee Jones <lee@kernel.org> 於 2025年4月4日 週五 下午10:21寫道： > > > > > ... > > > > > > > + 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. > > > > > > > > How? Please explain. > > > > > > > > This numbering looks sequential and arbitrary. > > > > > > > > What does PLATFORM_DEVID_AUTO do differently such that it is not useful? > > > > > > > > > > As far as I know, PLATFORM_DEVID_AUTO assigns dynamic IDs to devices, > > > but I need fixed IDs. > > > For example, the GPIO driver relies on these IDs to determine the > > > group, allowing the firmware to identify which GPIO group to operate > > > on through the API. > > > > PLATFORM_DEVID_AUTO will allocate IDs 0 through 16, the same as you've > > done here. These lines do not have any differentiating attributes, so > > either way we are not allocating specific IDs to specific pieces of the > > H/W. I still do not understand why you need to allocate them manually. > > > > I'm using PLATFORM_DEVID_AUTO to allocate child device IDs with > MFD_CELL_NAME(), like this: > > static const struct mfd_cell nct6694_dev[] = { > 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"), > ...... > }; > > For example, the device IDs retrieved in gpio-nct6694.c is 1~16, and > i2c-nct6694.c is 17~22. Does this mean each driver should > independently handle its dynamically assigned IDs? > Additionally, I originally referred to cgbc-core.c with i2c-cgbc.c, > and ab8500-core.c with pwm-ab8500.c for associating child devices. Do > you think this approach is appropriate in my case? Yes, if you _need_ the ranges to start from 0, then you will have to call mfd_add_devices() separately on those ranges. Otherwise one range will follow directly on to another range. But wait, you're using mfd_add_hotplug_devices(), which means you are using PLATFORM_DEVID_AUTO. So your .id values that you've added are being ignored anyway. Thus, if you have tested that this works, you don't need them anyway, right?
Lee Jones <lee@kernel.org> 於 2025年4月10日 週四 下午4:21寫道： > > On Mon, 07 Apr 2025, Ming Yu wrote: > > > Lee Jones <lee@kernel.org> 於 2025年4月4日 週五 下午10:21寫道： > > > > > > > ... > > > > > > > > + 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. > > > > > > > > > > How? Please explain. > > > > > > > > > > This numbering looks sequential and arbitrary. > > > > > > > > > > What does PLATFORM_DEVID_AUTO do differently such that it is not useful? > > > > > > > > > > > > > As far as I know, PLATFORM_DEVID_AUTO assigns dynamic IDs to devices, > > > > but I need fixed IDs. > > > > For example, the GPIO driver relies on these IDs to determine the > > > > group, allowing the firmware to identify which GPIO group to operate > > > > on through the API. > > > > > > PLATFORM_DEVID_AUTO will allocate IDs 0 through 16, the same as you've > > > done here. These lines do not have any differentiating attributes, so > > > either way we are not allocating specific IDs to specific pieces of the > > > H/W. I still do not understand why you need to allocate them manually. > > > > > > > I'm using PLATFORM_DEVID_AUTO to allocate child device IDs with > > MFD_CELL_NAME(), like this: > > > > static const struct mfd_cell nct6694_dev[] = { > > 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"), > > ...... > > }; > > > > For example, the device IDs retrieved in gpio-nct6694.c is 1~16, and > > i2c-nct6694.c is 17~22. Does this mean each driver should > > independently handle its dynamically assigned IDs? > > Additionally, I originally referred to cgbc-core.c with i2c-cgbc.c, > > and ab8500-core.c with pwm-ab8500.c for associating child devices. Do > > you think this approach is appropriate in my case? > > Yes, if you _need_ the ranges to start from 0, then you will have to > call mfd_add_devices() separately on those ranges. Otherwise one range > will follow directly on to another range. > > But wait, you're using mfd_add_hotplug_devices(), which means you are > using PLATFORM_DEVID_AUTO. So your .id values that you've added are > being ignored anyway. Thus, if you have tested that this works, you > don't need them anyway, right? > Yes, it uses PLATFORM_DEVID_AUTO, but in my implementation, the sub-devices use cell->id instead of platform_device->id, so it doesn't affect the current behavior. However, if you think there's a better approach or that this should be changed for consistency or correctness, I'm happy to update it, please let me know your recommendation. Thanks, Ming
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 +S: Supported +F: drivers/mfd/nct6694.c +F: include/linux/mfd/nct6694.h + NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER M: Antonino Daplas <adaplas@gmail.com> L: linux-fbdev@vger.kernel.org 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 + functionalities such as GPIO, I2C, CAN, WDT, HWMON, and RTC + management. + + 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 + * functionalities such as GPIO, I2C, CAN, WDT, HWMON and RTC + * management. + * + * Copyright (C) 2024 Nuvoton Technology Corp. + */ + +#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), + MFD_CELL_BASIC("gpio-nct6694", NULL, NULL, 0, 0x1), + 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), + 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"), +}; + +static int nct6694_response_err_handling(struct nct6694 *nct6694, + unsigned char err_status) +{ + switch (err_status) { + case NCT6694_NO_ERROR: + return err_status; + case NCT6694_NOT_SUPPORT_ERROR: + dev_warn(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_warn(nct6694->dev, "Command is pending!\n"); + 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 */ + memcpy(&msg->cmd_header, cmd_hd, sizeof(*cmd_hd)); + msg->cmd_header.hctrl = NCT6694_HCTRL_GET; + + 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; + + /* 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 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: + goto resubmit; + } + + while (*int_status) { + int irq = __ffs(*int_status); + + 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", + __func__, ERR_PTR(ret)); +} + +static void nct6694_irq_lock(struct irq_data *data) +{ + struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data); + + mutex_lock(&nct6694->irq_lock); +} + +static void nct6694_irq_sync_unlock(struct irq_data *data) +{ + struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data); + + mutex_unlock(&nct6694->irq_lock); +} + +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); + + 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); + + nct6694->irq_enable &= ~BIT(hwirq); +} + +static const struct irq_chip nct6694_irq_chip = { + .name = "nct6694-irq", + .flags = IRQCHIP_SKIP_SET_WAKE, + .irq_bus_lock = nct6694_irq_lock, + .irq_bus_sync_unlock = nct6694_irq_sync_unlock, + .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) +{ + 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 */ + + 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; + + 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)}, + {} +}; +MODULE_DEVICE_TABLE(usb, nct6694_ids); + +static struct usb_driver nct6694_usb_driver = { + .name = "nct6694", + .id_table = nct6694_ids, + .probe = nct6694_usb_probe, + .disconnect = nct6694_usb_disconnect, +}; + +module_usb_driver(nct6694_usb_driver); + +MODULE_DESCRIPTION("USB core driver for NCT6694"); +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); +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. + */ + +#ifndef __MFD_NCT6694_H +#define __MFD_NCT6694_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_HCTRL_SET 0x40 +#define NCT6694_HCTRL_GET 0x80 + +#define NCT6694_URB_TIMEOUT 1000 + +enum nct6694_irq_id { + NCT6694_IRQ_GPIO0 = 0, + NCT6694_IRQ_GPIO1, + NCT6694_IRQ_GPIO2, + NCT6694_IRQ_GPIO3, + NCT6694_IRQ_GPIO4, + NCT6694_IRQ_GPIO5, + NCT6694_IRQ_GPIO6, + NCT6694_IRQ_GPIO7, + NCT6694_IRQ_GPIO8, + NCT6694_IRQ_GPIO9, + NCT6694_IRQ_GPIOA, + NCT6694_IRQ_GPIOB, + NCT6694_IRQ_GPIOC, + NCT6694_IRQ_GPIOD, + NCT6694_IRQ_GPIOE, + NCT6694_IRQ_GPIOF, + NCT6694_IRQ_CAN0, + NCT6694_IRQ_CAN1, + NCT6694_IRQ_RTC, + NCT6694_NR_IRQS, +}; + +enum nct6694_response_err_status { + NCT6694_NO_ERROR = 0, + NCT6694_FORMAT_ERROR, + NCT6694_RESERVED1, + NCT6694_RESERVED2, + NCT6694_NOT_SUPPORT_ERROR, + NCT6694_NO_RESPONSE_ERROR, + NCT6694_TIMEOUT_ERROR, + NCT6694_PENDING, +}; + +struct __packed nct6694_cmd_header { + u8 rsv1; + u8 mod; + union __packed { + __le16 offset; + struct __packed { + u8 cmd; + u8 sel; + }; + }; + u8 hctrl; + u8 rsv2; + __le16 len; +}; + +struct __packed nct6694_response_header { + u8 sequence_id; + u8 sts; + u8 reserved[4]; + __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 irq_domain *domain; + /* Mutex to protect access to the device */ + struct mutex access_lock; + /* Mutex to protect access to the IRQ */ + struct mutex irq_lock; + struct urb *int_in_urb; + struct usb_device *udev; + union nct6694_usb_msg *usb_msg; + unsigned char *int_buffer; + unsigned int irq_enable; + /* Time in msec to wait for the URB to the complete */ + long timeout; +}; + +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); + +#endif
The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips, 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC, PWM, and RTC. This driver implements USB device functionality and shares the chip's peripherals as a child device. Each child device can use the USB functions nct6694_read_msg() and nct6694_write_msg() to issue a command. They can also request interrupt that will be called when the USB device receives its interrupt pipe. Signed-off-by: Ming Yu <a0282524688@gmail.com> --- 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