Apply another small batch of patches as preparation for adding support
of the newly available esd CAN-USB/3 to esd_usb.c.
Frank Jungclaus (6):
can: esd_usb: Make use of existing kernel macros
can: esd_usb: Replace initializer macros used for struct
can_bittiming_const
can: esd_usb: Use consistent prefixes for macros
can: esd_usb: Prefix all structures with the device name
can: esd_usb: Replace hardcoded message length given to USB commands
can: esd_usb: Don't bother the user with nonessential log message
drivers/net/can/usb/esd_usb.c | 339 +++++++++++++++++-----------------
1 file changed, 168 insertions(+), 171 deletions(-)
base-commit: 14575e3b5f3ece74e9143d7f7f195f3e5ff085f5
--
2.25.1
Initiated by a comment from Vincent Mailhol add the consistent prefix
ESD_USB_ to all macros defined within esd_usb.c.
For macros specific to esd CAN-USB/2 use ESD_USB_2_ as prefix.
For macros specific to esd CAN-USB/Micro use ESD_USB_M_ as prefix.
Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Suggested-by: Vincent MAILHOL <[email protected]>
Signed-off-by: Frank Jungclaus <[email protected]>
---
drivers/net/can/usb/esd_usb.c | 198 +++++++++++++++++-----------------
1 file changed, 99 insertions(+), 99 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 194aa1cf37b5..23a568bfcdc2 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -23,33 +23,33 @@ MODULE_DESCRIPTION("CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Mi
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 ESD_USB_ESDGMBH_VENDOR_ID 0x0ab4
+#define ESD_USB_CANUSB2_PRODUCT_ID 0x0010
+#define ESD_USB_CANUSBM_PRODUCT_ID 0x0011
/* CAN controller clock frequencies */
-#define ESD_USB2_CAN_CLOCK (60 * MEGA) /* Hz */
-#define ESD_USBM_CAN_CLOCK (36 * MEGA) /* Hz */
+#define ESD_USB_2_CAN_CLOCK (60 * MEGA) /* Hz */
+#define ESD_USB_M_CAN_CLOCK (36 * MEGA) /* Hz */
/* Maximum number of CAN nets */
#define ESD_USB_MAX_NETS 2
/* USB commands */
-#define CMD_VERSION 1 /* also used for VERSION_REPLY */
-#define CMD_CAN_RX 2 /* device to host only */
-#define CMD_CAN_TX 3 /* also used for TX_DONE */
-#define CMD_SETBAUD 4 /* also used for SETBAUD_REPLY */
-#define CMD_TS 5 /* also used for TS_REPLY */
-#define CMD_IDADD 6 /* also used for IDADD_REPLY */
+#define ESD_USB_CMD_VERSION 1 /* also used for VERSION_REPLY */
+#define ESD_USB_CMD_CAN_RX 2 /* device to host only */
+#define ESD_USB_CMD_CAN_TX 3 /* also used for TX_DONE */
+#define ESD_USB_CMD_SETBAUD 4 /* also used for SETBAUD_REPLY */
+#define ESD_USB_CMD_TS 5 /* also used for TS_REPLY */
+#define ESD_USB_CMD_IDADD 6 /* also used for IDADD_REPLY */
/* esd CAN message flags - dlc field */
#define ESD_RTR BIT(4) /* 0x10 */
/* esd CAN message flags - id field */
-#define ESD_EXTID BIT(29) /* 0x20000000 */
-#define ESD_EVENT BIT(30) /* 0x40000000 */
-#define ESD_IDMASK 0x1fffffff
+#define ESD_USB_EXTID BIT(29) /* 0x20000000 */
+#define ESD_USB_EVENT BIT(30) /* 0x40000000 */
+#define ESD_USB_IDMASK 0x1fffffff
/* esd CAN event ids */
#define ESD_EV_CAN_ERROR_EXT 2 /* CAN controller specific diagnostic data */
@@ -59,35 +59,35 @@ MODULE_LICENSE("GPL v2");
#define ESD_USB_UBR BIT(31) /* 0x80000000, User Bit Rate (controller BTR) in bits 0..27 */
#define ESD_USB_NO_BAUDRATE 0x7fffffff /* bit rate unconfigured */
-/* bit timing CAN-USB/2 */
-#define ESD_USB2_TSEG1_SHIFT 16
-#define ESD_USB2_TSEG2_SHIFT 20
-#define ESD_USB2_SJW_SHIFT 14
-#define ESD_USBM_SJW_SHIFT 24
-#define ESD_USB2_3_SAMPLES 0x00800000
+/* bit timing esd CAN-USB */
+#define ESD_USB_2_TSEG1_SHIFT 16
+#define ESD_USB_2_TSEG2_SHIFT 20
+#define ESD_USB_2_SJW_SHIFT 14
+#define ESD_USB_M_SJW_SHIFT 24
+#define ESD_USB_3_SAMPLES 0x00800000
/* esd IDADD message */
-#define ESD_ID_ENABLE 0x80
-#define ESD_MAX_ID_SEGMENT 64
+#define ESD_USB_ID_ENABLE 0x80
+#define ESD_USB_MAX_ID_SEGMENT 64
/* SJA1000 ECC register (emulated by usb firmware) */
-#define SJA1000_ECC_SEG 0x1F
-#define SJA1000_ECC_DIR 0x20
-#define SJA1000_ECC_ERR 0x06
-#define SJA1000_ECC_BIT 0x00
-#define SJA1000_ECC_FORM 0x40
-#define SJA1000_ECC_STUFF 0x80
-#define SJA1000_ECC_MASK 0xc0
+#define ESD_USB_SJA1000_ECC_SEG 0x1F
+#define ESD_USB_SJA1000_ECC_DIR 0x20
+#define ESD_USB_SJA1000_ECC_ERR 0x06
+#define ESD_USB_SJA1000_ECC_BIT 0x00
+#define ESD_USB_SJA1000_ECC_FORM 0x40
+#define ESD_USB_SJA1000_ECC_STUFF 0x80
+#define ESD_USB_SJA1000_ECC_MASK 0xc0
/* esd bus state event codes */
-#define ESD_BUSSTATE_MASK 0xc0
-#define ESD_BUSSTATE_WARN 0x40
-#define ESD_BUSSTATE_ERRPASSIVE 0x80
-#define ESD_BUSSTATE_BUSOFF 0xc0
+#define ESD_USB_BUSSTATE_MASK 0xc0
+#define ESD_USB_BUSSTATE_WARN 0x40
+#define ESD_USB_BUSSTATE_ERRPASSIVE 0x80
+#define ESD_USB_BUSSTATE_BUSOFF 0xc0
-#define RX_BUFFER_SIZE 1024
-#define MAX_RX_URBS 4
-#define MAX_TX_URBS 16 /* must be power of 2 */
+#define ESD_USB_RX_BUFFER_SIZE 1024
+#define ESD_USB_MAX_RX_URBS 4
+#define ESD_USB_MAX_TX_URBS 16 /* must be power of 2 */
struct header_msg {
u8 len; /* len is always the total message length in 32bit words */
@@ -156,7 +156,7 @@ struct id_filter_msg {
u8 cmd;
u8 net;
u8 option;
- __le32 mask[ESD_MAX_ID_SEGMENT + 1];
+ __le32 mask[ESD_USB_MAX_ID_SEGMENT + 1];
};
struct set_baudrate_msg {
@@ -180,8 +180,8 @@ 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(ESD_USB_ESDGMBH_VENDOR_ID, ESD_USB_CANUSB2_PRODUCT_ID)},
+ {USB_DEVICE(ESD_USB_ESDGMBH_VENDOR_ID, ESD_USB_CANUSBM_PRODUCT_ID)},
{}
};
MODULE_DEVICE_TABLE(usb, esd_usb_table);
@@ -202,8 +202,8 @@ struct esd_usb {
int net_count;
u32 version;
int rxinitdone;
- void *rxbuf[MAX_RX_URBS];
- dma_addr_t rxbuf_dma[MAX_RX_URBS];
+ void *rxbuf[ESD_USB_MAX_RX_URBS];
+ dma_addr_t rxbuf_dma[ESD_USB_MAX_RX_URBS];
};
struct esd_usb_net_priv {
@@ -211,7 +211,7 @@ struct esd_usb_net_priv {
atomic_t active_tx_jobs;
struct usb_anchor tx_submitted;
- struct esd_tx_urb_context tx_contexts[MAX_TX_URBS];
+ struct esd_tx_urb_context tx_contexts[ESD_USB_MAX_TX_URBS];
struct esd_usb *usb;
struct net_device *netdev;
@@ -226,7 +226,7 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
struct net_device_stats *stats = &priv->netdev->stats;
struct can_frame *cf;
struct sk_buff *skb;
- u32 id = le32_to_cpu(msg->rx.id) & ESD_IDMASK;
+ u32 id = le32_to_cpu(msg->rx.id) & ESD_USB_IDMASK;
if (id == ESD_EV_CAN_ERROR_EXT) {
u8 state = msg->rx.ev_can_err_ext.status;
@@ -255,15 +255,15 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
priv->old_state = state;
- switch (state & ESD_BUSSTATE_MASK) {
- case ESD_BUSSTATE_BUSOFF:
+ switch (state & ESD_USB_BUSSTATE_MASK) {
+ case ESD_USB_BUSSTATE_BUSOFF:
new_state = CAN_STATE_BUS_OFF;
can_bus_off(priv->netdev);
break;
- case ESD_BUSSTATE_WARN:
+ case ESD_USB_BUSSTATE_WARN:
new_state = CAN_STATE_ERROR_WARNING;
break;
- case ESD_BUSSTATE_ERRPASSIVE:
+ case ESD_USB_BUSSTATE_ERRPASSIVE:
new_state = CAN_STATE_ERROR_PASSIVE;
break;
default:
@@ -285,14 +285,14 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
- switch (ecc & SJA1000_ECC_MASK) {
- case SJA1000_ECC_BIT:
+ switch (ecc & ESD_USB_SJA1000_ECC_MASK) {
+ case ESD_USB_SJA1000_ECC_BIT:
cf->data[2] |= CAN_ERR_PROT_BIT;
break;
- case SJA1000_ECC_FORM:
+ case ESD_USB_SJA1000_ECC_FORM:
cf->data[2] |= CAN_ERR_PROT_FORM;
break;
- case SJA1000_ECC_STUFF:
+ case ESD_USB_SJA1000_ECC_STUFF:
cf->data[2] |= CAN_ERR_PROT_STUFF;
break;
default:
@@ -300,11 +300,11 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
}
/* Error occurred during transmission? */
- if (!(ecc & SJA1000_ECC_DIR))
+ if (!(ecc & ESD_USB_SJA1000_ECC_DIR))
cf->data[2] |= CAN_ERR_PROT_TX;
/* Bit stream position in CAN frame as the error was detected */
- cf->data[3] = ecc & SJA1000_ECC_SEG;
+ cf->data[3] = ecc & ESD_USB_SJA1000_ECC_SEG;
}
if (skb) {
@@ -331,7 +331,7 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
id = le32_to_cpu(msg->rx.id);
- if (id & ESD_EVENT) {
+ if (id & ESD_USB_EVENT) {
esd_usb_rx_event(priv, msg);
} else {
skb = alloc_can_skb(priv->netdev, &cf);
@@ -340,11 +340,11 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
return;
}
- cf->can_id = id & ESD_IDMASK;
+ cf->can_id = id & ESD_USB_IDMASK;
can_frame_set_cc_len(cf, msg->rx.dlc & ~ESD_RTR,
priv->can.ctrlmode);
- if (id & ESD_EXTID)
+ if (id & ESD_USB_EXTID)
cf->can_id |= CAN_EFF_FLAG;
if (msg->rx.dlc & ESD_RTR) {
@@ -371,7 +371,7 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
if (!netif_device_present(netdev))
return;
- context = &priv->tx_contexts[msg->txdone.hnd & (MAX_TX_URBS - 1)];
+ context = &priv->tx_contexts[msg->txdone.hnd & (ESD_USB_MAX_TX_URBS - 1)];
if (!msg->txdone.status) {
stats->tx_packets++;
@@ -383,7 +383,7 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
}
/* Release context */
- context->echo_index = MAX_TX_URBS;
+ context->echo_index = ESD_USB_MAX_TX_URBS;
atomic_dec(&priv->active_tx_jobs);
netif_wake_queue(netdev);
@@ -418,7 +418,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);
switch (msg->hdr.cmd) {
- case CMD_CAN_RX:
+ case ESD_USB_CMD_CAN_RX:
if (msg->rx.net >= dev->net_count) {
dev_err(dev->udev->dev.parent, "format error\n");
break;
@@ -427,7 +427,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);
break;
- case CMD_CAN_TX:
+ case ESD_USB_CMD_CAN_TX:
if (msg->txdone.net >= dev->net_count) {
dev_err(dev->udev->dev.parent, "format error\n");
break;
@@ -448,7 +448,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
resubmit_urb:
usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
- urb->transfer_buffer, RX_BUFFER_SIZE,
+ urb->transfer_buffer, ESD_USB_RX_BUFFER_SIZE,
esd_usb_read_bulk_callback, dev);
retval = usb_submit_urb(urb, GFP_ATOMIC);
@@ -557,7 +557,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
if (dev->rxinitdone)
return 0;
- for (i = 0; i < MAX_RX_URBS; i++) {
+ for (i = 0; i < ESD_USB_MAX_RX_URBS; i++) {
struct urb *urb = NULL;
u8 *buf = NULL;
dma_addr_t buf_dma;
@@ -569,7 +569,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
break;
}
- buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
+ buf = usb_alloc_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL,
&buf_dma);
if (!buf) {
dev_warn(dev->udev->dev.parent,
@@ -582,7 +582,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
usb_fill_bulk_urb(urb, dev->udev,
usb_rcvbulkpipe(dev->udev, 1),
- buf, RX_BUFFER_SIZE,
+ buf, ESD_USB_RX_BUFFER_SIZE,
esd_usb_read_bulk_callback, dev);
urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
usb_anchor_urb(urb, &dev->rx_submitted);
@@ -590,7 +590,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
err = usb_submit_urb(urb, GFP_KERNEL);
if (err) {
usb_unanchor_urb(urb);
- usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
+ usb_free_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE, buf,
urb->transfer_dma);
goto freeurb;
}
@@ -612,7 +612,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
}
/* Warn if we've couldn't transmit all the URBs */
- if (i < MAX_RX_URBS) {
+ if (i < ESD_USB_MAX_RX_URBS) {
dev_warn(dev->udev->dev.parent,
"rx performance may be slow\n");
}
@@ -647,14 +647,14 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)
* the number of the starting bitmask (0..64) to the filter.option
* field followed by only some bitmasks.
*/
- msg->hdr.cmd = CMD_IDADD;
- msg->hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+ msg->hdr.cmd = ESD_USB_CMD_IDADD;
+ msg->hdr.len = 2 + ESD_USB_MAX_ID_SEGMENT;
msg->filter.net = priv->index;
- msg->filter.option = ESD_ID_ENABLE; /* start with segment 0 */
- for (i = 0; i < ESD_MAX_ID_SEGMENT; i++)
+ msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */
+ for (i = 0; i < ESD_USB_MAX_ID_SEGMENT; i++)
msg->filter.mask[i] = cpu_to_le32(0xffffffff);
/* enable 29bit extended IDs */
- msg->filter.mask[ESD_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
+ msg->filter.mask[ESD_USB_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
err = esd_usb_send_msg(dev, msg);
if (err)
@@ -683,8 +683,8 @@ static void unlink_all_urbs(struct esd_usb *dev)
usb_kill_anchored_urbs(&dev->rx_submitted);
- for (i = 0; i < MAX_RX_URBS; ++i)
- usb_free_coherent(dev->udev, RX_BUFFER_SIZE,
+ for (i = 0; i < ESD_USB_MAX_RX_URBS; ++i)
+ usb_free_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE,
dev->rxbuf[i], dev->rxbuf_dma[i]);
for (i = 0; i < dev->net_count; i++) {
@@ -693,8 +693,8 @@ static void unlink_all_urbs(struct esd_usb *dev)
usb_kill_anchored_urbs(&priv->tx_submitted);
atomic_set(&priv->active_tx_jobs, 0);
- for (j = 0; j < MAX_TX_URBS; j++)
- priv->tx_contexts[j].echo_index = MAX_TX_URBS;
+ for (j = 0; j < ESD_USB_MAX_TX_URBS; j++)
+ priv->tx_contexts[j].echo_index = ESD_USB_MAX_TX_URBS;
}
}
}
@@ -760,7 +760,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
msg = (union esd_usb_msg *)buf;
msg->hdr.len = 3; /* minimal length */
- msg->hdr.cmd = CMD_CAN_TX;
+ msg->hdr.cmd = ESD_USB_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);
@@ -769,15 +769,15 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
msg->tx.dlc |= ESD_RTR;
if (cf->can_id & CAN_EFF_FLAG)
- msg->tx.id |= cpu_to_le32(ESD_EXTID);
+ msg->tx.id |= cpu_to_le32(ESD_USB_EXTID);
for (i = 0; i < cf->len; i++)
msg->tx.data[i] = cf->data[i];
msg->hdr.len += (cf->len + 3) >> 2;
- for (i = 0; i < MAX_TX_URBS; i++) {
- if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
+ for (i = 0; i < ESD_USB_MAX_TX_URBS; i++) {
+ if (priv->tx_contexts[i].echo_index == ESD_USB_MAX_TX_URBS) {
context = &priv->tx_contexts[i];
break;
}
@@ -809,7 +809,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
atomic_inc(&priv->active_tx_jobs);
/* Slow down tx path */
- if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
+ if (atomic_read(&priv->active_tx_jobs) >= ESD_USB_MAX_TX_URBS)
netif_stop_queue(netdev);
err = usb_submit_urb(urb, GFP_ATOMIC);
@@ -859,18 +859,18 @@ static int esd_usb_close(struct net_device *netdev)
return -ENOMEM;
/* Disable all IDs (see esd_usb_start()) */
- msg->hdr.cmd = CMD_IDADD;
- msg->hdr.len = 2 + ESD_MAX_ID_SEGMENT;
+ msg->hdr.cmd = ESD_USB_CMD_IDADD;
+ msg->hdr.len = 2 + ESD_USB_MAX_ID_SEGMENT;
msg->filter.net = priv->index;
- msg->filter.option = ESD_ID_ENABLE; /* start with segment 0 */
- for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
+ msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */
+ for (i = 0; i <= ESD_USB_MAX_ID_SEGMENT; i++)
msg->filter.mask[i] = 0;
if (esd_usb_send_msg(priv->usb, msg) < 0)
netdev_err(netdev, "sending idadd message failed\n");
/* set CAN controller to reset mode */
msg->hdr.len = 2;
- msg->hdr.cmd = CMD_SETBAUD;
+ msg->hdr.cmd = ESD_USB_CMD_SETBAUD;
msg->setbaud.net = priv->index;
msg->setbaud.rsvd = 0;
msg->setbaud.baud = cpu_to_le32(ESD_USB_NO_BAUDRATE);
@@ -928,27 +928,27 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
canbtr |= (bt->brp - 1) & (btc->brp_max - 1);
if (le16_to_cpu(priv->usb->udev->descriptor.idProduct) ==
- USB_CANUSBM_PRODUCT_ID)
- sjw_shift = ESD_USBM_SJW_SHIFT;
+ ESD_USB_CANUSBM_PRODUCT_ID)
+ sjw_shift = ESD_USB_M_SJW_SHIFT;
else
- sjw_shift = ESD_USB2_SJW_SHIFT;
+ sjw_shift = ESD_USB_2_SJW_SHIFT;
canbtr |= ((bt->sjw - 1) & (btc->sjw_max - 1))
<< sjw_shift;
canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1)
& (btc->tseg1_max - 1))
- << ESD_USB2_TSEG1_SHIFT;
+ << ESD_USB_2_TSEG1_SHIFT;
canbtr |= ((bt->phase_seg2 - 1) & (btc->tseg2_max - 1))
- << ESD_USB2_TSEG2_SHIFT;
+ << ESD_USB_2_TSEG2_SHIFT;
if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
- canbtr |= ESD_USB2_3_SAMPLES;
+ canbtr |= ESD_USB_3_SAMPLES;
msg = kmalloc(sizeof(*msg), GFP_KERNEL);
if (!msg)
return -ENOMEM;
msg->hdr.len = 2;
- msg->hdr.cmd = CMD_SETBAUD;
+ msg->hdr.cmd = ESD_USB_CMD_SETBAUD;
msg->setbaud.net = priv->index;
msg->setbaud.rsvd = 0;
msg->setbaud.baud = cpu_to_le32(canbtr);
@@ -994,7 +994,7 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
int err = 0;
int i;
- netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
+ netdev = alloc_candev(sizeof(*priv), ESD_USB_MAX_TX_URBS);
if (!netdev) {
dev_err(&intf->dev, "couldn't alloc candev\n");
err = -ENOMEM;
@@ -1006,8 +1006,8 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
init_usb_anchor(&priv->tx_submitted);
atomic_set(&priv->active_tx_jobs, 0);
- for (i = 0; i < MAX_TX_URBS; i++)
- priv->tx_contexts[i].echo_index = MAX_TX_URBS;
+ for (i = 0; i < ESD_USB_MAX_TX_URBS; i++)
+ priv->tx_contexts[i].echo_index = ESD_USB_MAX_TX_URBS;
priv->usb = dev;
priv->netdev = netdev;
@@ -1019,10 +1019,10 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
CAN_CTRLMODE_BERR_REPORTING;
if (le16_to_cpu(dev->udev->descriptor.idProduct) ==
- USB_CANUSBM_PRODUCT_ID)
- priv->can.clock.freq = ESD_USBM_CAN_CLOCK;
+ ESD_USB_CANUSBM_PRODUCT_ID)
+ priv->can.clock.freq = ESD_USB_M_CAN_CLOCK;
else {
- priv->can.clock.freq = ESD_USB2_CAN_CLOCK;
+ priv->can.clock.freq = ESD_USB_2_CAN_CLOCK;
priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
}
@@ -1085,7 +1085,7 @@ static int esd_usb_probe(struct usb_interface *intf,
}
/* query number of CAN interfaces (nets) */
- msg->hdr.cmd = CMD_VERSION;
+ msg->hdr.cmd = ESD_USB_CMD_VERSION;
msg->hdr.len = 2;
msg->version.rsvd = 0;
msg->version.flags = 0;
--
2.25.1
Initiated by a comment from Vincent Mailhol and suggested by Marc
Kleine-Budde replace a netdev_info(), emitting an informational
message about the BTR value to be send to the controller, with a debug
message by means of netdev_dbg().
Link: https://lore.kernel.org/all/[email protected]/
Suggested-by: Marc Kleine-Budde <[email protected]>
Suggested-by: Vincent MAILHOL <[email protected]>
Signed-off-by: Frank Jungclaus <[email protected]>
---
drivers/net/can/usb/esd_usb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 9053a338eb88..38212330cf50 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -955,7 +955,7 @@ static int esd_usb_2_set_bittiming(struct net_device *netdev)
msg->setbaud.rsvd = 0;
msg->setbaud.baud = cpu_to_le32(canbtr);
- netdev_info(netdev, "setting BTR=%#x\n", canbtr);
+ netdev_dbg(netdev, "setting BTR=%#x\n", canbtr);
err = esd_usb_send_msg(priv->usb, msg);
--
2.25.1
As suggested by Vincent Mailhol prefix all the structures with the
device name.
For commonly used structures make use of (the module name) esd_usb_.
For esd CAN-USB/2 and CAN-USB/Micro specific structures use
esd_usb_2_ and esd_usb_m.
Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Suggested-by: Vincent MAILHOL <[email protected]>
Signed-off-by: Frank Jungclaus <[email protected]>
---
drivers/net/can/usb/esd_usb.c | 42 +++++++++++++++++------------------
1 file changed, 21 insertions(+), 21 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 23a568bfcdc2..1a51a8541bdd 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -89,13 +89,13 @@ MODULE_LICENSE("GPL v2");
#define ESD_USB_MAX_RX_URBS 4
#define ESD_USB_MAX_TX_URBS 16 /* must be power of 2 */
-struct header_msg {
+struct esd_usb_header_msg {
u8 len; /* len is always the total message length in 32bit words */
u8 cmd;
u8 rsvd[2];
};
-struct version_msg {
+struct esd_usb_version_msg {
u8 len;
u8 cmd;
u8 rsvd;
@@ -103,7 +103,7 @@ struct version_msg {
__le32 drv_version;
};
-struct version_reply_msg {
+struct esd_usb_version_reply_msg {
u8 len;
u8 cmd;
u8 nets;
@@ -114,7 +114,7 @@ struct version_reply_msg {
__le32 ts;
};
-struct rx_msg {
+struct esd_usb_rx_msg {
u8 len;
u8 cmd;
u8 net;
@@ -132,7 +132,7 @@ struct rx_msg {
};
};
-struct tx_msg {
+struct esd_usb_tx_msg {
u8 len;
u8 cmd;
u8 net;
@@ -142,7 +142,7 @@ struct tx_msg {
u8 data[CAN_MAX_DLEN];
};
-struct tx_done_msg {
+struct esd_usb_tx_done_msg {
u8 len;
u8 cmd;
u8 net;
@@ -151,7 +151,7 @@ struct tx_done_msg {
__le32 ts;
};
-struct id_filter_msg {
+struct esd_usb_id_filter_msg {
u8 len;
u8 cmd;
u8 net;
@@ -159,7 +159,7 @@ struct id_filter_msg {
__le32 mask[ESD_USB_MAX_ID_SEGMENT + 1];
};
-struct set_baudrate_msg {
+struct esd_usb_set_baudrate_msg {
u8 len;
u8 cmd;
u8 net;
@@ -169,14 +169,14 @@ struct set_baudrate_msg {
/* Main message type used between library and application */
union __packed esd_usb_msg {
- struct header_msg hdr;
- struct version_msg version;
- struct version_reply_msg version_reply;
- struct rx_msg rx;
- struct tx_msg tx;
- struct tx_done_msg txdone;
- struct set_baudrate_msg setbaud;
- struct id_filter_msg filter;
+ struct esd_usb_header_msg hdr;
+ struct esd_usb_version_msg version;
+ struct esd_usb_version_reply_msg version_reply;
+ struct esd_usb_rx_msg rx;
+ struct esd_usb_tx_msg tx;
+ struct esd_usb_tx_done_msg txdone;
+ struct esd_usb_set_baudrate_msg setbaud;
+ struct esd_usb_id_filter_msg filter;
};
static struct usb_device_id esd_usb_table[] = {
@@ -899,8 +899,8 @@ static const struct ethtool_ops esd_usb_ethtool_ops = {
.get_ts_info = ethtool_op_get_ts_info,
};
-static const struct can_bittiming_const esd_usb2_bittiming_const = {
- .name = "esd_usb2",
+static const struct can_bittiming_const esd_usb_2_bittiming_const = {
+ .name = "esd_usb_2",
.tseg1_min = 1,
.tseg1_max = 16,
.tseg2_min = 1,
@@ -911,7 +911,7 @@ static const struct can_bittiming_const esd_usb2_bittiming_const = {
.brp_inc = 1,
};
-static int esd_usb2_set_bittiming(struct net_device *netdev)
+static int esd_usb_2_set_bittiming(struct net_device *netdev)
{
struct esd_usb_net_priv *priv = netdev_priv(netdev);
const struct can_bittiming_const *btc = priv->can.bittiming_const;
@@ -1026,8 +1026,8 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
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;
+ priv->can.bittiming_const = &esd_usb_2_bittiming_const;
+ priv->can.do_set_bittiming = esd_usb_2_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
As suggested by Vincent Mailhol make use of existing kernel macros:
- Use the unit suffixes from linux/units.h for the controller clock
frequencies
- Use the BIT() macro to set specific bits in some constants
- Use CAN_MAX_DLEN (instead of directly using the value 8) for the
maximum CAN payload length
Additionally:
- Spend some commenting for the previously changed constants
- Add the current year to the copyright notice
- While adding the header linux/units.h to the list of include files
also sort that list alphabetically
Suggested-by: Vincent MAILHOL <[email protected]>
Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Signed-off-by: Frank Jungclaus <[email protected]>
---
drivers/net/can/usb/esd_usb.c | 38 ++++++++++++++++++-----------------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index d33bac3a6c10..042dda98b3db 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -3,19 +3,20 @@
* CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Micro
*
* Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <[email protected]>
- * Copyright (C) 2022 esd electronics gmbh, Frank Jungclaus <[email protected]>
+ * Copyright (C) 2022-2023 esd electronics gmbh, Frank Jungclaus <[email protected]>
*/
+#include <linux/can.h>
+#include <linux/can/dev.h>
+#include <linux/can/error.h>
+
#include <linux/ethtool.h>
-#include <linux/signal.h>
-#include <linux/slab.h>
#include <linux/module.h>
#include <linux/netdevice.h>
+#include <linux/signal.h>
+#include <linux/slab.h>
+#include <linux/units.h>
#include <linux/usb.h>
-#include <linux/can.h>
-#include <linux/can/dev.h>
-#include <linux/can/error.h>
-
MODULE_AUTHOR("Matthias Fuchs <[email protected]>");
MODULE_AUTHOR("Frank Jungclaus <[email protected]>");
MODULE_DESCRIPTION("CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Micro interfaces");
@@ -27,8 +28,8 @@ MODULE_LICENSE("GPL v2");
#define USB_CANUSBM_PRODUCT_ID 0x0011
/* CAN controller clock frequencies */
-#define ESD_USB2_CAN_CLOCK 60000000
-#define ESD_USBM_CAN_CLOCK 36000000
+#define ESD_USB2_CAN_CLOCK (60 * MEGA) /* Hz */
+#define ESD_USBM_CAN_CLOCK (36 * MEGA) /* Hz */
/* Maximum number of CAN nets */
#define ESD_USB_MAX_NETS 2
@@ -42,20 +43,21 @@ MODULE_LICENSE("GPL v2");
#define CMD_IDADD 6 /* also used for IDADD_REPLY */
/* esd CAN message flags - dlc field */
-#define ESD_RTR 0x10
+#define ESD_RTR BIT(4) /* 0x10 */
+
/* esd CAN message flags - id field */
-#define ESD_EXTID 0x20000000
-#define ESD_EVENT 0x40000000
-#define ESD_IDMASK 0x1fffffff
+#define ESD_EXTID BIT(29) /* 0x20000000 */
+#define ESD_EVENT BIT(30) /* 0x40000000 */
+#define ESD_IDMASK 0x1fffffff
/* esd CAN event ids */
#define ESD_EV_CAN_ERROR_EXT 2 /* CAN controller specific diagnostic data */
/* baudrate message flags */
-#define ESD_USB_UBR 0x80000000
-#define ESD_USB_LOM 0x40000000
-#define ESD_USB_NO_BAUDRATE 0x7fffffff
+#define ESD_USB_LOM BIT(30) /* 0x40000000, Listen Only Mode */
+#define ESD_USB_UBR BIT(31) /* 0x80000000, User Bit Rate (controller BTR) in bits 0..27 */
+#define ESD_USB_NO_BAUDRATE 0x7fffffff /* bit rate unconfigured */
/* bit timing CAN-USB/2 */
#define ESD_USB2_TSEG1_MIN 1
@@ -128,7 +130,7 @@ struct rx_msg {
__le32 ts;
__le32 id; /* upper 3 bits contain flags */
union {
- u8 data[8];
+ u8 data[CAN_MAX_DLEN];
struct {
u8 status; /* CAN Controller Status */
u8 ecc; /* Error Capture Register */
@@ -145,7 +147,7 @@ struct tx_msg {
u8 dlc;
u32 hnd; /* opaque handle, not used by device */
__le32 id; /* upper 3 bits contain flags */
- u8 data[8];
+ u8 data[CAN_MAX_DLEN];
};
struct tx_done_msg {
--
2.25.1
Hi, thanks for the series. I went through it and just have two nitpicks.
On Thu. 18 May 2023 at 04:27, Frank Jungclaus <[email protected]> wrote:
> Initiated by a comment from Vincent Mailhol add the consistent prefix
No need to start all the series with:
Initiated by a comment from Vincent Mailhol
while I appreciate the ommage, the Suggested-by tag is sufficient for that.
> ESD_USB_ to all macros defined within esd_usb.c.
>
> For macros specific to esd CAN-USB/2 use ESD_USB_2_ as prefix.
> For macros specific to esd CAN-USB/Micro use ESD_USB_M_ as prefix.
>
> Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
> Suggested-by: Vincent MAILHOL <[email protected]>
> Signed-off-by: Frank Jungclaus <[email protected]>
> ---
> drivers/net/can/usb/esd_usb.c | 198 +++++++++++++++++-----------------
> 1 file changed, 99 insertions(+), 99 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 194aa1cf37b5..23a568bfcdc2 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -23,33 +23,33 @@ MODULE_DESCRIPTION("CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Mi
> 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 ESD_USB_ESDGMBH_VENDOR_ID 0x0ab4
> +#define ESD_USB_CANUSB2_PRODUCT_ID 0x0010
> +#define ESD_USB_CANUSBM_PRODUCT_ID 0x0011
>
> /* CAN controller clock frequencies */
> -#define ESD_USB2_CAN_CLOCK (60 * MEGA) /* Hz */
> -#define ESD_USBM_CAN_CLOCK (36 * MEGA) /* Hz */
> +#define ESD_USB_2_CAN_CLOCK (60 * MEGA) /* Hz */
> +#define ESD_USB_M_CAN_CLOCK (36 * MEGA) /* Hz */
>
> /* Maximum number of CAN nets */
> #define ESD_USB_MAX_NETS 2
>
> /* USB commands */
> -#define CMD_VERSION 1 /* also used for VERSION_REPLY */
> -#define CMD_CAN_RX 2 /* device to host only */
> -#define CMD_CAN_TX 3 /* also used for TX_DONE */
> -#define CMD_SETBAUD 4 /* also used for SETBAUD_REPLY */
> -#define CMD_TS 5 /* also used for TS_REPLY */
> -#define CMD_IDADD 6 /* also used for IDADD_REPLY */
> +#define ESD_USB_CMD_VERSION 1 /* also used for VERSION_REPLY */
> +#define ESD_USB_CMD_CAN_RX 2 /* device to host only */
> +#define ESD_USB_CMD_CAN_TX 3 /* also used for TX_DONE */
> +#define ESD_USB_CMD_SETBAUD 4 /* also used for SETBAUD_REPLY */
> +#define ESD_USB_CMD_TS 5 /* also used for TS_REPLY */
> +#define ESD_USB_CMD_IDADD 6 /* also used for IDADD_REPLY */
>
> /* esd CAN message flags - dlc field */
> #define ESD_RTR BIT(4) /* 0x10 */
>
>
> /* esd CAN message flags - id field */
> -#define ESD_EXTID BIT(29) /* 0x20000000 */
> -#define ESD_EVENT BIT(30) /* 0x40000000 */
> -#define ESD_IDMASK 0x1fffffff
> +#define ESD_USB_EXTID BIT(29) /* 0x20000000 */
> +#define ESD_USB_EVENT BIT(30) /* 0x40000000 */
> +#define ESD_USB_IDMASK 0x1fffffff
>
> /* esd CAN event ids */
> #define ESD_EV_CAN_ERROR_EXT 2 /* CAN controller specific diagnostic data */
> @@ -59,35 +59,35 @@ MODULE_LICENSE("GPL v2");
> #define ESD_USB_UBR BIT(31) /* 0x80000000, User Bit Rate (controller BTR) in bits 0..27 */
> #define ESD_USB_NO_BAUDRATE 0x7fffffff /* bit rate unconfigured */
>
> -/* bit timing CAN-USB/2 */
> -#define ESD_USB2_TSEG1_SHIFT 16
> -#define ESD_USB2_TSEG2_SHIFT 20
> -#define ESD_USB2_SJW_SHIFT 14
> -#define ESD_USBM_SJW_SHIFT 24
> -#define ESD_USB2_3_SAMPLES 0x00800000
That one should also use the BIT macro.
> +/* bit timing esd CAN-USB */
> +#define ESD_USB_2_TSEG1_SHIFT 16
> +#define ESD_USB_2_TSEG2_SHIFT 20
> +#define ESD_USB_2_SJW_SHIFT 14
> +#define ESD_USB_M_SJW_SHIFT 24
> +#define ESD_USB_3_SAMPLES 0x00800000
>
> /* esd IDADD message */
> -#define ESD_ID_ENABLE 0x80
> -#define ESD_MAX_ID_SEGMENT 64
> +#define ESD_USB_ID_ENABLE 0x80
> +#define ESD_USB_MAX_ID_SEGMENT 64
>
> /* SJA1000 ECC register (emulated by usb firmware) */
> -#define SJA1000_ECC_SEG 0x1F
> -#define SJA1000_ECC_DIR 0x20
> -#define SJA1000_ECC_ERR 0x06
> -#define SJA1000_ECC_BIT 0x00
> -#define SJA1000_ECC_FORM 0x40
> -#define SJA1000_ECC_STUFF 0x80
> -#define SJA1000_ECC_MASK 0xc0
> +#define ESD_USB_SJA1000_ECC_SEG 0x1F
> +#define ESD_USB_SJA1000_ECC_DIR 0x20
> +#define ESD_USB_SJA1000_ECC_ERR 0x06
> +#define ESD_USB_SJA1000_ECC_BIT 0x00
> +#define ESD_USB_SJA1000_ECC_FORM 0x40
> +#define ESD_USB_SJA1000_ECC_STUFF 0x80
> +#define ESD_USB_SJA1000_ECC_MASK 0xc0
>
> /* esd bus state event codes */
> -#define ESD_BUSSTATE_MASK 0xc0
> -#define ESD_BUSSTATE_WARN 0x40
> -#define ESD_BUSSTATE_ERRPASSIVE 0x80
> -#define ESD_BUSSTATE_BUSOFF 0xc0
> +#define ESD_USB_BUSSTATE_MASK 0xc0
> +#define ESD_USB_BUSSTATE_WARN 0x40
> +#define ESD_USB_BUSSTATE_ERRPASSIVE 0x80
> +#define ESD_USB_BUSSTATE_BUSOFF 0xc0
>
> -#define RX_BUFFER_SIZE 1024
> -#define MAX_RX_URBS 4
> -#define MAX_TX_URBS 16 /* must be power of 2 */
> +#define ESD_USB_RX_BUFFER_SIZE 1024
> +#define ESD_USB_MAX_RX_URBS 4
> +#define ESD_USB_MAX_TX_URBS 16 /* must be power of 2 */
>
> struct header_msg {
> u8 len; /* len is always the total message length in 32bit words */
> @@ -156,7 +156,7 @@ struct id_filter_msg {
> u8 cmd;
> u8 net;
> u8 option;
> - __le32 mask[ESD_MAX_ID_SEGMENT + 1];
> + __le32 mask[ESD_USB_MAX_ID_SEGMENT + 1];
> };
>
> struct set_baudrate_msg {
> @@ -180,8 +180,8 @@ 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(ESD_USB_ESDGMBH_VENDOR_ID, ESD_USB_CANUSB2_PRODUCT_ID)},
> + {USB_DEVICE(ESD_USB_ESDGMBH_VENDOR_ID, ESD_USB_CANUSBM_PRODUCT_ID)},
> {}
> };
> MODULE_DEVICE_TABLE(usb, esd_usb_table);
> @@ -202,8 +202,8 @@ struct esd_usb {
> int net_count;
> u32 version;
> int rxinitdone;
> - void *rxbuf[MAX_RX_URBS];
> - dma_addr_t rxbuf_dma[MAX_RX_URBS];
> + void *rxbuf[ESD_USB_MAX_RX_URBS];
> + dma_addr_t rxbuf_dma[ESD_USB_MAX_RX_URBS];
> };
>
> struct esd_usb_net_priv {
> @@ -211,7 +211,7 @@ struct esd_usb_net_priv {
>
> atomic_t active_tx_jobs;
> struct usb_anchor tx_submitted;
> - struct esd_tx_urb_context tx_contexts[MAX_TX_URBS];
> + struct esd_tx_urb_context tx_contexts[ESD_USB_MAX_TX_URBS];
>
> struct esd_usb *usb;
> struct net_device *netdev;
> @@ -226,7 +226,7 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> struct net_device_stats *stats = &priv->netdev->stats;
> struct can_frame *cf;
> struct sk_buff *skb;
> - u32 id = le32_to_cpu(msg->rx.id) & ESD_IDMASK;
> + u32 id = le32_to_cpu(msg->rx.id) & ESD_USB_IDMASK;
>
> if (id == ESD_EV_CAN_ERROR_EXT) {
> u8 state = msg->rx.ev_can_err_ext.status;
> @@ -255,15 +255,15 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
>
> priv->old_state = state;
>
> - switch (state & ESD_BUSSTATE_MASK) {
> - case ESD_BUSSTATE_BUSOFF:
> + switch (state & ESD_USB_BUSSTATE_MASK) {
> + case ESD_USB_BUSSTATE_BUSOFF:
> new_state = CAN_STATE_BUS_OFF;
> can_bus_off(priv->netdev);
> break;
> - case ESD_BUSSTATE_WARN:
> + case ESD_USB_BUSSTATE_WARN:
> new_state = CAN_STATE_ERROR_WARNING;
> break;
> - case ESD_BUSSTATE_ERRPASSIVE:
> + case ESD_USB_BUSSTATE_ERRPASSIVE:
> new_state = CAN_STATE_ERROR_PASSIVE;
> break;
> default:
> @@ -285,14 +285,14 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
>
> cf->can_id |= CAN_ERR_PROT | CAN_ERR_BUSERROR;
>
> - switch (ecc & SJA1000_ECC_MASK) {
> - case SJA1000_ECC_BIT:
> + switch (ecc & ESD_USB_SJA1000_ECC_MASK) {
> + case ESD_USB_SJA1000_ECC_BIT:
> cf->data[2] |= CAN_ERR_PROT_BIT;
> break;
> - case SJA1000_ECC_FORM:
> + case ESD_USB_SJA1000_ECC_FORM:
> cf->data[2] |= CAN_ERR_PROT_FORM;
> break;
> - case SJA1000_ECC_STUFF:
> + case ESD_USB_SJA1000_ECC_STUFF:
> cf->data[2] |= CAN_ERR_PROT_STUFF;
> break;
> default:
> @@ -300,11 +300,11 @@ static void esd_usb_rx_event(struct esd_usb_net_priv *priv,
> }
>
> /* Error occurred during transmission? */
> - if (!(ecc & SJA1000_ECC_DIR))
> + if (!(ecc & ESD_USB_SJA1000_ECC_DIR))
> cf->data[2] |= CAN_ERR_PROT_TX;
>
> /* Bit stream position in CAN frame as the error was detected */
> - cf->data[3] = ecc & SJA1000_ECC_SEG;
> + cf->data[3] = ecc & ESD_USB_SJA1000_ECC_SEG;
> }
>
> if (skb) {
> @@ -331,7 +331,7 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
>
> id = le32_to_cpu(msg->rx.id);
>
> - if (id & ESD_EVENT) {
> + if (id & ESD_USB_EVENT) {
> esd_usb_rx_event(priv, msg);
> } else {
> skb = alloc_can_skb(priv->netdev, &cf);
> @@ -340,11 +340,11 @@ static void esd_usb_rx_can_msg(struct esd_usb_net_priv *priv,
> return;
> }
>
> - cf->can_id = id & ESD_IDMASK;
> + cf->can_id = id & ESD_USB_IDMASK;
> can_frame_set_cc_len(cf, msg->rx.dlc & ~ESD_RTR,
> priv->can.ctrlmode);
>
> - if (id & ESD_EXTID)
> + if (id & ESD_USB_EXTID)
> cf->can_id |= CAN_EFF_FLAG;
>
> if (msg->rx.dlc & ESD_RTR) {
> @@ -371,7 +371,7 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
> if (!netif_device_present(netdev))
> return;
>
> - context = &priv->tx_contexts[msg->txdone.hnd & (MAX_TX_URBS - 1)];
> + context = &priv->tx_contexts[msg->txdone.hnd & (ESD_USB_MAX_TX_URBS - 1)];
>
> if (!msg->txdone.status) {
> stats->tx_packets++;
> @@ -383,7 +383,7 @@ static void esd_usb_tx_done_msg(struct esd_usb_net_priv *priv,
> }
>
> /* Release context */
> - context->echo_index = MAX_TX_URBS;
> + context->echo_index = ESD_USB_MAX_TX_URBS;
> atomic_dec(&priv->active_tx_jobs);
>
> netif_wake_queue(netdev);
> @@ -418,7 +418,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
> msg = (union esd_usb_msg *)(urb->transfer_buffer + pos);
>
> switch (msg->hdr.cmd) {
> - case CMD_CAN_RX:
> + case ESD_USB_CMD_CAN_RX:
> if (msg->rx.net >= dev->net_count) {
> dev_err(dev->udev->dev.parent, "format error\n");
> break;
> @@ -427,7 +427,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
> esd_usb_rx_can_msg(dev->nets[msg->rx.net], msg);
> break;
>
> - case CMD_CAN_TX:
> + case ESD_USB_CMD_CAN_TX:
> if (msg->txdone.net >= dev->net_count) {
> dev_err(dev->udev->dev.parent, "format error\n");
> break;
> @@ -448,7 +448,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
>
> resubmit_urb:
> usb_fill_bulk_urb(urb, dev->udev, usb_rcvbulkpipe(dev->udev, 1),
> - urb->transfer_buffer, RX_BUFFER_SIZE,
> + urb->transfer_buffer, ESD_USB_RX_BUFFER_SIZE,
> esd_usb_read_bulk_callback, dev);
>
> retval = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -557,7 +557,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> if (dev->rxinitdone)
> return 0;
>
> - for (i = 0; i < MAX_RX_URBS; i++) {
> + for (i = 0; i < ESD_USB_MAX_RX_URBS; i++) {
> struct urb *urb = NULL;
> u8 *buf = NULL;
> dma_addr_t buf_dma;
> @@ -569,7 +569,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> break;
> }
>
> - buf = usb_alloc_coherent(dev->udev, RX_BUFFER_SIZE, GFP_KERNEL,
> + buf = usb_alloc_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE, GFP_KERNEL,
> &buf_dma);
> if (!buf) {
> dev_warn(dev->udev->dev.parent,
> @@ -582,7 +582,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
>
> usb_fill_bulk_urb(urb, dev->udev,
> usb_rcvbulkpipe(dev->udev, 1),
> - buf, RX_BUFFER_SIZE,
> + buf, ESD_USB_RX_BUFFER_SIZE,
> esd_usb_read_bulk_callback, dev);
> urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> usb_anchor_urb(urb, &dev->rx_submitted);
> @@ -590,7 +590,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> err = usb_submit_urb(urb, GFP_KERNEL);
> if (err) {
> usb_unanchor_urb(urb);
> - usb_free_coherent(dev->udev, RX_BUFFER_SIZE, buf,
> + usb_free_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE, buf,
> urb->transfer_dma);
> goto freeurb;
> }
> @@ -612,7 +612,7 @@ static int esd_usb_setup_rx_urbs(struct esd_usb *dev)
> }
>
> /* Warn if we've couldn't transmit all the URBs */
> - if (i < MAX_RX_URBS) {
> + if (i < ESD_USB_MAX_RX_URBS) {
> dev_warn(dev->udev->dev.parent,
> "rx performance may be slow\n");
> }
> @@ -647,14 +647,14 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)
> * the number of the starting bitmask (0..64) to the filter.option
> * field followed by only some bitmasks.
> */
> - msg->hdr.cmd = CMD_IDADD;
> - msg->hdr.len = 2 + ESD_MAX_ID_SEGMENT;
> + msg->hdr.cmd = ESD_USB_CMD_IDADD;
> + msg->hdr.len = 2 + ESD_USB_MAX_ID_SEGMENT;
> msg->filter.net = priv->index;
> - msg->filter.option = ESD_ID_ENABLE; /* start with segment 0 */
> - for (i = 0; i < ESD_MAX_ID_SEGMENT; i++)
> + msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */
> + for (i = 0; i < ESD_USB_MAX_ID_SEGMENT; i++)
> msg->filter.mask[i] = cpu_to_le32(0xffffffff);
> /* enable 29bit extended IDs */
> - msg->filter.mask[ESD_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
> + msg->filter.mask[ESD_USB_MAX_ID_SEGMENT] = cpu_to_le32(0x00000001);
>
> err = esd_usb_send_msg(dev, msg);
> if (err)
> @@ -683,8 +683,8 @@ static void unlink_all_urbs(struct esd_usb *dev)
>
> usb_kill_anchored_urbs(&dev->rx_submitted);
>
> - for (i = 0; i < MAX_RX_URBS; ++i)
> - usb_free_coherent(dev->udev, RX_BUFFER_SIZE,
> + for (i = 0; i < ESD_USB_MAX_RX_URBS; ++i)
> + usb_free_coherent(dev->udev, ESD_USB_RX_BUFFER_SIZE,
> dev->rxbuf[i], dev->rxbuf_dma[i]);
>
> for (i = 0; i < dev->net_count; i++) {
> @@ -693,8 +693,8 @@ static void unlink_all_urbs(struct esd_usb *dev)
> usb_kill_anchored_urbs(&priv->tx_submitted);
> atomic_set(&priv->active_tx_jobs, 0);
>
> - for (j = 0; j < MAX_TX_URBS; j++)
> - priv->tx_contexts[j].echo_index = MAX_TX_URBS;
> + for (j = 0; j < ESD_USB_MAX_TX_URBS; j++)
> + priv->tx_contexts[j].echo_index = ESD_USB_MAX_TX_URBS;
> }
> }
> }
> @@ -760,7 +760,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
> msg = (union esd_usb_msg *)buf;
>
> msg->hdr.len = 3; /* minimal length */
> - msg->hdr.cmd = CMD_CAN_TX;
> + msg->hdr.cmd = ESD_USB_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);
> @@ -769,15 +769,15 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
> msg->tx.dlc |= ESD_RTR;
>
> if (cf->can_id & CAN_EFF_FLAG)
> - msg->tx.id |= cpu_to_le32(ESD_EXTID);
> + msg->tx.id |= cpu_to_le32(ESD_USB_EXTID);
>
> for (i = 0; i < cf->len; i++)
> msg->tx.data[i] = cf->data[i];
>
> msg->hdr.len += (cf->len + 3) >> 2;
>
> - for (i = 0; i < MAX_TX_URBS; i++) {
> - if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> + for (i = 0; i < ESD_USB_MAX_TX_URBS; i++) {
> + if (priv->tx_contexts[i].echo_index == ESD_USB_MAX_TX_URBS) {
> context = &priv->tx_contexts[i];
> break;
> }
> @@ -809,7 +809,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
> atomic_inc(&priv->active_tx_jobs);
>
> /* Slow down tx path */
> - if (atomic_read(&priv->active_tx_jobs) >= MAX_TX_URBS)
> + if (atomic_read(&priv->active_tx_jobs) >= ESD_USB_MAX_TX_URBS)
> netif_stop_queue(netdev);
>
> err = usb_submit_urb(urb, GFP_ATOMIC);
> @@ -859,18 +859,18 @@ static int esd_usb_close(struct net_device *netdev)
> return -ENOMEM;
>
> /* Disable all IDs (see esd_usb_start()) */
> - msg->hdr.cmd = CMD_IDADD;
> - msg->hdr.len = 2 + ESD_MAX_ID_SEGMENT;
> + msg->hdr.cmd = ESD_USB_CMD_IDADD;
> + msg->hdr.len = 2 + ESD_USB_MAX_ID_SEGMENT;
> msg->filter.net = priv->index;
> - msg->filter.option = ESD_ID_ENABLE; /* start with segment 0 */
> - for (i = 0; i <= ESD_MAX_ID_SEGMENT; i++)
> + msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */
> + for (i = 0; i <= ESD_USB_MAX_ID_SEGMENT; i++)
> msg->filter.mask[i] = 0;
> if (esd_usb_send_msg(priv->usb, msg) < 0)
> netdev_err(netdev, "sending idadd message failed\n");
>
> /* set CAN controller to reset mode */
> msg->hdr.len = 2;
> - msg->hdr.cmd = CMD_SETBAUD;
> + msg->hdr.cmd = ESD_USB_CMD_SETBAUD;
> msg->setbaud.net = priv->index;
> msg->setbaud.rsvd = 0;
> msg->setbaud.baud = cpu_to_le32(ESD_USB_NO_BAUDRATE);
> @@ -928,27 +928,27 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
> canbtr |= (bt->brp - 1) & (btc->brp_max - 1);
>
> if (le16_to_cpu(priv->usb->udev->descriptor.idProduct) ==
> - USB_CANUSBM_PRODUCT_ID)
> - sjw_shift = ESD_USBM_SJW_SHIFT;
> + ESD_USB_CANUSBM_PRODUCT_ID)
> + sjw_shift = ESD_USB_M_SJW_SHIFT;
> else
> - sjw_shift = ESD_USB2_SJW_SHIFT;
> + sjw_shift = ESD_USB_2_SJW_SHIFT;
>
> canbtr |= ((bt->sjw - 1) & (btc->sjw_max - 1))
> << sjw_shift;
> canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1)
> & (btc->tseg1_max - 1))
> - << ESD_USB2_TSEG1_SHIFT;
> + << ESD_USB_2_TSEG1_SHIFT;
> canbtr |= ((bt->phase_seg2 - 1) & (btc->tseg2_max - 1))
> - << ESD_USB2_TSEG2_SHIFT;
> + << ESD_USB_2_TSEG2_SHIFT;
> if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
> - canbtr |= ESD_USB2_3_SAMPLES;
> + canbtr |= ESD_USB_3_SAMPLES;
>
> msg = kmalloc(sizeof(*msg), GFP_KERNEL);
> if (!msg)
> return -ENOMEM;
>
> msg->hdr.len = 2;
> - msg->hdr.cmd = CMD_SETBAUD;
> + msg->hdr.cmd = ESD_USB_CMD_SETBAUD;
> msg->setbaud.net = priv->index;
> msg->setbaud.rsvd = 0;
> msg->setbaud.baud = cpu_to_le32(canbtr);
> @@ -994,7 +994,7 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
> int err = 0;
> int i;
>
> - netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> + netdev = alloc_candev(sizeof(*priv), ESD_USB_MAX_TX_URBS);
> if (!netdev) {
> dev_err(&intf->dev, "couldn't alloc candev\n");
> err = -ENOMEM;
> @@ -1006,8 +1006,8 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
> init_usb_anchor(&priv->tx_submitted);
> atomic_set(&priv->active_tx_jobs, 0);
>
> - for (i = 0; i < MAX_TX_URBS; i++)
> - priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> + for (i = 0; i < ESD_USB_MAX_TX_URBS; i++)
> + priv->tx_contexts[i].echo_index = ESD_USB_MAX_TX_URBS;
>
> priv->usb = dev;
> priv->netdev = netdev;
> @@ -1019,10 +1019,10 @@ static int esd_usb_probe_one_net(struct usb_interface *intf, int index)
> CAN_CTRLMODE_BERR_REPORTING;
>
> if (le16_to_cpu(dev->udev->descriptor.idProduct) ==
> - USB_CANUSBM_PRODUCT_ID)
> - priv->can.clock.freq = ESD_USBM_CAN_CLOCK;
> + ESD_USB_CANUSBM_PRODUCT_ID)
> + priv->can.clock.freq = ESD_USB_M_CAN_CLOCK;
> else {
> - priv->can.clock.freq = ESD_USB2_CAN_CLOCK;
> + priv->can.clock.freq = ESD_USB_2_CAN_CLOCK;
> priv->can.ctrlmode_supported |= CAN_CTRLMODE_3_SAMPLES;
> }
>
> @@ -1085,7 +1085,7 @@ static int esd_usb_probe(struct usb_interface *intf,
> }
>
> /* query number of CAN interfaces (nets) */
> - msg->hdr.cmd = CMD_VERSION;
> + msg->hdr.cmd = ESD_USB_CMD_VERSION;
> msg->hdr.len = 2;
> msg->version.rsvd = 0;
> msg->version.flags = 0;
> --
> 2.25.1
>
On 17.05.2023 21:22:46, Frank Jungclaus wrote:
> As suggested by Vincent Mailhol make use of existing kernel macros:
> - Use the unit suffixes from linux/units.h for the controller clock
> frequencies
> - Use the BIT() macro to set specific bits in some constants
> - Use CAN_MAX_DLEN (instead of directly using the value 8) for the
> maximum CAN payload length
>
> Additionally:
> - Spend some commenting for the previously changed constants
> - Add the current year to the copyright notice
> - While adding the header linux/units.h to the list of include files
> also sort that list alphabetically
>
> Suggested-by: Vincent MAILHOL <[email protected]>
> Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
> Signed-off-by: Frank Jungclaus <[email protected]>
> ---
> drivers/net/can/usb/esd_usb.c | 38 ++++++++++++++++++-----------------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index d33bac3a6c10..042dda98b3db 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -3,19 +3,20 @@
> * CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Micro
> *
> * Copyright (C) 2010-2012 esd electronic system design gmbh, Matthias Fuchs <[email protected]>
> - * Copyright (C) 2022 esd electronics gmbh, Frank Jungclaus <[email protected]>
> + * Copyright (C) 2022-2023 esd electronics gmbh, Frank Jungclaus <[email protected]>
> */
> +#include <linux/can.h>
> +#include <linux/can/dev.h>
> +#include <linux/can/error.h>
> +
> #include <linux/ethtool.h>
> -#include <linux/signal.h>
> -#include <linux/slab.h>
> #include <linux/module.h>
> #include <linux/netdevice.h>
> +#include <linux/signal.h>
> +#include <linux/slab.h>
> +#include <linux/units.h>
> #include <linux/usb.h>
>
> -#include <linux/can.h>
> -#include <linux/can/dev.h>
> -#include <linux/can/error.h>
> -
> MODULE_AUTHOR("Matthias Fuchs <[email protected]>");
> MODULE_AUTHOR("Frank Jungclaus <[email protected]>");
> MODULE_DESCRIPTION("CAN driver for esd electronics gmbh CAN-USB/2 and CAN-USB/Micro interfaces");
> @@ -27,8 +28,8 @@ MODULE_LICENSE("GPL v2");
> #define USB_CANUSBM_PRODUCT_ID 0x0011
>
> /* CAN controller clock frequencies */
> -#define ESD_USB2_CAN_CLOCK 60000000
> -#define ESD_USBM_CAN_CLOCK 36000000
> +#define ESD_USB2_CAN_CLOCK (60 * MEGA) /* Hz */
> +#define ESD_USBM_CAN_CLOCK (36 * MEGA) /* Hz */
>
> /* Maximum number of CAN nets */
> #define ESD_USB_MAX_NETS 2
> @@ -42,20 +43,21 @@ MODULE_LICENSE("GPL v2");
> #define CMD_IDADD 6 /* also used for IDADD_REPLY */
>
> /* esd CAN message flags - dlc field */
> -#define ESD_RTR 0x10
> +#define ESD_RTR BIT(4) /* 0x10 */
Nitpick, personal style preference, maintainability: For me the hex
constant is redundant information, and it's not checked by the compiler,
please remove it.
> +
>
> /* esd CAN message flags - id field */
> -#define ESD_EXTID 0x20000000
> -#define ESD_EVENT 0x40000000
> -#define ESD_IDMASK 0x1fffffff
> +#define ESD_EXTID BIT(29) /* 0x20000000 */
> +#define ESD_EVENT BIT(30) /* 0x40000000 */
> +#define ESD_IDMASK 0x1fffffff
Please use GEN_MASK.
>
> /* esd CAN event ids */
> #define ESD_EV_CAN_ERROR_EXT 2 /* CAN controller specific diagnostic data */
>
> /* baudrate message flags */
> -#define ESD_USB_UBR 0x80000000
> -#define ESD_USB_LOM 0x40000000
> -#define ESD_USB_NO_BAUDRATE 0x7fffffff
> +#define ESD_USB_LOM BIT(30) /* 0x40000000, Listen Only Mode */
> +#define ESD_USB_UBR BIT(31) /* 0x80000000, User Bit Rate (controller BTR) in bits 0..27 */
> +#define ESD_USB_NO_BAUDRATE 0x7fffffff /* bit rate unconfigured */
You might use GEN_MASK here, too.
regards,
Marc
--
Pengutronix e.K. | Marc Kleine-Budde |
Embedded Linux | https://www.pengutronix.de |
Vertretung Nürnberg | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-9 |