[PATCH v2 0/6] can: esd_usb: More preparation before supporting esd CAN-USB/3

[PATCH v2 0/6] can: esd_usb: More preparation before supporting esd CAN-USB/3

[Frank Jungclaus]

Apply another small batch of patches as preparation for adding support
of the newly available esd CAN-USB/3 to esd_usb.c.
---
* Changelog *

v1 -> v2:
* Make use of GENMASK() macro for ESD_USB_NO_BAUDRATE and
  ESD_USB_IDMASK
* Also use the BIT() macro for ESD_USB2_3_SAMPLES
* Removed comments with redundant hexadecimal values from
  BIT()-constants
* Reworded (shortened) the commit messages
* Changed the macro ESD_USB_3_SAMPLES to ESD_USB_TRIPLE_SAMPLES

v1:
* Link: https://lore.kernel.org/all/20230517192251.2405290-1-frank.jungclaus@esd.eu/

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: 833e24aeb4d9a4803af3b836464df01293ce9041
-- 
2.25.1

[PATCH v2 1/6] can: esd_usb: Make use of existing kernel macros

[Frank Jungclaus]

Make use of existing kernel macros:
- Use the unit suffixes from linux/units.h for the controller clock
frequencies
- Use the BIT() and the GENMASK() 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 <mailhol.vincent@wanadoo.fr>
Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Link: https://lore.kernel.org/all/CAMZ6RqKdg5YBufa0C+ttzJvoG=9yuti-8AmthCi4jBbd08JEtw@mail.gmail.com/
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Link: https://lore.kernel.org/all/20230518-grower-film-ea8b5f853f3e-mkl@pengutronix.de/
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 40 ++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index d33bac3a6c10..32354cfdf151 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 <socketcan@esd.eu>
- * Copyright (C) 2022 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
+ * Copyright (C) 2022-2023 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
  */
+#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 <socketcan@esd.eu>");
 MODULE_AUTHOR("Frank Jungclaus <frank.jungclaus@esd.eu>");
 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)
+
 
 /* esd CAN message flags - id field */
-#define ESD_EXTID		0x20000000
-#define ESD_EVENT		0x40000000
-#define ESD_IDMASK		0x1fffffff
+#define ESD_EXTID	BIT(29)
+#define ESD_EVENT	BIT(30)
+#define ESD_IDMASK	GENMASK(28, 0)
 
 /* 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	GENMASK(30, 0) /* bit rate unconfigured */
 
 /* bit timing CAN-USB/2 */
 #define ESD_USB2_TSEG1_MIN	1
@@ -70,7 +72,7 @@ MODULE_LICENSE("GPL v2");
 #define ESD_USB2_BRP_MIN	1
 #define ESD_USB2_BRP_MAX	1024
 #define ESD_USB2_BRP_INC	1
-#define ESD_USB2_3_SAMPLES	0x00800000
+#define ESD_USB2_3_SAMPLES	BIT(23)
 
 /* esd IDADD message */
 #define ESD_ID_ENABLE		0x80
@@ -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

Re: [PATCH v2 1/6] can: esd_usb: Make use of existing kernel macros

[Vincent MAILHOL]

Thanks for the patch.

On Sat. 20 May 2023 at 04:57, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> Make use of existing kernel macros:
> - Use the unit suffixes from linux/units.h for the controller clock
> frequencies
> - Use the BIT() and the GENMASK() 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 <mailhol.vincent@wanadoo.fr>
> Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
> Link: https://lore.kernel.org/all/CAMZ6RqKdg5YBufa0C+ttzJvoG=9yuti-8AmthCi4jBbd08JEtw@mail.gmail.com/
> Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> Link: https://lore.kernel.org/all/20230518-grower-film-ea8b5f853f3e-mkl@pengutronix.de/
> Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> ---
>  drivers/net/can/usb/esd_usb.c | 40 ++++++++++++++++++-----------------
>  1 file changed, 21 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index d33bac3a6c10..32354cfdf151 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 <socketcan@esd.eu>
> - * Copyright (C) 2022 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> + * Copyright (C) 2022-2023 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
>   */
> +#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 <socketcan@esd.eu>");
>  MODULE_AUTHOR("Frank Jungclaus <frank.jungclaus@esd.eu>");
>  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)
> +
>
>  /* esd CAN message flags - id field */
> -#define ESD_EXTID              0x20000000
> -#define ESD_EVENT              0x40000000
> -#define ESD_IDMASK             0x1fffffff
> +#define ESD_EXTID      BIT(29)
> +#define ESD_EVENT      BIT(30)
> +#define ESD_IDMASK     GENMASK(28, 0)
>
>  /* 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 */
                                     ^^^^^^^^^^

As pointented by Marc, no need for redundant comment with the hexadecimal value.

> +#define ESD_USB_NO_BAUDRATE    GENMASK(30, 0) /* bit rate unconfigured */
>
>  /* bit timing CAN-USB/2 */
>  #define ESD_USB2_TSEG1_MIN     1
> @@ -70,7 +72,7 @@ MODULE_LICENSE("GPL v2");
>  #define ESD_USB2_BRP_MIN       1
>  #define ESD_USB2_BRP_MAX       1024
>  #define ESD_USB2_BRP_INC       1
> -#define ESD_USB2_3_SAMPLES     0x00800000
> +#define ESD_USB2_3_SAMPLES     BIT(23)
>
>  /* esd IDADD message */
>  #define ESD_ID_ENABLE          0x80
> @@ -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
>

Re: [PATCH v2 1/6] can: esd_usb: Make use of existing kernel macros

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 4645 bytes --]

On 21.05.2023 18:16:13, Vincent MAILHOL wrote:
> Thanks for the patch.
> 
> On Sat. 20 May 2023 at 04:57, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > Make use of existing kernel macros:
> > - Use the unit suffixes from linux/units.h for the controller clock
> > frequencies
> > - Use the BIT() and the GENMASK() 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 <mailhol.vincent@wanadoo.fr>
> > Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
> > Link: https://lore.kernel.org/all/CAMZ6RqKdg5YBufa0C+ttzJvoG=9yuti-8AmthCi4jBbd08JEtw@mail.gmail.com/
> > Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
> > Link: https://lore.kernel.org/all/20230518-grower-film-ea8b5f853f3e-mkl@pengutronix.de/
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 40 ++++++++++++++++++-----------------
> >  1 file changed, 21 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index d33bac3a6c10..32354cfdf151 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 <socketcan@esd.eu>
> > - * Copyright (C) 2022 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> > + * Copyright (C) 2022-2023 esd electronics gmbh, Frank Jungclaus <frank.jungclaus@esd.eu>
> >   */
> > +#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 <socketcan@esd.eu>");
> >  MODULE_AUTHOR("Frank Jungclaus <frank.jungclaus@esd.eu>");
> >  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)
> > +
> >
> >  /* esd CAN message flags - id field */
> > -#define ESD_EXTID              0x20000000
> > -#define ESD_EVENT              0x40000000
> > -#define ESD_IDMASK             0x1fffffff
> > +#define ESD_EXTID      BIT(29)
> > +#define ESD_EVENT      BIT(30)
> > +#define ESD_IDMASK     GENMASK(28, 0)
> >
> >  /* 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 */
>                                      ^^^^^^^^^^
> 
> As pointented by Marc, no need for redundant comment with the hexadecimal value.

Fixed while applying.

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 2/6] can: esd_usb: Replace initializer macros used for struct can_bittiming_const

[Frank Jungclaus]

Replace the macros used to initialize the members of struct
can_bittiming_const with direct values. Then also use those struct
members to do the calculations in esd_usb2_set_bittiming().

Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 33 +++++++++++++--------------------
 1 file changed, 13 insertions(+), 20 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 32354cfdf151..2eecf352ec47 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -60,18 +60,10 @@ MODULE_LICENSE("GPL v2");
 #define ESD_USB_NO_BAUDRATE	GENMASK(30, 0) /* bit rate unconfigured */
 
 /* bit timing CAN-USB/2 */
-#define ESD_USB2_TSEG1_MIN	1
-#define ESD_USB2_TSEG1_MAX	16
 #define ESD_USB2_TSEG1_SHIFT	16
-#define ESD_USB2_TSEG2_MIN	1
-#define ESD_USB2_TSEG2_MAX	8
 #define ESD_USB2_TSEG2_SHIFT	20
-#define ESD_USB2_SJW_MAX	4
 #define ESD_USB2_SJW_SHIFT	14
 #define ESD_USBM_SJW_SHIFT	24
-#define ESD_USB2_BRP_MIN	1
-#define ESD_USB2_BRP_MAX	1024
-#define ESD_USB2_BRP_INC	1
 #define ESD_USB2_3_SAMPLES	BIT(23)
 
 /* esd IDADD message */
@@ -909,19 +901,20 @@ static const struct ethtool_ops esd_usb_ethtool_ops = {
 
 static const struct can_bittiming_const esd_usb2_bittiming_const = {
 	.name = "esd_usb2",
-	.tseg1_min = ESD_USB2_TSEG1_MIN,
-	.tseg1_max = ESD_USB2_TSEG1_MAX,
-	.tseg2_min = ESD_USB2_TSEG2_MIN,
-	.tseg2_max = ESD_USB2_TSEG2_MAX,
-	.sjw_max = ESD_USB2_SJW_MAX,
-	.brp_min = ESD_USB2_BRP_MIN,
-	.brp_max = ESD_USB2_BRP_MAX,
-	.brp_inc = ESD_USB2_BRP_INC,
+	.tseg1_min = 1,
+	.tseg1_max = 16,
+	.tseg2_min = 1,
+	.tseg2_max = 8,
+	.sjw_max = 4,
+	.brp_min = 1,
+	.brp_max = 1024,
+	.brp_inc = 1,
 };
 
 static int esd_usb2_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;
 	struct can_bittiming *bt = &priv->can.bittiming;
 	union esd_usb_msg *msg;
 	int err;
@@ -932,7 +925,7 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
 	if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
 		canbtr |= ESD_USB_LOM;
 
-	canbtr |= (bt->brp - 1) & (ESD_USB2_BRP_MAX - 1);
+	canbtr |= (bt->brp - 1) & (btc->brp_max - 1);
 
 	if (le16_to_cpu(priv->usb->udev->descriptor.idProduct) ==
 	    USB_CANUSBM_PRODUCT_ID)
@@ -940,12 +933,12 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
 	else
 		sjw_shift = ESD_USB2_SJW_SHIFT;
 
-	canbtr |= ((bt->sjw - 1) & (ESD_USB2_SJW_MAX - 1))
+	canbtr |= ((bt->sjw - 1) & (btc->sjw_max - 1))
 		<< sjw_shift;
 	canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1)
-		   & (ESD_USB2_TSEG1_MAX - 1))
+		   & (btc->tseg1_max - 1))
 		<< ESD_USB2_TSEG1_SHIFT;
-	canbtr |= ((bt->phase_seg2 - 1) & (ESD_USB2_TSEG2_MAX - 1))
+	canbtr |= ((bt->phase_seg2 - 1) & (btc->tseg2_max - 1))
 		<< ESD_USB2_TSEG2_SHIFT;
 	if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
 		canbtr |= ESD_USB2_3_SAMPLES;
-- 
2.25.1

Re: [PATCH v2 2/6] can: esd_usb: Replace initializer macros used for struct can_bittiming_const

[Vincent MAILHOL]

Thanks for the patch.

On Sat. 20 May 2023 at 04:57, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> Replace the macros used to initialize the members of struct
> can_bittiming_const with direct values. Then also use those struct
> members to do the calculations in esd_usb2_set_bittiming().
>
> Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
> Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> ---
>  drivers/net/can/usb/esd_usb.c | 33 +++++++++++++--------------------
>  1 file changed, 13 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> index 32354cfdf151..2eecf352ec47 100644
> --- a/drivers/net/can/usb/esd_usb.c
> +++ b/drivers/net/can/usb/esd_usb.c
> @@ -60,18 +60,10 @@ MODULE_LICENSE("GPL v2");
>  #define ESD_USB_NO_BAUDRATE    GENMASK(30, 0) /* bit rate unconfigured */
>
>  /* bit timing CAN-USB/2 */
> -#define ESD_USB2_TSEG1_MIN     1
> -#define ESD_USB2_TSEG1_MAX     16
>  #define ESD_USB2_TSEG1_SHIFT   16
> -#define ESD_USB2_TSEG2_MIN     1
> -#define ESD_USB2_TSEG2_MAX     8
>  #define ESD_USB2_TSEG2_SHIFT   20
> -#define ESD_USB2_SJW_MAX       4
>  #define ESD_USB2_SJW_SHIFT     14
>  #define ESD_USBM_SJW_SHIFT     24
> -#define ESD_USB2_BRP_MIN       1
> -#define ESD_USB2_BRP_MAX       1024
> -#define ESD_USB2_BRP_INC       1
>  #define ESD_USB2_3_SAMPLES     BIT(23)
>
>  /* esd IDADD message */
> @@ -909,19 +901,20 @@ static const struct ethtool_ops esd_usb_ethtool_ops = {
>
>  static const struct can_bittiming_const esd_usb2_bittiming_const = {
>         .name = "esd_usb2",
> -       .tseg1_min = ESD_USB2_TSEG1_MIN,
> -       .tseg1_max = ESD_USB2_TSEG1_MAX,
> -       .tseg2_min = ESD_USB2_TSEG2_MIN,
> -       .tseg2_max = ESD_USB2_TSEG2_MAX,
> -       .sjw_max = ESD_USB2_SJW_MAX,
> -       .brp_min = ESD_USB2_BRP_MIN,
> -       .brp_max = ESD_USB2_BRP_MAX,
> -       .brp_inc = ESD_USB2_BRP_INC,
> +       .tseg1_min = 1,
> +       .tseg1_max = 16,
> +       .tseg2_min = 1,
> +       .tseg2_max = 8,
> +       .sjw_max = 4,
> +       .brp_min = 1,
> +       .brp_max = 1024,
> +       .brp_inc = 1,
>  };
>
>  static int esd_usb2_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;

I initially suggested doing:

          const struct can_bittiming_const *btc = priv->can.bittiming_const;

But now that I think again about it, doing:

          const struct can_bittiming_const *btc = &esd_usb2_bittiming_const;

is slightly better as it will allow the compiler to fold the integer
constant expressions such as btc->brp_max - 1. The compiler is not
smart enough to figure out what values are held in
priv->can.bittiming_const at compile time.

Sorry for not figuring this the first time.

>         struct can_bittiming *bt = &priv->can.bittiming;
>         union esd_usb_msg *msg;
>         int err;
> @@ -932,7 +925,7 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
>         if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY)
>                 canbtr |= ESD_USB_LOM;
>
> -       canbtr |= (bt->brp - 1) & (ESD_USB2_BRP_MAX - 1);
> +       canbtr |= (bt->brp - 1) & (btc->brp_max - 1);
>
>         if (le16_to_cpu(priv->usb->udev->descriptor.idProduct) ==
>             USB_CANUSBM_PRODUCT_ID)
> @@ -940,12 +933,12 @@ static int esd_usb2_set_bittiming(struct net_device *netdev)
>         else
>                 sjw_shift = ESD_USB2_SJW_SHIFT;
>
> -       canbtr |= ((bt->sjw - 1) & (ESD_USB2_SJW_MAX - 1))
> +       canbtr |= ((bt->sjw - 1) & (btc->sjw_max - 1))
>                 << sjw_shift;
>         canbtr |= ((bt->prop_seg + bt->phase_seg1 - 1)
> -                  & (ESD_USB2_TSEG1_MAX - 1))
> +                  & (btc->tseg1_max - 1))
>                 << ESD_USB2_TSEG1_SHIFT;
> -       canbtr |= ((bt->phase_seg2 - 1) & (ESD_USB2_TSEG2_MAX - 1))
> +       canbtr |= ((bt->phase_seg2 - 1) & (btc->tseg2_max - 1))
>                 << ESD_USB2_TSEG2_SHIFT;
>         if (priv->can.ctrlmode & CAN_CTRLMODE_3_SAMPLES)
>                 canbtr |= ESD_USB2_3_SAMPLES;
> --
> 2.25.1
>

Re: [PATCH v2 2/6] can: esd_usb: Replace initializer macros used for struct can_bittiming_const

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 3568 bytes --]

On 21.05.2023 18:16:17, Vincent MAILHOL wrote:
> Thanks for the patch.
> 
> On Sat. 20 May 2023 at 04:57, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> > Replace the macros used to initialize the members of struct
> > can_bittiming_const with direct values. Then also use those struct
> > members to do the calculations in esd_usb2_set_bittiming().
> >
> > Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
> > Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
> > Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
> > ---
> >  drivers/net/can/usb/esd_usb.c | 33 +++++++++++++--------------------
> >  1 file changed, 13 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
> > index 32354cfdf151..2eecf352ec47 100644
> > --- a/drivers/net/can/usb/esd_usb.c
> > +++ b/drivers/net/can/usb/esd_usb.c
> > @@ -60,18 +60,10 @@ MODULE_LICENSE("GPL v2");
> >  #define ESD_USB_NO_BAUDRATE    GENMASK(30, 0) /* bit rate unconfigured */
> >
> >  /* bit timing CAN-USB/2 */
> > -#define ESD_USB2_TSEG1_MIN     1
> > -#define ESD_USB2_TSEG1_MAX     16
> >  #define ESD_USB2_TSEG1_SHIFT   16
> > -#define ESD_USB2_TSEG2_MIN     1
> > -#define ESD_USB2_TSEG2_MAX     8
> >  #define ESD_USB2_TSEG2_SHIFT   20
> > -#define ESD_USB2_SJW_MAX       4
> >  #define ESD_USB2_SJW_SHIFT     14
> >  #define ESD_USBM_SJW_SHIFT     24
> > -#define ESD_USB2_BRP_MIN       1
> > -#define ESD_USB2_BRP_MAX       1024
> > -#define ESD_USB2_BRP_INC       1
> >  #define ESD_USB2_3_SAMPLES     BIT(23)
> >
> >  /* esd IDADD message */
> > @@ -909,19 +901,20 @@ static const struct ethtool_ops esd_usb_ethtool_ops = {
> >
> >  static const struct can_bittiming_const esd_usb2_bittiming_const = {
> >         .name = "esd_usb2",
> > -       .tseg1_min = ESD_USB2_TSEG1_MIN,
> > -       .tseg1_max = ESD_USB2_TSEG1_MAX,
> > -       .tseg2_min = ESD_USB2_TSEG2_MIN,
> > -       .tseg2_max = ESD_USB2_TSEG2_MAX,
> > -       .sjw_max = ESD_USB2_SJW_MAX,
> > -       .brp_min = ESD_USB2_BRP_MIN,
> > -       .brp_max = ESD_USB2_BRP_MAX,
> > -       .brp_inc = ESD_USB2_BRP_INC,
> > +       .tseg1_min = 1,
> > +       .tseg1_max = 16,
> > +       .tseg2_min = 1,
> > +       .tseg2_max = 8,
> > +       .sjw_max = 4,
> > +       .brp_min = 1,
> > +       .brp_max = 1024,
> > +       .brp_inc = 1,
> >  };
> >
> >  static int esd_usb2_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;
> 
> I initially suggested doing:
> 
>           const struct can_bittiming_const *btc = priv->can.bittiming_const;
> 
> But now that I think again about it, doing:
> 
>           const struct can_bittiming_const *btc = &esd_usb2_bittiming_const;
> 
> is slightly better as it will allow the compiler to fold the integer
> constant expressions such as btc->brp_max - 1. The compiler is not
> smart enough to figure out what values are held in
> priv->can.bittiming_const at compile time.
> 
> Sorry for not figuring this the first time.

Good suggestion! Fixed up while applying.

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   |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[PATCH v2 3/6] can: esd_usb: Use consistent prefixes for macros

[Frank Jungclaus]

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.
Change the macro ESD_USB_3_SAMPLES to ESD_USB_TRIPLE_SAMPLES to not
mix up with the prefix ESD_USB_3_ which will be introduced for the
CAN-USB/3 device.

Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 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 2eecf352ec47..9b0ed07911e1 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)
 
 
 /* esd CAN message flags - id field */
-#define ESD_EXTID	BIT(29)
-#define ESD_EVENT	BIT(30)
-#define ESD_IDMASK	GENMASK(28, 0)
+#define ESD_USB_EXTID	BIT(29)
+#define ESD_USB_EVENT	BIT(30)
+#define ESD_USB_IDMASK	GENMASK(28, 0)
 
 /* 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	GENMASK(30, 0) /* 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	BIT(23)
+/* 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_TRIPLE_SAMPLES	BIT(23)
 
 /* 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_TRIPLE_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

[PATCH v2 4/6] can: esd_usb: Prefix all structures with the device name

[Frank Jungclaus]

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 <mailhol.vincent@wanadoo.fr>
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 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 9b0ed07911e1..24eebb7ee5f1 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

[PATCH v2 5/6] can: esd_usb: Replace hardcoded message length given to USB commands

[Frank Jungclaus]

Replace all hardcoded values supplied to the len element of
esd_usb_msg (and its siblings) by more readable expressions, based on
sizeof(), offsetof(), etc.
Also spend documentation / comments that the len element of esd_usb_msg
is in multiples of 32bit words and not in bytes.

Link: https://lore.kernel.org/all/CAMZ6RqLaDNy-fZ2G0+QMhUEckkXLL+ZyELVSDFmqpd++aBzZQg@mail.gmail.com/
Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 drivers/net/can/usb/esd_usb.c | 40 ++++++++++++++++++-----------------
 1 file changed, 21 insertions(+), 19 deletions(-)

diff --git a/drivers/net/can/usb/esd_usb.c b/drivers/net/can/usb/esd_usb.c
index 24eebb7ee5f1..a6a3ecb6eac6 100644
--- a/drivers/net/can/usb/esd_usb.c
+++ b/drivers/net/can/usb/esd_usb.c
@@ -90,13 +90,13 @@ MODULE_LICENSE("GPL v2");
 #define ESD_USB_MAX_TX_URBS		16 /* must be power of 2 */
 
 struct esd_usb_header_msg {
-	u8 len; /* len is always the total message length in 32bit words */
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 rsvd[2];
 };
 
 struct esd_usb_version_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 rsvd;
 	u8 flags;
@@ -104,7 +104,7 @@ struct esd_usb_version_msg {
 };
 
 struct esd_usb_version_reply_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 nets;
 	u8 features;
@@ -115,7 +115,7 @@ struct esd_usb_version_reply_msg {
 };
 
 struct esd_usb_rx_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 net;
 	u8 dlc;
@@ -133,7 +133,7 @@ struct esd_usb_rx_msg {
 };
 
 struct esd_usb_tx_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 net;
 	u8 dlc;
@@ -143,7 +143,7 @@ struct esd_usb_tx_msg {
 };
 
 struct esd_usb_tx_done_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 net;
 	u8 status;
@@ -152,15 +152,15 @@ struct esd_usb_tx_done_msg {
 };
 
 struct esd_usb_id_filter_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 net;
 	u8 option;
-	__le32 mask[ESD_USB_MAX_ID_SEGMENT + 1];
+	__le32 mask[ESD_USB_MAX_ID_SEGMENT + 1]; /* +1 for 29bit extended IDs */
 };
 
 struct esd_usb_set_baudrate_msg {
-	u8 len;
+	u8 len; /* total message length in 32bit words */
 	u8 cmd;
 	u8 net;
 	u8 rsvd;
@@ -438,7 +438,7 @@ static void esd_usb_read_bulk_callback(struct urb *urb)
 			break;
 		}
 
-		pos += msg->hdr.len << 2;
+		pos += msg->hdr.len * sizeof(u32); /* convert to # of bytes */
 
 		if (pos > urb->actual_length) {
 			dev_err(dev->udev->dev.parent, "format error\n");
@@ -532,7 +532,7 @@ static int esd_usb_send_msg(struct esd_usb *dev, union esd_usb_msg *msg)
 	return usb_bulk_msg(dev->udev,
 			    usb_sndbulkpipe(dev->udev, 2),
 			    msg,
-			    msg->hdr.len << 2,
+			    msg->hdr.len * sizeof(u32), /* convert to # of bytes */
 			    &actual_length,
 			    1000);
 }
@@ -648,7 +648,7 @@ static int esd_usb_start(struct esd_usb_net_priv *priv)
 	 * field followed by only some bitmasks.
 	 */
 	msg->hdr.cmd = ESD_USB_CMD_IDADD;
-	msg->hdr.len = 2 + ESD_USB_MAX_ID_SEGMENT;
+	msg->hdr.len = sizeof(struct esd_usb_id_filter_msg) / sizeof(u32); /* # of 32bit words */
 	msg->filter.net = priv->index;
 	msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */
 	for (i = 0; i < ESD_USB_MAX_ID_SEGMENT; i++)
@@ -759,7 +759,8 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 
 	msg = (union esd_usb_msg *)buf;
 
-	msg->hdr.len = 3; /* minimal length */
+	/* minimal length as # of 32bit words */
+	msg->hdr.len = offsetof(struct esd_usb_tx_msg, data) / sizeof(u32);
 	msg->hdr.cmd = ESD_USB_CMD_CAN_TX;
 	msg->tx.net = priv->index;
 	msg->tx.dlc = can_get_cc_dlc(cf, priv->can.ctrlmode);
@@ -774,7 +775,8 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 	for (i = 0; i < cf->len; i++)
 		msg->tx.data[i] = cf->data[i];
 
-	msg->hdr.len += (cf->len + 3) >> 2;
+	/* round up, then divide by 4 to add the payload length as # of 32bit words */
+	msg->hdr.len += DIV_ROUND_UP(cf->len, sizeof(u32));
 
 	for (i = 0; i < ESD_USB_MAX_TX_URBS; i++) {
 		if (priv->tx_contexts[i].echo_index == ESD_USB_MAX_TX_URBS) {
@@ -797,7 +799,7 @@ static netdev_tx_t esd_usb_start_xmit(struct sk_buff *skb,
 	msg->tx.hnd = 0x80000000 | i; /* returned in TX done message */
 
 	usb_fill_bulk_urb(urb, dev->udev, usb_sndbulkpipe(dev->udev, 2), buf,
-			  msg->hdr.len << 2,
+			  msg->hdr.len * sizeof(u32), /* convert to # of bytes */
 			  esd_usb_write_bulk_callback, context);
 
 	urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
@@ -860,7 +862,7 @@ static int esd_usb_close(struct net_device *netdev)
 
 	/* Disable all IDs (see esd_usb_start()) */
 	msg->hdr.cmd = ESD_USB_CMD_IDADD;
-	msg->hdr.len = 2 + ESD_USB_MAX_ID_SEGMENT;
+	msg->hdr.len = sizeof(struct esd_usb_id_filter_msg) / sizeof(u32);/* # of 32bit words */
 	msg->filter.net = priv->index;
 	msg->filter.option = ESD_USB_ID_ENABLE; /* start with segment 0 */
 	for (i = 0; i <= ESD_USB_MAX_ID_SEGMENT; i++)
@@ -869,7 +871,7 @@ static int esd_usb_close(struct net_device *netdev)
 		netdev_err(netdev, "sending idadd message failed\n");
 
 	/* set CAN controller to reset mode */
-	msg->hdr.len = 2;
+	msg->hdr.len = sizeof(struct esd_usb_set_baudrate_msg) / sizeof(u32); /* # of 32bit words */
 	msg->hdr.cmd = ESD_USB_CMD_SETBAUD;
 	msg->setbaud.net = priv->index;
 	msg->setbaud.rsvd = 0;
@@ -947,7 +949,7 @@ static int esd_usb_2_set_bittiming(struct net_device *netdev)
 	if (!msg)
 		return -ENOMEM;
 
-	msg->hdr.len = 2;
+	msg->hdr.len = sizeof(struct esd_usb_set_baudrate_msg) / sizeof(u32); /* # of 32bit words */
 	msg->hdr.cmd = ESD_USB_CMD_SETBAUD;
 	msg->setbaud.net = priv->index;
 	msg->setbaud.rsvd = 0;
@@ -1086,7 +1088,7 @@ static int esd_usb_probe(struct usb_interface *intf,
 
 	/* query number of CAN interfaces (nets) */
 	msg->hdr.cmd = ESD_USB_CMD_VERSION;
-	msg->hdr.len = 2;
+	msg->hdr.len = sizeof(struct esd_usb_version_msg) / sizeof(u32); /* # of 32bit words */
 	msg->version.rsvd = 0;
 	msg->version.flags = 0;
 	msg->version.drv_version = 0;
-- 
2.25.1

[PATCH v2 6/6] can: esd_usb: Don't bother the user with nonessential log message

[Frank Jungclaus]

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/20230509-superglue-hazy-38108aa66bfa-mkl@pengutronix.de/
Suggested-by: Marc Kleine-Budde <mkl@pengutronix.de>
Suggested-by: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
Signed-off-by: Frank Jungclaus <frank.jungclaus@esd.eu>
---
 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 a6a3ecb6eac6..4c0da3ff3a9d 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

Re: [PATCH v2 0/6] can: esd_usb: More preparation before supporting esd CAN-USB/3

[Vincent MAILHOL]

Hi Frank,

On Sat. 20 mai 2023 à 04:57, Frank Jungclaus <frank.jungclaus@esd.eu> wrote:
> Apply another small batch of patches as preparation for adding support
> of the newly available esd CAN-USB/3 to esd_usb.c.
> ---

I sent two nitpicks but aside from that the series looks good. Thank
you for taking time to clean-up the existing code before introducing
the new changes.

I do not think I will need to review the v3, so in advance:

Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr>