Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers
From: Stefan Roese
Date: Wed Jun 12 2019 - 04:17:50 EST
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