| Message ID | 20220622154659.8710-3-LinoSanfilippo@gmx.de |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fixes and cleanup for RS485 | expand |
On 22.06.22 at 19:06, Andy Shevchenko wrote: > On Wed, Jun 22, 2022 at 05:46:53PM +0200, Lino Sanfilippo wrote: >> From: Lino Sanfilippo <l.sanfilippo@kunbus.com> >> >> In serial8250_em485_config() the termination GPIO is set with the uart_port >> spinlock held. This is an issue if setting the GPIO line can sleep (e.g. >> since the concerning GPIO expander is connected via SPI or I2C). >> >> Fix this by setting the termination line outside of the uart_port spinlock >> in the serial core. > > This doesn't describe that this patch is actually changing GPIO to support > sleep mode. So, it doesn't fix anything. Please rephrase the commit message > accordingly. Good point, I will adjust the commit message in the next version. >> This also makes setting the termination GPIO generic for all uart drivers. > > UART > Right, upper letters should be used. Thanks a lot for the review! Regards, Lino
Hi, On 25.06.22 at 12:40, Ilpo Järvinen wrote: >> + >> int uart_rs485_config(struct uart_port *port) >> { >> struct serial_rs485 *rs485 = &port->rs485; >> int ret; >> >> uart_sanitize_serial_rs485(port, rs485); >> + uart_set_rs485_termination(port, rs485); >> >> ret = port->rs485_config(port, rs485); >> if (ret) >> @@ -1400,6 +1411,7 @@ static int uart_set_rs485_config(struct uart_port *port, >> if (ret) >> return ret; >> uart_sanitize_serial_rs485(port, &rs485); >> + uart_set_rs485_termination(port, &rs485); >> >> spin_lock_irqsave(&port->lock, flags); >> ret = port->rs485_config(port, &rs485); > > When port->rs485_config(port, &rs485) returns non-zero, the input got > partially applied? > > The thing is we dont know what the state of the termination GPIO (asserted or deasserted) was before we set it and port->rs485_config() failed, so we cannot restore it. We could read the GPIO before we change it but AFAIK this is unsafe since it is an output pin. Maybe add a boolean variable "rs485_termination_gpio_asserted" to uart_port to track the current state? Regards, Lino
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 3e3d784aa628..5245c179cc51 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c @@ -675,9 +675,6 @@ int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485) rs485->flags &= ~SER_RS485_RTS_AFTER_SEND; } - gpiod_set_value(port->rs485_term_gpio, - rs485->flags & SER_RS485_TERMINATE_BUS); - /* * Both serial8250_em485_init() and serial8250_em485_destroy() * are idempotent. diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c index 015f4e1da647..b387376e6fa2 100644 --- a/drivers/tty/serial/serial_core.c +++ b/drivers/tty/serial/serial_core.c @@ -1352,12 +1352,23 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4 memset(rs485->padding, 0, sizeof(rs485->padding)); } +static void uart_set_rs485_termination(struct uart_port *port, + const struct serial_rs485 *rs485) +{ + if (!port->rs485_term_gpio || !(rs485->flags & SER_RS485_ENABLED)) + return; + + gpiod_set_value_cansleep(port->rs485_term_gpio, + !!(rs485->flags & SER_RS485_TERMINATE_BUS)); +} + int uart_rs485_config(struct uart_port *port) { struct serial_rs485 *rs485 = &port->rs485; int ret; uart_sanitize_serial_rs485(port, rs485); + uart_set_rs485_termination(port, rs485); ret = port->rs485_config(port, rs485); if (ret) @@ -1400,6 +1411,7 @@ static int uart_set_rs485_config(struct uart_port *port, if (ret) return ret; uart_sanitize_serial_rs485(port, &rs485); + uart_set_rs485_termination(port, &rs485); spin_lock_irqsave(&port->lock, flags); ret = port->rs485_config(port, &rs485);