Dear Vincent,

Vincent Mailhol [off-list ref] 於 2024年11月22日 週五 下午4:25寫道：

quoted

quoted

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?

Yes. Note that you can use different names if you have a better idea.
It is just that generic names like "buf" or "cmd1" do not tell me what
this variable actually is. On the contrary, bittiming_cmd tells me
that this variable holds the payload for the command to set the device
bittiming. This way, suddenly, the code becomes easier to read and
understand. As long as your naming conveys this kind of information,
then I am fine with whatever you choose, just avoid the "buf" or
"cmd1" names.

Understood. I will make the modifications in v3.

quoted

quoted

quoted

+     const struct can_bittiming *n_bt = &priv->can.bittiming;
+     const struct can_bittiming *d_bt = &priv->can.data_bittiming;

...

quoted

quoted

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.

Do you mean that the device is designed to manage only one single CAN
frame or is the driver designed to only manage one single CAN frame?
If the device is capable of handling several CAN frames, your driver
should take advantage of this. Else the driver will slow down the
communication a lot whenever packets start to accumulate in the TX
queue.

I understand, but our device currently supports handling only one CAN frame.

Best regards,
Ming