Re: [PATCH] serial: samsung: add support for manual RTS setting

[Tomasz Figa <t.figa@samsung.com>]

Hi José,

Please see my comments below.

On Wednesday 11 of September 2013 11:08:27 José Miguel Gonçalves wrote:
> The Samsung serial driver currently does not support setting the
> RTS pin with an ioctl(TIOCMSET) call. This patch adds this support.
> 
> Signed-off-by: José Miguel Gonçalves <jose.goncalves@inov.pt>
> ---
>  drivers/tty/serial/samsung.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/tty/serial/samsung.c b/drivers/tty/serial/samsung.c
> index f3dfa19..e5dd808 100644
> --- a/drivers/tty/serial/samsung.c
> +++ b/drivers/tty/serial/samsung.c
> @@ -407,7 +407,14 @@ static unsigned int s3c24xx_serial_get_mctrl(struct
> uart_port *port)
> 
>  static void s3c24xx_serial_set_mctrl(struct uart_port *port, unsigned
> int mctrl) {
> -	/* todo - possibly remove AFC and do manual CTS */
> +	unsigned int umcon = rd_regl(port, S3C2410_UMCON);
> +
> +	if (mctrl & TIOCM_RTS)
> +		umcon |= S3C2410_UMCOM_RTS_LOW;
> +	else
> +		umcon &= ~S3C2410_UMCOM_RTS_LOW;
> +
> +	wr_regl(port, S3C2410_UMCON, umcon);

I wonder if port capability shouldn't be considered here. Depending on SoC, 
only selected ports provide modem control capability.

For example on S3C64xx only ports 0 and 1 support modem control, while 
ports 2 and 3 don't.

>  }
> 
>  static void s3c24xx_serial_break_ctl(struct uart_port *port, int
> break_state) @@ -774,8 +781,6 @@ static void
> s3c24xx_serial_set_termios(struct uart_port *port, if (termios->c_cflag
> & CSTOPB)
>  		ulcon |= S3C2410_LCON_STOPB;
> 
> -	umcon = (termios->c_cflag & CRTSCTS) ? S3C2410_UMCOM_AFC : 0;
> -
>  	if (termios->c_cflag & PARENB) {
>  		if (termios->c_cflag & PARODD)
>  			ulcon |= S3C2410_LCON_PODD;
> @@ -792,6 +797,12 @@ static void s3c24xx_serial_set_termios(struct
> uart_port *port,
> 
>  	wr_regl(port, S3C2410_ULCON, ulcon);
>  	wr_regl(port, S3C2410_UBRDIV, quot);
> +
> +	if (termios->c_cflag & CRTSCTS)
> +		umcon = S3C2410_UMCOM_AFC;

Is it correct to override the last manual RTS value set to this register 
when activating manual flow control?

Shouldn't the code be more like the following:

	umcon = rd_regb(port, S3C2410_UMCON);
	if (termios->c_cflag & CRTSCTS)
		umcon |= S3C2410_UMCOM_AFC;
	else
		umcon &= ~S3C2410_UMCOM_AFC;
	wr_regl(port, S3C2410_UMCON, umcon);

Probably port capability should be considered here as well.

Best regards,
Tomasz

--
To unsubscribe from this list: send the line "unsubscribe linux-serial" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html