[PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[Mathieu Peyrega]

Hello,

this is my first contribution to this list (and the Linux kernel) and I'd
like trying to revive a subject already discussed in a 2017/2018 thread about
adding a possibility to use the CTS pin instead of the DCD pin for 1PPS line
disciplining (cf. https://www.spinics.net/lists/linux-serial/msg27604.html)

The rationale is similar to the original poster's one: some TTL UARTs hardware
implementations have an incomplete wiring and do not expose a DCD pin (e.g. on
some SBCs). On those platforms only TX, RX, CTS and RTS are available. In such
cases, being able to use the CTS pin for 1PPS time disciplining is useful.

In addition, to that primary need, I believe there is another missing feature
in current implementation. Some non-RS232 UARTs (e.g. TTL UARTs also often
found on SBCs) have an inverted behaviour with respect to RS232 rising edge or
falling edge vs. assert or clear logics. Not taking this inversion into account
results in a disciplining where the kernel time ends with an offset from actual
signal time. Offset value is the width/duration of the 1PPS square pulse signal.
At least this is what I experienced in the testing process on an Odroid H2 SBC
(Intel x86_64 based) and a GNSS driven 1PPS signal (from a CORS station that I
manage). Maybe this can be handled from a later userland process (e.g. ntpd)
but I believe that being able to handle it straight at kernel level is better.

As for the module behaviour control, I went with adding 2 parameters to
pps-ldisc:

-	activepin (charp) wich can be dcd (default) or cts and drives the pin
	which should be consider to detect the 1PPS signal
-	invertlogic (bool) which can be false (default) or true and defines if
	the driver complies with a RS232 type signal where assert is on the
	rising edge or inverted as for some TTL UARTs. Default values match
	the current behaviour.

Those 2 parameters were made runtime readable/writable with standard kernel
module parameter callback mecanism. Final configuration does not really require
this writable feature, but it allows easier tuning (finding correct parameters
for a given 1PPS source and UART) together with ppstest/ppscheck tools as it
allows seeing the effect of inverting the logic, especially when the width or
polarity of the 1PPS signal is not well known.

As for the implementation details, it was chosen to extend the tty_ldisc_ops
structure with a new cts_change function pointer similar to the existing
dcd_change. Another option may have been to fully reuse the existing structure
and its int flag C field (which seems unused in current code as far as I
understand). From there it should be possible to implement some logics to use
dcd_change and flag value even for handling 1PPS on CTS pin. This choice would
have had less impacts on the existing interfaces (no changes to
include/linux/tty_ldisc.h) but suppose using the dcd_change named function
pointer to actually handle cts changes (I don't really like it), and this
implementation choice would also require tests on more variables (flag and
ops->dcd_change) altough those tests happens at low frequency (1Hz) and there
should be no performance impact. I prefered going the other way and modify the
interface. The flag plus renaming dcd_change to activepin_change could also be
a third option, but it also modifies the tty_ldisc_ops (while keeping it the
same size). Parameter settings runtime changes are checked and only cts or dcd
(caseless) is allowed for activepin, with or without a trailing cariage return
to allow missing -n in configuration commands like:
echo -n cts > /sys/module/pps_ldisc/parameters/activepin

The patch modifies 3 files under both PPS SUPPORT and TTY LAYER maintainer
reponsabilities. I have seen no requirements in the MAINTAINER file for the
PPS SUPPORT tree part so the patch is proposed against main branch of the
TTY LAYER git tree.

It has been tested to compile on this tree/branch only for x86_64 (I have not
performed cross compilation tests). Runtime behaviour has been mainly tested
against the 5.11.40 branch (both generic and lowlatency flavours) of the
ubuntu focal kernel on an Odroid H2 SBC (x86_64). The tests were conducted
with a rising edge aligned 1PPS signal, 100ms width and connected to CTS pin
of ttyS0 UART. I can provide some results if needed (such as effect of runtime
switching invertlogic on ppstest / ppscheck values).

I tried and hope I complied with most of the rules for proposing a patch and
posting here.
(By the way, I have not been able to reach the wiki or mailing list in the
PPS SUPPORT section of MAINTAINERS file.)

Best regards

From 7f515e29499bbd060d72bcb58d28220c9d9644be Mon Sep 17 00:00:00 2001
From: Mathieu Peyrega <mathieu.peyrega@gmail.com>
Date: Thu, 2 Dec 2021 09:55:10 +0100
Subject: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs

Signed-off-by: Mathieu Peyrega <mathieu.peyrega@gmail.com>
---
 drivers/pps/clients/pps-ldisc.c  | 137 ++++++++++++++++++++++++++++++-
 drivers/tty/serial/serial_core.c |  13 +++
 include/linux/tty_ldisc.h        |   6 ++
 3 files changed, 154 insertions(+), 2 deletions(-)

diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
index d73c4c2ed4e1..6221700ab3ad 100644
--- a/drivers/pps/clients/pps-ldisc.c
+++ b/drivers/pps/clients/pps-ldisc.c
@@ -8,12 +8,14 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/module.h>
+#include <linux/moduleparam.h>
 #include <linux/serial_core.h>
 #include <linux/tty.h>
 #include <linux/pps_kernel.h>
 #include <linux/bug.h>
+#include <linux/string.h>
 
-static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
+static void pps_tty_activepin_change(struct tty_struct *tty, unsigned int status)
 {
 	struct pps_device *pps;
 	struct pps_event_time ts;
@@ -36,6 +38,29 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
 			status ? "assert" : "clear", jiffies);
 }
 
+static void pps_tty_activepin_change_invertlogic(struct tty_struct *tty, unsigned int status)
+{
+	struct pps_device *pps;
+	struct pps_event_time ts;
+
+	pps_get_ts(&ts);
+
+	pps = pps_lookup_dev(tty);
+	/*
+	 * This should never fail, but the ldisc locking is very
+	 * convoluted, so don't crash just in case.
+	 */
+	if (WARN_ON_ONCE(pps == NULL))
+		return;
+
+	/* Now do the PPS event report */
+	pps_event(pps, &ts, status ? PPS_CAPTURECLEAR :
+			PPS_CAPTUREASSERT, NULL);
+
+	dev_dbg(pps->dev, "PPS %s at %lu\n",
+			status ? "assert" : "clear", jiffies);
+}
+
 static int (*alias_n_tty_open)(struct tty_struct *tty);
 
 static int pps_tty_open(struct tty_struct *tty)
@@ -99,10 +124,118 @@ static struct tty_ldisc_ops pps_ldisc_ops;
  * Module stuff
  */
 
+/*
+ * activepin parameter (dcd or cts) to handle the case of
+ * some SBC computers with incomplete UART wiring (no dcd)
+ * and still allow using cts as the 1PPS signal input pin
+ */
+
+static char *activepin = "dcd";
+
+/*
+ * invertlogic parameter (0 or 1) allows handling the case
+ * of some uarts (e.g. TTL UARTs) were the rising / falling
+ * edges have inverted logic with respect to conventions of
+ * the RS232 standard
+ */
+
+static bool invertlogic;
+
+static void pps_ldisc_update_settings(void)
+{
+	pr_info("%s: activepin=%s invertlogic=%c\n",
+		__func__, activepin, invertlogic?'Y':'N');
+
+	if (strncasecmp(activepin, "dcd", 3) == 0) {
+		pps_ldisc_ops.cts_change = 0;
+		if (!invertlogic)
+			pps_ldisc_ops.dcd_change = pps_tty_activepin_change;
+		else
+			pps_ldisc_ops.dcd_change = pps_tty_activepin_change_invertlogic;
+	} else if (strncasecmp(activepin, "cts", 3) == 0) {
+		pps_ldisc_ops.dcd_change = 0;
+		if (!invertlogic)
+			pps_ldisc_ops.cts_change = pps_tty_activepin_change;
+		else
+			pps_ldisc_ops.cts_change = pps_tty_activepin_change_invertlogic;
+	} else {
+		pps_ldisc_ops.cts_change = 0;
+		pps_ldisc_ops.dcd_change = 0;
+	}
+}
+
+static int pps_set_activepin(const char *val, const struct kernel_param *kp)
+{
+	int res = 0;
+	bool update_setting = false;
+	int lenval = strlen(val);
+	bool currentvalue_is_dcd = (strncasecmp(activepin, "dcd", 3) == 0);
+
+	if ((lenval == 3 && strncasecmp(val, "dcd", lenval) == 0) ||
+		(lenval == 4 && strncasecmp(val, "dcd\n", lenval) == 0)) {
+		if (!currentvalue_is_dcd) {
+			update_setting = true;
+			res = param_set_charp("dcd", kp);
+		}
+	} else if ((lenval == 3 && strncasecmp(val, "cts", lenval) == 0) ||
+		(lenval == 4 && strncasecmp(val, "cts\n", lenval) == 0)) {
+		if (currentvalue_is_dcd) {
+			update_setting = true;
+			res = param_set_charp("cts", kp);
+		}
+	} else
+		res = -1;
+
+	if (res == 0 && update_setting)
+		pps_ldisc_update_settings();
+	else if (res != 0)
+		pr_info("unable to set activepin parameter to %s\n", val);
+
+	return res;
+}
+
+static const struct kernel_param_ops activepin_ops = {
+	.set = &pps_set_activepin,
+	.get = &param_get_charp,
+};
+
+module_param_cb(activepin, &activepin_ops, &activepin, 0664);
+
+MODULE_PARM_DESC(activepin, \
+"\n\t\tparameter can be either dcd (default) or cts and defines the\n\
+\t\tphysical UART pin which receives the analogic PPS signal.");
+
+static int pps_set_invertlogic(const char *val, const struct kernel_param *kp)
+{
+	bool lastvalue = *(bool *)(kp->arg);
+	int res = param_set_bool(val, kp);
+
+	if (res == 0 && lastvalue != invertlogic)
+		pps_ldisc_update_settings();
+
+	return res;
+}
+
+static const struct kernel_param_ops invertlogic_ops = {
+	.set = &pps_set_invertlogic,
+	.get = &param_get_bool,
+};
+
+module_param_cb(invertlogic, &invertlogic_ops, &invertlogic, 0664);
+
+MODULE_PARM_DESC(invertlogic, \
+"\n\t\tflag can be either N (default) or Y and allows handling some\n\
+\t\tUARTs (e.g. TTL UARTs) whose reference edge vs. status logic\n\
+\t\t(rising/falling vs. active/inactive) is the opposite of the\n\
+\t\tRS232 standard.");
+
 static int __init pps_tty_init(void)
 {
 	int err;
 
+	pr_info("%s: activepin=%s invertlogic=%c\n",
+		__func__, activepin, invertlogic?'Y':'N');
+
 	/* Inherit the N_TTY's ops */
 	n_tty_inherit_ops(&pps_ldisc_ops);
 
@@ -114,7 +247,7 @@ static int __init pps_tty_init(void)
 	pps_ldisc_ops.owner = THIS_MODULE;
 	pps_ldisc_ops.num = N_PPS;
 	pps_ldisc_ops.name = "pps_tty";
-	pps_ldisc_ops.dcd_change = pps_tty_dcd_change;
+	pps_ldisc_update_settings();
 	pps_ldisc_ops.open = pps_tty_open;
 	pps_ldisc_ops.close = pps_tty_close;
 
diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index 1e738f265eea..abbab3fb1282 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -3096,8 +3096,21 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
  */
 void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
 {
+	struct tty_port *port = &uport->state->port;
+	struct tty_struct *tty = port->tty;
+	struct tty_ldisc *ld;
+
 	lockdep_assert_held_once(&uport->lock);
 
+	if (tty) {
+		ld = tty_ldisc_ref(tty);
+		if (ld) {
+			if (ld->ops->cts_change)
+				ld->ops->cts_change(tty, status);
+			tty_ldisc_deref(ld);
+		}
+	}
+
 	uport->icount.cts++;
 
 	if (uart_softcts_mode(uport)) {
diff --git a/include/linux/tty_ldisc.h b/include/linux/tty_ldisc.h
index b85d84fb5f49..1ec282b7c584 100644
--- a/include/linux/tty_ldisc.h
+++ b/include/linux/tty_ldisc.h
@@ -112,6 +112,11 @@ struct tty_struct;
  *	Tells the discipline that the DCD pin has changed its status.
  *	Used exclusively by the N_PPS (Pulse-Per-Second) line discipline.
  *
+ * void (*cts_change)(struct tty_struct *tty, unsigned int status)
+ *
+ *	Tells the discipline that the CTS pin has changed its status.
+ *	Used exclusively by the N_PPS (Pulse-Per-Second) line discipline.
+ *
  * int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
  *			char *fp, int count);
  *
@@ -208,6 +213,7 @@ struct tty_ldisc_ops {
 			       const char *fp, int count);
 	void	(*write_wakeup)(struct tty_struct *);
 	void	(*dcd_change)(struct tty_struct *, unsigned int);
+	void	(*cts_change)(struct tty_struct *, unsigned int);
 	int	(*receive_buf2)(struct tty_struct *, const unsigned char *cp,
 				const char *fp, int count);
 
-- 
2.25.1

Re: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[Greg KH]

On Thu, Dec 02, 2021 at 11:56:10AM +0100, Mathieu Peyrega wrote:
> Hello,
> 
> this is my first contribution to this list (and the Linux kernel) and I'd
> like trying to revive a subject already discussed in a 2017/2018 thread about
> adding a possibility to use the CTS pin instead of the DCD pin for 1PPS line
> disciplining (cf. https://www.spinics.net/lists/linux-serial/msg27604.html)

A few meta-comments about the patch.

Look at how other patches are submitted, there's no need for this type
of introduction in the changelog text, and in fact, you have no
changelog text in your patch at all.  So the body of this email needs to
go into the changelog area, please do so when you resend.


> 
> The rationale is similar to the original poster's one: some TTL UARTs hardware
> implementations have an incomplete wiring and do not expose a DCD pin (e.g. on
> some SBCs). On those platforms only TX, RX, CTS and RTS are available. In such
> cases, being able to use the CTS pin for 1PPS time disciplining is useful.
> 
> In addition, to that primary need, I believe there is another missing feature
> in current implementation. Some non-RS232 UARTs (e.g. TTL UARTs also often
> found on SBCs) have an inverted behaviour with respect to RS232 rising edge or
> falling edge vs. assert or clear logics. Not taking this inversion into account
> results in a disciplining where the kernel time ends with an offset from actual
> signal time. Offset value is the width/duration of the 1PPS square pulse signal.
> At least this is what I experienced in the testing process on an Odroid H2 SBC
> (Intel x86_64 based) and a GNSS driven 1PPS signal (from a CORS station that I
> manage). Maybe this can be handled from a later userland process (e.g. ntpd)
> but I believe that being able to handle it straight at kernel level is better.
> 
> As for the module behaviour control, I went with adding 2 parameters to
> pps-ldisc:
> 
> -	activepin (charp) wich can be dcd (default) or cts and drives the pin
> 	which should be consider to detect the 1PPS signal
> -	invertlogic (bool) which can be false (default) or true and defines if
> 	the driver complies with a RS232 type signal where assert is on the
> 	rising edge or inverted as for some TTL UARTs. Default values match
> 	the current behaviour.

New module parameters should never be added, as they only affect the
whole system, and not on a per-device basis.  If you have to add new
options, please do it some other way.

Also, by adding new options, that means you are changing the logic here,
why not create a new line discipline that acts the way you want it to
act?  The existing one is very small, it should not be much to make a
new one with just the new functionality, right?

> Those 2 parameters were made runtime readable/writable with standard kernel
> module parameter callback mecanism. Final configuration does not really require
> this writable feature, but it allows easier tuning (finding correct parameters
> for a given 1PPS source and UART) together with ppstest/ppscheck tools as it
> allows seeing the effect of inverting the logic, especially when the width or
> polarity of the 1PPS signal is not well known.
> 
> As for the implementation details, it was chosen to extend the tty_ldisc_ops
> structure with a new cts_change function pointer similar to the existing
> dcd_change. Another option may have been to fully reuse the existing structure
> and its int flag C field (which seems unused in current code as far as I
> understand). From there it should be possible to implement some logics to use
> dcd_change and flag value even for handling 1PPS on CTS pin. This choice would
> have had less impacts on the existing interfaces (no changes to
> include/linux/tty_ldisc.h) but suppose using the dcd_change named function
> pointer to actually handle cts changes (I don't really like it), and this
> implementation choice would also require tests on more variables (flag and
> ops->dcd_change) altough those tests happens at low frequency (1Hz) and there
> should be no performance impact. I prefered going the other way and modify the
> interface. The flag plus renaming dcd_change to activepin_change could also be
> a third option, but it also modifies the tty_ldisc_ops (while keeping it the
> same size). Parameter settings runtime changes are checked and only cts or dcd
> (caseless) is allowed for activepin, with or without a trailing cariage return
> to allow missing -n in configuration commands like:
> echo -n cts > /sys/module/pps_ldisc/parameters/activepin
> 
> The patch modifies 3 files under both PPS SUPPORT and TTY LAYER maintainer
> reponsabilities. I have seen no requirements in the MAINTAINER file for the
> PPS SUPPORT tree part so the patch is proposed against main branch of the
> TTY LAYER git tree.
> 
> It has been tested to compile on this tree/branch only for x86_64 (I have not
> performed cross compilation tests). Runtime behaviour has been mainly tested
> against the 5.11.40 branch (both generic and lowlatency flavours) of the
> ubuntu focal kernel on an Odroid H2 SBC (x86_64). The tests were conducted
> with a rising edge aligned 1PPS signal, 100ms width and connected to CTS pin
> of ttyS0 UART. I can provide some results if needed (such as effect of runtime
> switching invertlogic on ppstest / ppscheck values).
> 
> I tried and hope I complied with most of the rules for proposing a patch and
> posting here.
> (By the way, I have not been able to reach the wiki or mailing list in the
> PPS SUPPORT section of MAINTAINERS file.)
> 
> Best regards
> 
> >From 7f515e29499bbd060d72bcb58d28220c9d9644be Mon Sep 17 00:00:00 2001
> From: Mathieu Peyrega <mathieu.peyrega@gmail.com>
> Date: Thu, 2 Dec 2021 09:55:10 +0100
> Subject: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs
> 
> Signed-off-by: Mathieu Peyrega <mathieu.peyrega@gmail.com>
> ---
>  drivers/pps/clients/pps-ldisc.c  | 137 ++++++++++++++++++++++++++++++-

The current code is only 141 lines long, you doubled it, so I think a
seprate driver would be simpler :)



>  drivers/tty/serial/serial_core.c |  13 +++
>  include/linux/tty_ldisc.h        |   6 ++
>  3 files changed, 154 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pps/clients/pps-ldisc.c b/drivers/pps/clients/pps-ldisc.c
> index d73c4c2ed4e1..6221700ab3ad 100644
> --- a/drivers/pps/clients/pps-ldisc.c
> +++ b/drivers/pps/clients/pps-ldisc.c
> @@ -8,12 +8,14 @@
>  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>  
>  #include <linux/module.h>
> +#include <linux/moduleparam.h>
>  #include <linux/serial_core.h>
>  #include <linux/tty.h>
>  #include <linux/pps_kernel.h>
>  #include <linux/bug.h>
> +#include <linux/string.h>
>  
> -static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
> +static void pps_tty_activepin_change(struct tty_struct *tty, unsigned int status)
>  {
>  	struct pps_device *pps;
>  	struct pps_event_time ts;
> @@ -36,6 +38,29 @@ static void pps_tty_dcd_change(struct tty_struct *tty, unsigned int status)
>  			status ? "assert" : "clear", jiffies);
>  }
>  
> +static void pps_tty_activepin_change_invertlogic(struct tty_struct *tty, unsigned int status)
> +{
> +	struct pps_device *pps;
> +	struct pps_event_time ts;
> +
> +	pps_get_ts(&ts);
> +
> +	pps = pps_lookup_dev(tty);
> +	/*
> +	 * This should never fail, but the ldisc locking is very
> +	 * convoluted, so don't crash just in case.
> +	 */
> +	if (WARN_ON_ONCE(pps == NULL))
> +		return;
> +
> +	/* Now do the PPS event report */
> +	pps_event(pps, &ts, status ? PPS_CAPTURECLEAR :
> +			PPS_CAPTUREASSERT, NULL);
> +
> +	dev_dbg(pps->dev, "PPS %s at %lu\n",
> +			status ? "assert" : "clear", jiffies);
> +}
> +
>  static int (*alias_n_tty_open)(struct tty_struct *tty);
>  
>  static int pps_tty_open(struct tty_struct *tty)
> @@ -99,10 +124,118 @@ static struct tty_ldisc_ops pps_ldisc_ops;
>   * Module stuff
>   */
>  
> +/*
> + * activepin parameter (dcd or cts) to handle the case of
> + * some SBC computers with incomplete UART wiring (no dcd)
> + * and still allow using cts as the 1PPS signal input pin
> + */
> +
> +static char *activepin = "dcd";
> +
> +/*
> + * invertlogic parameter (0 or 1) allows handling the case
> + * of some uarts (e.g. TTL UARTs) were the rising / falling
> + * edges have inverted logic with respect to conventions of
> + * the RS232 standard
> + */
> +
> +static bool invertlogic;
> +
> +static void pps_ldisc_update_settings(void)
> +{
> +	pr_info("%s: activepin=%s invertlogic=%c\n",
> +		__func__, activepin, invertlogic?'Y':'N');
> +
> +	if (strncasecmp(activepin, "dcd", 3) == 0) {
> +		pps_ldisc_ops.cts_change = 0;
> +		if (!invertlogic)
> +			pps_ldisc_ops.dcd_change = pps_tty_activepin_change;
> +		else
> +			pps_ldisc_ops.dcd_change = pps_tty_activepin_change_invertlogic;
> +	} else if (strncasecmp(activepin, "cts", 3) == 0) {
> +		pps_ldisc_ops.dcd_change = 0;
> +		if (!invertlogic)
> +			pps_ldisc_ops.cts_change = pps_tty_activepin_change;
> +		else
> +			pps_ldisc_ops.cts_change = pps_tty_activepin_change_invertlogic;
> +	} else {
> +		pps_ldisc_ops.cts_change = 0;
> +		pps_ldisc_ops.dcd_change = 0;
> +	}
> +}
> +
> +static int pps_set_activepin(const char *val, const struct kernel_param *kp)
> +{
> +	int res = 0;
> +	bool update_setting = false;
> +	int lenval = strlen(val);
> +	bool currentvalue_is_dcd = (strncasecmp(activepin, "dcd", 3) == 0);
> +
> +	if ((lenval == 3 && strncasecmp(val, "dcd", lenval) == 0) ||
> +		(lenval == 4 && strncasecmp(val, "dcd\n", lenval) == 0)) {
> +		if (!currentvalue_is_dcd) {
> +			update_setting = true;
> +			res = param_set_charp("dcd", kp);
> +		}
> +	} else if ((lenval == 3 && strncasecmp(val, "cts", lenval) == 0) ||
> +		(lenval == 4 && strncasecmp(val, "cts\n", lenval) == 0)) {
> +		if (currentvalue_is_dcd) {
> +			update_setting = true;
> +			res = param_set_charp("cts", kp);
> +		}
> +	} else
> +		res = -1;
> +
> +	if (res == 0 && update_setting)
> +		pps_ldisc_update_settings();
> +	else if (res != 0)
> +		pr_info("unable to set activepin parameter to %s\n", val);
> +
> +	return res;
> +}
> +
> +static const struct kernel_param_ops activepin_ops = {
> +	.set = &pps_set_activepin,
> +	.get = &param_get_charp,
> +};
> +
> +module_param_cb(activepin, &activepin_ops, &activepin, 0664);
> +
> +MODULE_PARM_DESC(activepin, \
> +"\n\t\tparameter can be either dcd (default) or cts and defines the\n\
> +\t\tphysical UART pin which receives the analogic PPS signal.");
> +
> +static int pps_set_invertlogic(const char *val, const struct kernel_param *kp)
> +{
> +	bool lastvalue = *(bool *)(kp->arg);
> +	int res = param_set_bool(val, kp);
> +
> +	if (res == 0 && lastvalue != invertlogic)
> +		pps_ldisc_update_settings();
> +
> +	return res;
> +}
> +
> +static const struct kernel_param_ops invertlogic_ops = {
> +	.set = &pps_set_invertlogic,
> +	.get = &param_get_bool,
> +};
> +
> +module_param_cb(invertlogic, &invertlogic_ops, &invertlogic, 0664);
> +
> +MODULE_PARM_DESC(invertlogic, \
> +"\n\t\tflag can be either N (default) or Y and allows handling some\n\
> +\t\tUARTs (e.g. TTL UARTs) whose reference edge vs. status logic\n\
> +\t\t(rising/falling vs. active/inactive) is the opposite of the\n\
> +\t\tRS232 standard.");
> +
>  static int __init pps_tty_init(void)
>  {
>  	int err;
>  
> +	pr_info("%s: activepin=%s invertlogic=%c\n",
> +		__func__, activepin, invertlogic?'Y':'N');

You can remove your debugging code as well.

> +
>  	/* Inherit the N_TTY's ops */
>  	n_tty_inherit_ops(&pps_ldisc_ops);
>  
> @@ -114,7 +247,7 @@ static int __init pps_tty_init(void)
>  	pps_ldisc_ops.owner = THIS_MODULE;
>  	pps_ldisc_ops.num = N_PPS;
>  	pps_ldisc_ops.name = "pps_tty";
> -	pps_ldisc_ops.dcd_change = pps_tty_dcd_change;
> +	pps_ldisc_update_settings();
>  	pps_ldisc_ops.open = pps_tty_open;
>  	pps_ldisc_ops.close = pps_tty_close;
>  
> diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
> index 1e738f265eea..abbab3fb1282 100644
> --- a/drivers/tty/serial/serial_core.c
> +++ b/drivers/tty/serial/serial_core.c
> @@ -3096,8 +3096,21 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
>   */
>  void uart_handle_cts_change(struct uart_port *uport, unsigned int status)
>  {
> +	struct tty_port *port = &uport->state->port;
> +	struct tty_struct *tty = port->tty;
> +	struct tty_ldisc *ld;
> +
>  	lockdep_assert_held_once(&uport->lock);
>  
> +	if (tty) {
> +		ld = tty_ldisc_ref(tty);
> +		if (ld) {
> +			if (ld->ops->cts_change)
> +				ld->ops->cts_change(tty, status);
> +			tty_ldisc_deref(ld);
> +		}
> +	}

This is a pretty big abuse of CTS.  I understand your want for something
like this, but why not get a device that has a DCD line instead?

That's my biggest objection here, it feels like you are working around
really odd hardware with this change.  What real-world use case is this
for?

You also say you can handle this in userspace already, so why would this
have to be in the kernel?  What benefit is it for us to take this?

thanks,

greg k-h

Re: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[Mathieu Peyrega]

Le vendredi 03 décembre 2021 à 14:11 +0100, Greg KH a écrit :
> On Thu, Dec 02, 2021 at 11:56:10AM +0100, Mathieu Peyrega wrote:
> > Hello,
> > 
> > this is my first contribution to this list (and the Linux kernel)
> > and I'd
> > like trying to revive a subject already discussed in a 2017/2018
> > thread about
> > adding a possibility to use the CTS pin instead of the DCD pin for
> > 1PPS line
> > disciplining (cf. 
> > https://www.spinics.net/lists/linux-serial/msg27604.html)
> 
> A few meta-comments about the patch.
> 
> Look at how other patches are submitted, there's no need for this
> type
> of introduction in the changelog text, and in fact, you have no
> changelog text in your patch at all.  So the body of this email needs
> to
> go into the changelog area, please do so when you resend.

will do

> > The rationale is similar to the original poster's one: some TTL
> > UARTs hardware
> > implementations have an incomplete wiring and do not expose a DCD
> > pin (e.g. on
> > some SBCs). On those platforms only TX, RX, CTS and RTS are
> > available. In such
> > cases, being able to use the CTS pin for 1PPS time disciplining is
> > useful.
> > 
> > In addition, to that primary need, I believe there is another
> > missing feature
> > in current implementation. Some non-RS232 UARTs (e.g. TTL UARTs
> > also often
> > found on SBCs) have an inverted behaviour with respect to RS232
> > rising edge or
> > falling edge vs. assert or clear logics. Not taking this inversion
> > into account
> > results in a disciplining where the kernel time ends with an offset
> > from actual
> > signal time. Offset value is the width/duration of the 1PPS square
> > pulse signal.
> > At least this is what I experienced in the testing process on an
> > Odroid H2 SBC
> > (Intel x86_64 based) and a GNSS driven 1PPS signal (from a CORS
> > station that I
> > manage). Maybe this can be handled from a later userland process
> > (e.g. ntpd)
> > but I believe that being able to handle it straight at kernel level
> > is better.
> > 
> > As for the module behaviour control, I went with adding 2
> > parameters to
> > pps-ldisc:
> > 
> > -	activepin (charp) wich can be dcd (default) or cts and drives
> > the pin
> > 	which should be consider to detect the 1PPS signal
> > -	invertlogic (bool) which can be false (default) or true and
> > defines if
> > 	the driver complies with a RS232 type signal where assert is on
> > the
> > 	rising edge or inverted as for some TTL UARTs. Default values
> > match
> > 	the current behaviour.
> 
> New module parameters should never be added, as they only affect the
> whole system, and not on a per-device basis.  If you have to add new
> options, please do it some other way.

I don't fully understand the point. Isn't the existing pps_ldisc module
already affecting the whole system ? (with it's builtin fixed
"options"). How different tunable options such as the proposal make
things fundamentally different ? Still I agree that per device settings
would be better.

> 
> Also, by adding new options, that means you are changing the logic
> here,
> why not create a new line discipline that acts the way you want it to
> act?  The existing one is very small, it should not be much to make a
> new one with just the new functionality, right?

I can retry on this track. However I believe , it will also need
support from userland utility (especially ldattach). I don't know if
this kind of consequences is in the scope of the discussion here.

> 
> Those 2 parameters were made runtime readable/writable with standard
> kernel
> > module parameter callback mecanism. Final configuration does not
> > really require
> > this writable feature, but it allows easier tuning (finding correct
> > parameters
> > for a given 1PPS source and UART) together with ppstest/ppscheck
> > tools as it
> > allows seeing the effect of inverting the logic, especially when
> > the width or
> > polarity of the 1PPS signal is not well known.
> > 
> > As for the implementation details, it was chosen to extend the
> > tty_ldisc_ops
> > structure with a new cts_change function pointer similar to the
> > existing
> > dcd_change. Another option may have been to fully reuse the
> > existing structure
> > and its int flag C field (which seems unused in current code as far
> > as I
> > understand). From there it should be possible to implement some
> > logics to use
> > dcd_change and flag value even for handling 1PPS on CTS pin. This
> > choice would
> > have had less impacts on the existing interfaces (no changes to
> > include/linux/tty_ldisc.h) but suppose using the dcd_change named
> > function
> > pointer to actually handle cts changes (I don't really like it),
> > and this
> > implementation choice would also require tests on more variables
> > (flag and
> > ops->dcd_change) altough those tests happens at low frequency (1Hz)
> > and there
> > should be no performance impact. I prefered going the other way and
> > modify the
> > interface. The flag plus renaming dcd_change to activepin_change
> > could also be
> > a third option, but it also modifies the tty_ldisc_ops (while
> > keeping it the
> > same size). Parameter settings runtime changes are checked and only
> > cts or dcd
> > (caseless) is allowed for activepin, with or without a trailing
> > cariage return
> > to allow missing -n in configuration commands like:
> > echo -n cts > /sys/module/pps_ldisc/parameters/activepin
> > 
> > The patch modifies 3 files under both PPS SUPPORT and TTY LAYER
> > maintainer
> > reponsabilities. I have seen no requirements in the MAINTAINER file
> > for the
> > PPS SUPPORT tree part so the patch is proposed against main branch
> > of the
> > TTY LAYER git tree.
> > 
> > It has been tested to compile on this tree/branch only for x86_64
> > (I have not
> > performed cross compilation tests). Runtime behaviour has been
> > mainly tested
> > against the 5.11.40 branch (both generic and lowlatency flavours)
> > of the
> > ubuntu focal kernel on an Odroid H2 SBC (x86_64). The tests were
> > conducted
> > with a rising edge aligned 1PPS signal, 100ms width and connected
> > to CTS pin
> > of ttyS0 UART. I can provide some results if needed (such as effect
> > of runtime
> > switching invertlogic on ppstest / ppscheck values).
> > 
> > I tried and hope I complied with most of the rules for proposing a
> > patch and
> > posting here.
> > (By the way, I have not been able to reach the wiki or mailing list
> > in the
> > PPS SUPPORT section of MAINTAINERS file.)
> > 
> > Best regards
> > 
> > > From 7f515e29499bbd060d72bcb58d28220c9d9644be Mon Sep 17 00:00:00
> > > 2001
> > From: Mathieu Peyrega <mathieu.peyrega@gmail.com>
> > Date: Thu, 2 Dec 2021 09:55:10 +0100
> > Subject: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs
> > 
> > Signed-off-by: Mathieu Peyrega <mathieu.peyrega@gmail.com>
> > ---
> >  drivers/pps/clients/pps-ldisc.c  | 137
> > ++++++++++++++++++++++++++++++-
> 
> The current code is only 141 lines long, you doubled it, so I think a
> seprate driver would be simpler :)

As stated above : I can retry on this track.

> 
> 
> 
> >  drivers/tty/serial/serial_core.c |  13 +++
> >  include/linux/tty_ldisc.h        |   6 ++
> >  3 files changed, 154 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/pps/clients/pps-ldisc.c
> > b/drivers/pps/clients/pps-ldisc.c
> > index d73c4c2ed4e1..6221700ab3ad 100644
> > --- a/drivers/pps/clients/pps-ldisc.c
> > +++ b/drivers/pps/clients/pps-ldisc.c
> > @@ -8,12 +8,14 @@
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> >  #include <linux/module.h>
> > +#include <linux/moduleparam.h>
> >  #include <linux/serial_core.h>
> >  #include <linux/tty.h>
> >  #include <linux/pps_kernel.h>
> >  #include <linux/bug.h>
> > +#include <linux/string.h>
> >  
> > -static void pps_tty_dcd_change(struct tty_struct *tty, unsigned
> > int status)
> > +static void pps_tty_activepin_change(struct tty_struct *tty,
> > unsigned int status)
> >  {
> >  	struct pps_device *pps;
> >  	struct pps_event_time ts;
> > @@ -36,6 +38,29 @@ static void pps_tty_dcd_change(struct tty_struct
> > *tty, unsigned int status)
> >  			status ? "assert" : "clear", jiffies);
> >  }
> >  
> > +static void pps_tty_activepin_change_invertlogic(struct tty_struct
> > *tty, unsigned int status)
> > +{
> > +	struct pps_device *pps;
> > +	struct pps_event_time ts;
> > +
> > +	pps_get_ts(&ts);
> > +
> > +	pps = pps_lookup_dev(tty);
> > +	/*
> > +	 * This should never fail, but the ldisc locking is very
> > +	 * convoluted, so don't crash just in case.
> > +	 */
> > +	if (WARN_ON_ONCE(pps == NULL))
> > +		return;
> > +
> > +	/* Now do the PPS event report */
> > +	pps_event(pps, &ts, status ? PPS_CAPTURECLEAR :
> > +			PPS_CAPTUREASSERT, NULL);
> > +
> > +	dev_dbg(pps->dev, "PPS %s at %lu\n",
> > +			status ? "assert" : "clear", jiffies);
> > +}
> > +
> >  static int (*alias_n_tty_open)(struct tty_struct *tty);
> >  
> >  static int pps_tty_open(struct tty_struct *tty)
> > @@ -99,10 +124,118 @@ static struct tty_ldisc_ops pps_ldisc_ops;
> >   * Module stuff
> >   */
> >  
> > +/*
> > + * activepin parameter (dcd or cts) to handle the case of
> > + * some SBC computers with incomplete UART wiring (no dcd)
> > + * and still allow using cts as the 1PPS signal input pin
> > + */
> > +
> > +static char *activepin = "dcd";
> > +
> > +/*
> > + * invertlogic parameter (0 or 1) allows handling the case
> > + * of some uarts (e.g. TTL UARTs) were the rising / falling
> > + * edges have inverted logic with respect to conventions of
> > + * the RS232 standard
> > + */
> > +
> > +static bool invertlogic;
> > +
> > +static void pps_ldisc_update_settings(void)
> > +{
> > +	pr_info("%s: activepin=%s invertlogic=%c\n",
> > +		__func__, activepin, invertlogic?'Y':'N');
> > +
> > +	if (strncasecmp(activepin, "dcd", 3) == 0) {
> > +		pps_ldisc_ops.cts_change = 0;
> > +		if (!invertlogic)
> > +			pps_ldisc_ops.dcd_change =
> > pps_tty_activepin_change;
> > +		else
> > +			pps_ldisc_ops.dcd_change =
> > pps_tty_activepin_change_invertlogic;
> > +	} else if (strncasecmp(activepin, "cts", 3) == 0) {
> > +		pps_ldisc_ops.dcd_change = 0;
> > +		if (!invertlogic)
> > +			pps_ldisc_ops.cts_change =
> > pps_tty_activepin_change;
> > +		else
> > +			pps_ldisc_ops.cts_change =
> > pps_tty_activepin_change_invertlogic;
> > +	} else {
> > +		pps_ldisc_ops.cts_change = 0;
> > +		pps_ldisc_ops.dcd_change = 0;
> > +	}
> > +}
> > +
> > +static int pps_set_activepin(const char *val, const struct
> > kernel_param *kp)
> > +{
> > +	int res = 0;
> > +	bool update_setting = false;
> > +	int lenval = strlen(val);
> > +	bool currentvalue_is_dcd = (strncasecmp(activepin, "dcd", 3) ==
> > 0);
> > +
> > +	if ((lenval == 3 && strncasecmp(val, "dcd", lenval) == 0) ||
> > +		(lenval == 4 && strncasecmp(val, "dcd\n", lenval) ==
> > 0)) {
> > +		if (!currentvalue_is_dcd) {
> > +			update_setting = true;
> > +			res = param_set_charp("dcd", kp);
> > +		}
> > +	} else if ((lenval == 3 && strncasecmp(val, "cts", lenval) ==
> > 0) ||
> > +		(lenval == 4 && strncasecmp(val, "cts\n", lenval) ==
> > 0)) {
> > +		if (currentvalue_is_dcd) {
> > +			update_setting = true;
> > +			res = param_set_charp("cts", kp);
> > +		}
> > +	} else
> > +		res = -1;
> > +
> > +	if (res == 0 && update_setting)
> > +		pps_ldisc_update_settings();
> > +	else if (res != 0)
> > +		pr_info("unable to set activepin parameter to %s\n",
> > val);
> > +
> > +	return res;
> > +}
> > +
> > +static const struct kernel_param_ops activepin_ops = {
> > +	.set = &pps_set_activepin,
> > +	.get = &param_get_charp,
> > +};
> > +
> > +module_param_cb(activepin, &activepin_ops, &activepin, 0664);
> > +
> > +MODULE_PARM_DESC(activepin, \
> > +"\n\t\tparameter can be either dcd (default) or cts and defines
> > the\n\
> > +\t\tphysical UART pin which receives the analogic PPS signal.");
> > +
> > +static int pps_set_invertlogic(const char *val, const struct
> > kernel_param *kp)
> > +{
> > +	bool lastvalue = *(bool *)(kp->arg);
> > +	int res = param_set_bool(val, kp);
> > +
> > +	if (res == 0 && lastvalue != invertlogic)
> > +		pps_ldisc_update_settings();
> > +
> > +	return res;
> > +}
> > +
> > +static const struct kernel_param_ops invertlogic_ops = {
> > +	.set = &pps_set_invertlogic,
> > +	.get = &param_get_bool,
> > +};
> > +
> > +module_param_cb(invertlogic, &invertlogic_ops, &invertlogic,
> > 0664);
> > +
> > +MODULE_PARM_DESC(invertlogic, \
> > +"\n\t\tflag can be either N (default) or Y and allows handling
> > some\n\
> > +\t\tUARTs (e.g. TTL UARTs) whose reference edge vs. status
> > logic\n\
> > +\t\t(rising/falling vs. active/inactive) is the opposite of the\n\
> > +\t\tRS232 standard.");
> > +
> >  static int __init pps_tty_init(void)
> >  {
> >  	int err;
> >  
> > +	pr_info("%s: activepin=%s invertlogic=%c\n",
> > +		__func__, activepin, invertlogic?'Y':'N');
> 
> You can remove your debugging code as well.

Ok

> 
> > +
> >  	/* Inherit the N_TTY's ops */
> >  	n_tty_inherit_ops(&pps_ldisc_ops);
> >  
> > @@ -114,7 +247,7 @@ static int __init pps_tty_init(void)
> >  	pps_ldisc_ops.owner = THIS_MODULE;
> >  	pps_ldisc_ops.num = N_PPS;
> >  	pps_ldisc_ops.name = "pps_tty";
> > -	pps_ldisc_ops.dcd_change = pps_tty_dcd_change;
> > +	pps_ldisc_update_settings();
> >  	pps_ldisc_ops.open = pps_tty_open;
> >  	pps_ldisc_ops.close = pps_tty_close;
> >  
> > diff --git a/drivers/tty/serial/serial_core.c
> > b/drivers/tty/serial/serial_core.c
> > index 1e738f265eea..abbab3fb1282 100644
> > --- a/drivers/tty/serial/serial_core.c
> > +++ b/drivers/tty/serial/serial_core.c
> > @@ -3096,8 +3096,21 @@ EXPORT_SYMBOL_GPL(uart_handle_dcd_change);
> >   */
> >  void uart_handle_cts_change(struct uart_port *uport, unsigned int
> > status)
> >  {
> > +	struct tty_port *port = &uport->state->port;
> > +	struct tty_struct *tty = port->tty;
> > +	struct tty_ldisc *ld;
> > +
> >  	lockdep_assert_held_once(&uport->lock);
> >  
> > +	if (tty) {
> > +		ld = tty_ldisc_ref(tty);
> > +		if (ld) {
> > +			if (ld->ops->cts_change)
> > +				ld->ops->cts_change(tty, status);
> > +			tty_ldisc_deref(ld);
> > +		}
> > +	}
> 
> This is a pretty big abuse of CTS.  I understand your want for
> something
> like this, but why not get a device that has a DCD line instead?

Many of the x86 based SBC i've seen (with robotic/mobile devices) use
case have this kind of incomplete UARTs. I'd love if the perfect
hardware choice would be possible.
Also, is there a specific reason that I'm missing why the abuse (agreed
it is) of the primary CTS pin usage is of a different nature than the
abuse of the DCD pin already in place ?
Most GNSS boards I have seen only have RX, TX and don't even expose the
CTS/RTS. They provide the high precision PPS signal on a separate pin,
and being able to reuse the CTS pin from the host computer like a more
generic GPIO to handle would be nice (my opinion).

> 
> That's my biggest objection here, it feels like you are working
> around
> really odd hardware with this change.  What real-world use case is
> this
> for?y

I'm not sure this is such odd hardware. A lot of recent GNSS boards I'm
handling only have RX&TX on their UARTs. This is the case for all Ublox
chips I've seen ; Septentrio new Mosaic chips have CTS/RTS, but flow
control is disabled by default and consumer PCB featuring the chip do
not expose those pins. In most (all those I know) cases the PPS signal
is provided on a separate non UART related pin. And many x86 SBC boards
that could pair (for whatever use case like robotic projects etc.) with
those GNSS boards have incomplete UARTs without DCD (and often no GPIO
that could be use for that purpose either. ARM based devices are often
better equipped from that point of view, but other constraints sometime
do not let you go to ARM hardware).

> 
> You also say you can handle this in userspace already, so why would
> this
> have to be in the kernel?  What benefit is it for us to take this?

I don't know how to accuratly handle CTS handle from user space, only
type of handling in user space I have workaround is the inversion of
rising falling edge (e.g. with time1 offset and flag2 of ntpd PPS
driver : https://doc.ntp.org/archives/drivers/driver22/ ) but this
doesnt pair well with all kind of other drivers that carry the numeric
part of timing information). It's a matter of taste, but I dislike this
workaround even more than the CTS pin abuse.

> 
> thanks,
> 
> greg k-h

regards,

Mathieu Peyrega

Re: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[Greg KH]

On Sat, Dec 04, 2021 at 04:57:56PM +0100, Mathieu Peyrega wrote:
> Le vendredi 03 décembre 2021 à 14:11 +0100, Greg KH a écrit :
> > On Thu, Dec 02, 2021 at 11:56:10AM +0100, Mathieu Peyrega wrote:
> > > Hello,
> > > 
> > > this is my first contribution to this list (and the Linux kernel)
> > > and I'd
> > > like trying to revive a subject already discussed in a 2017/2018
> > > thread about
> > > adding a possibility to use the CTS pin instead of the DCD pin for
> > > 1PPS line
> > > disciplining (cf. 
> > > https://www.spinics.net/lists/linux-serial/msg27604.html)
> > 
> > A few meta-comments about the patch.
> > 
> > Look at how other patches are submitted, there's no need for this
> > type
> > of introduction in the changelog text, and in fact, you have no
> > changelog text in your patch at all.  So the body of this email needs
> > to
> > go into the changelog area, please do so when you resend.
> 
> will do
> 
> > > The rationale is similar to the original poster's one: some TTL
> > > UARTs hardware
> > > implementations have an incomplete wiring and do not expose a DCD
> > > pin (e.g. on
> > > some SBCs). On those platforms only TX, RX, CTS and RTS are
> > > available. In such
> > > cases, being able to use the CTS pin for 1PPS time disciplining is
> > > useful.
> > > 
> > > In addition, to that primary need, I believe there is another
> > > missing feature
> > > in current implementation. Some non-RS232 UARTs (e.g. TTL UARTs
> > > also often
> > > found on SBCs) have an inverted behaviour with respect to RS232
> > > rising edge or
> > > falling edge vs. assert or clear logics. Not taking this inversion
> > > into account
> > > results in a disciplining where the kernel time ends with an offset
> > > from actual
> > > signal time. Offset value is the width/duration of the 1PPS square
> > > pulse signal.
> > > At least this is what I experienced in the testing process on an
> > > Odroid H2 SBC
> > > (Intel x86_64 based) and a GNSS driven 1PPS signal (from a CORS
> > > station that I
> > > manage). Maybe this can be handled from a later userland process
> > > (e.g. ntpd)
> > > but I believe that being able to handle it straight at kernel level
> > > is better.
> > > 
> > > As for the module behaviour control, I went with adding 2
> > > parameters to
> > > pps-ldisc:
> > > 
> > > -	activepin (charp) wich can be dcd (default) or cts and drives
> > > the pin
> > > 	which should be consider to detect the 1PPS signal
> > > -	invertlogic (bool) which can be false (default) or true and
> > > defines if
> > > 	the driver complies with a RS232 type signal where assert is on
> > > the
> > > 	rising edge or inverted as for some TTL UARTs. Default values
> > > match
> > > 	the current behaviour.
> > 
> > New module parameters should never be added, as they only affect the
> > whole system, and not on a per-device basis.  If you have to add new
> > options, please do it some other way.
> 
> I don't fully understand the point. Isn't the existing pps_ldisc module
> already affecting the whole system ? (with it's builtin fixed
> "options"). How different tunable options such as the proposal make
> things fundamentally different ? Still I agree that per device settings
> would be better.

Per device settings are required, this would prevent multiple devices
working in the same system, one using the existing line discipline
functionality, and one with your new changes.

> > Also, by adding new options, that means you are changing the logic
> > here,
> > why not create a new line discipline that acts the way you want it to
> > act?  The existing one is very small, it should not be much to make a
> > new one with just the new functionality, right?
> 
> I can retry on this track. However I believe , it will also need
> support from userland utility (especially ldattach). I don't know if
> this kind of consequences is in the scope of the discussion here.

Why would userspace need to be modified?

Try this as a new line discipline, should be much easier and simpler
overall for everyone.

thanks,

greg k-h

Re: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[Mathieu Peyrega]

Le lundi 20 décembre 2021 à 16:43 +0100, Greg KH a écrit :
> On Sat, Dec 04, 2021 at 04:57:56PM +0100, Mathieu Peyrega wrote:
> > Le vendredi 03 décembre 2021 à 14:11 +0100, Greg KH a écrit :
> > > On Thu, Dec 02, 2021 at 11:56:10AM +0100, Mathieu Peyrega wrote:
> > > > 
> I don't fully understand the point. Isn't the existing pps_ldisc
> > module
> > already affecting the whole system ? (with it's builtin fixed
> > "options"). How different tunable options such as the proposal make
> > things fundamentally different ? Still I agree that per device
> > settings
> > would be better.
> 
> Per device settings are required, this would prevent multiple devices
> working in the same system, one using the existing line discipline
> functionality, and one with your new changes.

Is this per device settings requierement valid also for a new line
discipline module or is it acceptable if a new "module level settable"
linee discipline module has also a global behaviour (as current one)
?If per tty device setting is requiered pointers/doc on possible
exemples/mecanismes to achieve this are welcome.

> > I can retry on this track. However I believe , it will also need
> > support from userland utility (especially ldattach). I don't know
> > if
> > this kind of consequences is in the scope of the discussion here.
> 
> Why would userspace need to be modified?

You're right, I initialy thought that ldattach support would be
requiered
(
https://github.com/util-linux/util-linux/blob/master/sys-utils/ldattach.c
as for the drivers list) but after first tests on a separate module,
it's still possible to use ldattach as is with the new line discipine
index. Only support for using the module name as argument is
requierement which is not blocking.


> 
> Try this as a new line discipline, should be much easier and simpler
> overall for everyone.

I have been working a little on this already, was waiting for list
comments/answers to proceed further.
Is there a prefered name as the module is mostly a clone of the pps-
ldisc current one one with first patch changes ? I went for ppsex-ldisc 
for now.
Is it Ok to share & modify the C structures of pps-ldisc (especially
tty_ldisc_ops as per initially proposed patch) or should a full new set
also be added ?

Regards,
Mathieu Peyrega

Re: [PATCH] Allow PPS on CTS pin and non-RS232 UARTs

[Greg KH]

On Tue, Dec 21, 2021 at 10:36:39AM +0100, Mathieu Peyrega wrote:
> Le lundi 20 décembre 2021 à 16:43 +0100, Greg KH a écrit :
> > On Sat, Dec 04, 2021 at 04:57:56PM +0100, Mathieu Peyrega wrote:
> > > Le vendredi 03 décembre 2021 à 14:11 +0100, Greg KH a écrit :
> > > > On Thu, Dec 02, 2021 at 11:56:10AM +0100, Mathieu Peyrega wrote:
> > > > > 
> > I don't fully understand the point. Isn't the existing pps_ldisc
> > > module
> > > already affecting the whole system ? (with it's builtin fixed
> > > "options"). How different tunable options such as the proposal make
> > > things fundamentally different ? Still I agree that per device
> > > settings
> > > would be better.
> > 
> > Per device settings are required, this would prevent multiple devices
> > working in the same system, one using the existing line discipline
> > functionality, and one with your new changes.
> 
> Is this per device settings requierement valid also for a new line
> discipline module or is it acceptable if a new "module level settable"
> linee discipline module has also a global behaviour (as current one)
> ?If per tty device setting is requiered pointers/doc on possible
> exemples/mecanismes to achieve this are welcome.

If you make this a new line discipline, that is fine, as you can set
each tty device to have their own discipline.

But do not have options for that line discipline, on a module-basis,
that way will not work (think about multiple devices all using the same
line discipline.)

> > Try this as a new line discipline, should be much easier and simpler
> > overall for everyone.
> 
> I have been working a little on this already, was waiting for list
> comments/answers to proceed further.
> Is there a prefered name as the module is mostly a clone of the pps-
> ldisc current one one with first patch changes ? I went for ppsex-ldisc 
> for now.
> Is it Ok to share & modify the C structures of pps-ldisc (especially
> tty_ldisc_ops as per initially proposed patch) or should a full new set
> also be added ?

I do not know, let's see what you think would work and post that and we
can go from there.

thanks,

greg k-h