Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO

[Farouk Bouabid]

Hi,

On 17.05.20 23:56, Heiko Stuebner wrote:
> From: Heiko Stuebner<heiko.stuebner@xxxxxxxxxxxxxxxxxxxxx>
>
> The RE signal is used to control the duplex mode of transmissions,
> aka receiving data while sending in full duplex mode, while stopping
> receiving data in half-duplex mode.
>
> On a number of boards the !RE signal is tied to ground so reception
> is always enabled except if the UART allows disabling the receiver.
> This can be taken advantage of to implement half-duplex mode - like
> done on 8250_bcm2835aux.
>
> Another solution is to tie !RE to RTS always forcing half-duplex mode.
>
> And finally there is the option to control the RE signal separately,
> like done here by introducing a new rs485-specific gpio that can be
> set depending on the RX_DURING_TX setting in the common em485 callbacks.
>
> Signed-off-by: Heiko Stuebner<heiko.stuebner@xxxxxxxxxxxxxxxxxxxxx>
> ---
>   drivers/tty/serial/8250/8250_port.c |  7 ++++++-
>   drivers/tty/serial/serial_core.c    | 10 ++++++++++
>   include/linux/serial_core.h         |  1 +
>   3 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 6975bd3ecb7d..9e8fec85d1a3 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1444,6 +1444,7 @@ static void serial8250_stop_rx(struct uart_port *port)
>   void serial8250_em485_stop_tx(struct uart_8250_port *p)
>   {
>   	unsigned char mcr = serial8250_in_MCR(p);
> +	struct uart_port *port = &p->port;
>   
>   	if (p->port.rs485.flags & SER_RS485_RTS_AFTER_SEND)
>   		mcr |= UART_MCR_RTS;
> @@ -1457,6 +1458,7 @@ void serial8250_em485_stop_tx(struct uart_8250_port *p)
>   	 * Enable previously disabled RX interrupts.
>   	 */
>   	if (!(p->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +		gpiod_set_value(port->rs485_re_gpio, 1);
>   		serial8250_clear_and_reinit_fifos(p);
>   
>   		p->ier |= UART_IER_RLSI | UART_IER_RDI;
> @@ -1597,9 +1599,12 @@ static inline void __start_tx(struct uart_port *port)
>   void serial8250_em485_start_tx(struct uart_8250_port *up)
>   {
>   	unsigned char mcr = serial8250_in_MCR(up);
> +	struct uart_port *port = &up->port;
>   
> -	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX))
> +	if (!(up->port.rs485.flags & SER_RS485_RX_DURING_TX)) {
> +		gpiod_set_value(port->rs485_re_gpio, 0);
>   		serial8250_stop_rx(&up->port);
> +	}
>   
>   	if (up->port.rs485.flags & SER_RS485_RTS_ON_SEND)
>   		mcr |= UART_MCR_RTS;
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 15ad8737b72b..c0d3ab32b49a 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3149,6 +3149,16 @@ int uart_get_rs485_mode(struct uart_port *port)
>   		return ret;
>   	}
>   
> +	port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable",
> +						      GPIOD_OUT_HIGH);

The half duplex emulation can be disabled on-boot or later-on using 
ioctls in systems that can switch between half and full duplex modes. In 
that case this gpio should probably be released as it can probably 
prevent enabling the transceiver-sender mode. I suggest we set the gpio 
to output active in the em485_init callback and stop driving it in the 
em485_destroy callback.

Regards

> +	if (IS_ERR(port->rs485_re_gpio)) {
> +		ret = PTR_ERR(port->rs485_re_gpio);
> +		port->rs485_re_gpio = NULL;
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "Cannot get rs485-rx-enable-gpios\n");
> +		return ret;
> +	}
> +
>   	return 0;
>   }
>   EXPORT_SYMBOL_GPL(uart_get_rs485_mode);
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> index 108f95411471..dfe18ddb1674 100644
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -253,6 +253,7 @@ struct uart_port {
>   	const struct attribute_group **tty_groups;	/* all attributes (serial core use only) */
>   	struct serial_rs485     rs485;
>   	struct gpio_desc	*rs485_term_gpio;	/* enable RS485 bus termination */
> +	struct gpio_desc	*rs485_re_gpio;		/* gpio RS485 receive enable */
>   	struct serial_iso7816   iso7816;
>   	void			*private_data;		/* generic platform data pointer */
>   };