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

[Vincent Mailhol <mailhol@kernel.org>]

On 31/12/2025 at 19:13, Rakuram Eswaran wrote:
> 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
>   ip link set can0 type can termination potato
>   ip link set can0 type can termination 10000
>   
>  drivers/net/can/dummy_can.c | 25 +++++++++++++++++++++++--
>  1 file changed, 23 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/can/dummy_can.c b/drivers/net/can/dummy_can.c
> index 41953655e3d3c9187d6574710e6aa90fc01c92a7..418d9e25bfca1c7af924ad451c8dd8ae1bca78a3 100644
> --- a/drivers/net/can/dummy_can.c
> +++ b/drivers/net/can/dummy_can.c
> @@ -86,6 +86,11 @@ static const struct can_pwm_const dummy_can_pwm_const = {
>  	.pwmo_max = 16,
>  };
>  
> +static const u16 dummy_can_termination_const[] = {
> +	CAN_TERMINATION_DISABLED,	/* 0 = off */
> +	120,				/* 120 Ohms */

Nitpick: no need to explain that disabled means "off", the first comment
can be removed. Also, to be consistent with how the can.bitrate_max and
can.clock.freq are declared, you can add the unit just next to the value.

	static const u16 dummy_can_termination_const[] = {
		CAN_TERMINATION_DISABLED,
		120 /* Ohms */,
	};

(above comment is notwithstanding).

> +};
> +
>  static void dummy_can_print_bittiming(struct net_device *dev,
>  				      struct can_bittiming *bt)
>  {
> @@ -179,6 +184,16 @@ static void dummy_can_print_bittiming_info(struct net_device *dev)
>  	netdev_dbg(dev, "\n");
>  }
>  
> +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;
> +}
> +
>  static int dummy_can_netdev_open(struct net_device *dev)
>  {
>  	int ret;
> @@ -243,17 +258,23 @@ static int __init dummy_can_init(void)
>  	dev->ethtool_ops = &dummy_can_ethtool_ops;
>  	priv = netdev_priv(dev);
>  	priv->can.bittiming_const = &dummy_can_bittiming_const;
> -	priv->can.bitrate_max = 20 * MEGA /* BPS */;
> -	priv->can.clock.freq = 160 * MEGA /* Hz */;

Don't add unrelated changes to your patch. Your patch should do one
thing (here: add the resistance termination). If you want to reorder the
existing lines, that should go in a separate clean-up patch. But here,
there is no need to touch those lines, so just drop this reorder.

>  	priv->can.fd.data_bittiming_const = &dummy_can_fd_databittiming_const;
>  	priv->can.fd.tdc_const = &dummy_can_fd_tdc_const;
>  	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;
> +	priv->can.bitrate_max = 20 * MEGA /* BPS */;
> +	priv->can.clock.freq = 160 * MEGA /* Hz */;
> +	priv->can.termination_const_cnt = ARRAY_SIZE(dummy_can_termination_const);
> +	priv->can.termination_const = dummy_can_termination_const;
> +
>  	priv->can.ctrlmode_supported = CAN_CTRLMODE_LISTENONLY |
>  		CAN_CTRLMODE_FD | CAN_CTRLMODE_TDC_AUTO |
>  		CAN_CTRLMODE_RESTRICTED | CAN_CTRLMODE_XL |
>  		CAN_CTRLMODE_XL_TDC_AUTO | CAN_CTRLMODE_XL_TMS;
> +
> +	priv->can.do_set_termination = dummy_can_set_termination;
> +
>  	priv->dev = dev;
>  
>  	ret = register_candev(priv->dev);

Aside from the above remark this is OK. Please send a v2 with that last
remark addressed. You can also add my review tag:

Reviewed-by: Vincent Mailhol <mailhol@kernel.org>


Yours sincerely,
Vincent Mailhol