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

[Johan Hovold <johan@kernel.org>]

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