From: Lino Sanfilippo <[email protected]>
The following series includes cleanup and fixes around RS485 in the serial
core and uart drivers:
Patch 1: ar933x: Fix check for RS485 support
Patch 2: Remove superfluous code in ar933x.
Patch 3: Set the rs485 termination GPIO in the serial core. This is needed
since if the gpio is only accessible in sleepable context. It also
is a further step to make the RS485 handling more generic.
Patch 4: Move sanitizing of RS485 delays into an own function. This is in
preparation of patch 4.
Patch 5: Sanitize RS485 delays read from device tree.
Patch 6: Correct RS485 delays in binding documentation.
Patch 7: Remove redundant code in 8250_dwlib.
Patch 8: Remove redundant code in 8250-lpc18xx.
Changes in v3:
- remove obsolete patch (due to changes by Ilpo)
- corrected and rephrase commit messages (pointed out by Andy)
- remove superfluous check (pointed out by Andy)
- separate ar933x UART bugfix and cleanup into different patches (as
suggested by Ilpo)
- put the ar933x fix at the beginning of the series (as suggested by Andy)
Changes in v2:
- print a warning if termination GPIO is specified in DT/ACPI but is not
supported by driver
- fixed commit message for devtree documentation (as suggested by Andy)
- fixed code comment
- added patch 7
Lino Sanfilippo (8):
serial: ar933x: Fix check for RS485 support
serial: ar933x: Remove superfluous code in ar933x_config_rs485()
serial: core, 8250: set RS485 termination gpio in serial core
serial: core: move sanitizing of RS485 delays into own function
serial: core: sanitize RS485 delays read from device tree
dt_bindings: rs485: Correct delay values
serial: 8250_dwlib: remove redundant sanity check for RS485 flags
serial: 8250: lpc18xx: Remove redundant sanity check for RS485 flags
.../devicetree/bindings/serial/rs485.yaml | 4 +-
drivers/tty/serial/8250/8250_dwlib.c | 10 +---
drivers/tty/serial/8250/8250_lpc18xx.c | 6 +-
drivers/tty/serial/8250/8250_port.c | 3 -
drivers/tty/serial/ar933x_uart.c | 18 ++----
drivers/tty/serial/serial_core.c | 60 ++++++++++++-------
6 files changed, 50 insertions(+), 51 deletions(-)
base-commit: 7e5b4322cde067e1d0f1bf8f490e93f664a7c843
--
2.25.1
From: Lino Sanfilippo <[email protected]>
In ar933x_config_rs485() the check for the RTS GPIO is not needed since in
case the GPIO is not available at driver init ar933x_no_rs485 is assigned
to port->rs485_supported and this function is never called. So remove the
check.
Also in uart_set_rs485_config() the serial core already assigns the passed
serial_rs485 struct to the uart port. So remove the assignment in the
drivers rs485_config() function to avoid redundancy.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/ar933x_uart.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index f7b4638d69e5..32caeac12985 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -583,15 +583,6 @@ static const struct uart_ops ar933x_uart_ops = {
static int ar933x_config_rs485(struct uart_port *port, struct ktermios *termios,
struct serial_rs485 *rs485conf)
{
- struct ar933x_uart_port *up =
- container_of(port, struct ar933x_uart_port, port);
-
- if ((rs485conf->flags & SER_RS485_ENABLED) &&
- !up->rts_gpiod) {
- dev_err(port->dev, "RS485 needs rts-gpio\n");
- return 1;
- }
- port->rs485 = *rs485conf;
return 0;
}
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Before the drivers rs485_config() function is called the serial core
already ensures that only one of both options RTS on send or RTS after send
is set. So remove the concerning sanity check in the driver function to
avoid redundancy.
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/8250/8250_lpc18xx.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_lpc18xx.c b/drivers/tty/serial/8250/8250_lpc18xx.c
index d6ca0d47e9d5..6dc85aaba5d0 100644
--- a/drivers/tty/serial/8250/8250_lpc18xx.c
+++ b/drivers/tty/serial/8250/8250_lpc18xx.c
@@ -44,12 +44,8 @@ static int lpc18xx_rs485_config(struct uart_port *port, struct ktermios *termios
rs485_ctrl_reg |= LPC18XX_UART_RS485CTRL_NMMEN |
LPC18XX_UART_RS485CTRL_DCTRL;
- if (rs485->flags & SER_RS485_RTS_ON_SEND) {
+ if (rs485->flags & SER_RS485_RTS_ON_SEND)
rs485_ctrl_reg |= LPC18XX_UART_RS485CTRL_OINV;
- rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
- } else {
- rs485->flags |= SER_RS485_RTS_AFTER_SEND;
- }
}
if (rs485->delay_rts_after_send) {
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Currently the documentation claims that a maximum of 1000 msecs is allowed
for RTS delays. However nothing actually checks the values read from device
tree/ACPI and so it is possible to set much higher values.
There is already a maximum of 100 ms enforced for RTS delays that are set
via the uart TIOCSRS485 ioctl. To be consistent with that use the same
limit for DT/ACPI values.
Although this change is visible to userspace the risk of breaking anything
when reducing the max delays from 1000 to 100 ms should be very low, since
100 ms is already a very high maximum for delays that are usually rather in
the usecs range.
Signed-off-by: Lino Sanfilippo <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index f2c9c9fe6aa7..90a1bab40f05 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -22,12 +22,12 @@ properties:
- description: Delay between rts signal and beginning of data sent in
milliseconds. It corresponds to the delay before sending data.
default: 0
- maximum: 1000
+ maximum: 100
- description: Delay between end of data sent and rts signal in milliseconds.
It corresponds to the delay after sending data and actual release
of the line.
default: 0
- maximum: 1000
+ maximum: 100
rs485-rts-active-low:
description: drive RTS low when sending (default is high).
--
2.25.1
From: Lino Sanfilippo <[email protected]>
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 and using gpiod_set_value_cansleep() which instead of
gpiod_set_value() allows it to sleep.
Beside fixing the termination GPIO line setting for the 8250 driver this
change also makes setting the termination GPIO generic for all UART
drivers.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/8250/8250_port.c | 3 ---
drivers/tty/serial/serial_core.c | 12 ++++++++++++
2 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ed2a606f2da7..72252d956f17 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -676,9 +676,6 @@ int serial8250_em485_config(struct uart_port *port, struct ktermios *termios,
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 1db44cde76f6..612a97788341 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1358,12 +1358,23 @@ 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)
+{
+ if (!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, NULL, rs485);
if (ret)
@@ -1406,6 +1417,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);
spin_lock_irqsave(&port->lock, flags);
ret = port->rs485_config(port, &tty->termios, &rs485);
--
2.25.1
Hi Lino,
I love your patch! Perhaps something to improve:
[auto build test WARNING on 7e5b4322cde067e1d0f1bf8f490e93f664a7c843]
url: https://github.com/intel-lab-lkp/linux/commits/Lino-Sanfilippo/Fixes-and-cleanup-for-RS485/20220710-230624
base: 7e5b4322cde067e1d0f1bf8f490e93f664a7c843
config: hexagon-randconfig-r041-20220710 (https://download.01.org/0day-ci/archive/20220710/[email protected]/config)
compiler: clang version 15.0.0 (https://github.com/llvm/llvm-project 6ce63e267aab79ca87bf63453d34dd3909ab978d)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/846f02e6da9692810ed632dd72f45af667c3cc67
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Lino-Sanfilippo/Fixes-and-cleanup-for-RS485/20220710-230624
git checkout 846f02e6da9692810ed632dd72f45af667c3cc67
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=hexagon SHELL=/bin/bash drivers/tty/serial/
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/tty/serial/serial_core.c:1364:6: warning: logical not is only applied to the left hand side of this bitwise operator [-Wlogical-not-parentheses]
if (!rs485->flags & SER_RS485_ENABLED)
^ ~
drivers/tty/serial/serial_core.c:1364:6: note: add parentheses after the '!' to evaluate the bitwise operator first
if (!rs485->flags & SER_RS485_ENABLED)
^
( )
drivers/tty/serial/serial_core.c:1364:6: note: add parentheses around left hand side expression to silence this warning
if (!rs485->flags & SER_RS485_ENABLED)
^
( )
1 warning generated.
vim +1364 drivers/tty/serial/serial_core.c
1360
1361 static void uart_set_rs485_termination(struct uart_port *port,
1362 const struct serial_rs485 *rs485)
1363 {
> 1364 if (!rs485->flags & SER_RS485_ENABLED)
1365 return;
1366
1367 gpiod_set_value_cansleep(port->rs485_term_gpio,
1368 !!(rs485->flags & SER_RS485_TERMINATE_BUS));
1369 }
1370
--
0-DAY CI Kernel Test Service
https://01.org/lkp