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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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