Dear Vincent,

Thank you for your comments,

Vincent Mailhol [off-list ref] 於 2024年11月21日 週四 下午3:47寫道：

quoted

+
+struct __packed nct6694_can_cmd0 {

Give more meaningfull names to your structures. For example:

  /* cmd1 */
  struct __packed nct6694_can_bittiming

Understood. I will make the modifications in v3.

quoted

+     u32 nbr;
+     u32 dbr;
+     u32 active:8;
+     u32 reserved:24;
+     u16 ctrl1;
+     u16 ctrl2;
+     u32 nbtp;
+     u32 dbtp;
+};

Always use the specific endianess types in the structures that you are
sending to the device. e.g. replace u32 by __le32 (assuming little endian).

Okay, I'll fix it in v3.

quoted

+struct __packed nct6694_can_cmd1 {

...

quoted

+
+struct nct6694_canfd_priv {

Be consistent in your name space. Sometime you prefix your names with
nct6694_can and sometimes with nct6694_canfd for no apparent reasons.

Understood. I will make the modifications in v3.

quoted

+     struct can_priv can;    /* must be the first member */

...

quoted

+     } else {
+             if (buf->flag & NCT6694_CAN_FLAG_BRS)
+                     cf->flags |= CANFD_BRS;
+
+             for (i = 0; i < cf->len; i++)
+                     cf->data[i] = buf->data[i];

Use memcpy().

Okay, I'll fix it in v3.

quoted

+     }
+
+     /* Remove the packet from FIFO */
+     stats->rx_packets++;
+     stats->rx_bytes += cf->len;

Do not increment the rx_bytes if the frame is RTR.

Okay, I'll fix it in v3.

quoted

+     netif_receive_skb(skb);

...

quoted

+
+     switch (new_state) {
+     case CAN_STATE_ERROR_WARNING:
+             /* error warning state */

Such comment can be removed. Here you are just paraphrasing the macro. I
can already see that CAN_STATE_ERROR_WARNING means the "error warning
state". The comments should add information.

Okay, I will drop them in v3.

quoted

+             cf->can_id |= CAN_ERR_CRTL;
+             cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
+                                                     CAN_ERR_CRTL_RX_WARNING;

...

quoted

+static int nct6694_canfd_start(struct net_device *ndev)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+     struct nct6694_can_cmd0 *buf = (struct nct6694_can_cmd0 *)priv->tx_buf;

Give a more memorable name to buf, for example: bittiming_cmd.

Got it. So, every buf that uses a command must be renamed similarly, right?

quoted

+     const struct can_bittiming *n_bt = &priv->can.bittiming;
+     const struct can_bittiming *d_bt = &priv->can.data_bittiming;
+     int ret;
+
+     guard(mutex)(&priv->lock);
+
+     memset(priv->tx_buf, 0, NCT6694_CAN_CMD0_LEN);

Remove those CMD*_LEN macros, instead, use sizeof() of your structures.

  memset(buf, 0, sizeof(*buf));

Understood. I will make the modifications in v3.

quoted

+     buf->nbr = n_bt->bitrate;
+     buf->dbr = d_bt->bitrate;

...

quoted

+static netdev_tx_t nct6694_canfd_start_xmit(struct sk_buff *skb,
+                                         struct net_device *ndev)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+
+     if (priv->tx_skb || priv->tx_busy) {
+             netdev_err(ndev, "hard_xmit called while tx busy\n");
+             return NETDEV_TX_BUSY;
+     }
+
+     if (can_dev_dropped_skb(ndev, skb))
+             return NETDEV_TX_OK;
+
+     netif_stop_queue(ndev);

Here, you are inconditionally stopping the queue. Does it mean that your
device is only able to manage one CAN frame at the time?

Yes, we designed it to manage a single CAN frame, so we stop the queue
here until a TX event is received to wake queue.
But It seems I lost the error handling code for the tx command in
nct6694_canfd_tx(), I will fix it in the next patch.

quoted

+     priv->tx_skb = skb;
+     queue_work(priv->wq, &priv->tx_work);
+
+     return NETDEV_TX_OK;
+}
+
+static void nct6694_canfd_tx(struct net_device *ndev, struct canfd_frame *cf)
+{
+     struct nct6694_canfd_priv *priv = netdev_priv(ndev);
+     struct nct6694_can_cmd10_11 *buf = (struct nct6694_can_cmd10_11 *)priv->tx_buf;
+     u32 txid = 0;
+     int i;

...

quoted

+
+     /* set data to buf */
+     for (i = 0; i < cf->len; i++)
+             buf->data[i] = cf->data[i];
+
+     nct6694_write_msg(priv->nct6694, NCT6694_CAN_MOD,
+                       NCT6694_CAN_CMD10_OFFSET(1),
+                       NCT6694_CAN_CMD10_LEN,
+                       buf);

I will add the error handling to wake the queue in the next patch.

quoted

+}
+

...

quoted

+static const struct net_device_ops nct6694_canfd_netdev_ops = {
+     .ndo_open = nct6694_canfd_open,
+     .ndo_stop = nct6694_canfd_stop,
+     .ndo_start_xmit = nct6694_canfd_start_xmit,
+     .ndo_change_mtu = can_change_mtu,
+};

Also add a struct ethtool_ops for the default timestamps:

  static const struct ethtool_ops nct6694_ethtool_ops = {
          .get_ts_info = ethtool_op_get_ts_info,
  };

This assumes that your device does not support hardware timestamps. If
you do have hardware timestamping support, please adjust accordingly.

Understood. I will make the modifications in v3.

Best regards,
Ming