[v7,4/7] can: Add Nuvoton NCT6694 CANFD support

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

Signed-off-by: Ming Yu <a0282524688@gmail.com>
---
 MAINTAINERS                         |   1 +
 drivers/net/can/usb/Kconfig         |  11 +
 drivers/net/can/usb/Makefile        |   1 +
 drivers/net/can/usb/nct6694_canfd.c | 809 ++++++++++++++++++++++++++++
 4 files changed, 822 insertions(+)
 create mode 100644 drivers/net/can/usb/nct6694_canfd.c

Dear Marc,

Thank you for reviewing,

Marc Kleine-Budde <mkl@pengutronix.de> 於 2025年2月7日 週五 下午8:15寫道：
>
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Nuvoton NCT6694 Socket CANfd driver based on USB interface.
> > + *
> > + * Copyright (C) 2024 Nuvoton Technology Corp.
> > + */
>
> /* net-dev style multiline
>  * comments look like this
>  */
>

Okay, I will fix these comments in the next patch.

> > +
> > +#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 DRVNAME "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.
> > + */
>
> /* net-dev style multiline
>  * comments look like this
>  */
>
> or remove the comment altogether as it's stating the obvious :)
>

Okay, I will remove it in the next patch.

> > +#define NCT6694_CAN_MOD                      0x05
> > +
> > +/* Command 00h - CAN Setting and Initialization */
> > +#define NCT6694_CAN_SETTING          0x00
> > +#define NCT6694_CAN_SETTING_CTRL1_MON        BIT(0)
> > +#define NCT6694_CAN_SETTING_CTRL1_NISO       BIT(1)
> > +#define NCT6694_CAN_SETTING_CTRL1_LBCK       BIT(2)
> > +
> > +/* Command 01h - CAN Information */
> > +#define NCT6694_CAN_INFORMATION              0x01
> > +#define NCT6694_CAN_INFORMATION_SEL  0x00
> > +
> > +/* Command 02h - CAN Event */
> > +#define NCT6694_CAN_EVENT            0x02
> > +#define NCT6694_CAN_EVENT_SEL(idx, mask)     \
> > +     ((idx ? 0x80 : 0x00) | ((mask) & 0x7F))
> > +
> > +#define NCT6694_CAN_EVENT_MASK               GENMASK(5, 0)
> > +#define NCT6694_CAN_EVT_TX_FIFO_EMPTY        BIT(7)  /* Read-clear */
> > +#define NCT6694_CAN_EVT_RX_DATA_LOST BIT(5)  /* Read-clear */
> > +#define NCT6694_CAN_EVT_RX_DATA_IN   BIT(7)  /* Read-clear*/
> > +
> > +/* Command 10h - CAN Deliver */
> > +#define NCT6694_CAN_DELIVER          0x10
> > +#define NCT6694_CAN_DELIVER_SEL(buf_cnt)     \
> > +     ((buf_cnt) & 0xFF)
> > +
> > +/* Command 11h - CAN Receive */
> > +#define NCT6694_CAN_RECEIVE          0x11
> > +#define NCT6694_CAN_RECEIVE_SEL(idx, buf_cnt)        \
> > +     ((idx ? 0x80 : 0x00) | ((buf_cnt) & 0x7F))
> > +
> > +#define NCT6694_CAN_FRAME_TAG_CAN0   0xC0
> > +#define NCT6694_CAN_FRAME_TAG_CAN1   0xC1
>
> in "include/linux/mfd/nct6694.h" it's CAN1 and CAN2, can you make it "0"
> based everywhere?
>

Understood. I will update these macros in the header.

> what about making it:
>
> #define NCT6694_CAN_FRAME_TAG_CAN(idx)  (0xC0 | (idx))
>

Fix it in the v8.

> > +#define NCT6694_CAN_FRAME_FLAG_EFF   BIT(0)
> > +#define NCT6694_CAN_FRAME_FLAG_RTR   BIT(1)
> > +#define NCT6694_CAN_FRAME_FLAG_FD    BIT(2)
> > +#define NCT6694_CAN_FRAME_FLAG_BRS   BIT(3)
> > +#define NCT6694_CAN_FRAME_FLAG_ERR   BIT(4)
> > +
> > +#define NCT6694_NAPI_WEIGHT          32
> > +
...
> > +struct nct6694_can_priv {
> > +     struct can_priv can;    /* must be the first member */
> > +     struct can_rx_offload offload;
> > +     struct net_device *ndev;
> > +     struct nct6694 *nct6694;
> > +     struct workqueue_struct *wq;
> > +     struct work_struct tx_work;
> > +     struct nct6694_can_frame tx;
> > +     struct nct6694_can_frame rx;
> > +     struct nct6694_can_event event[2];
> > +     struct can_berr_counter bec;
> > +     unsigned char can_idx;
>
> can you use ndev->dev_port instead?
>

Fix it in the v8.

> > +};
> > +
...
> > +static void nct6694_can_rx(struct net_device *ndev, u8 rx_evt)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct nct6694_can_frame *frame = &priv->rx;
> > +     struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_RECEIVE,
> > +             .sel = NCT6694_CAN_RECEIVE_SEL(priv->can_idx, 1),
> > +             .len = cpu_to_le16(sizeof(*frame))
> > +     };
> > +     struct sk_buff *skb;
> > +     int ret;
> > +
> > +     ret = nct6694_read_msg(priv->nct6694, &cmd_hd, frame);
> > +     if (ret)
> > +             return;
> > +
> > +     if (frame->flag & NCT6694_CAN_FRAME_FLAG_FD) {
> > +             struct canfd_frame *cfd;
> > +
> > +             skb = alloc_canfd_skb(priv->ndev, &cfd);
> > +             if (!skb)
> > +                     return;
> > +
> > +             cfd->can_id = le32_to_cpu(frame->id);
> > +             cfd->len = canfd_sanitize_len(frame->length);
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF)
> > +                     cfd->can_id |= CAN_EFF_FLAG;
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_BRS)
> > +                     cfd->flags |= CANFD_BRS;
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_ERR)
> > +                     cfd->flags |= CANFD_ESI;
> > +
> > +             memcpy(cfd->data, frame->data, cfd->len);
> > +     } else {
> > +             struct can_frame *cf;
> > +
> > +             skb = alloc_can_skb(priv->ndev, &cf);
> > +             if (!skb)
> > +                     return;
> > +
> > +             cf->can_id = le32_to_cpu(frame->id);
> > +             cf->len = min_t(u8, frame->length, CAN_MAX_DLEN);
>
> use can_cc_dlc2len()
>

Fix it in the v8.

> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_EFF)
> > +                     cf->can_id |= CAN_EFF_FLAG;
> > +
> > +             if (frame->flag & NCT6694_CAN_FRAME_FLAG_RTR)
> > +                     cf->can_id |= CAN_RTR_FLAG;
> > +             else
> > +                     memcpy(cf->data, frame->data, cf->len);
> > +     }
> > +
> > +     nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static int nct6694_can_get_berr_counter(const struct net_device *ndev,
> > +                                     struct can_berr_counter *bec)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > +     *bec = priv->bec;
> > +
> > +     return 0;
> > +}
> > +
> > +static void nct6694_can_handle_state_change(struct net_device *ndev,
> > +                                         enum can_state new_state)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct can_berr_counter bec;
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +
> > +     skb = alloc_can_err_skb(ndev, &cf);
> > +
> > +     nct6694_can_get_berr_counter(ndev, &bec);
> > +
> > +     switch (new_state) {
> > +     case CAN_STATE_ERROR_ACTIVE:
> > +             priv->can.can_stats.error_warning++;
>                                     ^^^^^^^^^^^^^
> > +             priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +             if (cf)
> > +                     cf->data[1] |= CAN_ERR_CRTL_ACTIVE;
> > +             break;
> > +     case CAN_STATE_ERROR_WARNING:
> > +             priv->can.can_stats.error_warning++;
> > +             priv->can.state = CAN_STATE_ERROR_WARNING;
> > +             if (cf) {
> > +                     cf->can_id |= CAN_ERR_CRTL;
> > +                     if (bec.txerr > bec.rxerr)
> > +                             cf->data[1] = CAN_ERR_CRTL_TX_WARNING;
> > +                     else
> > +                             cf->data[1] = CAN_ERR_CRTL_RX_WARNING;
> > +                     cf->data[6] = bec.txerr;
> > +                     cf->data[7] = bec.rxerr;
> > +             }
> > +             break;
> > +     case CAN_STATE_ERROR_PASSIVE:
> > +             priv->can.can_stats.error_passive++;
> > +             priv->can.state = CAN_STATE_ERROR_PASSIVE;
> > +             if (cf) {
> > +                     cf->can_id |= CAN_ERR_CRTL;
> > +                     cf->data[1] |= CAN_ERR_CRTL_RX_PASSIVE;
> > +                     if (bec.txerr >= CAN_ERROR_PASSIVE_THRESHOLD)
> > +                             cf->data[1] |= CAN_ERR_CRTL_TX_PASSIVE;
> > +                     cf->data[6] = bec.txerr;
> > +                     cf->data[7] = bec.rxerr;
> > +             }
> > +             break;
> > +     case CAN_STATE_BUS_OFF:
> > +             priv->can.state = CAN_STATE_BUS_OFF;
> > +             priv->can.can_stats.bus_off++;
> > +             if (cf)
> > +                     cf->can_id |= CAN_ERR_BUSOFF;
> > +             can_free_echo_skb(ndev, 0, NULL);
> > +             netif_stop_queue(ndev);
> > +             can_bus_off(ndev);
> > +             break;
> > +     default:
> > +             break;
> > +     }
> > +
> > +     nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_can_handle_state_errors(struct net_device *ndev, u8 status)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
>
> It seems you don't have dedicated RX and TX states, so call
> nct6694_can_get_berr_counter() and use can_state_get_by_berr_counter()
> to get the states. Then basically do that what what
> mcp251xfd_handle_cerrif() does, starting with "new_state = max(tx_state, rx_state);"
>

Excuse me, do you mean that nct6694_can_handle_state_change() the flow
should like v5?
https://lore.kernel.org/linux-can/CAMZ6RqLHEoukxDfV33iDWXjM1baK922QnWSkOP01VzZ0S_9H8g@mail.gmail.com/

> > +
> > +     if (status == NCT6694_CAN_EVT_STS_ERROR_ACTIVE &&
> > +         priv->can.state != CAN_STATE_ERROR_ACTIVE) {
> > +             netdev_dbg(ndev, "Error, entered active state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_ACTIVE);
> > +     }
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_WARNING &&
> > +         priv->can.state != CAN_STATE_ERROR_WARNING) {
> > +             netdev_dbg(ndev, "Error, entered warning state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_WARNING);
> > +     }
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_ERROR_PASSIVE &&
> > +         priv->can.state != CAN_STATE_ERROR_PASSIVE) {
> > +             netdev_dbg(ndev, "Error, entered passive state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_ERROR_PASSIVE);
> > +     }
> > +
> > +     if (status == NCT6694_CAN_EVT_STS_BUS_OFF &&
> > +         priv->can.state != CAN_STATE_BUS_OFF) {
> > +             netdev_dbg(ndev, "Error, entered bus-off state\n");
> > +             nct6694_can_handle_state_change(ndev, CAN_STATE_BUS_OFF);
> > +     }
> > +}
> > +
> > +static void nct6694_can_handle_bus_err(struct net_device *ndev, u8 bus_err)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct can_frame *cf;
> > +     struct sk_buff *skb;
> > +
> > +     if (bus_err == NCT6694_CAN_EVT_ERR_NO_ERROR)
> > +             return;
> > +
> > +     priv->can.can_stats.bus_error++;
> > +
> > +     skb = alloc_can_err_skb(ndev, &cf);
> > +     if (skb)
> > +             cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
> > +
> > +     switch (bus_err) {
> > +     case NCT6694_CAN_EVT_ERR_CRC_ERROR:
> > +             netdev_dbg(ndev, "CRC error\n");
> > +             ndev->stats.rx_errors++;
> > +             if (skb)
> > +                     cf->data[3] |= CAN_ERR_PROT_LOC_CRC_SEQ;
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_STUFF_ERROR:
> > +             netdev_dbg(ndev, "Stuff error\n");
> > +             ndev->stats.rx_errors++;
> > +             if (skb)
> > +                     cf->data[2] |= CAN_ERR_PROT_STUFF;
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_ACK_ERROR:
> > +             netdev_dbg(ndev, "Ack error\n");
> > +             ndev->stats.tx_errors++;
> > +             if (skb) {
> > +                     cf->can_id |= CAN_ERR_ACK;
> > +                     cf->data[2] |= CAN_ERR_PROT_TX;
> > +             }
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_FORM_ERROR:
> > +             netdev_dbg(ndev, "Form error\n");
> > +             ndev->stats.rx_errors++;
> > +             if (skb)
> > +                     cf->data[2] |= CAN_ERR_PROT_FORM;
> > +             break;
> > +
> > +     case NCT6694_CAN_EVT_ERR_BIT_ERROR:
> > +             netdev_dbg(ndev, "Bit error\n");
> > +             ndev->stats.tx_errors++;
> > +             if (skb)
> > +                     cf->data[2] |= CAN_ERR_PROT_TX | CAN_ERR_PROT_BIT;
> > +             break;
> > +
> > +     default:
> > +             break;
> > +     }
> > +
>
> missing check:
>
>         if (skb)
>

Fix it in the v8.

> > +     nct6694_can_rx_offload(&priv->offload, skb);
> > +}
> > +
> > +static void nct6694_can_tx_irq(struct net_device *ndev)
> > +{
> > +     struct net_device_stats *stats = &ndev->stats;
> > +
> > +     stats->tx_bytes += can_get_echo_skb(ndev, 0, NULL);
>
> use can_rx_offload_get_echo_skb_queue_tail() here
>

Fix it in the v8.

> > +     stats->tx_packets++;
> > +     netif_wake_queue(ndev);
> > +}
> > +
> > +static irqreturn_t nct6694_can_irq(int irq, void *data)
> > +{
> > +     struct net_device *ndev = data;
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct nct6694_can_event *evt = priv->event;
> > +     struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_EVENT,
> > +             .sel = NCT6694_CAN_EVENT_SEL(priv->can_idx, NCT6694_CAN_EVENT_MASK),
> > +             .len = cpu_to_le16(sizeof(priv->event))
> > +     };
> > +     irqreturn_t handled = IRQ_NONE;
> > +     int can_idx = priv->can_idx;
> > +     int ret;
>
> it would make sense to have a event pointer here instead of the can_idx?
>
>         const struct nct6694_can_event *event = &priv->event[priv->can_idx];
>

Fix it in the v8.

> > +
> > +     ret = nct6694_read_msg(priv->nct6694, &cmd_hd, evt);
> > +     if (ret < 0)
> > +             return handled;
> > +
> > +     if (priv->event[can_idx].rx_evt & NCT6694_CAN_EVT_RX_DATA_IN) {
> > +             nct6694_can_rx(ndev, priv->event[can_idx].rx_evt);
> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (priv->event[can_idx].rx_evt & NCT6694_CAN_EVT_RX_DATA_LOST) {
> > +             nct6694_can_handle_lost_msg(ndev);
> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (priv->event[can_idx].status) {
> > +             nct6694_can_handle_state_errors(ndev, priv->event[can_idx].status);
> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_BERR_REPORTING) {
> > +             nct6694_can_handle_bus_err(ndev, priv->event[can_idx].err);
> > +             handled = IRQ_HANDLED;
> > +     }
> > +
> > +     if (handled)
> > +             can_rx_offload_threaded_irq_finish(&priv->offload);
> > +
> > +     if (priv->event[can_idx].tx_evt & NCT6694_CAN_EVT_TX_FIFO_EMPTY)
> > +             nct6694_can_tx_irq(ndev);
>
> Move this in front of the handled check and add "handled = IRQ_HANDLED;"
> to the if-block.
>

Fix it in the v8.

> > +
> > +     priv->bec.rxerr = priv->event[can_idx].rec;
> > +     priv->bec.txerr = priv->event[can_idx].tec;
> > +
> > +     return handled;
> > +}
> > +
> > +static void nct6694_can_tx_work(struct work_struct *work)
> > +{
> > +     struct nct6694_can_priv *priv = container_of(work,
> > +                                                  struct nct6694_can_priv,
> > +                                                  tx_work);
> > +     struct nct6694_can_frame *frame = &priv->tx;
> > +     struct net_device *ndev = priv->ndev;
> > +     struct net_device_stats *stats = &ndev->stats;
> > +     struct sk_buff *skb = priv->can.echo_skb[0];
> > +     static const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_DELIVER,
> > +             .sel = NCT6694_CAN_DELIVER_SEL(1),
> > +             .len = cpu_to_le16(sizeof(*frame))
> > +     };
> > +     u32 txid;
> > +     int err;
> > +
> > +     memset(frame, 0, sizeof(*frame));
> > +
> > +     if (priv->can_idx == 0)
> > +             frame->tag = NCT6694_CAN_FRAME_TAG_CAN0;
> > +     else
> > +             frame->tag = NCT6694_CAN_FRAME_TAG_CAN1;
>
>         frame->tag = NCT6694_CAN_FRAME_TAG_CAN(priv->can_idx);
>

Fix it in the v8.

> > +
> > +     if (can_is_canfd_skb(skb)) {
> > +             struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> > +
> > +             if (cfd->flags & CANFD_BRS)
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_BRS;
> > +
> > +             if (cfd->can_id & CAN_EFF_FLAG) {
> > +                     txid = cfd->can_id & CAN_EFF_MASK;
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF;
> > +             } else {
> > +                     txid = cfd->can_id & CAN_SFF_MASK;
> > +             }
> > +             frame->flag |= NCT6694_CAN_FRAME_FLAG_FD;
> > +             frame->id = cpu_to_le32(txid);
> > +             frame->length = cfd->len;
> > +
> > +             memcpy(frame->data, cfd->data, cfd->len);
> > +     } else {
> > +             struct can_frame *cf = (struct can_frame *)skb->data;
> > +
> > +             if (cf->can_id & CAN_EFF_FLAG) {
> > +                     txid = cf->can_id & CAN_EFF_MASK;
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_EFF;
> > +             } else {
> > +                     txid = cf->can_id & CAN_SFF_MASK;
> > +             }
> > +
> > +             if (cf->can_id & CAN_RTR_FLAG)
> > +                     frame->flag |= NCT6694_CAN_FRAME_FLAG_RTR;
> > +             else
> > +                     memcpy(frame->data, cf->data, cf->len);
> > +
> > +             frame->id = cpu_to_le32(txid);
> > +             frame->length = cf->len;
> > +     }
> > +
> > +     err = nct6694_write_msg(priv->nct6694, &cmd_hd, frame);
> > +     if (err) {
> > +             netdev_err(ndev, "%s: TX FIFO is full!\n", __func__);
> > +             can_free_echo_skb(ndev, 0, NULL);
> > +             stats->tx_dropped++;
> > +             stats->tx_errors++;
> > +             netif_wake_queue(ndev);
> > +     }
> > +}
> > +
> > +static netdev_tx_t nct6694_can_start_xmit(struct sk_buff *skb,
> > +                                       struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
> > +     if (can_dev_dropped_skb(ndev, skb))
> > +             return NETDEV_TX_OK;
> > +
> > +     netif_stop_queue(ndev);
> > +     can_put_echo_skb(skb, ndev, 0, 0);
> > +     queue_work(priv->wq, &priv->tx_work);
> > +
> > +     return NETDEV_TX_OK;
> > +}
> > +
> > +static int nct6694_can_start(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct nct6694_can_setting *setting;
> > +     struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_SETTING,
> > +             .sel = priv->can_idx,
> > +             .len = cpu_to_le16(sizeof(*setting))
> > +     };
> > +     const struct can_bittiming *n_bt = &priv->can.bittiming;
> > +     const struct can_bittiming *d_bt = &priv->can.data_bittiming;
>
> nitpick:
> move these 2 right after priv and d_bt first.
>

Fix it in the v8.

> > +     int ret;
> > +
> > +     setting = kzalloc(sizeof(*setting), GFP_KERNEL);
> > +     if (!setting)
> > +             return -ENOMEM;
> > +
> > +     setting->nbr = cpu_to_le32(n_bt->bitrate);
> > +     setting->dbr = cpu_to_le32(d_bt->bitrate);
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> > +             setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_MON);
> > +
> > +     if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
> > +         priv->can.ctrlmode & CAN_CTRLMODE_FD_NON_ISO)
>
> We should move this check into the infrastructure. Please only check for
> CAN_CTRLMODE_FD_NON_ISO.
>

Fix it in the v8.

> > +             setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_NISO);
> > +
> > +     if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)
> > +             setting->ctrl1 |= cpu_to_le16(NCT6694_CAN_SETTING_CTRL1_LBCK);
> > +
> > +     ret = nct6694_write_msg(priv->nct6694, &cmd_hd, setting);
> > +     if (ret)
> > +             goto exit;
> > +
> > +     priv->can.state = CAN_STATE_ERROR_ACTIVE;
> > +
> > +exit:
> > +     kfree(setting);
> > +     return ret;
> > +}
> > +
> > +static int nct6694_can_stop(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +
>
> How does the device know, that it should leave the CAN bus? Is there a
> message you can send to it? If not put it at least into listen only mode.
>

My device only resets upon receiving the initial command, so can I
assign priv->can.ctrlmode = CAN_CTRLMODE_LISTENONLY directly?

> > +     netif_stop_queue(ndev);
> > +     free_irq(ndev->irq, ndev);
> > +     destroy_workqueue(priv->wq);
> > +     priv->wq = NULL;
>
> no need to set wq to NULL
>

Fix it in the v8.

> > +     priv->can.state = CAN_STATE_STOPPED;
> > +     can_rx_offload_disable(&priv->offload);
>
> Please change move can_rx_offload_disable() in front of "priv->can.state
> = CAN_STATE_STOPPED".
>

Fix it in the v8.

> > +     close_candev(ndev);
> > +
> > +     return 0;
> > +}
> > +
> > +static int nct6694_can_set_mode(struct net_device *ndev, enum can_mode mode)
> > +{
> > +     switch (mode) {
> > +     case CAN_MODE_START:
> > +             nct6694_can_start(ndev);
>
> please add error checking
>

Fix it in the v8.

> > +             netif_wake_queue(ndev);
> > +             return 0;
> > +     default:
> > +             return -EOPNOTSUPP;
> > +     }
> > +}
> > +
...
> > +static int nct6694_can_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_can_priv *priv;
> > +     struct net_device *ndev;
> > +     int ret, irq, can_clk;
> > +
> > +     irq = irq_create_mapping(nct6694->domain,
> > +                              NCT6694_IRQ_CAN1 + cell->id);
> > +     if (!irq)
> > +             return irq;
> > +
> > +     ndev = alloc_candev(sizeof(struct nct6694_can_priv), 1);
> > +     if (!ndev)
> > +             return -ENOMEM;
> > +
> > +     ndev->irq = irq;
> > +     ndev->flags |= IFF_ECHO;
> > +     ndev->dev_port = cell->id;
> > +     ndev->netdev_ops = &nct6694_can_netdev_ops;
> > +     ndev->ethtool_ops = &nct6694_can_ethtool_ops;
> > +
> > +     priv = netdev_priv(ndev);
> > +     priv->nct6694 = nct6694;
> > +     priv->ndev = ndev;
> > +
> > +     can_clk = nct6694_can_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_can_tx_work);
> > +
> > +     priv->can_idx = cell->id;
> > +     priv->can.state = CAN_STATE_STOPPED;
>
> The device is allocated with CAN_STATE_STOPPED set, can be removed.
>

Fix it in the v8.

> > +     priv->can.clock.freq = can_clk;
> > +     priv->can.bittiming_const = &nct6694_can_bittiming_nominal_const;
> > +     priv->can.data_bittiming_const = &nct6694_can_bittiming_data_const;
> > +     priv->can.do_set_mode = nct6694_can_set_mode;
> > +     priv->can.do_get_berr_counter = nct6694_can_get_berr_counter;
> > +
> > +     priv->can.ctrlmode = CAN_CTRLMODE_FD;
> > +
> > +     priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK            |
> > +                                    CAN_CTRLMODE_LISTENONLY          |
> > +                                    CAN_CTRLMODE_FD                  |
> > +                                    CAN_CTRLMODE_FD_NON_ISO          |
> > +                                    CAN_CTRLMODE_BERR_REPORTING;
>
> nitpick: one space in front of "|" please
>
> Does your device run in CAN-FD mode all the time? If so, please use
> can_set_static_ctrlmode() to set it after priv->can.ctrlmode_supported
> and remove CAN_CTRLMODE_FD from ctrlmode_supported.
>

Fix it in the v8.

> > +
> > +     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 del_rx_offload;
> > +
> > +     return 0;
> > +
> > +del_rx_offload:
>
> nitpick: please rename to rx_offload_del or can_rx_offload_del
>

Fix it in the v8.

> > +     can_rx_offload_del(&priv->offload);
> > +free_candev:
> > +     free_candev(ndev);
> > +     return ret;
> > +}
> > +
> > +static void nct6694_can_remove(struct platform_device *pdev)
> > +{
> > +     struct nct6694_can_priv *priv = platform_get_drvdata(pdev);
> > +
> > +     cancel_work_sync(&priv->tx_work);
>
> The workqueue has already been destroyed in nct6694_can_stop(), so more
> work should be pending.
>

Fix it in the v8.

> > +     unregister_candev(priv->ndev);
> > +     can_rx_offload_del(&priv->offload);
> > +     free_candev(priv->ndev);
> > +}
> > +


Best regards,
Ming

Dear Christophe,

Thank you for reviewing,
I will make the change in the next patch.


Best regards,
Ming

Christophe JAILLET <christophe.jaillet@wanadoo.fr> 於 2025年2月8日 週六 上午3:18寫道：
>
> Le 07/02/2025 à 08:44, Ming Yu a écrit :
> > This driver supports Socket CANFD functionality for NCT6694 MFD
> > device based on USB interface.
>
> ...
>
> > +static int nct6694_can_start(struct net_device *ndev)
> > +{
> > +     struct nct6694_can_priv *priv = netdev_priv(ndev);
> > +     struct nct6694_can_setting *setting;
>
> Could be:
> struct nct6694_can_setting *setting __free(kfree) = NULL;
>
> to slightly simplify code below.
>
>
> > +     struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_SETTING,
> > +             .sel = priv->can_idx,
> > +             .len = cpu_to_le16(sizeof(*setting))
> > +     };
>
> ...
>
> > +static int nct6694_can_get_clock(struct nct6694_can_priv *priv)
> > +{
> > +     struct nct6694_can_information *info;
>
> Could be:
> struct nct6694_can_information *info __free(kfree) = NULL;
>
> to slightly simplify code below.
>
> > +     static const struct nct6694_cmd_header cmd_hd = {
> > +             .mod = NCT6694_CAN_MOD,
> > +             .cmd = NCT6694_CAN_INFORMATION,
> > +             .sel = NCT6694_CAN_INFORMATION_SEL,
> > +             .len = cpu_to_le16(sizeof(*info))
> > +     };
> > +     int ret, can_clk;
> > +
> > +     info = kzalloc(sizeof(*info), GFP_KERNEL);
> > +     if (!info)
> > +             return -ENOMEM;
>
> ...
>
> CJ
>