Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1311025

Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250

Path csiph.com!aioe.org!bofh.it!news.nic.it!robomod
From "Matwey V. Kornilov" <matwey@sai.msu.ru>
Newsgroups linux.kernel
Subject Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250
Date Sat, 16 Jan 2016 21:30:02 +0100
Message-ID <qRFDY-VC-9@gated-at.bofh.it> (permalink)
References <qIdnA-1xE-23@gated-at.bofh.it> <qIdnB-1xE-43@gated-at.bofh.it> <qRfgu-8O-9@gated-at.bofh.it> <qRhBE-1AN-3@gated-at.bofh.it> <qRixJ-2cl-37@gated-at.bofh.it> <qRiR4-2yH-7@gated-at.bofh.it> <qRjWO-3cE-13@gated-at.bofh.it> <qRkSR-3Rl-1@gated-at.bofh.it> <qRufw-1NG-11@gated-at.bofh.it> <qREeS-8iK-5@gated-at.bofh.it>
Dkim-Signature v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc:content-type; bh=D/3rbO0KM5N26s7cxh0Jrf2JKrUbbWZuCUX4oQ0MeTg=; b=0FVB6xoRDrMCM7Ohb1qpt3kqpSaHVT0OxaJ1zolWtvF5HeQ3fQMkqmGjj7g0Hptd+f qIhZzyQgeS8afT62+a7Z/7bdmx0Brwm6yiC5rPNA2MQfHpdEFqwpDkwmdjJ9vn9MiCWP 7dYkdgwHbcTsjiWv2o6v2Ha71PjD1Aqo+VTRmOy6FyAiVUc4SPfRS7sQlC8ldP/lR8fL Co3GhsMcYW9FIp/QAZfXvV6lICA10Hqu6kW+L+I6VdIFIV7LYnJB8ZRTFaAdDN8HkV+u BqgT+q9j7JCD91puAM1UjrWazaDinpA7BtHXhCqXDgJhj1YgLApoJAaPCiWoxPO94ZLe rpPg==
X-Google-Dkim-Signature v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc:content-type; bh=D/3rbO0KM5N26s7cxh0Jrf2JKrUbbWZuCUX4oQ0MeTg=; b=WX1qTqd8s29YCT8ilFILSHT0q7z4keyxf81nRHE2H3ot7hv5K0QkCcW8yBXtrtajOz E9dsIpf+DuesZz1c8jLGhZevjgiTD3AQWyzq7KiIe64OfRvpeyvYX1GybdaBZnQbNAWn YsPEIpFdVjq38zbfoPTDuTPXDQwVWmtH7TgeLYO/c/JTxrS/vyBRvA2vnE5wBT5f/J44 vq+mZkpiBm1kO95ysq8Fbk2XWSmw1PqVe32QljNcOMa2VQ2s+RwzUSBlrPEHBApQRfon vf66LtbZ0qEdFDzfsJ30DOwNhPh5Awnbr8PChA3joY5SLr8BL525MewRx12YONlAJRiF tt6A==
X-Gm-Message-State AG10YOTvanm4vgBzColhi95ChV5bP6jPyQXTf6b6zR1rLaCsRkEId30lydZm2nU0xI8uLcwQrLO8C/g7axN7CA==
X-Received by 10.28.97.11 with SMTP id v11mr5531121wmb.42.1452976118264; Sat, 16 Jan 2016 12:28:38 -0800 (PST)
MIME-Version 1.0
X-Google-Sender-Auth 2vAccQGhP66TiliO58mvw8mqGh0
Content-Type text/plain; charset=UTF-8
Sender robomod@news.nic.it
List-ID <linux-kernel.vger.kernel.org>
X-Mailing-List linux-kernel@vger.kernel.org
Approved robomod@news.nic.it
Lines 501
Organization linux.* mail to news gateway
X-Original-Cc Greg KH <gregkh@linuxfoundation.org>, Jiri Slaby <jslaby@suse.com>, Andy Shevchenko <andy.shevchenko@gmail.com>, One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>, linux-kernel <linux-kernel@vger.kernel.org>, linux-serial@vger.kernel.org
X-Original-Date Sat, 16 Jan 2016 23:28:18 +0300
X-Original-Message-ID <CAJs94EZMGnxhw6dDzYbtMF2HMTzPd0AONiQAeKMNaE2_zaVyxw@mail.gmail.com>
X-Original-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> <CAJs94Ea3MfLOniYEe39MisTdV=0pZ2CsRR5PBnCbanLU5ng5GQ@mail.gmail.com> <56994C4F.7040008@hurleysoftware.com> <CAJs94EYv-oVh1pOSU4ZqigMDQiSHL8gPzeJy_4J=csydgncuqg@mail.gmail.com> <CAJs94EbL6iRLE3=09vFDwRc=MvaORj0D_SuDuDP2F4k=AL-+Rw@mail.gmail.com> <56996FE5.8020604@hurleysoftware.com> <CAJs94EZwneXs1S5EpaqV8dcYNw1rjSiC0XqLjS5cLu1t=F2DJQ@mail.gmail.com> <569A9255.6040207@hurleysoftware.com>
X-Original-Sender linux-kernel-owner@vger.kernel.org
Xref csiph.com linux.kernel:1311025

Show key headers only | View raw


2016-01-16 21:56 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
> On 01/16/2016 12:12 AM, Matwey V. Kornilov wrote:
>> 2016-01-16 1:17 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>> On 01/15/2016 01:16 PM, Matwey V. Kornilov wrote:
>>>> 2016-01-15 23:01 GMT+03:00 Matwey V. Kornilov <matwey@sai.msu.ru>:
>>>>> 2016-01-15 22:45 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>> On 01/15/2016 10:42 AM, Matwey V. Kornilov wrote:
>>>>>>> 2016-01-15 19:14 GMT+03:00 Peter Hurley <peter@hurleysoftware.com>:
>>>>>>>> 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.
>>>>>>>>
>>>>>>>>> Signed-off-by: Matwey V. Kornilov <matwey@sai.msu.ru>
>>>>>>>>> ---
>>>>>>>>>  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 <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.
>>>>>>>>
>>>>>>>>
>>>>>>>>> +{
>>>>>>>>> +     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).
>>>>>>>
>>>>>>> This is required to call del_timer_sync(&p->em485->start_tx_timer);
>>>>>>> from __stop_tx_rs485
>>>>>>
>>>>>> I know; that doesn't mean it's ok.
>>>>>>
>>>>>
>>>>> What do you suggest? Run __stop_tx as a tasklet? I am not sure whether
>>>>> it introduces races or not.
>>>>
>>>> Would it be fine to use workqueues instead of timers? I mean
>>>> schedule_delayed_work and cancel_delayed_work_sync.
>>>> They use same timers with TIMER_IRQSAFE under the hood.
>>>> Or it is better to allocate separate work queue in order to achieve
>>>> better latency than shared system wq can provide?
>>>
>>> I think just del_timer() and locking with the port lock should be
>>> sufficient; timer + irq handler is nothing new.
>>>
>>
>> Do I understand correctly, that internals of
>> serial8250_em485_handle_stop_tx and serial8250_em485_handle_start_tx
>> should be wrapped with port->lock in order to ensure that they are not
>> running during the call going to run del_timer?
>
> Yes.
>
> Of course, you'll need some state mechanism to know in the timer function
> that the timer was cancelled. For example, in this situation

Sure, that's why I asked.
I've looked through the timer implementation, and found that I need an
additional variable to keep the state.
The running timer is indistinguishable from deleted one. Initially, I
hoped to check corresponding timer_list variable from timer function,
but this would not work.

>
> CPU 0                                CPU 1
>
> start_tx_rs485()                     [timer fires]
>   del_timer(stop_tx_timer)
>                                      handle_stop_tx()
>                                        spin_lock_irqsave(port lock)
>                                        *waits*
>   rts_on_send()
>   mod_timer(start_tx_timer)
>                                        *claims port lock*
>
>                                        * obviously would be bad if  *
>                                        * do_stop_tx_rs485() ran now *
>
>
>
>>>>>>>>> +     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?
>>>>>>>
>>>>>>> Both start_tx and rs485_config (which calls destroy) are wrapped with
>>>>>>> port->lock in serial_core.c
>>>>>>
>>>>>> Ahh, missed that.
>>>>>>
>>>>>> Maybe it would be better simply to implement the config_rs485()
>>>>>> generically, and just call it from the omap_8250 config_rs485().
>>>>>>
>>>>>> And put a note about the locking in a function comment header
>>>>>>
>>>>>> /**
>>>>>>  *      serial8250_config_em485()       -       rs485 config helper
>>>>>>  *
>>>>>>  *      ....
>>>>>>  */
>>>>>>
>>>>>>
>>>>>>
>>>>>>>>> +     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.
>>>>>>>
>>>>>>> I use double __ for consistency with __start_tx. Now I have:
>>>>>>>
>>>>>>>         if (up->em485)
>>>>>>>                 __start_tx_rs485(port);
>>>>>>>         else
>>>>>>>                 __start_tx(port);
>>>>>>
>>>>>> But __start_tx() is labelled that way to differentiate it from being identified
>>>>>> as the start_tx() handler (which is serial8250_start_tx()). IOW, contributors
>>>>>> unfamiliar with the 8250 driver itself won't become confused when grepping
>>>>>> for start_tx (or at least the idea is to minimize that confusion).
>>>>>>
>>>>>> start_tx_rs485() doesn't need differentiation, so doesn't require the
>>>>>> double __ leader.
>>>>>>
>>>>>> FWIW, this is consistent and typical elsewhere in the kernel.
>>>>>>
>>>>>>
>>>>>>>>> +{
>>>>>>>>> +     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 (!(....) !=
>>>>>>>>             !(....)) {
>>>>>>>>
>>>>>>>
>>>>>>> I would like to keep the double negation, in my opinion it is more
>>>>>>> clear to the reader and I believe that the compiler is able to
>>>>>>> optimize it.
>>>>>>>
>>>>>>>>> +             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?
>>>>>>>>
>>>>>>>
>>>>>>> Why not?
>>>>>>
>>>>>> The expected yardstick for inline is some demonstrable speed improvement;
>>>>>> otherwise, size is favored.
>>>>>>
>>>>>> And __stop_tx() is already inlined in 3 places, which really doesn't
>>>>>> need inlining either -- a call/ret is nothing compared to device i/o.
>>>>>>
>>>>>
>>>>> Ok then, probably I am biased with my C++ experience and I am used to
>>>>> think that compiler considers `inline` only as a hint.
>>>>>
>>>>>>
>>>>>> Regards,
>>>>>> Peter Hurley
>>>>>>
>>>>>>>>> +{
>>>>>>>>> +     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;
>>>>>>>>
>>>>>>>>> +
>>>>>>>>> +     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)
>>>>>>>>>
>>>
>>
>>
>>
>



-- 
With best regards,
Matwey V. Kornilov.
Sternberg Astronomical Institute, Lomonosov Moscow State University, Russia
119991, Moscow, Universitetsky pr-k 13, +7 (495) 9392382

Back to linux.kernel | Previous | NextPrevious in thread | Next in thread | Find similar | Unroll thread


Thread

Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Peter Hurley <peter@hurleysoftware.com> - 2016-01-15 17:20 +0100
  Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Peter Hurley <peter@hurleysoftware.com> - 2016-01-15 17:50 +0100
  Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-01-15 19:50 +0100
    Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Peter Hurley <peter@hurleysoftware.com> - 2016-01-15 20:50 +0100
      Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-01-15 21:10 +0100
        Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-01-15 22:20 +0100
          Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Peter Hurley <peter@hurleysoftware.com> - 2016-01-15 23:20 +0100
            Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-01-16 09:20 +0100
              Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Peter Hurley <peter@hurleysoftware.com> - 2016-01-16 20:00 +0100
                Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-01-16 21:30 +0100
      Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 "Matwey V. Kornilov" <matwey@sai.msu.ru> - 2016-01-15 21:30 +0100
        Re: [PATCH v6 2/3] tty: Add software emulated RS485 support for 8250 Peter Hurley <peter@hurleysoftware.com> - 2016-01-15 23:00 +0100

csiph-web