Re: [PATCH v2 1/2] can: Add Loongson CAN-FD controller driver

[Vincent Mailhol <mailhol@kernel.org>]

On 18/06/2026 at 11:49, Binbin Zhou wrote:
> Hi Vincent:
> 
> Thanks for your detailed review and sorry for the late reply.
> 
> On Tue, Jun 9, 2026 at 1:43 AM Vincent Mailhol <mailhol@kernel.org> wrote:
>>
>> On 08/06/2026 at 10:49, Binbin Zhou wrote:

(...)

>>> +static void loongson_canfd_set_mode(struct loongson_canfd_priv *priv,
>>> +                                 const struct can_ctrlmode *ctrlmode)
>>
>> Rename this to loongson_canfd_set_ctrlmode(). Otherwhise there is a
>> risk of confusion with loongson_canfd_do_set_mode()
> 
> Actually, this function sets the `LOONGSON_CANFD_MODE` and
> `LOONGSON_CANFD_CONF` registers based on `priv->can.ctrlmode`.
> 
> How about rewriting it as follows:
> 
> static void loongson_canfd_set_confmode(struct loongson_canfd_priv *priv)
> {
>         u32 ctrlmode = priv->can.ctrlmode;
> .....
> }

That also works. My point was to avoid the confusion with
loongson_canfd_do_set_mode(). As long as there is enough disambiguation,
it is OK for me.

(...)

>>> +     /* Write Data payload */
>>> +     if (!(cf->can_id & CAN_RTR_FLAG)) {
>>> +             for (unsigned int i = 0; i < cf->len; i += LOONGSON_CANFD_DW_BYTE) {
>>> +                     regmap_write(priv->regmap,
>>> +                                  LOONGSON_CANFD_TX_DATA_1 + LOONGSON_CANFD_FRAME_DB_1 + i,
>>> +                                  le32_to_cpu(*(__le32 *)(cf->data + i)));
>>                                                   ^^^^^^^^^^
>> This cast is just wrong. What if the code is run on a big endian
>> machine?
>>
>> To begin with, here, you are writing to your registers. So it should
>> be the other way around: cpu_to_le32(). And to add to it, if you need
>> endian conversion, populate regmap_config.val_format_endian.
>>
>> Also, why do you need some endian conversion only here and not on the
>> other registers?
> 
> Sorry, this is the old test code—I forgot to update it. Just use `u32`:
> 
>  for (i = 0; i < len; i += LOONGSON_CANFD_DW_BYTE)
>                 regmap_read(priv->regmap, LOONGSON_CANFD_RX_DATA, (u32
> *)(cf->data + i));

OK. Note that I don't mind if you want to keep the declaration of i in
the for loop:

  for (unsigned int i = 0; i < len; i += LOONGSON_CANFD_DW_BYTE)
  	/* ... */

>>> +             }
>>> +     }
>>> +
>>> +     return true;
>>> +}

(...)

>>> +static irqreturn_t loongson_canfd_interrupt(int irq, void *dev_id)
>>> +{
>>> +     struct net_device *ndev = (struct net_device *)dev_id;
>>> +     struct loongson_canfd_priv *priv = netdev_priv(ndev);
>>> +     u32 isr, imask;
>>> +
>>> +     for (unsigned int irq_loops = 0; irq_loops < 10000; irq_loops++) {
>>> +             /* Get the interrupt status */
>>> +             regmap_read(priv->regmap, LOONGSON_CANFD_INT_STAT, &isr);
>>> +             if (!isr)
>>> +                     return irq_loops ? IRQ_HANDLED : IRQ_NONE;
>>
>>                         if (irq_loops)
>>                                 return IRQ_HANDLED;
>>                         else
>>                                 return IRQ_NONE;
>>
> Emm...
> Is the ternary operator here more concise than `if-else`?
> 
>                 if (!isr) {
>                         if (irq_loops)
>                                 return IRQ_HANDLED;
>                         else
>                                 return IRQ_NONE;
>                 }

Unless you are getting an actual checkpatch.pl warning here, if I record
correctly, the curly brackets can be ommitted here:

		if (!isr)
			if (irq_loops)
				return IRQ_HANDLED;
			else
				return IRQ_NONE;

> vs
> 
>                if (!isr)
>                        return irq_loops ? IRQ_HANDLED : IRQ_NONE;

Yes, it is more consise, but is it more readable?

Actually, you might say that which one is the most readable is a
subjective topic and you would be right.

But it is the prefered style here. The indentation of the if/else makes
it more visual. And when you review thousands of line of code, it starts
to make a difference.

And I this is not my personnal opinion. For example, see this message
from Greg:

  https://lore.kernel.org/all/20250311150130.7a875e63@bahia/

So, even if this is an arguable case, let this be an if/else.

(...)

>>> +static const struct net_device_ops loongson_canfd_netdev_ops = {
>>> +     .ndo_open       = loongson_canfd_open,
>>> +     .ndo_stop       = loongson_canfd_close,
>>> +     .ndo_start_xmit = loongson_canfd_start_xmit,
>>> +};
>>
>> Also add a struct ethtool_ops and populate
>> ethtool_ops.get_ts_info. Something like:
>>
>>         static const struct ethtool_ops es58x_ethtool_ops = {
>>                 .get_ts_info = can_ethtool_op_get_ts_info_hwts,
>>         };
>>
>> Refer to what the other CAN drivers with hardware timestamps are
>> doing.
> 
> Regarding the hardware timestamp, could this be submitted as a
> separate feature patch? I need some time to resolve the related
> issues.
> Could we proceed as follows for now:
> 
> static const struct ethtool_ops loongson_canfd_ethtool_ops = {
>         .get_ts_info = ethtool_op_get_ts_info,
> };

OK, but make sure to remove all your local hacks. We will not accept the
use of the reserved fields in sturct canfd_frame for that.

Note that once you undertsand the timestamp framework, I would expect
the patch to be rather tiny.



Yours sincerely,
Vincent Mailhol