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

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

[Brian Kloppenborg]

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(-)

-- 
2.34.1

[PATCH 1/2] Make cp210x respond to modem status changes (CTS, DSR, RI, DCD) by default.

[Brian Kloppenborg]

---
 drivers/usb/serial/cp210x.c | 62 ++++++++++++++++++++++++++++++-------
 1 file changed, 51 insertions(+), 11 deletions(-)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index 1e61fe043171..af96d592456b 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -285,6 +285,7 @@ struct cp210x_port_private {
 	bool			event_mode;
 	enum cp210x_event_state event_state;
 	u8			lsr;
+	u8			msr;
 
 	struct mutex		mutex;
 	bool			crtscts;
@@ -459,6 +460,15 @@ struct cp210x_comm_status {
 #define CP210X_LSR_FRAME	BIT(3)
 #define CP210X_LSR_BREAK	BIT(4)
 
+/* Bits for Modem Status EMBED_EVENTS as described in AN571 */
+#define CP210X_MSR_DELTA_CTS_BIT 	BIT(0)
+#define CP210X_MSR_DELTA_DSR_BIT 	BIT(1)
+#define CP210X_MSR_DELTA_RI_BIT 	BIT(2)
+#define CP210X_MSR_DELTA_DCD_BIT 	BIT(3)
+#define CP210X_MSR_CTS_STATE_BIT 	BIT(4)
+#define CP210X_MSR_DSR_STATE_BIT 	BIT(5)
+#define CP210X_MSR_RI_STATE_BIT 	BIT(6)
+#define CP210X_MSR_DCD_STATE_BIT	BIT(7)
 
 /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
 struct cp210x_flow_ctl {
@@ -786,6 +796,8 @@ static int cp210x_open(struct tty_struct *tty, struct usb_serial_port *port)
 	if (result)
 		goto err_disable;
 
+	cp210x_enable_event_mode(port);
+
 	return 0;
 
 err_disable:
@@ -799,6 +811,8 @@ static void cp210x_close(struct usb_serial_port *port)
 {
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
 
+	cp210x_disable_event_mode(port);
+
 	usb_serial_generic_close(port);
 
 	/* Clear both queues; cp2108 needs this to avoid an occasional hang */
@@ -829,6 +843,41 @@ static void cp210x_process_lsr(struct usb_serial_port *port, unsigned char lsr,
 	}
 }
 
+static void cp210x_process_msr(struct usb_serial_port *port, unsigned char msr, char *flag)
+{
+	struct tty_struct *tty;
+
+	if(msr & CP210X_MSR_DELTA_CTS_BIT) {
+		port->icount.cts++;
+	}
+
+	if(msr & CP210X_MSR_DELTA_DSR_BIT) {
+		port->icount.dsr++;
+	}
+
+	if(msr & CP210X_MSR_DELTA_RI_BIT) {
+		port->icount.rng++;
+	}
+
+	if(msr & CP210X_MSR_DELTA_DCD_BIT) {
+		port->icount.dcd++;
+
+		tty = tty_port_tty_get(&port->port);
+		if (tty) {
+			usb_serial_handle_dcd_change(port, tty,
+				(msr) & CP210X_MSR_DCD_STATE_BIT);
+		}
+		tty_kref_put(tty);
+
+	}
+
+	if(msr & CP210X_MSR_CTS_STATE_BIT) {
+		port->icount.cts++;
+	}
+
+	wake_up_interruptible(&port->port.delta_msr_wait);
+}
+
 static bool cp210x_process_char(struct usb_serial_port *port, unsigned char *ch, char *flag)
 {
 	struct cp210x_port_private *port_priv = usb_get_serial_port_data(port);
@@ -880,9 +929,9 @@ static bool cp210x_process_char(struct usb_serial_port *port, unsigned char *ch,
 		break;
 	case ES_MSR:
 		dev_dbg(&port->dev, "%s - msr = 0x%02x\n", __func__, *ch);
-		/* unimplemented */
+		port_priv->msr = *ch;
+		cp210x_process_msr(port, port_priv->msr, flag);
 		port_priv->event_state = ES_DATA;
-		break;
 	}
 
 	return true;
@@ -1310,15 +1359,6 @@ static void cp210x_set_termios(struct tty_struct *tty,
 		dev_err(&port->dev, "failed to set line control: %d\n", ret);
 
 	cp210x_set_flow_control(tty, port, old_termios);
-
-	/*
-	 * Enable event-insertion mode only if input parity checking is
-	 * enabled for now.
-	 */
-	if (I_INPCK(tty))
-		cp210x_enable_event_mode(port);
-	else
-		cp210x_disable_event_mode(port);
 }
 
 static int cp210x_tiocmset(struct tty_struct *tty,
-- 
2.34.1

Re: [PATCH 1/2] Make cp210x respond to modem status changes (CTS, DSR, RI, DCD) by default.

[Greg KH]

On Sun, Oct 08, 2023 at 08:34:24PM -0600, Brian Kloppenborg wrote:
> ---
>  drivers/usb/serial/cp210x.c | 62 ++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 

Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/process/submitting-patches.rst and resend
  it after adding that line.  Note, the line needs to be in the body of
  the email, before the patch, not at the bottom of the patch or in the
  email signature.

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what is needed in
  order to properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what a proper
  Subject: line should look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

Re: [PATCH 1/2] Make cp210x respond to modem status changes (CTS, DSR, RI, DCD) by default.

[Greg KH]

On Sun, Oct 08, 2023 at 08:34:24PM -0600, Brian Kloppenborg wrote:
> ---
>  drivers/usb/serial/cp210x.c | 62 ++++++++++++++++++++++++++++++-------
>  1 file changed, 51 insertions(+), 11 deletions(-)
> 


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch contains warnings and/or errors noticed by the
  scripts/checkpatch.pl tool.


If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

[PATCH 2/2] Make cp210x register GPS PPS signals on the RI pin.

[Brian Kloppenborg]

---
 drivers/usb/serial/cp210x.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
index af96d592456b..458360851a71 100644
--- a/drivers/usb/serial/cp210x.c
+++ b/drivers/usb/serial/cp210x.c
@@ -857,6 +857,15 @@ static void cp210x_process_msr(struct usb_serial_port *port, unsigned char msr,
 
 	if(msr & CP210X_MSR_DELTA_RI_BIT) {
 		port->icount.rng++;
+
+		// Support PPS signal on Ring Indicator pin. While uncommon, this is
+		// found on some devices, like the Adafruit Ultimate GPS USB-C edition.
+		tty = tty_port_tty_get(&port->port);
+		if (tty) {
+			usb_serial_handle_dcd_change(port, tty,
+				(msr) & CP210X_MSR_RI_STATE_BIT);
+		}
+		tty_kref_put(tty);
 	}
 
 	if(msr & CP210X_MSR_DELTA_DCD_BIT) {
-- 
2.34.1

Re: [PATCH 2/2] Make cp210x register GPS PPS signals on the RI pin.

[Greg KH]

On Sun, Oct 08, 2023 at 08:34:25PM -0600, Brian Kloppenborg wrote:
> ---
>  drivers/usb/serial/cp210x.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 


Hi,

This is the friendly patch-bot of Greg Kroah-Hartman.  You have sent him
a patch that has triggered this response.  He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created.  Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.

You are receiving this message because of the following common error(s)
as indicated below:

- Your patch does not have a Signed-off-by: line.  Please read the
  kernel file, Documentation/process/submitting-patches.rst and resend
  it after adding that line.  Note, the line needs to be in the body of
  the email, before the patch, not at the bottom of the patch or in the
  email signature.

- You did not specify a description of why the patch is needed, or
  possibly, any description at all, in the email body.  Please read the
  section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what is needed in
  order to properly describe the change.

- You did not write a descriptive Subject: for the patch, allowing Greg,
  and everyone else, to know what this patch is all about.  Please read
  the section entitled "The canonical patch format" in the kernel file,
  Documentation/process/submitting-patches.rst for what a proper
  Subject: line should look like.

If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.

thanks,

greg k-h's patch email bot

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

[Johan Hovold]

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

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

[Brian Kloppenborg]

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