Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

[Vincent Mailhol]

+cc: linux-usb@vger.kernel.org

On 27/12/2024 at 18:57, 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 request
> interrupt that will be called when the USB device receives its
> interrupt pipe.
> 
> Signed-off-by: Ming Yu <a0282524688@gmail.com>
> ---
>  MAINTAINERS                 |   7 +
>  drivers/mfd/Kconfig         |  10 +
>  drivers/mfd/Makefile        |   2 +
>  drivers/mfd/nct6694.c       | 394 ++++++++++++++++++++++++++++++++++++
>  include/linux/mfd/nct6694.h | 142 +++++++++++++
>  5 files changed, 555 insertions(+)
>  create mode 100644 drivers/mfd/nct6694.c
>  create mode 100644 include/linux/mfd/nct6694.h
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 910305c11e8a..acb270037642 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -16722,6 +16722,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 ae23b317a64e..5429ba97b457 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -558,6 +558,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.
                                ^^^^^^
device

> +	  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 e057d6d6faef..9d0365ba6a26 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -117,6 +117,8 @@ obj-$(CONFIG_TWL6040_CORE)	+= twl6040.o
>  
>  obj-$(CONFIG_MFD_MX25_TSADC)	+= fsl-imx25-tsadc.o
>  
> +obj-$(CONFIG_MFD_NCT6694)	+= nct6694.o

Keep the alphabetic order. MFD_NCT6694 is after MFD_MC13XXX in the alphabet.

>  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

?

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.

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

> @@ -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/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>
> +
> +#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(HWMON),
> +	MFD_DEV_SIMPLE(RTC),
> +};
> +
> +static int nct6694_response_err_handling(struct nct6694 *nct6694,
> +					 unsigned char err_status)
> +{
> +	struct device *dev = &nct6694->udev->dev;
> +
> +	switch (err_status) {
> +	case NCT6694_NO_ERROR:
> +		return err_status;
> +	case NCT6694_NOT_SUPPORT_ERROR:
> +		dev_dbg(dev, "%s: Command is not support!\n", __func__);

Grammar: Command is not supported

> +		break;
> +	case NCT6694_NO_RESPONSE_ERROR:
> +		dev_dbg(dev, "%s: Command is no response!\n", __func__);

Not sure what you meant here. Maybe: command didn't get a response? But
then, I do not see the nuance with the timeout.

> +		break;
> +	case NCT6694_TIMEOUT_ERROR:
> +		dev_dbg(dev, "%s: Command is timeout!\n", __func__);

Grammar: Command timed out
> +		break;
> +	case NCT6694_PENDING:
> +		dev_dbg(dev, "%s: Command is pending!\n", __func__);
> +		break;> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return -EIO;
> +}
> +
> +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?

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.

> +	msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
> +	msg->cmd_header.len = cpu_to_le16(length);
> +
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> +			   &msg->cmd_header, sizeof(*msg), &tx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		return ret;
> +
> +	/* Receive response packet from USB device */
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> +			   &msg->response_header, sizeof(*msg), &rx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		return ret;
> +
> +	/* Receive data packet from USB device */
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> +			   buf, NCT6694_MAX_PACKET_SZ, &rx_len, nct6694->timeout);
> +	if (ret)
> +		return ret;
> +
> +	if (rx_len != length) {
> +		dev_dbg(&nct6694->udev->dev, "%s: Received length is not match!\n",
> +			__func__);
> +		return -EIO;
> +	}
> +
> +	return nct6694_response_err_handling(nct6694, msg->response_header.sts);
> +}
> +EXPORT_SYMBOL(nct6694_read_msg);
> +
> +int nct6694_write_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;
> +	msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> +	msg->cmd_header.len = cpu_to_le16(length);
> +
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> +			   &msg->cmd_header, sizeof(*msg), &tx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		return ret;
> +
> +	/* Send data packet to USB device */
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> +			   buf, length, &tx_len, nct6694->timeout);
> +	if (ret)
> +		return ret;
> +
> +	/* Receive response packet from USB device */
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> +			   &msg->response_header, sizeof(*msg), &rx_len,
> +			   nct6694->timeout);
> +	if (ret)
> +		return ret;
> +
> +	/* Receive data packet from USB device */
> +	ret = usb_bulk_msg(nct6694->udev,
> +			   usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> +			   buf, NCT6694_MAX_PACKET_SZ, &rx_len, nct6694->timeout);

What if the object size of buf is smaller than NCT6694_MAX_PACKET_SZ?

> +	if (ret)
> +		return ret;
> +
> +	if (rx_len != length) {
> +		dev_dbg(&nct6694->udev->dev, "%s: Sent length is not match!\n",
> +			__func__);
> +		return -EIO;
> +	}
> +
> +	return nct6694_response_err_handling(nct6694, msg->response_header.sts);
> +}
> +EXPORT_SYMBOL(nct6694_write_msg);
> +
> +static void usb_int_callback(struct urb *urb)
> +{
> +	struct nct6694 *nct6694 = urb->context;
> +	struct device *dev = &nct6694->udev->dev;
> +	unsigned int *int_status = urb->transfer_buffer;
> +	int ret;
> +
> +	switch (urb->status) {
> +	case 0:
> +		break;
> +	case -ECONNRESET:
> +	case -ENOENT:
> +	case -ESHUTDOWN:
> +		return;
> +	default:
> +		goto resubmit;
> +	}
> +
> +	while (*int_status) {
> +		int irq = __ffs(*int_status);
> +
> +		generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
> +		*int_status &= ~BIT(irq);
> +	}
> +
> +resubmit:
> +	ret = usb_submit_urb(urb, GFP_ATOMIC);
> +	if (ret)
> +		dev_dbg(dev, "%s: Failed to resubmit urb, status %d",
> +			__func__, ret);

Print the error mnemotecnic instead of the error code:

  	dev_dbg(dev, "%s: Failed to resubmit urb, status %pe",
  		__func__, ERR_PTR(ret));

(apply to other location where you print error).

> +}
> +
> +static void nct6694_irq_lock(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +
> +	mutex_lock(&nct6694->irq_lock);
> +}
> +
> +static void nct6694_irq_sync_unlock(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +
> +	mutex_unlock(&nct6694->irq_lock);
> +}
> +
> +static void nct6694_irq_enable(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(data);
> +
> +	nct6694->irq_enable |= BIT(hwirq);
> +}
> +
> +static void nct6694_irq_disable(struct irq_data *data)
> +{
> +	struct nct6694 *nct6694 = irq_data_get_irq_chip_data(data);
> +	irq_hw_number_t hwirq = irqd_to_hwirq(data);
> +
> +	nct6694->irq_enable &= ~BIT(hwirq);
> +}
> +
> +static struct irq_chip nct6694_irq_chip = {
> +	.name = "nct6694-irq",
> +	.flags = IRQCHIP_SKIP_SET_WAKE,
> +	.irq_bus_lock = nct6694_irq_lock,
> +	.irq_bus_sync_unlock = nct6694_irq_sync_unlock,
> +	.irq_enable = nct6694_irq_enable,
> +	.irq_disable = nct6694_irq_disable,
> +};
> +
> +static int nct6694_irq_domain_map(struct irq_domain *d, unsigned int irq,
> +				  irq_hw_number_t hw)
> +{
> +	struct nct6694 *nct6694 = d->host_data;
> +
> +	irq_set_chip_data(irq, nct6694);
> +	irq_set_chip_and_handler(irq, &nct6694_irq_chip, handle_simple_irq);
> +
> +	return 0;
> +}
> +
> +static void nct6694_irq_domain_unmap(struct irq_domain *d, unsigned int irq)
> +{
> +	irq_set_chip_and_handler(irq, NULL, NULL);
> +	irq_set_chip_data(irq, NULL);
> +}
> +
> +static const struct irq_domain_ops nct6694_irq_domain_ops = {
> +	.map	= nct6694_irq_domain_map,
> +	.unmap	= nct6694_irq_domain_unmap,
> +};
> +
> +static int nct6694_usb_probe(struct usb_interface *iface,
> +			     const struct usb_device_id *id)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct usb_endpoint_descriptor *int_endpoint;
> +	struct usb_host_interface *interface;
> +	struct device *dev = &udev->dev;
> +	struct nct6694 *nct6694;
> +	int pipe, maxp;
> +	int ret;
> +
> +	interface = iface->cur_altsetting;
> +
> +	int_endpoint = &interface->endpoint[0].desc;
> +	if (!usb_endpoint_is_int_in(int_endpoint))
> +		return -ENODEV;
> +
> +	nct6694 = devm_kzalloc(dev, sizeof(*nct6694), GFP_KERNEL);
> +	if (!nct6694)
> +		return -ENOMEM;
> +
> +	pipe = usb_rcvintpipe(udev, NCT6694_INT_IN_EP);
> +	maxp = usb_maxpacket(udev, pipe);
> +
> +	nct6694->usb_msg = devm_kzalloc(dev, sizeof(union nct6694_usb_msg),
> +					GFP_KERNEL);
> +	if (!nct6694->usb_msg)
> +		return -ENOMEM;
> +
> +	nct6694->int_buffer = devm_kzalloc(dev, maxp, GFP_KERNEL);
> +	if (!nct6694->int_buffer)
> +		return -ENOMEM;
> +
> +	nct6694->int_in_urb = usb_alloc_urb(0, GFP_KERNEL);
> +	if (!nct6694->int_in_urb)
> +		return -ENOMEM;
> +
> +	nct6694->domain = irq_domain_add_simple(NULL, NCT6694_NR_IRQS, 0,
> +						&nct6694_irq_domain_ops,
> +						nct6694);
> +	if (!nct6694->domain) {
> +		ret = -ENODEV;
> +		goto err_urb;
> +	}
> +
> +	nct6694->udev = udev;
> +	nct6694->timeout = NCT6694_URB_TIMEOUT;	/* Wait until urb complete */
> +
> +	devm_mutex_init(dev, &nct6694->access_lock);
> +	devm_mutex_init(dev, &nct6694->irq_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(dev, nct6694);
> +	usb_set_intfdata(iface, nct6694);
> +
> +	ret = mfd_add_hotplug_devices(dev, nct6694_dev, ARRAY_SIZE(nct6694_dev));
> +	if (ret)
> +		goto err_mfd;
> +
> +	return 0;
> +
> +err_mfd:
> +	usb_kill_urb(nct6694->int_in_urb);
> +err_urb:
> +	usb_free_urb(nct6694->int_in_urb);
> +	return ret;
> +}
> +
> +static void nct6694_usb_disconnect(struct usb_interface *iface)
> +{
> +	struct usb_device *udev = interface_to_usbdev(iface);
> +	struct nct6694 *nct6694 = usb_get_intfdata(iface);
> +
> +	mfd_remove_devices(&udev->dev);
> +	usb_kill_urb(nct6694->int_in_urb);
> +	usb_free_urb(nct6694->int_in_urb);
> +}
> +
> +static const struct usb_device_id nct6694_ids[] = {
> +	{ USB_DEVICE_AND_INTERFACE_INFO(NCT6694_VENDOR_ID,
> +					NCT6694_PRODUCT_ID,
> +					0xFF, 0x00, 0x00)},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(usb, nct6694_ids);
> +
> +static struct usb_driver nct6694_usb_driver = {
> +	.name	= "nct6694",
> +	.id_table = nct6694_ids,
> +	.probe = nct6694_usb_probe,
> +	.disconnect = nct6694_usb_disconnect,
> +};
> +
> +module_usb_driver(nct6694_usb_driver);
> +
> +MODULE_DESCRIPTION("USB-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..38c8c7af10d5
> --- /dev/null
> +++ b/include/linux/mfd/nct6694.h
> @@ -0,0 +1,142 @@
> +/* 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_HWMON	"nct6694-hwmon"
> +#define NCT6694_DEV_RTC		"nct6694-rtc"
> +
> +#define NCT6694_VENDOR_ID	0x0416
> +#define NCT6694_PRODUCT_ID	0x200B
> +#define NCT6694_INT_IN_EP	0x81
> +#define NCT6694_BULK_IN_EP	0x02
> +#define NCT6694_BULK_OUT_EP	0x03
> +#define NCT6694_MAX_PACKET_SZ	512
> +
> +#define NCT6694_HCTRL_SET	0x40
> +#define NCT6694_HCTRL_GET	0x80
> +
> +#define NCT6694_URB_TIMEOUT	1000
> +
> +enum nct6694_irq_id {
> +	NCT6694_IRQ_GPIO0 = 0,
> +	NCT6694_IRQ_GPIO1,
> +	NCT6694_IRQ_GPIO2,
> +	NCT6694_IRQ_GPIO3,
> +	NCT6694_IRQ_GPIO4,
> +	NCT6694_IRQ_GPIO5,
> +	NCT6694_IRQ_GPIO6,
> +	NCT6694_IRQ_GPIO7,
> +	NCT6694_IRQ_GPIO8,
> +	NCT6694_IRQ_GPIO9,
> +	NCT6694_IRQ_GPIOA,
> +	NCT6694_IRQ_GPIOB,
> +	NCT6694_IRQ_GPIOC,
> +	NCT6694_IRQ_GPIOD,
> +	NCT6694_IRQ_GPIOE,
> +	NCT6694_IRQ_GPIOF,
> +	NCT6694_IRQ_CAN1,
> +	NCT6694_IRQ_CAN2,
> +	NCT6694_IRQ_RTC,
> +	NCT6694_NR_IRQS,
> +};
> +
> +enum nct6694_response_err_status {
> +	NCT6694_NO_ERROR = 0,
> +	NCT6694_FORMAT_ERROR,
> +	NCT6694_RESERVED1,
> +	NCT6694_RESERVED2,
> +	NCT6694_NOT_SUPPORT_ERROR,
> +	NCT6694_NO_RESPONSE_ERROR,
> +	NCT6694_TIMEOUT_ERROR,
> +	NCT6694_PENDING,
> +};
> +
> +struct nct6694_cmd_header {
> +	u8 rsv1;
> +	u8 mod;
> +	u8 cmd;
> +	u8 sel;
> +	u8 hctrl;
> +	u8 rsv2;
> +	__le16 len;
> +} __packed;
> +
> +struct nct6694_response_header {
> +	u8 sequence_id;
> +	u8 sts;
> +	u8 reserved[4];
> +	__le16 len;
> +} __packed;
> +
> +union nct6694_usb_msg {
> +	struct nct6694_cmd_header cmd_header;
> +	struct nct6694_response_header response_header;
> +};
> +
> +struct nct6694 {
> +	struct usb_device *udev;
> +	struct urb *int_in_urb;
> +	struct irq_domain *domain;
> +	struct mutex access_lock;
> +	struct mutex irq_lock;
> +	union nct6694_usb_msg *usb_msg;
> +	unsigned char *int_buffer;
> +	unsigned int irq_enable;
> +	/* time in msec to wait for the urb to the complete */
> +	long timeout;
> +};
> +
> +/*
> + * nct6694_read_msg - Receive data from NCT6694 USB device
> + *
> + * @nct6694 - Nuvoton NCT6694 structure
> + * @mod - Module byte
> + * @offset - Offset byte or (Select byte | Command byte)
> + * @length - Length byte
> + * @buf - Read data from rx buffer
> + *
> + * USB Transaction format:
> + *
> + *	OUT	|RSV|MOD|CMD|SEL|HCTL|RSV|LEN_L|LEN_H|
> + *	OUT	|SEQ|STS|RSV|RSV|RSV|RSV||LEN_L|LEN_H|
> + *	IN	|-------D------A------D------A-------|
> + *	IN			......
> + *	IN	|-------D------A------D------A-------|

The structure already discribes this information pretty well. No need
for this table.

> + */
> +int nct6694_read_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> +		     u16 length, void *buf);
> +
> +/*
> + * nct6694_read_msg - Transmit data to NCT6694 USB device
> + *
> + * @nct6694 - Nuvoton NCT6694 structure
> + * @mod - Module byte
> + * @offset - Offset byte or (Select byte | Command byte)
> + * @length - Length byte
> + * @buf - Write data to tx buffer
> + *
> + * USB Transaction format:
> + *
> + *	OUT	|RSV|MOD|CMD|SEL|HCTL|RSV|LEN_L|LEN_H|
> + *	OUT	|-------D------A------D------A-------|
> + *	OUT			......
> + *	OUT	|-------D------A------D------A-------|
> + *	IN	|SEQ|STS|RSV|RSV|RSV|RSV||LEN_L|LEN_H|
> + *	IN	|-------D------A------D------A-------|
> + *	IN			......
> + *	IN	|-------D------A------D------A-------|
> + */
> +int nct6694_write_msg(struct nct6694 *nct6694, u8 mod, u16 offset,
> +		      u16 length, void *buf);
> +
> +#endif

Yours sincerely,
Vincent Mailhol

Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

[Ming Yu]

Dear Vincent,

Thank you for your comments,

Vincent Mailhol <mailhol.vincent@wanadoo.fr> 於 2024年12月27日 週五 下午11:34寫道：
>
> +cc: linux-usb@vger.kernel.org
>
...
> > +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.
>                                 ^^^^^^
> device
>

Fix it in v5.

> > +       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 e057d6d6faef..9d0365ba6a26 100644
> > --- a/drivers/mfd/Makefile
> > +++ b/drivers/mfd/Makefile
> > @@ -117,6 +117,8 @@ obj-$(CONFIG_TWL6040_CORE)        += twl6040.o
> >
> >  obj-$(CONFIG_MFD_MX25_TSADC) += fsl-imx25-tsadc.o
> >
> > +obj-$(CONFIG_MFD_NCT6694)    += nct6694.o
>
> Keep the alphabetic order. MFD_NCT6694 is after MFD_MC13XXX in the alphabet.
>

Fix it in v5.

> >  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/

>
> 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.

> 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.

...
> > +static int nct6694_response_err_handling(struct nct6694 *nct6694,
> > +                                      unsigned char err_status)
> > +{
> > +     struct device *dev = &nct6694->udev->dev;
> > +
> > +     switch (err_status) {
> > +     case NCT6694_NO_ERROR:
> > +             return err_status;
> > +     case NCT6694_NOT_SUPPORT_ERROR:
> > +             dev_dbg(dev, "%s: Command is not support!\n", __func__);
>
> Grammar: Command is not supported
>

Fix it in v5.

> > +             break;
> > +     case NCT6694_NO_RESPONSE_ERROR:
> > +             dev_dbg(dev, "%s: Command is no response!\n", __func__);
>
> Not sure what you meant here. Maybe: command didn't get a response? But
> then, I do not see the nuance with the timeout.
>

The firmware engine returns an error response.
If it returns NO_RESPONSE_ERROR, it means the target device did not
respond(e.g., I2C slave NACK).
If it returns TIMEOUT_ERROR, it means the engine process the command timed out.
I will add the comments to describe these error status in v5.

> > +             break;
> > +     case NCT6694_TIMEOUT_ERROR:
> > +             dev_dbg(dev, "%s: Command is timeout!\n", __func__);
>
> Grammar: Command timed out

Fix it in v5.

> > +             break;
> > +     case NCT6694_PENDING:
> > +             dev_dbg(dev, "%s: Command is pending!\n", __func__);
> > +             break;> +       default:
> > +             return -EINVAL;
> > +     }
> > +
> > +     return -EIO;
> > +}
> > +
> > +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);
        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?

> > +     msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
> > +     msg->cmd_header.len = cpu_to_le16(length);
> > +
...
> > +int nct6694_write_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;
> > +     msg->cmd_header.hctrl = NCT6694_HCTRL_SET;
> > +     msg->cmd_header.len = cpu_to_le16(length);
> > +
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> > +                        &msg->cmd_header, sizeof(*msg), &tx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Send data packet to USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_sndbulkpipe(nct6694->udev, NCT6694_BULK_OUT_EP),
> > +                        buf, length, &tx_len, nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Receive response packet from USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> > +                        &msg->response_header, sizeof(*msg), &rx_len,
> > +                        nct6694->timeout);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /* Receive data packet from USB device */
> > +     ret = usb_bulk_msg(nct6694->udev,
> > +                        usb_rcvbulkpipe(nct6694->udev, NCT6694_BULK_IN_EP),
> > +                        buf, NCT6694_MAX_PACKET_SZ, &rx_len, nct6694->timeout);
>
> What if the object size of buf is smaller than NCT6694_MAX_PACKET_SZ?
>

I will fix it to le16_to_cpu(header->len) in the v5.

> > +     if (ret)
> > +             return ret;
> > +
> > +     if (rx_len != length) {
> > +             dev_dbg(&nct6694->udev->dev, "%s: Sent length is not match!\n",
> > +                     __func__);
> > +             return -EIO;
> > +     }
> > +
> > +     return nct6694_response_err_handling(nct6694, msg->response_header.sts);
> > +}
> > +EXPORT_SYMBOL(nct6694_write_msg);
> > +
> > +static void usb_int_callback(struct urb *urb)
> > +{
> > +     struct nct6694 *nct6694 = urb->context;
> > +     struct device *dev = &nct6694->udev->dev;
> > +     unsigned int *int_status = urb->transfer_buffer;
> > +     int ret;
> > +
> > +     switch (urb->status) {
> > +     case 0:
> > +             break;
> > +     case -ECONNRESET:
> > +     case -ENOENT:
> > +     case -ESHUTDOWN:
> > +             return;
> > +     default:
> > +             goto resubmit;
> > +     }
> > +
> > +     while (*int_status) {
> > +             int irq = __ffs(*int_status);
> > +
> > +             generic_handle_irq_safe(irq_find_mapping(nct6694->domain, irq));
> > +             *int_status &= ~BIT(irq);
> > +     }
> > +
> > +resubmit:
> > +     ret = usb_submit_urb(urb, GFP_ATOMIC);
> > +     if (ret)
> > +             dev_dbg(dev, "%s: Failed to resubmit urb, status %d",
> > +                     __func__, ret);
>
> Print the error mnemotecnic instead of the error code:
>
>         dev_dbg(dev, "%s: Failed to resubmit urb, status %pe",
>                 __func__, ERR_PTR(ret));
>
> (apply to other location where you print error).
>

Understood. Fix these in v5.

> > +}
> > +
...
> > + * nct6694_read_msg - Receive data from NCT6694 USB device
> > + *
> > + * @nct6694 - Nuvoton NCT6694 structure
> > + * @mod - Module byte
> > + * @offset - Offset byte or (Select byte | Command byte)
> > + * @length - Length byte
> > + * @buf - Read data from rx buffer
> > + *
> > + * USB Transaction format:
> > + *
> > + *   OUT     |RSV|MOD|CMD|SEL|HCTL|RSV|LEN_L|LEN_H|
> > + *   OUT     |SEQ|STS|RSV|RSV|RSV|RSV||LEN_L|LEN_H|
> > + *   IN      |-------D------A------D------A-------|
> > + *   IN                      ......
> > + *   IN      |-------D------A------D------A-------|
>
> The structure already discribes this information pretty well. No need
> for this table.
>

Drop it in v5.

> > + */

Best regards,
Ming

Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

[Vincent Mailhol]

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

Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

[Ming Yu]

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

Re: [PATCH v4 1/7] mfd: Add core driver for Nuvoton NCT6694

[Vincent Mailhol]

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