Messages in this thread Patch in this message | | | Subject | Re: [PATCH 2/2 v5] tty/serial/8250: use mctrl_gpio helpers | From | Stefan Roese <> | Date | Wed, 12 Jun 2019 10:13:05 +0200 |
| |
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
| |