On Fri, 02 May 2025, Ming Yu wrote: > Dear Lee, > > Thank you for your review. I have a few questions and would appreciate > your advice. > > Lee Jones <lee@xxxxxxxxxx> 於 2025年5月1日 週四 下午8:22寫道： > > > > On Wed, 23 Apr 2025, a0282524688@xxxxxxxxx wrote: > > > > > From: Ming Yu <tmyu0@xxxxxxxxxxx> > > > > > > The Nuvoton NCT6694 provides an USB interface to the host to > > > access its features. > > > > > > Sub-devices can use the USB functions nct6694_read_msg() and > > > nct6694_write_msg() to issue a command. They can also request > > > interrupt that will be called when the USB device receives its > > > interrupt pipe. > > > > > > Signed-off-by: Ming Yu <tmyu0@xxxxxxxxxxx> > > > --- > > > > v10 and no change log? Please add a change log. > > > > The change log is currently available at > https://patchwork.ozlabs.org/project/rtc-linux/cover/20250423094058.1656204-1-tmyu0@xxxxxxxxxxx/ > I will move the relevant entries into each individual patch in the > next revision. > > > > MAINTAINERS | 6 + > > > drivers/mfd/Kconfig | 15 ++ > > > drivers/mfd/Makefile | 2 + > > > drivers/mfd/nct6694.c | 387 ++++++++++++++++++++++++++++++++++++ > > > include/linux/mfd/nct6694.h | 101 ++++++++++ > > > 5 files changed, 511 insertions(+) > > > create mode 100644 drivers/mfd/nct6694.c > > > create mode 100644 include/linux/mfd/nct6694.h > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS > > > index fa1e04e87d1d..b2dfcc063f88 100644 > > > --- a/MAINTAINERS > > > +++ b/MAINTAINERS > > > @@ -17358,6 +17358,12 @@ F: drivers/nubus/ > > > F: include/linux/nubus.h > > > F: include/uapi/linux/nubus.h > > > > > > +NUVOTON NCT6694 MFD DRIVER > > > +M: Ming Yu <tmyu0@xxxxxxxxxxx> > > > +S: Supported > > > +F: drivers/mfd/nct6694.c > > > +F: include/linux/mfd/nct6694.h > > > + > > > NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER > > > M: Antonino Daplas <adaplas@xxxxxxxxx> > > > L: linux-fbdev@xxxxxxxxxxxxxxx > > > diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig > > > index 22b936310039..cd4d826a7fcb 100644 > > > --- a/drivers/mfd/Kconfig > > > +++ b/drivers/mfd/Kconfig > > > @@ -1058,6 +1058,21 @@ config MFD_MENF21BMC > > > This driver can also be built as a module. If so the module > > > will be called menf21bmc. > > > > > > +config MFD_NCT6694 > > > + tristate "Nuvoton NCT6694 support" > > > + select MFD_CORE > > > + depends on USB > > > + help > > > + This enables support for the Nuvoton USB device NCT6694, which shares > > > + peripherals. > > > + The Nuvoton NCT6694 is a peripheral expander with 16 GPIO chips, > > > + 6 I2C controllers, 2 CANfd controllers, 2 Watchdog timers, ADC, > > > + PWM, and RTC. > > > + This driver provides core APIs to access the NCT6694 hardware > > > + monitoring and control features. > > > + Additional drivers must be enabled to utilize the specific > > > + functionalities of the device. > > > + > > > config MFD_OCELOT > > > tristate "Microsemi Ocelot External Control Support" > > > depends on SPI_MASTER > > > diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile > > > index 948cbdf42a18..471dc1f183b8 100644 > > > --- a/drivers/mfd/Makefile > > > +++ b/drivers/mfd/Makefile > > > @@ -120,6 +120,8 @@ obj-$(CONFIG_MFD_MC13XXX) += mc13xxx-core.o > > > obj-$(CONFIG_MFD_MC13XXX_SPI) += mc13xxx-spi.o > > > obj-$(CONFIG_MFD_MC13XXX_I2C) += mc13xxx-i2c.o > > > > > > +obj-$(CONFIG_MFD_NCT6694) += nct6694.o > > > + > > > obj-$(CONFIG_MFD_CORE) += mfd-core.o > > > > > > ocelot-soc-objs := ocelot-core.o ocelot-spi.o > > > diff --git a/drivers/mfd/nct6694.c b/drivers/mfd/nct6694.c > > > new file mode 100644 > > > index 000000000000..2480ca56f350 > > > --- /dev/null > > > +++ b/drivers/mfd/nct6694.c > > > @@ -0,0 +1,387 @@ > > > +// SPDX-License-Identifier: GPL-2.0 > > > +/* > > > + * Copyright (C) 2024 Nuvoton Technology Corp. > > > + * > > > + * Nuvoton NCT6694 core driver using USB interface to provide > > > + * access to the NCT6694 hardware monitoring and control features. > > > + * > > > + * The NCT6694 is an integrated controller that provides GPIO, I2C, > > > + * CAN, WDT, HWMON and RTC management. > > > + * > > > > Superfluous blank line. > > > > Remove it in v11. > > > > + */ > > > + > > > +#include <linux/bits.h> > > > +#include <linux/interrupt.h> > > > +#include <linux/irq.h> > > > +#include <linux/irqdomain.h> > > > +#include <linux/kernel.h> > > > +#include <linux/mfd/core.h> > > > +#include <linux/mfd/nct6694.h> > > > +#include <linux/module.h> > > > +#include <linux/slab.h> > > > +#include <linux/usb.h> > > > + > > > +static const struct mfd_cell nct6694_devs[] = { > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 0), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 1), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 2), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 3), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 4), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 5), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 6), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 7), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 8), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 9), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 10), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 11), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 12), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 13), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 14), > > > + MFD_CELL_BASIC("nct6694-gpio", NULL, NULL, 0, 15), > > > > These are all identical. > > > > I thought you were going to use PLATFORM_DEVID_AUTO? In fact, you are > > already using PLATFORM_DEVID_AUTO since you are calling > > mfd_add_hotplug_devices(). So you don't need this IDs. > > > > MFD_CELL_NAME() should do. > > > > Yes, it uses PLATFORM_DEVID_AUTO, but in my implementation, the > sub-devices use cell->id instead of platform_device->id, so it doesn't > affect the current behavior. > However, if you think there's a better approach or that this should be > changed for consistency or correctness, I'm happy to update it, please > let me know your recommendation. > > When using MFD_CELL_NAME(), the platform_device->id for the GPIO > devices is assigned values from 1 to 16, and for the I2C devices from > 1 to 6, but I need the ID offset to start from 0 instead. Oh no, don't do that. mfd_cell isn't supposed to be used outside of MFD. Just use the platform_device id-- if you really need to start from 0. As an aside, I'm surprised numbering starts from 1. -- Lee Jones [李琼斯]
