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@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
