Re: [PATCH 0/2] Enable modem line status events on cp210x, add support for PPS on RI pin

[Brian Kloppenborg <bkloppenborg@gmail.com>]

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