On 1/15/21 1:41 AM, Vincent MAILHOL wrote: > 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. 3. Deprecate struct can_bittiming as the user space interface and transfer each member individually via netlink. Extend the kernel-only can_bittiming by the tdc related parameters, and add these to the new netlink interface. I prefer this, as I want to extend the bittiming_const in this way, too. There are CAN controllers, where the bit timing calculation: > bt->prop_seg = tseg1 / 2; > bt->phase_seg1 = tseg1 - bt->prop_seg; doesn't work anymore, as they have asymmetric prog_seg and phase_seg1, so that splitting tseg1 in half doesn't work anymore. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Attachment:
signature.asc
Description: OpenPGP digital signature
