On Tue, Nov 05, 2013 at 01:14:29PM +0100, Colin Leitner wrote:
> > Comment style should be
> >
> > /*
> > * ...
> > */
>
> I tried sticking to the style of the rest of the code but I'll keep that
> in mind from now on. Wouldn't that be something checkpatch.pl could check?
You'd think so, but I believe there's some subsystem which uses a
different convention.
> > But I'm not sure you need to be that verbose in this case. You can
> > probably just drop the comment.
>
> Will do.
>
> > 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.
>
> I moved it here, because I'd have to replicate the switch to handle
> old_termios->c_cflag instead of cflag:
>
> 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");
> if (!old_termios) {
> termios->c_cflag = (termios->c_cflag & ~CSIZE) | CS8;
>
> urb_value |= 8;
> dev_dbg(ddev, "Setting CS8\n");
> } else {
> termios->c_cflag = (termios->c_cflag & ~CSIZE) |
> (old_termios->c_cflag & CSIZE);
> switch (old_termios->c_cflag & CSIZE) {
> case CS7:
> urb_value |= 7;
> dev_dbg(ddev, "Setting CS7\n");
> break;
> case CS8:
> default:
> urb_value |= 8;
> dev_dbg(ddev, "Setting CS8\n");
> break;
> }
> }
> break;
> }
>
> Not sure if that's really an enhancement.
No, you're right. Sorry. Stick to how you did it in v1, but:
> + /* 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 */
you can drop the second part of the comment, and
> + if (((termios->c_cflag & CSIZE) != CS8) &&
> + ((termios->c_cflag & CSIZE) != CS7)) {
use the C_CSIZE(tty) macro here and the clear the CSIZE bits before
or:ing (|=) in the conditional below. That should make the code a bit
more compact and easier to read.
> + 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");
> + }
> +
> > 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?
>
> Yes, that's correct. I have no protocol documentation at hand to see what
> that means but it clearly doesn't switch to 0 data bits :).
You could mention it in you commit message anyway if you want (as you
are also fixing the malformed request).
Thanks,
Johan
--
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
