Re: [PATCH 1/4] can: usb: ixxat_usb: add shared header

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Low] Mathematically incorrect formula in timestamp documentation.
- [Low] Struct field documentation mismatch and missing kernel-doc features.
- [Low] Duplicate USB Product ID macro values.
--

--- Patch [1]: [PATCH 1/4] can: usb: ixxat_usb: add shared header ---
commit 9b5608c77a4fdbba5ae14afe4c9f4c448d769837
Author: Stéphane Grosjean <[email protected]>

can: usb: ixxat_usb: add shared header

Add ixxat_usb_core.h, which defines the data structures, constants and
USB wire-protocol layouts shared by all three IXXAT USB communication
layers (CL1, CL2 and CAN IDM).

> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h
> new file mode 100644
> index 0000000000000..62799d8040b1c
> --- /dev/null
> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_core.h

[ ... ]

> +/* supported device ids: CL2 */
> +#define USB2CAN_FD_COMPACT_PRODUCT_ID		0x0014
> +#define USB2CAN_FD_EMBEDDED_PRODUCT_ID		0x0015
> +#define USB2CAN_FD_AUTOMOTIVE_PRODUCT_ID	0x0017
> +#define USB2CAN_FD_PCIE_MINI_PRODUCT_ID		0x001B
> +#define USB2CAR_PRODUCT_ID			0x001C
> +#define CAN_IDM101_PRODUCT_ID			0xFF12
> +#define CAN_IDM200_PRODUCT_ID			0xFF13
> +
> +/* supported device ids: CL2 */
> +#define USB2CAN_FD_PRO_PRODUCT_ID		0x0010
> +#define USB2CAN_FD_STANDARD_PRODUCT_ID		0x0011
> +#define USB2CAN_FD_STANDARD_CARD_PRODUCT_ID	0x0012
> +#define USB2CAN_FD_PRO_MODULE_PRODUCT_ID	0x0013
> +#define USB2CAN_FD_STANDARD_MODULE_PRODUCT_ID	0x0014

[Severity: Low]
Is this duplication intentional? Both USB2CAN_FD_COMPACT_PRODUCT_ID and
USB2CAN_FD_STANDARD_MODULE_PRODUCT_ID are assigned the same constant 0x0014,
which might create device ID collisions.

[ ... ]

> + * To correlate the start timestamp to the host clock we determine:
> + *
> + *     t_host_C = (t_host_B + t_host_A) / 2
> + *              = t_host_A + ((t_host_A - t_host_B) / 2)

[Severity: Low]
Is the mathematical simplification in this documentation correct?

The formula t_host_A + ((t_host_A - t_host_B) / 2) simplifies to
1.5 * t_host_A - 0.5 * t_host_B, not the mathematical midpoint.

Should it be t_host_A + ((t_host_B - t_host_A) / 2) instead?

[ ... ]

> +/* struct ixxat_tx_urb_context URB content for transmission
> + * @dev: pointer to the IXXAT USB CAN device
> + * @urb: USB request block
> + * @urb_index: index of this URB (used to mark the context as occupied)
> + * @msg_index: index of message (client_id)
> + * @msg_packet_len: Data length code (only used if no loopback is enabled)
> + * @msg_packet_no: number of packets (only used if no loopback is enabled)
> + *
> + * Contains content for USB request block transmissions
> + */
> +struct ixxat_tx_urb_context {
> +	struct ixxat_usb_candevice *dev;
> +	struct urb *urb;
> +	u16  urb_index;
> +	u16  msg_index;
> +};

[Severity: Low]
Are these docs out of date? The fields @msg_packet_len and @msg_packet_no
are documented but missing from the structure definition.

Also, it looks like this header uses standard types such as spinlock_t,
atomic_t, struct urb, and struct can_priv without including their respective
headers. Does it need the appropriate #include directives to be self-contained?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1