| Message ID | 20241227095727.2401257-1-a0282524688@gmail.com |
|---|---|
| Headers | show |
| Series | Add Nuvoton NCT6694 MFD drivers | expand |
On 30/12/2024 at 15:32, Ming Yu wrote: > Dear Vincent, > > Thank you for your comments, > > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月27日 週五 下午11:34寫道： (...) >>> 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..0f31489ef9fa >>> --- /dev/null >>> +++ b/drivers/mfd/nct6694.c >> >> If I understand correctly, your device is an USB device, so shouldn't it >> be under >> >> drivers/usb/mfd/nct6694.c >> >> ? > > I understand, but there is no drivers/usb/mfd/ directory, I believe my > device is similar to dln2.c and viperboard.c, which is why I placed it > under drivers/mfd/ Well, at the end, this is not my tree. Maybe I am saying something silly here? I am fine to defer this problem to the more relevant people. If the maintainers from the linux-usb mailing list are happy like you did, then so am I. >> At the moment, I see no USB maintainers in CC (this is why I added >> linux-usb myself). By putting it in the correct folder, the >> get_maintainers.pl will give you the correct list of persons to put in copy. >> > > Okay, I will add CC to linux-usb from now on. Ack. >> The same comment applies to the other modules. For example, I would >> expect to see the CAN module under: >> >> drivers/net/can/usb/nct6694_canfd.c >> > > Understood! I will move the can driver to drivers/net/can/usb/ in v5. Ack. (...) >>> +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, >>> + u16 length, void *buf) >>> +{ >>> + union nct6694_usb_msg *msg = nct6694->usb_msg; >>> + int tx_len, rx_len, ret; >>> + >>> + guard(mutex)(&nct6694->access_lock); >>> + >>> + memset(msg, 0, sizeof(*msg)); >>> + >>> + /* Send command packet to USB device */ >>> + msg->cmd_header.mod = mod; >>> + msg->cmd_header.cmd = offset & 0xFF; >>> + msg->cmd_header.sel = (offset >> 8) & 0xFF; >> >> In the other modules, you have some macros to combine together the cmd >> and the sel (selector, I guess?). For example from nct6694_canfd.c: >> >> #define NCT6694_CAN_DELIVER(buf_cnt) \ >> ((((buf_cnt) & 0xFF) << 8) | 0x10) /* CMD|SEL */ >> >> And here, you split them again. So what was the point to combine those >> together in the first place? >> > > Due to these two bytes may used to OFFSET in report channel for other > modules(gpio, hwmon), I will modify them below... > >> Can't you just pass both the cmd and the sel as two separate argument? >> Those cmd and sel concatenation macros are too confusing. >> >> Also, if you are worried of having too many arguments in >> nct6694_read_msg(), you may just directly pass a pointer to a struct >> nct6694_cmd_header instead of all the arguments separately. >> > > ... > in mfd/nct6694.c > inline struct nct6694_cmd_header nct6694_init_cmd(u8 mod, u8 cmd, u8 sel, > u16 offset, u16 length) > { > struct nct6694_cmd_header header; > > header.mod = mod; > header.cmd = cmd; > header.sel = sel; > header.offset = cpu_to_le16(offset); I am not sure how this is supposed to work. If the both the offset and the cmd/sel pair occupies the same slot in memory, then the offset would just overwrite what you just put in the cmd and sel fields. > header.len = cpu_to_le16(length); > > return header; > } > EXPORT_SYMBOL(nct6694_init_cmd); > > int nct6694_read_msg(struct nct6694 *nct6694, struct nct6694_cmd_header *header, > void *buf) > { > union nct6694_usb_msg *msg = nct6694->usb_msg; > ... > msg->cmd_header.mod = header->mod; > msg->cmd_header.hctrl = NCT6694_HCTRL_GET; > msg->cmd_header.len = header->len; > if (msg->cmd_header.mod == 0xFF) { > msg->cmd_header.offset = header->offset; > } else { > msg->cmd_header.cmd = header->cmd; > msg->cmd_header.sel = header->sel; > } > ... > } > (also apply to nct6694_write_msg) > > in other drivers, for example: gpio-nct6694.c > struct nct6694_cmd_header cmd; > int ret; > > guard(mutex)(&data->lock); > > cmd = nct6694_init_cmd(NCT6694_GPIO_MOD, 0, 0, > NCT6694_GPO_DIR + data->group, > sizeof(data->reg_val)); > > ret = nct6694_read_msg(data->nct6694, &cmd, &data->reg_val); > if (ret < 0) > return ret; > > Do you think this approach would be better? If the two bytes may be used separately or in combination, then I think it is better to describe this in your structure. Something like this: struct nct6694_cmd_header { u8 rsv1; u8 mod; union { __le16 offset; struct { u8 cmd; u8 sel; }; __packed } __packed; u8 hctrl; u8 rsv2; __le16 len; } __packed; Then, your prototype becomes: int nct6694_read_msg(struct nct6694 *nct6694, struct nct6694_cmd_header *cmd_hd, void *buf) If the caller needs to pass an offset: void foo(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length) { struct nct6694_cmd_header cmd_hd = { 0 }; cmd_hd.mod = mod; cmd_hd.offset = cpu_to_le16(offset); cmd_hd.len = cpu_to_le16(length); nct6694_read_msg(nct6694, &cmd_hd, NULL); } If the caller needs to pass a cmd and sel pair: void foo(struct nct6694 *nct6694, u8 mod, u8 cmd, u8 sel, u16 length) { struct nct6694_cmd_header cmd_hd = { 0 }; cmd_hd.mod = mod; cmd_hd.cmd = cmd; cmd_hd.sel = sel; cmd_hd.len = cpu_to_le16(length); nct6694_read_msg(nct6694, &cmd_hd, NULL); } This way, no more cmd and sel concatenation/deconcatenation and no conditional if/else logic. cmd_hd.hctrl (and other similar fields which are common to everyone) may be set in nct6694_read_msg(). (...) Yours sincerely, Vincent Mailhol
Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月30日 週一 下午3:34寫道： > ... > >> If I understand correctly, your device is an USB device, so shouldn't it > >> be under > >> > >> drivers/usb/mfd/nct6694.c > >> > >> ? > > > > I understand, but there is no drivers/usb/mfd/ directory, I believe my > > device is similar to dln2.c and viperboard.c, which is why I placed it > > under drivers/mfd/ > > Well, at the end, this is not my tree. Maybe I am saying something silly > here? I am fine to defer this problem to the more relevant people. If > the maintainers from the linux-usb mailing list are happy like you did, > then so am I. > Understood. > >> At the moment, I see no USB maintainers in CC (this is why I added > >> linux-usb myself). By putting it in the correct folder, the > >> get_maintainers.pl will give you the correct list of persons to put in copy. > >> > > > > Okay, I will add CC to linux-usb from now on. > > Ack. > > >> The same comment applies to the other modules. For example, I would > >> expect to see the CAN module under: > >> > >> drivers/net/can/usb/nct6694_canfd.c > >> > > > > Understood! I will move the can driver to drivers/net/can/usb/ in v5. > > Ack. > > (...) > > >>> +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset, > >>> + u16 length, void *buf) > >>> +{ ... > > If the two bytes may be used separately or in combination, then I think > it is better to describe this in your structure. Something like this: > > struct nct6694_cmd_header { > u8 rsv1; > u8 mod; > union { > __le16 offset; > struct { > u8 cmd; > u8 sel; > }; __packed > } __packed; > u8 hctrl; > u8 rsv2; > __le16 len; > } __packed; > Sorry for forgetting to list the structure in last mail, but the revised structure is same as yours. > Then, your prototype becomes: > > int nct6694_read_msg(struct nct6694 *nct6694, > struct nct6694_cmd_header *cmd_hd, > void *buf) > > If the caller needs to pass an offset: > > void foo(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length) > { > struct nct6694_cmd_header cmd_hd = { 0 }; > > cmd_hd.mod = mod; > cmd_hd.offset = cpu_to_le16(offset); > cmd_hd.len = cpu_to_le16(length); > > nct6694_read_msg(nct6694, &cmd_hd, NULL); > } > > If the caller needs to pass a cmd and sel pair: > > void foo(struct nct6694 *nct6694, u8 mod, u8 cmd, u8 sel, u16 length) > { > struct nct6694_cmd_header cmd_hd = { 0 }; > > cmd_hd.mod = mod; > cmd_hd.cmd = cmd; > cmd_hd.sel = sel; > cmd_hd.len = cpu_to_le16(length); > > nct6694_read_msg(nct6694, &cmd_hd, NULL); > } > > This way, no more cmd and sel concatenation/deconcatenation and no > conditional if/else logic. > > cmd_hd.hctrl (and other similar fields which are common to everyone) may > be set in nct6694_read_msg(). > Understood, that means I need to export these four function, right? - int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u8 cmd, u8 sel, u16 length, void *buf); - int nct6694_read_rpt(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, void *buf); - int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u8 cmd, u8 sel, u16 length, void *buf); - int nct6694_write_rpt(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length, void *buf); Both nct6694_read_msg() and nct6694_read_rpt() call nct6694_read(struct nct6694 *nct6694, struct nct6694_cmd_header cmd_hd, void *buf), then nct6694_write_msg() and nct6694_write_rpt() call nct6694_write(struct nct6694 *nct6694, struct nct6694_cmd_header cmd_hd, void *buf). (nct6694_read/nct6694_write is origin nct6694_read_msg/nct6694_write_msg) Thanks, Ming
On 30/12/2024 at 17:47, Ming Yu wrote: > Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月30日 週一 下午3:34寫道： (...) >> If the two bytes may be used separately or in combination, then I think >> it is better to describe this in your structure. Something like this: >> >> struct nct6694_cmd_header { >> u8 rsv1; >> u8 mod; >> union { >> __le16 offset; >> struct { >> u8 cmd; >> u8 sel; >> }; __packed >> } __packed; >> u8 hctrl; >> u8 rsv2; >> __le16 len; >> } __packed; >> > > Sorry for forgetting to list the structure in last mail, but the > revised structure is same as yours. > >> Then, your prototype becomes: >> >> int nct6694_read_msg(struct nct6694 *nct6694, >> struct nct6694_cmd_header *cmd_hd, >> void *buf) >> >> If the caller needs to pass an offset: >> >> void foo(struct nct6694 *nct6694, u8 mod, u16 offset, u16 length) >> { >> struct nct6694_cmd_header cmd_hd = { 0 }; >> >> cmd_hd.mod = mod; >> cmd_hd.offset = cpu_to_le16(offset); >> cmd_hd.len = cpu_to_le16(length); >> >> nct6694_read_msg(nct6694, &cmd_hd, NULL); >> } >> >> If the caller needs to pass a cmd and sel pair: >> >> void foo(struct nct6694 *nct6694, u8 mod, u8 cmd, u8 sel, u16 length) >> { >> struct nct6694_cmd_header cmd_hd = { 0 }; >> >> cmd_hd.mod = mod; >> cmd_hd.cmd = cmd; >> cmd_hd.sel = sel; >> cmd_hd.len = cpu_to_le16(length); >> >> nct6694_read_msg(nct6694, &cmd_hd, NULL); >> } >> >> This way, no more cmd and sel concatenation/deconcatenation and no >> conditional if/else logic. >> >> cmd_hd.hctrl (and other similar fields which are common to everyone) may >> be set in nct6694_read_msg(). >> > > Understood, that means I need to export these four function, right? > - int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u8 cmd, > u8 sel, u16 length, void *buf); > - int nct6694_read_rpt(struct nct6694 *nct6694, u8 mod, u16 offset, > u16 length, void *buf); > - int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u8 cmd, > u8 sel, u16 length, void *buf); > - int nct6694_write_rpt(struct nct6694 *nct6694, u8 mod, u16 offset, > u16 length, void *buf); > > Both nct6694_read_msg() and nct6694_read_rpt() call > nct6694_read(struct nct6694 *nct6694, struct nct6694_cmd_header > cmd_hd, void *buf), > then nct6694_write_msg() and nct6694_write_rpt() call > nct6694_write(struct nct6694 *nct6694, struct nct6694_cmd_header > cmd_hd, void *buf). > (nct6694_read/nct6694_write is origin nct6694_read_msg/nct6694_write_msg) I was more thinking of exposing: int nct6694_read_msg(struct nct6694 *nct6694, struct nct6694_cmd_header *cmd_hd, void *buf); and: int nct6694_write_msg(struct nct6694 *nct6694, struct nct6694_cmd_header *cmd_hd, void *buf); and then the different modules fill the cmd_hd argument themselves and directly call one of those two functions with no intermediaries. But your solution is also acceptable. The core issue is the artificial packing and depacking of the cmd and sel pair. As long as this core issue is resolved and as long as the ugly concatenation macro can be removed, I think it is OK. Choose the approach you prefer :) Yours sincerely, Vincent Mailhol
This patch series introduces support for Nuvoton NCT6694, a peripheral expander based on USB interface. It models the chip as an MFD driver (1/7), GPIO driver(2/7), I2C Adapter driver(3/7), CANfd driver(4/7), WDT driver(5/7), HWMON driver(6/7), and RTC driver(7/7). 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 request interrupt 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 Changes since version 3: - Modify array buffer to structure for each drivers - Fix defines and comments for each drivers - Add header <linux/bits.h> and use BIT macro in nct6694.c and gpio-nct6694.c - Modify mutex_init() to devm_mutex_init() - Add rx-offload helper in nct6694_canfd.c - Drop watchdog_init_timeout() in nct6694_wdt.c - Modify the division method to DIV_ROUND_CLOSEST() in nct6694-hwmon.c - Drop private mutex and use rtc core lock in rtc-nct6694.c - Modify device_set_wakeup_capable() to device_init_wakeup() in rtc-nct6694.c Changes since version 2: - Add MODULE_ALIAS() for each child driver - Modify gpio line names be a local variable in gpio-nct6694.c - Drop unnecessary platform_get_drvdata() in gpio-nct6694.c - Rename each command in nct6694_canfd.c - Modify each function name consistently in nct6694_canfd.c - Modify the pretimeout validation procedure in nct6694_wdt.c - Fix warnings in nct6694-hwmon.c Changes since version 1: - Implement IRQ domain to handle IRQ demux in nct6694.c - Modify USB_DEVICE to USB_DEVICE_AND_INTERFACE_INFO API in nct6694.c - Add each driver's command structure - Fix USB functions in nct6694.c - Fix platform driver registration in each child driver - Sort each driver's header files alphabetically - Drop unnecessary header in gpio-nct6694.c - Add gpio line names in gpio-nct6694.c - Fix errors and warnings in nct6694_canfd.c - Fix TX-flow control in nct6694_canfd.c - Fix warnings in nct6694_wdt.c - Drop unnecessary logs in nct6694_wdt.c - Modify start() function to setup device in nct6694_wdt.c - Add voltage sensors functionality in nct6694-hwmon.c - Add temperature sensors functionality in nct6694-hwmon.c - Fix overwrite error return values in nct6694-hwmon.c - Add write value limitation for each write() function in nct6694-hwmon.c - Drop unnecessary logs in rtc-nct6694.c - Fix overwrite error return values in rtc-nct6694.c - Modify to use dev_err_probe API in rtc-nct6694.c Ming Yu (7): 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 rtc: Add Nuvoton NCT6694 RTC support MAINTAINERS | 13 + drivers/gpio/Kconfig | 12 + drivers/gpio/Makefile | 1 + drivers/gpio/gpio-nct6694.c | 462 +++++++++++++++++ drivers/hwmon/Kconfig | 10 + drivers/hwmon/Makefile | 1 + drivers/hwmon/nct6694-hwmon.c | 851 +++++++++++++++++++++++++++++++ drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile | 1 + drivers/i2c/busses/i2c-nct6694.c | 156 ++++++ 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 | 826 ++++++++++++++++++++++++++++++ drivers/rtc/Kconfig | 10 + drivers/rtc/Makefile | 1 + drivers/rtc/rtc-nct6694.c | 263 ++++++++++ drivers/watchdog/Kconfig | 11 + drivers/watchdog/Makefile | 1 + drivers/watchdog/nct6694_wdt.c | 281 ++++++++++ include/linux/mfd/nct6694.h | 142 ++++++ 23 files changed, 3469 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/mfd/nct6694.c create mode 100644 drivers/net/can/nct6694_canfd.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