| Message ID | 20241024085922.133071-1-tmyu0@nuvoton.com |
|---|---|
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD devices | expand |
On 24.10.2024 16:59:14, Ming Yu wrote: > +static int nct6694_usb_probe(struct usb_interface *iface, > + const struct usb_device_id *id) > +{ > + struct usb_device *udev = interface_to_usbdev(iface); > + struct device *dev = &udev->dev; > + struct usb_host_interface *interface; > + struct usb_endpoint_descriptor *int_endpoint; > + struct nct6694 *nct6694; > + int pipe, maxp, bulk_pipe; > + int ret = EINVAL; > + > + interface = iface->cur_altsetting; > + /* Binding interface class : 0xFF */ > + if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC || > + interface->desc.bInterfaceSubClass != 0x00 || > + interface->desc.bInterfaceProtocol != 0x00) > + return -ENODEV; I think you can use USB_DEVICE_INFO() and remove this manual check https://elixir.bootlin.com/linux/v6.11.5/source/include/linux/usb.h#L1056 [...] > + > +static const struct usb_device_id nct6694_ids[] = { > + { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)}, > + {}, > +}; > +MODULE_DEVICE_TABLE(usb, nct6694_ids); regards, Marc
On 24.10.2024 16:59:14, Ming Yu wrote: > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips, > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC, > PWM, and RTC. > > This driver implements USB device functionality and shares the > chip's peripherals as a child device. > > Each child device can use the USB functions nct6694_read_msg() > and nct6694_write_msg() to issue a command. They can also register > a handler function that will be called when the USB device receives > its interrupt pipe. [...] > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c > new file mode 100644 > index 000000000000..9838c7be0b98 > --- /dev/null > +++ b/drivers/mfd/nct6694.c [...] > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, > + u8 rd_idx, u8 rd_len, unsigned char *buf) why not make buf a void *? > +{ > + struct usb_device *udev = nct6694->udev; > + unsigned char err_status; > + int len, packet_len, tx_len, rx_len; > + int i, ret; > + > + mutex_lock(&nct6694->access_lock); > + > + /* Send command packet to USB device */ > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET; > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; > + > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > + nct6694->timeout); > + if (ret) > + goto err; > + > + /* Receive response packet from USB device */ > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > + nct6694->timeout); > + if (ret) > + goto err; > + > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > + > + /* > + * Segmented reception of messages that exceed the size of USB bulk > + * pipe packets. > + */ The Linux USB stack can receive bulk messages longer than the max packet size. > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > + if (len > nct6694->maxp) > + packet_len = nct6694->maxp; > + else > + packet_len = len; > + > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > + nct6694->rx_buffer + nct6694->maxp * i, > + packet_len, &rx_len, nct6694->timeout); > + if (ret) > + goto err; > + } > + > + for (i = 0; i < rd_len; i++) > + buf[i] = nct6694->rx_buffer[i + rd_idx]; memcpy()? Or why don't you directly receive data into the provided buffer? Copying of the data doesn't make it faster. On the other hand, receiving directly into the target buffer means the target buffer must not live on the stack. > + > + if (err_status) { > + pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status); > + ret = -EIO; > + } > + > +err: > + mutex_unlock(&nct6694->access_lock); > + return ret; > +} > +EXPORT_SYMBOL(nct6694_read_msg); > + > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > + u16 length, unsigned char *buf) > +{ > + struct usb_device *udev = nct6694->udev; > + unsigned char err_status; > + int len, packet_len, tx_len, rx_len; > + int i, ret; > + > + mutex_lock(&nct6694->access_lock); > + > + /* Send command packet to USB device */ > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_SET; > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; What about creating a struct that describes the cmd_buffer layout? > + > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > + nct6694->timeout); > + if (ret) > + goto err; > + > + /* > + * Segmented transmission of messages that exceed the size of USB bulk > + * pipe packets. > + */ same as above > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > + if (len > nct6694->maxp) > + packet_len = nct6694->maxp; > + else > + packet_len = len; > + > + memcpy(nct6694->tx_buffer + nct6694->maxp * i, > + buf + nct6694->maxp * i, packet_len); > + > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > + nct6694->tx_buffer + nct6694->maxp * i, > + packet_len, &tx_len, nct6694->timeout); > + if (ret) > + goto err; > + } > + > + /* Receive response packet from USB device */ > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > + nct6694->timeout); > + if (ret) > + goto err; > + > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > + > + /* > + * Segmented reception of messages that exceed the size of USB bulk > + * pipe packets. > + */ same as above > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > + if (len > nct6694->maxp) > + packet_len = nct6694->maxp; > + else > + packet_len = len; > + > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > + nct6694->rx_buffer + nct6694->maxp * i, > + packet_len, &rx_len, nct6694->timeout); > + if (ret) > + goto err; > + } > + > + memcpy(buf, nct6694->rx_buffer, length); why not rx into the destination buffer directly? > + > + if (err_status) { > + pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status); > + ret = -EIO; > + } > + > +err: > + mutex_unlock(&nct6694->access_lock); > + return ret; > +} > +EXPORT_SYMBOL(nct6694_write_msg); [...] > +static int nct6694_usb_probe(struct usb_interface *iface, > + const struct usb_device_id *id) > +{ > + struct usb_device *udev = interface_to_usbdev(iface); > + struct device *dev = &udev->dev; > + struct usb_host_interface *interface; > + struct usb_endpoint_descriptor *int_endpoint; > + struct nct6694 *nct6694; > + int pipe, maxp, bulk_pipe; > + int ret = EINVAL; > + > + interface = iface->cur_altsetting; > + /* Binding interface class : 0xFF */ > + if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC || > + interface->desc.bInterfaceSubClass != 0x00 || > + interface->desc.bInterfaceProtocol != 0x00) > + return -ENODEV; > + > + int_endpoint = &interface->endpoint[0].desc; > + if (!usb_endpoint_is_int_in(int_endpoint)) > + return -ENODEV; > + > + nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL); > + if (!nct6694) > + return -ENOMEM; > + > + pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT); > + maxp = usb_maxpacket(udev, pipe); better move these 2 down to the usb_fill_int_urb(). > + > + nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ, > + sizeof(unsigned char), GFP_KERNEL); > + if (!nct6694->cmd_buffer) > + return -ENOMEM; > + nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > + sizeof(unsigned char), GFP_KERNEL); > + if (!nct6694->rx_buffer) > + return -ENOMEM; > + nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > + sizeof(unsigned char), GFP_KERNEL); > + if (!nct6694->tx_buffer) > + return -ENOMEM; > + nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > + sizeof(unsigned char), GFP_KERNEL); > + if (!nct6694->int_buffer) > + return -ENOMEM; > + > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > + if (!nct6694->int_in_urb) { > + dev_err(&udev->dev, "Failed to allocate INT-in urb!\n"); > + return -ENOMEM; > + } > + > + /* Bulk pipe maximum packet for each transaction */ > + bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT); > + nct6694->maxp = usb_maxpacket(udev, bulk_pipe); > + > + mutex_init(&nct6694->access_lock); > + nct6694->udev = udev; > + nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */ > + > + INIT_LIST_HEAD(&nct6694->handler_list); > + spin_lock_init(&nct6694->lock); > + > + usb_fill_int_urb(nct6694->int_in_urb, udev, pipe, > + nct6694->int_buffer, maxp, usb_int_callback, > + nct6694, int_endpoint->bInterval); > + ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL); > + if (ret) > + goto err_urb; > + > + dev_set_drvdata(&udev->dev, nct6694); > + usb_set_intfdata(iface, nct6694); > + > + ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev, > + ARRAY_SIZE(nct6694_dev)); > + if (ret) { > + dev_err(&udev->dev, "Failed to add mfd's child device\n"); > + goto err_mfd; > + } > + > + nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0); Where is the async_workqueue used? > + > + dev_info(&udev->dev, "Probed device: (%04X:%04X)\n", > + id->idVendor, id->idProduct); > + return 0; > + > +err_mfd: > + usb_kill_urb(nct6694->int_in_urb); > +err_urb: > + usb_free_urb(nct6694->int_in_urb); > + return ret; > +} > + > +static void nct6694_usb_disconnect(struct usb_interface *iface) > +{ > + struct usb_device *udev = interface_to_usbdev(iface); > + struct nct6694 *nct6694 = usb_get_intfdata(iface); > + > + mfd_remove_devices(&udev->dev); > + flush_workqueue(nct6694->async_workqueue); > + destroy_workqueue(nct6694->async_workqueue); > + usb_set_intfdata(iface, NULL); I think this is not needed. > + usb_kill_urb(nct6694->int_in_urb); > + usb_free_urb(nct6694->int_in_urb); > +} > + > +static const struct usb_device_id nct6694_ids[] = { > + { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)}, > + {}, > +}; > +MODULE_DEVICE_TABLE(usb, nct6694_ids); > + > +static struct usb_driver nct6694_usb_driver = { > + .name = DRVNAME, > + .id_table = nct6694_ids, > + .probe = nct6694_usb_probe, > + .disconnect = nct6694_usb_disconnect, > +}; > + > +module_usb_driver(nct6694_usb_driver); > + > +MODULE_DESCRIPTION("USB-MFD driver for NCT6694"); > +MODULE_AUTHOR("Ming Yu <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..0797564363be > --- /dev/null > +++ b/include/linux/mfd/nct6694.h > @@ -0,0 +1,168 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Nuvoton NCT6694 USB transaction and data structure. > + * > + * Copyright (C) 2024 Nuvoton Technology Corp. > + */ > + > +#ifndef __MFD_NCT6694_H > +#define __MFD_NCT6694_H > + > +#define NCT6694_DEV_GPIO "nct6694-gpio" > +#define NCT6694_DEV_I2C "nct6694-i2c" > +#define NCT6694_DEV_CAN "nct6694-can" > +#define NCT6694_DEV_WDT "nct6694-wdt" > +#define NCT6694_DEV_IIO "nct6694-iio" > +#define NCT6694_DEV_HWMON "nct6694-hwmon" > +#define NCT6694_DEV_PWM "nct6694-pwm" > +#define NCT6694_DEV_RTC "nct6694-rtc" > + > +#define NCT6694_VENDOR_ID 0x0416 > +#define NCT6694_PRODUCT_ID 0x200B > +#define INT_IN_ENDPOINT 0x81 > +#define BULK_IN_ENDPOINT 0x82 In Linux we don't add the 0x80 for IN endpoint, the framework does this for you. > +#define BULK_OUT_ENDPOINT 0x03 > +#define MAX_PACKET_SZ 0x100 > + > +#define CMD_PACKET_SZ 0x8 > +#define HCTRL_SET 0x40 > +#define HCTRL_GET 0x80 > + > +#define REQUEST_MOD_IDX 0x01 > +#define REQUEST_CMD_IDX 0x02 > +#define REQUEST_SEL_IDX 0x03 > +#define REQUEST_HCTRL_IDX 0x04 > +#define REQUEST_LEN_L_IDX 0x06 > +#define REQUEST_LEN_H_IDX 0x07 > + > +#define RESPONSE_STS_IDX 0x01 > + > +#define INT_IN_IRQ_IDX 0x00 > +#define GPIO_IRQ_STATUS BIT(0) > +#define CAN_IRQ_STATUS BIT(2) > +#define RTC_IRQ_STATUS BIT(3) > + > +#define URB_TIMEOUT 1000 > + > +/* > + * struct nct6694 - Nuvoton NCT6694 structure > + * > + * @udev: Pointer to the USB device > + * @int_in_urb: Interrupt pipe urb > + * @access_lock: USB transaction lock > + * @handler_list: List of registered handlers > + * @async_workqueue: Workqueue of processing asynchronous work > + * @tx_buffer: USB write message buffer > + * @rx_buffer: USB read message buffer > + * @cmd_buffer: USB send command message buffer > + * @int_buffer: USB receive interrupt message buffer > + * @lock: Handlers lock > + * @timeout: URB timeout > + * @maxp: Maximum packet of bulk pipe > + */ > +struct nct6694 { > + struct usb_device *udev; > + struct urb *int_in_urb; > + struct list_head handler_list; > + struct workqueue_struct *async_workqueue; > + > + /* Make sure that every USB transaction is not interrupted */ > + struct mutex access_lock; > + > + unsigned char *tx_buffer; > + unsigned char *rx_buffer; > + unsigned char *cmd_buffer; > + unsigned char *int_buffer; > + > + /* Prevent races within handlers */ > + spinlock_t lock; > + > + /* time in msec to wait for the urb to the complete */ > + long timeout; > + > + /* Bulk pipe maximum packet for each transaction */ > + int maxp; > +}; > + > +/* > + * struct nct6694_handler_entry - Stores the interrupt handling information > + * for each registered peripheral > + * > + * @irq_bit: The bit in irq_status[INT_IN_IRQ_IDX] representing interrupt ^^^ I think this comment could be more precise, you can emphasize, that it's not the bit number but the bit mask. > + * @handler: Function pointer to the interrupt handler of the peripheral > + * @private_data: Private data specific to the peripheral driver > + * @list: Node used to link to the handler_list > + */ > +struct nct6694_handler_entry { > + int irq_bit; the int_status you compare against in the IRQ callback ist a unsigned char. Better make all a u8. > + void (*handler)(void *private_data); > + void *private_data; > + struct list_head list; > +}; regards, Marc
On 10/24/24 01:59, Ming Yu wrote: > This driver supports Watchdog timer functionality for NCT6694 MFD > device based on USB interface. > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > --- > MAINTAINERS | 1 + > drivers/watchdog/Kconfig | 11 ++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/nct6694_wdt.c | 329 +++++++++++++++++++++++++++++++++ > 4 files changed, 342 insertions(+) > create mode 100644 drivers/watchdog/nct6694_wdt.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index eccd5e795daa..63387c0d4ab6 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16442,6 +16442,7 @@ F: drivers/gpio/gpio-nct6694.c > F: drivers/i2c/busses/i2c-nct6694.c > F: drivers/mfd/nct6694.c > F: drivers/net/can/nct6694_canfd.c > +F: drivers/watchdog/nct6694_wdt.c > F: include/linux/mfd/nct6694.h > > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 684b9fe84fff..bc9d63d69204 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -739,6 +739,17 @@ config MAX77620_WATCHDOG > MAX77620 chips. To compile this driver as a module, > choose M here: the module will be called max77620_wdt. > > +config NCT6694_WATCHDOG > + tristate "Nuvoton NCT6694 watchdog support" > + depends on MFD_NCT6694 > + select WATCHDOG_CORE > + help > + If you say yes to this option, support will be included for Nuvoton > + NCT6694, a USB device to watchdog timer. > + > + This driver can also be built as a module. If so, the module > + will be called nct6694_wdt. > + > config IMX2_WDT > tristate "IMX2+ Watchdog" > depends on ARCH_MXC || ARCH_LAYERSCAPE || COMPILE_TEST > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index ab6f2b41e38e..453ceacd43ab 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -231,6 +231,7 @@ obj-$(CONFIG_WM831X_WATCHDOG) += wm831x_wdt.o > obj-$(CONFIG_WM8350_WATCHDOG) += wm8350_wdt.o > obj-$(CONFIG_MAX63XX_WATCHDOG) += max63xx_wdt.o > obj-$(CONFIG_MAX77620_WATCHDOG) += max77620_wdt.o > +obj-$(CONFIG_NCT6694_WATCHDOG) += nct6694_wdt.o > obj-$(CONFIG_ZIIRAVE_WATCHDOG) += ziirave_wdt.o > obj-$(CONFIG_SOFT_WATCHDOG) += softdog.o > obj-$(CONFIG_MENF21BMC_WATCHDOG) += menf21bmc_wdt.o > diff --git a/drivers/watchdog/nct6694_wdt.c b/drivers/watchdog/nct6694_wdt.c > new file mode 100644 > index 000000000000..68e2926ec504 > --- /dev/null > +++ b/drivers/watchdog/nct6694_wdt.c > @@ -0,0 +1,329 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Nuvoton NCT6694 WDT driver based on USB interface. > + * > + * Copyright (C) 2024 Nuvoton Technology Corp. > + */ > + > +#include <linux/watchdog.h> > +#include <linux/slab.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mfd/core.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/nct6694.h> > + > +#define DRVNAME "nct6694-wdt" > + > +#define WATCHDOG_TIMEOUT 10 > +#define WATCHDOG_PRETIMEOUT 0 > + > +/* Host interface */ > +#define REQUEST_WDT_MOD 0x07 > + > +/* Message Channel*/ > +/* Command 00h */ > +#define REQUEST_WDT_CMD0_LEN 0x0F > +#define REQUEST_WDT_CMD0_OFFSET(idx) (idx ? 0x0100 : 0x0000) /* OFFSET = SEL|CMD */ > +#define WDT_PRETIMEOUT_IDX 0x00 > +#define WDT_PRETIMEOUT_LEN 0x04 /* PRETIMEOUT(3byte) | ACT(1byte) */ > +#define WDT_TIMEOUT_IDX 0x04 > +#define WDT_TIMEOUT_LEN 0x04 /* TIMEOUT(3byte) | ACT(1byte) */ > +#define WDT_COUNTDOWN_IDX 0x0C > +#define WDT_COUNTDOWN_LEN 0x03 > + > +#define WDT_PRETIMEOUT_ACT BIT(1) > +#define WDT_TIMEOUT_ACT BIT(1) > + > +/* Command 01h */ > +#define REQUEST_WDT_CMD1_LEN 0x04 > +#define REQUEST_WDT_CMD1_OFFSET(idx) (idx ? 0x0101 : 0x0001) /* OFFSET = SEL|CMD */ > +#define WDT_CMD_IDX 0x00 > +#define WDT_CMD_LEN 0x04 > + > +static unsigned int timeout; > +module_param(timeout, int, 0); > +MODULE_PARM_DESC(timeout, "Watchdog timeout in seconds"); > + > +static unsigned int pretimeout; > +module_param(pretimeout, int, 0); > +MODULE_PARM_DESC(pretimeout, "Watchdog pre-timeout in seconds"); > + > +static bool nowayout = WATCHDOG_NOWAYOUT; > +module_param(nowayout, bool, 0); > +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default=" > + __MODULE_STRING(WATCHDOG_NOWAYOUT) ")"); > + > +struct nct6694_wdt_data { > + struct nct6694 *nct6694; > + struct watchdog_device wdev; > + unsigned int wdev_idx; > +}; > + > +static inline void set_buf32(void *buf, u32 u32_val) > +{ > + u8 *p = (u8 *)buf; > + > + p[0] = u32_val & 0xFF; > + p[1] = (u32_val >> 8) & 0xFF; > + p[2] = (u32_val >> 16) & 0xFF; > + p[3] = (u32_val >> 24) & 0xFF; > +} > + > +static int nct6694_wdt_start(struct watchdog_device *wdev) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + > + pr_debug("%s: WDT(%d) Start\n", __func__, data->wdev_idx); > + > + return 0; > +} > + > +static int nct6694_wdt_stop(struct watchdog_device *wdev) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + struct nct6694 *nct6694 = data->nct6694; > + unsigned char buf[REQUEST_WDT_CMD1_LEN] = {'W', 'D', 'T', 'C'}; > + int ret; > + > + pr_debug("%s: WDT(%d) Close\n", __func__, data->wdev_idx); > + ret = nct6694_write_msg(nct6694, REQUEST_WDT_MOD, > + REQUEST_WDT_CMD1_OFFSET(data->wdev_idx), > + REQUEST_WDT_CMD1_LEN, buf); > + if (ret) > + pr_err("%s: Failed to start WDT device!\n", __func__); Please refrain from logging noise. Besides, the message is wrong: the watchdog is stopped here, not started. Also, all messages should use dev_, not pr_ functions. > + > + return ret; > +} > + > +static int nct6694_wdt_ping(struct watchdog_device *wdev) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + struct nct6694 *nct6694 = data->nct6694; > + unsigned char buf[REQUEST_WDT_CMD1_LEN] = {'W', 'D', 'T', 'S'}; > + int ret; > + > + pr_debug("%s: WDT(%d) Ping\n", __func__, data->wdev_idx); > + ret = nct6694_write_msg(nct6694, REQUEST_WDT_MOD, > + REQUEST_WDT_CMD1_OFFSET(data->wdev_idx), > + REQUEST_WDT_CMD1_LEN, buf); > + if (ret) > + pr_err("%s: Failed to ping WDT device!\n", __func__); Same as above and everywhere else. > + > + return ret; > +} > + > +static int nct6694_wdt_set_timeout(struct watchdog_device *wdev, > + unsigned int timeout) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + struct nct6694 *nct6694 = data->nct6694; > + unsigned int timeout_fmt, pretimeout_fmt; > + unsigned char buf[REQUEST_WDT_CMD0_LEN]; > + int ret; > + > + if (timeout < wdev->pretimeout) { > + pr_err("%s: 'timeout' must be greater than 'pre timeout'!\n", > + __func__); > + return -EINVAL; the driver is supposed to adjust pretimeout in this case. And please, again, refrain from logging noise. > + } > + > + timeout_fmt = timeout * 1000 | (WDT_TIMEOUT_ACT << 24); > + pretimeout_fmt = wdev->pretimeout * 1000 | (WDT_PRETIMEOUT_ACT << 24); > + set_buf32(&buf[WDT_TIMEOUT_IDX], le32_to_cpu(timeout_fmt)); > + set_buf32(&buf[WDT_PRETIMEOUT_IDX], le32_to_cpu(pretimeout_fmt)); > + > + ret = nct6694_write_msg(nct6694, REQUEST_WDT_MOD, > + REQUEST_WDT_CMD0_OFFSET(data->wdev_idx), > + REQUEST_WDT_CMD0_LEN, buf); > + if (ret) { > + pr_err("%s: Don't write the setup command in Start stage!\n", > + __func__); > + return ret; > + } > + > + wdev->timeout = timeout; > + > + return 0; > +} > + > +static int nct6694_wdt_set_pretimeout(struct watchdog_device *wdev, > + unsigned int pretimeout) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + struct nct6694 *nct6694 = data->nct6694; > + unsigned int timeout_fmt, pretimeout_fmt; > + unsigned char buf[REQUEST_WDT_CMD0_LEN]; > + int ret; > + > + if (pretimeout > wdev->timeout) { > + pr_err("%s: 'pre timeout' must be less than 'timeout'!\n", > + __func__); > + return -EINVAL; Already checked by the watchdog core. > + } > + timeout_fmt = wdev->timeout * 1000 | (WDT_TIMEOUT_ACT << 24); > + pretimeout_fmt = pretimeout * 1000 | (WDT_PRETIMEOUT_ACT << 24); > + set_buf32(&buf[WDT_TIMEOUT_IDX], le32_to_cpu(timeout_fmt)); > + set_buf32(&buf[WDT_PRETIMEOUT_IDX], le32_to_cpu(pretimeout_fmt)); > + > + ret = nct6694_write_msg(nct6694, REQUEST_WDT_MOD, > + REQUEST_WDT_CMD0_OFFSET(data->wdev_idx), > + REQUEST_WDT_CMD0_LEN, buf); > + if (ret) { > + pr_err("%s: Don't write the setup command in Start stage!\n", __func__); Besides it being noise, I don't even understand what this message is supposed to mean, and neither would anyone else. > + return ret; > + } > + > + wdev->pretimeout = pretimeout; > + return 0; > +} > + > +static unsigned int nct6694_wdt_get_time(struct watchdog_device *wdev) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + struct nct6694 *nct6694 = data->nct6694; > + unsigned char buf[WDT_COUNTDOWN_LEN]; > + unsigned int timeleft_ms; > + int ret; > + > + ret = nct6694_read_msg(nct6694, REQUEST_WDT_MOD, > + REQUEST_WDT_CMD0_OFFSET(data->wdev_idx), > + REQUEST_WDT_CMD0_LEN, WDT_COUNTDOWN_IDX, > + WDT_COUNTDOWN_LEN, buf); > + if (ret) > + pr_err("%s: Failed to get WDT device!\n", __func__); > + > + timeleft_ms = ((buf[2] << 16) | (buf[1] << 8) | buf[0]) & 0xFFFFFF; If the above command failed this will be a random number. > + > + return timeleft_ms / 1000; > +} > + > +static int nct6694_wdt_setup(struct watchdog_device *wdev) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + struct nct6694 *nct6694 = data->nct6694; > + unsigned char buf[REQUEST_WDT_CMD0_LEN] = {0}; > + unsigned int timeout_fmt, pretimeout_fmt; > + int ret; > + > + if (timeout) > + wdev->timeout = timeout; > + Already set. > + if (pretimeout) { > + wdev->pretimeout = pretimeout; Pretimeout is already set in the probe function. Do it completely there. > + pretimeout_fmt = wdev->pretimeout * 1000 | (WDT_PRETIMEOUT_ACT << 24); > + } else { > + pretimeout_fmt = 0; > + } > + > + timeout_fmt = wdev->timeout * 1000 | (WDT_TIMEOUT_ACT << 24); > + set_buf32(&buf[WDT_TIMEOUT_IDX], le32_to_cpu(timeout_fmt)); > + set_buf32(&buf[WDT_PRETIMEOUT_IDX], le32_to_cpu(pretimeout_fmt)); > + > + ret = nct6694_write_msg(nct6694, REQUEST_WDT_MOD, > + REQUEST_WDT_CMD0_OFFSET(data->wdev_idx), > + REQUEST_WDT_CMD0_LEN, buf); This seems pretty pointless at this time. Why not do it in the watchdog start function ? > + if (ret) > + return ret; > + > + pr_info("Setting WDT(%d): timeout = %d, pretimeout = %d\n", > + data->wdev_idx, wdev->timeout, wdev->pretimeout); > + > + return 0; > +} > + > +static const struct watchdog_info nct6694_wdt_info = { > + .options = WDIOF_SETTIMEOUT | > + WDIOF_KEEPALIVEPING | > + WDIOF_MAGICCLOSE | > + WDIOF_PRETIMEOUT, > + .identity = DRVNAME, > +}; > + > +static const struct watchdog_ops nct6694_wdt_ops = { > + .owner = THIS_MODULE, > + .start = nct6694_wdt_start, > + .stop = nct6694_wdt_stop, > + .set_timeout = nct6694_wdt_set_timeout, > + .set_pretimeout = nct6694_wdt_set_pretimeout, > + .get_timeleft = nct6694_wdt_get_time, > + .ping = nct6694_wdt_ping, > +}; > + > +static int nct6694_wdt_probe(struct platform_device *pdev) > +{ > + const struct mfd_cell *cell = mfd_get_cell(pdev); > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > + struct nct6694_wdt_data *data; > + struct watchdog_device *wdev; > + int ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->nct6694 = nct6694; > + data->wdev_idx = cell->id; > + > + wdev = &data->wdev; > + wdev->info = &nct6694_wdt_info; > + wdev->ops = &nct6694_wdt_ops; > + wdev->timeout = WATCHDOG_TIMEOUT; > + wdev->pretimeout = WATCHDOG_PRETIMEOUT; > + wdev->min_timeout = 1; > + wdev->max_timeout = 255; > + > + platform_set_drvdata(pdev, data); > + > + /* Register watchdog timer device to WDT framework */ > + watchdog_set_drvdata(&data->wdev, data); > + watchdog_init_timeout(&data->wdev, timeout, &pdev->dev); > + watchdog_set_nowayout(&data->wdev, nowayout); > + watchdog_stop_on_reboot(&data->wdev); > + > + ret = devm_watchdog_register_device(&pdev->dev, &data->wdev); > + if (ret) { > + dev_err(&pdev->dev, "%s: Failed to register watchdog device: %d\n", > + __func__, ret); > + return ret; > + } > + > + ret = nct6694_wdt_setup(&data->wdev); This is too late. It needs to be done before registering the watchdog. > + if (ret) { > + dev_err(&pdev->dev, "Failed to setup WDT device!\n"); > + return ret; > + } > + > + return 0; > +} > + > +static struct platform_driver nct6694_wdt_driver = { > + .driver = { > + .name = DRVNAME, > + }, > + .probe = nct6694_wdt_probe, > +}; > + > +static int __init nct6694_init(void) > +{ > + int err; > + > + err = platform_driver_register(&nct6694_wdt_driver); > + if (!err) { > + if (err) > + platform_driver_unregister(&nct6694_wdt_driver); > + } > + > + return err; > +} > +subsys_initcall(nct6694_init); > + > +static void __exit nct6694_exit(void) > +{ > + platform_driver_unregister(&nct6694_wdt_driver); > +} > +module_exit(nct6694_exit); > + > +MODULE_DESCRIPTION("USB-WDT driver for NCT6694"); > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > +MODULE_LICENSE("GPL");
On 10/24/24 01:59, Ming Yu wrote: > This driver supports Watchdog timer functionality for NCT6694 MFD > device based on USB interface. > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > --- [ ... ] > + > +static int nct6694_wdt_start(struct watchdog_device *wdev) > +{ > + struct nct6694_wdt_data *data = watchdog_get_drvdata(wdev); > + > + pr_debug("%s: WDT(%d) Start\n", __func__, data->wdev_idx); > + > + return 0; > +} > + That doesn't make sense. How is the watchdog started if not here ? Something is conceptually wrong. Guenter
Sorry, resending this email in plain text format. Dear Marc, Thank you for your comments. I'll add the nct6694_free_handler() function in the next patch. Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午5:12寫道： > > On 24.10.2024 16:59:14, Ming Yu wrote: > > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips, > > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC, > > PWM, and RTC. > > > > This driver implements USB device functionality and shares the > > chip's peripherals as a child device. > > > > Each child device can use the USB functions nct6694_read_msg() > > and nct6694_write_msg() to issue a command. They can also register > > a handler function that will be called when the USB device receives > > its interrupt pipe. > > > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > > --- > > MAINTAINERS | 7 + > > drivers/mfd/Kconfig | 10 + > > drivers/mfd/Makefile | 2 + > > drivers/mfd/nct6694.c | 394 ++++++++++++++++++++++++++++++++++++ > > include/linux/mfd/nct6694.h | 168 +++++++++++++++ > > 5 files changed, 581 insertions(+) > > create mode 100644 drivers/mfd/nct6694.c > > create mode 100644 include/linux/mfd/nct6694.h > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index e9659a5a7fb3..30157ca95cf3 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16434,6 +16434,13 @@ F: drivers/nubus/ > > F: include/linux/nubus.h > > F: include/uapi/linux/nubus.h > > > > +NUVOTON NCT6694 MFD DRIVER > > +M: Ming Yu <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 f9325bcce1b9..da2600958697 100644 > > --- a/drivers/mfd/Kconfig > > +++ b/drivers/mfd/Kconfig > > @@ -546,6 +546,16 @@ config MFD_MX25_TSADC > > i.MX25 processors. They consist of a conversion queue for general > > purpose ADC and a queue for Touchscreens. > > > > +config MFD_NCT6694 > > + tristate "Nuvoton NCT6694 support" > > + select MFD_CORE > > + depends on USB > > + help > > + This adds support for Nuvoton USB device NCT6694 sharing peripherals > > + This includes the USB devcie driver and core APIs. > > + Additional drivers must be enabled in order to use the functionality > > + of the device. > > + > > config MFD_HI6421_PMIC > > tristate "HiSilicon Hi6421 PMU/Codec IC" > > depends on OF > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > index 2a9f91e81af8..2cf816d67d03 100644 > > --- a/drivers/mfd/Makefile > > +++ b/drivers/mfd/Makefile > > @@ -116,6 +116,8 @@ obj-$(CONFIG_TWL6040_CORE) += twl6040.o > > > > obj-$(CONFIG_MFD_MX25_TSADC) += fsl-imx25-tsadc.o > > > > +obj-$(CONFIG_MFD_NCT6694) += nct6694.o > > + > > obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o > > obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o > > obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o > > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c > > new file mode 100644 > > index 000000000000..9838c7be0b98 > > --- /dev/null > > +++ b/drivers/mfd/nct6694.c > > @@ -0,0 +1,394 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Nuvoton NCT6694 MFD driver based on USB interface. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > + */ > > + > > +#include <linux/io.h> > > +#include <linux/usb.h> > > +#include <linux/slab.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/mfd/core.h> > > +#include <linux/mfd/nct6694.h> > > + > > +#define DRVNAME "nct6694-usb_mfd" > > + > > +#define MFD_DEV_SIMPLE(_name) \ > > +{ \ > > + .name = NCT6694_DEV_##_name, \ > > +} \ > > + > > +#define MFD_DEV_WITH_ID(_name, _id) \ > > +{ \ > > + .name = NCT6694_DEV_##_name, \ > > + .id = _id, \ > > +} > > + > > +/* MFD device resources */ > > +static const struct mfd_cell nct6694_dev[] = { > > + MFD_DEV_WITH_ID(GPIO, 0x0), > > + MFD_DEV_WITH_ID(GPIO, 0x1), > > + MFD_DEV_WITH_ID(GPIO, 0x2), > > + MFD_DEV_WITH_ID(GPIO, 0x3), > > + MFD_DEV_WITH_ID(GPIO, 0x4), > > + MFD_DEV_WITH_ID(GPIO, 0x5), > > + MFD_DEV_WITH_ID(GPIO, 0x6), > > + MFD_DEV_WITH_ID(GPIO, 0x7), > > + MFD_DEV_WITH_ID(GPIO, 0x8), > > + MFD_DEV_WITH_ID(GPIO, 0x9), > > + MFD_DEV_WITH_ID(GPIO, 0xA), > > + MFD_DEV_WITH_ID(GPIO, 0xB), > > + MFD_DEV_WITH_ID(GPIO, 0xC), > > + MFD_DEV_WITH_ID(GPIO, 0xD), > > + MFD_DEV_WITH_ID(GPIO, 0xE), > > + MFD_DEV_WITH_ID(GPIO, 0xF), > > + > > + MFD_DEV_WITH_ID(I2C, 0x0), > > + MFD_DEV_WITH_ID(I2C, 0x1), > > + MFD_DEV_WITH_ID(I2C, 0x2), > > + MFD_DEV_WITH_ID(I2C, 0x3), > > + MFD_DEV_WITH_ID(I2C, 0x4), > > + MFD_DEV_WITH_ID(I2C, 0x5), > > + > > + MFD_DEV_WITH_ID(CAN, 0x0), > > + MFD_DEV_WITH_ID(CAN, 0x1), > > + > > + MFD_DEV_WITH_ID(WDT, 0x0), > > + MFD_DEV_WITH_ID(WDT, 0x1), > > + > > + MFD_DEV_SIMPLE(IIO), > > + MFD_DEV_SIMPLE(HWMON), > > + MFD_DEV_SIMPLE(PWM), > > + MFD_DEV_SIMPLE(RTC), > > +}; > > + > > +int nct6694_register_handler(struct nct6694 *nct6694, int irq_bit, > > + void (*handler)(void *), void *private_data) > > +{ > > + struct nct6694_handler_entry *entry; > > + unsigned long flags; > > + > > + entry = kmalloc(sizeof(*entry), GFP_KERNEL); > > + if (!entry) > > + return -ENOMEM; > > + > > + entry->irq_bit = irq_bit; > > + entry->handler = handler; > > + entry->private_data = private_data; > > + > > + spin_lock_irqsave(&nct6694->lock, flags); > > + list_add_tail(&entry->list, &nct6694->handler_list); > > + spin_unlock_irqrestore(&nct6694->lock, flags); > > + > > + return 0; > > +} > > +EXPORT_SYMBOL(nct6694_register_handler); > > Where's the corresponding nct6694_free_handler() function? > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Sorry, resending this email in plain text format. Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午5:57寫道： > > On 24.10.2024 16:59:14, Ming Yu wrote: > > +static int nct6694_usb_probe(struct usb_interface *iface, > > + const struct usb_device_id *id) > > +{ > > + struct usb_device *udev = interface_to_usbdev(iface); > > + struct device *dev = &udev->dev; > > + struct usb_host_interface *interface; > > + struct usb_endpoint_descriptor *int_endpoint; > > + struct nct6694 *nct6694; > > + int pipe, maxp, bulk_pipe; > > + int ret = EINVAL; > > + > > + interface = iface->cur_altsetting; > > + /* Binding interface class : 0xFF */ > > + if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC || > > + interface->desc.bInterfaceSubClass != 0x00 || > > + interface->desc.bInterfaceProtocol != 0x00) > > + return -ENODEV; > > I think you can use USB_DEVICE_INFO() and remove this manual check > > https://elixir.bootlin.com/linux/v6.11.5/source/include/linux/usb.h#L1056 > > [...] [Ming] Okay! I'll remove it and change USB_DEVICE() to USB_DEVICE_AND_INTERFACE_INFO(). > > > + > > +static const struct usb_device_id nct6694_ids[] = { > > + { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(usb, nct6694_ids); > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Sorry, resending this email in plain text format. Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午11:21寫道： > > On 24.10.2024 16:59:14, Ming Yu wrote: > > The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips, > > 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC, > > PWM, and RTC. > > > > This driver implements USB device functionality and shares the > > chip's peripherals as a child device. > > > > Each child device can use the USB functions nct6694_read_msg() > > and nct6694_write_msg() to issue a command. They can also register > > a handler function that will be called when the USB device receives > > its interrupt pipe. > > [...] > > > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c > > new file mode 100644 > > index 000000000000..9838c7be0b98 > > --- /dev/null > > +++ b/drivers/mfd/nct6694.c > > [...] > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, > > + u8 rd_idx, u8 rd_len, unsigned char *buf) > > why not make buf a void *? [Ming] I'll change the type in the next patch. > > > +{ > > + struct usb_device *udev = nct6694->udev; > > + unsigned char err_status; > > + int len, packet_len, tx_len, rx_len; > > + int i, ret; > > + > > + mutex_lock(&nct6694->access_lock); > > + > > + /* Send command packet to USB device */ > > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET; > > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; > > + > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > > + nct6694->timeout); > > + if (ret) > > + goto err; > > + > > + /* Receive response packet from USB device */ > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > > + nct6694->timeout); > > + if (ret) > > + goto err; > > + > > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > > + > > + /* > > + * Segmented reception of messages that exceed the size of USB bulk > > + * pipe packets. > > + */ > > The Linux USB stack can receive bulk messages longer than the max packet size. [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device. The core will divide packet 256 bytes for high speed USB device, but it is exceeds the hardware limitation, so I am dividing it manually. > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > + if (len > nct6694->maxp) > > + packet_len = nct6694->maxp; > > + else > > + packet_len = len; > > + > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > + nct6694->rx_buffer + nct6694->maxp * i, > > + packet_len, &rx_len, nct6694->timeout); > > + if (ret) > > + goto err; > > + } > > + > > + for (i = 0; i < rd_len; i++) > > + buf[i] = nct6694->rx_buffer[i + rd_idx]; > > memcpy()? > > Or why don't you directly receive data into the provided buffer? Copying > of the data doesn't make it faster. > > On the other hand, receiving directly into the target buffer means the > target buffer must not live on the stack. [Ming] Okay! I'll change it to memcpy(). This is my perspective: the data is uniformly received by the rx_bffer held by the MFD device. does it need to be changed? > > > + > > + if (err_status) { > > + pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status); > > + ret = -EIO; > > + } > > + > > +err: > > + mutex_unlock(&nct6694->access_lock); > > + return ret; > > +} > > +EXPORT_SYMBOL(nct6694_read_msg); > > + > > +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > > + u16 length, unsigned char *buf) > > +{ > > + struct usb_device *udev = nct6694->udev; > > + unsigned char err_status; > > + int len, packet_len, tx_len, rx_len; > > + int i, ret; > > + > > + mutex_lock(&nct6694->access_lock); > > + > > + /* Send command packet to USB device */ > > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_SET; > > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; > > What about creating a struct that describes the cmd_buffer layout? [Ming] I've thought about this before, thanks for your comments. > > > + > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > > + nct6694->timeout); > > + if (ret) > > + goto err; > > + > > + /* > > + * Segmented transmission of messages that exceed the size of USB bulk > > + * pipe packets. > > + */ > > same as above > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > + if (len > nct6694->maxp) > > + packet_len = nct6694->maxp; > > + else > > + packet_len = len; > > + > > + memcpy(nct6694->tx_buffer + nct6694->maxp * i, > > + buf + nct6694->maxp * i, packet_len); > > + > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > > + nct6694->tx_buffer + nct6694->maxp * i, > > + packet_len, &tx_len, nct6694->timeout); > > + if (ret) > > + goto err; > > + } > > + > > + /* Receive response packet from USB device */ > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > > + nct6694->timeout); > > + if (ret) > > + goto err; > > + > > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > > + > > + /* > > + * Segmented reception of messages that exceed the size of USB bulk > > + * pipe packets. > > + */ > > same as above > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > + if (len > nct6694->maxp) > > + packet_len = nct6694->maxp; > > + else > > + packet_len = len; > > + > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > + nct6694->rx_buffer + nct6694->maxp * i, > > + packet_len, &rx_len, nct6694->timeout); > > + if (ret) > > + goto err; > > + } > > + > > + memcpy(buf, nct6694->rx_buffer, length); > > why not rx into the destination buffer directly? > > > + > > + if (err_status) { > > + pr_debug("%s: MSG CH status = %2Xh\n", __func__, err_status); > > + ret = -EIO; > > + } > > + > > +err: > > + mutex_unlock(&nct6694->access_lock); > > + return ret; > > +} > > +EXPORT_SYMBOL(nct6694_write_msg); > > [...] > > > +static int nct6694_usb_probe(struct usb_interface *iface, > > + const struct usb_device_id *id) > > +{ > > + struct usb_device *udev = interface_to_usbdev(iface); > > + struct device *dev = &udev->dev; > > + struct usb_host_interface *interface; > > + struct usb_endpoint_descriptor *int_endpoint; > > + struct nct6694 *nct6694; > > + int pipe, maxp, bulk_pipe; > > + int ret = EINVAL; > > + > > + interface = iface->cur_altsetting; > > + /* Binding interface class : 0xFF */ > > + if (interface->desc.bInterfaceClass != USB_CLASS_VENDOR_SPEC || > > + interface->desc.bInterfaceSubClass != 0x00 || > > + interface->desc.bInterfaceProtocol != 0x00) > > + return -ENODEV; > > + > > + int_endpoint = &interface->endpoint[0].desc; > > + if (!usb_endpoint_is_int_in(int_endpoint)) > > + return -ENODEV; > > + > > + nct6694 = devm_kzalloc(&udev->dev, sizeof(*nct6694), GFP_KERNEL); > > + if (!nct6694) > > + return -ENOMEM; > > + > > + pipe = usb_rcvintpipe(udev, INT_IN_ENDPOINT); > > + maxp = usb_maxpacket(udev, pipe); > > better move these 2 down to the usb_fill_int_urb(). [Ming] Okay! I'll move these in the next patch. > > > + > > + nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ, > > + sizeof(unsigned char), GFP_KERNEL); > > + if (!nct6694->cmd_buffer) > > + return -ENOMEM; > > + nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > + sizeof(unsigned char), GFP_KERNEL); > > + if (!nct6694->rx_buffer) > > + return -ENOMEM; > > + nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > + sizeof(unsigned char), GFP_KERNEL); > > + if (!nct6694->tx_buffer) > > + return -ENOMEM; > > + nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > + sizeof(unsigned char), GFP_KERNEL); > > + if (!nct6694->int_buffer) > > + return -ENOMEM; > > + > > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > > + if (!nct6694->int_in_urb) { > > + dev_err(&udev->dev, "Failed to allocate INT-in urb!\n"); > > + return -ENOMEM; > > + } > > + > > + /* Bulk pipe maximum packet for each transaction */ > > + bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT); > > + nct6694->maxp = usb_maxpacket(udev, bulk_pipe); > > + > > + mutex_init(&nct6694->access_lock); > > + nct6694->udev = udev; > > + nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */ > > + > > + INIT_LIST_HEAD(&nct6694->handler_list); > > + spin_lock_init(&nct6694->lock); > > + > > + usb_fill_int_urb(nct6694->int_in_urb, udev, pipe, > > + nct6694->int_buffer, maxp, usb_int_callback, > > + nct6694, int_endpoint->bInterval); > > + ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL); > > + if (ret) > > + goto err_urb; > > + > > + dev_set_drvdata(&udev->dev, nct6694); > > + usb_set_intfdata(iface, nct6694); > > + > > + ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev, > > + ARRAY_SIZE(nct6694_dev)); > > + if (ret) { > > + dev_err(&udev->dev, "Failed to add mfd's child device\n"); > > + goto err_mfd; > > + } > > + > > + nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0); > > Where is the async_workqueue used? > > > + > > + dev_info(&udev->dev, "Probed device: (%04X:%04X)\n", > > + id->idVendor, id->idProduct); > > + return 0; > > + > > +err_mfd: > > + usb_kill_urb(nct6694->int_in_urb); > > +err_urb: > > + usb_free_urb(nct6694->int_in_urb); > > + return ret; > > +} > > + > > +static void nct6694_usb_disconnect(struct usb_interface *iface) > > +{ > > + struct usb_device *udev = interface_to_usbdev(iface); > > + struct nct6694 *nct6694 = usb_get_intfdata(iface); > > + > > + mfd_remove_devices(&udev->dev); > > + flush_workqueue(nct6694->async_workqueue); > > + destroy_workqueue(nct6694->async_workqueue); > > + usb_set_intfdata(iface, NULL); > > I think this is not needed. [Ming] I'll remove it in the next patch. > > > + usb_kill_urb(nct6694->int_in_urb); > > + usb_free_urb(nct6694->int_in_urb); > > +} > > + > > +static const struct usb_device_id nct6694_ids[] = { > > + { USB_DEVICE(NCT6694_VENDOR_ID, NCT6694_PRODUCT_ID)}, > > + {}, > > +}; > > +MODULE_DEVICE_TABLE(usb, nct6694_ids); > > + > > +static struct usb_driver nct6694_usb_driver = { > > + .name = DRVNAME, > > + .id_table = nct6694_ids, > > + .probe = nct6694_usb_probe, > > + .disconnect = nct6694_usb_disconnect, > > +}; > > + > > +module_usb_driver(nct6694_usb_driver); > > + > > +MODULE_DESCRIPTION("USB-MFD driver for NCT6694"); > > +MODULE_AUTHOR("Ming Yu <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..0797564363be > > --- /dev/null > > +++ b/include/linux/mfd/nct6694.h > > @@ -0,0 +1,168 @@ > > +/* SPDX-License-Identifier: GPL-2.0 */ > > +/* > > + * Nuvoton NCT6694 USB transaction and data structure. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > + */ > > + > > +#ifndef __MFD_NCT6694_H > > +#define __MFD_NCT6694_H > > + > > +#define NCT6694_DEV_GPIO "nct6694-gpio" > > +#define NCT6694_DEV_I2C "nct6694-i2c" > > +#define NCT6694_DEV_CAN "nct6694-can" > > +#define NCT6694_DEV_WDT "nct6694-wdt" > > +#define NCT6694_DEV_IIO "nct6694-iio" > > +#define NCT6694_DEV_HWMON "nct6694-hwmon" > > +#define NCT6694_DEV_PWM "nct6694-pwm" > > +#define NCT6694_DEV_RTC "nct6694-rtc" > > + > > +#define NCT6694_VENDOR_ID 0x0416 > > +#define NCT6694_PRODUCT_ID 0x200B > > +#define INT_IN_ENDPOINT 0x81 > > +#define BULK_IN_ENDPOINT 0x82 > > In Linux we don't add the 0x80 for IN endpoint, the framework does this > for you. [Ming] I'll change it in the next patch. > > > +#define BULK_OUT_ENDPOINT 0x03 > > +#define MAX_PACKET_SZ 0x100 > > + > > +#define CMD_PACKET_SZ 0x8 > > +#define HCTRL_SET 0x40 > > +#define HCTRL_GET 0x80 > > + > > +#define REQUEST_MOD_IDX 0x01 > > +#define REQUEST_CMD_IDX 0x02 > > +#define REQUEST_SEL_IDX 0x03 > > +#define REQUEST_HCTRL_IDX 0x04 > > +#define REQUEST_LEN_L_IDX 0x06 > > +#define REQUEST_LEN_H_IDX 0x07 > > + > > +#define RESPONSE_STS_IDX 0x01 > > + > > +#define INT_IN_IRQ_IDX 0x00 > > +#define GPIO_IRQ_STATUS BIT(0) > > +#define CAN_IRQ_STATUS BIT(2) > > +#define RTC_IRQ_STATUS BIT(3) > > + > > +#define URB_TIMEOUT 1000 > > + > > +/* > > + * struct nct6694 - Nuvoton NCT6694 structure > > + * > > + * @udev: Pointer to the USB device > > + * @int_in_urb: Interrupt pipe urb > > + * @access_lock: USB transaction lock > > + * @handler_list: List of registered handlers > > + * @async_workqueue: Workqueue of processing asynchronous work > > + * @tx_buffer: USB write message buffer > > + * @rx_buffer: USB read message buffer > > + * @cmd_buffer: USB send command message buffer > > + * @int_buffer: USB receive interrupt message buffer > > + * @lock: Handlers lock > > + * @timeout: URB timeout > > + * @maxp: Maximum packet of bulk pipe > > + */ > > +struct nct6694 { > > + struct usb_device *udev; > > + struct urb *int_in_urb; > > + struct list_head handler_list; > > + struct workqueue_struct *async_workqueue; > > + > > + /* Make sure that every USB transaction is not interrupted */ > > + struct mutex access_lock; > > + > > + unsigned char *tx_buffer; > > + unsigned char *rx_buffer; > > + unsigned char *cmd_buffer; > > + unsigned char *int_buffer; > > + > > + /* Prevent races within handlers */ > > + spinlock_t lock; > > + > > + /* time in msec to wait for the urb to the complete */ > > + long timeout; > > + > > + /* Bulk pipe maximum packet for each transaction */ > > + int maxp; > > +}; > > + > > +/* > > + * struct nct6694_handler_entry - Stores the interrupt handling information > > + * for each registered peripheral > > + * > > + * @irq_bit: The bit in irq_status[INT_IN_IRQ_IDX] representing interrupt > ^^^ > > I think this comment could be more precise, you can emphasize, that it's > not the bit number but the bit mask. [Ming] Okay! I'll change it in the next patch. > > > + * @handler: Function pointer to the interrupt handler of the peripheral > > + * @private_data: Private data specific to the peripheral driver > > + * @list: Node used to link to the handler_list > > + */ > > +struct nct6694_handler_entry { > > + int irq_bit; > > the int_status you compare against in the IRQ callback ist a unsigned > char. Better make all a u8. [Ming] Okay! I'll change it in the next patch. > > > + void (*handler)(void *private_data); > > + void *private_data; > > + struct list_head list; > > +}; > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Dear Marc, Excuse me, I'm a bit confused. Is there anything I need to improve on? Thanks, Ming Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午11:34寫道： > > On 24.10.2024 17:20:57, Marc Kleine-Budde wrote: > > [...] > > > > + nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ, > > > + sizeof(unsigned char), GFP_KERNEL); > > > + if (!nct6694->cmd_buffer) > > > + return -ENOMEM; > > > + nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > > + sizeof(unsigned char), GFP_KERNEL); > > > + if (!nct6694->rx_buffer) > > > + return -ENOMEM; > > > + nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > > + sizeof(unsigned char), GFP_KERNEL); > > > + if (!nct6694->tx_buffer) > > > + return -ENOMEM; > > > + nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > > + sizeof(unsigned char), GFP_KERNEL); > > > + if (!nct6694->int_buffer) > > > + return -ENOMEM; > > > + > > > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > > > + if (!nct6694->int_in_urb) { > > > + dev_err(&udev->dev, "Failed to allocate INT-in urb!\n"); > > > + return -ENOMEM; > > > + } > > > + > > > + /* Bulk pipe maximum packet for each transaction */ > > > + bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT); > > > + nct6694->maxp = usb_maxpacket(udev, bulk_pipe); > > > + > > > + mutex_init(&nct6694->access_lock); > > > + nct6694->udev = udev; > > > + nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */ > > > + > > > + INIT_LIST_HEAD(&nct6694->handler_list); > > > + spin_lock_init(&nct6694->lock); > > > + > > > + usb_fill_int_urb(nct6694->int_in_urb, udev, pipe, > > > + nct6694->int_buffer, maxp, usb_int_callback, > > > + nct6694, int_endpoint->bInterval); > > > + ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL); > > > + if (ret) > > > + goto err_urb; > > > + > > > + dev_set_drvdata(&udev->dev, nct6694); > > > + usb_set_intfdata(iface, nct6694); > > > + > > > + ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev, > > > + ARRAY_SIZE(nct6694_dev)); > > > + if (ret) { > > > + dev_err(&udev->dev, "Failed to add mfd's child device\n"); > > > + goto err_mfd; > > > + } > > > + > > > + nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0); > > > > Where is the async_workqueue used? > > Sorry - it's used in the driver, which live in separate directories - > you can ignore this comment. > > But then the question comes up, it looks racy to _first_ add the devices > and _then_ the workqueue. > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Hi Marc, I think the currently planned IRQ process meets expectations. Is there anything that needs improvement? Thanks, Ming Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午7:57寫道： > > On 24.10.2024 16:59:13, Ming Yu wrote: > > This patch series introduces support for Nuvoton NCT6694, a peripheral > > expander based on USB interface. It models the chip as an MFD driver > > (1/9), GPIO driver(2/9), I2C Adapter driver(3/9), CANfd driver(4/9), > > WDT driver(5/9), HWMON driver(6/9), IIO driver(7/9), PWM driver(8/9), > > and RTC driver(9/9). > > > > The MFD driver implements USB device functionality to issue > > custom-define USB bulk pipe packets for NCT6694. Each child device can > > use the USB functions nct6694_read_msg() and nct6694_write_msg() to issue > > a command. They can also register a handler function that will be called > > when the USB device receives its interrupt pipe. > > What about implementing a proper IRQ demux handler instead? > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 25.10.2024 16:22:01, Ming Yu wrote: > Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午7:57寫道： > > On 24.10.2024 16:59:13, Ming Yu wrote: > > > This patch series introduces support for Nuvoton NCT6694, a peripheral > > > expander based on USB interface. It models the chip as an MFD driver > > > (1/9), GPIO driver(2/9), I2C Adapter driver(3/9), CANfd driver(4/9), > > > WDT driver(5/9), HWMON driver(6/9), IIO driver(7/9), PWM driver(8/9), > > > and RTC driver(9/9). > > > > > > The MFD driver implements USB device functionality to issue > > > custom-define USB bulk pipe packets for NCT6694. Each child device can > > > use the USB functions nct6694_read_msg() and nct6694_write_msg() to issue > > > a command. They can also register a handler function that will be called > > > when the USB device receives its interrupt pipe. > > > > What about implementing a proper IRQ demux handler instead? > I think the currently planned IRQ process meets expectations. > Is there anything that needs improvement? You can register the IRQs of the MFD device with the Linux kernel. This way the devices can request a threaded IRQ handler directly via the kernel function, instead of registering the callback. With a threaded IRQ handler you can directly call the nct6694_read_msg(), nct6694_write_msg() without the need to start a workqueue from the callback. Marc
On 25.10.2024 16:14:03, Ming Yu wrote: > Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月24日 週四 下午11:34寫道： > > > > On 24.10.2024 17:20:57, Marc Kleine-Budde wrote: > > > > [...] > > > > > > + nct6694->cmd_buffer = devm_kcalloc(dev, CMD_PACKET_SZ, > > > > + sizeof(unsigned char), GFP_KERNEL); > > > > + if (!nct6694->cmd_buffer) > > > > + return -ENOMEM; > > > > + nct6694->rx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > > > + sizeof(unsigned char), GFP_KERNEL); > > > > + if (!nct6694->rx_buffer) > > > > + return -ENOMEM; > > > > + nct6694->tx_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > > > + sizeof(unsigned char), GFP_KERNEL); > > > > + if (!nct6694->tx_buffer) > > > > + return -ENOMEM; > > > > + nct6694->int_buffer = devm_kcalloc(dev, MAX_PACKET_SZ, > > > > + sizeof(unsigned char), GFP_KERNEL); > > > > + if (!nct6694->int_buffer) > > > > + return -ENOMEM; > > > > + > > > > + nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL); > > > > + if (!nct6694->int_in_urb) { > > > > + dev_err(&udev->dev, "Failed to allocate INT-in urb!\n"); > > > > + return -ENOMEM; > > > > + } > > > > + > > > > + /* Bulk pipe maximum packet for each transaction */ > > > > + bulk_pipe = usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT); > > > > + nct6694->maxp = usb_maxpacket(udev, bulk_pipe); > > > > + > > > > + mutex_init(&nct6694->access_lock); > > > > + nct6694->udev = udev; > > > > + nct6694->timeout = URB_TIMEOUT; /* Wait until urb complete */ > > > > + > > > > + INIT_LIST_HEAD(&nct6694->handler_list); > > > > + spin_lock_init(&nct6694->lock); > > > > + > > > > + usb_fill_int_urb(nct6694->int_in_urb, udev, pipe, > > > > + nct6694->int_buffer, maxp, usb_int_callback, > > > > + nct6694, int_endpoint->bInterval); > > > > + ret = usb_submit_urb(nct6694->int_in_urb, GFP_KERNEL); > > > > + if (ret) > > > > + goto err_urb; > > > > + > > > > + dev_set_drvdata(&udev->dev, nct6694); > > > > + usb_set_intfdata(iface, nct6694); > > > > + > > > > + ret = mfd_add_hotplug_devices(&udev->dev, nct6694_dev, > > > > + ARRAY_SIZE(nct6694_dev)); > > > > + if (ret) { > > > > + dev_err(&udev->dev, "Failed to add mfd's child device\n"); > > > > + goto err_mfd; > > > > + } > > > > + > > > > + nct6694->async_workqueue = alloc_ordered_workqueue("asyn_workqueue", 0); > > > > > > Where is the async_workqueue used? > > > > Sorry - it's used in the driver, which live in separate directories - > > you can ignore this comment. > > > > But then the question comes up, it looks racy to _first_ add the devices > > and _then_ the workqueue. > Excuse me, I'm a bit confused. Is there anything I need to > improve on? It looks racy to _first_ add the devices and _then_ the workqueue. So the obvious solution is to allocate the worklist first and then add the devices. regards, Marc
On 25.10.2024 10:35:35, Marc Kleine-Budde wrote: > > Excuse me, I'm a bit confused. Is there anything I need to > > improve on? > > It looks racy to _first_ add the devices and _then_ the workqueue. > > So the obvious solution is to allocate the worklist first and then add workqueue > the devices. Marc
On 25.10.2024 16:08:10, Ming Yu wrote: > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, > > > + u8 rd_idx, u8 rd_len, unsigned char *buf) > > > > why not make buf a void *? > > [Ming] I'll change the type in the next patch. > > > > > > +{ > > > + struct usb_device *udev = nct6694->udev; > > > + unsigned char err_status; > > > + int len, packet_len, tx_len, rx_len; > > > + int i, ret; > > > + > > > + mutex_lock(&nct6694->access_lock); > > > + > > > + /* Send command packet to USB device */ > > > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > > > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > > > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > > > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET; > > > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > > > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; > > > + > > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > > > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > > > + nct6694->timeout); > > > + if (ret) > > > + goto err; > > > + > > > + /* Receive response packet from USB device */ > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > > > + nct6694->timeout); > > > + if (ret) > > > + goto err; > > > + > > > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > > > + > > > + /* > > > + * Segmented reception of messages that exceed the size of USB bulk > > > + * pipe packets. > > > + */ > > > > The Linux USB stack can receive bulk messages longer than the max packet size. > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device. > The core will divide packet 256 bytes for high speed USB device, but > it is exceeds > the hardware limitation, so I am dividing it manually. You say the endpoint descriptor is correctly reporting it's max packet size of 128, but the Linux USB will send packets of 256 bytes? > > > > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > > + if (len > nct6694->maxp) > > > + packet_len = nct6694->maxp; > > > + else > > > + packet_len = len; > > > + > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > + nct6694->rx_buffer + nct6694->maxp * i, > > > + packet_len, &rx_len, nct6694->timeout); > > > + if (ret) > > > + goto err; > > > + } > > > + > > > + for (i = 0; i < rd_len; i++) > > > + buf[i] = nct6694->rx_buffer[i + rd_idx]; > > > > memcpy()? > > > > Or why don't you directly receive data into the provided buffer? Copying > > of the data doesn't make it faster. > > > > On the other hand, receiving directly into the target buffer means the > > target buffer must not live on the stack. > > [Ming] Okay! I'll change it to memcpy(). fine! > This is my perspective: the data is uniformly received by the rx_bffer held > by the MFD device. does it need to be changed? My question is: Why do you first receive into the nct6694->rx_buffer and then memcpy() to the buffer provided by the caller, why don't you directly receive into the memory provided by the caller? Marc
Got it! I'll make the change in the next patch. Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月25日 週五 下午5:02寫道： > > On 25.10.2024 10:35:35, Marc Kleine-Budde wrote: > > > Excuse me, I'm a bit confused. Is there anything I need to > > > improve on? > > > > It looks racy to _first_ add the devices and _then_ the workqueue. > > > > So the obvious solution is to allocate the worklist first and then add > workqueue > > the devices. > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
Oh! I'm sorry about that I confused the packet size. The NCT6694 bulk maximum packet size is 256 bytes, and USB High speed bulk maximum packet size is 512 bytes. Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月25日 週五 下午6:08寫道： > > On 25.10.2024 16:08:10, Ming Yu wrote: > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, > > > > + u8 rd_idx, u8 rd_len, unsigned char *buf) > > > > > > why not make buf a void *? > > > > [Ming] I'll change the type in the next patch. > > > > > > > > > +{ > > > > + struct usb_device *udev = nct6694->udev; > > > > + unsigned char err_status; > > > > + int len, packet_len, tx_len, rx_len; > > > > + int i, ret; > > > > + > > > > + mutex_lock(&nct6694->access_lock); > > > > + > > > > + /* Send command packet to USB device */ > > > > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > > > > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > > > > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > > > > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET; > > > > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > > > > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; > > > > + > > > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > > > > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > > > > + nct6694->timeout); > > > > + if (ret) > > > > + goto err; > > > > + > > > > + /* Receive response packet from USB device */ > > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > > > > + nct6694->timeout); > > > > + if (ret) > > > > + goto err; > > > > + > > > > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > > > > + > > > > + /* > > > > + * Segmented reception of messages that exceed the size of USB bulk > > > > + * pipe packets. > > > > + */ > > > > > > The Linux USB stack can receive bulk messages longer than the max packet size. > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device. > > The core will divide packet 256 bytes for high speed USB device, but > > it is exceeds > > the hardware limitation, so I am dividing it manually. > > You say the endpoint descriptor is correctly reporting it's max packet > size of 128, but the Linux USB will send packets of 256 bytes? [Ming] The endpoint descriptor is correctly reporting it's max packet size of 256, but the Linux USB may send more than 256 (max is 512) https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446 > > > > > > > > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > > > + if (len > nct6694->maxp) > > > > + packet_len = nct6694->maxp; > > > > + else > > > > + packet_len = len; > > > > + > > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > > + nct6694->rx_buffer + nct6694->maxp * i, > > > > + packet_len, &rx_len, nct6694->timeout); > > > > + if (ret) > > > > + goto err; > > > > + } > > > > + > > > > + for (i = 0; i < rd_len; i++) > > > > + buf[i] = nct6694->rx_buffer[i + rd_idx]; > > > > > > memcpy()? > > > > > > Or why don't you directly receive data into the provided buffer? Copying > > > of the data doesn't make it faster. > > > > > > On the other hand, receiving directly into the target buffer means the > > > target buffer must not live on the stack. > > > > [Ming] Okay! I'll change it to memcpy(). > > fine! > > > This is my perspective: the data is uniformly received by the rx_bffer held > > by the MFD device. does it need to be changed? > > My question is: Why do you first receive into the nct6694->rx_buffer and > then memcpy() to the buffer provided by the caller, why don't you > directly receive into the memory provided by the caller? [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently using the MFD'd dynamically allocated buffer to submit URBs will better manage USB-related operations > > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |
On 25.10.2024 19:03:55, Ming Yu wrote: > Oh! I'm sorry about that I confused the packet size. > The NCT6694 bulk maximum packet size is 256 bytes, > and USB High speed bulk maximum packet size is 512 bytes. > > Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月25日 週五 下午6:08寫道： > > > > On 25.10.2024 16:08:10, Ming Yu wrote: > > > > > +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, > > > > > + u8 rd_idx, u8 rd_len, unsigned char *buf) > > > > > > > > why not make buf a void *? > > > > > > [Ming] I'll change the type in the next patch. > > > > > > > > > > > > +{ > > > > > + struct usb_device *udev = nct6694->udev; > > > > > + unsigned char err_status; > > > > > + int len, packet_len, tx_len, rx_len; > > > > > + int i, ret; > > > > > + > > > > > + mutex_lock(&nct6694->access_lock); > > > > > + > > > > > + /* Send command packet to USB device */ > > > > > + nct6694->cmd_buffer[REQUEST_MOD_IDX] = mod; > > > > > + nct6694->cmd_buffer[REQUEST_CMD_IDX] = offset & 0xFF; > > > > > + nct6694->cmd_buffer[REQUEST_SEL_IDX] = (offset >> 8) & 0xFF; > > > > > + nct6694->cmd_buffer[REQUEST_HCTRL_IDX] = HCTRL_GET; > > > > > + nct6694->cmd_buffer[REQUEST_LEN_L_IDX] = length & 0xFF; > > > > > + nct6694->cmd_buffer[REQUEST_LEN_H_IDX] = (length >> 8) & 0xFF; > > > > > + > > > > > + ret = usb_bulk_msg(udev, usb_sndbulkpipe(udev, BULK_OUT_ENDPOINT), > > > > > + nct6694->cmd_buffer, CMD_PACKET_SZ, &tx_len, > > > > > + nct6694->timeout); > > > > > + if (ret) > > > > > + goto err; > > > > > + > > > > > + /* Receive response packet from USB device */ > > > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > > > + nct6694->rx_buffer, CMD_PACKET_SZ, &rx_len, > > > > > + nct6694->timeout); > > > > > + if (ret) > > > > > + goto err; > > > > > + > > > > > + err_status = nct6694->rx_buffer[RESPONSE_STS_IDX]; > > > > > + > > > > > + /* > > > > > + * Segmented reception of messages that exceed the size of USB bulk > > > > > + * pipe packets. > > > > > + */ > > > > > > > > The Linux USB stack can receive bulk messages longer than the max packet size. > > > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device. > > > The core will divide packet 256 bytes for high speed USB device, but > > > it is exceeds > > > the hardware limitation, so I am dividing it manually. > > > > You say the endpoint descriptor is correctly reporting it's max packet > > size of 128, but the Linux USB will send packets of 256 bytes? > > [Ming] The endpoint descriptor is correctly reporting it's max packet > size of 256, but the Linux USB may send more than 256 (max is 512) > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446 AFAIK according to the USB-2.0 spec the maximum packet size for high-speed bulk transfers is fixed set to 512 bytes. Does this mean that your device is a non-compliant USB device? > > > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > > > > + if (len > nct6694->maxp) > > > > > + packet_len = nct6694->maxp; > > > > > + else > > > > > + packet_len = len; > > > > > + > > > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > > > + nct6694->rx_buffer + nct6694->maxp * i, > > > > > + packet_len, &rx_len, nct6694->timeout); > > > > > + if (ret) > > > > > + goto err; > > > > > + } > > > > > + > > > > > + for (i = 0; i < rd_len; i++) > > > > > + buf[i] = nct6694->rx_buffer[i + rd_idx]; > > > > > > > > memcpy()? > > > > > > > > Or why don't you directly receive data into the provided buffer? Copying > > > > of the data doesn't make it faster. > > > > > > > > On the other hand, receiving directly into the target buffer means the > > > > target buffer must not live on the stack. > > > > > > [Ming] Okay! I'll change it to memcpy(). > > > > fine! > > > > > This is my perspective: the data is uniformly received by the rx_bffer held > > > by the MFD device. does it need to be changed? > > > > My question is: Why do you first receive into the nct6694->rx_buffer and > > then memcpy() to the buffer provided by the caller, why don't you > > directly receive into the memory provided by the caller? > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently > using the MFD'd dynamically allocated buffer to submit URBs will better > manage USB-related operations The non-compliant max packet size limitation is unrelated to the question which RX or TX buffer to use. regards, Marc
Hello, 2024年10月24日(木) 18:04 Ming Yu <a0282524688@gmail.com>: > > This driver supports RTC functionality for NCT6694 MFD device > based on USB interface. > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > --- > MAINTAINERS | 1 + > drivers/rtc/Kconfig | 10 ++ > drivers/rtc/Makefile | 1 + > drivers/rtc/rtc-nct6694.c | 276 ++++++++++++++++++++++++++++++++++++++ > 4 files changed, 288 insertions(+) > create mode 100644 drivers/rtc/rtc-nct6694.c > > diff --git a/MAINTAINERS b/MAINTAINERS > index 4d5a5eded3b9..8de90bda8b5e 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -16445,6 +16445,7 @@ F: drivers/i2c/busses/i2c-nct6694.c > F: drivers/mfd/nct6694.c > F: drivers/net/can/nct6694_canfd.c > F: drivers/pwm/pwm-nct6694.c > +F: drivers/rtc/rtc-nct6694.c > F: drivers/watchdog/nct6694_wdt.c > F: include/linux/mfd/nct6694.h > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > index 66eb1122248b..240c496d95f7 100644 > --- a/drivers/rtc/Kconfig > +++ b/drivers/rtc/Kconfig > @@ -406,6 +406,16 @@ config RTC_DRV_NCT3018Y > This driver can also be built as a module, if so, the module will be > called "rtc-nct3018y". > > +config RTC_DRV_NCT6694 > + tristate "Nuvoton NCT6694 RTC support" > + depends on MFD_NCT6694 > + help > + If you say yes to this option, support will be included for Nuvoton > + NCT6694, a USB device to RTC. > + > + This driver can also be built as a module. If so, the module > + will be called rtc-nct6694. > + > config RTC_DRV_RK808 > tristate "Rockchip RK805/RK808/RK809/RK817/RK818 RTC" > depends on MFD_RK8XX > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > index f62340ecc534..64443d26bb5b 100644 > --- a/drivers/rtc/Makefile > +++ b/drivers/rtc/Makefile > @@ -116,6 +116,7 @@ obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o > obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o > obj-$(CONFIG_RTC_DRV_GAMECUBE) += rtc-gamecube.o > obj-$(CONFIG_RTC_DRV_NCT3018Y) += rtc-nct3018y.o > +obj-$(CONFIG_RTC_DRV_NCT6694) += rtc-nct6694.o > obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o > obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o > obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o > diff --git a/drivers/rtc/rtc-nct6694.c b/drivers/rtc/rtc-nct6694.c > new file mode 100644 > index 000000000000..622bb9fbe6f6 > --- /dev/null > +++ b/drivers/rtc/rtc-nct6694.c > @@ -0,0 +1,276 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Nuvoton NCT6694 RTC driver based on USB interface. > + * > + * Copyright (C) 2024 Nuvoton Technology Corp. > + */ > + > +#include <linux/slab.h> > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/rtc.h> > +#include <linux/bcd.h> > +#include <linux/platform_device.h> > +#include <linux/mfd/nct6694.h> Please sort header files alphabetically. > + > +#define DRVNAME "nct6694-rtc" > + > +/* Host interface */ > +#define REQUEST_RTC_MOD 0x08 > + > +/* Message Channel */ > +/* Command 00h */ > +#define REQUEST_RTC_CMD0_LEN 0x07 > +#define REQUEST_RTC_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > +#define RTC_SEC_IDX 0x00 > +#define RTC_MIN_IDX 0x01 > +#define RTC_HOUR_IDX 0x02 > +#define RTC_WEEK_IDX 0x03 > +#define RTC_DAY_IDX 0x04 > +#define RTC_MONTH_IDX 0x05 > +#define RTC_YEAR_IDX 0x06 > +/* Command 01h */ > +#define REQUEST_RTC_CMD1_LEN 0x05 > +#define REQUEST_RTC_CMD1_OFFSET 0x0001 /* OFFSET = SEL|CMD */ > +#define RTC_ALRM_EN_IDX 0x03 > +#define RTC_ALRM_PEND_IDX 0x04 > +/* Command 02h */ > +#define REQUEST_RTC_CMD2_LEN 0x02 > +#define REQUEST_RTC_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > +#define RTC_IRQ_EN_IDX 0x00 > +#define RTC_IRQ_PEND_IDX 0x01 > + > +#define RTC_IRQ_EN (BIT(0) | BIT(5)) RTC_IRQ_INT_EN | RTC_IRQ_GPO_EN ? > +#define RTC_IRQ_INT_EN BIT(0) /* Transmit a USB INT-in when RTC alarm */ > +#define RTC_IRQ_GPO_EN BIT(5) /* Trigger a GPO Low Pulse when RTC alarm */ > +#define RTC_IRQ_STS BIT(0) /* Write 1 clear IRQ status */ > + > +struct nct6694_rtc_data { > + struct nct6694 *nct6694; > + struct rtc_device *rtc; > + struct work_struct alarm_work; > +}; > + > +static int nct6694_rtc_read_time(struct device *dev, struct rtc_time *tm) > +{ > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > + unsigned char buf[REQUEST_RTC_CMD0_LEN]; > + int ret; > + > + ret = nct6694_read_msg(data->nct6694, REQUEST_RTC_MOD, > + REQUEST_RTC_CMD0_OFFSET, REQUEST_RTC_CMD0_LEN, > + 0, REQUEST_RTC_CMD0_LEN, buf); > + if (ret) { > + pr_err("%s: Failed to get rtc device!\n", __func__); > + return -EIO; > + } > + > + tm->tm_sec = bcd2bin(buf[RTC_SEC_IDX]); /* tm_sec expect 0 ~ 59 */ > + tm->tm_min = bcd2bin(buf[RTC_MIN_IDX]); /* tm_min expect 0 ~ 59 */ > + tm->tm_hour = bcd2bin(buf[RTC_HOUR_IDX]); /* tm_hour expect 0 ~ 23 */ > + tm->tm_wday = bcd2bin(buf[RTC_WEEK_IDX]) - 1; /* tm_wday expect 0 ~ 6 */ > + tm->tm_mday = bcd2bin(buf[RTC_DAY_IDX]); /* tm_mday expect 1 ~ 31 */ > + tm->tm_mon = bcd2bin(buf[RTC_MONTH_IDX]) - 1; /* tm_month expect 0 ~ 11 */ > + tm->tm_year = bcd2bin(buf[RTC_YEAR_IDX]) + 100; /* tm_year expect since 1900 */ > + > + return ret; > +} > + > +static int nct6694_rtc_set_time(struct device *dev, struct rtc_time *tm) > +{ > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > + unsigned char buf[REQUEST_RTC_CMD0_LEN]; > + int ret; > + > + buf[RTC_SEC_IDX] = bin2bcd(tm->tm_sec); > + buf[RTC_MIN_IDX] = bin2bcd(tm->tm_min); > + buf[RTC_HOUR_IDX] = bin2bcd(tm->tm_hour); > + buf[RTC_WEEK_IDX] = bin2bcd(tm->tm_wday + 1); > + buf[RTC_DAY_IDX] = bin2bcd(tm->tm_mday); > + buf[RTC_MONTH_IDX] = bin2bcd(tm->tm_mon + 1); > + buf[RTC_YEAR_IDX] = bin2bcd(tm->tm_year - 100); > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > + REQUEST_RTC_CMD0_OFFSET, REQUEST_RTC_CMD0_LEN, > + buf); > + if (ret) { > + pr_err("%s: Failed to set rtc device!\n", __func__); > + return -EIO; Why do you return -EIO? Please do not overwrite error codes. > + } > + > + return ret; > +} > + > +static int nct6694_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > + unsigned char buf[REQUEST_RTC_CMD1_LEN]; > + int ret; > + > + ret = nct6694_read_msg(data->nct6694, REQUEST_RTC_MOD, > + REQUEST_RTC_CMD1_OFFSET, REQUEST_RTC_CMD1_LEN, > + 0, REQUEST_RTC_CMD1_LEN, buf); > + if (ret) { > + pr_err("%s: Failed to get rtc device!\n", __func__); > + return -EIO; same as above. > + } > + > + alrm->time.tm_sec = bcd2bin(buf[RTC_SEC_IDX]); > + alrm->time.tm_min = bcd2bin(buf[RTC_MIN_IDX]); > + alrm->time.tm_hour = bcd2bin(buf[RTC_HOUR_IDX]); > + > + alrm->enabled = buf[RTC_ALRM_EN_IDX]; > + alrm->pending = buf[RTC_ALRM_PEND_IDX]; > + > + return ret; > +} > + > +static int nct6694_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > +{ > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > + unsigned char buf[REQUEST_RTC_CMD1_LEN]; > + int ret; > + > + buf[RTC_SEC_IDX] = bin2bcd(alrm->time.tm_sec); > + buf[RTC_MIN_IDX] = bin2bcd(alrm->time.tm_min); > + buf[RTC_HOUR_IDX] = bin2bcd(alrm->time.tm_hour); > + buf[RTC_ALRM_EN_IDX] = alrm->enabled ? RTC_IRQ_EN : 0; > + buf[RTC_ALRM_PEND_IDX] = 0; > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > + REQUEST_RTC_CMD1_OFFSET, REQUEST_RTC_CMD1_LEN, > + buf); > + if (ret) { > + pr_err("%s: Failed to set rtc device!\n", __func__); > + return -EIO; same as above. > + } > + > + return ret; > +} > + > +static int nct6694_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > +{ > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > + unsigned char buf[REQUEST_RTC_CMD2_LEN] = {0}; > + int ret; > + > + if (enabled) > + buf[RTC_IRQ_EN_IDX] |= RTC_IRQ_EN; > + else > + buf[RTC_IRQ_EN_IDX] &= ~RTC_IRQ_EN; > + > + ret = nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > + REQUEST_RTC_CMD2_OFFSET, REQUEST_RTC_CMD2_LEN, > + buf); > + if (ret) { > + pr_err("%s: Failed to set rtc device!\n", __func__); > + return -EIO; same as above. > + } > + > + return ret; > +} > + > +static const struct rtc_class_ops nct6694_rtc_ops = { > + .read_time = nct6694_rtc_read_time, > + .set_time = nct6694_rtc_set_time, > + .read_alarm = nct6694_rtc_read_alarm, > + .set_alarm = nct6694_rtc_set_alarm, > + .alarm_irq_enable = nct6694_rtc_alarm_irq_enable, > +}; > + > +static void nct6694_rtc_alarm(struct work_struct *work) > +{ > + struct nct6694_rtc_data *data; > + unsigned char buf[REQUEST_RTC_CMD2_LEN] = {0}; > + > + data = container_of(work, struct nct6694_rtc_data, alarm_work); > + > + pr_info("%s: Got RTC alarm!\n", __func__); > + buf[RTC_IRQ_EN_IDX] = RTC_IRQ_EN; > + buf[RTC_IRQ_PEND_IDX] = RTC_IRQ_STS; > + nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > + REQUEST_RTC_CMD2_OFFSET, > + REQUEST_RTC_CMD2_LEN, buf); > +} > + > +static void nct6694_rtc_handler(void *private_data) > +{ > + struct nct6694_rtc_data *data = private_data; > + struct nct6694 *nct6694 = data->nct6694; > + > + queue_work(nct6694->async_workqueue, &data->alarm_work); > +} > + > +static int nct6694_rtc_probe(struct platform_device *pdev) > +{ > + struct nct6694_rtc_data *data; > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > + int ret; > + > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > + if (!data) > + return -ENOMEM; > + > + data->rtc = devm_rtc_allocate_device(&pdev->dev); > + if (IS_ERR(data->rtc)) > + return PTR_ERR(data->rtc); Please use dev_err_probe. > + > + data->nct6694 = nct6694; > + data->rtc->ops = &nct6694_rtc_ops; > + data->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000; > + data->rtc->range_max = RTC_TIMESTAMP_END_2099; > + > + INIT_WORK(&data->alarm_work, nct6694_rtc_alarm); > + > + ret = nct6694_register_handler(nct6694, RTC_IRQ_STATUS, > + nct6694_rtc_handler, data); > + if (ret) { > + dev_err(&pdev->dev, "%s: Failed to register handler: %pe\n", > + __func__, ERR_PTR(ret)); Please use dev_err_probe. > + return ret; > + } > + > + device_set_wakeup_capable(&pdev->dev, 1); > + > + platform_set_drvdata(pdev, data); > + > + /* Register rtc device to RTC framework */ > + ret = devm_rtc_register_device(data->rtc); > + if (ret) { > + dev_err(&pdev->dev, "Failed to register rtc device!\n"); > + return ret; > + } You can simplify return devm_rtc_register_device. > + > + return 0; > +} > + > +static struct platform_driver nct6694_rtc_driver = { > + .driver = { > + .name = DRVNAME, > + }, > + .probe = nct6694_rtc_probe, > +}; > + > +static int __init nct6694_init(void) > +{ > + int err; > + > + err = platform_driver_register(&nct6694_rtc_driver); > + if (!err) { > + if (err) This looks strange. You can simplify return platform_driver_register. > + platform_driver_unregister(&nct6694_rtc_driver); > + } > + > + return err; > +} > +subsys_initcall(nct6694_init); > + > +static void __exit nct6694_exit(void) > +{ > + platform_driver_unregister(&nct6694_rtc_driver); > +} > +module_exit(nct6694_exit); > + > +MODULE_DESCRIPTION("USB-RTC driver for NCT6694"); > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > +MODULE_LICENSE("GPL"); > -- > 2.34.1 > > Best regards, Nobuhiro
Dear Nobuhiro, Thank you for your comments, Nobuhiro Iwamatsu <iwamatsu@nigauri.org> 於 2024年10月26日 週六 上午7:35寫道： > > Hello, > > 2024年10月24日(木) 18:04 Ming Yu <a0282524688@gmail.com>: > > > > This driver supports RTC functionality for NCT6694 MFD device > > based on USB interface. > > > > Signed-off-by: Ming Yu <tmyu0@nuvoton.com> > > --- > > MAINTAINERS | 1 + > > drivers/rtc/Kconfig | 10 ++ > > drivers/rtc/Makefile | 1 + > > drivers/rtc/rtc-nct6694.c | 276 ++++++++++++++++++++++++++++++++++++++ > > 4 files changed, 288 insertions(+) > > create mode 100644 drivers/rtc/rtc-nct6694.c > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > index 4d5a5eded3b9..8de90bda8b5e 100644 > > --- a/MAINTAINERS > > +++ b/MAINTAINERS > > @@ -16445,6 +16445,7 @@ F: drivers/i2c/busses/i2c-nct6694.c > > F: drivers/mfd/nct6694.c > > F: drivers/net/can/nct6694_canfd.c > > F: drivers/pwm/pwm-nct6694.c > > +F: drivers/rtc/rtc-nct6694.c > > F: drivers/watchdog/nct6694_wdt.c > > F: include/linux/mfd/nct6694.h > > > > diff --git a/drivers/rtc/Kconfig b/drivers/rtc/Kconfig > > index 66eb1122248b..240c496d95f7 100644 > > --- a/drivers/rtc/Kconfig > > +++ b/drivers/rtc/Kconfig > > @@ -406,6 +406,16 @@ config RTC_DRV_NCT3018Y > > This driver can also be built as a module, if so, the module will be > > called "rtc-nct3018y". > > > > +config RTC_DRV_NCT6694 > > + tristate "Nuvoton NCT6694 RTC support" > > + depends on MFD_NCT6694 > > + help > > + If you say yes to this option, support will be included for Nuvoton > > + NCT6694, a USB device to RTC. > > + > > + This driver can also be built as a module. If so, the module > > + will be called rtc-nct6694. > > + > > config RTC_DRV_RK808 > > tristate "Rockchip RK805/RK808/RK809/RK817/RK818 RTC" > > depends on MFD_RK8XX > > diff --git a/drivers/rtc/Makefile b/drivers/rtc/Makefile > > index f62340ecc534..64443d26bb5b 100644 > > --- a/drivers/rtc/Makefile > > +++ b/drivers/rtc/Makefile > > @@ -116,6 +116,7 @@ obj-$(CONFIG_RTC_DRV_MXC) += rtc-mxc.o > > obj-$(CONFIG_RTC_DRV_MXC_V2) += rtc-mxc_v2.o > > obj-$(CONFIG_RTC_DRV_GAMECUBE) += rtc-gamecube.o > > obj-$(CONFIG_RTC_DRV_NCT3018Y) += rtc-nct3018y.o > > +obj-$(CONFIG_RTC_DRV_NCT6694) += rtc-nct6694.o > > obj-$(CONFIG_RTC_DRV_NTXEC) += rtc-ntxec.o > > obj-$(CONFIG_RTC_DRV_OMAP) += rtc-omap.o > > obj-$(CONFIG_RTC_DRV_OPAL) += rtc-opal.o > > diff --git a/drivers/rtc/rtc-nct6694.c b/drivers/rtc/rtc-nct6694.c > > new file mode 100644 > > index 000000000000..622bb9fbe6f6 > > --- /dev/null > > +++ b/drivers/rtc/rtc-nct6694.c > > @@ -0,0 +1,276 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Nuvoton NCT6694 RTC driver based on USB interface. > > + * > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > + */ > > + > > +#include <linux/slab.h> > > +#include <linux/kernel.h> > > +#include <linux/module.h> > > +#include <linux/rtc.h> > > +#include <linux/bcd.h> > > +#include <linux/platform_device.h> > > +#include <linux/mfd/nct6694.h> > > Please sort header files alphabetically. [Ming] Okay! I will sort these headers in the next patch. > > > + > > +#define DRVNAME "nct6694-rtc" > > + > > +/* Host interface */ > > +#define REQUEST_RTC_MOD 0x08 > > + > > +/* Message Channel */ > > +/* Command 00h */ > > +#define REQUEST_RTC_CMD0_LEN 0x07 > > +#define REQUEST_RTC_CMD0_OFFSET 0x0000 /* OFFSET = SEL|CMD */ > > +#define RTC_SEC_IDX 0x00 > > +#define RTC_MIN_IDX 0x01 > > +#define RTC_HOUR_IDX 0x02 > > +#define RTC_WEEK_IDX 0x03 > > +#define RTC_DAY_IDX 0x04 > > +#define RTC_MONTH_IDX 0x05 > > +#define RTC_YEAR_IDX 0x06 > > +/* Command 01h */ > > +#define REQUEST_RTC_CMD1_LEN 0x05 > > +#define REQUEST_RTC_CMD1_OFFSET 0x0001 /* OFFSET = SEL|CMD */ > > +#define RTC_ALRM_EN_IDX 0x03 > > +#define RTC_ALRM_PEND_IDX 0x04 > > +/* Command 02h */ > > +#define REQUEST_RTC_CMD2_LEN 0x02 > > +#define REQUEST_RTC_CMD2_OFFSET 0x0002 /* OFFSET = SEL|CMD */ > > +#define RTC_IRQ_EN_IDX 0x00 > > +#define RTC_IRQ_PEND_IDX 0x01 > > + > > +#define RTC_IRQ_EN (BIT(0) | BIT(5)) > > RTC_IRQ_INT_EN | RTC_IRQ_GPO_EN ? [Ming] Yes, it is used to enable both of them. > > > +#define RTC_IRQ_INT_EN BIT(0) /* Transmit a USB INT-in when RTC alarm */ > > +#define RTC_IRQ_GPO_EN BIT(5) /* Trigger a GPO Low Pulse when RTC alarm */ > > +#define RTC_IRQ_STS BIT(0) /* Write 1 clear IRQ status */ > > + > > +struct nct6694_rtc_data { > > + struct nct6694 *nct6694; > > + struct rtc_device *rtc; > > + struct work_struct alarm_work; > > +}; > > + > > +static int nct6694_rtc_read_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > > + unsigned char buf[REQUEST_RTC_CMD0_LEN]; > > + int ret; > > + > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RTC_MOD, > > + REQUEST_RTC_CMD0_OFFSET, REQUEST_RTC_CMD0_LEN, > > + 0, REQUEST_RTC_CMD0_LEN, buf); > > + if (ret) { > > + pr_err("%s: Failed to get rtc device!\n", __func__); > > + return -EIO; > > + } > > + > > + tm->tm_sec = bcd2bin(buf[RTC_SEC_IDX]); /* tm_sec expect 0 ~ 59 */ > > + tm->tm_min = bcd2bin(buf[RTC_MIN_IDX]); /* tm_min expect 0 ~ 59 */ > > + tm->tm_hour = bcd2bin(buf[RTC_HOUR_IDX]); /* tm_hour expect 0 ~ 23 */ > > + tm->tm_wday = bcd2bin(buf[RTC_WEEK_IDX]) - 1; /* tm_wday expect 0 ~ 6 */ > > + tm->tm_mday = bcd2bin(buf[RTC_DAY_IDX]); /* tm_mday expect 1 ~ 31 */ > > + tm->tm_mon = bcd2bin(buf[RTC_MONTH_IDX]) - 1; /* tm_month expect 0 ~ 11 */ > > + tm->tm_year = bcd2bin(buf[RTC_YEAR_IDX]) + 100; /* tm_year expect since 1900 */ > > + > > + return ret; > > +} > > + > > +static int nct6694_rtc_set_time(struct device *dev, struct rtc_time *tm) > > +{ > > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > > + unsigned char buf[REQUEST_RTC_CMD0_LEN]; > > + int ret; > > + > > + buf[RTC_SEC_IDX] = bin2bcd(tm->tm_sec); > > + buf[RTC_MIN_IDX] = bin2bcd(tm->tm_min); > > + buf[RTC_HOUR_IDX] = bin2bcd(tm->tm_hour); > > + buf[RTC_WEEK_IDX] = bin2bcd(tm->tm_wday + 1); > > + buf[RTC_DAY_IDX] = bin2bcd(tm->tm_mday); > > + buf[RTC_MONTH_IDX] = bin2bcd(tm->tm_mon + 1); > > + buf[RTC_YEAR_IDX] = bin2bcd(tm->tm_year - 100); > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > > + REQUEST_RTC_CMD0_OFFSET, REQUEST_RTC_CMD0_LEN, > > + buf); > > + if (ret) { > > + pr_err("%s: Failed to set rtc device!\n", __func__); > > + return -EIO; > > Why do you return -EIO? Please do not overwrite error codes. [Ming] Understood, thank you. > > > + } > > + > > + return ret; > > +} > > + > > +static int nct6694_rtc_read_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > +{ > > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > > + unsigned char buf[REQUEST_RTC_CMD1_LEN]; > > + int ret; > > + > > + ret = nct6694_read_msg(data->nct6694, REQUEST_RTC_MOD, > > + REQUEST_RTC_CMD1_OFFSET, REQUEST_RTC_CMD1_LEN, > > + 0, REQUEST_RTC_CMD1_LEN, buf); > > + if (ret) { > > + pr_err("%s: Failed to get rtc device!\n", __func__); > > + return -EIO; > > same as above. > > > + } > > + > > + alrm->time.tm_sec = bcd2bin(buf[RTC_SEC_IDX]); > > + alrm->time.tm_min = bcd2bin(buf[RTC_MIN_IDX]); > > + alrm->time.tm_hour = bcd2bin(buf[RTC_HOUR_IDX]); > > + > > + alrm->enabled = buf[RTC_ALRM_EN_IDX]; > > + alrm->pending = buf[RTC_ALRM_PEND_IDX]; > > + > > + return ret; > > +} > > + > > +static int nct6694_rtc_set_alarm(struct device *dev, struct rtc_wkalrm *alrm) > > +{ > > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > > + unsigned char buf[REQUEST_RTC_CMD1_LEN]; > > + int ret; > > + > > + buf[RTC_SEC_IDX] = bin2bcd(alrm->time.tm_sec); > > + buf[RTC_MIN_IDX] = bin2bcd(alrm->time.tm_min); > > + buf[RTC_HOUR_IDX] = bin2bcd(alrm->time.tm_hour); > > + buf[RTC_ALRM_EN_IDX] = alrm->enabled ? RTC_IRQ_EN : 0; > > + buf[RTC_ALRM_PEND_IDX] = 0; > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > > + REQUEST_RTC_CMD1_OFFSET, REQUEST_RTC_CMD1_LEN, > > + buf); > > + if (ret) { > > + pr_err("%s: Failed to set rtc device!\n", __func__); > > + return -EIO; > > same as above. > > > + } > > + > > + return ret; > > +} > > + > > +static int nct6694_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) > > +{ > > + struct nct6694_rtc_data *data = dev_get_drvdata(dev); > > + unsigned char buf[REQUEST_RTC_CMD2_LEN] = {0}; > > + int ret; > > + > > + if (enabled) > > + buf[RTC_IRQ_EN_IDX] |= RTC_IRQ_EN; > > + else > > + buf[RTC_IRQ_EN_IDX] &= ~RTC_IRQ_EN; > > + > > + ret = nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > > + REQUEST_RTC_CMD2_OFFSET, REQUEST_RTC_CMD2_LEN, > > + buf); > > + if (ret) { > > + pr_err("%s: Failed to set rtc device!\n", __func__); > > + return -EIO; > > same as above. > > > + } > > + > > + return ret; > > +} > > + > > +static const struct rtc_class_ops nct6694_rtc_ops = { > > + .read_time = nct6694_rtc_read_time, > > + .set_time = nct6694_rtc_set_time, > > + .read_alarm = nct6694_rtc_read_alarm, > > + .set_alarm = nct6694_rtc_set_alarm, > > + .alarm_irq_enable = nct6694_rtc_alarm_irq_enable, > > +}; > > + > > +static void nct6694_rtc_alarm(struct work_struct *work) > > +{ > > + struct nct6694_rtc_data *data; > > + unsigned char buf[REQUEST_RTC_CMD2_LEN] = {0}; > > + > > + data = container_of(work, struct nct6694_rtc_data, alarm_work); > > + > > + pr_info("%s: Got RTC alarm!\n", __func__); > > + buf[RTC_IRQ_EN_IDX] = RTC_IRQ_EN; > > + buf[RTC_IRQ_PEND_IDX] = RTC_IRQ_STS; > > + nct6694_write_msg(data->nct6694, REQUEST_RTC_MOD, > > + REQUEST_RTC_CMD2_OFFSET, > > + REQUEST_RTC_CMD2_LEN, buf); > > +} > > + > > +static void nct6694_rtc_handler(void *private_data) > > +{ > > + struct nct6694_rtc_data *data = private_data; > > + struct nct6694 *nct6694 = data->nct6694; > > + > > + queue_work(nct6694->async_workqueue, &data->alarm_work); > > +} > > + > > +static int nct6694_rtc_probe(struct platform_device *pdev) > > +{ > > + struct nct6694_rtc_data *data; > > + struct nct6694 *nct6694 = dev_get_drvdata(pdev->dev.parent); > > + int ret; > > + > > + data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + data->rtc = devm_rtc_allocate_device(&pdev->dev); > > + if (IS_ERR(data->rtc)) > > + return PTR_ERR(data->rtc); > > Please use dev_err_probe. [Ming] Okay! I will change it in the next patch. > > > + > > + data->nct6694 = nct6694; > > + data->rtc->ops = &nct6694_rtc_ops; > > + data->rtc->range_min = RTC_TIMESTAMP_BEGIN_2000; > > + data->rtc->range_max = RTC_TIMESTAMP_END_2099; > > + > > + INIT_WORK(&data->alarm_work, nct6694_rtc_alarm); > > + > > + ret = nct6694_register_handler(nct6694, RTC_IRQ_STATUS, > > + nct6694_rtc_handler, data); > > + if (ret) { > > + dev_err(&pdev->dev, "%s: Failed to register handler: %pe\n", > > + __func__, ERR_PTR(ret)); > > Please use dev_err_probe. [Ming] Okay! I will change it in the next patch. > > > + return ret; > > + } > > + > > + device_set_wakeup_capable(&pdev->dev, 1); > > + > > + platform_set_drvdata(pdev, data); > > + > > + /* Register rtc device to RTC framework */ > > + ret = devm_rtc_register_device(data->rtc); > > + if (ret) { > > + dev_err(&pdev->dev, "Failed to register rtc device!\n"); > > + return ret; > > + } > > You can simplify return devm_rtc_register_device. [Ming] Okay! I will change it in the next patch. > > > + > > + return 0; > > +} > > + > > +static struct platform_driver nct6694_rtc_driver = { > > + .driver = { > > + .name = DRVNAME, > > + }, > > + .probe = nct6694_rtc_probe, > > +}; > > + > > +static int __init nct6694_init(void) > > +{ > > + int err; > > + > > + err = platform_driver_register(&nct6694_rtc_driver); > > + if (!err) { > > + if (err) > > This looks strange. You can simplify return platform_driver_register. [Ming] For platform driver registration, I'll change it to module_platform_driver() in the next patch. > > > + platform_driver_unregister(&nct6694_rtc_driver); > > + } > > + > > + return err; > > +} > > +subsys_initcall(nct6694_init); > > + > > +static void __exit nct6694_exit(void) > > +{ > > + platform_driver_unregister(&nct6694_rtc_driver); > > +} > > +module_exit(nct6694_exit); > > + > > +MODULE_DESCRIPTION("USB-RTC driver for NCT6694"); > > +MODULE_AUTHOR("Ming Yu <tmyu0@nuvoton.com>"); > > +MODULE_LICENSE("GPL"); > > -- > > 2.34.1 > > > > > > Best regards, > Nobuhiro > > -- > Nobuhiro Iwamatsu > iwamatsu at {nigauri.org / debian.org / kernel.org} > GPG ID: 32247FBB40AD1FA6 Best regards, Ming
Marc Kleine-Budde <mkl@pengutronix.de> 於 2024年10月28日 週一 下午10:06寫道： > > On 28.10.2024 16:31:25, Ming Yu wrote: > > > > > > > > > The Linux USB stack can receive bulk messages longer than the max packet size. > > > > > > > > > > > > > > > > [Ming] Since NCT6694's bulk pipe endpoint size is 128 bytes for this MFD device. > > > > > > > > The core will divide packet 256 bytes for high speed USB device, but > > > > > > > > it is exceeds > > > > > > > > the hardware limitation, so I am dividing it manually. > > > > > > > > > > > > > > You say the endpoint descriptor is correctly reporting it's max packet > > > > > > > size of 128, but the Linux USB will send packets of 256 bytes? > > > > > > > > > > > > [Ming] The endpoint descriptor is correctly reporting it's max packet > > > > > > size of 256, but the Linux USB may send more than 256 (max is 512) > > > > > > https://elixir.bootlin.com/linux/v6.11.5/source/drivers/usb/host/xhci-mem.c#L1446 > > > > > > > > > > AFAIK according to the USB-2.0 spec the maximum packet size for > > > > > high-speed bulk transfers is fixed set to 512 bytes. Does this mean that > > > > > your device is a non-compliant USB device? > > > > > > > > We will reduce the endpoint size of other interfaces to ensure that MFD device > > > > meets the USB2.0 spec. In other words, I will remove the code for manual > > > > unpacking in the next patch. > > > > > > I was not talking about the driver, but your USB device. According to > > > the USB2.0 spec, the packet size is fixed to 512 for high-speed bulk > > > transfers. So your device must be able to handle 512 byte transfers or > > > it's a non-compliant USB device. > > > > I understand. Therefore, the USB device's firmware will be modified to support > > bulk pipe size of 512 bytes to comply with the USB 2.0 spec. > > Then you don't need manual segmentation of bulk transfers anymore! Understood, thank you very much. > > > > > > > > > > > + for (i = 0, len = length; len > 0; i++, len -= packet_len) { > > > > > > > > > > + if (len > nct6694->maxp) > > > > > > > > > > + packet_len = nct6694->maxp; > > > > > > > > > > + else > > > > > > > > > > + packet_len = len; > > > > > > > > > > + > > > > > > > > > > + ret = usb_bulk_msg(udev, usb_rcvbulkpipe(udev, BULK_IN_ENDPOINT), > > > > > > > > > > + nct6694->rx_buffer + nct6694->maxp * i, > > > > > > > > > > + packet_len, &rx_len, nct6694->timeout); > > > > > > > > > > + if (ret) > > > > > > > > > > + goto err; > > > > > > > > > > + } > > > > > > > > > > + > > > > > > > > > > + for (i = 0; i < rd_len; i++) > > > > > > > > > > + buf[i] = nct6694->rx_buffer[i + rd_idx]; > > > > > > > > > > > > > > > > > > memcpy()? > > > > > > > > > > > > > > > > > > Or why don't you directly receive data into the provided buffer? Copying > > > > > > > > > of the data doesn't make it faster. > > > > > > > > > > > > > > > > > > On the other hand, receiving directly into the target buffer means the > > > > > > > > > target buffer must not live on the stack. > > > > > > > > > > > > > > > > [Ming] Okay! I'll change it to memcpy(). > > > > > > > > > > > > > > fine! > > > > > > > > > > > > > > > This is my perspective: the data is uniformly received by the rx_bffer held > > > > > > > > by the MFD device. does it need to be changed? > > > > > > > > > > > > > > My question is: Why do you first receive into the nct6694->rx_buffer and > > > > > > > then memcpy() to the buffer provided by the caller, why don't you > > > > > > > directly receive into the memory provided by the caller? > > > > > > > > > > > > [Ming] Due to the bulk pipe maximum packet size limitation, I think consistently > > > > > > using the MFD'd dynamically allocated buffer to submit URBs will better > > > > > > manage USB-related operations > > > > > > > > > > The non-compliant max packet size limitation is unrelated to the > > > > > question which RX or TX buffer to use. > > > > > > > > I think these two USB functions can be easily called using the buffer > > > > dynamically > > > > allocated by the MFD. However, if they transfer data directly to the > > > > target buffer, > > > > they must ensure that it is not located on the stack. > > > > > > You have a high coupling between the MFD driver and the individual > > > drivers anyways, so why not directly use the dynamically allocated > > > buffer provided by the caller and get rid of the memcpy()? > > > > Okay! I will provide a function to request and free buffer for child devices, > > and update the caller's variables to use these two functions in the next patch. > > I don't see a need to provide dedicated function to allocate and free > the buffers. The caller can allocate them as part of their private data, > or allocate them during probe(). Okay, so each child device may allocate a buffer like this during probe(): priv->xmit_buf = devm_kcalloc(dev, MAX_PACKET_SZ, sizeof(unsigned char), GFP_KERNEL), right? > > regards, > Marc > > -- > Pengutronix e.K. | Marc Kleine-Budde | > Embedded Linux | https://www.pengutronix.de | > Vertretung Nürnberg | Phone: +49-5121-206917-129 | > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 | Thanks, Ming
On 29.10.2024 11:45:30, Ming Yu wrote: > > > > You have a high coupling between the MFD driver and the individual > > > > drivers anyways, so why not directly use the dynamically allocated > > > > buffer provided by the caller and get rid of the memcpy()? > > > > > > Okay! I will provide a function to request and free buffer for child devices, > > > and update the caller's variables to use these two functions in the next patch. > > > > I don't see a need to provide dedicated function to allocate and free > > the buffers. The caller can allocate them as part of their private data, > > or allocate them during probe(). > > Okay, so each child device may allocate a buffer like this during probe(): > priv->xmit_buf = devm_kcalloc(dev, MAX_PACKET_SZ, sizeof(unsigned char), > GFP_KERNEL), right? basically yes, probably devm_kzalloc() or embed it into the priv struct directly with ____cacheline_aligned: | https://elixir.bootlin.com/linux/v6.11.5/source/drivers/net/can/spi/mcp251xfd/mcp251xfd.h#L498 The size of the driver's RX and TX buffers depend on what they want to send and expect to receive. The next step would be to create structs the describe the RX and TX buffers for each driver. If you have a common header between each driver, create that first. regards, Marc
This patch series introduces support for Nuvoton NCT6694, a peripheral expander based on USB interface. It models the chip as an MFD driver (1/9), GPIO driver(2/9), I2C Adapter driver(3/9), CANfd driver(4/9), WDT driver(5/9), HWMON driver(6/9), IIO driver(7/9), PWM driver(8/9), and RTC driver(9/9). The MFD driver implements USB device functionality to issue custom-define USB bulk pipe packets for NCT6694. Each child device can use the USB functions nct6694_read_msg() and nct6694_write_msg() to issue a command. They can also register a handler function that will be called when the USB device receives its interrupt pipe. The following introduces the custom-define USB transactions: nct6694_read_msg - Send bulk-out pipe to write request packet Receive bulk-in pipe to read response packet Receive bulk-in pipe to read data packet nct6694_write_msg - Send bulk-out pipe to write request packet Send bulk-out pipe to write data packet Receive bulk-in pipe to read response packet Receive bulk-in pipe to read data packet Ming Yu (9): mfd: Add core driver for Nuvoton NCT6694 gpio: Add Nuvoton NCT6694 GPIO support i2c: Add Nuvoton NCT6694 I2C support can: Add Nuvoton NCT6694 CAN support watchdog: Add Nuvoton NCT6694 WDT support hwmon: Add Nuvoton NCT6694 HWMON support iio: adc: Add Nuvoton NCT6694 IIO support pwm: Add Nuvoton NCT6694 PWM support rtc: Add Nuvoton NCT6694 RTC support MAINTAINERS | 15 + drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-nct6694.c | 489 ++++++++++++++++++ drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/nct6694-hwmon.c | 407 +++++++++++++++ drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-nct6694.c | 166 ++++++ drivers/iio/adc/Kconfig | 10 + drivers/iio/adc/Makefile | 1 + drivers/iio/adc/nct6694_adc.c | 616 ++++++++++++++++++++++ drivers/mfd/Kconfig | 10 + drivers/mfd/Makefile | 2 + drivers/mfd/nct6694.c | 394 +++++++++++++++ drivers/net/can/Kconfig | 10 + drivers/net/can/Makefile | 1 + drivers/net/can/nct6694_canfd.c | 843 +++++++++++++++++++++++++++++++ drivers/pwm/Kconfig | 10 + drivers/pwm/Makefile | 1 + drivers/pwm/pwm-nct6694.c | 245 +++++++++ drivers/rtc/Kconfig | 10 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-nct6694.c | 276 ++++++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/nct6694_wdt.c | 329 ++++++++++++ include/linux/mfd/nct6694.h | 168 ++++++ 29 files changed, 4051 insertions(+) create mode 100644 drivers/gpio/gpio-nct6694.c create mode 100644 drivers/hwmon/nct6694-hwmon.c create mode 100644 drivers/i2c/busses/i2c-nct6694.c create mode 100644 drivers/iio/adc/nct6694_adc.c create mode 100644 drivers/mfd/nct6694.c create mode 100644 drivers/net/can/nct6694_canfd.c create mode 100644 drivers/pwm/pwm-nct6694.c create mode 100644 drivers/rtc/rtc-nct6694.c create mode 100644 drivers/watchdog/nct6694_wdt.c create mode 100644 include/linux/mfd/nct6694.h