Hi Brian, and sorry about the late reply. I was also waiting to see if you'd address the issues pointed out by Greg. On Sun, Oct 08, 2023 at 08:34:23PM -0600, Brian Kloppenborg wrote: > This is my first patch to the Linux kernel. There are some form issues as Greg's bot mentioned, like there being no commit message, missing Subject prefix, missing Signed-off by, and some coding style issues (space after if keyword, brackets around single line statements, etc). Make sure you have read Documentation/process/submitting-patches.rst and you should run scripts/checkpatch.pl on your patches before posting as it would find some of these issues. Just looking at the git log for this driver may also give you an idea of the expected patch format. > Patch 1 enables support for modem line status changes to the cp210x > driver. This is required to receive pulse-per-second (PPS) signals > from GPS receivers. Support for this feature exists in the FTDI > driver, but is not present in cp210x. The patch is implemented through > (1) enabling the device's event mode by default when the port is > opened or closed, and (2) registering the CTS, DSR, RI, and DCD > changes with the kernel through conventional means. So there are a few issues with this. First, as I mentioned in the commit message when adding support for the event mode, on at least some of the cp210x devices modem events appeared to be buffered until the next character was received: https://lore.kernel.org/r/all/20200713105517.27796-3-johan@xxxxxxxxxx/ Second, the event mode comes at a cost since all received characters needs to be processed one-by-one instead of simply being copied to the tty buffers. For those reasons, I left modem-event support unimplemented and only enabled event-mode when the user specifically requested input-parity checking. Perhaps some of these issues only affect some device types, and perhaps we can allow users to avoid the processing cost by only enabling event mode when, for example, CLOCAL is not set. Hmm, scratch that last bit, usb_serial_handle_dcd_change() would hang up the port when the CD is deasserted with !CLOCAL. > Patch 2 enables support for GPS PPS signals on the RI pin. While most > GPS devices typically expose this signal on the DCD pin, the Adafruit > Ultimate GPS with USB-C placed it on the RI pin instead. So this patch > is highly focused on that specific device. From what I can tell, the > usb_serial_handle_dcd_change function is used exclusively to register > PPS signals with the kernel, so calling it from the RI block shouldn't > result in unexpected behavior. So I'm afraid this is not going to work. First of all, serial drivers need to work with all types of devices and can't have hacks for quirky connected hardware like this. Second, the usb_serial_handle_dcd_change() also handles waiting for a modem connection on open and hangups using the carrier-detect line, which would break if called on RI instead of CD events. You also generally should not be using a slow USB device for things like PPS as I imagine latency and jitter would make it practically useless. Johan
