Add PPS pulse-width support
Subsystems (2)
cover Add PPS pulse-width support
This series of five patches adds pulse-width calculation support to the pps core (patch 1)
It then enables pulse-width calculation for the pps-gpio driver (patches 2 - 5).
*) Pulse-width in measured in nano seconds.
*) Width time can be calculated for both assert-time and reset-time. *) New sysfs were added to expose the pulse-width.
*) Support was added to pps-gpio driver to enable capture-clear based on
device-tree.
*) Enabling pulse-width calculation for pps-gpio driver is done based on
new boolean device-tree properties.
Eliav Farber (5):
pps: add pulse-width calculation in nsec
dt-bindings: pps: pps-gpio: introduce capture-clear property
pps: clients: gpio: add option to set capture-clear from device-tree
dt-bindings: pps: pps-gpio: introduce pulse-width properties
pps: clients: gpio: enable pps pulse-width calculations based on
device-tree
.../devicetree/bindings/pps/pps-gpio.txt | 10 ++++ drivers/pps/clients/pps-gpio.c | 15 ++++++ drivers/pps/kapi.c | 49 +++++++++++++++++++ drivers/pps/pps.c | 9 ++++ drivers/pps/sysfs.c | 30 ++++++++++++ include/linux/pps_kernel.h | 3 ++ include/uapi/linux/pps.h | 19 +++++++ 7 files changed, 135 insertions(+)
Signature · 1 line
2.40.1
PATCH 1/5 pps: add pulse-width calculation in nsec
This change adds PPS pulse-width calculation in nano seconds. Width time can be calculated for both assert time and reset time.
Calculation can be done only if capture ASSERT and capture CLEAR modes are both enabled.
Assert width is calculated as:
clear-time - assert-time
and clear width is calculated as:
assert-time - clear-time
Read-only sysfs were added to get the last pulse-width time and event sequence.
Examples:
* cat /sys/class/pps/pps0/pulse_width_assert
20001450#85
* cat /sys/class/pps/pps1/pulse_width_clear
979893314#16
--- drivers/pps/kapi.c | 49 ++++++++++++++++++++++++++++++++++++++ drivers/pps/pps.c | 9 +++++++ drivers/pps/sysfs.c | 30 +++++++++++++++++++++++ include/linux/pps_kernel.h | 3 +++ include/uapi/linux/pps.h | 19 +++++++++++++++ 5 files changed, 110 insertions(+)
drivers/pps/kapi.c +49 -0
@@ -82,6 +82,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info, goto pps_register_source_exit; } +if ((info->mode &PPS_WIDTHBOTH ) && + ((info->mode &PPS_CAPTUREBOTH ) !=PPS_CAPTUREBOTH )) { +pr_err ("%s: width can't be calculated without both captures (mode = 0x%x)\n" , + info->name, info->mode); + err = -EINVAL ; + goto pps_register_source_exit; + } +/* Allocate memory for the new PPS source struct */ pps =kzalloc (sizeof (struct pps_device ),GFP_KERNEL );if (pps ==NULL ) {
@@ -143,6 +151,39 @@ void pps_unregister_source(struct pps_device *pps) }EXPORT_SYMBOL (pps_unregister_source); +static u64 pps_ktime_sub (struct pps_ktime *ts1,struct pps_ktime *ts2) +{ +if (ts1->sec == ts2->sec) +return (ts1->nsec > ts2->nsec) ? (ts1->nsec - ts2->nsec) : (ts2->nsec - ts1->nsec); + +if (ts1->sec > ts2->sec) +return (ts1->sec - ts2->sec) *NSEC_PER_SEC + ts1->nsec - ts2->nsec; + +return (ts2->sec - ts1->sec) *NSEC_PER_SEC + ts2->nsec - ts1->nsec; +} + +static void pps_calc_clear_width (struct pps_device *pps) +{ +if (pps->clear_sequence ==0 ) +return ; + + pps->clear_width.sequence++; + pps->clear_width.nsec =pps_ktime_sub (&pps->assert_tu, &pps->clear_tu); +dev_dbg (pps->dev,"PPS clear width = %llu#%u\n" , + pps->clear_width.nsec, pps->clear_width.sequence); +} + +static void pps_calc_assert_width (struct pps_device *pps) +{ +if (pps->assert_sequence ==0 ) +return ; + + pps->assert_width.sequence++; + pps->assert_width.nsec =pps_ktime_sub (&pps->clear_tu, &pps->assert_tu); +dev_dbg (pps->dev,"PPS assert width = %llu#%u\n" , + pps->assert_width.nsec, pps->assert_width.sequence); +} +/* pps_event - register a PPS event into the system * @pps: the PPS device * @ts: the event timestamp
@@ -191,6 +232,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,dev_dbg (pps->dev,"capture assert seq #%u\n" , pps->assert_sequence); +/* Calculate clear pulse-width */ +if (pps->params.mode &PPS_WIDTHCLEAR ) +pps_calc_clear_width (pps); + captured = ~0 ; }if (event & pps->params.mode &PPS_CAPTURECLEAR ) {
@@ -205,6 +250,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,dev_dbg (pps->dev,"capture clear seq #%u\n" , pps->clear_sequence); +/* Calculate assert pulse-width */ +if (pps->params.mode &PPS_WIDTHASSERT ) +pps_calc_assert_width (pps); + captured = ~0 ; }
drivers/pps/pps.c +9 -0
@@ -195,6 +195,11 @@ static long pps_cdev_ioctl(struct file *file, fdata.info.clear_tu = pps->clear_tu; fdata.info.current_mode = pps->current_mode; +memcpy (&fdata.info.assert_width, &pps->assert_width, +sizeof (struct pps_kwidth )); +memcpy (&fdata.info.clear_width, &pps->clear_width, +sizeof (struct pps_kwidth )); +spin_unlock_irq (&pps->lock); err =copy_to_user (uarg, &fdata,sizeof (struct pps_fdata ));
@@ -283,6 +288,10 @@ static long pps_cdev_compat_ioctl(struct file *file, sizeof(struct pps_ktime_compat));memcpy (&compat.info.clear_tu, &pps->clear_tu,sizeof (struct pps_ktime_compat )); +memcpy (&compat.info.assert_width, &pps->assert_width, +sizeof (struct pps_kwidth_compat )); +memcpy (&compat.info.clear_width, &pps->clear_width, +sizeof (struct pps_kwidth_compat ));spin_unlock_irq (&pps->lock);
drivers/pps/sysfs.c +30 -0
@@ -79,6 +79,34 @@ static ssize_t path_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (path); +static ssize_t pulse_width_assert_show (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct pps_device *pps =dev_get_drvdata (dev); + +if (!(pps->info.mode &PPS_WIDTHASSERT )) +return 0 ; + +return sprintf (buf,"%llu#%u\n" , + pps->assert_width.nsec, pps->assert_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_assert); + +static ssize_t pulse_width_clear_show (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct pps_device *pps =dev_get_drvdata (dev); + +if (!(pps->info.mode &PPS_WIDTHCLEAR )) +return 0 ; + +return sprintf (buf,"%llu#%u\n" , + pps->clear_width.nsec, pps->clear_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_clear); +static struct attribute *pps_attrs[] = { &dev_attr_assert.attr, &dev_attr_clear.attr,
@@ -86,6 +114,8 @@ static struct attribute *pps_attrs[] = { &dev_attr_echo.attr, &dev_attr_name.attr, &dev_attr_path.attr, + &dev_attr_pulse_width_assert.attr, + &dev_attr_pulse_width_clear.attr, NULL, };
include/linux/pps_kernel.h +3 -0
@@ -51,6 +51,9 @@ struct pps_device {struct pps_ktime clear_tu;int current_mode;/* PPS mode at event time */ +struct pps_kwidth assert_width;/* PPS assert pulse-width time and event seq # */ +struct pps_kwidth clear_width;/* PPS clear pulse-width time and event seq # */ +unsigned int last_ev;/* last PPS event id */ wait_queue_head_t queue;/* PPS event queue */
include/uapi/linux/pps.h +19 -0
@@ -64,12 +64,24 @@ struct pps_ktime_compat { } __attribute__((packed, aligned(4)));#define PPS_TIME_INVALID (1<<0)/* used to specify timeout==NULL */ +struct pps_kwidth { +__u64 nsec; +__u32 sequence; +}; + +struct pps_kwidth_compat { +__u64 nsec; +__u32 sequence; +} __attribute__((packed,aligned (4 ))); +struct pps_kinfo {__u32 assert_sequence;/* seq. num. of assert event */ __u32 clear_sequence;/* seq. num. of clear event */ struct pps_ktime assert_tu;/* time of assert event */ struct pps_ktime clear_tu;/* time of clear event */ int current_mode;/* current mode bits */ +struct pps_kwidth assert_width;/* assert pulse-width time and seq. num. */ +struct pps_kwidth clear_width;/* clear pulse-width time and seq. num. */ };struct pps_kinfo_compat {
@@ -78,6 +90,8 @@ struct pps_kinfo_compat {struct pps_ktime_compat assert_tu;/* time of assert event */ struct pps_ktime_compat clear_tu;/* time of clear event */ int current_mode;/* current mode bits */ +struct pps_kwidth_compat assert_width;/* assert pulse-width time and seq. num. */ +struct pps_kwidth_compat clear_width;/* clear pulse-width time and seq. num. */ };struct pps_kparams {
@@ -96,6 +110,11 @@ struct pps_kparams {#define PPS_CAPTURECLEAR 0x02/* capture clear events */ #define PPS_CAPTUREBOTH 0x03/* capture assert and clear events */ +/* Pulse-width calculation */ +#define PPS_WIDTHASSERT 0x04/* calculate assert width */ +#define PPS_WIDTHCLEAR 0x08/* calculate clear width */ +#define PPS_WIDTHBOTH 0x0c/* calculate assert and clear width */ +#define PPS_OFFSETASSERT 0x10/* apply compensation for assert event */ #define PPS_OFFSETCLEAR 0x20/* apply compensation for clear event */
Signature · 1 line
2.40.1
Rodolfo Giometti
maintainerRe: [PATCH 1/5] pps: add pulse-width calculation in nsec
Quoted 33 lines · Eliav Farber
This change adds PPS pulse-width calculation in nano seconds. Width time can be calculated for both assert time and reset time.
Calculation can be done only if capture ASSERT and capture CLEAR modes are both enabled.
Assert width is calculated as:
clear-time - assert-time
and clear width is calculated as:
assert-time - clear-timeRead-only sysfs were added to get the last pulse-width time and event sequence.
Examples:
* cat/sys/class/pps/pps0/pulse_width_assert20001450#85 * cat /sys/class/pps/pps1/pulse_width_clear 979893314#16Signed-off-by Eliav Farber <[email protected]>--- drivers/pps/kapi.c | 49 ++++++++++++++++++++++++++++++++++++++ drivers/pps/pps.c | 9 +++++++ drivers/pps/sysfs.c | 30 +++++++++++++++++++++++ include/linux/pps_kernel.h | 3 +++ include/uapi/linux/pps.h | 19 +++++++++++++++ 5 files changed, 110 insertions(+)drivers/pps/kapi.c +0
-0
drivers/pps/kapi.c struct pps_device *pps_register_source(struct pps_source_info *info, · line 91
Quoted 10 lines
drivers/pps/kapi.c +7
-0@@ -82,6 +82,14 @@ struct pps_device *pps_register_source(struct pps_source_info *info, goto pps_register_source_exit; } +if ((info->mode &PPS_WIDTHBOTH ) && + ((info->mode &PPS_CAPTUREBOTH ) !=PPS_CAPTUREBOTH )) { +pr_err ("%s: width can't be calculated without both captures (mode = 0x%x)\n" , + info->name, info->mode); + err = -EINVAL ; + goto pps_register_source_exit; + }
See the comment below where you define PPS_WIDTHBOTH.
Quoted 14 lines
drivers/pps/kapi.c +7
-0/* Allocate memory for the new PPS source struct */ pps =kzalloc (sizeof (struct pps_device ),GFP_KERNEL );if (pps ==NULL ) {@@ -143,6 +151,39 @@ void pps_unregister_source(struct pps_device *pps) }EXPORT_SYMBOL (pps_unregister_source); +static u64 pps_ktime_sub (struct pps_ktime *ts1,struct pps_ktime *ts2) +{ +if (ts1->sec == ts2->sec) +return (ts1->nsec > ts2->nsec) ? (ts1->nsec - ts2->nsec) : (ts2->nsec - ts1->nsec); + +if (ts1->sec > ts2->sec) +return (ts1->sec - ts2->sec) *NSEC_PER_SEC + ts1->nsec - ts2->nsec;
drivers/pps/kapi.c void pps_unregister_source(struct pps_device *pps) · line 170
Quoted 10 lines
drivers/pps/kapi.c +10
-0+ +return (ts2->sec - ts1->sec) *NSEC_PER_SEC + ts2->nsec - ts1->nsec; +} + +static void pps_calc_clear_width (struct pps_device *pps) +{ +if (pps->clear_sequence ==0 ) +return ; + + pps->clear_width.sequence++;
I don't understand the meaning of this field... regarding assert and clear it states the n-th sample but in this case...? Why do you need it?
drivers/pps/kapi.c void pps_unregister_source(struct pps_device *pps) · line 181
Quoted 11 lines
drivers/pps/kapi.c +11
-0+ pps->clear_width.nsec =pps_ktime_sub (&pps->assert_tu, &pps->clear_tu); +dev_dbg (pps->dev,"PPS clear width = %llu#%u\n" , + pps->clear_width.nsec, pps->clear_width.sequence); +} + +static void pps_calc_assert_width (struct pps_device *pps) +{ +if (pps->assert_sequence ==0 ) +return ; + + pps->assert_width.sequence++;
Ditto.
Quoted 126 lines
drivers/pps/kapi.c +13
-0+ pps->assert_width.nsec =pps_ktime_sub (&pps->clear_tu, &pps->assert_tu); +dev_dbg (pps->dev,"PPS assert width = %llu#%u\n" , + pps->assert_width.nsec, pps->assert_width.sequence); +} +/* pps_event - register a PPS event into the system * @pps: the PPS device * @ts: the event timestamp @@ -191,6 +232,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,dev_dbg (pps->dev,"capture assert seq #%u\n" , pps->assert_sequence); +/* Calculate clear pulse-width */ +if (pps->params.mode &PPS_WIDTHCLEAR ) +pps_calc_clear_width (pps); + captured = ~0 ; }if (event & pps->params.mode &PPS_CAPTURECLEAR ) {@@ -205,6 +250,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event,dev_dbg (pps->dev,"capture clear seq #%u\n" , pps->clear_sequence); +/* Calculate assert pulse-width */ +if (pps->params.mode &PPS_WIDTHASSERT ) +pps_calc_assert_width (pps); + captured = ~0 ; }drivers/pps/pps.c +9
-0@@ -195,6 +195,11 @@ static long pps_cdev_ioctl(struct file *file, fdata.info.clear_tu = pps->clear_tu; fdata.info.current_mode = pps->current_mode; +memcpy (&fdata.info.assert_width, &pps->assert_width, +sizeof (struct pps_kwidth )); +memcpy (&fdata.info.clear_width, &pps->clear_width, +sizeof (struct pps_kwidth )); +spin_unlock_irq (&pps->lock); err =copy_to_user (uarg, &fdata,sizeof (struct pps_fdata ));@@ -283,6 +288,10 @@ static long pps_cdev_compat_ioctl(struct file *file, sizeof(struct pps_ktime_compat));memcpy (&compat.info.clear_tu, &pps->clear_tu,sizeof (struct pps_ktime_compat )); +memcpy (&compat.info.assert_width, &pps->assert_width, +sizeof (struct pps_kwidth_compat )); +memcpy (&compat.info.clear_width, &pps->clear_width, +sizeof (struct pps_kwidth_compat ));spin_unlock_irq (&pps->lock);drivers/pps/sysfs.c +30
-0@@ -79,6 +79,34 @@ static ssize_t path_show(struct device *dev, struct device_attribute *attr, }static DEVICE_ATTR_RO (path); +static ssize_t pulse_width_assert_show (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct pps_device *pps =dev_get_drvdata (dev); + +if (!(pps->info.mode &PPS_WIDTHASSERT )) +return 0 ; + +return sprintf (buf,"%llu#%u\n" , + pps->assert_width.nsec, pps->assert_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_assert); + +static ssize_t pulse_width_clear_show (struct device *dev, +struct device_attribute *attr, +char *buf) +{ +struct pps_device *pps =dev_get_drvdata (dev); + +if (!(pps->info.mode &PPS_WIDTHCLEAR )) +return 0 ; + +return sprintf (buf,"%llu#%u\n" , + pps->clear_width.nsec, pps->clear_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_clear); +static struct attribute *pps_attrs[] = { &dev_attr_assert.attr, &dev_attr_clear.attr,@@ -86,6 +114,8 @@ static struct attribute *pps_attrs[] = { &dev_attr_echo.attr, &dev_attr_name.attr, &dev_attr_path.attr, + &dev_attr_pulse_width_assert.attr, + &dev_attr_pulse_width_clear.attr, NULL, };include/linux/pps_kernel.h +3
-0@@ -51,6 +51,9 @@ struct pps_device {struct pps_ktime clear_tu;int current_mode;/* PPS mode at event time */ +struct pps_kwidth assert_width;/* PPS assert pulse-width time and event seq # */ +struct pps_kwidth clear_width;/* PPS clear pulse-width time and event seq # */ +unsigned int last_ev;/* last PPS event id */ wait_queue_head_t queue;/* PPS event queue */ include/uapi/linux/pps.h +0
-0@@ -64,12 +64,24 @@ struct pps_ktime_compat { } __attribute__((packed, aligned(4)));#define PPS_TIME_INVALID (1<<0)/* used to specify timeout==NULL */
include/uapi/linux/pps.h struct pps_ktime_compat { · line 75
Quoted 10 lines
include/uapi/linux/pps.h +9
-0+struct pps_kwidth { +__u64 nsec; +__u32 sequence; +}; + +struct pps_kwidth_compat { +__u64 nsec; +__u32 sequence; +} __attribute__((packed,aligned (4 )));
Why do you need a new type? Since both assert_width and clear_width are time quantities as far as assert_tu and clear_tu, they can be of the same type, can't they? Or, at least they can simply be __u64 since having an assert_width or clear_width longer than 1 second is a non-sense...
include/uapi/linux/pps.h struct pps_ktime_compat { · line 85
Quoted 9 lines
include/uapi/linux/pps.h +2
-0struct pps_kinfo {__u32 assert_sequence;/* seq. num. of assert event */ __u32 clear_sequence;/* seq. num. of clear event */ struct pps_ktime assert_tu;/* time of assert event */ struct pps_ktime clear_tu;/* time of clear event */ int current_mode;/* current mode bits */ +struct pps_kwidth assert_width;/* assert pulse-width time and seq. num. */ +struct pps_kwidth clear_width;/* clear pulse-width time and seq. num. */ };
Altering this structure may break userspace code... also rfc2783 at section-3.2 states that:
The API defines these new data structures:
typedef struct {
pps_seq_t assert_sequence; /* assert event seq # */
pps_seq_t clear_sequence; /* clear event seq # */
pps_timeu_t assert_tu;
pps_timeu_t clear_tu;
int current_mode; /* current mode bits */
} pps_info_t;So, I'm not willing to change this structure just to add this new data that I don't even know where it's used...
If you just read these information via sysfs, please drop these part.
Quoted 11 lines
include/uapi/linux/pps.h +2
-0struct pps_kinfo_compat {@@ -78,6 +90,8 @@ struct pps_kinfo_compat {struct pps_ktime_compat assert_tu;/* time of assert event */ struct pps_ktime_compat clear_tu;/* time of clear event */ int current_mode;/* current mode bits */ +struct pps_kwidth_compat assert_width;/* assert pulse-width time and seq. num. */ +struct pps_kwidth_compat clear_width;/* clear pulse-width time and seq. num. */ };struct pps_kparams {
include/uapi/linux/pps.h struct pps_kparams { · line 117
Quoted 8 lines
include/uapi/linux/pps.h +5
-0@@ -96,6 +110,11 @@ struct pps_kparams {#define PPS_CAPTURECLEAR 0x02/* capture clear events */ #define PPS_CAPTUREBOTH 0x03/* capture assert and clear events */ +/* Pulse-width calculation */ +#define PPS_WIDTHASSERT 0x04/* calculate assert width */ +#define PPS_WIDTHCLEAR 0x08/* calculate clear width */ +#define PPS_WIDTHBOTH 0x0c/* calculate assert and clear width */ +
I don't understand why a process should ask for just PPS_WIDTHASSERT or PPS_WIDTHCLEAR... I think you can avoid defining these values and just enabling pulse width calculation when both assert and clear events are available.
include/uapi/linux/pps.h struct pps_kparams { · line 119
Quoted 2 lines
include/uapi/linux/pps.h +0
-0#define PPS_OFFSETASSERT 0x10/* apply compensation for assert event */ #define PPS_OFFSETCLEAR 0x20/* apply compensation for clear event */
However, the real point is: since an userpsace program can retrieve the time of assert and clear events, why it cannot compute the pulses width by itself? :)
Ciao,
Rodolfo
Signature · 4 lines
GNU/Linux Solutions e-mail: [email protected] Linux Device Driver [email protected] Embedded Systems phone: +39 349 2432127 UNIX programming skype: rodolfo.giometti
Re: [PATCH 1/5] pps: add pulse-width calculation in nsec
Quoted 71 lines · Rodolfo Giometti
Quoted 35 lines · Eliav Farber
This change adds PPS pulse-width calculation in nano seconds. Width time can be calculated for both assert time and reset time.
Calculation can be done only if capture ASSERT and capture CLEAR modes are both enabled.
Assert width is calculated as:
clear-time - assert-time
and clear width is calculated as:
assert-time - clear-timeRead-only sysfs were added to get the last pulse-width time and event sequence.
Examples:
* cat/sys/class/pps/pps0/pulse_width_assert
20001450#85
* cat/sys/class/pps/pps1/pulse_width_clear
979893314#16Signed-off-by Eliav Farber <[email protected]>---
drivers/pps/kapi.c | 49 ++++++++++++++++++++++++++++++++++++++ drivers/pps/pps.c | 9 +++++++ drivers/pps/sysfs.c | 30 +++++++++++++++++++++++include/linux/pps_kernel.h| 3 +++ include/uapi/linux/pps.h | 19 +++++++++++++++5 files changed, 110 insertions(+)drivers/pps/kapi.c +0
-0@@ -82,6 +82,14 @@ struct pps_device *pps_register_source(structpps_source_info *info, goto pps_register_source_exit;drivers/pps/kapi.c struct pps_device *pps_register_source(struct · line 93
Quoted 10 lines
drivers/pps/kapi.c +7
-0} + if ((info->mode & PPS_WIDTHBOTH) && + ((info->mode & PPS_CAPTUREBOTH) != PPS_CAPTUREBOTH)) { + pr_err("%s: width can't be calculated without both captures (mode = 0x%x)\n", + info->name, info->mode); + err = -EINVAL; + goto pps_register_source_exit; + }See the comment below where you define
PPS_WIDTHBOTH.Quoted 22 lines
drivers/pps/kapi.c +0
-0/* Allocate memory for the new PPS source struct */pps =
kzalloc(sizeof(struct pps_device), GFP_KERNEL); if (pps == NULL) {drivers/pps/kapi.c +12
-1@@ -143,6 +151,39 @@ void pps_unregister_source(struct pps_device *pps) } EXPORT_SYMBOL(pps_unregister_source); +static u64 pps_ktime_sub (struct pps_ktime *ts1,struct pps_ktime *ts2) +{ + if (ts1->sec == ts2->sec) + return (ts1->nsec > ts2->nsec) ? (ts1->nsec - ts2->nsec) : (ts2->nsec - ts1->nsec); + + if (ts1->sec > ts2->sec) + return (ts1->sec - ts2->sec) * NSEC_PER_SEC + ts1->nsec - ts2->nsec; + + return (ts2->sec - ts1->sec) * NSEC_PER_SEC + ts2->nsec - ts1->nsec; +} + +static void pps_calc_clear_width (struct pps_device *pps)
Quoted 8 lines
drivers/pps/kapi.c void pps_unregister_source(struct pps_device *pps) · line 170
Quoted 5 lines
drivers/pps/kapi.c +5
-0+{ + if (pps->clear_sequence == 0) + return; + + pps->clear_width.sequence++;I don't understand the meaning of this field... regarding assert and clear it states the n-th sample but in this case...? Why do you need it?
For assert and clear, the sequence parameter is basically the counter of assert/clear events.
Similarly, I wanted to have a counter for the number of pulses which there width was counted.
The sequence was used by me in the sysfs to show the pulse counter and pulse width in nano-seconds.
Will counter make more sense instead of sequence? Initially, I used the assert_sequence and clear_sequence as the pulse counter, but there were few cases to handle.
In case first interrupt happened during a pulse, then assert_sequence != clear_sequence, but if not then assert_sequence == clear_sequence.
So I preferred to add an new independent value.
Quoted 150 lines
drivers/pps/kapi.c void pps_unregister_source(struct pps_device *pps) · line 181
Quoted 12 lines
drivers/pps/kapi.c +1
-0+ pps->clear_width.nsec = pps_ktime_sub(&pps->assert_tu,
&pps->clear_tu);
+ dev_dbg(pps->dev, "PPS clear width = %llu#%u\n", +pps->clear_width.nsec,pps->clear_width.sequence);drivers/pps/kapi.c +8
-0+} + +static void pps_calc_assert_width (struct pps_device *pps) +{ + if (pps->assert_sequence == 0) + return; + + pps->assert_width.sequence++;Ditto.
Quoted 135 lines
+
pps->assert_width.nsec=pps_ktime_sub(&pps->clear_tu, &pps->assert_tu);
+ dev_dbg(pps->dev, "PPS assert width = %llu#%u\n", +pps->assert_width.nsec,pps->assert_width.sequence);drivers/pps/kapi.c +2
-0+} +/*
pps_event- register a PPS event into the system
* @pps: the PPS device
* @ts: the event timestampdrivers/pps/kapi.c +4
-0@@ -191,6 +232,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event, dev_dbg(pps->dev, "capture assert seq #%u\n", pps->assert_sequence); + /* Calculate clear pulse-width */ + if (pps->params.mode & PPS_WIDTHCLEAR) + pps_calc_clear_width(pps); + captured = ~0; }if (event &
pps->params.mode&PPS_CAPTURECLEAR) {drivers/pps/kapi.c +4
-0@@ -205,6 +250,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event, dev_dbg(pps->dev, "capture clear seq #%u\n", pps->clear_sequence); + /* Calculate assert pulse-width */ + if (pps->params.mode & PPS_WIDTHASSERT) + pps_calc_assert_width(pps); + captured = ~0; }drivers/pps/pps.c +9
-0@@ -195,6 +195,11 @@ static long pps_cdev_ioctl(struct file *file, fdata.info.clear_tu = pps->clear_tu; fdata.info.current_mode = pps->current_mode; + memcpy(&fdata.info.assert_width, &pps->assert_width, + sizeof(struct pps_kwidth)); + memcpy(&fdata.info.clear_width, &pps->clear_width, + sizeof(struct pps_kwidth)); + spin_unlock_irq(&pps->lock); err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));@@ -283,6 +288,10 @@ static long pps_cdev_compat_ioctl(struct file *file, sizeof(struct pps_ktime_compat)); memcpy(&compat.info.clear_tu, &pps->clear_tu, sizeof(struct pps_ktime_compat)); + memcpy(&compat.info.assert_width, &pps->assert_width, + sizeof(struct pps_kwidth_compat)); + memcpy(&compat.info.clear_width, &pps->clear_width, + sizeof(struct pps_kwidth_compat)); spin_unlock_irq(&pps->lock);drivers/pps/sysfs.c +28
-0@@ -79,6 +79,34 @@ static ssize_t path_show(struct device *dev,struct device_attribute *attr, } static DEVICE_ATTR_RO(path); +static ssize_t pulse_width_assert_show (struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct pps_device *pps = dev_get_drvdata(dev); + + if (!(pps->info.mode & PPS_WIDTHASSERT)) + return 0; + + return sprintf(buf, "%llu#%u\n", + pps->assert_width.nsec, pps->assert_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_assert); + +static ssize_t pulse_width_clear_show (struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct pps_device *pps = dev_get_drvdata(dev); + + if (!(pps->info.mode & PPS_WIDTHCLEAR)) + return 0; + + return sprintf(buf, "%llu#%u\n", + pps->clear_width.nsec, pps->clear_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_clear); + static struct attribute *pps_attrs[] = {&dev_attr_assert.attr,
&dev_attr_clear.attr,drivers/pps/sysfs.c +2
-0@@ -86,6 +114,8 @@ static struct attribute *pps_attrs[] = { &dev_attr_echo.attr, &dev_attr_name.attr, &dev_attr_path.attr, + &dev_attr_pulse_width_assert.attr, + &dev_attr_pulse_width_clear.attr, NULL, };include/linux/pps_kernel.h +0
-0@@ -51,6 +51,9 @@ struct pps_device { struct pps_ktime clear_tu;int current_mode; /* PPS mode at event time */
+ struct
pps_kwidthassert_width; /* PPS assert pulse-width time and event seq # */
+ structpps_kwidthclear_width; /* PPS clear pulse-width time and event seq # */
+
unsigned int last_ev; /* last PPS event id */wait_queue_head_tqueue; /* PPS event queue */include/uapi/linux/pps.h +0
-0@@ -64,12 +64,24 @@ struct pps_ktime_compat { } __attribute__((packed, aligned(4))); #define PPS_TIME_INVALID (1<<0) /* used to specify timeout==NULL */
Quoted 14 lines
include/uapi/linux/pps.h struct pps_ktime_compat { · line 75
Quoted 9 lines
include/uapi/linux/pps.h +9
-0+struct pps_kwidth { + __u64 nsec; + __u32 sequence; +}; + +struct pps_kwidth_compat { + __u64 nsec; + __u32 sequence; +} __attribute__((packed,aligned (4 )));Why do you need a new type? Since both
assert_widthandclear_widthare time quantities as far asassert_tuandclear_tu, they can be of the same type, can't they? Or, at least they can simply be__u64since having anassert_widthorclear_widthlonger than 1 second is a non-sense...
For each pulse I wanted to save width in nsec (without sec) and counter.
I need it twice for both assert and clear, hence I added a new structure for it.
Quoted 25 lines
Quoted 11 lines
include/uapi/linux/pps.h +0
-0struct pps_kinfo {
__u32assert_sequence; /* seq. num. of assert event */__u32clear_sequence; /* seq. num. of clear event */ structpps_ktimeassert_tu; /* time of assert event */ structpps_ktimeclear_tu; /* time of clear event */ int current_mode; /* current mode bits */ + structpps_kwidthassert_width; /* assert pulse-width time and seq. num. */
+ structpps_kwidthclear_width; /* clear pulse-width time and seq. num. */
};Altering this structure may break userspace code... also rfc2783 at section-3.2 states that:
The API defines these new data structures:
typedef struct {
pps_seq_t assert_sequence; /* assert event seq # */ pps_seq_t clear_sequence; /* clear event seq # */pps_timeu_tassert_tu;
pps_timeu_tclear_tu;
int current_mode; /* current mode bits */ }pps_info_t;
Quoted 4 lines
So, I'm not willing to change this structure just to add this new data that I don't even know where it's used...
If you just read these information via sysfs, please drop these part.
ACK. I'll drop this part.
Quoted 16 lines
Quoted 16 lines
struct
pps_kinfo_compat{include/uapi/linux/pps.h +0
-0@@ -78,6 +90,8 @@ struct pps_kinfo_compat {struct
pps_ktime_compatassert_tu; /* time of assert event */ structpps_ktime_compatclear_tu; /* time of clear event */ int current_mode; /* current mode bits */ + structpps_kwidth_compatassert_width; /* assert pulse-width time and seq. num. */
+ structpps_kwidth_compatclear_width; /* clear pulse-width time and seq. num. */
};struct
pps_kparams{include/uapi/linux/pps.h +0
-0@@ -96,6 +110,11 @@ struct pps_kparams { #define PPS_CAPTURECLEAR 0x02 /* capture clear events */ #define PPS_CAPTUREBOTH 0x03 /* capture assert and clear events */
Quoted 12 lines
Quoted 8 lines
include/uapi/linux/pps.h +2
-0+/* Pulse-width calculation */ +#define PPS_WIDTHASSERT 0x04/* calculate assert width */
+#define PPS_WIDTHCLEAR 0x08 /* calculate clear width */
+#define PPS_WIDTHBOTH 0x0c /* calculate assert and clear width */
+I don't understand why a process should ask for just
PPS_WIDTHASSERTorPPS_WIDTHCLEAR... I think you can avoid defining these values and just enabling pulse width calculation when both assert and clear events are available.
ACK. I'll drop the new defines and enable width calculation when PPS_CAPTUREASSERT and PPS_CAPTURECLEAR are both defined.
Quoted 5 lines
Quoted 2 lines
#define PPS_OFFSETASSERT 0x10 /* apply compensation for assert event */ #define PPS_OFFSETCLEAR 0x20 /* apply compensation for clear event */
However, the real point is: since an userpsace program can retrieve the time of assert and clear events, why it cannot compute the pulses width by itself? :)
The userpsace program can retrieve the time of assert and clear events, but it is not always clear how to compute it.
Initially that was how I did it:
Read both times, make sure sequence of both times was identical, and then compute: clear_time – assert_time.
But as I mentioned, when using wide pulses, it might be that when driver starts, it is a the middle of a pulse.
In that case clear_time will be captured first (seq #1). Then assert_time is captured (seq #1).
However, assert pulse width can only be calculated for the second clear-time sequence and first assert-time sequence.
So to simplify this for the user, I added the calculation to the driver.
Hope this was clear.
Please let me know if this satisfies you, and then I’ll share a second version of patches which fixes all the other comments you gave.
---
Regards, Eliav
Rodolfo Giometti
maintainerRe: [PATCH 1/5] pps: add pulse-width calculation in nsec
Quoted 80 lines · Eliav
Quoted 70 lines · Rodolfo Giometti
Quoted 35 lines · Eliav Farber
This change adds PPS pulse-width calculation in nano seconds. Width time can be calculated for both assert time and reset time.
Calculation can be done only if capture ASSERT and capture CLEAR modes are both enabled.
Assert width is calculated as:
clear-time - assert-time
and clear width is calculated as:
assert-time - clear-timeRead-only sysfs were added to get the last pulse-width time and event sequence.
Examples:
* cat/sys/class/pps/pps0/pulse_width_assert
20001450#85
* cat/sys/class/pps/pps1/pulse_width_clear
979893314#16Signed-off-by Eliav Farber <[email protected]>---
drivers/pps/kapi.c | 49 ++++++++++++++++++++++++++++++++++++++ drivers/pps/pps.c | 9 +++++++ drivers/pps/sysfs.c | 30 +++++++++++++++++++++++include/linux/pps_kernel.h| 3 +++ include/uapi/linux/pps.h | 19 +++++++++++++++5 files changed, 110 insertions(+)drivers/pps/kapi.c +0
-0@@ -82,6 +82,14 @@ struct pps_device *pps_register_source(structpps_source_info *info, goto pps_register_source_exit;drivers/pps/kapi.c struct pps_device *pps_register_source(struct · line 93
Quoted 10 lines
drivers/pps/kapi.c +7
-0} + if ((info->mode & PPS_WIDTHBOTH) && + ((info->mode & PPS_CAPTUREBOTH) != PPS_CAPTUREBOTH)) { + pr_err("%s: width can't be calculated without both captures (mode = 0x%x)\n", + info->name, info->mode); + err = -EINVAL; + goto pps_register_source_exit; + }See the comment below where you define
PPS_WIDTHBOTH.Quoted 21 lines
drivers/pps/kapi.c +0
-0/* Allocate memory for the new PPS source struct */pps =
kzalloc(sizeof(struct pps_device), GFP_KERNEL); if (pps == NULL) {drivers/pps/kapi.c +12
-0@@ -143,6 +151,39 @@ void pps_unregister_source(struct pps_device *pps) } EXPORT_SYMBOL(pps_unregister_source); +static u64 pps_ktime_sub (struct pps_ktime *ts1,struct pps_ktime *ts2) +{ + if (ts1->sec == ts2->sec) + return (ts1->nsec > ts2->nsec) ? (ts1->nsec - ts2->nsec) : (ts2->nsec - ts1->nsec); + + if (ts1->sec > ts2->sec) + return (ts1->sec - ts2->sec) * NSEC_PER_SEC + ts1->nsec - ts2->nsec; + + return (ts2->sec - ts1->sec) * NSEC_PER_SEC + ts2->nsec - ts1->nsec; +} + +static void pps_calc_clear_width (struct pps_device *pps)Quoted 8 lines
drivers/pps/kapi.c void pps_unregister_source(struct pps_device *pps) · line 170
Quoted 5 lines
drivers/pps/kapi.c +5
-0+{ + if (pps->clear_sequence == 0) + return; + + pps->clear_width.sequence++;I don't understand the meaning of this field... regarding assert and clear it states the n-th sample but in this case...? Why do you need it?
Quoted 13 lines
For assert and clear, the sequence parameter is basically the counter of assert/clear events.
Similarly, I wanted to have a counter for the number of pulses which there width was counted.
The sequence was used by me in the sysfs to show the pulse counter and pulse width in nano-seconds.
Will counter make more sense instead of sequence? Initially, I used theassert_sequenceandclear_sequenceas the pulse counter, but there were few cases to handle.
In case first interrupt happened during a pulse, thenassert_sequence!=clear_sequence, but if not thenassert_sequence==clear_sequence.
So I preferred to add an new independent value.
OK.
Quoted 161 lines
Quoted 146 lines
drivers/pps/kapi.c void pps_unregister_source(struct pps_device *pps) · line 181
Quoted 11 lines
drivers/pps/kapi.c +11
-0+ pps->clear_width.nsec = pps_ktime_sub(&pps->assert_tu, &pps->clear_tu); + dev_dbg(pps->dev, "PPS clear width = %llu#%u\n", + pps->clear_width.nsec, pps->clear_width.sequence); +} + +static void pps_calc_assert_width (struct pps_device *pps) +{ + if (pps->assert_sequence == 0) + return; + + pps->assert_width.sequence++;Ditto.
Quoted 132 lines
drivers/pps/kapi.c +5
-0+ pps->assert_width.nsec = pps_ktime_sub(&pps->clear_tu, &pps->assert_tu); + dev_dbg(pps->dev, "PPS assert width = %llu#%u\n", + pps->assert_width.nsec, pps->assert_width.sequence); +} + /* pps_event - register a PPS event into the system* @pps: the PPS device
* @ts: the event timestampdrivers/pps/kapi.c +4
-0@@ -191,6 +232,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event, dev_dbg(pps->dev, "capture assert seq #%u\n", pps->assert_sequence); + /* Calculate clear pulse-width */ + if (pps->params.mode & PPS_WIDTHCLEAR) + pps_calc_clear_width(pps); + captured = ~0; }if (event &
pps->params.mode&PPS_CAPTURECLEAR) {drivers/pps/kapi.c +4
-0@@ -205,6 +250,10 @@ void pps_event(struct pps_device *pps, struct pps_event_time *ts, int event, dev_dbg(pps->dev, "capture clear seq #%u\n", pps->clear_sequence); + /* Calculate assert pulse-width */ + if (pps->params.mode & PPS_WIDTHASSERT) + pps_calc_assert_width(pps); + captured = ~0; }drivers/pps/pps.c +9
-0@@ -195,6 +195,11 @@ static long pps_cdev_ioctl(struct file *file, fdata.info.clear_tu = pps->clear_tu; fdata.info.current_mode = pps->current_mode; + memcpy(&fdata.info.assert_width, &pps->assert_width, + sizeof(struct pps_kwidth)); + memcpy(&fdata.info.clear_width, &pps->clear_width, + sizeof(struct pps_kwidth)); + spin_unlock_irq(&pps->lock); err = copy_to_user(uarg, &fdata, sizeof(struct pps_fdata));@@ -283,6 +288,10 @@ static long pps_cdev_compat_ioctl(struct file *file, sizeof(struct pps_ktime_compat)); memcpy(&compat.info.clear_tu, &pps->clear_tu, sizeof(struct pps_ktime_compat)); + memcpy(&compat.info.assert_width, &pps->assert_width, + sizeof(struct pps_kwidth_compat)); + memcpy(&compat.info.clear_width, &pps->clear_width, + sizeof(struct pps_kwidth_compat)); spin_unlock_irq(&pps->lock);drivers/pps/sysfs.c +28
-0@@ -79,6 +79,34 @@ static ssize_t path_show(struct device *dev, structdevice_attribute *attr, } static DEVICE_ATTR_RO(path); +static ssize_t pulse_width_assert_show (struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct pps_device *pps = dev_get_drvdata(dev); + + if (!(pps->info.mode & PPS_WIDTHASSERT)) + return 0; + + return sprintf(buf, "%llu#%u\n", + pps->assert_width.nsec, pps->assert_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_assert); + +static ssize_t pulse_width_clear_show (struct device *dev, + struct device_attribute *attr, + char *buf) +{ + struct pps_device *pps = dev_get_drvdata(dev); + + if (!(pps->info.mode & PPS_WIDTHCLEAR)) + return 0; + + return sprintf(buf, "%llu#%u\n", + pps->clear_width.nsec, pps->clear_width.sequence); +} +static DEVICE_ATTR_RO (pulse_width_clear); + static struct attribute *pps_attrs[] = { &dev_attr_assert.attr,&dev_attr_clear.attr,
drivers/pps/sysfs.c +2
-0@@ -86,6 +114,8 @@ static struct attribute *pps_attrs[] = { &dev_attr_echo.attr, &dev_attr_name.attr, &dev_attr_path.attr, + &dev_attr_pulse_width_assert.attr, + &dev_attr_pulse_width_clear.attr, NULL, };include/linux/pps_kernel.h +0
-0@@ -51,6 +51,9 @@ struct pps_device { struct pps_ktime clear_tu;int current_mode; /* PPS mode at event time */
+ struct
pps_kwidthassert_width; /* PPS assert pulse-width time and event seq # */
+ structpps_kwidthclear_width; /* PPS clear pulse-width time and event seq # */
+
unsigned int last_ev; /* last PPS event id */wait_queue_head_tqueue; /* PPS event queue */include/uapi/linux/pps.h +0
-0@@ -64,12 +64,24 @@ struct pps_ktime_compat { } __attribute__((packed, aligned(4))); #define PPS_TIME_INVALID (1<<0) /* used to specify timeout==NULL */Quoted 14 lines
include/uapi/linux/pps.h struct pps_ktime_compat { · line 75
Quoted 9 lines
include/uapi/linux/pps.h +9
-0+struct pps_kwidth { + __u64 nsec; + __u32 sequence; +}; + +struct pps_kwidth_compat { + __u64 nsec; + __u32 sequence; +} __attribute__((packed,aligned (4 )));Why do you need a new type? Since both
assert_widthandclear_widthare time quantities as far asassert_tuandclear_tu, they can be of the same type, can't they? Or, at least they can simply be__u64since having anassert_widthorclear_widthlonger than 1 second is a non-sense...
Quoted 4 lines
For each pulse I wanted to save width in nsec (without sec) and counter.
I need it twice for both assert and clear, hence I added a new structure for it.
I see, but I prefere you do as in struct pps_kinfo where times are times and sequence numbers are numbers, and not mixing them.
Quoted 67 lines
Quoted 24 lines
Quoted 10 lines
include/uapi/linux/pps.h +0
-0struct pps_kinfo {
__u32assert_sequence; /* seq. num. of assert event */__u32clear_sequence; /* seq. num. of clear event */ structpps_ktimeassert_tu; /* time of assert event */ structpps_ktimeclear_tu; /* time of clear event */
int current_mode; /* current mode bits */
+ structpps_kwidthassert_width; /* assert pulse-width time and seq. num. */
+ structpps_kwidthclear_width; /* clear pulse-width time and seq. num. */ };Altering this structure may break userspace code... also rfc2783 at section-3.2 states that:
The API defines these new data structures:
typedef struct {
pps_seq_t assert_sequence; /* assert event seq # */ pps_seq_t clear_sequence; /* clear event seq # */pps_timeu_tassert_tu;
pps_timeu_tclear_tu;
int current_mode; /* current mode bits */ }pps_info_t;Quoted 4 lines
So, I'm not willing to change this structure just to add this new data that I don't even know where it's used...
If you just read these information via sysfs, please drop these part.
ACK. I'll drop this part.
Quoted 16 lines
Quoted 16 lines
struct
pps_kinfo_compat{include/uapi/linux/pps.h +0
-0@@ -78,6 +90,8 @@ struct pps_kinfo_compat {struct
pps_ktime_compatassert_tu; /* time of assert event */ structpps_ktime_compatclear_tu; /* time of clear event */ int current_mode; /* current mode bits */ + structpps_kwidth_compatassert_width; /* assert pulse-width time and seq. num. */
+ structpps_kwidth_compatclear_width; /* clear pulse-width time and seq. num. */
};struct
pps_kparams{include/uapi/linux/pps.h +0
-0@@ -96,6 +110,11 @@ struct pps_kparams { #define PPS_CAPTURECLEAR 0x02 /* capture clear events */ #define PPS_CAPTUREBOTH 0x03 /* capture assert and clear events */Quoted 10 lines
Quoted 6 lines
include/uapi/linux/pps.h +4
-0+/* Pulse-width calculation */ +#define PPS_WIDTHASSERT 0x04/* calculate assert width */ +#define PPS_WIDTHCLEAR 0x08/* calculate clear width */ +#define PPS_WIDTHBOTH 0x0c/* calculate assert and clear width */
+I don't understand why a process should ask for just
PPS_WIDTHASSERTorPPS_WIDTHCLEAR... I think you can avoid defining these values and just enabling pulse width calculation when both assert and clear events are available.ACK. I'll drop the new defines and enable width calculation when
PPS_CAPTUREASSERTandPPS_CAPTURECLEARare both defined.Quoted 5 lines
Quoted 2 lines
#define PPS_OFFSETASSERT 0x10 /* apply compensation for assert event */ #define PPS_OFFSETCLEAR 0x20 /* apply compensation for clear event */
However, the real point is: since an userpsace program can retrieve the time of assert and clear events, why it cannot compute the pulses width by itself? :)
Quoted 16 lines
The userpsace program can retrieve the time of assert and clear events, but it is not always clear how to compute it.
Initially that was how I did it:
Read both times, make sure sequence of both times was identical, and then compute:clear_time–assert_time.
But as I mentioned, when using wide pulses, it might be that when driver starts, it is a the middle of a pulse.
In that caseclear_timewill be captured first (seq #1). Thenassert_timeis captured (seq #1).
However, assert pulse width can only be calculated for the second clear-time sequence and first assert-time sequence.
So to simplify this for the user, I added the calculation to the driver.
Hope this was clear.
Please let me know if this satisfies you, and then I’ll share a second version of patches which fixes all the other comments you gave.
Mmm... kernel drivers should implement mechanisms and not policies and since RFC2783 doesn't state this computation I think you are implementing a policy.
Let me suggest to add this piece of code to the pps-utils (maybe within the ppstest.c utility).
Ciao,
Rodolfo
Signature · 4 lines
GNU/Linux Solutions e-mail: [email protected] Linux Device Driver [email protected] Embedded Systems phone: +39 349 2432127 UNIX programming skype: rodolfo.giometti
PATCH 2/5 dt-bindings: pps: pps-gpio: introduce capture-clear property
In reply to Eliav FarberAdd a new optional "capture-clear" boolean property. When present, PPS clear events shall also be reported.
--- Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++ 1 file changed, 4 insertions(+)
Documentation/devicetree/bindings/pps/pps-gpio.txt +4 -0
@@ -14,6 +14,10 @@ Additional required properties for the PPS ECHO functionality: Optional properties: - assert-falling-edge: when present, assert is indicated by a falling edge (instead of by a rising edge) +- capture-clear: when present, report also PPS clear events, which is the + opposite of the assert edge (if assert is rising-edge then + clear is falling-edge and if assert is falling-edge then + clear is rising-edge). Example: pps {
Signature · 1 line
2.40.1
Re: [PATCH 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property
Quoted 6 lines · Eliav Farber
Add a new optional "capture-clear" boolean property. When present, PPS clear events shall also be reported.
Signed-off-by Eliav Farber <[email protected]>--- Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++
Please convert the bindings to DT schema first.
Quoted 7 lines
1 file changed, 4 insertions(+)Documentation/devicetree/bindings/pps/pps-gpio.txt +0
-0
Documentation/devicetree/bindings/pps/pps-gpio.txt Additional required properties for the PPS ECHO functionality: · line 20
Quoted 7 lines
Documentation/devicetree/bindings/pps/pps-gpio.txt +4
-0@@ -14,6 +14,10 @@ Additional required properties for the PPS ECHO functionality: Optional properties: - assert-falling-edge: when present, assert is indicated by a falling edge (instead of by a rising edge) +- capture-clear: when present, report also PPS clear events, which is the + opposite of the assert edge (if assert is rising-edge then + clear is falling-edge and if assert is falling-edge then + clear is rising-edge).
Why this is board-dependant? Falling edge is happening anyway, so it is in the hardware all the time. DT describes the hardware, not Linux driver behavior.
What's more, your property name sounds a lot like a driver property, so even if this is suitable for DT, you would need to name it properly to match hardware feature/characteristic.
Best regards,
Krzysztof
Re: [PATCH 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property
Quoted 9 lines · Krzysztof Kozlowski
Quoted 6 lines · Eliav Farber
Add a new optional "capture-clear" boolean property. When present, PPS clear events shall also be reported.
Signed-off-by Eliav Farber <[email protected]>---
Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++Please convert the bindings to DT schema first.
I will convert to DT schema first, if I indeed end up modifying this file.
Quoted 14 lines
Quoted 14 lines
1 file changed, 4 insertions(+)Documentation/devicetree/bindings/pps/pps-gpio.txt +0
-0@@ -14,6 +14,10 @@ Additional required properties for the PPS ECHOfunctionality:
Optional properties:
- assert-falling-edge: when present, assert is indicated by a falling edge (instead of by a rising edge)
+- capture-clear: when present, report also PPS clear events, which is the
+ opposite of the assert edge (if assert is
Quoted 8 lines
Quoted 4 lines
rising-edge then
+ clear is falling-edge and if assert is falling-edge then
+ clear is rising-edge).Why this is board-dependant? Falling edge is happening anyway, so it is in the hardware all the time. DT describes the hardware, not Linux driver behavior.
Falling edge of the pulse is happening all the time, but the falling edge event is currently never reported by the pps-gpio driver.
It is because there is no place in the pps-gpio driver that sets info->capture_clear, so
pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data); will never be called.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pps/clients/pps-gpio.c?h=v6.4#n59
There was an option in the past to set info->capture_clear, but that option was removed in commit ee89646619ba ("pps: clients: gpio: Get rid of legacy platform data").
This node in the DT isn't a pure HW device, but rather a SW driver. The patch I shared allows to set capture-clear from device-tree same as setting of assert-falling-edge is done.
Do you suggest I enable capture_clear in a different way? Perhaps module-param?
Quoted 3 lines
What's more, your property name sounds a lot like a driver property, so even if this is suitable for DT, you would need to name it properly to match hardware feature/characteristic.
I chose capture-clear as a name since it is aligned with the driver's terminology. It sets the capture_clear parameter, just like assert-falling-edge in DT sets assert_falling_edge parameter.
---
Regards, Eliav
Re: [PATCH 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property
Quoted 45 lines · Eliav
Quoted 9 lines · Krzysztof Kozlowski
Quoted 6 lines · Eliav Farber
Add a new optional "capture-clear" boolean property. When present, PPS clear events shall also be reported.
Signed-off-by Eliav Farber <[email protected]>---
Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++Please convert the bindings to DT schema first.
I will convert to DT schema first, if I indeed end up modifying this file.
Quoted 16 lines
Quoted 16 lines
1 file changed, 4 insertions(+)Documentation/devicetree/bindings/pps/pps-gpio.txt +0
-0@@ -14,6 +14,10 @@ Additional required properties for the PPS ECHOfunctionality:
Optional properties:
- assert-falling-edge: when present, assert is indicated by a falling edge
(instead of by a rising edge)
+- capture-clear: when present, report also PPS clear events, which is the
+ opposite of the assert edge (if assert isQuoted 8 lines
Quoted 4 lines
rising-edge then
+ clear is falling-edge and if assert is falling-edge then
+ clear is rising-edge).Why this is board-dependant? Falling edge is happening anyway, so it is in the hardware all the time. DT describes the hardware, not Linux driver behavior.
Falling edge of the pulse is happening all the time, but the falling edge event is currently never reported by the pps-gpio driver.
It is because there is no place in the pps-gpio driver that sets
info->capture_clear, so
pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data);
will never be called.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pps/clients/pps-gpio.c?h=v6.4#n59
Quoted 3 lines
There was an option in the past to set
info->capture_clear, but that option was removed in commitee89646619ba("pps: clients: gpio: Get rid of legacy platform data").
I know the history and question was not about it. You add DT support, so whatever board files were doing, does not matter really.
Quoted 4 lines
This node in the DT isn't a pure HW device, but rather a SW driver. The patch I shared allows to set capture-clear from device-tree same as setting of assert-falling-edge is done.
The binding still describes actual hardware - there is a system with a PPS signal on a GPIO.
Your reasoning for this property is because you need it:
"It is because there is no place in the pps-gpio driver that sets" With this approach we would need to accept every weird or bogus property, because someone needs it for the driver.
Bindings are for the hardware, even if the concept is a bit more software-driven.
Quoted 3 lines
Do you suggest I enable
capture_clearin a different way? Perhaps module-param?
module params are also disliked, so usually the answer for non-hardware properties is sysfs ABI.
Whether this is hardware property or not, I don't know. You did not provide rationale supporting it, so I tend to look at this as candidate for sysfs.
Quoted 7 lines
Quoted 4 lines
What's more, your property name sounds a lot like a driver property, so even if this is suitable for DT, you would need to name it properly to match hardware feature/characteristic.
I chose capture-clear as a name since it is aligned with the driver's terminology. It sets the
capture_clearparameter, just like assert-falling-edge in DT setsassert_falling_edgeparameter.
Sure, poor examples like to multiply. For assert-falling-edge, it looks like some fake property was added instead of using proper, existing properties indicating the polarity of GPIO and/or type of interrupt (rising/falling edge).
Best regards,
Krzysztof
PATCH 3/5 pps: clients: gpio: add option to set capture-clear from device-tree
In reply to Eliav FarberEnable capture clear events if "capture-clear" boolean property exists in device-tree.
--- drivers/pps/clients/pps-gpio.c | 1 + 1 file changed, 1 insertion(+)
drivers/pps/clients/pps-gpio.c +1 -0
@@ -112,6 +112,7 @@ static int pps_gpio_setup(struct device *dev) data->assert_falling_edge =device_property_read_bool (dev,"assert-falling-edge" ); + data->capture_clear =device_property_read_bool (dev,"capture-clear" ); data->echo_pin =devm_gpiod_get_optional (dev,"echo" ,GPIOD_OUT_LOW );if (IS_ERR (data->echo_pin))
Signature · 1 line
2.40.1
A message that is not in the archive (sent off-list or not yet received): [email protected]
Rodolfo Giometti
maintainerRe: [PATCH 3/5] pps: clients: gpio: add option to set capture-clear from device-tree
Quoted 15 lines · Eliav
Quoted 14 lines · Eliav Farber
Enable capture clear events if "capture-clear" boolean property exists in device-tree.
Signed-off-by Eliav Farber <[email protected]>---
drivers/pps/clients/pps-gpio.c| 1 +1 file changed, 1 insertion(+)drivers/pps/clients/pps-gpio.c +0
-0@@ -112,6 +112,7 @@ static int pps_gpio_setup(struct device *dev)
Quoted 7 lines
Quoted 3 lines
data->assert_falling_edge =
device_property_read_bool(dev, "assert-falling-edge"); +data->capture_clear=device_property_read_bool(dev, "capture-clear");Rodolfo, currently
data->capture_clearis false by default, and there is no way of setting it in the pps-gpio driver.
How would you suggest setting it?
I think that driver needs a review... in fact, by using gpiod_*() functions we should be able to remove both assert_falling_edge and capture_clear flags.
Quoted 1 line
Will it be OK to setting
PPS_WIDTHASSERTby default?
I think they can both be asserted by default. Why should someone may prefer to disable one of them?
Ciao,
Rodolfo
Signature · 4 lines
GNU/Linux Solutions e-mail: [email protected] Linux Device Driver [email protected] Embedded Systems phone: +39 349 2432127 UNIX programming skype: rodolfo.giometti
PATCH 4/5 dt-bindings: pps: pps-gpio: introduce pulse-width properties
In reply to Eliav FarberAdd two new optional properties to calculate PPS pulse-width in nano seconds:
- assert-pulse-width
- clear-pulse-width
--- Documentation/devicetree/bindings/pps/pps-gpio.txt | 6 ++++++ 1 file changed, 6 insertions(+)
Documentation/devicetree/bindings/pps/pps-gpio.txt +6 -0
@@ -18,6 +18,12 @@ Optional properties: opposite of the assert edge (if assert is rising-edge then clear is falling-edge and if assert is falling-edge then clear is rising-edge). +- assert-pulse-width: when present, assert pulse width will be calculated in + nano seconds. + It should be enabled only if 'capture-clear' is enabled. +- clear-pulse-width: when present, clear pulse width will be calculated in + nano seconds. + It should be enabled only if 'capture-clear' is enabled. Example: pps {
Signature · 1 line
2.40.1
Re: [PATCH 4/5] dt-bindings: pps: pps-gpio: introduce pulse-width properties
Quoted 8 lines · Eliav Farber
Add two new optional properties to calculate PPS pulse-width in nano seconds:
- assert-pulse-width
- clear-pulse-widthSigned-off-by Eliav Farber <[email protected]>--- Documentation/devicetree/bindings/pps/pps-gpio.txt | 6 ++++++
No, no, one new property could sneak in, but three are a no go. DT schema.
Quoted 7 lines
1 file changed, 6 insertions(+)Documentation/devicetree/bindings/pps/pps-gpio.txt +0
-0
Documentation/devicetree/bindings/pps/pps-gpio.txt Optional properties: · line 26
Quoted 9 lines
Documentation/devicetree/bindings/pps/pps-gpio.txt +6
-0@@ -18,6 +18,12 @@ Optional properties: opposite of the assert edge (if assert is rising-edge then clear is falling-edge and if assert is falling-edge then clear is rising-edge). +- assert-pulse-width: when present, assert pulse width will be calculated in + nano seconds. + It should be enabled only if 'capture-clear' is enabled. +- clear-pulse-width: when present, clear pulse width will be calculated in + nano seconds. + It should be enabled only if 'capture-clear' is enabled.
I don't understand the description. Property name suggests there is some units (so you need to use common property unit suffix), but description suggests that you change the units to nanoseconds. That's very confusing.
Best regards,
Krzysztof
PATCH 5/5 pps: clients: gpio: enable pps pulse-width calculations based on device-tree
In reply to Eliav FarberThis change adds setting of PPS_WIDTHASSERT and PPS_WIDTHCLEAR modes to enable PPS pulse-width calculation.
Width calculation will be enabled if
- assert-pulse-width
- clear-pulse-width
boolean properties exist in device-tree.
--- drivers/pps/clients/pps-gpio.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)
drivers/pps/clients/pps-gpio.c +14 -0
@@ -33,6 +33,8 @@ struct pps_gpio_device_data {struct timer_list echo_timer;/* timer to reset echo active state */ bool assert_falling_edge;bool capture_clear; +bool assert_pulse_width; +bool clear_pulse_width;unsigned int echo_active_ms;/* PPS echo active duration */ unsigned long echo_timeout;/* timer timeout value in jiffies */ };
@@ -113,6 +115,10 @@ static int pps_gpio_setup(struct device *dev) data->assert_falling_edge =device_property_read_bool (dev,"assert-falling-edge" ); data->capture_clear =device_property_read_bool (dev,"capture-clear" ); + data->assert_pulse_width = +device_property_read_bool (dev,"assert-pulse-width" ); + data->clear_pulse_width = +device_property_read_bool (dev,"clear-pulse-width" ); data->echo_pin =devm_gpiod_get_optional (dev,"echo" ,GPIOD_OUT_LOW );if (IS_ERR (data->echo_pin))
@@ -186,6 +192,10 @@ static int pps_gpio_probe(struct platform_device *pdev)if (data->capture_clear) data->info.mode |=PPS_CAPTURECLEAR |PPS_OFFSETCLEAR |PPS_ECHOCLEAR ; +if (data->assert_pulse_width) + data->info.mode |=PPS_WIDTHASSERT ; +if (data->clear_pulse_width) + data->info.mode |=PPS_WIDTHCLEAR ; data->info.owner =THIS_MODULE ;snprintf (data->info.name,PPS_MAX_NAME_LEN -1 ,"%s.%d" , pdev->name, pdev->id);
@@ -199,6 +209,10 @@ static int pps_gpio_probe(struct platform_device *pdev) pps_default_params =PPS_CAPTUREASSERT |PPS_OFFSETASSERT ;if (data->capture_clear) pps_default_params |=PPS_CAPTURECLEAR |PPS_OFFSETCLEAR ; +if (data->assert_pulse_width) + pps_default_params |=PPS_WIDTHASSERT ; +if (data->clear_pulse_width) + pps_default_params |=PPS_WIDTHCLEAR ; data->pps =pps_register_source (&data->info, pps_default_params);if (IS_ERR (data->pps)) { dev_err(dev, "failed to register IRQ %d as PPS source\n",
Signature · 1 line
2.40.1