diff mbox

[3/4] sc16is7xx: expose RTS inversion in RS-485 mode

Message ID 1426548529-21738-4-git-send-email-moorray3@wp.pl
State Not Applicable
Headers show

Commit Message

Jakub Kiciński March 16, 2015, 11:28 p.m. UTC
From: Jakub Kicinski <kubakici@wp.pl>

Hardware is capable of inverting RTS signal when working
in RS-485 mode.  Expose this functionality to user space.
Relay on a matching combination of standard flags
(SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND) to
detect when user space is requesting inverted RTS mode.

Signed-off-by: Jakub Kicinski <kubakici@wp.pl>
---
 drivers/tty/serial/sc16is7xx.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

Comments

Jon Ringle March 17, 2015, 2:45 p.m. UTC | #1
This makes sense. I did have to fix up my user space app to set 
SER_RS485_RTS_ON_SEND after applying this patch.

On Mon, 16 Mar 2015, Jakub Kicinski wrote:

> From: Jakub Kicinski <kubakici@wp.pl>
> 
> Hardware is capable of inverting RTS signal when working
> in RS-485 mode.  Expose this functionality to user space.
> Relay on a matching combination of standard flags
> (SER_RS485_RTS_ON_SEND and SER_RS485_RTS_AFTER_SEND) to
> detect when user space is requesting inverted RTS mode.
> 
> Signed-off-by: Jakub Kicinski <kubakici@wp.pl>

Signed-off-by: Jon Ringle <jringle@gridpoint.com>

> ---
>  drivers/tty/serial/sc16is7xx.c | 32 +++++++++++++++++++++++---------
>  1 file changed, 23 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index e6b396e584f3..24902d589b07 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -829,16 +829,30 @@ static void sc16is7xx_set_termios(struct uart_port *port,
>  }
>  
>  static int sc16is7xx_config_rs485(struct uart_port *port,
> -				   struct serial_rs485 *rs485)
> +				  struct serial_rs485 *rs485)
>  {
> -	if (port->rs485.flags & SER_RS485_ENABLED)
> -		sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
> -				      SC16IS7XX_EFCR_AUTO_RS485_BIT,
> -				      SC16IS7XX_EFCR_AUTO_RS485_BIT);
> -	else
> -		sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
> -				      SC16IS7XX_EFCR_AUTO_RS485_BIT,
> -				      0);
> +	const u32 mask = SC16IS7XX_EFCR_AUTO_RS485_BIT |
> +			 SC16IS7XX_EFCR_RTS_INVERT_BIT;
> +	u32 efcr = 0;
> +
> +	if (rs485->flags & SER_RS485_ENABLED) {
> +		bool rts_during_rx, rts_during_tx;
> +
> +		rts_during_rx = rs485->flags & SER_RS485_RTS_AFTER_SEND;
> +		rts_during_tx = rs485->flags & SER_RS485_RTS_ON_SEND;
> +
> +		efcr |= SC16IS7XX_EFCR_AUTO_RS485_BIT;
> +
> +		if (!rts_during_rx && rts_during_tx)
> +			/* default */;
> +		else if (rts_during_rx && !rts_during_tx)
> +			efcr |= SC16IS7XX_EFCR_RTS_INVERT_BIT;
> +		else
> +			return -EINVAL;
> +	}
> +
> +	sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG, mask, efcr);
> +
>  	port->rs485 = *rs485;
>  
>  	return 0;
> -- 
> 2.1.0
> 
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jakub Kiciński March 17, 2015, 2:57 p.m. UTC | #2
On Tue, 17 Mar 2015 10:45:26 -0400 (EDT), Jon Ringle wrote:
> This makes sense. I did have to fix up my user space app to set 
> SER_RS485_RTS_ON_SEND after applying this patch.

Yes, perhaps I should have mentioned that this makes the ioctl return
-EINVAL when neither of SER_RS485_RTS_*_SEND flags is set.  This
definitely has potential to break people's apps.  Let's see if anyone
else has an opinion on this.

I am also very tempted to remove rs485.delay_rts_*_send mdelays from
this driver and return -EINVAL in a similar fashion when there are set.
Normally when the driver is responsible for wiggling the GPIO/RTS line
mdelays can indeed impact when the lines change state but in sc16is7xx
case transitions are controlled by HW and there is nothing the driver
can do about the the relation between TX/RX mode switch and state of 
RTS lines...  Do you agree?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
gregkh@linuxfoundation.org March 26, 2015, 9:37 p.m. UTC | #3
On Tue, Mar 17, 2015 at 03:57:00PM +0100, Jakub Kiciński wrote:
> On Tue, 17 Mar 2015 10:45:26 -0400 (EDT), Jon Ringle wrote:
> > This makes sense. I did have to fix up my user space app to set 
> > SER_RS485_RTS_ON_SEND after applying this patch.
> 
> Yes, perhaps I should have mentioned that this makes the ioctl return
> -EINVAL when neither of SER_RS485_RTS_*_SEND flags is set.  This
> definitely has potential to break people's apps.  Let's see if anyone
> else has an opinion on this.

Yes, don't break people's apps.

I can't take this, sorry.

greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jakub Kiciński March 26, 2015, 9:48 p.m. UTC | #4
On Thu, 26 Mar 2015 22:37:01 +0100, Greg Kroah-Hartman wrote:
> On Tue, Mar 17, 2015 at 03:57:00PM +0100, Jakub Kiciński wrote:
> > On Tue, 17 Mar 2015 10:45:26 -0400 (EDT), Jon Ringle wrote:
> > > This makes sense. I did have to fix up my user space app to set 
> > > SER_RS485_RTS_ON_SEND after applying this patch.
> > 
> > Yes, perhaps I should have mentioned that this makes the ioctl return
> > -EINVAL when neither of SER_RS485_RTS_*_SEND flags is set.  This
> > definitely has potential to break people's apps.  Let's see if anyone
> > else has an opinion on this.
> 
> Yes, don't break people's apps.

Do you mean it would be OK if I ignore the incorrect combinations
instead of returning -EINVAL or is any user-visible change in the
behaviour of the driver unacceptable?
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index e6b396e584f3..24902d589b07 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -829,16 +829,30 @@  static void sc16is7xx_set_termios(struct uart_port *port,
 }
 
 static int sc16is7xx_config_rs485(struct uart_port *port,
-				   struct serial_rs485 *rs485)
+				  struct serial_rs485 *rs485)
 {
-	if (port->rs485.flags & SER_RS485_ENABLED)
-		sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
-				      SC16IS7XX_EFCR_AUTO_RS485_BIT,
-				      SC16IS7XX_EFCR_AUTO_RS485_BIT);
-	else
-		sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
-				      SC16IS7XX_EFCR_AUTO_RS485_BIT,
-				      0);
+	const u32 mask = SC16IS7XX_EFCR_AUTO_RS485_BIT |
+			 SC16IS7XX_EFCR_RTS_INVERT_BIT;
+	u32 efcr = 0;
+
+	if (rs485->flags & SER_RS485_ENABLED) {
+		bool rts_during_rx, rts_during_tx;
+
+		rts_during_rx = rs485->flags & SER_RS485_RTS_AFTER_SEND;
+		rts_during_tx = rs485->flags & SER_RS485_RTS_ON_SEND;
+
+		efcr |= SC16IS7XX_EFCR_AUTO_RS485_BIT;
+
+		if (!rts_during_rx && rts_during_tx)
+			/* default */;
+		else if (rts_during_rx && !rts_during_tx)
+			efcr |= SC16IS7XX_EFCR_RTS_INVERT_BIT;
+		else
+			return -EINVAL;
+	}
+
+	sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG, mask, efcr);
+
 	port->rs485 = *rs485;
 
 	return 0;