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

[Rakuram Eswaran]

Hi Vincent, 

Thank you for the review and the detailed feedback.

On Wed, 24 Dec 2025 at 03:03, Vincent Mailhol <mailhol@xxxxxxxxxx> wrote:
>
> 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@xxxxxxxxx>
> > ---
> > 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

Noted. I did test with invalid numeric values and observed that
they were ignored.

>   ip link set can0 type can termination potato

Good point — I had not explicitly tested non-numeric inputs.
I will verify this case as well.

>
> (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.

Acknowledged. I will reorder the termination-related additions to match
the existing structure in the next revision.

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

Ack. Will remove 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.
>

Ack. I will align the declaration and the corresponding assignments to
keep the ordering consistent.

I will address these points and send an updated version. Is it okay to send
the next version without the RFC tag?

Best Regards,
Rakuram