On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng@xxxxxxxxxxxxxxxxxxxxxx> wrote:
> On 9/23/2024 11:41 AM, Vincent MAILHOL wrote:
> > Hi Hal,
> >
> > A few more comments on top of what Andrew already wrote.
> >
> > On Mon. 23 Sep. 2024 at 00:09, Hal Feng <hal.feng@xxxxxxxxxxxxxxxx> wrote:
> >> From: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> >>
> >> Add driver for CAST CAN Bus Controller used on
> >> StarFive JH7110 SoC.
> >>
> >> Signed-off-by: William Qiu <william.qiu@xxxxxxxxxxxxxxxx>
> >> Co-developed-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> Signed-off-by: Hal Feng <hal.feng@xxxxxxxxxxxxxxxx>
> >> ---
(...)
> >> + stats->rx_packets++;
> >> + netif_receive_skb(skb);
> >> +
> >> + return 1;
> >
> > Why return 1 on success and 0 on failure? The convention in the kernel
> > is that 0 means success. If you really want to keep 0 for failure, at
> > least make this return boolean true or boolean false, but overall, try
> > to follow the return conventions.
>
> The return value here represents the number of successfully received packets.
> It is used in ccan_rx_poll() for counting the number of successfully
> received packets.
Ack. I guess this will become more clear after you implement the queue logic.
(...)
> >> +
> >> + if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> >> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_FD;
> >> + priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> >> + } else {
> >> + priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> >> + }
> >
> > Nitpick, consider doing this:
> >
> > priv->can.ctrlmode_supported = CAN_CTRLMODE_LOOPBACK;
> > if (priv->cantype == CAST_CAN_TYPE_CANFD) {
> > priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
> > priv->can.data_bittiming_const = &ccan_data_bittiming_const_canfd;
> > }
>
> OK.
>
> >
> > Also, does you hardware support dlc greater than 8 (c.f.
> > CAN_CTRLMODE_CC_LEN8_DLC)?
>
> The class CAN (CC) mode does not support, but the CAN FD mode supports.
So, CAN_CTRLMODE_CC_LEN8_DLC is a Classical CAN feature. Strictly
speaking, this does not exist in CAN FD. Do you mean that only the
CAST_CAN_TYPE_CANFD supports sending Classical CAN frames with a DLC
greater than 8?
If none of the Classical CAN or CAN FD variants of your device is able
to send Classical CAN frames with a DLC greater than 8, then this is
just not supported by your device.
Could you share the datasheet so that I can double check this?
(...)
> Sorry for the late reply. Thank you for your detailed review.
No problem, take your time!
Yours sincerely,
Vincent Mailhol
