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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html