* [PATCH 0/4] rs485 bus termination GPIO
@ 2020-05-05 14:42 Lukas Wunner
2020-05-05 14:42 ` [PATCH 3/4] dt-bindings: serial: Add binding for " Lukas Wunner
2020-05-05 14:42 ` [PATCH 4/4] serial: 8250: Support " Lukas Wunner
0 siblings, 2 replies; 6+ messages in thread
From: Lukas Wunner @ 2020-05-05 14:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Rob Herring
Cc: Matwey V. Kornilov, Giulio Benetti, Heiko Stuebner,
Andy Shevchenko, Christoph Muellner, Nicolas Ferre,
Codrin Ciubotariu, Razvan Stefanescu, Radu Pirea, Sascha Hauer,
Bich HEMON, Uwe Kleine-Koenig, Sascha Weisenberger, Jan Kiszka,
devicetree, linux-serial
Commit e8759ad17d41 ("serial: uapi: Add support for bus termination"),
allowed user space to change rs485 bus termination through a flag in
struct serial_rs485. But so far only a single driver, 8250_exar.c,
supports the flag: It hardcodes a GPIO specific to Siemens IOT2040
products.
Provide for a more generic solution: Define a device tree binding
for an rs485 bus termination GPIO (patch [3/4]), amend the serial core
to retrieve the GPIO from the device tree and amend the default
->rs485_config() callback for 8250 drivers to change the GPIO on
request from user space (patch [4/4]).
Retrieving the GPIO from the device tree may fail, so allow
uart_get_rs485_mode() to return an errno and change all callers
to check for failure (patch [2/4]).
Testing has exposed a bug in the 8250 core if retrieval of the GPIO
initially fails with -EPROBE_DEFER and is later retried. That bug
is fixed by patch [1/4].
Lukas Wunner (4):
serial: 8250: Avoid error message on reprobe
serial: Allow uart_get_rs485_mode() to return errno
dt-bindings: serial: Add binding for rs485 bus termination GPIO
serial: 8250: Support rs485 bus termination GPIO
.../devicetree/bindings/serial/rs485.yaml | 4 +++
drivers/tty/serial/8250/8250_core.c | 16 +++++++---
drivers/tty/serial/8250/8250_port.c | 4 +++
drivers/tty/serial/ar933x_uart.c | 6 ++--
drivers/tty/serial/atmel_serial.c | 6 ++--
drivers/tty/serial/fsl_lpuart.c | 5 +++-
drivers/tty/serial/imx.c | 6 +++-
drivers/tty/serial/omap-serial.c | 4 ++-
drivers/tty/serial/serial_core.c | 30 ++++++++++++++++++-
drivers/tty/serial/stm32-usart.c | 8 ++---
include/linux/serial_core.h | 4 ++-
11 files changed, 76 insertions(+), 17 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/4] dt-bindings: serial: Add binding for rs485 bus termination GPIO
2020-05-05 14:42 [PATCH 0/4] rs485 bus termination GPIO Lukas Wunner
@ 2020-05-05 14:42 ` Lukas Wunner
2020-05-05 14:42 ` [PATCH 4/4] serial: 8250: Support " Lukas Wunner
1 sibling, 0 replies; 6+ messages in thread
From: Lukas Wunner @ 2020-05-05 14:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Rob Herring
Cc: Matwey V. Kornilov, Giulio Benetti, Heiko Stuebner,
Andy Shevchenko, Christoph Muellner, Jan Kiszka, devicetree,
linux-serial
Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
introduced the ability to enable rs485 bus termination from user space.
So far the feature is only used by a single driver, 8250_exar.c, using a
hardcoded GPIO pin specific to Siemens IOT2040 products.
Provide for a more generic solution by allowing specification of an
rs485 bus termination GPIO pin in the device tree. An upcoming commit
implements support for this pin for any 8250 driver. The binding is
used in device trees of the "Revolution Pi" PLCs offered by KUNBUS.
[Heiko Stuebner converted the binding to YAML, hence his Signed-off-by.]
Signed-off-by: Heiko Stuebner <heiko.stuebner@theobroma-systems.com>
Signed-off-by: Lukas Wunner <lukas@wunner.de>
Cc: Jan Kiszka <jan.kiszka@siemens.com>
---
Documentation/devicetree/bindings/serial/rs485.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/serial/rs485.yaml b/Documentation/devicetree/bindings/serial/rs485.yaml
index d4beaf11222d..a9ad17864889 100644
--- a/Documentation/devicetree/bindings/serial/rs485.yaml
+++ b/Documentation/devicetree/bindings/serial/rs485.yaml
@@ -43,3 +43,7 @@ properties:
rs485-rx-during-tx:
description: enables the receiving of data even while sending data.
$ref: /schemas/types.yaml#/definitions/flag
+
+ rs485-term-gpios:
+ description: GPIO pin to enable RS485 bus termination.
+ maxItems: 1
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO
2020-05-05 14:42 [PATCH 0/4] rs485 bus termination GPIO Lukas Wunner
2020-05-05 14:42 ` [PATCH 3/4] dt-bindings: serial: Add binding for " Lukas Wunner
@ 2020-05-05 14:42 ` Lukas Wunner
2020-05-05 16:10 ` Andy Shevchenko
1 sibling, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2020-05-05 14:42 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby, Rob Herring
Cc: Matwey V. Kornilov, Giulio Benetti, Heiko Stuebner,
Andy Shevchenko, Christoph Muellner, Jan Kiszka, devicetree,
linux-serial
Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
introduced the ability to enable rs485 bus termination from user space.
So far the feature is only used by a single driver, 8250_exar.c, using a
hardcoded GPIO pin specific to Siemens IOT2040 products.
Provide for a more generic solution by allowing specification of an
rs485 bus termination GPIO pin in the device tree: Amend the serial
core to retrieve the GPIO from the device tree (or ACPI table) and amend
the default ->rs485_config() callback for 8250 drivers to change the
GPIO on request from user space.
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
drivers/tty/serial/8250/8250_port.c | 4 ++++
drivers/tty/serial/serial_core.c | 24 ++++++++++++++++++++++++
include/linux/serial_core.h | 2 ++
3 files changed, 30 insertions(+)
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index f77bf820b7a3..b5b630d02110 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -681,6 +681,10 @@ int serial8250_em485_config(struct uart_port *port, struct serial_rs485 *rs485)
memset(rs485->padding, 0, sizeof(rs485->padding));
port->rs485 = *rs485;
+ if (port->rs485_term_gpio)
+ 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 43b6682877d5..7c929aad066e 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3317,6 +3317,7 @@ int uart_get_rs485_mode(struct uart_port *port)
* to get to a defined state with the following properties:
*/
rs485conf->flags &= ~(SER_RS485_RX_DURING_TX | SER_RS485_ENABLED |
+ SER_RS485_TERMINATE_BUS |
SER_RS485_RTS_AFTER_SEND);
rs485conf->flags |= SER_RS485_RTS_ON_SEND;
@@ -3331,6 +3332,29 @@ int uart_get_rs485_mode(struct uart_port *port)
rs485conf->flags |= SER_RS485_RTS_AFTER_SEND;
}
+ if (port->rs485_term_gpio)
+ devm_gpiod_put(dev, port->rs485_term_gpio);
+
+ port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
+ GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
+ if (IS_ERR(port->rs485_term_gpio)) {
+ ret = PTR_ERR(port->rs485_term_gpio);
+ port->rs485_term_gpio = NULL;
+ if (ret != -EPROBE_DEFER)
+ dev_err(dev, "Cannot get rs485-term-gpios\n");
+ return ret;
+ }
+
+ if (port->rs485_term_gpio) {
+ ret = gpiod_get_value(port->rs485_term_gpio);
+ if (ret < 0) {
+ dev_err(dev, "Cannot get rs485-term-gpios value\n");
+ return ret;
+ }
+ if (ret)
+ rs485conf->flags |= SER_RS485_TERMINATE_BUS;
+ }
+
return 0;
}
EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
index b649a2b894e7..9fd550e7946a 100644
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -10,6 +10,7 @@
#include <linux/bitops.h>
#include <linux/compiler.h>
#include <linux/console.h>
+#include <linux/gpio/consumer.h>
#include <linux/interrupt.h>
#include <linux/circ_buf.h>
#include <linux/spinlock.h>
@@ -251,6 +252,7 @@ struct uart_port {
struct attribute_group *attr_group; /* port specific attributes */
const struct attribute_group **tty_groups; /* all attributes (serial core use only) */
struct serial_rs485 rs485;
+ struct gpio_desc *rs485_term_gpio; /* enable RS485 bus termination */
struct serial_iso7816 iso7816;
void *private_data; /* generic platform data pointer */
};
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO
2020-05-05 14:42 ` [PATCH 4/4] serial: 8250: Support " Lukas Wunner
@ 2020-05-05 16:10 ` Andy Shevchenko
2020-05-06 6:29 ` Lukas Wunner
0 siblings, 1 reply; 6+ messages in thread
From: Andy Shevchenko @ 2020-05-05 16:10 UTC (permalink / raw)
To: Lukas Wunner
Cc: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, Matwey V. Kornilov,
Giulio Benetti, Heiko Stuebner, Christoph Muellner, Jan Kiszka,
devicetree, linux-serial
On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote:
> Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
> introduced the ability to enable rs485 bus termination from user space.
> So far the feature is only used by a single driver, 8250_exar.c, using a
> hardcoded GPIO pin specific to Siemens IOT2040 products.
>
> Provide for a more generic solution by allowing specification of an
> rs485 bus termination GPIO pin in the device tree: Amend the serial
> core to retrieve the GPIO from the device tree (or ACPI table) and amend
> the default ->rs485_config() callback for 8250 drivers to change the
> GPIO on request from user space.
...
> @@ -3331,6 +3332,29 @@ int uart_get_rs485_mode(struct uart_port *port)
> + devm_gpiod_put(dev, port->rs485_term_gpio);
> + port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
Using devm_*() in uart_get_rs485_mode() seems not right.
Why do you need this?
> + GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
Parameter has a specific macro GPIOD_OUT_HIGH.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO
2020-05-05 16:10 ` Andy Shevchenko
@ 2020-05-06 6:29 ` Lukas Wunner
2020-05-06 10:06 ` Andy Shevchenko
0 siblings, 1 reply; 6+ messages in thread
From: Lukas Wunner @ 2020-05-06 6:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, Matwey V. Kornilov,
Giulio Benetti, Heiko Stuebner, Christoph Muellner, Jan Kiszka,
devicetree, linux-serial
On Tue, May 05, 2020 at 07:10:35PM +0300, Andy Shevchenko wrote:
> On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote:
> > Commit e8759ad17d41 ("serial: uapi: Add support for bus termination")
> > introduced the ability to enable rs485 bus termination from user space.
> > So far the feature is only used by a single driver, 8250_exar.c, using a
> > hardcoded GPIO pin specific to Siemens IOT2040 products.
> >
> > Provide for a more generic solution by allowing specification of an
> > rs485 bus termination GPIO pin in the device tree: Amend the serial
> > core to retrieve the GPIO from the device tree (or ACPI table) and amend
> > the default ->rs485_config() callback for 8250 drivers to change the
> > GPIO on request from user space.
>
> ...
>
> > @@ -3331,6 +3332,29 @@ int uart_get_rs485_mode(struct uart_port *port)
>
> > + devm_gpiod_put(dev, port->rs485_term_gpio);
>
> > + port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
>
> Using devm_*() in uart_get_rs485_mode() seems not right.
> Why do you need this?
uart_get_rs485_mode() is called from a driver's ->probe() hook and we
do not have a corresponding function that is called from a ->remove()
hook where we'd be able to relinquish rs485 resources we've acquired
on probe.
Of course I could add that but it would be more heavy-weight compared
to simply using devm_*(). Do you disagree?
devm_gpiod_put() isn't strictly necessary here. It is only necessary
if one of the drivers would invoke uart_get_rs485_mode() multiple
times, which none of them does AFAICS. It's just a safety measure.
I can drop it if that is preferred.
> > + GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
>
> Parameter has a specific macro GPIOD_OUT_HIGH.
Good point. It's also occurred to me now that reading the GPIO's
value after changing its direction to output is nonsense. If anything
it ought to be read *before* changing the direction to output.
That would make sense in case the board has a pullup or pulldown on
the Termination Enable pin. In other cases the pin may just float
and the value will be unpredictable. However if I do not read the
pin, I'd have to choose either high or low as initial state. Hm.
Let me check back with our hardware engineers today and see what they
recommend.
Thanks,
Lukas
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 4/4] serial: 8250: Support rs485 bus termination GPIO
2020-05-06 6:29 ` Lukas Wunner
@ 2020-05-06 10:06 ` Andy Shevchenko
0 siblings, 0 replies; 6+ messages in thread
From: Andy Shevchenko @ 2020-05-06 10:06 UTC (permalink / raw)
To: Lukas Wunner
Cc: Greg Kroah-Hartman, Jiri Slaby, Rob Herring, Matwey V. Kornilov,
Giulio Benetti, Heiko Stuebner, Christoph Muellner, Jan Kiszka,
devicetree, linux-serial
On Wed, May 06, 2020 at 08:29:43AM +0200, Lukas Wunner wrote:
> On Tue, May 05, 2020 at 07:10:35PM +0300, Andy Shevchenko wrote:
> > On Tue, May 05, 2020 at 04:42:04PM +0200, Lukas Wunner wrote:
...
> > > + devm_gpiod_put(dev, port->rs485_term_gpio);
> >
> > > + port->rs485_term_gpio = devm_gpiod_get_optional(dev, "rs485-term",
> >
> > Using devm_*() in uart_get_rs485_mode() seems not right.
> > Why do you need this?
>
> uart_get_rs485_mode() is called from a driver's ->probe() hook and we
> do not have a corresponding function that is called from a ->remove()
> hook where we'd be able to relinquish rs485 resources we've acquired
> on probe.
>
> Of course I could add that but it would be more heavy-weight compared
> to simply using devm_*(). Do you disagree?
>
> devm_gpiod_put() isn't strictly necessary here. It is only necessary
> if one of the drivers would invoke uart_get_rs485_mode() multiple
> times, which none of them does AFAICS. It's just a safety measure.
> I can drop it if that is preferred.
I think putting and re-requesting here is also racy. Somebody can request the
very same GPIO in between (for example crazy user space tool).
Setting the same value many times won't hurt.
> > > + GPIOD_FLAGS_BIT_DIR_SET | GPIOD_FLAGS_BIT_DIR_OUT);
> >
> > Parameter has a specific macro GPIOD_OUT_HIGH.
>
> Good point. It's also occurred to me now that reading the GPIO's
> value after changing its direction to output is nonsense. If anything
> it ought to be read *before* changing the direction to output.
It's not a complete nonsense, depends what you actually want to achieve here.
> That would make sense in case the board has a pullup or pulldown on
> the Termination Enable pin. In other cases the pin may just float
> and the value will be unpredictable. However if I do not read the
> pin, I'd have to choose either high or low as initial state. Hm.
> Let me check back with our hardware engineers today and see what they
> recommend.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-05-06 10:06 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-05-05 14:42 [PATCH 0/4] rs485 bus termination GPIO Lukas Wunner
2020-05-05 14:42 ` [PATCH 3/4] dt-bindings: serial: Add binding for " Lukas Wunner
2020-05-05 14:42 ` [PATCH 4/4] serial: 8250: Support " Lukas Wunner
2020-05-05 16:10 ` Andy Shevchenko
2020-05-06 6:29 ` Lukas Wunner
2020-05-06 10:06 ` Andy Shevchenko
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).