Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
From: Peter Hurley <hidden>
Date: 2016-01-15 16:14:35
Also in:
lkml
On 12/21/2015 10:26 AM, Matwey V. Kornilov wrote:
Implementation of software emulation of RS485 direction handling is based on omap_serial driver. Before and after transmission RTS is set to the appropriate value. Note that before calling serial8250_em485_init the caller has to ensure that UART will interrupt when shift register empty. Otherwise, emultaion cannot be used. Both serial8250_em485_init and serial8250_em485_destroy are idempotent functions.
Apologies for the long delay; comments below.
quoted hunk ↗ jump to hunk
Signed-off-by: Matwey V. Kornilov <redacted> --- drivers/tty/serial/8250/8250.h | 6 ++ drivers/tty/serial/8250/8250_port.c | 161 +++++++++++++++++++++++++++++++++++- include/linux/serial_8250.h | 7 ++ 3 files changed, 170 insertions(+), 4 deletions(-)diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h index d54dcd8..0189cb3 100644 --- a/drivers/tty/serial/8250/8250.h +++ b/drivers/tty/serial/8250/8250.h@@ -117,6 +117,12 @@ static inline void serial_dl_write(struct uart_8250_port *up, int value) struct uart_8250_port *serial8250_get_port(int line); void serial8250_rpm_get(struct uart_8250_port *p); void serial8250_rpm_put(struct uart_8250_port *p); +int serial8250_em485_init(struct uart_8250_port *p); +void serial8250_em485_destroy(struct uart_8250_port *p); +static inline bool serial8250_em485_enabled(struct uart_8250_port *p) +{ + return p->em485 && (p->port.rs485.flags & SER_RS485_ENABLED);
Under what circumstances is p->em485 != NULL but (p->port.rs485.flags & SER_RS485_ENABLED) is true? ISTM, p->em485 is necessary and sufficient to determine if em485 is enabled. In which case, this function can be eliminated and callers can be reduced to if (p->em485) ....
quoted hunk ↗ jump to hunk
+} #if defined(__alpha__) && !defined(CONFIG_PCI) /*diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c index 8ad0b2d..d67a848 100644 --- a/drivers/tty/serial/8250/8250_port.c +++ b/drivers/tty/serial/8250/8250_port.c@@ -37,6 +37,7 @@ #include <linux/slab.h> #include <linux/uaccess.h> #include <linux/pm_runtime.h> +#include <linux/timer.h> #include <asm/io.h> #include <asm/irq.h>@@ -504,6 +505,31 @@ static void serial8250_clear_fifos(struct uart_8250_port *p) } } +static inline void serial8250_em485_rts_on_send(struct uart_8250_port *p)
Only one call site, so please drop inline.
+{
+ unsigned char mcr = serial_in(p, UART_MCR);
+
+ if (p->port.rs485.flags & SER_RS485_RTS_ON_SEND)
+ mcr |= UART_MCR_RTS;
+ else
+ mcr &= ~UART_MCR_RTS;
+ serial_out(p, UART_MCR, mcr);
+}
+
+static inline void serial8250_em485_rts_after_send(struct uart_8250_port *p)Doesn't really need to be inline.
quoted hunk ↗ jump to hunk
+{ + unsigned char mcr = serial_in(p, UART_MCR); + + if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND) + mcr |= UART_MCR_RTS; + else + mcr &= ~UART_MCR_RTS; + serial_out(p, UART_MCR, mcr); +} + +static void serial8250_em485_handle_start_tx(unsigned long arg); +static void serial8250_em485_handle_stop_tx(unsigned long arg); + void serial8250_clear_and_reinit_fifos(struct uart_8250_port *p) { serial8250_clear_fifos(p);@@ -528,6 +554,42 @@ void serial8250_rpm_put(struct uart_8250_port *p) } EXPORT_SYMBOL_GPL(serial8250_rpm_put); +int serial8250_em485_init(struct uart_8250_port *p) +{ + if (p->em485 != NULL) + return 0; + + p->em485 = kmalloc(sizeof(struct uart_8250_em485), GFP_KERNEL); + if (p->em485 == NULL) + return -ENOMEM; + + init_timer(&p->em485->stop_tx_timer); + p->em485->stop_tx_timer.function = serial8250_em485_handle_stop_tx; + p->em485->stop_tx_timer.data = (unsigned long)p; + p->em485->stop_tx_timer.flags |= TIMER_IRQSAFE;
Not sure this is going to fly; this would be the only user of TIMER_IRQSAFE (which was specifically introduced to workaround workqueue issues and not meant for general use).
+ init_timer(&p->em485->start_tx_timer); + p->em485->start_tx_timer.function = serial8250_em485_handle_start_tx; + p->em485->start_tx_timer.data = (unsigned long)p; + p->em485->start_tx_timer.flags |= TIMER_IRQSAFE; + + serial8250_em485_rts_after_send(p); + + return 0; +} +EXPORT_SYMBOL_GPL(serial8250_em485_init);
Newline.
+void serial8250_em485_destroy(struct uart_8250_port *p)
+{
+ if (p->em485 == NULL)
+ return;
+
+ del_timer_sync(&p->em485->start_tx_timer);
+ del_timer_sync(&p->em485->stop_tx_timer);What keeps start_tx() from restarting a new timer right here?
quoted hunk ↗ jump to hunk
+ kfree(p->em485); + p->em485 = NULL; +} +EXPORT_SYMBOL_GPL(serial8250_em485_destroy); + /* * These two wrappers ensure that enable_runtime_pm_tx() can be called more than * once and disable_runtime_pm_tx() will still disable RPM because the fifo is@@ -1293,7 +1355,61 @@ static void serial8250_stop_rx(struct uart_port *port) serial8250_rpm_put(up); } -static inline void __stop_tx(struct uart_8250_port *p) +static __u32 __start_tx_rs485(struct uart_8250_port *p)
^^^^^ No need to preserve the userspace type here. The double underline leader in an identifier is typically used to distinguish an unlocked version from a locked version. I don't think it's necessary here or any of the other newly-introduced functions.
+{
+ if (!serial8250_em485_enabled(p))
+ return 0;Already checked that em485 was enabled in lone caller.
+ if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_stop_rx(&p->port);
+
+ del_timer_sync(&p->em485->stop_tx_timer);
+
+ if (!!(p->port.rs485.flags & SER_RS485_RTS_ON_SEND) != !!(serial_in(p, UART_MCR) & UART_MCR_RTS)) {
Line too long. And just one negation is sufficient, ie.
if (!(....) !=
!(....)) {
+ serial8250_em485_rts_on_send(p); + return p->port.rs485.delay_rts_before_send; + } + + return 0; +} + +static inline void __do_stop_tx_rs485(struct uart_8250_port *p)
Does this really need to be inline?
+{
+ if (!serial8250_em485_enabled(p))
+ return;
+
+ serial8250_em485_rts_after_send(p);
+ /*
+ * Empty the RX FIFO, we are not interested in anything
+ * received during the half-duplex transmission.
+ */Malformed block comment. /* * * */
+ if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ serial8250_clear_fifos(p);
+}
+
+static void serial8250_em485_handle_stop_tx(unsigned long arg)
+{
+ struct uart_8250_port *p = (struct uart_8250_port *)arg;
+
+ __do_stop_tx_rs485(p);
+}
+
+static inline void __stop_tx_rs485(struct uart_8250_port *p)Single caller so drop inline.
+{
+ if (!serial8250_em485_enabled(p))
+ return;
+
+ del_timer_sync(&p->em485->start_tx_timer);
+
+ /* __do_stop_tx_rs485 is going to set RTS according to config AND flush RX FIFO if required */Block comment.
+ if (p->port.rs485.delay_rts_after_send > 0) {
+ mod_timer(&p->em485->stop_tx_timer, jiffies + p->port.rs485.delay_rts_after_send * HZ / 1000);Line too long; please re-format. This is one problem with overly long identifiers.
quoted hunk ↗ jump to hunk
+ } else { + __do_stop_tx_rs485(p); + } +} + +static inline void __do_stop_tx(struct uart_8250_port *p) { if (p->ier & UART_IER_THRI) { p->ier &= ~UART_IER_THRI;@@ -1302,6 +1418,21 @@ static inline void __stop_tx(struct uart_8250_port *p) } } +static inline void __stop_tx(struct uart_8250_port *p) +{ + if (serial8250_em485_enabled(p)) { + unsigned char lsr = serial_in(p, UART_LSR); + /* To provide required timeing and allow FIFO transfer, + * __stop_tx_rs485 must be called only when both FIFO and shift register + * are empty. It is for device driver to enable interrupt on TEMT. + */
Block indent. This code path should cancel start timer also.
+ if (!((lsr & UART_LSR_TEMT) && (lsr & UART_LSR_THRE)))
if ((lsr & BOTH_EMPTY) != BOTH_EMPTY)
quoted hunk ↗ jump to hunk
+ return; + } + __do_stop_tx(p); + __stop_tx_rs485(p); +} + static void serial8250_stop_tx(struct uart_port *port) { struct uart_8250_port *up = up_to_u8250p(port);@@ -1319,12 +1450,10 @@ static void serial8250_stop_tx(struct uart_port *port) serial8250_rpm_put(up); } -static void serial8250_start_tx(struct uart_port *port) +static inline void __start_tx(struct uart_port *port) { struct uart_8250_port *up = up_to_u8250p(port); - serial8250_rpm_get_tx(up); - if (up->dma && !up->dma->tx_dma(up)) return;@@ -1350,6 +1479,30 @@ static void serial8250_start_tx(struct uart_port *port) } } +static void serial8250_em485_handle_start_tx(unsigned long arg) +{ + struct uart_8250_port *p = (struct uart_8250_port *)arg; + + __start_tx(&p->port); +} + +static void serial8250_start_tx(struct uart_port *port) +{ + struct uart_8250_port *up = up_to_u8250p(port); + __u32 delay;
int delay;
+
+ serial8250_rpm_get_tx(up);
+
+ if (up->em485 && timer_pending(&up->em485->start_tx_timer))
+ return;
+
+ if (up->em485 && (delay = __start_tx_rs485(up))) {No assignment in conditional please.
+ mod_timer(&up->em485->start_tx_timer, jiffies + delay * HZ / 1000);
+ } else {
+ __start_tx(port);
+ }Generally, braces aren't used for single statement if..else. That probably won't apply here after removing the assignment-in-conditional, but I thought it worth mentioning just so you know. Regards, Peter Hurley
quoted hunk ↗ jump to hunk
+} + static void serial8250_throttle(struct uart_port *port) { port->throttle(port);diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h index faa0e03..71516ec 100644 --- a/include/linux/serial_8250.h +++ b/include/linux/serial_8250.h@@ -76,6 +76,11 @@ struct uart_8250_ops { void (*release_irq)(struct uart_8250_port *); }; +struct uart_8250_em485 { + struct timer_list start_tx_timer; /* "rs485 start tx" timer */ + struct timer_list stop_tx_timer; /* "rs485 stop tx" timer */ +}; + /* * This should be used by drivers which want to register * their own 8250 ports without registering their own@@ -122,6 +127,8 @@ struct uart_8250_port { /* 8250 specific callbacks */ int (*dl_read)(struct uart_8250_port *); void (*dl_write)(struct uart_8250_port *, int); + + struct uart_8250_em485 *em485; }; static inline struct uart_8250_port *up_to_u8250p(struct uart_port *up)