Add basic support for RS485: Provide a callback to configure RS485
settings. Handle the RS485 specific part in the functions
pl011_rs485_tx_start() and pl011_rs485_tx_stop() which extend the generic
start/stop callbacks.
Beside via IOCTL from userspace RS485 can be enabled by means of the
device tree property "rs485-enabled-at-boot-time".
Signed-off-by: Lino Sanfilippo <[email protected]>
---
This patch applies against Gregs tty-testing branch and was tested on a
Raspberry Pi CM3.
Changes in V2:
- clamp RTS delays to 100ms as suggested by Jiri Slaby
- instead of counting bits "by hand" use the new function tty_get_frame_size()
(also suggested by Jiri)
- use the term RS485 consistently in the commit message
- remove one blank line
drivers/tty/serial/amba-pl011.c | 152 +++++++++++++++++++++++++++++++-
1 file changed, 150 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
index e14f3378b8a0..92c6b08d56fb 100644
--- a/drivers/tty/serial/amba-pl011.c
+++ b/drivers/tty/serial/amba-pl011.c
@@ -265,6 +265,8 @@ struct uart_amba_port {
unsigned int old_cr; /* state during shutdown */
unsigned int fixed_baud; /* vendor-set fixed baud rate */
char type[12];
+ bool rs485_tx_started;
+ unsigned int rs485_tx_drain_interval; /* usecs */
#ifdef CONFIG_DMA_ENGINE
/* DMA stuff */
bool using_tx_dma;
@@ -275,6 +277,8 @@ struct uart_amba_port {
#endif
};
+static unsigned int pl011_tx_empty(struct uart_port *port);
+
static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
unsigned int reg)
{
@@ -1282,6 +1286,34 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
#define pl011_dma_flush_buffer NULL
#endif
+static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
+{
+ struct uart_port *port = &uap->port;
+ u32 cr;
+
+ /* Wait until hardware tx queue is empty */
+ while (!pl011_tx_empty(port))
+ udelay(uap->rs485_tx_drain_interval);
+
+ if (port->rs485.delay_rts_after_send)
+ mdelay(port->rs485.delay_rts_after_send);
+
+ cr = pl011_read(uap, REG_CR);
+
+ if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+ cr &= ~UART011_CR_RTS;
+ else
+ cr |= UART011_CR_RTS;
+ /* Disable the transmitter and reenable the transceiver */
+ cr &= ~UART011_CR_TXE;
+ cr |= UART011_CR_RXE;
+ pl011_write(cr, uap, REG_CR);
+
+ uap->rs485_tx_started = false;
+
+ return 0;
+}
+
static void pl011_stop_tx(struct uart_port *port)
{
struct uart_amba_port *uap =
@@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
uap->im &= ~UART011_TXIM;
pl011_write(uap->im, uap, REG_IMSC);
pl011_dma_tx_stop(uap);
+
+ if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
+ pl011_rs485_tx_stop(uap);
}
static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
@@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
return true;
}
+static void pl011_rs485_tx_start(struct uart_amba_port *uap)
+{
+ struct uart_port *port = &uap->port;
+ u32 cr;
+
+ /* Enable transmitter */
+ cr = pl011_read(uap, REG_CR);
+ cr |= UART011_CR_TXE;
+ /* Disable receiver if half-duplex */
+ if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ cr &= ~UART011_CR_RXE;
+
+ if (port->rs485.flags & SER_RS485_RTS_ON_SEND)
+ cr &= ~UART011_CR_RTS;
+ else
+ cr |= UART011_CR_RTS;
+
+ pl011_write(cr, uap, REG_CR);
+
+ if (port->rs485.delay_rts_before_send)
+ mdelay(port->rs485.delay_rts_before_send);
+
+ uap->rs485_tx_started = true;
+}
+
/* Returns true if tx interrupts have to be (kept) enabled */
static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
{
@@ -1397,6 +1457,10 @@ static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
return false;
}
+ if ((uap->port.rs485.flags & SER_RS485_ENABLED) &&
+ !uap->rs485_tx_started)
+ pl011_rs485_tx_start(uap);
+
/* If we are using DMA mode, try to send some characters. */
if (pl011_dma_tx_irq(uap))
return true;
@@ -1542,6 +1606,9 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
container_of(port, struct uart_amba_port, port);
unsigned int cr;
+ if (port->rs485.flags & SER_RS485_ENABLED)
+ mctrl &= ~TIOCM_RTS;
+
cr = pl011_read(uap, REG_CR);
#define TIOCMBIT(tiocmbit, uartbit) \
@@ -1763,7 +1830,17 @@ static int pl011_startup(struct uart_port *port)
/* restore RTS and DTR */
cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
- cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
+ cr |= UART01x_CR_UARTEN | UART011_CR_RXE;
+
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
+ cr &= ~UART011_CR_RTS;
+ else
+ cr |= UART011_CR_RTS;
+ } else {
+ cr |= UART011_CR_TXE;
+ }
+
pl011_write(cr, uap, REG_CR);
spin_unlock_irq(&uap->port.lock);
@@ -1864,6 +1941,9 @@ static void pl011_shutdown(struct uart_port *port)
pl011_dma_shutdown(uap);
+ if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
+ pl011_rs485_tx_stop(uap);
+
free_irq(uap->port.irq, uap);
pl011_disable_uart(uap);
@@ -1941,6 +2021,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
unsigned int lcr_h, old_cr;
unsigned long flags;
unsigned int baud, quot, clkdiv;
+ unsigned int bits;
if (uap->vendor->oversampling)
clkdiv = 8;
@@ -1991,18 +2072,29 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
if (uap->fifosize > 1)
lcr_h |= UART01x_LCRH_FEN;
+ bits = tty_get_frame_size(termios->c_cflag);
+
spin_lock_irqsave(&port->lock, flags);
/*
* Update the per-port timeout.
*/
uart_update_timeout(port, termios->c_cflag, baud);
+ /*
+ * Calculate the approximated time it takes to transmit one character
+ * with the given baud rate. We use this as the poll interval when we
+ * wait for the tx queue to empty.
+ */
+ uap->rs485_tx_drain_interval = (bits * 1000 * 1000) / baud;
pl011_setup_status_masks(port, termios);
if (UART_ENABLE_MS(port, termios->c_cflag))
pl011_enable_ms(port);
+ if (port->rs485.flags & SER_RS485_ENABLED)
+ termios->c_cflag &= ~CRTSCTS;
+
/* first, disable everything */
old_cr = pl011_read(uap, REG_CR);
pl011_write(0, uap, REG_CR);
@@ -2124,6 +2216,40 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
return ret;
}
+static int pl011_rs485_config(struct uart_port *port,
+ struct serial_rs485 *rs485)
+{
+ struct uart_amba_port *uap =
+ container_of(port, struct uart_amba_port, port);
+
+ /* pick sane settings if the user hasn't */
+ if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
+ !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
+ rs485->flags |= SER_RS485_RTS_ON_SEND;
+ rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
+ }
+ /* clamp the delays to [0, 100ms] */
+ rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+ rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
+ memset(rs485->padding, 0, sizeof(rs485->padding));
+
+ if (port->rs485.flags & SER_RS485_ENABLED)
+ pl011_rs485_tx_stop(uap);
+
+ /* Set new configuration */
+ port->rs485 = *rs485;
+ /* Make sure auto RTS is disabled */
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ u32 cr = pl011_read(uap, REG_CR);
+
+ cr &= ~UART011_CR_RTSEN;
+ pl011_write(cr, uap, REG_CR);
+ port->status &= ~UPSTAT_AUTORTS;
+ }
+
+ return 0;
+}
+
static const struct uart_ops amba_pl011_pops = {
.tx_empty = pl011_tx_empty,
.set_mctrl = pl011_set_mctrl,
@@ -2588,10 +2714,28 @@ static int pl011_find_free_port(void)
return -EBUSY;
}
+static int pl011_get_rs485_mode(struct uart_amba_port *uap)
+{
+ struct uart_port *port = &uap->port;
+ struct serial_rs485 *rs485 = &port->rs485;
+ int ret;
+
+ ret = uart_get_rs485_mode(port);
+ if (ret)
+ return ret;
+
+ /* clamp the delays to [0, 100ms] */
+ rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
+ rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
+
+ return 0;
+}
+
static int pl011_setup_port(struct device *dev, struct uart_amba_port *uap,
struct resource *mmiobase, int index)
{
void __iomem *base;
+ int ret;
base = devm_ioremap_resource(dev, mmiobase);
if (IS_ERR(base))
@@ -2608,6 +2752,10 @@ static int pl011_setup_port(struct device *dev, struct uart_amba_port *uap,
uap->port.flags = UPF_BOOT_AUTOCONF;
uap->port.line = index;
+ ret = pl011_get_rs485_mode(uap);
+ if (ret)
+ return ret;
+
amba_ports[index] = uap;
return 0;
@@ -2665,7 +2813,7 @@ static int pl011_probe(struct amba_device *dev, const struct amba_id *id)
uap->port.iotype = vendor->access_32b ? UPIO_MEM32 : UPIO_MEM;
uap->port.irq = dev->irq[0];
uap->port.ops = &amba_pl011_pops;
-
+ uap->port.rs485_config = pl011_rs485_config;
snprintf(uap->type, sizeof(uap->type), "PL011 rev%u", amba_rev(dev));
ret = pl011_setup_port(&dev->dev, uap, &dev->res, portnr);
base-commit: b61c8bf4694b5115766849378dcb8787ff54e65e
--
2.31.1
On Fri, Jun 18, 2021 at 04:51:53PM +0200, Lino Sanfilippo wrote:
> Add basic support for RS485: Provide a callback to configure RS485
> settings. Handle the RS485 specific part in the functions
> pl011_rs485_tx_start() and pl011_rs485_tx_stop() which extend the generic
> start/stop callbacks.
> Beside via IOCTL from userspace RS485 can be enabled by means of the
> device tree property "rs485-enabled-at-boot-time".
>
> Signed-off-by: Lino Sanfilippo <[email protected]>
> ---
> This patch applies against Gregs tty-testing branch and was tested on a
> Raspberry Pi CM3.
>
> Changes in V2:
> - clamp RTS delays to 100ms as suggested by Jiri Slaby
> - instead of counting bits "by hand" use the new function tty_get_frame_size()
> (also suggested by Jiri)
> - use the term RS485 consistently in the commit message
> - remove one blank line
>
>
> drivers/tty/serial/amba-pl011.c | 152 +++++++++++++++++++++++++++++++-
> 1 file changed, 150 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/amba-pl011.c b/drivers/tty/serial/amba-pl011.c
> index e14f3378b8a0..92c6b08d56fb 100644
> --- a/drivers/tty/serial/amba-pl011.c
> +++ b/drivers/tty/serial/amba-pl011.c
> @@ -265,6 +265,8 @@ struct uart_amba_port {
> unsigned int old_cr; /* state during shutdown */
> unsigned int fixed_baud; /* vendor-set fixed baud rate */
> char type[12];
> + bool rs485_tx_started;
> + unsigned int rs485_tx_drain_interval; /* usecs */
> #ifdef CONFIG_DMA_ENGINE
> /* DMA stuff */
> bool using_tx_dma;
> @@ -275,6 +277,8 @@ struct uart_amba_port {
> #endif
> };
>
> +static unsigned int pl011_tx_empty(struct uart_port *port);
> +
> static unsigned int pl011_reg_to_offset(const struct uart_amba_port *uap,
> unsigned int reg)
> {
> @@ -1282,6 +1286,34 @@ static inline bool pl011_dma_rx_running(struct uart_amba_port *uap)
> #define pl011_dma_flush_buffer NULL
> #endif
>
> +static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
> +{
> + struct uart_port *port = &uap->port;
> + u32 cr;
> +
> + /* Wait until hardware tx queue is empty */
> + while (!pl011_tx_empty(port))
> + udelay(uap->rs485_tx_drain_interval);
No way out if the hardware doesn't ever empty? Shouldn't you have an
"upper bound" on this loop somehow?
> +
> + if (port->rs485.delay_rts_after_send)
> + mdelay(port->rs485.delay_rts_after_send);
> +
> + cr = pl011_read(uap, REG_CR);
> +
> + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> + cr &= ~UART011_CR_RTS;
> + else
> + cr |= UART011_CR_RTS;
Blank line here please.
> + /* Disable the transmitter and reenable the transceiver */
> + cr &= ~UART011_CR_TXE;
> + cr |= UART011_CR_RXE;
> + pl011_write(cr, uap, REG_CR);
> +
> + uap->rs485_tx_started = false;
> +
> + return 0;
Why does this function return a value if it can not fail and you do not
check the return value of it?
> +}
> +
> static void pl011_stop_tx(struct uart_port *port)
> {
> struct uart_amba_port *uap =
> @@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
> uap->im &= ~UART011_TXIM;
> pl011_write(uap->im, uap, REG_IMSC);
> pl011_dma_tx_stop(uap);
> +
> + if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
> + pl011_rs485_tx_stop(uap);
So, no check :(
> }
>
> static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
> @@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
> return true;
> }
>
> +static void pl011_rs485_tx_start(struct uart_amba_port *uap)
> +{
> + struct uart_port *port = &uap->port;
> + u32 cr;
> +
> + /* Enable transmitter */
> + cr = pl011_read(uap, REG_CR);
> + cr |= UART011_CR_TXE;
Blank line please.
> + /* Disable receiver if half-duplex */
> + if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> + cr &= ~UART011_CR_RXE;
> +
> + if (port->rs485.flags & SER_RS485_RTS_ON_SEND)
> + cr &= ~UART011_CR_RTS;
> + else
> + cr |= UART011_CR_RTS;
> +
> + pl011_write(cr, uap, REG_CR);
> +
> + if (port->rs485.delay_rts_before_send)
> + mdelay(port->rs485.delay_rts_before_send);
> +
> + uap->rs485_tx_started = true;
> +}
> +
> /* Returns true if tx interrupts have to be (kept) enabled */
> static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
> {
> @@ -1397,6 +1457,10 @@ static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq)
> return false;
> }
>
> + if ((uap->port.rs485.flags & SER_RS485_ENABLED) &&
> + !uap->rs485_tx_started)
> + pl011_rs485_tx_start(uap);
> +
> /* If we are using DMA mode, try to send some characters. */
> if (pl011_dma_tx_irq(uap))
> return true;
> @@ -1542,6 +1606,9 @@ static void pl011_set_mctrl(struct uart_port *port, unsigned int mctrl)
> container_of(port, struct uart_amba_port, port);
> unsigned int cr;
>
> + if (port->rs485.flags & SER_RS485_ENABLED)
> + mctrl &= ~TIOCM_RTS;
> +
> cr = pl011_read(uap, REG_CR);
>
> #define TIOCMBIT(tiocmbit, uartbit) \
> @@ -1763,7 +1830,17 @@ static int pl011_startup(struct uart_port *port)
>
> /* restore RTS and DTR */
> cr = uap->old_cr & (UART011_CR_RTS | UART011_CR_DTR);
> - cr |= UART01x_CR_UARTEN | UART011_CR_RXE | UART011_CR_TXE;
> + cr |= UART01x_CR_UARTEN | UART011_CR_RXE;
> +
> + if (port->rs485.flags & SER_RS485_ENABLED) {
> + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
> + cr &= ~UART011_CR_RTS;
> + else
> + cr |= UART011_CR_RTS;
> + } else {
> + cr |= UART011_CR_TXE;
> + }
> +
> pl011_write(cr, uap, REG_CR);
>
> spin_unlock_irq(&uap->port.lock);
> @@ -1864,6 +1941,9 @@ static void pl011_shutdown(struct uart_port *port)
>
> pl011_dma_shutdown(uap);
>
> + if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
> + pl011_rs485_tx_stop(uap);
> +
> free_irq(uap->port.irq, uap);
>
> pl011_disable_uart(uap);
> @@ -1941,6 +2021,7 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
> unsigned int lcr_h, old_cr;
> unsigned long flags;
> unsigned int baud, quot, clkdiv;
> + unsigned int bits;
>
> if (uap->vendor->oversampling)
> clkdiv = 8;
> @@ -1991,18 +2072,29 @@ pl011_set_termios(struct uart_port *port, struct ktermios *termios,
> if (uap->fifosize > 1)
> lcr_h |= UART01x_LCRH_FEN;
>
> + bits = tty_get_frame_size(termios->c_cflag);
> +
> spin_lock_irqsave(&port->lock, flags);
>
> /*
> * Update the per-port timeout.
> */
> uart_update_timeout(port, termios->c_cflag, baud);
Blank line
> + /*
> + * Calculate the approximated time it takes to transmit one character
> + * with the given baud rate. We use this as the poll interval when we
> + * wait for the tx queue to empty.
> + */
> + uap->rs485_tx_drain_interval = (bits * 1000 * 1000) / baud;
>
> pl011_setup_status_masks(port, termios);
>
> if (UART_ENABLE_MS(port, termios->c_cflag))
> pl011_enable_ms(port);
>
> + if (port->rs485.flags & SER_RS485_ENABLED)
> + termios->c_cflag &= ~CRTSCTS;
> +
> /* first, disable everything */
> old_cr = pl011_read(uap, REG_CR);
> pl011_write(0, uap, REG_CR);
> @@ -2124,6 +2216,40 @@ static int pl011_verify_port(struct uart_port *port, struct serial_struct *ser)
> return ret;
> }
>
> +static int pl011_rs485_config(struct uart_port *port,
> + struct serial_rs485 *rs485)
> +{
> + struct uart_amba_port *uap =
> + container_of(port, struct uart_amba_port, port);
> +
> + /* pick sane settings if the user hasn't */
> + if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
Why the !! in an if statement?
> + !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
Same here, why?
> + rs485->flags |= SER_RS485_RTS_ON_SEND;
> + rs485->flags &= ~SER_RS485_RTS_AFTER_SEND;
> + }
> + /* clamp the delays to [0, 100ms] */
> + rs485->delay_rts_before_send = min(rs485->delay_rts_before_send, 100U);
> + rs485->delay_rts_after_send = min(rs485->delay_rts_after_send, 100U);
> + memset(rs485->padding, 0, sizeof(rs485->padding));
> +
> + if (port->rs485.flags & SER_RS485_ENABLED)
> + pl011_rs485_tx_stop(uap);
> +
> + /* Set new configuration */
> + port->rs485 = *rs485;
Blank line please.
> + /* Make sure auto RTS is disabled */
> + if (port->rs485.flags & SER_RS485_ENABLED) {
> + u32 cr = pl011_read(uap, REG_CR);
> +
> + cr &= ~UART011_CR_RTSEN;
> + pl011_write(cr, uap, REG_CR);
> + port->status &= ~UPSTAT_AUTORTS;
> + }
> +
> + return 0;
> +}
> +
thanks,
greg k-h
Hi,
On 24.06.21 at 14:55, Greg KH wrote:
>>
>> +static int pl011_rs485_tx_stop(struct uart_amba_port *uap)
>> +{
>> + struct uart_port *port = &uap->port;
>> + u32 cr;
>> +
>> + /* Wait until hardware tx queue is empty */
>> + while (!pl011_tx_empty(port))
>> + udelay(uap->rs485_tx_drain_interval);
>
> No way out if the hardware doesn't ever empty? Shouldn't you have an
> "upper bound" on this loop somehow?
Yes, indeed. I will fix this.
>
>> +
>> + if (port->rs485.delay_rts_after_send)
>> + mdelay(port->rs485.delay_rts_after_send);
>> +
>> + cr = pl011_read(uap, REG_CR);
>> +
>> + if (port->rs485.flags & SER_RS485_RTS_AFTER_SEND)
>> + cr &= ~UART011_CR_RTS;
>> + else
>> + cr |= UART011_CR_RTS;
>
> Blank line here please.
Ok.
>
>> + /* Disable the transmitter and reenable the transceiver */
>> + cr &= ~UART011_CR_TXE;
>> + cr |= UART011_CR_RXE;
>> + pl011_write(cr, uap, REG_CR);
>> +
>> + uap->rs485_tx_started = false;
>> +
>> + return 0;
>
> Why does this function return a value if it can not fail and you do not
> check the return value of it?
>> +}
>> +
>> static void pl011_stop_tx(struct uart_port *port)
>> {
>> struct uart_amba_port *uap =
>> @@ -1290,6 +1322,9 @@ static void pl011_stop_tx(struct uart_port *port)
>> uap->im &= ~UART011_TXIM;
>> pl011_write(uap->im, uap, REG_IMSC);
>> pl011_dma_tx_stop(uap);
>> +
>> + if ((port->rs485.flags & SER_RS485_ENABLED) && uap->rs485_tx_started)
>> + pl011_rs485_tx_stop(uap);
>
> So, no check :(
>
Ah, right. The return value is a leftover from an earlier version of the function. I will
correct this in the next patch version.
>
>> }
>>
>> static bool pl011_tx_chars(struct uart_amba_port *uap, bool from_irq);
>> @@ -1380,6 +1415,31 @@ static bool pl011_tx_char(struct uart_amba_port *uap, unsigned char c,
>> return true;
>> }
>>
>> +static void pl011_rs485_tx_start(struct uart_amba_port *uap)
>> +{
>> + struct uart_port *port = &uap->port;
>> + u32 cr;
>> +
>> + /* Enable transmitter */
>> + cr = pl011_read(uap, REG_CR);
>> + cr |= UART011_CR_TXE;
>
> Blank line please.
>
Ok.
>> +
>> spin_lock_irqsave(&port->lock, flags);
>>
>> /*
>> * Update the per-port timeout.
>> */
>> uart_update_timeout(port, termios->c_cflag, baud);
>
> Blank line
>
Ok.
>>
>> +static int pl011_rs485_config(struct uart_port *port,
>> + struct serial_rs485 *rs485)
>> +{
>> + struct uart_amba_port *uap =
>> + container_of(port, struct uart_amba_port, port);
>> +
>> + /* pick sane settings if the user hasn't */
>> + if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>
> Why the !! in an if statement?
>
>> + !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>
> Same here, why?
>
This was copied from serial8250_em485_config(). But I think we can simply use
if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
rs485->flags &= ~SER_RS485_RTS_ON_SEND;
else
rs485->flags |= SER_RS485_RTS_ON_SEND;
instead. I will adjust the code accordingly.
>> +
>> + if (port->rs485.flags & SER_RS485_ENABLED)
>> + pl011_rs485_tx_stop(uap);
>> +
>> + /* Set new configuration */
>> + port->rs485 = *rs485;
>
> Blank line please.
>
Ok.
>
> thanks,
>
> greg k-h
>
Thank you for the review!
Regards,
Lino
On 25. 06. 21, 2:15, Lino Sanfilippo wrote:
>>> +static int pl011_rs485_config(struct uart_port *port,
>>> + struct serial_rs485 *rs485)
>>> +{
>>> + struct uart_amba_port *uap =
>>> + container_of(port, struct uart_amba_port, port);
>>> +
>>> + /* pick sane settings if the user hasn't */
>>> + if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
>>
>> Why the !! in an if statement?
>>
>>> + !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
>>
>> Same here, why?
>>
>
> This was copied from serial8250_em485_config(). But I think we can simply use
>
> if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
> rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> else
> rs485->flags |= SER_RS485_RTS_ON_SEND;
>
> instead. I will adjust the code accordingly.
This is different. You want to set ON_SEND when none is set. And unset
AFTER_SEND when both are set. In your code, when both are set, you leave
AFTER_SEND.
regards,
--
js
suse labs
Hi,
> On 25. 06. 21, 2:15, Lino Sanfilippo wrote:
> >>> +static int pl011_rs485_config(struct uart_port *port,
> >>> + struct serial_rs485 *rs485)
> >>> +{
> >>> + struct uart_amba_port *uap =
> >>> + container_of(port, struct uart_amba_port, port);
> >>> +
> >>> + /* pick sane settings if the user hasn't */
> >>> + if (!!(rs485->flags & SER_RS485_RTS_ON_SEND) ==
> >>
> >> Why the !! in an if statement?
> >>
> >>> + !!(rs485->flags & SER_RS485_RTS_AFTER_SEND)) {
> >>
> >> Same here, why?
> >>
> >
> > This was copied from serial8250_em485_config(). But I think we can simply use
> >
> > if (rs485->flags & SER_RS485_RTS_AFTER_SEND)
> > rs485->flags &= ~SER_RS485_RTS_ON_SEND;
> > else
> > rs485->flags |= SER_RS485_RTS_ON_SEND;
> >
> > instead. I will adjust the code accordingly.
>
> This is different. You want to set ON_SEND when none is set. And unset
> AFTER_SEND when both are set. In your code, when both are set, you leave
> AFTER_SEND.
>
> regards,
> --
> js
> suse labs
>
Thats right, the logic has slightly changed. I thought this does not matter as long as
we make sure that exactly one of both ON_SEND or AFTER_SEND is set. We can stick with the logic
in serial8250_em485_config() (i.e. always set ON_SEND and delete AFTER_SEND in case
of an invalid setting), but I think this will require more than the four lines that we
have now (especially if we want to avoid the !!).
Thanks,
Lino
Hi,
On 25. 06. 21, 13:21, Lino Sanfilippo wrote:
> Thats right, the logic has slightly changed. I thought this does not matter as long as
> we make sure that exactly one of both ON_SEND or AFTER_SEND is set.
I actually don't know. But I would remain consistent with other drivers.
> We can stick with the logic
> in serial8250_em485_config() (i.e. always set ON_SEND and delete AFTER_SEND in case
> of an invalid setting), but I think this will require more than the four lines that we
> have now (especially if we want to avoid the !!).
What if you used only a single !?
thanks,
--
js
suse labs
Hi,
On 26.06.21 at 05:41, Jiri Slaby wrote:
> On 25. 06. 21, 13:21, Lino Sanfilippo wrote:
>> Thats right, the logic has slightly changed. I thought this does not matter as long as
>> we make sure that exactly one of both ON_SEND or AFTER_SEND is set.
>
> I actually don't know. But I would remain consistent with other drivers.
>
>> We can stick with the logic
>> in serial8250_em485_config() (i.e. always set ON_SEND and delete AFTER_SEND in case
>> of an invalid setting), but I think this will require more than the four lines that we
>> have now (especially if we want to avoid the !!).
>
> What if you used only a single !?
>
You are right, this is the easiest way to keep the current logic. I will do that.
Thanks,
Lino