On Wed, 11 Oct 2023 20:15:39 +0200 Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx> wrote: > Both the imx and stm32 driver set the rx-during-tx GPIO in the > rs485_config() function by means of gpiod_set_value(). Since rs485_config() > is called with the port lock held, this can be an problem in case that > setting the GPIO line can sleep (e.g. if a GPIO expander is used which is > connected via SPI or I2C). > > Avoid this issue by setting the GPIO outside of the port lock in the serial > core and by using gpiod_set_value_cansleep() instead of gpiod_set_value(). Hi Lino, it seems to me that both drivers were already using gpiod_set_value_cansleep()? Maybe update your commit message if this is the case. > > Since now both the term and the rx-during-tx GPIO are set within the serial > core use a common function uart_set_rs485_gpios() to set both. > > With moving it into the serial core setting the rx-during-tx GPIO is now > automatically done for all drivers that support such a GPIO. > > Fixes: c54d48543689 ("serial: stm32: Add support for rs485 RX_DURING_TX output GPIO") > Fixes: ca530cfa968c ("serial: imx: Add support for RS485 RX_DURING_TX output GPIO") > Cc: stable@xxxxxxxxxxxxxxx > Signed-off-by: Lino Sanfilippo <l.sanfilippo@xxxxxxxxxx> > --- > drivers/tty/serial/imx.c | 4 ---- > drivers/tty/serial/serial_core.c | 10 ++++++---- > drivers/tty/serial/stm32-usart.c | 5 +---- > 3 files changed, 7 insertions(+), 12 deletions(-) > > diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c > index 13cb78340709..edb2ec6a5567 100644 > --- a/drivers/tty/serial/imx.c > +++ b/drivers/tty/serial/imx.c > @@ -1947,10 +1947,6 @@ static int imx_uart_rs485_config(struct uart_port *port, struct ktermios *termio > rs485conf->flags & SER_RS485_RX_DURING_TX) > imx_uart_start_rx(port); > > - if (port->rs485_rx_during_tx_gpio) > - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, > - !!(rs485conf->flags & SER_RS485_RX_DURING_TX)); > - > return 0; > } > > diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c > index 7bdc21d5e13b..ef0500be3553 100644 > --- a/drivers/tty/serial/serial_core.c > +++ b/drivers/tty/serial/serial_core.c > @@ -1391,14 +1391,16 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 > memset(rs485->padding1, 0, sizeof(rs485->padding1)); > } > > -static void uart_set_rs485_termination(struct uart_port *port, > - const struct serial_rs485 *rs485) > +static void uart_set_rs485_gpios(struct uart_port *port, > + const struct serial_rs485 *rs485) > { > if (!(rs485->flags & SER_RS485_ENABLED)) > return; > > gpiod_set_value_cansleep(port->rs485_term_gpio, > !!(rs485->flags & SER_RS485_TERMINATE_BUS)); > + gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, > + !!(rs485->flags & SER_RS485_RX_DURING_TX)); > } > > static int uart_rs485_config(struct uart_port *port) > @@ -1407,7 +1409,7 @@ static int uart_rs485_config(struct uart_port *port) > int ret; > > uart_sanitize_serial_rs485(port, rs485); > - uart_set_rs485_termination(port, rs485); > + uart_set_rs485_gpios(port, rs485); Suggestion: define a new function to handle rx_during_tx, to keep uart_set_rs485_termination(), which is more self-documenting than uart_set_rs485_gpios(). ex: uart_set_rs485_termination(port, rs485); + uart_set_rs485_rx_during_tx(port, rs485); Hugo. > > ret = port->rs485_config(port, NULL, rs485); > if (ret) > @@ -1449,7 +1451,7 @@ static int uart_set_rs485_config(struct tty_struct *tty, struct uart_port *port, > if (ret) > return ret; > uart_sanitize_serial_rs485(port, &rs485); > - uart_set_rs485_termination(port, &rs485); > + uart_set_rs485_gpios(port, &rs485); > > spin_lock_irqsave(&port->lock, flags); > ret = port->rs485_config(port, &tty->termios, &rs485); > diff --git a/drivers/tty/serial/stm32-usart.c b/drivers/tty/serial/stm32-usart.c > index 5e9cf0c48813..8eb13bf055f2 100644 > --- a/drivers/tty/serial/stm32-usart.c > +++ b/drivers/tty/serial/stm32-usart.c > @@ -226,10 +226,7 @@ static int stm32_usart_config_rs485(struct uart_port *port, struct ktermios *ter > > stm32_usart_clr_bits(port, ofs->cr1, BIT(cfg->uart_enable_bit)); > > - if (port->rs485_rx_during_tx_gpio) > - gpiod_set_value_cansleep(port->rs485_rx_during_tx_gpio, > - !!(rs485conf->flags & SER_RS485_RX_DURING_TX)); > - else > + if (!port->rs485_rx_during_tx_gpio) > rs485conf->flags |= SER_RS485_RX_DURING_TX; > > if (rs485conf->flags & SER_RS485_ENABLED) { > -- > 2.40.1 >