Message-ID: <CAMZ6Rq+WePVM2aSk2HzXoa-t+ZE97yeqS6bndpGXUh+NuiM8sg@mail.gmail.com>
Date: Fri, 22 Nov 2024 17:18:09 +0900
From: Vincent Mailhol <mailhol.vincent@...adoo.fr>
To: Ming Yu <a0282524688@...il.com>
Cc: tmyu0@...oton.com, lee@...nel.org, linus.walleij@...aro.org, brgl@...ev.pl, 
	andi.shyti@...nel.org, mkl@...gutronix.de, andrew+netdev@...n.ch, 
	davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, 
	wim@...ux-watchdog.org, linux@...ck-us.net, jdelvare@...e.com, 
	alexandre.belloni@...tlin.com, linux-kernel@...r.kernel.org, 
	linux-gpio@...r.kernel.org, linux-i2c@...r.kernel.org, 
	linux-can@...r.kernel.org, netdev@...r.kernel.org, 
	linux-watchdog@...r.kernel.org, linux-hwmon@...r.kernel.org, 
	linux-rtc@...r.kernel.org
Subject: Re: [PATCH v2 4/7] can: Add Nuvoton NCT6694 CAN support

On Fri. 22 Nov. 2024 at 17:05, Ming Yu <a0282524688@...il.com> wrote:
> Dear Vincent,
>
> Thank you for your comments,
>
> Vincent Mailhol <mailhol.vincent@...adoo.fr> 於 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?

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.

> > > +     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.

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.

> 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

Powered by blists - more mailing lists