Dear Marc,

Thank you for reviewing.

Marc Kleine-Budde [off-list ref] 於 2025年5月3日 週六 下午9:57寫道：

quoted

This driver supports Socket CANFD functionality for NCT6694 MFD
device based on USB interface.

Signed-off-by: Ming Yu <tmyu0@nuvoton.com>

The destroy functions nct6694_canfd_close() and nct6694_canfd_remove()
are not the exact inverse of their init functions. Se comments inline.

Please fix and add:

Reviewed-by: Marc Kleine-Budde <mkl@pengutronix.de>

Feel free to mainline this patch as part of the series outside of the
linux-can-next tree. Better ask the netdev maintainers for their OK, too.

What about transceiver delay compensation for higher CAN-FD bitrates?
How does you device handle these?

In the CAN CMD0's DBTP field, bit 23 is the TDC flag, I will add
support for enabling tdc, and firmware will automatically configure
tdco. Do you think this approach is appropriate?

quoted

---
 MAINTAINERS                         |   1 +
 drivers/net/can/usb/Kconfig         |  11 +
 drivers/net/can/usb/Makefile        |   1 +
 drivers/net/can/usb/nct6694_canfd.c | 814 ++++++++++++++++++++++++++++
 4 files changed, 827 insertions(+)
 create mode 100644 drivers/net/can/usb/nct6694_canfd.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 751b9108524a..ee8583edc2d2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS

@@ -17364,6 +17364,7 @@ S:    Supported
 F:   drivers/gpio/gpio-nct6694.c
 F:   drivers/i2c/busses/i2c-nct6694.c
 F:   drivers/mfd/nct6694.c
+F:   drivers/net/can/usb/nct6694_canfd.c
 F:   include/linux/mfd/nct6694.h

 NVIDIA (rivafb and nvidiafb) FRAMEBUFFER DRIVER

diff --git a/drivers/net/can/usb/Kconfig b/drivers/net/can/usb/Kconfig
index 9dae0c71a2e1..759e724a67cf 100644
--- a/drivers/net/can/usb/Kconfig
+++ b/drivers/net/can/usb/Kconfig

@@ -133,6 +133,17 @@ config CAN_MCBA_USB
        This driver supports the CAN BUS Analyzer interface
        from Microchip (http://www.microchip.com/development-tools/).

+config CAN_NCT6694
+     tristate "Nuvoton NCT6694 Socket CANfd support"
+     depends on MFD_NCT6694
+     select CAN_RX_OFFLOAD
+     help
+       If you say yes to this option, support will be included for Nuvoton
+       NCT6694, a USB device to socket CANfd controller.
+
+       This driver can also be built as a module. If so, the module will
+       be called nct6694_canfd.
+
 config CAN_PEAK_USB
      tristate "PEAK PCAN-USB/USB Pro interfaces for CAN 2.0b/CAN-FD"
      help

diff --git a/drivers/net/can/usb/Makefile b/drivers/net/can/usb/Makefile
index 8b11088e9a59..fcafb1ac262e 100644
--- a/drivers/net/can/usb/Makefile
+++ b/drivers/net/can/usb/Makefile

@@ -11,5 +11,6 @@ obj-$(CONFIG_CAN_F81604) += f81604.o
 obj-$(CONFIG_CAN_GS_USB) += gs_usb.o
 obj-$(CONFIG_CAN_KVASER_USB) += kvaser_usb/
 obj-$(CONFIG_CAN_MCBA_USB) += mcba_usb.o
+obj-$(CONFIG_CAN_NCT6694) += nct6694_canfd.o
 obj-$(CONFIG_CAN_PEAK_USB) += peak_usb/
 obj-$(CONFIG_CAN_UCAN) += ucan.o

diff --git a/drivers/net/can/usb/nct6694_canfd.c b/drivers/net/can/usb/nct6694_canfd.c
new file mode 100644
index 000000000000..9cf6230ffb7d
--- /dev/null
+++ b/drivers/net/can/usb/nct6694_canfd.c

@@ -0,0 +1,814 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Nuvoton NCT6694 Socket CANfd driver based on USB interface.
+ *
+ * Copyright (C) 2024 Nuvoton Technology Corp.
+ */
+
+#include <linux/can/dev.h>
+#include <linux/can/rx-offload.h>
+#include <linux/ethtool.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/netdevice.h>
+#include <linux/platform_device.h>
+
+#define DEVICE_NAME "nct6694-canfd"
+
+/* USB command module type for NCT6694 CANfd controller.
+ * This defines the module type used for communication with the NCT6694
+ * CANfd controller over the USB interface.
+ */
+#define NCT6694_CANFD_MOD                    0x05
+
+/* Command 00h - CAN Setting and Initialization */
+#define NCT6694_CANFD_SETTING                        0x00
+#define NCT6694_CANFD_SETTING_ACTIVE_CTRL1   BIT(0)
+#define NCT6694_CANFD_SETTING_ACTIVE_CTRL2   BIT(1)
+#define NCT6694_CANFD_SETTING_ACTIVE_NBTP_DBTP       BIT(2)
+#define NCT6694_CANFD_SETTING_CTRL1_MON              BIT(0)
+#define NCT6694_CANFD_SETTING_CTRL1_NISO     BIT(1)
+#define NCT6694_CANFD_SETTING_CTRL1_LBCK     BIT(2)
+#define NCT6694_CANFD_SETTING_NBTP_NTSEG2    GENMASK(6, 0)
+#define NCT6694_CANFD_SETTING_NBTP_NTSEG1    GENMASK(15, 8)
+#define NCT6694_CANFD_SETTING_NBTP_NBRP              GENMASK(24, 16)
+#define NCT6694_CANFD_SETTING_NBTP_NSJW              GENMASK(31, 25)
+#define NCT6694_CANFD_SETTING_DBTP_DSJW              GENMASK(3, 0)
+#define NCT6694_CANFD_SETTING_DBTP_DTSEG2    GENMASK(7, 4)
+#define NCT6694_CANFD_SETTING_DBTP_DTSEG1    GENMASK(12, 8)
+#define NCT6694_CANFD_SETTING_DBTP_DBRP              GENMASK(20, 16)

I will add the macro in v11: #define NCT6694_CANFD_SETTING_DBTP_TDC

quoted

+
+/* Command 01h - CAN Information */
+#define NCT6694_CANFD_INFORMATION            0x01
+#define NCT6694_CANFD_INFORMATION_SEL                0x00
+
+/* Command 02h - CAN Event */
+#define NCT6694_CANFD_EVENT                  0x02
+#define NCT6694_CANFD_EVENT_SEL(idx, mask)   \
+     ((idx ? 0x80 : 0x00) | ((mask) & 0x7F))
+
+#define NCT6694_CANFD_EVENT_MASK             GENMASK(5, 0)
+#define NCT6694_CANFD_EVT_TX_FIFO_EMPTY              BIT(7)  /* Read-clear */
+#define NCT6694_CANFD_EVT_RX_DATA_LOST               BIT(5)  /* Read-clear */
+#define NCT6694_CANFD_EVT_RX_DATA_IN         BIT(7)  /* Read-clear*/

                                                                    ^^
add a space

quoted

+

...

quoted

+static int nct6694_canfd_start(struct net_device *ndev)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+     const struct can_bittiming *d_bt = &priv->can.data_bittiming;
+     const struct can_bittiming *n_bt = &priv->can.bittiming;
+     struct nct6694_canfd_setting *setting __free(kfree) = NULL;
+     const struct nct6694_cmd_header cmd_hd = {
+             .mod = NCT6694_CANFD_MOD,
+             .cmd = NCT6694_CANFD_SETTING,
+             .sel = ndev->dev_port,
+             .len = cpu_to_le16(sizeof(*setting))
+     };
+     int ret;
+
+     setting = kzalloc(sizeof(*setting), GFP_KERNEL);
+     if (!setting)
+             return -ENOMEM;
+
+     if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
+             setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_MON);
+
+     if (priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
+             setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_NISO);
+
+     if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
+             setting->ctrl1 |= cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_LBCK);
+
+     /* Disable clock divider */
+     setting->ctrl2 = 0;
+
+     setting->nbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NSJW,
+                                            n_bt->sjw - 1) |
+                                 FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NBRP,
+                                            n_bt->brp - 1) |
+                                 FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG2,
+                                            n_bt->phase_seg2 - 1) |
+                                 FIELD_PREP(NCT6694_CANFD_SETTING_NBTP_NTSEG1,
+                                            n_bt->prop_seg + n_bt->phase_seg1 - 1));
+
+     setting->dbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DSJW,
+                                            d_bt->sjw - 1) |
+                                 FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DBRP,
+                                            d_bt->brp - 1) |
+                                 FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG2,
+                                            d_bt->phase_seg2 - 1) |
+                                 FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG1,
+                                            d_bt->prop_seg + d_bt->phase_seg1 - 1));

I'll update the code to:
     setting->dbtp = cpu_to_le32(FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DSJW,
                                            d_bt->sjw - 1) |
                                 FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DBRP,
                                            d_bt->brp - 1) |
                                 FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG2,
                                            d_bt->phase_seg2 - 1) |
                                FIELD_PREP(NCT6694_CANFD_SETTING_DBTP_DTSEG1,
                                           d_bt->prop_seg +
d_bt->phase_seg1 - 1) |
                                NCT6694_CANFD_SETTING_DBTP_TDC);

quoted

+
+     setting->active = NCT6694_CANFD_SETTING_ACTIVE_CTRL1 |
+                       NCT6694_CANFD_SETTING_ACTIVE_CTRL2 |
+                       NCT6694_CANFD_SETTING_ACTIVE_NBTP_DBTP;
+
+     ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
+     if (ret)
+             return ret;
+
+     priv->can.state = CAN_STATE_ERROR_ACTIVE;
+
+     return 0;
+}
+
+static void nct6694_canfd_stop(struct net_device *ndev)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+     struct nct6694_canfd_setting *setting __free(kfree) = NULL;
+     const struct nct6694_cmd_header cmd_hd = {
+             .mod = NCT6694_CANFD_MOD,
+             .cmd = NCT6694_CANFD_SETTING,
+             .sel = ndev->dev_port,
+             .len = cpu_to_le16(sizeof(*setting))
+     };
+
+     /* The NCT6694 cannot be stopped. To ensure safe operation and avoid
+      * interference, the control mode is set to Listen-Only mode. This
+      * mode allows the device to monitor bus activity without actively
+      * participating in communication.
+      */
+     setting = kzalloc(sizeof(*setting), GFP_KERNEL);
+     if (!setting)
+             return;
+
+     nct6694_read_msg(priv->nct6694, &cmd_hd, setting);
+     setting->ctrl1 = cpu_to_le16(NCT6694_CANFD_SETTING_CTRL1_MON);
+     setting->active = NCT6694_CANFD_SETTING_ACTIVE_CTRL1;
+     nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
+
+     priv->can.state = CAN_STATE_STOPPED;
+}
+
+static int nct6694_canfd_close(struct net_device *ndev)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+

make this inverse to nct6694_canfd_open()

Fix it in v11.

quoted

+     netif_stop_queue(ndev);
+     can_rx_offload_disable(&priv->offload);
+     nct6694_canfd_stop(ndev);
+     free_irq(ndev->irq, ndev);
+     destroy_workqueue(priv->wq);
+     close_candev(ndev);
+     return 0;
+}
+
+static int nct6694_canfd_set_mode(struct net_device *ndev, enum can_mode mode)
+{
+     int ret;
+
+     switch (mode) {
+     case CAN_MODE_START:
+             ret = nct6694_canfd_start(ndev);
+             if (ret)
+                     return ret;
+
+             netif_wake_queue(ndev);
+             break;
+
+     default:
+             return -EOPNOTSUPP;
+     }
+
+     return ret;
+}
+
+static int nct6694_canfd_open(struct net_device *ndev)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+     int ret;
+
+     ret = open_candev(ndev);
+     if (ret)
+             return ret;
+
+     can_rx_offload_enable(&priv->offload);
+
+     ret = request_threaded_irq(ndev->irq, NULL,
+                                nct6694_canfd_irq, IRQF_ONESHOT,
+                                "nct6694_canfd", ndev);
+     if (ret) {
+             netdev_err(ndev, "Failed to request IRQ\n");
+             goto close_candev;

nitpick: rename to can_rx_offload_disable

Fix it in v11.

quoted

+     }
+
+     priv->wq = alloc_ordered_workqueue("%s-nct6694_wq",
+                                        WQ_FREEZABLE | WQ_MEM_RECLAIM,
+                                        ndev->name);
+     if (!priv->wq) {
+             ret = -ENOMEM;
+             goto free_irq;
+     }
+
+     ret = nct6694_canfd_start(ndev);
+     if (ret)
+             goto destroy_wq;
+
+     netif_start_queue(ndev);
+
+     return 0;
+
+destroy_wq:
+     destroy_workqueue(priv->wq);
+free_irq:
+     free_irq(ndev->irq, ndev);
+close_candev:
+     can_rx_offload_disable(&priv->offload);
+     close_candev(ndev);
+     return ret;
+}
+
+static const struct net_device_ops nct6694_canfd_netdev_ops = {
+     .ndo_open = nct6694_canfd_open,
+     .ndo_stop = nct6694_canfd_close,
+     .ndo_start_xmit = nct6694_canfd_start_xmit,
+     .ndo_change_mtu = can_change_mtu,
+};
+
+static const struct ethtool_ops nct6694_canfd_ethtool_ops = {
+     .get_ts_info = ethtool_op_get_ts_info,
+};
+
+static int nct6694_canfd_get_clock(struct nct6694_canfd_priv *priv)
+{
+     struct nct6694_canfd_information *info __free(kfree) = NULL;
+     static const struct nct6694_cmd_header cmd_hd = {
+             .mod = NCT6694_CANFD_MOD,
+             .cmd = NCT6694_CANFD_INFORMATION,
+             .sel = NCT6694_CANFD_INFORMATION_SEL,
+             .len = cpu_to_le16(sizeof(*info))
+     };
+     int ret, can_clk;
+
+     info = kzalloc(sizeof(*info), GFP_KERNEL);
+     if (!info)
+             return -ENOMEM;
+
+     ret = nct6694_read_msg(priv->nct6694, &cmd_hd, info);
+     if (ret)
+             return ret;
+
+     can_clk = le32_to_cpu(info->can_clk);

return le32_to_cpu(info->can_clk);

Fix it in v11.

quoted

+
+     return can_clk;
+}
+
+static int nct6694_canfd_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_canfd_priv *priv;
+     struct net_device *ndev;
+     int ret, irq, can_clk;
+
+     irq = irq_create_mapping(nct6694->domain,
+                              NCT6694_IRQ_CAN0 + cell->id);
+     if (!irq)
+             return irq;
+
+     ndev = alloc_candev(sizeof(struct nct6694_canfd_priv), 1);
+     if (!ndev) {
+             ret = -ENOMEM;
+             goto dispose_irq;
+     }
+
+     ndev->irq = irq;
+     ndev->flags |= IFF_ECHO;
+     ndev->dev_port = cell->id;
+     ndev->netdev_ops = &nct6694_canfd_netdev_ops;
+     ndev->ethtool_ops = &nct6694_canfd_ethtool_ops;
+
+     priv = netdev_priv(ndev);
+     priv->nct6694 = nct6694;
+     priv->ndev = ndev;
+
+     can_clk = nct6694_canfd_get_clock(priv);
+     if (can_clk < 0) {
+             ret = dev_err_probe(&pdev->dev, can_clk,
+                                 "Failed to get clock\n");
+             goto free_candev;
+     }
+
+     INIT_WORK(&priv->tx_work, nct6694_canfd_tx_work);
+
+     priv->can.clock.freq = can_clk;
+     priv->can.bittiming_const = &nct6694_canfd_bittiming_nominal_const;
+     priv->can.data_bittiming_const = &nct6694_canfd_bittiming_data_const;
+     priv->can.do_set_mode = nct6694_canfd_set_mode;
+     priv->can.do_get_berr_counter = nct6694_canfd_get_berr_counter;
+     priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK |
+             CAN_CTRLMODE_LISTENONLY | CAN_CTRLMODE_BERR_REPORTING |
+             CAN_CTRLMODE_FD_NON_ISO;
+
+     ret = can_set_static_ctrlmode(ndev, CAN_CTRLMODE_FD);
+     if (ret)
+             goto free_candev;
+
+     ret = can_rx_offload_add_manual(ndev, &priv->offload,
+                                     NCT6694_NAPI_WEIGHT);
+     if (ret) {
+             dev_err_probe(&pdev->dev, ret, "Failed to add rx_offload\n");
+             goto free_candev;
+     }
+
+     platform_set_drvdata(pdev, priv);
+     SET_NETDEV_DEV(priv->ndev, &pdev->dev);
+
+     ret = register_candev(priv->ndev);
+     if (ret)
+             goto rx_offload_del;
+
+     return 0;
+
+rx_offload_del:
+     can_rx_offload_del(&priv->offload);
+free_candev:
+     free_candev(ndev);
+dispose_irq:
+     irq_dispose_mapping(irq);
+     return ret;
+}
+
+static void nct6694_canfd_remove(struct platform_device *pdev)
+{
+     struct nct6694_canfd_priv *priv = platform_get_drvdata(pdev);
+     struct net_device *ndev = priv->ndev;
+
+     unregister_candev(ndev);
+     irq_dispose_mapping(ndev->irq);
+     can_rx_offload_del(&priv->offload);
+     free_candev(ndev);

Make the order inverse to the nct6694_canfd_probe() function.

Fix it in v11.


Best regards,
Ming