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

Hello Stéphane,

On Mon, 27 Jul 2026 12:08:08 +0200, Vincent Mailhol <[email protected]> wrote:

> Bonjour Stéphane,
> 
> On 23/07/2026 ac 09:29, Stéphane Grosjean wrote:
> > From: Stéphane Grosjean <[email protected]>  
> 
> 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 <[email protected]>
> > 
> > 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
> 
> 

Take a look at '[PATCH v9] can: usb: IXXAT USB-to-CAN adapters drivers' ([1]) and
'[PATCH v10 1/3] can: usb: IXXAT USB-to-CAN adapters drivers' ([2]) for
previous attempts (and some more history) to bring the IXXAT driver upstream
already addressing a lot of Vincent Mailhol comments/suggestions...

Sorry, no updates from my side as the project (and the access to the hardware)
for me ended some time ago...

Regards,
Peter

[1] https://lore.kernel.org/linux-can/[email protected]/T/#u
[2] https://lore.kernel.org/linux-can/[email protected]/T/#u