lkml.org 
[lkml]   [2019]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
SubjectRe: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From
Date
On 11.06.19 16:48, Andy Shevchenko wrote:
> On Tue, Jun 11, 2019 at 04:02:54PM +0200, Stefan Roese wrote:
>> On 11.06.19 14:44, Andy Shevchenko wrote:
>>> On Tue, Jun 11, 2019 at 12:56:03PM +0200, Stefan Roese wrote:
>
>>>> static inline void serial8250_out_MCR(struct uart_8250_port *up, int value)
>>>> {
>>>> serial_out(up, UART_MCR, value);
>>>> +
>>>> + if (up->gpios) {
>>>> + int mctrl_gpio = 0;
>>>> +
>>>> + if (value & UART_MCR_RTS)
>>>> + mctrl_gpio |= TIOCM_RTS;
>>>> + if (value & UART_MCR_DTR)
>>>> + mctrl_gpio |= TIOCM_DTR;
>>>> +
>>>> + mctrl_gpio_set(up->gpios, mctrl_gpio);
>>>> + }
>>>> }
>
>>>> static inline int serial8250_in_MCR(struct uart_8250_port *up)
>>>> {
>>>> - return serial_in(up, UART_MCR);
>>>> + int mctrl;
>>>> +
>>>> + mctrl = serial_in(up, UART_MCR);
>>>> +
>>>> + if (up->gpios) {
>>>> + int mctrl_gpio = 0;
>>>> +
>>>> + /* save current MCR values */
>>>> + if (mctrl & UART_MCR_RTS)
>>>> + mctrl_gpio |= TIOCM_RTS;
>>>> + if (mctrl & UART_MCR_DTR)
>>>> + mctrl_gpio |= TIOCM_DTR;
>>>> +
>>>> + mctrl_gpio = mctrl_gpio_get_outputs(up->gpios, &mctrl_gpio);
>>>> + if (mctrl_gpio & TIOCM_RTS)
>>>> + mctrl |= UART_MCR_RTS;
>>>> + else
>>>> + mctrl &= ~UART_MCR_RTS;
>>>> +
>>>> + if (mctrl_gpio & TIOCM_DTR)
>>>> + mctrl |= UART_MCR_DTR;
>>>> + else
>>>> + mctrl &= ~UART_MCR_DTR;
>>>> + }
>>>> +
>>>> + return mctrl;
>>>> }
>>>
>>> These are using OR logic with potentially volatile data. Shouldn't we mask
>>> unused bits in UART_MCR in case of up->gpios != NULL?
>>
>> Sorry, I don't see, which bits you are referring to? Could you please be
>> a bit more specific with the variable / macro meant (example)?
>
> I meant that we double write values in the out() which might have some
> consequences, though I hope nothing wrong with it happens.

Where is the double write to a register? Sorry, I fail to spot it.

> In the in() we read the all bits in the register.
>
> As now I look at the implementation of mctrl_gpio_get_outputs(),
> I think we rather get helpers for conversion between TIOCM and UART_MCR values,
> so, they can be used in get_mctrl() / set_mctrl() and above.

Do you something like this in mind?

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index dc9354e34b60..f44561fcb941 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1954,19 +1954,12 @@ unsigned int serial8250_do_get_mctrl(struct uart_port *port)
status = serial8250_modem_status(up);
serial8250_rpm_put(up);

- ret = 0;
-
if (up->gpios)
return mctrl_gpio_get(up->gpios, &ret);

- if (status & UART_MSR_DCD)
- ret |= TIOCM_CAR;
- if (status & UART_MSR_RI)
- ret |= TIOCM_RNG;
- if (status & UART_MSR_DSR)
- ret |= TIOCM_DSR;
- if (status & UART_MSR_CTS)
- ret |= TIOCM_CTS;
+ ret = UART_MSR_TO_TIOCM_DCD(status) | UART_MSR_TO_TIOCM_RI(status) |
+ UART_MSR_TO_TIOCM_DSR(status) | UART_MSR_TO_TIOCM_CTS(status);
+
return ret;
}
EXPORT_SYMBOL_GPL(serial8250_do_get_mctrl);
@@ -1983,16 +1976,9 @@ void serial8250_do_set_mctrl(struct uart_port *port, unsigned int mctrl)
struct uart_8250_port *up = up_to_u8250p(port);
unsigned char mcr = 0;

- if (mctrl & TIOCM_RTS)
- mcr |= UART_MCR_RTS;
- if (mctrl & TIOCM_DTR)
- mcr |= UART_MCR_DTR;
- if (mctrl & TIOCM_OUT1)
- mcr |= UART_MCR_OUT1;
- if (mctrl & TIOCM_OUT2)
- mcr |= UART_MCR_OUT2;
- if (mctrl & TIOCM_LOOP)
- mcr |= UART_MCR_LOOP;
+ mcr = TIOCM_TO_UART_MCR_RTS(mctrl) | TIOCM_TO_UART_MCR_DTR(mctrl) |
+ TIOCM_TO_UART_MCR_OUT1(mctrl) | TIOCM_TO_UART_MCR_OUT2(mctrl) |
+ TIOCM_TO_UART_MCR_LOOP(mctrl);

mcr = (mcr & up->mcr_mask) | up->mcr_force | up->mcr;

diff --git a/include/uapi/linux/serial_reg.h b/include/uapi/linux/serial_reg.h
index be07b5470f4b..bda905a1b765 100644
--- a/include/uapi/linux/serial_reg.h
+++ b/include/uapi/linux/serial_reg.h
@@ -376,5 +376,22 @@
#define UART_ALTR_EN_TXFIFO_LW 0x01 /* Enable the TX FIFO Low Watermark */
#define UART_ALTR_TX_LOW 0x41 /* Tx FIFO Low Watermark */

+#define UART_MSR_TO_TIOCM_DCD(val) ((val & UART_MSR_DCD) ? TIOCM_CAR : 0)
+#define UART_MSR_TO_TIOCM_RI(val) ((val & UART_MSR_RI) ? TIOCM_RNG : 0)
+#define UART_MSR_TO_TIOCM_DSR(val) ((val & UART_MSR_DSR) ? TIOCM_DSR : 0)
+#define UART_MSR_TO_TIOCM_CTS(val) ((val & UART_MSR_CTS) ? TIOCM_CTS : 0)
+#define UART_MSR_TO_TIOCM_RTS(val) ((val & UART_MSR_RTS) ? TIOCM_RTS : 0)
+#define UART_MSR_TO_TIOCM_DTR(val) ((val & UART_MSR_DTR) ? TIOCM_DTR : 0)
+
+#define TIOCM_TO_UART_MCR_DCD(val) ((val & TIOCM_DCD) ? UART_MCR_CAR : 0)
+#define TIOCM_TO_UART_MCR_RI(val) ((val & TIOCM_RI) ? UART_MCR_RNG : 0)
+#define TIOCM_TO_UART_MCR_DSR(val) ((val & TIOCM_DSR) ? UART_MCR_DSR : 0)
+#define TIOCM_TO_UART_MCR_CTS(val) ((val & TIOCM_CTS) ? UART_MCR_CTS : 0)
+#define TIOCM_TO_UART_MCR_RTS(val) ((val & TIOCM_RTS) ? UART_MCR_RTS : 0)
+#define TIOCM_TO_UART_MCR_DTR(val) ((val & TIOCM_DTR) ? UART_MCR_DTR : 0)
+#define TIOCM_TO_UART_MCR_LOOP(val) ((val & TIOCM_LOOP) ? UART_MCR_LOOP : 0)
+#define TIOCM_TO_UART_MCR_OUT1(val) ((val & TIOCM_OUT1) ? UART_MCR_OUT1 : 0)
+#define TIOCM_TO_UART_MCR_OUT2(val) ((val & TIOCM_OUT2) ? UART_MCR_OUT2 : 0)
+
#endif /* _LINUX_SERIAL_REG_H */

Plus the use of these macros in this patch of course.

If yes, then I'll add a new patch (pretty similar to the one included
above) to this series.

Thanks,
Stefan

\
 
 \ /
  Last update: 2019-06-12 10:14    [W:0.033 / U:26.560 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site