Re: [PATCH 01/13] HID: ft260: add serial driver

[Linus Walleij]

Hi Michael, Christina,

thanks for your patch!

On Sat, Aug 22, 2026 at 11:40 PM Michael Zaidman
<michael.zaidman@xxxxxxxxx> wrote:

> From: Christina Quast <contact@xxxxxxxxxxxxxxxxxx>
>
> Add a TTY serial interface (/dev/ttyFTx) for FT260 USB HID devices,
> providing baud rate configuration, data transfer, and termios settings
> without requiring userspace HID report formatting.
>
> The FT260 exposes I2C and UART through separate HID interfaces. Only
> I2C was supported previously; UART required FTDI hidraw example code.
>
> This is a port of the v4 patch, adapted to the I2C hardening that has
> since been merged upstream: input-report length validation, the
> read_buf/raw_event race fix with read_lock, and SMBus block-read
> START/STOP handling. HID_FT260 now depends on TTY because the UART code
> is part of this driver.
>
> Routing UART reports through ft260_raw_event() also made v4 fall through
> the report dispatch after an I2C input report had already been consumed,
> so every I2C input report was additionally reported as unhandled. This
> regression is fixed here.
>
> Ported from:
> https://lore.kernel.org/all/20231218093153.192268-1-contact@xxxxxxxxxxxxxxxxxx/
>
> The follow-up v5 series stalled without being applied:
> https://lore.kernel.org/linux-input/20240216-ft260_review5-v5-1-36db44673ac7@xxxxxxxxxxxxxxxxxx/
>
> Signed-off-by: Christina Quast <contact@xxxxxxxxxxxxxxxxxx>
> Co-developed-by: Daniel Beer <daniel.beer@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Daniel Beer <daniel.beer@xxxxxxxxxxxxxxxxx>
> Co-developed-by: Michael Zaidman <michael.zaidman@xxxxxxxxx>
> Signed-off-by: Michael Zaidman <michael.zaidman@xxxxxxxxx>
(...)
> +/* UART interface configuration */
> +enum {
> +       FT260_CFG_FLOW_CTRL_OFF         = 0x00,
> +       FT260_CFG_FLOW_CTRL_RTS_CTS     = 0x01,
> +       FT260_CFG_FLOW_CTRL_DTR_DSR     = 0x02,
> +       FT260_CFG_FLOW_CTRL_XON_XOFF    = 0x03,
> +       FT260_CFG_FLOW_CTRL_NONE        = 0x04,
> +
> +       FT260_CFG_DATA_BITS_7           = 0x07,
> +       FT260_CFG_DATA_BITS_8           = 0x08,
> +
> +       FT260_CFG_PAR_NO                = 0x00,
> +       FT260_CFG_PAR_ODD               = 0x01,
> +       FT260_CFG_PAR_EVEN              = 0x02,
> +       FT260_CFG_PAR_HIGH              = 0x03,
> +       FT260_CFG_PAR_LOW               = 0x04,
> +
> +       FT260_CFG_STOP_ONE_BIT          = 0x00,
> +       FT260_CFG_STOP_TWO_BIT          = 0x02,
> +
> +       FT260_CFG_BREAKING_NO           = 0x00,
> +       FT260_CFG_BEAKING_YES           = 0x01,
> +
> +       FT260_CFG_BAUD_MIN              = 1200,
> +       FT260_CFG_BAUD_MAX              = 12000000,
> +};

What kind of "enum" is this that re-assigns 0x00, 0x01 etc to
several enumerators?

Either this is four different enums or you should just do a
series of #defines instead if they are all really coming from
the same place, because using an enum like this does not
make sense: it's like numering pears, apples, oranges
and peaches in the same enum or something.

Also use the UART_ infix as mentioned in the GPIO patch
so you don't need to immediately change it in the next
patch.

Yours,
Linus Walleij