Re: [PATCH 1/3] usb: misc: Add Intel USBIO bridge driver

Greg Kroah-Hartman
2025-08-09T14:28:53Z

On Sat, Aug 09, 2025 at 12:23:24PM +0200, Hans de Goede wrote:
> +struct usbio_protver {
> + uint8_t ver;

Nit, but you do this everywhere. Kernel types are "u8", not "uint8_t",
that's a userspace C type. Please use the correct ones when writing
kernel code.

> +} __packed;
> +
> +struct usbio_fwver {
> + uint8_t major;
> + uint8_t minor;
> + uint16_t patch;
> + uint16_t build;

What is the endian of these u16 variables?

> +/* USBIO Packet Header */
> +struct usbio_packet_header {
> + uint8_t type;
> + uint8_t cmd;
> + uint8_t flags;

Are these crossing the user/kernel boundry? I think so (same with
above), and so shouldn't they use the proper types (__u8)?

> +} __packed;
> +
> +/* USBIO Control Transfer Packet */
> +struct usbio_ctrl_packet {
> + struct usbio_packet_header header;
> + uint8_t len;
> + uint8_t data[] __counted_by(len);

Same here.

> +} __packed;
> +
> +/* USBIO Bulk Transfer Packet */
> +struct usbio_bulk_packet {
> + struct usbio_packet_header header;
> + uint16_t len;

Endian-ness of len?

> + uint8_t data[] __counted_by(len);
> +} __packed;
> +
> +/* USBIO GPIO commands */
> +enum usbio_gpio_cmd {
> + USBIO_GPIOCMD_DEINIT,
> + USBIO_GPIOCMD_INIT,
> + USBIO_GPIOCMD_READ,
> + USBIO_GPIOCMD_WRITE,
> + USBIO_GPIOCMD_END

No specific value of these enums?

> +};
> +
> +/* USBIO GPIO config */
> +enum usbio_gpio_pincfg {
> + USBIO_GPIO_PINCFG_DEFAULT,
> + USBIO_GPIO_PINCFG_PULLUP,
> + USBIO_GPIO_PINCFG_PULLDOWN,
> + USBIO_GPIO_PINCFG_PUSHPULL

Same here, no specific values?

> +};
> +
> +#define USBIO_GPIO_PINCFG_SHIFT 2
> +#define USBIO_GPIO_PINCFG_MASK (0x3 << USBIO_GPIO_PINCFG_SHIFT)
> +#define USBIO_GPIO_SET_PINCFG(pincfg) \
> + (((pincfg) << USBIO_GPIO_PINCFG_SHIFT) & USBIO_GPIO_PINCFG_MASK)
> +
> +enum usbio_gpio_pinmode {
> + USBIO_GPIO_PINMOD_INVAL,
> + USBIO_GPIO_PINMOD_INPUT,
> + USBIO_GPIO_PINMOD_OUTPUT,
> + USBIO_GPIO_PINMOD_MAXVAL

And here?

> +};
> +
> +#define USBIO_GPIO_PINMOD_MASK 0x3
> +#define USBIO_GPIO_SET_PINMOD(pin) (pin & USBIO_GPIO_PINMOD_MASK)
> +
> +/*************************
> + * USBIO GPIO Controller *
> + *************************/
> +
> +#define USBIO_MAX_GPIOBANKS 5
> +#define USBIO_GPIOSPERBANK 32
> +
> +struct usbio_gpio_bank_desc {
> + uint8_t id;
> + uint8_t pins;
> + uint32_t bmap;

endian?

> +} __packed;
> +
> +struct usbio_gpio_init {
> + u8 bankid;
> + u8 config;
> + u8 pincount;
> + u8 pin;

Now you use "u8" :)

> +} __packed;
> +
> +struct usbio_gpio_rw {
> + u8 bankid;
> + u8 pincount;
> + u8 pin;
> + u32 value;

endian?

> +} __packed;
> +
> +/* USBIO I2C commands */
> +enum usbio_i2c_cmd {
> + USBIO_I2CCMD_UNINIT,
> + USBIO_I2CCMD_INIT,
> + USBIO_I2CCMD_READ,
> + USBIO_I2CCMD_WRITE,
> + USBIO_I2CCMD_END

No specific values?

> +};
> +
> +/************************
> + * USBIO I2C Controller *
> + ************************/
> +
> +#define USBIO_MAX_I2CBUSES 5
> +
> +#define USBIO_I2C_BUS_ADDR_CAP_10B BIT(3) /* 10bit address support */
> +#define USBIO_I2C_BUS_MODE_CAP_MASK 0x3
> +#define USBIO_I2C_BUS_MODE_CAP_SM 0 /* Standard Mode */
> +#define USBIO_I2C_BUS_MODE_CAP_FM 1 /* Fast Mode */
> +#define USBIO_I2C_BUS_MODE_CAP_FMP 2 /* Fast Mode+ */
> +#define USBIO_I2C_BUS_MODE_CAP_HSM 3 /* High-Speed Mode */
> +
> +struct usbio_i2c_bus_desc {
> + uint8_t id;
> + uint8_t caps;
> +} __packed;
> +
> +struct usbio_i2c_uninit {
> + u8 busid;
> + u16 config;
> +} __packed;

You are using both types, again, please make everything "u8" and
friends.

And again, endian? Same for the rest of this .h file.

thanks,

greg k-h