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

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

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