On Tue, Dec 20, 2022 at 09:47:07PM -0800, 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 <[email protected]>
Nit, you should use "A" and "K" here and in your email "From:" line as
you use that in the change below.
> ---
> Changes in v2:
> - Remove "staging",wrap changelog text at 72 columns and use sysfs_emit().
>
> Documentation/ABI/testing/sysfs-pps | 27 +++++++++++++++++++
> drivers/pps/kapi.c | 38 ++++++++++++++++++++++++---
> drivers/pps/sysfs.c | 40 ++++++++++++++++++++++++++---
> include/linux/pps_kernel.h | 2 ++
> 4 files changed, 101 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/ABI/testing/sysfs-pps b/Documentation/ABI/testing/sysfs-pps
> index 25028c7bc37d..e5fd7bc89ea9 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 <[email protected]>
> +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:
Trailing whitespace :(
> +
> + <secs>.<nsec>#<sequence>
> +
> + If the source has no elapsed real-time assert events
> + the content of this file is empty.
The file should not be present if there is no such events, please fix
that up in the code.
> +
> +What: /sys/class/pps/ppsX/clear_elapsed
> +Date: October 2021
> +Contact: Alexander Komrakov <[email protected]>
> +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 <[email protected]>
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index d9d566f70ed1..c4c6e885b768 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -23,6 +23,26 @@
> /*
> * Local functions
> */
> + #define NANOSEC_PER_SEC 1000000000 /* 10^9 */
This should go into a core include file instead.
> +
> +/**
> + * clock_gettime - get the monotonic clock in pps_ktime format
> + * @kt: pointer to the pps_ktime to be set to raw monotonic time
> + *
> + * The function calculates the monotonic clock from the timespec clock and
> + * stores the result in pps_ktime format in the variable pointed to by @kt.
> + *
> + * The function returns the monotonic clock normalized format in nanosec.
> + */
> +static __u64 clock_gettime(struct pps_ktime *kt)
> +{
> + struct timespec64 ts = { .tv_sec = 0, .tv_nsec = 0 };
> +
> + ktime_get_boottime_ts64(&ts);
> + kt->sec = ts.tv_sec;
> + kt->nsec = ts.tv_nsec;
> + return (__u64) ts.tv_sec * NANOSEC_PER_SEC + ts.tv_nsec;
> +}
We don't already have this in a core function?
And why __u64? This does not cross the user/kernel boundry, right?
>
> static void pps_add_offset(struct pps_ktime *ts, struct pps_ktime *offset)
> {
> @@ -162,11 +182,15 @@ 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 };
>
> /* check event type */
> BUG_ON((event & (PPS_CAPTUREASSERT | PPS_CAPTURECLEAR)) == 0);
> + clock_gettime(&ts_real_elapsed);
> + 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 at %lld.%09ld\n",
> + 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 +205,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 +223,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..d905ac78da7e 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>
Trailing whitespace. Please fix your editor to highlight this in
bright-red or something like that.
checkpatch would have also caught all of this, why didn't you run that
on your patch before sending it out?
>
> /*
> * Attribute functions
> @@ -23,12 +24,27 @@ static ssize_t assert_show(struct device *dev, struct device_attribute *attr,
> if (!(pps->info.mode & PPS_CAPTUREASSERT))
> return 0;
>
> - return sprintf(buf, "%lld.%09d#%d\n",
> - (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
> - pps->assert_sequence);
> + return sysfs_emit(buf, "%lld.%09d#%d\n",
> + (long long) pps->assert_tu.sec, pps->assert_tu.nsec,
> + pps->assert_sequence);
Why change this now? It should be a separate patch, right?
> }
> 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 sysfs_emit(buf, "%lld.%09d#%d\n",
> + (long long) pps->assert_elapsed_tu.sec,
No extra ' ' before the variable, right?
> + 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;
No, don't have this file if it's not possible for this to have a valid
value.
> +
> + return sprintf(buf, "%lld.%09d#%d\n",
> + (long long) pps->clear_elapsed_tu.sec,
> + pps->clear_elapsed_tu.nsec,
> + pps->clear_sequence);
No sysfs_emit()? Odd...
thanks,
greg k-h