Re: [PATCH 03/13] HID: ft260: add GPIO support on top of UART

[Linus Walleij]

Hi Michael,

thanks for your patch!

It seems this patch title should just be
"HID: ft260: add GPIO support"
the "on top of UART" part seems to be more about
which order you did the patches and that is about
development process, not technical content, we don't
put that into Subject.

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

> Add gpiochip support for the FT260 multifunctional pins, including
> GPIO2/GPIOA/GPIOG function selection via sysfs, and enable the
> available GPIOs according to the active UART mode.
>
> This supersedes the earlier pre-UART GPIO series:
> https://lore.kernel.org/lkml/20230211115752.26276-2-michael.zaidman@xxxxxxxxx/
>
> The gpio_chip.set callback returns int to match the current gpiolib
> API, based on the adaptation originally contributed by Rio Liu
> <rio@xxxxxx>.
>
> Signed-off-by: Michael Zaidman <michael.zaidman@xxxxxxxxx>

You probably want:

#include <linus/bits.h>

(...)
>  static int ft260_debug = 1;
> @@ -57,6 +58,11 @@ MODULE_PARM_DESC(debug, "Toggle FT260 debugging messages");
>  #define FT260_RD_DATA_MAX (180)
>  #define FT260_WR_I2C_DATA_MAX (60)
>  #define FT260_WR_UART_DATA_MAX (62)
> +#define FT260_GPIOCHIP "ft260_gpio"
> +#define FT260_GPIO_MAX (6)
> +#define FT260_GPIO_EX_MAX (8)

So 6 GPIOs is "max" and then there are extended GPIOs and then there is
a new max which is 8?

I would give these defines some easier to understand name such as:

#define FT260_BASE_GPIOS 6
#define FT260_EXTENDED_GPIOS 8

> +#define FT260_GPIO_TOTAL (FT260_GPIO_MAX + FT260_GPIO_EX_MAX)
> +#define FT260_GPIO_MASK (~(0xffff << FT260_GPIO_TOTAL))

This macro seems to be using funky arithmetic with bits
rolling out on the left.

What about just:

#define FT260_GPIO_MASK GENMASK(FT260_GPIO_TOTAL, 0)

> +/* GPIO offsets */
> +enum {

Is this *really* an enum? I feel an enum should be something like
consecutive or sparse integer ranges.
What about just using #define for those.

> +       FT260_GPIO_0                    = (1 << 0),
> +       FT260_GPIO_1                    = (1 << 1),

#define FT259_GPIO_0 BIT(0)
#define FT259_GPIO_1 BIT(1)
(...)

> +       FT260_GPIO_A                    = (1 << (FT260_GPIO_MAX + 0)),
> +       FT260_GPIO_B                    = (1 << (FT260_GPIO_MAX + 1)),

Those are the extended GPIOs I guess? Mention
that in the macro name? Also you know the bit number.
Do this:

#define FT260_GPIO_EXT_A BIT(7)
#define FT260_GPIO_EXT_B BIT(8)

> +/* GPIO groups */
> +enum {
> +       FT260_GPIO_WAKEUP               = (FT260_GPIO_3),
> +       FT260_GPIO_I2C_DEFAULT          = (FT260_GPIO_0 | FT260_GPIO_1),
> +       FT260_GPIO_UART_RX_TX           = (FT260_GPIO_C | FT260_GPIO_D),
> +       FT260_GPIO_UART_DCD_RI          = (FT260_GPIO_4 | FT260_GPIO_5),
> +       FT260_GPIO_UART_RTS_CTS         = (FT260_GPIO_B | FT260_GPIO_E),
> +       FT260_GPIO_UART_DTR_DSR         = (FT260_GPIO_F | FT260_GPIO_H),
> +       FT260_GPIO_UART_MODE_0_SET      = (FT260_GPIO_UART_RX_TX |
> +                                          FT260_GPIO_UART_DCD_RI |
> +                                          FT260_GPIO_UART_RTS_CTS |
> +                                          FT260_GPIO_UART_DTR_DSR),
> +       FT260_GPIO_UART_MODE_1_SET      = (FT260_GPIO_UART_DTR_DSR),
> +       FT260_GPIO_UART_MODE_2_SET      = (FT260_GPIO_UART_RTS_CTS),
> +       FT260_GPIO_UART_MODE_3_SET      = (FT260_GPIO_UART_RTS_CTS |
> +                                          FT260_GPIO_UART_DTR_DSR),
> +       FT260_GPIO_UART_MODE_4_SET      = (FT260_GPIO_UART_MODE_3_SET),
> +       FT260_GPIO_UART_DEFAULT         = (FT260_GPIO_UART_MODE_0_SET),
> +       FT260_GPIO_UART_MODE_1_CLR      = (FT260_GPIO_UART_RX_TX |
> +                                          FT260_GPIO_UART_RTS_CTS),
> +       FT260_GPIO_UART_MODE_2_CLR      = (FT260_GPIO_UART_RX_TX |
> +                                          FT260_GPIO_UART_DTR_DSR),
> +       FT260_GPIO_UART_MODE_3_CLR      = (FT260_GPIO_UART_RX_TX),
> +       FT260_GPIO_UART_MODES           = (5),

Use #defines for all of this as well.

> +struct ft260_gpio_state {
> +       u8 vals;                /* GPIO[0-5] values in bits 0 - 5 */
> +       u8 dirs;                /* GPIO[0-5] directions, 0 - in, 1 - out */
> +       u8 ex_vals;             /* GPIO[A-H] values in bits 0 - 7 */
> +       u8 ex_dirs;             /* GPIO[A-H] directions, 0 - in, 1 - out */
> +} __packed;

So instead of creating your own cache of all registers,
have you considered just using regmap for this?
Regmap is essentially a register cache.

>  /* 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_UART_CFG_FLOW_CTRL_OFF            = 0x00,
> +       FT260_UART_CFG_FLOW_CTRL_RTS_CTS        = 0x01,
> +       FT260_UART_CFG_FLOW_CTRL_DTR_DSR        = 0x02,
> +       FT260_UART_CFG_FLOW_CTRL_XON_XOFF       = 0x03,
> +       FT260_UART_CFG_FLOW_CTRL_NONE           = 0x04,

This looks like names you could have already assigned
in the UART patch and then the diff would be less noisy.

> -       FT260_CFG_DATA_BITS_7           = 0x07,
> -       FT260_CFG_DATA_BITS_8           = 0x08,
> +       FT260_UART_CFG_DATA_BITS_7              = 0x07,
> +       FT260_UART_CFG_DATA_BITS_8              = 0x08,

Dito.

> -#define FT260_UART_EN_PW_SAVE_BAUD     (4800)
> +#define FT260_UART_EN_PW_SAVE_BAUD (4800)

Dito.

> +static void ft260_gpio_en_set(struct ft260_device *dev, u16 bitmap)
> +{
> +       dev->gpio_en |= bitmap & FT260_GPIO_MASK;
> +}
> +
> +static void ft260_gpio_en_clr(struct ft260_device *dev, u16 bitmap)
> +{
> +       dev->gpio_en &= ~bitmap & FT260_GPIO_MASK;
> +}

Linux has bitmap manipulation functions in
<linus/bitmap.h>, use these instead of reinventing the wheel.

Yours,
Linus Walleij