On Tue, Nov 05, 2013 at 11:10:29AM +0100, Colin Leitner wrote:
> FTDI UARTs support only 7 or 8 data dits. Until now the ftdi_sio driver would
s/dits/bits/
> only report this limitation for CS6 to dmesg and fail to reflect this fact to
> tcgetattr.
>
> This patch reverts the unsupported CSIZE setting and reports the fact with less
> severance to dmesg for both CS5 and CS6.
>
> To test the patch it's sufficient to call
>
> stty -F /dev/ttyUSB0 cs5
>
> which will succeed without the patch and report an error with the patch
> applied.
>
> Signed-off-by: Colin Leitner <colin.leitner@xxxxxxxxx>
> ---
> drivers/usb/serial/ftdi_sio.c | 41 ++++++++++++++++++++++++++++-------------
> 1 file changed, 28 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
> index b21d553..2ff128c 100644
> --- a/drivers/usb/serial/ftdi_sio.c
> +++ b/drivers/usb/serial/ftdi_sio.c
> @@ -2115,6 +2115,21 @@ static void ftdi_set_termios(struct tty_struct *tty,
> termios->c_cflag |= CRTSCTS;
> }
>
> + /* All FTDI UART chips are limited to CS7/8. We won't pretend to
> + support CS5/6 and revert the CSIZE setting instead.
> +
> + This is no critical error, as termios(3) clearly states that users
> + have to check the settings to see if they took effect */
Comment style should be
/*
* ...
*/
But I'm not sure you need to be that verbose in this case. You can
probably just drop the comment.
> + if (((termios->c_cflag & CSIZE) != CS8) &&
> + ((termios->c_cflag & CSIZE) != CS7)) {
> + if (old_termios)
> + termios->c_cflag = (termios->c_cflag & ~CSIZE) |
> + (old_termios->c_cflag & CSIZE);
> + else
> + termios->c_cflag = (termios->c_cflag & ~CSIZE) | CS8;
> + dev_info(ddev, "requested CSIZE setting not supported\n");
> + }
> +
Please move this termios update to the default-case in the switch below.
It's ok to do some redundant processing even the resulting data bits
settings remain unchanged.
Perhaps dev_warn is more appropriate than dev_info?
> cflag = termios->c_cflag;
>
> if (!old_termios)
> @@ -2151,19 +2166,19 @@ no_skip:
> } else {
> urb_value |= FTDI_SIO_SET_DATA_PARITY_NONE;
> }
> - if (cflag & CSIZE) {
> - switch (cflag & CSIZE) {
> - case CS7:
> - urb_value |= 7;
> - dev_dbg(ddev, "Setting CS7\n");
> - break;
> - case CS8:
> - urb_value |= 8;
> - dev_dbg(ddev, "Setting CS8\n");
> - break;
> - default:
> - dev_err(ddev, "CSIZE was set but not CS7-CS8\n");
> - }
> + switch (cflag & CSIZE) {
> + case CS7:
> + urb_value |= 7;
> + dev_dbg(ddev, "Setting CS7\n");
> + break;
> + case CS8:
> + urb_value |= 8;
> + dev_dbg(ddev, "Setting CS8\n");
> + break;
> + default:
> + /* It would actually be a driver bug if we ended up in here */
> + dev_err(ddev, "CSIZE was set but not CS7-CS8\n");
Do the dev_warn and termios settings revert here.
> + break;
Doesn't this all mean that we've been trying to set 0 data bits
(whatever that would mean) if CS5 or CS6 was requested this far, as the
least significant byte of urb_value would then have been zero?
Care to send a v2?
Please include a "[PATCH v2]" subject-prefix when you resend. Sending as
a reply to this mail is also a good idea if possible, in order to keep
everything in one mail thread. The git send-email command has an
--in-reply-to switch if you want to use that.
Thanks,
Johan
> }
>
> /* This is needed by the break command since it uses the same command
> --
> 1.7.10.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
