| Message ID | 20231009023425.366783-1-brian@kloppenborg.net (mailing list archive) |
|---|---|
| Headers | show |
| Series | Enable modem line status events on cp210x, add support for PPS on RI pin | expand |
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@kernel.org/ 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
Johan, Thank you for returning to this topic. I apologize about not following through on Greg's comments. Life simply got in the way. I understand your concerns regarding this patch. The performance implications are clearly undesirable as are triggering open and hang-up operations on the CD line as a result of signals on the RI pin. Before I abandon this patch entirely, I am curious, is there any way we could enable the proposed behavior conditionally? For example, is there a way to pass a parameter to a driver much like one does a program? I ask because the timing accuracy of this device is much better than I anticipated. Despite the jitter introduced by USB polling, chrony is able to work out the correct time to better than 10 microseconds. This is to be contrasted with 200-1000 microseconds to typical internet-based NTP servers. While this is certainly much worse than PPS over a true serial port, it might be a reasonable performance compromise for machines without a real serial port. Please let me know your thoughts on this. Thank you, Brian On 12/15/23 06:02, Johan Hovold wrote: > 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@kernel.org/ > > 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
Dear Johan, This is my first patch to the Linux kernel. 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. 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. Please let me know if you require any further information. Regards Brian Kloppenborg Signed-off-by: Brian Kloppenborg <brian@kloppenborg.net> Brian Kloppenborg (2): Make cp210x respond to modem status changes (CTS, DSR, RI, DCD) by default. Make cp210x register GPS PPS signals on the RI pin. drivers/usb/serial/cp210x.c | 71 +++++++++++++++++++++++++++++++------ 1 file changed, 60 insertions(+), 11 deletions(-)