Re: [RFC PATCH 1/2] can: dummy_can: add CAN termination support

[Vincent Mailhol <mailhol@kernel.org>]

Hi Rakuram,

Thanks for the patch. My comments are only on the cosmetic aspect.

Le 27/11/2025 à 20:18, Rakuram Eswaran a écrit :
> Add support for configuring bus termination in the dummy_can driver.
> This allows users to emulate a properly terminated CAN bus when
> setting up virtual test environments.
> 
> Signed-off-by: Rakuram Eswaran <rakuram.e96@gmail.com>
> ---
> Tested the termination setting using below iproute commands:
> 
>   ip link set can0 type can termination 120
>   ip link set can0 type can termination off

When you test, do not forget to also try incorrect values ;)

  ip link set can0 type can termination 100
  ip link set can0 type can termination potato

(I think that the code is correct, just see this as a generic
comment).

>  drivers/net/can/dummy_can.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/net/can/dummy_can.c b/drivers/net/can/dummy_can.c
> index 41953655e3d3..2949173547e6 100644
> --- a/drivers/net/can/dummy_can.c
> +++ b/drivers/net/can/dummy_can.c
> @@ -23,6 +23,21 @@ struct dummy_can {
>  
>  static struct dummy_can *dummy_can;
>  
> +static const u16 dummy_can_termination_const[] = {
> +	CAN_TERMINATION_DISABLED,	/* 0 = off */
> +	120,				/* 120 Ohms */
> +};
> +
> +static int dummy_can_set_termination(struct net_device *dev, u16 term)
> +{
> +	struct dummy_can *priv = netdev_priv(dev);
> +
> +	netdev_dbg(dev, "set termination to %u Ohms\n", term);
> +	priv->can.termination = term;
> +
> +	return 0;
> +}

The driver has a kind of structure:

  - first the const bittiming struct declarations
  - then the dummy_can_print_*() functions
  - finally the actual code

Try to preserve this structure when adding your changes.

>  static const struct can_bittiming_const dummy_can_bittiming_const = {
>  	.name = "dummy_can CC",
>  	.tseg1_min = 2,
> @@ -250,6 +265,12 @@ static int __init dummy_can_init(void)
>  	priv->can.xl.data_bittiming_const = &dummy_can_xl_databittiming_const;
>  	priv->can.xl.tdc_const = &dummy_can_xl_tdc_const;
>  	priv->can.xl.pwm_const = &dummy_can_pwm_const;
> +	
> +	/* Advertise software termination support */

This comment doesn't add much value. You may omit it.

> +	priv->can.termination_const = dummy_can_termination_const;
> +	priv->can.termination_const_cnt = ARRAY_SIZE(dummy_can_termination_const);
> +	priv->can.do_set_termination = dummy_can_set_termination;

Here also try to maintain so kind of order: your declaration of
dummy_can_termination_const is before the other const struct
declarations, but the priv->can assignment is done after the other
assignments. Not a big deal but it is nicer to keep the declaration
and the assignments in the same order.

>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
>  		CAN_CTRLMODE_FD | CAN_CTRLMODE_TDC_AUTO |
>  		CAN_CTRLMODE_RESTRICTED | CAN_CTRLMODE_XL |


Yours sincerely,
Vincent Mailhol