Re: [PATCH 08/13] HID: ft260: uart: add modem pins control via ioctl

Re: [PATCH 08/13] HID: ft260: uart: add modem pins control via ioctl

[Michael Zaidman]

On Tue, 25 Aug 2026 at 10:08 +0200, Linus Walleij wrote:
> I'm not the authortative expert on modem control using GPIO,
> but what I think you should do is to
>
> select GPIOLIB
> select SERIAL_MCTRL_GPIO
>
> On Kconfig, so that gpiolib is always available and you can use
> the generic modem control helpers for modem control over GPIO.

I looked into this, and it does not work for the FT260 without
changing serial_mctrl_gpio.c first. Three blockers:

- Every GPIO access on this chip is a USB transfer, so the
  gpiochip has can_sleep = true. mctrl_gpio_set() calls
  gpiod_set_array_value() and mctrl_gpio_get() calls
  gpiod_get_value(), and gpiolib does WARN_ON(can_sleep) in both,
  so every TIOCMGET/TIOCMSET would give a WARN backtrace.

- mctrl_gpio_init() takes a struct uart_port and its IRQ handler
  needs it: uart_port_lock_irqsave(), uart_handle_dcd_change(),
  port->icount, delta_msr_wait. This UART is a plain tty_driver
  with a tty_port, so only mctrl_gpio_init_noauto() is left - and
  the FT260 GPIO lines have no interrupts anyway.

- mctrl_gpio_init_noauto() only picks up lines that exist as
  firmware properties: device_property_present(dev, "cts-gpios")
  and friends. A gpiod_add_lookup_table() table is the machine
  lookup path, so every line would be skipped, all descriptors
  would stay NULL and both helpers would silently do nothing.
  Software nodes could satisfy that check, but there is no
  PROPERTY_ENTRY_GPIO in the tree to build them with.

serial_mctrl_gpio.h is also private to drivers/tty/serial - all
eleven users are serial_core drivers in that directory.

Registering a uart_port instead was tried for this device and
turned down. Daniel Beer's 2022 FT260 UART patch was built on
serial_core and called uart_add_one_port(); Greg asked for
usb-serial, and Johan Hovold answered that "neither USB-serial or
serial (core) is a good fit for such a HID device", pointing at
Christina Quast's tty driver as the right approach - which patch
1 of this series is a port of.

  https://lore.kernel.org/lkml/638c51a2.170a0220.3af16.18f8@mx.google.com/
  https://lore.kernel.org/lkml/Y6WNl6+ySy8zcSyg@hovoldconsulting.com/

That patch left set_mctrl empty and get_mctrl returning a
constant, which is this same constraint seen from the other side:
uart_ops.set_mctrl and .get_mctrl must not sleep, while every
FT260 line access is a HID feature report over USB.

> This can be a bit delicate in this case since the gpiochip that you
> use for mctrl is also registered in this driver, so you need to
> register the gpiochip *first*, then add a look-up table for the
> GPIOs, then register this modem control.
>
> Then look in e.g. drivers/mfd/sm501.c which is an
> MFD device that register a gpiochip and then consume
> GPIOs from itself.

Agreed on the ordering, and thanks for the reference. The UART
probe currently registers the tty port before the gpiochip, so
that would have to be inverted, and the gpiochip label is built
from the HID device name, so the table would have to be built at
probe rather than being static. Both are workable; they are not
what blocks this. sm501 does not hit the sleeping problem because
its gpiochip is memory mapped.

> The core idea is that the serial modem control should look
> up the GPIOs from its own gpiochip and use the MCTRL
> library helpers, then this should result in very little and
> compact code that is easy to read.

No argument with the goal - I would rather have that than my own
TIOCM handling. But making it usable here means work inside the
serial helpers: cansleep set/get, a path that does not require a
uart_port, a lookup that works without firmware properties, and
the header moved to include/linux. That is a serial subsystem
series to agree with Greg and Jiri Slaby, so I propose keeping
the ioctl implementation in this series and doing the conversion
as a follow-up.

Even then only the set/get helpers would apply: with no GPIO
interrupts, modem status changes come from the FT260's own
interrupt status input report (0xB1), so that part stays in the
driver either way.

Thanks,
Michael

Re: [PATCH 08/13] HID: ft260: uart: add modem pins control via ioctl

[Linus Walleij]

Hi Michael,

On Fri, Aug 28, 2026 at 12:08 AM Michael Zaidman
<michael.zaidman@gmail.com> wrote:

[Me]
> > On Kconfig, so that gpiolib is always available and you can use
> > the generic modem control helpers for modem control over GPIO.
>
> I looked into this, and it does not work for the FT260 without
> changing serial_mctrl_gpio.c first. Three blockers:
>
> - Every GPIO access on this chip is a USB transfer, so the
>   gpiochip has can_sleep = true. mctrl_gpio_set() calls
>   gpiod_set_array_value() and mctrl_gpio_get() calls
>   gpiod_get_value(), and gpiolib does WARN_ON(can_sleep) in both,
>   so every TIOCMGET/TIOCMSET would give a WARN backtrace.

Can't you just patch mctrl to use gpiod_set_array_value_cansleep()
and gpiod_get_value_cansleep()?

I don't think any of the users depend on call thing this
in atomic context.

> - mctrl_gpio_init() takes a struct uart_port and its IRQ handler
>   needs it: uart_port_lock_irqsave(), uart_handle_dcd_change(),
>   port->icount, delta_msr_wait. This UART is a plain tty_driver
>   with a tty_port, so only mctrl_gpio_init_noauto() is left - and
>   the FT260 GPIO lines have no interrupts anyway.

I don't understand this :D
But hopefully the TTY/serial maintainer does.

> - mctrl_gpio_init_noauto() only picks up lines that exist as
>   firmware properties: device_property_present(dev, "cts-gpios")
>   and friends. A gpiod_add_lookup_table() table is the machine
>   lookup path, so every line would be skipped, all descriptors
>   would stay NULL and both helpers would silently do nothing.
>   Software nodes could satisfy that check, but there is no
>   PROPERTY_ENTRY_GPIO in the tree to build them with.

Using software nodes is the way to go I think,
<linux/gpio/property.h> contains PROPERTY_ENTRY_GPIO.

> serial_mctrl_gpio.h is also private to drivers/tty/serial - all
> eleven users are serial_core drivers in that directory.

Well having serial drivers in drivers/hid and having all kinds
of misc drivers in drivers/hid has made it a dumping ground
for anything HID.

> Registering a uart_port instead was tried for this device and
> turned down. Daniel Beer's 2022 FT260 UART patch was built on
> serial_core and called uart_add_one_port(); Greg asked for
> usb-serial, and Johan Hovold answered that "neither USB-serial or
> serial (core) is a good fit for such a HID device", pointing at
> Christina Quast's tty driver as the right approach - which patch
> 1 of this series is a port of.
>
>   https://lore.kernel.org/lkml/638c51a2.170a0220.3af16.18f8@mx.google.com/
>   https://lore.kernel.org/lkml/Y6WNl6+ySy8zcSyg@hovoldconsulting.com/

Well if it absolutely has to live in drivers/hid then do the
ugly thing and

include "../tty/serial/serial_mctrl_gpio.h"

It's perhaps the lesser evil then?

Otherwise we just bite the bullet and move
drivers/tty/serial/serial_mctrl_gpio.h
to
include/linux/serial_mctrl_gpio.h
?

(Unless Greg want some new subdir such as
include/linux/serial/serial_mctrl_gpio.h)

> > The core idea is that the serial modem control should look
> > up the GPIOs from its own gpiochip and use the MCTRL
> > library helpers, then this should result in very little and
> > compact code that is easy to read.
>
> No argument with the goal - I would rather have that than my own
> TIOCM handling. But making it usable here means work inside the
> serial helpers: cansleep set/get, a path that does not require a
> uart_port, a lookup that works without firmware properties, and
> the header moved to include/linux. That is a serial subsystem
> series to agree with Greg and Jiri Slaby, so I propose keeping
> the ioctl implementation in this series and doing the conversion
> as a follow-up.

These things have a tendency to never happen and it's
not like we have a shortage of technical debt.

Yours,
Linus Walleij

Re: [PATCH 08/13] HID: ft260: uart: add modem pins control via ioctl

[Michael Zaidman]

Hi Linus,

On Mon, 14 Sep 2026 at 10:57 +0200, Linus Walleij wrote:

> Can't you just patch mctrl to use gpiod_set_array_value_cansleep()
> and gpiod_get_value_cansleep()?
>
> I don't think any of the users depend on call thing this
> in atomic context.

That change is in drivers/tty/serial/serial_mctrl_gpio.c, not in
hid-ft260.c. mctrl_gpio_set() calls gpiod_set_array_value() there, and
both mctrl_gpio_get() and mctrl_gpio_get_outputs() call
gpiod_get_value(). The FT260 gpiochip sets can_sleep, so calling those
three functions from this driver warns until that file uses the
_cansleep variants.

The switch is not only a rename. serial_core.c calls ops->set_mctrl()
under uart_port_lock_irqsave() in uart_update_mctrl(), and
ops->get_mctrl() under uart_port_lock_irq() in uart_tiocmget().
imx_uart_set_mctrl() and atmel_set_mctrl() call mctrl_gpio_set() from
there, and serial8250_do_get_mctrl() calls mctrl_gpio_get() from
there. These callers do run in atomic context, so _cansleep alone
would put a sleeping call under a spinlock with interrupts off. A
sleeping gpiochip also needs the serial core to call set_mctrl and
get_mctrl outside the port lock. I will not put that in the FT260 v2
series.

> > - mctrl_gpio_init() takes a struct uart_port and its IRQ handler
> >   needs it: uart_port_lock_irqsave(), uart_handle_dcd_change(),
> >   port->icount, delta_msr_wait. This UART is a plain tty_driver
> >   with a tty_port, so only mctrl_gpio_init_noauto() is left - and
> >   the FT260 GPIO lines have no interrupts anyway.
>
> I don't understand this :D
> But hopefully the TTY/serial maintainer does.

mctrl_gpio_init() in drivers/tty/serial/serial_mctrl_gpio.c takes a
struct uart_port. Its interrupt handler, mctrl_gpio_irq_handle(), uses
that port for uart_port_lock_irqsave(), uart_handle_dcd_change(),
port->icount and delta_msr_wait.

The FT260 UART is a tty_driver with a tty_port. It has no struct
uart_port, so it cannot call mctrl_gpio_init(). The function it can
call is mctrl_gpio_init_noauto(), which takes a struct device and does
not set up that handler.

The GPIO chip in hid-ft260.c registers no interrupt callback, so there
is no GPIO interrupt for that handler to bind to.

> Using software nodes is the way to go I think,
> <linux/gpio/property.h> contains PROPERTY_ENTRY_GPIO.

You are right. PROPERTY_ENTRY_GPIO has been available in
include/linux/gpio/property.h since v6.2-rc1, and hid-ft260.c does not
use it.

mctrl_gpio_init_noauto() skips a line if the device lacks the
corresponding property, such as cts-gpios. Providing these properties
requires registering a software node using PROPERTY_ENTRY_GPIO. I will
register that node in the patch that switches this driver to the
serial_mctrl_gpio.c helpers, so it is not part of v2 either.

> include "../tty/serial/serial_mctrl_gpio.h"
>
> Otherwise we just bite the bullet and move
> drivers/tty/serial/serial_mctrl_gpio.h
> to
> include/linux/serial_mctrl_gpio.h

That move belongs to the same serial core patches, not to v2. I will
ask Greg to say if he wants the relative include instead.

> > That is a serial subsystem
> > series to agree with Greg and Jiri Slaby, so I propose keeping
> > the ioctl implementation in this series and doing the conversion
> > as a follow-up.
>
> These things have a tendency to never happen and it's
> not like we have a shortage of technical debt.

Fair point, a follow-up can stall. I will send the serial core
RFC as its own thread, after this series is merged, and I ask that it
not gate this series: it changes drivers/tty/serial, and the FT260
UART works without it. If the serial core does not take it, what
stays is ft260_uart_tiocmget() and ft260_uart_tiocmset() in
hid-ft260.c, and nothing in drivers/tty.

Thanks,
Michael