[PATCH v2 3/4] can: Add driver for CAST CAN Bus Controller

[Vincent MAILHOL]

On Tue. 15 Oct. 2024 at 18:33, Hal Feng <hal.feng at linux.starfivetech.com> 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 at starfivetech.com> wrote:
> >> From: William Qiu <william.qiu at starfivetech.com>
> >>
> >> Add driver for CAST CAN Bus Controller used on
> >> StarFive JH7110 SoC.
> >>
> >> Signed-off-by: William Qiu <william.qiu at starfivetech.com>
> >> Co-developed-by: Hal Feng <hal.feng at starfivetech.com>
> >> Signed-off-by: Hal Feng <hal.feng at starfivetech.com>
> >> ---

(...)

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