Dear Vincent,
Thank you for your comments,
Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> 於 2024年11月21日 週四 下午3:47寫道：
> > +
> > +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.
> > + 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.
> > +struct __packed nct6694_can_cmd1 {
...
> > +
> > +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.
> > + struct can_priv can; /* must be the first member */
...
> > + } 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.
> > + }
> > +
> > + /* 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.
> > + netif_receive_skb(skb);
...
> > +
> > + 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.
> > + cf->can_id |= CAN_ERR_CRTL;
> > + cf->data[1] = (bec.txerr > bec.rxerr) ? CAN_ERR_CRTL_TX_WARNING :
> > + CAN_ERR_CRTL_RX_WARNING;
...
> > +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?
> > + 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.
> > + buf->nbr = n_bt->bitrate;
> > + buf->dbr = d_bt->bitrate;
...
> > +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.
> > + 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;
...
> > +
> > + /* 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.
> > +}
> > +
...
> > +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
