Hi,
> > Note that the patch has only been fully tested with kernel 4.9.28, and
> > test-compiled against 4.16.8. Tested only with the pl2303 adapters I have
> > available (which seem to be type 0 if I interpret the code correctly), I
> > don't have a clue whether this works with other versions of the chip.
>
> What's the usb-devices (or lsusb -v) output for your device?
| T: Bus=06 Lev=01 Prnt=01 Port=02 Cnt=02 Dev#= 4 Spd=12 MxCh= 0
| D: Ver= 1.10 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1
| P: Vendor=067b ProdID=2303 Rev=03.00
| S: Manufacturer=Prolific Technology Inc.
| S: Product=USB-Serial Controller
| C: #Ifs= 1 Cfg#= 1 Atr=80 MxPwr=100mA
| I: If#= 0 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=00 Prot=00 Driver=pl2303
> > --- linux-4.16.8/drivers/usb/serial/pl2303.c.orig 2018-05-09 09:53:14.000000000 +0200
> > +++ linux-4.16.8/drivers/usb/serial/pl2303.c 2018-05-14 04:32:27.860780856 +0200
> > @@ -544,8 +544,12 @@ static void pl2303_set_termios(struct tt
> > int ret;
> > u8 control;
> >
> > - if (old_termios && !tty_termios_hw_change(&tty->termios, old_termios))
> > - return;
> > + if (old_termios &&
> > + !(tty_termios_hw_change(&tty->termios, old_termios) ||
> > + ((tty->termios.c_iflag ^ old_termios->c_iflag) & (IXON | IXANY)) ||
> > + tty->termios.c_cc[VSTOP] != old_termios->c_cc[VSTOP] ||
> > + tty->termios.c_cc[VSTART] != old_termios->c_cc[VSTART]))
>
> Use a helper function for this (if everything is indeed needed, see
> below).
>
> > + return;
>
> Odd indentation (which checkpatch would have caught).
Well, it did, but it didn't exactly offer a suggestion as to how to
reformat things that would still be readable, nor does the style guide,
AFAICS, so I kept it as it is. (Also, I doubt that had I encountered this
condition moved out of line into a separate function that would have made
it easier for me to follow what's going on.)
> > buf = kzalloc(7, GFP_KERNEL);
> > if (!buf) {
> > @@ -662,6 +666,9 @@ static void pl2303_set_termios(struct tt
> > pl2303_vendor_write(serial, 0x0, 0x41);
> > else
> > pl2303_vendor_write(serial, 0x0, 0x61);
> > + } else if (I_IXON(tty) && !I_IXANY(tty) &&
> > + START_CHAR(tty) == 0x11 && STOP_CHAR(tty) == 0x13) {
>
> If the device does not support IXANY, I think you should just clear that
> bit to indicate lack of support.
>
> And the start and stop characters cannot be modified (as far as you
> know)? Then we should probably set these in the termios whenever IXON is
> set as well since usbserial does not support falling back to the
> line-discipline IXON implementation.
I don't know how to enable IXANY or change the control characters, so I am
working with what I do know (also, I guess the screenshot of the "ROM
programming tool" in the "datasheet" suggests that none of that is
supported).
Now, I agree that signaling lack of support would be better in general, but
it does change what the code does for (still) unsupported cases. After all,
usbserial in general does not signal that it doesn't support IXON/IXANY,
but just pretends that it does. So, if there is a good reason why usbserial
ignores POSIX on this point, I would think that would still apply for the
remaining unsupported cases after partial support is implemented?!
However, in any case, the part of setting particular control characters
only when IXON is set seems like a very bad idea, in that it potentially
changes settings where no change was requested, so userspace code cannot
really be expected to check for that (and certainly not based on POSIX,
which explicitly states that other fields have to stay unchanged). So, if
we want to go with indicating lack of support, I think the correct strategy
would be to always force the control characters to what the hardware
supports.
> > + pl2303_vendor_write(serial, 0x0, 0xc0);
>
> Odd indentation again.
See above.
Regards, Florian
--
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
