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

[Michael Zaidman <michael.zaidman@gmail.com>]

On Tue, 25 Aug 2026 at 09:44 +0200, Linus Walleij wrote:
> 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.

Will drop that from the subject in v2.

> You probably want:
>
> #include <linus/bits.h>

<linux/bits.h>. I'll switch the pin constants to #define + BIT()
and the group masks to #defines.

> So 6 GPIOs is "max" and then there are extended GPIOs and then there is
> a new max which is 8?
>
> #define FT260_BASE_GPIOS 6
> #define FT260_EXTENDED_GPIOS 8

Agreed, the names are misleading. FT260_GPIO_MAX and
FT260_GPIO_EX_MAX are counts of the two GPIO groups the chip
exposes, not bit indices: 6 pins named GPIO0-5 and 8 named
GPIOA-H. v2 will use FT260_BASE_GPIOS and FT260_EXTENDED_GPIOS.

One distinction worth making, since it explains the bit numbers
you suggested. The GPIO report (0xB0) carries the two groups
in separate bytes: GPIO0-5 values and directions in bits 0-5 of
one byte pair, GPIOA-H values and directions in bits 0-7 of the
next pair. So in the report GPIOA is bit 0.

The FT260_GPIO_* masks are not report bits. They are the
gpiochip offset space, where the two groups are concatenated
into one 14-line chip: GPIO0-5 at offsets 0-5, GPIOA-H at
offsets 6-13. There GPIOA is BIT(6) and GPIOB is BIT(7), so
I'll write them as explicit BIT() with the group named:

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

> What about just:
>
> #define FT260_GPIO_MASK GENMASK(FT260_GPIO_TOTAL, 0)

Yes, that mask is over the 14 gpiochip offsets, and GENMASK is
clearer than the shift. It needs to stop at the top offset:

#define FT260_GPIO_MASK GENMASK(FT260_GPIO_TOTAL - 1, 0)

which is the 0x3fff the current expression produces.
GENMASK(FT260_GPIO_TOTAL, 0) would add bit 14.

> 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.
>
> Use #defines for all of this as well.

Will do.

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

There is no register space for regmap to model. 0xB0
is one 4-byte feature report, a value byte and a direction byte
per GPIO group, reached with hid_hw_raw_request(). The mux that
decides which of those bits are GPIO at all is in a different
report, 0xA1, with an unrelated layout.

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

v2 will put the UART_ infix in the serial patch, as in my
other mail.

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

gpio_en is "GPIO enabled": a mask of which of the 14 lines are
muxed to GPIO at all, derived from chip_mode and uart_mode, not
pin state. The setters only OR and mask it, and the only readers
test one line before touching the pin. That is a u16 flag word,
and the bitmap helpers work on unsigned long arrays, so I'd keep
the u16 and use BIT(offset) in those tests instead of 1 << offset.
I can convert it to DECLARE_BITMAP() if you prefer.

Thanks,
Michael