From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754512AbcAOQk7 (ORCPT ); Fri, 15 Jan 2016 11:40:59 -0500 Received: from mail-pa0-f49.google.com ([209.85.220.49]:32769 "EHLO mail-pa0-f49.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754435AbcAOQk5 (ORCPT ); Fri, 15 Jan 2016 11:40:57 -0500 Subject: Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 To: "Matwey V. Kornilov" References: <1450722379-13438-1-git-send-email-matwey@sai.msu.ru> <1450722379-13438-3-git-send-email-matwey@sai.msu.ru> <56991AE5.50503@hurleysoftware.com> Cc: gregkh@linuxfoundation.org, jslaby@suse.com, andy.shevchenko@gmail.com, gnomes@lxorguk.ukuu.org.uk, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org From: Peter Hurley Message-ID: <56992115.4030107@hurleysoftware.com> Date: Fri, 15 Jan 2016 08:40:53 -0800 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <56991AE5.50503@hurleysoftware.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 01/15/2016 08:14 AM, Peter Hurley wrote: > 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. More comments below. >> Signed-off-by: Matwey V. Kornilov >> --- >> 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) > .... > >> +} >> >> #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 >> #include >> #include >> +#include >> >> #include >> #include >> @@ -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. > > >> +{ >> + 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? > >> + 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. > >> + } 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) > >> + 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; Actually, just refactor the mod_timer() below into __start_tx_rs485(); 'delay' and assignment-in-conditional goes away. >> + >> + 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 > >> +} >> + >> 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) >> >