From: Lino Sanfilippo <[email protected]>
The following series includes cleanup and fixes around RS485 in the serial
core and uart drivers:
Patch 1: Only request the rs485 termination gpio if it is supported.
NOTE:
This patch follows the design decision that "rs485_supported" is
set by the driver at initialization and cannot be modified
afterwards. However the better approach would be to let the serial
core modify the termination GPIO support setting based on the
existence of a termination GPIO. If "rs485_supported" is not a
read-only value any more in future the logic implemented in this
patch should be adjusted accordingly.
Patch 2: 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 3: Move sanitizing of RS485 delays into an own function. This is in
preparation of patch 4.
Patch 4: Sanitize RS485 delays read from device tree.
Patch 5: Correct RS485 delays in binding documentation.
Patch 6: Remove redundant code in 8250_dwlib.
Patch 7: Fix check for RS485 support.
Patch 8: Remove redundant code in ar933x.
Patch 9: Remove redundant code in 8250-lpc18xx.
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 (9):
serial: core: only get RS485 termination GPIO if supported
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: ar933x: Fix check for RS485 support
serial: ar933x: Remove redundant assignment in rs485_config
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 | 70 +++++++++++++------
6 files changed, 60 insertions(+), 51 deletions(-)
base-commit: 7349660438603ed19282e75949561406531785a5
--
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]>
---
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]>
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 d7cb3bb52069..fba0bb17e536 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]>
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_dwlib.c | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_dwlib.c b/drivers/tty/serial/8250/8250_dwlib.c
index a8bbed74ea70..f4ae262d00fb 100644
--- a/drivers/tty/serial/8250/8250_dwlib.c
+++ b/drivers/tty/serial/8250/8250_dwlib.c
@@ -187,16 +187,10 @@ static int dw8250_rs485_config(struct uart_port *p, struct ktermios *termios,
if (rs485->flags & SER_RS485_ENABLED) {
tcr |= DW_UART_TCR_RS485_EN;
- if (rs485->flags & SER_RS485_RX_DURING_TX) {
+ if (rs485->flags & SER_RS485_RX_DURING_TX)
tcr |= DW_UART_TCR_XFER_MODE_DE_DURING_RE;
- } else {
- /* HW does not support same DE level for tx and rx */
- if (!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
- !(rs485->flags & SER_RS485_RTS_AFTER_SEND))
- return -EINVAL;
-
+ else
tcr |= DW_UART_TCR_XFER_MODE_DE_OR_RE;
- }
dw8250_writel_ext(p, DW_UART_DE_EN, 1);
dw8250_writel_ext(p, DW_UART_RE_EN, 1);
} else {
--
2.25.1
From: Lino Sanfilippo <[email protected]>
When setting the RS485 configuration from userspace via TIOCSRS485 the
delays are clamped to 100ms. Make this consistent with the values passed
in by means of device tree parameters.
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/serial_core.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 05ed3acad09a..58cdad5f45dd 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3395,6 +3395,8 @@ int uart_get_rs485_mode(struct uart_port *port)
rs485conf->delay_rts_after_send = 0;
}
+ uart_sanitize_serial_rs485_delays(port, rs485conf);
+
/*
* Clear full-duplex and enabled flags, set RTS polarity to active high
* to get to a defined state with the following properties:
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Move the sanitizing of RS485 delays out of uart_sanitize_serial_rs485()
into the new function uart_sanitize_serial_rs485_delays().
Signed-off-by: Lino Sanfilippo <[email protected]>
Reviewed-by: Ilpo Järvinen <[email protected]>
---
drivers/tty/serial/serial_core.c | 46 ++++++++++++++++++--------------
1 file changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 9c29d031b404..05ed3acad09a 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -1302,27 +1302,9 @@ static int uart_check_rs485_flags(struct uart_port *port, struct serial_rs485 *r
return 0;
}
-static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs485 *rs485)
+static void uart_sanitize_serial_rs485_delays(struct uart_port *port,
+ struct serial_rs485 *rs485)
{
- u32 supported_flags = port->rs485_supported->flags;
-
- if (!(rs485->flags & SER_RS485_ENABLED)) {
- memset(rs485, 0, sizeof(*rs485));
- return;
- }
-
- /* pick sane settings if the user hasn't */
- if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
- !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
- !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
- dev_warn_ratelimited(port->dev,
- "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
- port->name, port->line);
- rs485->flags |= SER_RS485_RTS_ON_SEND;
- rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
- supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
- }
-
if (!port->rs485_supported->delay_rts_before_send) {
if (rs485->delay_rts_before_send) {
dev_warn_ratelimited(port->dev,
@@ -1350,9 +1332,33 @@ static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs4
"%s (%d): RTS delay after sending clamped to %u ms\n",
port->name, port->line, rs485->delay_rts_after_send);
}
+}
+
+static void uart_sanitize_serial_rs485(struct uart_port *port, struct serial_rs485 *rs485)
+{
+ u32 supported_flags = port->rs485_supported->flags;
+
+ if (!(rs485->flags & SER_RS485_ENABLED)) {
+ memset(rs485, 0, sizeof(*rs485));
+ return;
+ }
+
+ /* Pick sane settings if the user hasn't */
+ if ((supported_flags & (SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND)) &&
+ !(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+ !(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
+ dev_warn_ratelimited(port->dev,
+ "%s (%d): invalid RTS setting, using RTS_ON_SEND instead\n",
+ port->name, port->line);
+ rs485->flags |= SER_RS485_RTS_ON_SEND;
+ rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+ supported_flags |= SER_RS485_RTS_ON_SEND|SER_RS485_RTS_AFTER_SEND;
+ }
rs485->flags &= supported_flags;
+ uart_sanitize_serial_rs485_delays(port, rs485);
+
/* Return clean padding area to userspace */
memset(rs485->padding0, 0, sizeof(rs485->padding0));
memset(rs485->padding1, 0, sizeof(rs485->padding1));
--
2.25.1
From: Lino Sanfilippo <[email protected]>
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 | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index dac48a330db6..e07d8a550a49 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -583,7 +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)
{
- port->rs485 = *rs485conf;
return 0;
}
--
2.25.1
From: Lino Sanfilippo <[email protected]>
Without an RTS GPIO RS485 is not possible so disable the support
regardless of whether RS485 is enabled at boottime or not. Also remove the
now superfluous check for the RTS GPIO in ar933x_config_rs485().
Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
Signed-off-by: Lino Sanfilippo <[email protected]>
---
drivers/tty/serial/ar933x_uart.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/tty/serial/ar933x_uart.c b/drivers/tty/serial/ar933x_uart.c
index b73ce13683db..dac48a330db6 100644
--- a/drivers/tty/serial/ar933x_uart.c
+++ b/drivers/tty/serial/ar933x_uart.c
@@ -583,14 +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;
}
@@ -798,11 +790,12 @@ static int ar933x_uart_probe(struct platform_device *pdev)
up->rts_gpiod = mctrl_gpio_to_gpiod(up->gpios, UART_GPIO_RTS);
- if ((port->rs485.flags & SER_RS485_ENABLED) &&
- !up->rts_gpiod) {
- dev_err(&pdev->dev, "lacking rts-gpio, disabling RS485\n");
- port->rs485.flags &= ~SER_RS485_ENABLED;
+ if (!up->rts_gpiod) {
port->rs485_supported = &ar933x_no_rs485;
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ dev_err(&pdev->dev, "lacking rts-gpio, disabling RS485\n");
+ port->rs485.flags &= ~SER_RS485_ENABLED;
+ }
}
#ifdef CONFIG_SERIAL_AR933X_CONSOLE
--
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 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 3768663dfa4d..9c29d031b404 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 (!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, 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
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>
> From: Lino Sanfilippo <[email protected]>
>
> Without an RTS GPIO RS485 is not possible so disable the support
> regardless of whether RS485 is enabled at boottime or not. Also remove the
boot time
> now superfluous check for the RTS GPIO in ar933x_config_rs485().
>
> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
Is it an independent fix? If so, it should be the first patch in the
series, otherwise if it's dependent on something from previous patches
you need to mark all of them as a fix.
--
With Best Regards,
Andy Shevchenko
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>
> 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 to sleep.
allows it to
> Beside fixing the termination GPIO line setting for the 8250 driver this
> change also makes setting the termination GPIO generic for all UART
> drivers.
...
> +static void uart_set_rs485_termination(struct uart_port *port,
> + const struct serial_rs485 *rs485)
> +{
> + if (!port->rs485_term_gpio
This duplicates the check the GPIO library does. Drop it.
> || !(rs485->flags & SER_RS485_ENABLED))
> + return;
> +
> + gpiod_set_value_cansleep(port->rs485_term_gpio,
> + !!(rs485->flags & SER_RS485_TERMINATE_BUS));
> +}
--
With Best Regards,
Andy Shevchenko
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>
> 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.
Yeah, something similar is what you need to add to the previous patch IIUC.
--
With Best Regards,
Andy Shevchenko
On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>
> From: Lino Sanfilippo <[email protected]>
>
> When setting the RS485 configuration from userspace via TIOCSRS485 the
> delays are clamped to 100ms. Make this consistent with the values passed
> in by means of device tree parameters.
I'm not sure I got it right. Is the values from DT now clampet as well
as user space does or other way around? In either way the commit
message misses the explanation why it's not a problem if user
previously passed bigger values either via user space or via DT,
because it's an ABI change, right?
--
With Best Regards,
Andy Shevchenko
On 03.07.22 20:31, Andy Shevchenko wrote:
> On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>>
>> 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 to sleep.
>
> allows it to
>
Ok.
>> Beside fixing the termination GPIO line setting for the 8250 driver this
>> change also makes setting the termination GPIO generic for all UART
>> drivers.
>
> ...
>
>> +static void uart_set_rs485_termination(struct uart_port *port,
>> + const struct serial_rs485 *rs485)
>> +{
>
>> + if (!port->rs485_term_gpio
>
> This duplicates the check the GPIO library does. Drop it.
>
Ok.
>> || !(rs485->flags & SER_RS485_ENABLED))
>> + return;
>> +
>> + gpiod_set_value_cansleep(port->rs485_term_gpio,
>> + !!(rs485->flags & SER_RS485_TERMINATE_BUS));
>> +}
>
Thanks for the review!
Regards,
Lino
On 03.07.22 20:34, Andy Shevchenko wrote:
> On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>>
>> From: Lino Sanfilippo <[email protected]>
>>
>> When setting the RS485 configuration from userspace via TIOCSRS485 the
>> delays are clamped to 100ms. Make this consistent with the values passed
>> in by means of device tree parameters.
>
> I'm not sure I got it right. Is the values from DT now clampet as well
> as user space does or other way around? In either way the commit
> message misses the explanation why it's not a problem if user
> previously passed bigger values either via user space or via DT,
> because it's an ABI change, right?
>
Values are now clamped to 100 ms if set by userspace via ioctl and
not clamped at all if set by DT. I will improve the commit message
to make this more clear.
Thanks,
Lino
On Sun, 3 Jul 2022, Lino Sanfilippo wrote:
> 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 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);
> -
I sent a series to make .rs485_supported per uart_port and properly set
SER_RS485_TERMINATE_BUS according to DT config. With that series added
first, SER_RS485_TERMINATE_BUS should also be removed from
serial8250_em485_supported so that serial core can properly manage
it all.
--
i.
On Mon, 4 Jul 2022, Lino Sanfilippo wrote:
> On 03.07.22 20:39, Andy Shevchenko wrote:
> > On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
> >>
> >> From: Lino Sanfilippo <[email protected]>
> >>
> >> Without an RTS GPIO RS485 is not possible so disable the support
> >> regardless of whether RS485 is enabled at boottime or not. Also remove the
> >
> > boot time
> >
> >> now superfluous check for the RTS GPIO in ar933x_config_rs485().
> >>
> >> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
> >
> > Is it an independent fix? If so, it should be the first patch in the
> > series, otherwise if it's dependent on something from previous patches
> > you need to mark all of them as a fix.
> >
>
> The fix is independent, patch 8 depends on the fix however. I was not
> aware of this fixes-first rule for series with patches that are independent
> from each other. I will change the order accordingly in the next version of the series.
While at it, you could separate just the fix to own patch and the
->rs485_config() cleanup to own patch (or move it all to patch 8).
Not that this fix is expected to go anywhere else besides tty-next.
--
i.
On 03.07.22 20:39, Andy Shevchenko wrote:
> On Sun, Jul 3, 2022 at 7:02 PM Lino Sanfilippo <[email protected]> wrote:
>>
>> From: Lino Sanfilippo <[email protected]>
>>
>> Without an RTS GPIO RS485 is not possible so disable the support
>> regardless of whether RS485 is enabled at boottime or not. Also remove the
>
> boot time
>
>> now superfluous check for the RTS GPIO in ar933x_config_rs485().
>>
>> Fixes: e849145e1fdd ("serial: ar933x: Fill in rs485_supported")
>
> Is it an independent fix? If so, it should be the first patch in the
> series, otherwise if it's dependent on something from previous patches
> you need to mark all of them as a fix.
>
The fix is independent, patch 8 depends on the fix however. I was not
aware of this fixes-first rule for series with patches that are independent
from each other. I will change the order accordingly in the next version of the series.
Thanks,
Lino
On 04.07.22 11:51, Ilpo Järvinen wrote:
> On Sun, 3 Jul 2022, Lino Sanfilippo wrote:
>
>> 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 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);
>> -
>
> I sent a series to make .rs485_supported per uart_port and properly set
> SER_RS485_TERMINATE_BUS according to DT config. With that series added
> first, SER_RS485_TERMINATE_BUS should also be removed from
> serial8250_em485_supported so that serial core can properly manage
> it all.
>
Ok, I will rebase the next version of my patches on your series then.
Regards,
Lino
On Sun, 03 Jul 2022 19:00:35 +0200, Lino Sanfilippo wrote:
> 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]>
> ---
> Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
Acked-by: Rob Herring <[email protected]>