Re: [PATCH 0/4] can: usb: Add driver for HMS IXXAT USB-to-CAN adapters

[Vincent Mailhol <mailhol@kernel.org>]

Bonjour Stéphane,

On 23/07/2026 ac 09:29, Stéphane Grosjean wrote:
> From: Stéphane Grosjean <s.grosjean@peak-system.fr>

Could you put the maintainers in Cc? Use either

  ./scripts/get_maintainer.pl

or switch to the more modern b4 tool which will handle all this
automatically for you.

  https://b4.docs.kernel.org/en/latest/

I am asking because I do not put the same focus on e-mails sent to the
mailing list as I do for e-mails sent directly to me.

> This series adds a SocketCAN driver for the HMS Networks IXXAT family of
> USB-to-CAN and USB-to-CAN FD interface adapters.
> 
> The driver supports two USB communication layers:
> - CL1: the protocol for classic CAN 2.0b devices with legacy firmware
> - CL2: the newer protocol used by recent device firmware versions
> 
> Supported devices (VID 0x08d8 and 0x08db):
>   USB-to-CAN Compact / Embedded / Professional / Automotive / Plugin (CL1)
>   USB-to-CAN FD Compact / Professional / Automotive / MiniPCIe       (CL2)
>   USB-to-CAN/FD Pro / Standard / Standard Card / Pro Module /
>                       Standard Module                                (CL2)
>   USB-to-CAR, CAN-IDM101, CAN-IDM200                                 (CL2)
> 
> Supported CAN control modes (device-dependent):
>   listen-only, loopback, triple-sampling, bus-error reporting,
>   CAN FD (ISO 11898-1:2015), non-ISO CAN FD, one-shot
> 
> Additional features:
>   - Hardware receive timestamps propagated via skb_hwtstamps
>   - Bus error counter (do_get_berr_counter)
>   - TX echo via the SocketCAN echo skb mechanism (up to 32 in flight)
>   - Up to 5 independent CAN channels per physical device
>   - Automatic protocol layer selection based on firmware version
> 
> Signed-off-by: Stéphane Grosjean <s.grosjean@peak-system.fr>
> 
> Stéphane Grosjean (4):
>   can: usb: ixxat_usb: add shared header
>   can: usb: ixxat_usb: add CL1 legacy protocol layer
>   can: usb: ixxat_usb: add CL2/V2 protocol layer with CAN
>   can: usb: ixxat_usb: add core driver and Kconfig/Makefile

I think that the series is upside-down. We should be able to compile
each individual patch.

You should start by adding relevant core portion of the header and the
core driver but with an empty

  struct usb_device_id

table. And then add the different protocols while updating the core and
the header accordingly of the new needs.

>  drivers/net/can/usb/Kconfig                   |   17 +
>  drivers/net/can/usb/Makefile                  |    1 +
>  drivers/net/can/usb/ixxat_usb/Makefile        |    3 +
>  drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c |  188 ++
>  drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c |  347 +++
>  .../net/can/usb/ixxat_usb/ixxat_usb_core.c    | 2567 +++++++++++++++++
>  .../net/can/usb/ixxat_usb/ixxat_usb_core.h    |  862 ++++++
>  7 files changed, 3985 insertions(+)
>  create mode 100644 drivers/net/can/usb/ixxat_usb/Makefile
>  create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_cl1.c
>  create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
>  create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c
>  create mode 100644 drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h

A few other global comments:

  - You are doing a lot of open coded mask manipulations. For example:
    IXXAT_USB_GET_BUSTYPE() or IXXAT_USB_DECODE_DLC(). Please define a
    mask with GENMASK() (or one of its GENMASK_U*() variants) and use
    FIELD_GET() and FIELD_PREP to get and set the values.

  - When you initialize a static const struct, directly put the literal
    value. It doesn't improve the readability to have an intermediate
    macro. For example do:

	static const struct can_bittiming_const usb2can_bt = {
		.name = KBUILD_MODNAME,
		.tseg1_min = 1,
		.tseg1_max = 16,
		.tseg2_min = 1,
		.tseg2_max = 8,
		.sjw_max = 4,
		.brp_min = 1,
		.brp_max = 64,
		.brp_inc = 1,
	};

    instead of:

	static const struct can_bittiming_const usb2can_bt = {
		.name = KBUILD_MODNAME,
		.tseg1_min = IXXAT_USB2CAN_TSEG1_MIN,
		.tseg1_max = IXXAT_USB2CAN_TSEG1_MAX,
		.tseg2_min = IXXAT_USB2CAN_TSEG2_MIN,
		.tseg2_max = IXXAT_USB2CAN_TSEG2_MAX,
		.sjw_max = IXXAT_USB2CAN_SJW_MAX,
		.brp_min = IXXAT_USB2CAN_BRP_MIN,
		.brp_max = IXXAT_USB2CAN_BRP_MAX,
		.brp_inc = IXXAT_USB2CAN_BRP_INC,
	};



Yours sincerely,
Vincent Mailhol