Dear Vincent,

Thank you for your comments,

Vincent Mailhol [off-list ref] 於 2024年12月27日 週五 下午11:34寫道：

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

...

quoted

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

quoted

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

quoted

 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.

...

quoted

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

quoted

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

quoted

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

Grammar: Command timed out

Fix it in v5.

quoted

+             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?

quoted

+     msg->cmd_header.hctrl = NCT6694_HCTRL_GET;
+     msg->cmd_header.len = cpu_to_le16(length);
+

...

quoted

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

quoted

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

quoted

+}
+

...

quoted

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

quoted

+ */

Best regards,
Ming