| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAMZ6RqKgJs-yJaaqREmN1SkUzE1EkGtjBzXiATKx5WL+=J48dQ@mail.gmail.com>
Date: Sun, 7 May 2023 18:58:41 +0900
From: Vincent MAILHOL <mailhol.vincent@...adoo.fr>
To: Frank Jungclaus <frank.jungclaus@....eu>
Cc: linux-can@...r.kernel.org, Marc Kleine-Budde <mkl@...gutronix.de>,
Wolfgang Grandegger <wg@...ndegger.com>,
Stefan Mätje <stefan.maetje@....eu>,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] can: esd_usb: Add support for esd CAN-USB/3
Hi Frank,
Thank you for your patch. Here is my first batch of comments.
Some comments also apply to the existing code. So you may want to
address those in separate clean-up patches first.
On Fri. 5 May 2023 at 01:16, Frank Jungclaus <frank.jungclaus@....eu> wrote:
> Add support for esd CAN-USB/3 and CAN FD to esd_usb.
>
> Signed-off-by: Frank Jungclaus <frank.jungclaus@....eu>
> ---
> drivers/net/can/usb/esd_usb.c | 282 ++++++++++++++++++++++++++++++----
> 1 file changed, 249 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index e24fa48b9b42..48cf5e88d216 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -1,6 +1,6 @@
> // SPDX-License-Identifier: GPL-2.0-only
> /*
> - * CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Micro
> + * CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro
> *
> * Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <socketcan@....eu>
> * Copyright (C) 2022-2023 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@....eu>
> @@ -18,17 +18,19 @@
>
> MODULE_AUTHOR("Matthias Fuchs <socketcan@....eu>");
> MODULE_AUTHOR("Frank Jungclaus <frank.jungclaus@....eu>");
> -MODULE_DESCRIPTION("CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Micro interfaces");
> +MODULE_DESCRIPTION("CAN driver for esd electronics gmbh CAN-USB/2, CAN-USB/3 and CAN-USB/Micro interfaces");
> MODULE_LICENSE("GPL v2");
>
> /* USB vendor and product ID */
> #define USB_ESDGMBH_VENDOR_ID 0x0ab4
> #define USB_CANUSB2_PRODUCT_ID 0x0010
> #define USB_CANUSBM_PRODUCT_ID 0x0011
> +#define USB_CANUSB3_PRODUCT_ID 0x0014
>
> /* CAN controller clock frequencies */
> #define ESD_USB2_CAN_CLOCK 60000000
> #define ESD_USBM_CAN_CLOCK 36000000
> +#define ESD_USB3_CAN_CLOCK 80000000
Nitpick: consider using the unit suffixes from linux/units.h:
#define ESD_USB3_CAN_CLOCK (80 * MEGA)
> /* Maximum number of CAN nets */
> #define ESD_USB_MAX_NETS 2
> @@ -43,6 +45,9 @@ MODULE_LICENSE("GPL v2");
>
> /* esd CAN message flags - dlc field */
> #define ESD_DLC_RTR 0x10
> +#define ESD_DLC_NO_BRS 0x10
> +#define ESD_DLC_ESI 0x20
> +#define ESD_DLC_FD 0x80
Use the BIT() macro:
#define ESD_DLC_NO_BRS BIT(4)
#define ESD_DLC_ESI BIT(5)
#define ESD_DLC_FD BIT(7)
Also, if this is specific to the ESD_USB3 then add it in the prefix.
> /* esd CAN message flags - id field */
> #define ESD_EXTID 0x20000000
> @@ -72,6 +77,28 @@ MODULE_LICENSE("GPL v2");
> #define ESD_USB2_BRP_INC 1
> #define ESD_USB2_3_SAMPLES 0x00800000
>
> +/* Bit timing CAN-USB/3 */
> +#define ESD_USB3_TSEG1_MIN 1
> +#define ESD_USB3_TSEG1_MAX 256
> +#define ESD_USB3_TSEG2_MIN 1
> +#define ESD_USB3_TSEG2_MAX 128
> +#define ESD_USB3_SJW_MAX 128
> +#define ESD_USB3_BRP_MIN 1
> +#define ESD_USB3_BRP_MAX 1024
> +#define ESD_USB3_BRP_INC 1
> +/* Bit timing CAN-USB/3, data phase */
> +#define ESD_USB3_DATA_TSEG1_MIN 1
> +#define ESD_USB3_DATA_TSEG1_MAX 32
> +#define ESD_USB3_DATA_TSEG2_MIN 1
> +#define ESD_USB3_DATA_TSEG2_MAX 16
> +#define ESD_USB3_DATA_SJW_MAX 8
> +#define ESD_USB3_DATA_BRP_MIN 1
> +#define ESD_USB3_DATA_BRP_MAX 32
> +#define ESD_USB3_DATA_BRP_INC 1
There is no clear benefit to define macros for some initializers of a
const struct.
Doing as below has zero ambiguity:
static const struct can_bittiming_const esd_usb3_bittiming_const = {
.name = "esd_usb3",
.tseg1_min = 1,
.tseg1_max = 256,
.tseg2_min = 1,
.tseg2_max = 128,
.sjw_max = 128,
.brp_min = 1,
.brp_max = 1024,
.brp_inc = 1,
};
> +/* Transmitter Delay Compensation */
> +#define ESD_TDC_MODE_AUTO 0
> +
> /* esd IDADD message */
> #define ESD_ID_ENABLE 0x80
> #define ESD_MAX_ID_SEGMENT 64
> @@ -95,6 +122,21 @@ MODULE_LICENSE("GPL v2");
> #define MAX_RX_URBS 4
> #define MAX_TX_URBS 16 /* must be power of 2 */
>
> +/* Modes for NTCAN_BAUDRATE_X */
> +#define ESD_BAUDRATE_MODE_DISABLE 0 /* remove from bus */
> +#define ESD_BAUDRATE_MODE_INDEX 1 /* ESD (CiA) bit rate idx */
> +#define ESD_BAUDRATE_MODE_BTR_CTRL 2 /* BTR values (Controller)*/
> +#define ESD_BAUDRATE_MODE_BTR_CANONICAL 3 /* BTR values (Canonical) */
> +#define ESD_BAUDRATE_MODE_NUM 4 /* numerical bit rate */
> +#define ESD_BAUDRATE_MODE_AUTOBAUD 5 /* autobaud */
> +
> +/* Flags for NTCAN_BAUDRATE_X */
> +#define ESD_BAUDRATE_FLAG_FD 0x0001 /* enable CAN FD Mode */
> +#define ESD_BAUDRATE_FLAG_LOM 0x0002 /* enable Listen Only mode */
> +#define ESD_BAUDRATE_FLAG_STM 0x0004 /* enable Self test mode */
> +#define ESD_BAUDRATE_FLAG_TRS 0x0008 /* enable Triple Sampling */
> +#define ESD_BAUDRATE_FLAG_TXP 0x0010 /* enable Transmit Pause */
> +
> struct header_msg {
> u8 len; /* len is always the total message length in 32bit words */
> u8 cmd;
> @@ -129,6 +171,7 @@ struct rx_msg {
> __le32 id; /* upper 3 bits contain flags */
> union {
> u8 data[8];
> + u8 data_fd[64];
> struct {
> u8 status; /* CAN Controller Status */
> u8 ecc; /* Error Capture Register */
> @@ -144,8 +187,11 @@ struct tx_msg {
> u8 net;
> u8 dlc;
> u32 hnd; /* opaque handle, not used by device */
> - __le32 id; /* upper 3 bits contain flags */
> - u8 data[8];
> + __le32 id; /* upper 3 bits contain flags */
> + union {
> + u8 data[8];
> + u8 data_fd[64];
Nitpick, use the macro:
u8 data[CAN_MAX_DLEN];
u8 data_fd[CANFD_MAX_DLEN];
> + };
> };
>
> struct tx_done_msg {
> @@ -165,12 +211,37 @@ struct id_filter_msg {
> __le32 mask[ESD_MAX_ID_SEGMENT + 1];
> };
>
> +struct baudrate_x_cfg {
> + __le16 brp; /* bit rate pre-scaler */
> + __le16 tseg1; /* TSEG1 register */
> + __le16 tseg2; /* TSEG2 register */
> + __le16 sjw; /* SJW register */
> +};
> +
> +struct tdc_cfg {
Please prefix all the structures with the device name. e.g.
struct esd_usb3_tdc_cfg {
> + u8 tdc_mode; /* transmitter Delay Compensation mode */
> + u8 ssp_offset; /* secondary Sample Point offset in mtq */
> + s8 ssp_shift; /* secondary Sample Point shift in mtq */
> + u8 tdc_filter; /* Transmitter Delay Compensation */
> +};
> +
> +struct baudrate_x {
The x in baudrate_x and baudrate_x_cfg is confusing me. Is it meant to
signify that the structure applies to both nominal and data baudrate?
In that case, just put a comment and remove the x from the name.
> + __le16 mode; /* mode word */
> + __le16 flags; /* control flags */
> + struct tdc_cfg tdc; /* TDC configuration */
> + struct baudrate_x_cfg arb; /* bit rate during arbitration phase */
/* nominal bit rate */
The comment is incorrect. CAN-FD may use the nominal bitrate for the
data phase if the bit rate switch (BRS) is not set.
> + struct baudrate_x_cfg data; /* bit rate during data phase */
/* data bit rate */
Please adjust the field names accordingly.
> +};
> +
> struct set_baudrate_msg {
> u8 len;
> u8 cmd;
> u8 net;
> u8 rsvd;
> - __le32 baud;
> + union {
> + __le32 baud;
> + struct baudrate_x baud_x;
> + };
> };
>
> /* Main message type used between library and application */
> @@ -188,6 +259,7 @@ union __packed esd_usb_msg {
> static struct usb_device_id esd_usb_table[] = {
> {USB_DEVICE(USB_ESDGMBH_VENDOR_ID, USB_CANUSB2_PRODUCT_ID)},
> {USB_DEVICE(USB_ESDGMBH_VENDOR_ID, USB_CANUSBM_PRODUCT_ID)},
> + {USB_DEVICE(USB_ESDGMBH_VENDOR_ID, USB_CANUSB3_PRODUCT_ID)},
> {}
> };
> MODULE_DEVICE_TABLE(usb, esd_usb_table);
> @@ -326,11 +398,13 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
> union esd_usb_msg *msg)
> {
> + bool is_canfd = msg->rx.dlc & ESD_DLC_FD ? true : false;
This is redundant. Just this is enough:
bool is_canfd = msg->rx.dlc & ESD_DLC_FD;
This variable being used only twice, you may want to consider not
declaring it and simply doing directly:
if (msg->rx.dlc & ESD_DLC_FD)
> struct net_device_stats *stats = &priv->netdev->stats;
> struct can_frame *cf;
> + struct canfd_frame *cfd;
> struct sk_buff *skb;
> - int i;
> u32 id;
> + u8 len;
>
> if (!netif_device_present(priv->netdev))
> return;
> @@ -340,27 +414,42 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
> if (id & ESD_EVENT) {
> esd_usb_rx_event(priv, msg);
> } else {
> - skb = alloc_can_skb(priv->netdev, &cf);
> + if (is_canfd) {
> + skb = alloc_canfd_skb(priv->netdev, &cfd);
> + } else {
> + skb = alloc_can_skb(priv->netdev, &cf);
> + cfd = (struct canfd_frame *)cf;
> + }
> +
> if (skb == NULL) {
> stats->rx_dropped++;
> return;
> }
>
> - cf->can_id = id & ESD_IDMASK;
> - can_frame_set_cc_len(cf, msg->rx.dlc & ~ESD_DLC_RTR,
> - priv->can.ctrlmode);
> -
> - if (id & ESD_EXTID)
> - cf->can_id |= CAN_EFF_FLAG;
> + cfd->can_id = id & ESD_IDMASK;
>
> - if (msg->rx.dlc & ESD_DLC_RTR) {
> - cf->can_id |= CAN_RTR_FLAG;
> + if (is_canfd) {
> + /* masking by 0x0F is already done within can_fd_dlc2len() */
> + cfd->len = can_fd_dlc2len(msg->rx.dlc);
> + len = cfd->len;
> + if ((msg->rx.dlc & ESD_DLC_NO_BRS) == 0)
> + cfd->flags |= CANFD_BRS;
> + if (msg->rx.dlc & ESD_DLC_ESI)
> + cfd->flags |= CANFD_ESI;
> } else {
> - for (i = 0; i < cf->len; i++)
> - cf->data[i] = msg->rx.data[i];
> -
> - stats->rx_bytes += cf->len;
> + can_frame_set_cc_len(cf, msg->rx.dlc & ~ESD_DLC_RTR, priv->can.ctrlmode);
> + len = cf->len;
> + if (msg->rx.dlc & ESD_DLC_RTR) {
> + cf->can_id |= CAN_RTR_FLAG;
> + len = 0;
> + }
> }
> +
> + if (id & ESD_EXTID)
> + cfd->can_id |= CAN_EFF_FLAG;
> +
> + memcpy(cfd->data, msg->rx.data_fd, len);
> + stats->rx_bytes += len;
> stats->rx_packets++;
>
> netif_rx(skb);
> @@ -735,7 +824,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
> struct esd_usb *dev = priv->usb;
> struct esd_tx_urb_context *context = NULL;
> struct net_device_stats *stats = &netdev->stats;
> - struct can_frame *cf = (struct can_frame *)skb->data;
> + struct canfd_frame *cfd = (struct canfd_frame *)skb->data;
> union esd_usb_msg *msg;
> struct urb *urb;
> u8 *buf;
> @@ -768,19 +857,28 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
> msg->hdr.len = 3; /* minimal length */
> msg->hdr.cmd = CMD_CAN_TX;
> msg->tx.net = priv->index;
> - msg->tx.dlc = can_get_cc_dlc(cf, priv->can.ctrlmode);
> - msg->tx.id = cpu_to_le32(cf->can_id & CAN_ERR_MASK);
>
> - if (cf->can_id & CAN_RTR_FLAG)
> - msg->tx.dlc |= ESD_DLC_RTR;
> + if (can_is_canfd_skb(skb)) {
> + msg->tx.dlc = can_fd_len2dlc(cfd->len);
> + msg->tx.dlc |= ESD_DLC_FD;
> +
> + if ((cfd->flags & CANFD_BRS) == 0)
> + msg->tx.dlc |= ESD_DLC_NO_BRS;
> + } else {
> + msg->tx.dlc = can_get_cc_dlc((struct can_frame *)cfd, priv->can.ctrlmode);
> +
> + if (cfd->can_id & CAN_RTR_FLAG)
> + msg->tx.dlc |= ESD_DLC_RTR;
> + }
>
> - if (cf->can_id & CAN_EFF_FLAG)
> + msg->tx.id = cpu_to_le32(cfd->can_id & CAN_ERR_MASK);
> +
> + if (cfd->can_id & CAN_EFF_FLAG)
> msg->tx.id |= cpu_to_le32(ESD_EXTID);
>
> - for (i = 0; i < cf->len; i++)
> - msg->tx.data[i] = cf->data[i];
> + memcpy(msg->tx.data_fd, cfd->data, cfd->len);
>
> - msg->hdr.len += (cf->len + 3) >> 2;
> + msg->hdr.len += (cfd->len + 3) >> 2;
I do not get the logic.
Assuming cfd->len is 8. Then
hdr.len += (8 + 3) >> 2
hdr.len += 2
And because hdr.len is initially 3, hdr.len becomes 5. Right? Shouldn't it be 8?
> for (i = 0; i < MAX_TX_URBS; i++) {
> if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> @@ -966,6 +1064,108 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
> return err;
> }
>
> +static const struct can_bittiming_const esd_usb3_bittiming_const = {
> + .name = "esd_usb3",
> + .tseg1_min = ESD_USB3_TSEG1_MIN,
> + .tseg1_max = ESD_USB3_TSEG1_MAX,
> + .tseg2_min = ESD_USB3_TSEG2_MIN,
> + .tseg2_max = ESD_USB3_TSEG2_MAX,
> + .sjw_max = ESD_USB3_SJW_MAX,
> + .brp_min = ESD_USB3_BRP_MIN,
> + .brp_max = ESD_USB3_BRP_MAX,
> + .brp_inc = ESD_USB3_BRP_INC,
> +};
> +
> +static const struct can_bittiming_const esd_usb3_data_bittiming_const = {
> + .name = "esd_usb3",
> + .tseg1_min = ESD_USB3_DATA_TSEG1_MIN,
> + .tseg1_max = ESD_USB3_DATA_TSEG1_MAX,
> + .tseg2_min = ESD_USB3_DATA_TSEG2_MIN,
> + .tseg2_max = ESD_USB3_DATA_TSEG2_MAX,
> + .sjw_max = ESD_USB3_DATA_SJW_MAX,
> + .brp_min = ESD_USB3_DATA_BRP_MIN,
> + .brp_max = ESD_USB3_DATA_BRP_MAX,
> + .brp_inc = ESD_USB3_DATA_BRP_INC,
> +};
> +
> +static int esd_usb3_set_bittiming(struct net_device *netdev)
> +{
> + struct esd_usb_net_priv *priv = netdev_priv(netdev);
> + struct can_bittiming *bt = &priv->can.bittiming;
> + struct can_bittiming *d_bt = &priv->can.data_bittiming;
> + union esd_usb_msg *msg;
> + int err;
> + u16 mode;
> + u16 flags = 0;
> + u16 brp, tseg1, tseg2, sjw;
> + u16 d_brp, d_tseg1, d_tseg2, d_sjw;
> +
> + msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> + if (!msg)
> + return -ENOMEM;
> +
> + /* Canonical is the most reasonable mode for SocketCAN on CAN-USB/3 ... */
> + mode = ESD_BAUDRATE_MODE_BTR_CANONICAL;
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
> + flags |= ESD_BAUDRATE_FLAG_LOM;
> +
> + if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> + flags |= ESD_BAUDRATE_FLAG_TRS;
> +
> + brp = bt->brp & (ESD_USB3_BRP_MAX - 1);
> + sjw = bt->sjw & (ESD_USB3_SJW_MAX - 1);
> + tseg1 = (bt->prop_seg + bt->phase_seg1) & (ESD_USB3_TSEG1_MAX - 1);
> + tseg2 = bt->phase_seg2 & (ESD_USB3_TSEG2_MAX - 1);
I am not convinced by the use of these intermediate variables brp,
sjw, tseg1 and tseg2. I think you can directly assign them to baud_x.
> + msg->setbaud.baud_x.arb.brp = cpu_to_le16(brp);
> + msg->setbaud.baud_x.arb.sjw = cpu_to_le16(sjw);
> + msg->setbaud.baud_x.arb.tseg1 = cpu_to_le16(tseg1);
> + msg->setbaud.baud_x.arb.tseg2 = cpu_to_le16(tseg2);
You may want to declare a local variable
struct baudrate_x *baud_x = &msg->setbaud.baud_x;
so that you do not have to do msg->setbaud.baud_x each time.
> + if (priv->can.ctrlmode & CAN_CTRLMODE_FD) {
> + d_brp = d_bt->brp & (ESD_USB3_DATA_BRP_MAX - 1);
> + d_sjw = d_bt->sjw & (ESD_USB3_DATA_SJW_MAX - 1);
> + d_tseg1 = (d_bt->prop_seg + d_bt->phase_seg1) & (ESD_USB3_DATA_TSEG1_MAX - 1);
> + d_tseg2 = d_bt->phase_seg2 & (ESD_USB3_DATA_TSEG2_MAX - 1);
> + flags |= ESD_BAUDRATE_FLAG_FD;
> + } else {
> + d_brp = 0;
> + d_sjw = 0;
> + d_tseg1 = 0;
> + d_tseg2 = 0;
> + }
> +
> + msg->setbaud.baud_x.data.brp = cpu_to_le16(d_brp);
> + msg->setbaud.baud_x.data.sjw = cpu_to_le16(d_sjw);
> + msg->setbaud.baud_x.data.tseg1 = cpu_to_le16(d_tseg1);
> + msg->setbaud.baud_x.data.tseg2 = cpu_to_le16(d_tseg2);
> + msg->setbaud.baud_x.mode = cpu_to_le16(mode);
> + msg->setbaud.baud_x.flags = cpu_to_le16(flags);
> + msg->setbaud.baud_x.tdc.tdc_mode = ESD_TDC_MODE_AUTO;
> + msg->setbaud.baud_x.tdc.ssp_offset = 0;
> + msg->setbaud.baud_x.tdc.ssp_shift = 0;
> + msg->setbaud.baud_x.tdc.tdc_filter = 0;
It seems that your device supports TDC. What is the reason to not configure it?
Please have a look at struct can_tdc:
https://elixir.bootlin.com/linux/v6.2/source/include/linux/can/bittiming.h#L21
Please refer to this patch if you want an example of how to use TDC:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1010a8fa9608
> + msg->hdr.len = 7;
What is this magic number? If possible, replace it with a sizeof().
It seems that this relates to the size of struct set_baudrate_msg, but
that structure is 8 bytes. Is the last byte of struct set_baudrate_msg
really used? If not, reflect this in the declaration of the structure.
> + msg->hdr.cmd = CMD_SETBAUD;
> +
> + msg->setbaud.net = priv->index;
> + msg->setbaud.rsvd = 0;
> +
> + netdev_info(netdev,
> + "ctrlmode=%#x/%#x, esd-net=%u, esd-mode=%#x, esd-flg=%#x, arb: brp=%u, ts1=%u, ts2=%u, sjw=%u, data: dbrp=%u, dts1=%u, dts2=%u dsjw=%u\n",
> + priv->can.ctrlmode, priv->can.ctrlmode_supported,
> + priv->index, mode, flags,
> + brp, tseg1, tseg2, sjw,
> + d_brp, d_tseg1, d_tseg2, d_sjw);
Remove this debug message. The bittiming information can be retrieved
with the ip tool.
ip --details link show canX
> + err = esd_usb_send_msg(priv->usb, msg);
> +
> + kfree(msg);
esd_usb_send_msg() uses usb_bulk_msg() which does a synchronous call.
It would be great to go asynchronous and use usb_submit_urb() so that
you minimize the time spent in the driver.
I know that esd_usb2_set_bittiming() also uses the synchronous call,
so I am fine to have it as-is for this patch but for the future, it
would be great to consider refactoring this.
> + return err;
> +}
> +
> static int esd_usb_get_berr_counter(const struct net_device *netdev,
> struct can_berr_counter *bec)
> {
> @@ -1023,16 +1223,32 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
> CAN_CTRLMODE_CC_LEN8_DLC |
> CAN_CTRLMODE_BERR_REPORTING;
>
> - if (le16_to_cpu(dev->udev->descriptor.idProduct) ==
> - USB_CANUSBM_PRODUCT_ID)
> + switch (le16_to_cpu(dev->udev->descriptor.idProduct)) {
Instead of doing a switch on idProduct, you can use the driver_info
field from struct usb_device_id to store the device quirks.
You can pass either a pointer or some flags into driver_info. Examples:
https://elixir.bootlin.com/linux/v6.2/source/drivers/net/can/usb/peak_usb/pcan_usb_core.c#L30
https://elixir.bootlin.com/linux/v6.2/source/drivers/net/can/usb/etas_es58x/es58x_core.c#L37
> + case USB_CANUSB3_PRODUCT_ID:
> + priv->can.clock.freq = ESD_USB3_CAN_CLOCK;
> + priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> + priv->can.ctrlmode_supported |= CAN_CTRLMODE_FD;
> + priv->can.bittiming_const = &esd_usb3_bittiming_const;
> + priv->can.data_bittiming_const = &esd_usb3_data_bittiming_const;
> + priv->can.do_set_bittiming = esd_usb3_set_bittiming;
> + priv->can.do_set_data_bittiming = esd_usb3_set_bittiming;
> + break;
> +
> + case USB_CANUSBM_PRODUCT_ID:
> priv->can.clock.freq = ESD_USBM_CAN_CLOCK;
> - else {
> + priv->can.bittiming_const = &esd_usb2_bittiming_const;
> + priv->can.do_set_bittiming = esd_usb2_set_bittiming;
> + break;
> +
> + case USB_CANUSB2_PRODUCT_ID:
> + default:
> priv->can.clock.freq = ESD_USB2_CAN_CLOCK;
> priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> + priv->can.bittiming_const = &esd_usb2_bittiming_const;
> + priv->can.do_set_bittiming = esd_usb2_set_bittiming;
> + break;
> }
>
> - priv->can.bittiming_const = &esd_usb2_bittiming_const;
> - priv->can.do_set_bittiming = esd_usb2_set_bittiming;
> priv->can.do_set_mode = esd_usb_set_mode;
> priv->can.do_get_berr_counter = esd_usb_get_berr_counter;
>
> --
> 2.25.1
>
Powered by blists - more mailing lists