[PATCH 0/4] serial: sc16is7xx: bunch of small fixes

[PATCH 0/4] serial: sc16is7xx: bunch of small fixes

[Jakub Kicinski]

From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

This series fixes three small problems with the sc16is7xx driver
and adds an ability to configure from user space the RTS inversion
for RS-485 mode.

None of the fixed issues is pressing so this seems like a -next
material (based on tty.git/tty-next).

Jakub Kicinski (4):
  sc16is7xx: remove ports on probe error path
  sc16is7xx: don't wipe out port configuration on shutdown
  sc16is7xx: expose RTS inversion in RS-485 mode
  sc16is7xx: enable the clock

 drivers/tty/serial/sc16is7xx.c | 44 ++++++++++++++++++++++++++++++------------
 1 file changed, 32 insertions(+), 12 deletions(-)

-- 
2.1.0

[PATCH 1/4] sc16is7xx: remove ports on probe error path

[Jakub Kicinski]

From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

If ports are not explicitly removed on the error path
the device will not get properly unregistered leaving
/dev/ttySC* nodes in the filesystem.

Signed-off-by: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>
---
 drivers/tty/serial/sc16is7xx.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index df9a384dfbda..11bb44039a57 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1120,6 +1120,9 @@ static int sc16is7xx_probe(struct device *dev,
 	if (!ret)
 		return 0;
 
+	for (i = 0; i < s->uart.nr; i++)
+		uart_remove_one_port(&s->uart, &s->p[i].port);
+
 	mutex_destroy(&s->mutex);
 
 #ifdef CONFIG_GPIOLIB
-- 
2.1.0

[PATCH 2/4] sc16is7xx: don't wipe out port configuration on shutdown

[Jakub Kicinski]

From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

EFCR register contains RS-485 configuration which should not
be changed by shutdown.  Instead of doing a write only update
the appropriate bits.

Signed-off-by: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>
---
 drivers/tty/serial/sc16is7xx.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 11bb44039a57..e6b396e584f3 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -903,9 +903,11 @@ static void sc16is7xx_shutdown(struct uart_port *port)
 	/* Disable all interrupts */
 	sc16is7xx_port_write(port, SC16IS7XX_IER_REG, 0);
 	/* Disable TX/RX */
-	sc16is7xx_port_write(port, SC16IS7XX_EFCR_REG,
-			     SC16IS7XX_EFCR_RXDISABLE_BIT |
-			     SC16IS7XX_EFCR_TXDISABLE_BIT);
+	sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
+			      SC16IS7XX_EFCR_RXDISABLE_BIT |
+			      SC16IS7XX_EFCR_TXDISABLE_BIT,
+			      SC16IS7XX_EFCR_RXDISABLE_BIT |
+			      SC16IS7XX_EFCR_TXDISABLE_BIT);
 
 	sc16is7xx_power(port, 0);
 }
-- 
2.1.0

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

[Jakub Kicinski]

From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

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-5tc4TXWwyLM@public.gmane.org>
---
 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

[PATCH 4/4] sc16is7xx: enable the clock

[Jakub Kicinski]

From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

Although clk_disable_unprepare() is called both on .remove()
and in .probe() error path, the clock is never actually enabled.

Signed-off-by: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>
---
 drivers/tty/serial/sc16is7xx.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
index 24902d589b07..20573d72715d 100644
--- a/drivers/tty/serial/sc16is7xx.c
+++ b/drivers/tty/serial/sc16is7xx.c
@@ -1064,6 +1064,7 @@ static int sc16is7xx_probe(struct device *dev,
 		else
 			return PTR_ERR(s->clk);
 	} else {
+		clk_prepare_enable(s->clk);
 		freq = clk_get_rate(s->clk);
 	}
 
-- 
2.1.0

Re: [PATCH 1/4] sc16is7xx: remove ports on probe error path

[Jon Ringle]

On Mon, 16 Mar 2015, Jakub Kicinski wrote:

> From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>
> 
> If ports are not explicitly removed on the error path
> the device will not get properly unregistered leaving
> /dev/ttySC* nodes in the filesystem.
> 
> Signed-off-by: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

Signed-off-by: Jon Ringle <jringle-gkqdKIIuv0Hby3iVrkZq2A@public.gmane.org>

> ---
>  drivers/tty/serial/sc16is7xx.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index df9a384dfbda..11bb44039a57 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1120,6 +1120,9 @@ static int sc16is7xx_probe(struct device *dev,
>  	if (!ret)
>  		return 0;
>  
> +	for (i = 0; i < s->uart.nr; i++)
> +		uart_remove_one_port(&s->uart, &s->p[i].port);
> +
>  	mutex_destroy(&s->mutex);
>  
>  #ifdef CONFIG_GPIOLIB
> -- 
> 2.1.0
> 
> 

Re: [PATCH 2/4] sc16is7xx: don't wipe out port configuration on shutdown

[Jon Ringle]

On Mon, 16 Mar 2015, Jakub Kicinski wrote:

> From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>
> 
> EFCR register contains RS-485 configuration which should not
> be changed by shutdown.  Instead of doing a write only update
> the appropriate bits.
> 
> Signed-off-by: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

Signed-off-by: Jon Ringle <jringle-gkqdKIIuv0Hby3iVrkZq2A@public.gmane.org>

> ---
>  drivers/tty/serial/sc16is7xx.c | 8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 11bb44039a57..e6b396e584f3 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -903,9 +903,11 @@ static void sc16is7xx_shutdown(struct uart_port *port)
>  	/* Disable all interrupts */
>  	sc16is7xx_port_write(port, SC16IS7XX_IER_REG, 0);
>  	/* Disable TX/RX */
> -	sc16is7xx_port_write(port, SC16IS7XX_EFCR_REG,
> -			     SC16IS7XX_EFCR_RXDISABLE_BIT |
> -			     SC16IS7XX_EFCR_TXDISABLE_BIT);
> +	sc16is7xx_port_update(port, SC16IS7XX_EFCR_REG,
> +			      SC16IS7XX_EFCR_RXDISABLE_BIT |
> +			      SC16IS7XX_EFCR_TXDISABLE_BIT,
> +			      SC16IS7XX_EFCR_RXDISABLE_BIT |
> +			      SC16IS7XX_EFCR_TXDISABLE_BIT);
>  
>  	sc16is7xx_power(port, 0);
>  }
> -- 
> 2.1.0
> 
> 

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

[Jon Ringle]

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-5tc4TXWwyLM@public.gmane.org>
> 
> 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-5tc4TXWwyLM@public.gmane.org>

Signed-off-by: Jon Ringle <jringle-gkqdKIIuv0Hby3iVrkZq2A@public.gmane.org>

> ---
>  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
> 
> 

Re: [PATCH 4/4] sc16is7xx: enable the clock

[Jon Ringle]

On Mon, 16 Mar 2015, Jakub Kicinski wrote:

> From: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>
> 
> Although clk_disable_unprepare() is called both on .remove()
> and in .probe() error path, the clock is never actually enabled.
> 
> Signed-off-by: Jakub Kicinski <kubakici-5tc4TXWwyLM@public.gmane.org>

Signed-off-by: Jon Ringle <jringle-gkqdKIIuv0Hby3iVrkZq2A@public.gmane.org>

> ---
>  drivers/tty/serial/sc16is7xx.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 24902d589b07..20573d72715d 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -1064,6 +1064,7 @@ static int sc16is7xx_probe(struct device *dev,
>  		else
>  			return PTR_ERR(s->clk);
>  	} else {
> +		clk_prepare_enable(s->clk);
>  		freq = clk_get_rate(s->clk);
>  	}
>  
> -- 
> 2.1.0
> 
> 

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

[Jakub Kiciński]

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?

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

[Greg Kroah-Hartman]

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

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

[Jakub Kiciński]

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?