[PATCH v2] tty: serial: samsung_tty: loopback mode support

[PATCH v2] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

Internal loopback mode can be supported by setting
UCON register's Loopback Mode bit. The mode & bit can be supported since
s3c2410 and later SoCs. The prefix of LOOPBACK / BIT(5) naming should be
also changed to S3C2410_ to avoid confusion.

We can test it by linux-serial-test program
with -k option. The tool will set TIOCM_LOOP mode when the option is
specified.
-k, --loopback     Use internal hardware loop back

Signed-off-by: Chanho Park <chanho61.park@samsung.com>
---
Changes from v1:
- Drop TIOCM_LOOP return from s3c24xx_serial_get_mctrl as pointed out by
  Ilpo. Documentation/driver-api/serial/driver.rst indicates the bit is
  only for set_mctrl.
- Change the loopback bit definition from S3C2443_UCON_LOOPBACK to
  S3C2410_UCON_LOOPBACK because it has been supported since s3c2410.
- Remove the unnecessary footnote and put a blank line before Signed-off-by

 drivers/tty/serial/samsung_tty.c | 8 ++++++++
 include/linux/serial_s3c.h       | 2 +-
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
index d5ca904def34..03ef4ff506fd 100644
--- a/drivers/tty/serial/samsung_tty.c
+++ b/drivers/tty/serial/samsung_tty.c
@@ -1012,6 +1012,7 @@ static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
 static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 {
 	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
+	unsigned int ucon = rd_reg(port, S3C2410_UCON);
 
 	if (mctrl & TIOCM_RTS)
 		umcon |= S3C2410_UMCOM_RTS_LOW;
@@ -1019,6 +1020,13 @@ static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
 		umcon &= ~S3C2410_UMCOM_RTS_LOW;
 
 	wr_regl(port, S3C2410_UMCON, umcon);
+
+	if (mctrl & TIOCM_LOOP)
+		ucon |= S3C2410_UCON_LOOPBACK;
+	else
+		ucon &= ~S3C2410_UCON_LOOPBACK;
+
+	wr_regl(port, S3C2410_UCON, ucon);
 }
 
 static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
index dec15f5b3dec..1672cf0810ef 100644
--- a/include/linux/serial_s3c.h
+++ b/include/linux/serial_s3c.h
@@ -83,7 +83,7 @@
 #define S3C2410_UCON_RXIRQMODE	  (1<<0)
 #define S3C2410_UCON_RXFIFO_TOI	  (1<<7)
 #define S3C2443_UCON_RXERR_IRQEN  (1<<6)
-#define S3C2443_UCON_LOOPBACK	  (1<<5)
+#define S3C2410_UCON_LOOPBACK	  (1<<5)
 
 #define S3C2410_UCON_DEFAULT	  (S3C2410_UCON_TXILEVEL  | \
 				   S3C2410_UCON_RXILEVEL  | \
-- 
2.36.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] tty: serial: samsung_tty: loopback mode support

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2934 bytes --]

On Tue, 28 Jun 2022, Chanho Park wrote:

> Internal loopback mode can be supported by setting
> UCON register's Loopback Mode bit. The mode & bit can be supported since
> s3c2410 and later SoCs. The prefix of LOOPBACK / BIT(5) naming should be
> also changed to S3C2410_ to avoid confusion.
> 
> We can test it by linux-serial-test program
> with -k option. The tool will set TIOCM_LOOP mode when the option is
> specified.
> -k, --loopback     Use internal hardware loop back
> 
> Signed-off-by: Chanho Park <chanho61.park@samsung.com>

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

You should have included Krzysztof's Reviewed-by when you sent this next 
version so that it won't get lost.

I suspect Greg will again complain that you didn't address his comment 
about the out of the place "-k, --loopback ..." line.


-- 
 i.


> ---
> Changes from v1:
> - Drop TIOCM_LOOP return from s3c24xx_serial_get_mctrl as pointed out by
>   Ilpo. Documentation/driver-api/serial/driver.rst indicates the bit is
>   only for set_mctrl.
> - Change the loopback bit definition from S3C2443_UCON_LOOPBACK to
>   S3C2410_UCON_LOOPBACK because it has been supported since s3c2410.
> - Remove the unnecessary footnote and put a blank line before Signed-off-by
> 
>  drivers/tty/serial/samsung_tty.c | 8 ++++++++
>  include/linux/serial_s3c.h       | 2 +-
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c
> index d5ca904def34..03ef4ff506fd 100644
> --- a/drivers/tty/serial/samsung_tty.c
> +++ b/drivers/tty/serial/samsung_tty.c
> @@ -1012,6 +1012,7 @@ static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port)
>  static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  {
>  	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
> +	unsigned int ucon = rd_reg(port, S3C2410_UCON);
>  
>  	if (mctrl & TIOCM_RTS)
>  		umcon |= S3C2410_UMCOM_RTS_LOW;
> @@ -1019,6 +1020,13 @@ static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned int mctrl)
>  		umcon &= ~S3C2410_UMCOM_RTS_LOW;
>  
>  	wr_regl(port, S3C2410_UMCON, umcon);
> +
> +	if (mctrl & TIOCM_LOOP)
> +		ucon |= S3C2410_UCON_LOOPBACK;
> +	else
> +		ucon &= ~S3C2410_UCON_LOOPBACK;
> +
> +	wr_regl(port, S3C2410_UCON, ucon);
>  }
>  
>  static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
> diff --git a/include/linux/serial_s3c.h b/include/linux/serial_s3c.h
> index dec15f5b3dec..1672cf0810ef 100644
> --- a/include/linux/serial_s3c.h
> +++ b/include/linux/serial_s3c.h
> @@ -83,7 +83,7 @@
>  #define S3C2410_UCON_RXIRQMODE	  (1<<0)
>  #define S3C2410_UCON_RXFIFO_TOI	  (1<<7)
>  #define S3C2443_UCON_RXERR_IRQEN  (1<<6)
> -#define S3C2443_UCON_LOOPBACK	  (1<<5)
> +#define S3C2410_UCON_LOOPBACK	  (1<<5)
>  
>  #define S3C2410_UCON_DEFAULT	  (S3C2410_UCON_TXILEVEL  | \
>  				   S3C2410_UCON_RXILEVEL  | \
> 

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

> > Internal loopback mode can be supported by setting UCON register's
> > Loopback Mode bit. The mode & bit can be supported since
> > s3c2410 and later SoCs. The prefix of LOOPBACK / BIT(5) naming should
> > be also changed to S3C2410_ to avoid confusion.
> >
> > We can test it by linux-serial-test program with -k option. The tool
> > will set TIOCM_LOOP mode when the option is specified.
> > -k, --loopback     Use internal hardware loop back
> >
> > Signed-off-by: Chanho Park <chanho61.park@samsung.com>
> 
> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

Thanks for your review.

> 
> You should have included Krzysztof's Reviewed-by when you sent this next
> version so that it won't get lost.

I thought the codes have been changed since v1 so I'm not convinced I can
put his R-B tag.

> 
> I suspect Greg will again complain that you didn't address his comment
> about the out of the place "-k, --loopback ..." line.

The commit messages seems to be not necessary. I'll remove them next patch.

Best Regards,
Chanho Park



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v2] tty: serial: samsung_tty: loopback mode support

[Greg Kroah-Hartman]

On Tue, Jun 28, 2022 at 09:40:50PM +0900, Chanho Park wrote:
> Internal loopback mode can be supported by setting
> UCON register's Loopback Mode bit. The mode & bit can be supported since
> s3c2410 and later SoCs. The prefix of LOOPBACK / BIT(5) naming should be
> also changed to S3C2410_ to avoid confusion.
> 
> We can test it by linux-serial-test program
> with -k option. The tool will set TIOCM_LOOP mode when the option is
> specified.
> -k, --loopback     Use internal hardware loop back

Again, what does this line mean in a changelog text?

And properly wrap your lines at 72 columns.

thanks,

greg k-h

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

RE: [PATCH v2] tty: serial: samsung_tty: loopback mode support

[Chanho Park]

> > We can test it by linux-serial-test program with -k option. The tool
> > will set TIOCM_LOOP mode when the option is specified.
> > -k, --loopback     Use internal hardware loop back
> 
> Again, what does this line mean in a changelog text?

I put the option to refer how the tool can use. I can see it is not related
with this commit.
I'll remove them next patch.

> 
> And properly wrap your lines at 72 columns.

I'll keep that in mind.
Thanks for your review :)

Best Regards,
Chanho Park


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel