| Message ID | 20220627032353.8868-1-chanho61.park@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tty: serial: samsung_tty: loopback mode support | expand |
On 27/06/2022 05:23, Chanho Park wrote: > Internal loopback mode can be supported by setting > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since > s3c2410 and later SoCs. We can test it by linux-serial-test program[1] > with -k option. It will set TIOCM_LOOP mode during test. > > -k, --loopback Use internal hardware loop back > > [1]: https://github.com/cbrake/linux-serial-test > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > --- > drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> Best regards, Krzysztof
On Mon, 27 Jun 2022, Chanho Park wrote: > Internal loopback mode can be supported by setting > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since > s3c2410 and later SoCs. In that case, why is the LOOPBACK define named 2443 and not 2410??? The change looks fine otherwise. I note though that many of the current drivers won't return TOICM_LOOP from ->get_mctrl() but I don't think it's exactly wrong to return it either. Perhaps lack of returning it is due to Documentation/driver-api/serial/driver.rst not including TOICM_LOOP in get_mctrl's list of information but only in set_mctrl's one.
On Mon, Jun 27, 2022 at 12:23:53PM +0900, Chanho Park wrote: > Internal loopback mode can be supported by setting > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since > s3c2410 and later SoCs. We can test it by linux-serial-test program[1] > with -k option. It will set TIOCM_LOOP mode during test. > > -k, --loopback Use internal hardware loop back I do not understand this line in the changelog text, what is it for? > > [1]: https://github.com/cbrake/linux-serial-test > Signed-off-by: Chanho Park <chanho61.park@samsung.com> Our tools require a blank line before the signed-off-by line. Did you not get a warning when checking this patch before sending it out? Also, no need for a footnote in a changelog that is so short, just put the link in the body of the text itself. thanks, greg k-h
> Subject: Re: [PATCH] tty: serial: samsung_tty: loopback mode support > > On Mon, Jun 27, 2022 at 12:23:53PM +0900, Chanho Park wrote: > > Internal loopback mode can be supported by setting > > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since > > s3c2410 and later SoCs. We can test it by linux-serial-test program[1] > > with -k option. It will set TIOCM_LOOP mode during test. > > > > -k, --loopback Use internal hardware loop back > > I do not understand this line in the changelog text, what is it for? Ah. I'd like to show the option of linux-serial-test tool how we can test it. > > > > > [1]: > > https://protect2.fireeye.com/v1/url?k=a0d4e46c-c15ff149-a0d56f23-74fe4 > > 85cbff6-c2c20e5c0d4f5c92&q=1&e=7c91006c-af87-4dad-8069-360450d31b36&u= > > https%3A%2F%2Fgithub.com%2Fcbrake%2Flinux-serial-test > > Signed-off-by: Chanho Park <chanho61.park@samsung.com> > > Our tools require a blank line before the signed-off-by line. Did you not > get a warning when checking this patch before sending it out? I'm able to see below warning. I'll fix it. Thanks. WARNING: Do not use whitespace before Signed-off-by: #15: > > Also, no need for a footnote in a changelog that is so short, just put the > link in the body of the text itself. Okay. I'll put the link in the body instead of the footnote. Best Regards, Chanho Park
> Subject: Re: [PATCH] tty: serial: samsung_tty: loopback mode support > > On Mon, 27 Jun 2022, Chanho Park wrote: > > > Internal loopback mode can be supported by setting > > S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since > > s3c2410 and later SoCs. > > In that case, why is the LOOPBACK define named 2443 and not 2410??? It might be copied from mach-s3c24xx. > > The change looks fine otherwise. > > I note though that many of the current drivers won't return TOICM_LOOP > from ->get_mctrl() but I don't think it's exactly wrong to return it > either. Perhaps lack of returning it is due to Documentation/driver- > api/serial/driver.rst not including TOICM_LOOP in get_mctrl's list of > information but only in set_mctrl's one. According to the description, TIOCM_LOOP bit might be used only for setting. I'll drop the bit manipulation from get_mctrl callback. Thanks for the heads up. Best Regards, Chanho Park
diff --git a/drivers/tty/serial/samsung_tty.c b/drivers/tty/serial/samsung_tty.c index d5ca904def34..d7d035cd95c0 100644 --- a/drivers/tty/serial/samsung_tty.c +++ b/drivers/tty/serial/samsung_tty.c @@ -1002,16 +1002,22 @@ static unsigned int s3c24xx_serial_tx_empty(struct uart_port *port) static unsigned int s3c24xx_serial_get_mctrl(struct uart_port *port) { unsigned int umstat = rd_reg(port, S3C2410_UMSTAT); + unsigned int ucon = rd_reg(port, S3C2410_UCON); + unsigned int mctrl = TIOCM_CAR | TIOCM_DSR; if (umstat & S3C2410_UMSTAT_CTS) - return TIOCM_CAR | TIOCM_DSR | TIOCM_CTS; - else - return TIOCM_CAR | TIOCM_DSR; + mctrl |= TIOCM_CTS; + + if (ucon & S3C2443_UCON_LOOPBACK) + mctrl |= TIOCM_LOOP; + + return mctrl; } 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 +1025,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 |= S3C2443_UCON_LOOPBACK; + else + ucon &= ~S3C2443_UCON_LOOPBACK; + + wr_regl(port, S3C2410_UCON, ucon); } static void s3c24xx_serial_break_ctl(struct uart_port *port, int break_state)
Internal loopback mode can be supported by setting S3C2443_UCON_LOOPBACK bit. The mode & bit can be supported since s3c2410 and later SoCs. We can test it by linux-serial-test program[1] with -k option. It will set TIOCM_LOOP mode during test. -k, --loopback Use internal hardware loop back [1]: https://github.com/cbrake/linux-serial-test Signed-off-by: Chanho Park <chanho61.park@samsung.com> --- drivers/tty/serial/samsung_tty.c | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-)