On Fri. 15 Jan 2021 at 02:23, Oliver Hartkopp <socketcan@xxxxxxxxxxxx> wrote: > > Hi Vincent, > > On 12.01.21 14:05, Vincent Mailhol wrote: > > This driver supports the ES581.4, ES582.1 and ES584.1 interfaces from > > ETAS GmbH (https://www.etas.com/en/products/es58x.php). > > (..) > > > diff --git a/drivers/net/can/usb/etas_es58x/es58x_fd.c b/drivers/net/can/usb/etas_es58x/es58x_fd.c > > new file mode 100644 > > index 000000000000..6b9534f23c96 > > --- /dev/null > > +++ b/drivers/net/can/usb/etas_es58x/es58x_fd.c > > (..) > > > +static void es58x_fd_print_bittiming(struct net_device *netdev, > > + struct es58x_fd_bittiming > > + *es58x_fd_bittiming, char *type) > > +{ > > + netdev_vdbg(netdev, "bitrate %s = %d\n", type, > > + le32_to_cpu(es58x_fd_bittiming->bitrate)); > > + netdev_vdbg(netdev, "tseg1 %s = %d\n", type, > > + le16_to_cpu(es58x_fd_bittiming->tseg1)); > > + netdev_vdbg(netdev, "tseg2 %s = %d\n", type, > > + le16_to_cpu(es58x_fd_bittiming->tseg2)); > > + netdev_vdbg(netdev, "brp %s = %d\n", type, > > + le16_to_cpu(es58x_fd_bittiming->brp)); > > + netdev_vdbg(netdev, "sjw %s = %d\n", type, > > + le16_to_cpu(es58x_fd_bittiming->sjw)); > > +} > > What is the reason for this code? > > These values can be retrieved with the 'ip' tool and are probably > interesting for development - but not in the final code. First thing, I used netdev_vdbg() (verbose debug). That macro will only produce code if VERBOSE_DEBUG is defined. Normal users will not see those. So yes, this is mostly for development. Also, just realised that netdev_vdbg() is barely used anywhere (only three files use it: https://elixir.bootlin.com/linux/v5.11-rc3/C/ident/netdev_vdbg). I guess that I will remove it :) > > + > > +static void es58x_fd_print_conf(struct net_device *netdev, > > + struct es58x_fd_tx_conf_msg *tx_conf_msg) > > +{ > > + es58x_fd_print_bittiming(netdev, &tx_conf_msg->nominal_bittiming, > > + "nominal"); > > + netdev_vdbg(netdev, "samples_per_bit = %d\n", > > + tx_conf_msg->samples_per_bit); > > + netdev_vdbg(netdev, "sync_edge = %d\n", > > + tx_conf_msg->sync_edge); > > + netdev_vdbg(netdev, "physical_layer = %d\n", > > + tx_conf_msg->physical_layer); > > + netdev_vdbg(netdev, "self_reception = %d\n", > > + tx_conf_msg->self_reception_mode); > > + netdev_vdbg(netdev, "ctrlmode = %d\n", tx_conf_msg->ctrlmode); > > + netdev_vdbg(netdev, "canfd_enabled = %d\n", > > + tx_conf_msg->canfd_enabled); > > + if (tx_conf_msg->canfd_enabled) { > > + es58x_fd_print_bittiming(netdev, > > + &tx_conf_msg->data_bittiming, "data"); > > + netdev_vdbg(netdev, > > + "Transmitter Delay Compensation = %d\n", > > + tx_conf_msg->tdc); > > + netdev_vdbg(netdev, > > + "Transmitter Delay Compensation Offset = %d\n", > > + le16_to_cpu(tx_conf_msg->tdco)); > > + netdev_vdbg(netdev, > > + "Transmitter Delay Compensation Filter = %d\n", > > + le16_to_cpu(tx_conf_msg->tdcf)); > > + } > > +} > > Same here. > > Either the information can be retrieved with the 'ip' tool OR the are > not necessary as set to some reasonable default anyway Ack, will remove. > OR we should > implement the functionality in the general CAN driver infrastructure. Would make sense to me to add the tdco (Transmitter Delay Compensation Offset). Ref: ISO 11898-1 section 11.3.3 "Transmitter delay compensation" I would just like your opinion on one topic: the tdco is specific to CAN FD. If we add it, we have two choices: 1. put it in struct can_bittiming: that will mean that we will have an unused field for classical CAN (field bittiming of struct can_priv). 2. put it in struct can_priv (but outside of struct can_bittiming): no unused field but less pretty. I think that 1/ is best. Yours sincerely, Vincent
