[PATCH v4] pps: Add elapsed realtime timestamping

[PATCH v4] pps: Add elapsed realtime timestamping

[Alexander Komrakov]

[-- Attachment #1: Type: text/plain, Size: 8356 bytes --]

Some applications like Android needs elapsed realtime timestamping
to PPS pulse for its clock management. Add sysfs node for this.

Signed-off-by: Alexander Komrakov <alexander.komrakov@broadcom.com>
---
Changes in v4:
  - Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()
  - clock_gettime removed, trailing whitespaces removed, <tab> added, spaces removed
  - Information Real-time assert event added to Documentation/driver-api/pps.rst

 Documentation/ABI/testing/sysfs-pps | 27 +++++++++++++++++++++++
 Documentation/driver-api/pps.rst    |  8 +++++++
 drivers/pps/Makefile                |  4 ++++
 drivers/pps/kapi.c                  | 23 ++++++++++++++++---
 drivers/pps/sysfs.c                 | 34 +++++++++++++++++++++++++++++
 include/linux/pps_kernel.h          |  2 ++
 6 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-pps b/Documentation/ABI/testing/sysfs-pps
index 25028c7bc37d..031ec89e1ed6 100644
--- a/Documentation/ABI/testing/sysfs-pps
+++ b/Documentation/ABI/testing/sysfs-pps
@@ -1,3 +1,30 @@
+What:		/sys/class/pps/pps0/assert_elapsed
+Date:		October 2021
+Contact:	Alexander Komrakov <alexander.komrakov@broadcom.com>
+Description:
+		The /sys/class/pps/ppsX/assert_elapsed file reports the
+		elapsed real-time assert events and the elapsed
+		real-time assert sequence number of the X-th source
+		in the form:
+
+			<secs>.<nsec>#<sequence>
+
+		If the source has no elapsed real-time assert events
+		the content of this file is empty.
+
+What:		/sys/class/pps/ppsX/clear_elapsed
+Date:		October 2021
+Contact:	Alexander Komrakov <alexander.komrakov@broadcom.com>
+Description:
+		The /sys/class/pps/ppsX/clear_elapsed file reports the elapsed
+		real-time clear events and the elapsed real-time clear
+		sequence number of the X-th source in the form:
+
+			<secs>.<nsec>#<sequence>
+
+		If the source has no elapsed real-time clear events the
+		content of this file is empty.
+
 What:		/sys/class/pps/
 Date:		February 2008
 Contact:	Rodolfo Giometti <giometti@linux.it>
diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
index 2d6b99766ee8..80bd7bf048e8 100644
--- a/Documentation/driver-api/pps.rst
+++ b/Documentation/driver-api/pps.rst
@@ -167,6 +167,14 @@ sequence number. Other files are:
  * path: reports the PPS source's device path, that is the device the
    PPS source is connected to (if it exists).
 
+Real-time assert event::
+
+   Calculate the monotonic clock from the timespec clock to generate PPS elapsed real-time event value and store the result into /sys/class/pps/pps0/assert_elapsed.
+   Because we have requirements to make sure the delta between standard time, say the GPS Time, and elapsedRealtime < 1 millisecond,
+   regular linux clock timestamp is not enough for our use case.
+   The pin PPS will generate elapsedRealtime event at 1 sec boundary which is an exact value of the monotonic clock from the kernel PPS driver
+   /sys/class/pps/pps0/assert_elapsed.
+
 
 Testing the PPS support
 -----------------------
diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
index ceaf65cc1f1d..443501310445 100644
--- a/drivers/pps/Makefile
+++ b/drivers/pps/Makefile
@@ -8,4 +8,8 @@ pps_core-$(CONFIG_NTP_PPS)	+= kc.o
 obj-$(CONFIG_PPS)		:= pps_core.o
 obj-y				+= clients/ generators/
 
+ifeq ($(CONFIG_ELAPSED_REALTIME_PPS),y)
+EXTRA_CFLAGS += -DENABLE_ELAPSED_REALTIME_PPS
+endif
+
 ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index d9d566f70ed1..8a97f4cae9e1 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -162,11 +162,20 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 	unsigned long flags;
 	int captured = 0;
 	struct pps_ktime ts_real = { .sec = 0, .nsec = 0, .flags = 0 };
+	struct pps_ktime ts_real_elapsed = { .sec = 0, .nsec = 0, .flags = 0 };
+	struct timespec64 ts64 = { .tv_sec = 0, .tv_nsec = 0 };
 
 	/* check event type */
 	BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
+	/* Calculate the monotonic clock from the timespec clock and stores the result in pps_ktime format
+	   ktime_get_boottime_ts64() : because elapsed realtime includes time spent in sleep */
+	ktime_get_boottime_ts64(&ts64);
+	timespec_to_pps_ktime(&ts_real_elapsed,ts64);
 
-	dev_dbg(pps->dev, "PPS event at %lld.%09ld\n",
+	dev_dbg(pps->dev, "PPS event (monotonic) at %lld.%09d\n",
+			(s64)ts_real_elapsed.sec, ts_real_elapsed.nsec);
+
+	dev_dbg(pps->dev, "PPS event (timestamp) at %lld.%09ld\n",
 			(s64)ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
 
 	timespec_to_pps_ktime(&ts_real, ts->ts_real);
@@ -181,11 +190,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 	pps->current_mode = pps->params.mode;
 	if (event & pps->params.mode & PPS_CAPTUREASSERT) {
 		/* We have to add an offset? */
-		if (pps->params.mode & PPS_OFFSETASSERT)
+		if (pps->params.mode & PPS_OFFSETASSERT) {
+			pps_add_offset(&ts_real_elapsed,
+					&pps->params.assert_off_tu);
 			pps_add_offset(&ts_real,
 					&pps->params.assert_off_tu);
+		}
 
 		/* Save the time stamp */
+		pps->assert_elapsed_tu = ts_real_elapsed;
 		pps->assert_tu = ts_real;
 		pps->assert_sequence++;
 		dev_dbg(pps->dev, "capture assert seq #%u\n",
@@ -195,11 +208,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
 	}
 	if (event & pps->params.mode & PPS_CAPTURECLEAR) {
 		/* We have to add an offset? */
-		if (pps->params.mode & PPS_OFFSETCLEAR)
+		if (pps->params.mode & PPS_OFFSETCLEAR)	{
+			pps_add_offset(&ts_real_elapsed,
+					&pps->params.clear_off_tu);
 			pps_add_offset(&ts_real,
 					&pps->params.clear_off_tu);
+		}
 
 		/* Save the time stamp */
+		pps->clear_elapsed_tu = ts_real_elapsed;
 		pps->clear_tu = ts_real;
 		pps->clear_sequence++;
 		dev_dbg(pps->dev, "capture clear seq #%u\n",
diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
index 134bc33f6ad0..24f505cd7233 100644
--- a/drivers/pps/sysfs.c
+++ b/drivers/pps/sysfs.c
@@ -10,6 +10,7 @@
 #include <linux/module.h>
 #include <linux/string.h>
 #include <linux/pps_kernel.h>
+#include <linux/sysfs.h>
 
 /*
  * Attribute functions
@@ -29,6 +30,21 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(assert);
 
+static ssize_t assert_elapsed_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct pps_device *pps = dev_get_drvdata(dev);
+
+	if (!(pps->info.mode & PPS_CAPTUREASSERT))
+		return 0;
+
+	return sprintf(buf, "%lld.%09d#%d\n",
+			   (long long) pps->assert_elapsed_tu.sec,
+			   pps->assert_elapsed_tu.nsec,
+			   pps->assert_sequence);
+}
+static DEVICE_ATTR_RO(assert_elapsed);
+
 static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
 			  char *buf)
 {
@@ -43,6 +59,22 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(clear);
 
+static ssize_t clear_elapsed_show(struct device *dev,
+			  struct device_attribute *attr,
+			  char *buf)
+{
+	struct pps_device *pps = dev_get_drvdata(dev);
+
+	if (!(pps->info.mode & PPS_CAPTURECLEAR))
+		return 0;
+
+	return sprintf(buf, "%lld.%09d#%d\n",
+			(long long) pps->clear_elapsed_tu.sec,
+			pps->clear_elapsed_tu.nsec,
+			pps->clear_sequence);
+}
+static DEVICE_ATTR_RO(clear_elapsed);
+
 static ssize_t mode_show(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
@@ -82,6 +114,8 @@ static DEVICE_ATTR_RO(path);
 static struct attribute *pps_attrs[] = {
 	&dev_attr_assert.attr,
 	&dev_attr_clear.attr,
+	&dev_attr_assert_elapsed.attr,
+	&dev_attr_clear_elapsed.attr,
 	&dev_attr_mode.attr,
 	&dev_attr_echo.attr,
 	&dev_attr_name.attr,
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 78c8ac4951b5..807642da12f7 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -47,6 +47,8 @@ struct pps_device {
 
 	__u32 assert_sequence;			/* PPS assert event seq # */
 	__u32 clear_sequence;			/* PPS clear event seq # */
+	struct pps_ktime assert_elapsed_tu;	/* PPS elapsed rt assert seq # */
+	struct pps_ktime clear_elapsed_tu;	/* PPS elapsed rt clear event seq */
 	struct pps_ktime assert_tu;
 	struct pps_ktime clear_tu;
 	int current_mode;			/* PPS mode at event time */
-- 
2.25.1


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4222 bytes --]

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Greg KH]

On Fri, Mar 17, 2023 at 12:47:39AM -0700, Alexander Komrakov wrote:
> Some applications like Android needs elapsed realtime timestamping
> to PPS pulse for its clock management. Add sysfs node for this.
> 
> Signed-off-by: Alexander Komrakov <alexander.komrakov@broadcom.com>
> ---
> Changes in v4:
>   - Remove "staging",wrap changelog text at 72 columns and remove sysfs_emit()

Why remove sysfs_emit()?  That's the correct thing to use, please use
it.

>   - clock_gettime removed, trailing whitespaces removed, <tab> added, spaces removed
>   - Information Real-time assert event added to Documentation/driver-api/pps.rst
> 
>  Documentation/ABI/testing/sysfs-pps | 27 +++++++++++++++++++++++
>  Documentation/driver-api/pps.rst    |  8 +++++++
>  drivers/pps/Makefile                |  4 ++++
>  drivers/pps/kapi.c                  | 23 ++++++++++++++++---
>  drivers/pps/sysfs.c                 | 34 +++++++++++++++++++++++++++++
>  include/linux/pps_kernel.h          |  2 ++
>  6 files changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-pps b/Documentation/ABI/testing/sysfs-pps
> index 25028c7bc37d..031ec89e1ed6 100644
> --- a/Documentation/ABI/testing/sysfs-pps
> +++ b/Documentation/ABI/testing/sysfs-pps
> @@ -1,3 +1,30 @@
> +What:		/sys/class/pps/pps0/assert_elapsed
> +Date:		October 2021
> +Contact:	Alexander Komrakov <alexander.komrakov@broadcom.com>
> +Description:
> +		The /sys/class/pps/ppsX/assert_elapsed file reports the
> +		elapsed real-time assert events and the elapsed
> +		real-time assert sequence number of the X-th source
> +		in the form:
> +
> +			<secs>.<nsec>#<sequence>
> +
> +		If the source has no elapsed real-time assert events
> +		the content of this file is empty.
> +
> +What:		/sys/class/pps/ppsX/clear_elapsed
> +Date:		October 2021
> +Contact:	Alexander Komrakov <alexander.komrakov@broadcom.com>
> +Description:
> +		The /sys/class/pps/ppsX/clear_elapsed file reports the elapsed
> +		real-time clear events and the elapsed real-time clear
> +		sequence number of the X-th source in the form:
> +
> +			<secs>.<nsec>#<sequence>
> +
> +		If the source has no elapsed real-time clear events the
> +		content of this file is empty.
> +
>  What:		/sys/class/pps/
>  Date:		February 2008
>  Contact:	Rodolfo Giometti <giometti@linux.it>
> diff --git a/Documentation/driver-api/pps.rst b/Documentation/driver-api/pps.rst
> index 2d6b99766ee8..80bd7bf048e8 100644
> --- a/Documentation/driver-api/pps.rst
> +++ b/Documentation/driver-api/pps.rst
> @@ -167,6 +167,14 @@ sequence number. Other files are:
>   * path: reports the PPS source's device path, that is the device the
>     PPS source is connected to (if it exists).
>  
> +Real-time assert event::
> +
> +   Calculate the monotonic clock from the timespec clock to generate PPS elapsed real-time event value and store the result into /sys/class/pps/pps0/assert_elapsed.
> +   Because we have requirements to make sure the delta between standard time, say the GPS Time, and elapsedRealtime < 1 millisecond,
> +   regular linux clock timestamp is not enough for our use case.
> +   The pin PPS will generate elapsedRealtime event at 1 sec boundary which is an exact value of the monotonic clock from the kernel PPS driver
> +   /sys/class/pps/pps0/assert_elapsed.

Please wrap your lines properly :(


> +
>  
>  Testing the PPS support
>  -----------------------
> diff --git a/drivers/pps/Makefile b/drivers/pps/Makefile
> index ceaf65cc1f1d..443501310445 100644
> --- a/drivers/pps/Makefile
> +++ b/drivers/pps/Makefile
> @@ -8,4 +8,8 @@ pps_core-$(CONFIG_NTP_PPS)	+= kc.o
>  obj-$(CONFIG_PPS)		:= pps_core.o
>  obj-y				+= clients/ generators/
>  
> +ifeq ($(CONFIG_ELAPSED_REALTIME_PPS),y)
> +EXTRA_CFLAGS += -DENABLE_ELAPSED_REALTIME_PPS
> +endif

Why do you need a new CFLAG?  Why not just look at the CONFIG entry
instead?  And your .c code does not use this new CFLAG at all, so why is
it even here?


> +
>  ccflags-$(CONFIG_PPS_DEBUG) := -DDEBUG
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index d9d566f70ed1..8a97f4cae9e1 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -162,11 +162,20 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>  	unsigned long flags;
>  	int captured = 0;
>  	struct pps_ktime ts_real = { .sec = 0, .nsec = 0, .flags = 0 };
> +	struct pps_ktime ts_real_elapsed = { .sec = 0, .nsec = 0, .flags = 0 };
> +	struct timespec64 ts64 = { .tv_sec = 0, .tv_nsec = 0 };
>  
>  	/* check event type */
>  	BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> +	/* Calculate the monotonic clock from the timespec clock and stores the result in pps_ktime format
> +	   ktime_get_boottime_ts64() : because elapsed realtime includes time spent in sleep */

Properly wrap your lines.

Also a blenk line after the BUG_ON()?


> +	ktime_get_boottime_ts64(&ts64);
> +	timespec_to_pps_ktime(&ts_real_elapsed,ts64);
>  
> -	dev_dbg(pps->dev, "PPS event at %lld.%09ld\n",
> +	dev_dbg(pps->dev, "PPS event (monotonic) at %lld.%09d\n",
> +			(s64)ts_real_elapsed.sec, ts_real_elapsed.nsec);
> +
> +	dev_dbg(pps->dev, "PPS event (timestamp) at %lld.%09ld\n",
>  			(s64)ts->ts_real.tv_sec, ts->ts_real.tv_nsec);
>  
>  	timespec_to_pps_ktime(&ts_real, ts->ts_real);
> @@ -181,11 +190,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>  	pps->current_mode = pps->params.mode;
>  	if (event & pps->params.mode & PPS_CAPTUREASSERT) {
>  		/* We have to add an offset? */
> -		if (pps->params.mode & PPS_OFFSETASSERT)
> +		if (pps->params.mode & PPS_OFFSETASSERT) {
> +			pps_add_offset(&ts_real_elapsed,
> +					&pps->params.assert_off_tu);
>  			pps_add_offset(&ts_real,
>  					&pps->params.assert_off_tu);
> +		}
>  
>  		/* Save the time stamp */
> +		pps->assert_elapsed_tu = ts_real_elapsed;
>  		pps->assert_tu = ts_real;
>  		pps->assert_sequence++;
>  		dev_dbg(pps->dev, "capture assert seq #%u\n",
> @@ -195,11 +208,15 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,
>  	}
>  	if (event & pps->params.mode & PPS_CAPTURECLEAR) {
>  		/* We have to add an offset? */
> -		if (pps->params.mode & PPS_OFFSETCLEAR)
> +		if (pps->params.mode & PPS_OFFSETCLEAR)	{
> +			pps_add_offset(&ts_real_elapsed,
> +					&pps->params.clear_off_tu);
>  			pps_add_offset(&ts_real,
>  					&pps->params.clear_off_tu);
> +		}
>  
>  		/* Save the time stamp */
> +		pps->clear_elapsed_tu = ts_real_elapsed;
>  		pps->clear_tu = ts_real;
>  		pps->clear_sequence++;
>  		dev_dbg(pps->dev, "capture clear seq #%u\n",
> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
> index 134bc33f6ad0..24f505cd7233 100644
> --- a/drivers/pps/sysfs.c
> +++ b/drivers/pps/sysfs.c
> @@ -10,6 +10,7 @@
>  #include <linux/module.h>
>  #include <linux/string.h>
>  #include <linux/pps_kernel.h>
> +#include <linux/sysfs.h>

Why is this .h needed now and not previously?

>  
>  /*
>   * Attribute functions
> @@ -29,6 +30,21 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(assert);
>  
> +static ssize_t assert_elapsed_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct pps_device *pps = dev_get_drvdata(dev);
> +
> +	if (!(pps->info.mode & PPS_CAPTUREASSERT))
> +		return 0;

Why are you not returning an error?

> +
> +	return sprintf(buf, "%lld.%09d#%d\n",
> +			   (long long) pps->assert_elapsed_tu.sec,
> +			   pps->assert_elapsed_tu.nsec,
> +			   pps->assert_sequence);

sysfs_emit() please.

> +}
> +static DEVICE_ATTR_RO(assert_elapsed);
> +
>  static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
>  			  char *buf)
>  {
> @@ -43,6 +59,22 @@ static ssize_t clear_show(struct device *dev, struct device_attribute *attr,
>  }
>  static DEVICE_ATTR_RO(clear);
>  
> +static ssize_t clear_elapsed_show(struct device *dev,
> +			  struct device_attribute *attr,
> +			  char *buf)
> +{
> +	struct pps_device *pps = dev_get_drvdata(dev);
> +
> +	if (!(pps->info.mode & PPS_CAPTURECLEAR))
> +		return 0;

Again, why not an error?

And why are these sysfs files even present if the mode is not set
properly?  Can the mode be set while the device is attached or is this
only defined at probe time?  If at probe time, just never create these
files.

> +
> +	return sprintf(buf, "%lld.%09d#%d\n",
> +			(long long) pps->clear_elapsed_tu.sec,
> +			pps->clear_elapsed_tu.nsec,
> +			pps->clear_sequence);

Again, sysfs_emit() please.

thanks,

greg k-h

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Greg KH]

On Fri, Mar 17, 2023 at 02:51:50AM -0700, Alex Komrakov wrote:

<snip>

Please fix up your email client as it is sending html email which is
rejected by the mailing list and can not properly quote emails to engage
in proper discussion of changes.  The kernel documentation has some
hints for how to do this.

thanks,

greg k-h

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Rodolfo Giometti]

On 17/03/23 10:51, Alex Komrakov wrote:
>> +     if (!(pps->info.mode & PPS_CAPTURECLEAR))
>> +             return 0;   Why are you not returning an error?
> [AK] I used the style in this file sysfs.c.
>   assert_show() and clear_show()  have the same condition.
> When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted  and it is not error
> Probably Rodolfo can get more info why return 0

It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or 
PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.

> And why are these sysfs files even present if the mode is not set
> properly?  Can the mode be set while the device is attached or is this
> only defined at probe time?  If at probe time, just never create these
> files.
> [AK] we can understand mode is set when interrupts asserted and  
> file assert_elapsed will be updated.

PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Greg KH]

On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
> On 17/03/23 10:51, Alex Komrakov wrote:
> > > +     if (!(pps->info.mode & PPS_CAPTURECLEAR))
> > > +             return 0;   Why are you not returning an error?
> > [AK] I used the style in this file sysfs.c.
> >   assert_show() and clear_show()  have the same condition.
> > When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted  and it is not error
> > Probably Rodolfo can get more info why return 0
> 
> It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
> PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.

But shouldn't you return an error instead of an empty string?

> > And why are these sysfs files even present if the mode is not set
> > properly?  Can the mode be set while the device is attached or is this
> > only defined at probe time?  If at probe time, just never create these
> > files.
> > [AK] we can understand mode is set when interrupts asserted and
> > file assert_elapsed will be updated.
> 
> PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.

Ok, that's good to know.  But I think the error return value is a better
indication that something went wrong here and this attribute does not
work for this device at this point in time.

thanks,

greg k-h

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Rodolfo Giometti]

On 17/03/23 15:22, Greg KH wrote:
> On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
>> On 17/03/23 10:51, Alex Komrakov wrote:
>>>> +     if (!(pps->info.mode & PPS_CAPTURECLEAR))
>>>> +             return 0;   Why are you not returning an error?
>>> [AK] I used the style in this file sysfs.c.
>>>    assert_show() and clear_show()  have the same condition.
>>> When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted  and it is not error
>>> Probably Rodolfo can get more info why return 0
>>
>> It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
>> PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
> 
> But shouldn't you return an error instead of an empty string?

This is not an error... it's just a disabled capability. :)

>>> And why are these sysfs files even present if the mode is not set
>>> properly?  Can the mode be set while the device is attached or is this
>>> only defined at probe time?  If at probe time, just never create these
>>> files.
>>> [AK] we can understand mode is set when interrupts asserted and
>>> file assert_elapsed will be updated.
>>
>> PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
> 
> Ok, that's good to know.  But I think the error return value is a better
> indication that something went wrong here and this attribute does not
> work for this device at this point in time.

I see... however I suppose several code relays on this behavior.

If we decide to change it, which should be the better way to do it? Any 
suggestions are appreciated.

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Greg KH]

On Fri, Mar 17, 2023 at 05:34:28PM +0100, Rodolfo Giometti wrote:
> On 17/03/23 15:22, Greg KH wrote:
> > On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
> > > On 17/03/23 10:51, Alex Komrakov wrote:
> > > > > +     if (!(pps->info.mode & PPS_CAPTURECLEAR))
> > > > > +             return 0;   Why are you not returning an error?
> > > > [AK] I used the style in this file sysfs.c.
> > > >    assert_show() and clear_show()  have the same condition.
> > > > When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted  and it is not error
> > > > Probably Rodolfo can get more info why return 0
> > > 
> > > It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
> > > PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
> > 
> > But shouldn't you return an error instead of an empty string?
> 
> This is not an error... it's just a disabled capability. :)

Then maybe return "0" or something like that?

> > > > And why are these sysfs files even present if the mode is not set
> > > > properly?  Can the mode be set while the device is attached or is this
> > > > only defined at probe time?  If at probe time, just never create these
> > > > files.
> > > > [AK] we can understand mode is set when interrupts asserted and
> > > > file assert_elapsed will be updated.
> > > 
> > > PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
> > 
> > Ok, that's good to know.  But I think the error return value is a better
> > indication that something went wrong here and this attribute does not
> > work for this device at this point in time.
> 
> I see... however I suppose several code relays on this behavior.

If that's the case, then you are right, you can't change it, and I'll
stop complaining here :)

What tools use these sysfs files?  How did you test your changes?

thanks,

greg k-h

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Greg KH]

On Fri, Mar 17, 2023 at 01:32:16PM -0700, Alex Komrakov wrote:
>     AOSP time tags reported location with elapsed realtime and pps provides
> best accuracy.

<snip>

Again, please do not send HTML and please do not top-post as the mailing
list rejects HTML email and top-posting makes it impossible to have a
technical discussion on your changes.

thanks,

greg k-h

Re: [PATCH v4] pps: Add elapsed realtime timestamping

[Rodolfo Giometti]

On 17/03/23 18:06, Greg KH wrote:
> On Fri, Mar 17, 2023 at 05:34:28PM +0100, Rodolfo Giometti wrote:
>> On 17/03/23 15:22, Greg KH wrote:
>>> On Fri, Mar 17, 2023 at 03:04:31PM +0100, Rodolfo Giometti wrote:
>>>> On 17/03/23 10:51, Alex Komrakov wrote:
>>>>>> +     if (!(pps->info.mode & PPS_CAPTURECLEAR))
>>>>>> +             return 0;   Why are you not returning an error?
>>>>> [AK] I used the style in this file sysfs.c.
>>>>>     assert_show() and clear_show()  have the same condition.
>>>>> When '& PPS_CAPTURECLEAR' -- 0 means no interrupt asserted  and it is not error
>>>>> Probably Rodolfo can get more info why return 0
>>>>
>>>> It's just as Alex said, if the PPS source has no PPS_CAPTUREASSERT or
>>>> PPS_CAPTURECLEAR mode it should not print ASSERT and CLEAR info.
>>>
>>> But shouldn't you return an error instead of an empty string?
>>
>> This is not an error... it's just a disabled capability. :)
> 
> Then maybe return "0" or something like that?

Yes, it could be a valid solution.

>>>>> And why are these sysfs files even present if the mode is not set
>>>>> properly?  Can the mode be set while the device is attached or is this
>>>>> only defined at probe time?  If at probe time, just never create these
>>>>> files.
>>>>> [AK] we can understand mode is set when interrupts asserted and
>>>>> file assert_elapsed will be updated.
>>>>
>>>> PPS source's "mode bits" can be set at runtime via PPS_SETPARAMS.
>>>
>>> Ok, that's good to know.  But I think the error return value is a better
>>> indication that something went wrong here and this attribute does not
>>> work for this device at this point in time.
>>
>> I see... however I suppose several code relays on this behavior.
> 
> If that's the case, then you are right, you can't change it, and I'll
> stop complaining here :)
> 
> What tools use these sysfs files?

Mainly are debugging tools via scripts. Normal usage should be via C API.

> How did you test your changes?

As above, I use scripts whose get access to sysfs to test a PPS source 
functionality. Regarding the C API I use the pps-tools:

https://github.com/redlab-i/pps-tools

Ciao,

Rodolfo

-- 
GNU/Linux Solutions                  e-mail: giometti@enneenne.com
Linux Device Driver                          giometti@linux.it
Embedded Systems                     phone:  +39 349 2432127
UNIX programming                     skype:  rodolfo.giometti