Re: [PATCH 5/6] tty: serial: 8250-core: add rs485 support

[One Thousand Gnomes <gnomes@lxorguk.ukuu.org.uk>]

>  static inline void __stop_tx(struct uart_8250_port *p)
>  {
> +	if (p->rs485.flags & SER_RS485_ENABLED) {
> +		int ret;
> +
> +		ret = (p->rs485.flags & SER_RS485_RTS_AFTER_SEND) ? 1 : 0;
> +		if (gpio_get_value(p->rts_gpio) != ret) {
> +			if (p->rs485.delay_rts_after_send > 0)
> +				mdelay(p->rs485.delay_rts_after_send);
> +			gpio_set_value(p->rts_gpio, ret);
> +		}

RTS is not normally a GPIO. We should be controlling the UART RTS here,
and if the UART has a magic special case RTS wired to a GPIO then really
the hardware specific part should handle that gunge. I don't care whether
the drive does it via serial_out magic or a more explicit hook but it
doesn't belong here in core code.

Likewise the mdelay probably should be in the device specific bits or
controlled by a flag as not all hardware is so braindead.

> @@ -1330,6 +1356,20 @@ static void serial8250_start_tx(struct uart_port *port)
>  	if (up->dma && !serial8250_tx_dma(up)) {

Ditto

> +int serial8250_probe_rs485(struct uart_8250_port *up,
> +		struct device *dev)
> +{
> +	struct serial_rs485 *rs485conf = &up->rs485;
> +	struct device_node *np = dev->of_node;
> +	u32 rs485_delay[2];
> +	enum of_gpio_flags flags;
> +	int ret;
> +
> +	rs485conf->flags = 0;
> +	if (!np)
> +		return 0;
> +
> +	/* check for tx enable gpio */
> +	up->rts_gpio = of_get_named_gpio_flags(np, "rts-gpio", 0, &flags);

No of_ dependencies in core 8250.c either please. That looks a perfectly
good implementation of serial8250_of_probe_rs485 however, just belongs in
the right place.

> +static int serial8250_ioctl(struct uart_port *port, unsigned int cmd,
> +		unsigned long arg)
> +{
> +	struct serial_rs485 rs485conf;
> +	struct uart_8250_port *up;
> +
> +	up = container_of(port, struct uart_8250_port, port);
> +	switch (cmd) {
> +	case TIOCSRS485:
> +		if (!gpio_is_valid(up->rts_gpio))
> +			return -ENODEV;

GPIO assumption again needs to go


> diff --git a/include/linux/serial_8250.h b/include/linux/serial_8250.h
> index 0ec21ec..056a73f 100644
> --- a/include/linux/serial_8250.h
> +++ b/include/linux/serial_8250.h
> @@ -78,6 +78,7 @@ struct uart_8250_port {
>  	unsigned char		acr;
>  	unsigned char		ier;
>  	unsigned char		lcr;
> +	unsigned char		fcr;
>  	unsigned char		mcr;
>  	unsigned char		mcr_mask;	/* mask of user bits */
>  	unsigned char		mcr_force;	/* mask of forced bits */
> @@ -94,6 +95,9 @@ struct uart_8250_port {
>  	unsigned char		msr_saved_flags;
>  
>  	struct uart_8250_dma	*dma;
> +	struct serial_rs485	rs485;
> +	int			rts_gpio;
> +	bool			rts_gpio_valid;

Keeping the gpio here doesn't look unreasonable if one is in use.

Alan