[PATCH 0/5] Add PPS pulse-width support

[PATCH 0/5] Add PPS pulse-width support

[Eliav Farber]

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

-- 
2.40.1

[PATCH 1/5] pps: add pulse-width calculation in nsec

[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-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

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 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(+)

diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
index d9d566f70ed1..deeecfc0a3ee 100644
--- a/drivers/pps/kapi.c
+++ b/drivers/pps/kapi.c
@@ -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;
 	}
 
diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
index 5d19baae6a38..8299a272af11 100644
--- a/drivers/pps/pps.c
+++ b/drivers/pps/pps.c
@@ -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);
 
diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
index 134bc33f6ad0..3e34de77dba6 100644
--- a/drivers/pps/sysfs.c
+++ b/drivers/pps/sysfs.c
@@ -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,
 };
 
diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
index 78c8ac4951b5..15f2338095c6 100644
--- a/include/linux/pps_kernel.h
+++ b/include/linux/pps_kernel.h
@@ -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 */
 
diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
index 009ebcd8ced5..dd93dac0afc1 100644
--- a/include/uapi/linux/pps.h
+++ b/include/uapi/linux/pps.h
@@ -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 */
 
-- 
2.40.1

[PATCH 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property

[Eliav Farber]

Add a new optional "capture-clear" boolean property.
When present, PPS clear events shall also be reported.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
index 9012a2a02e14..8d588e38c44e 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
@@ -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 {
-- 
2.40.1

[PATCH 3/5] pps: clients: gpio: add option to set capture-clear from device-tree

[Eliav Farber]

Enable capture clear events if "capture-clear" boolean property exists
in device-tree.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/pps/clients/pps-gpio.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 2f4b11b4dfcd..a61dc1299ce9 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -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))
-- 
2.40.1

[PATCH 4/5] dt-bindings: pps: pps-gpio: introduce pulse-width properties

[Eliav Farber]

Add two new optional properties to calculate PPS pulse-width in nano
seconds:
 - assert-pulse-width
 - clear-pulse-width

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 Documentation/devicetree/bindings/pps/pps-gpio.txt | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
index 8d588e38c44e..9ecfd5fb3b63 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
@@ -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 {
-- 
2.40.1

[PATCH 5/5] pps: clients: gpio: enable pps pulse-width calculations based on device-tree

[Eliav Farber]

This 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.

Signed-off-by: Eliav Farber <farbere@amazon.com>
---
 drivers/pps/clients/pps-gpio.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index a61dc1299ce9..ca4b8047e924 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -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",
-- 
2.40.1

Re: [PATCH 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property

[Krzysztof Kozlowski]

On 25/06/2023 16:21, Eliav Farber wrote:
> Add a new optional "capture-clear" boolean property.
> When present, PPS clear events shall also be reported.
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++

Please convert the bindings to DT schema first.

>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
> index 9012a2a02e14..8d588e38c44e 100644
> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
> @@ -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 4/5] dt-bindings: pps: pps-gpio: introduce pulse-width properties

[Krzysztof Kozlowski]

On 25/06/2023 16:21, Eliav Farber wrote:
> Add two new optional properties to calculate PPS pulse-width in nano
> seconds:
>  - assert-pulse-width
>  - clear-pulse-width
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>  Documentation/devicetree/bindings/pps/pps-gpio.txt | 6 ++++++

No, no, one new property could sneak in, but three are a no go. DT schema.

>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
> index 8d588e38c44e..9ecfd5fb3b63 100644
> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
> @@ -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

Re: [PATCH 1/5] pps: add pulse-width calculation in nsec

[Rodolfo Giometti]

On 25/06/23 16:21, Eliav Farber wrote:
> 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
> 
> Signed-off-by: Eliav Farber <farbere@amazon.com>
> ---
>   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(+)
> 
> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
> index d9d566f70ed1..deeecfc0a3ee 100644
> --- a/drivers/pps/kapi.c
> +++ b/drivers/pps/kapi.c
> @@ -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.

>   	/* 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++;

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?

> +	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.

> +	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;
>   	}
>   
> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
> index 5d19baae6a38..8299a272af11 100644
> --- a/drivers/pps/pps.c
> +++ b/drivers/pps/pps.c
> @@ -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);
>   
> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
> index 134bc33f6ad0..3e34de77dba6 100644
> --- a/drivers/pps/sysfs.c
> +++ b/drivers/pps/sysfs.c
> @@ -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,
>   };
>   
> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
> index 78c8ac4951b5..15f2338095c6 100644
> --- a/include/linux/pps_kernel.h
> +++ b/include/linux/pps_kernel.h
> @@ -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 */
>   
> diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
> index 009ebcd8ced5..dd93dac0afc1 100644
> --- a/include/uapi/linux/pps.h
> +++ b/include/uapi/linux/pps.h
> @@ -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)));

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...

>   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. */
>   };

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.

>   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 */
> +

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.

>   #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

-- 
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 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property

[Farber, Eliav]

On 6/25/2023 6:46 PM, Krzysztof Kozlowski wrote:
> On 25/06/2023 16:21, Eliav Farber wrote:
>> Add a new optional "capture-clear" boolean property.
>> When present, PPS clear events shall also be reported.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>>  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.

>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt 
>> b/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> index 9012a2a02e14..8d588e38c44e 100644
>> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> @@ -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.
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?

> 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

[Krzysztof Kozlowski]

On 28/06/2023 10:47, Farber, Eliav wrote:
> On 6/25/2023 6:46 PM, Krzysztof Kozlowski wrote:
>> On 25/06/2023 16:21, Eliav Farber wrote:
>>> Add a new optional "capture-clear" boolean property.
>>> When present, PPS clear events shall also be reported.
>>>
>>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>>> ---
>>>  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.
> 
>>>  1 file changed, 4 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt 
>>> b/Documentation/devicetree/bindings/pps/pps-gpio.txt
>>> index 9012a2a02e14..8d588e38c44e 100644
>>> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
>>> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
>>> @@ -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.
> 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").

I know the history and question was not about it. You add DT support, so
whatever board files were doing, does not matter really.

> 
> 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.

> 
> Do you suggest I enable capture_clear in 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.

> 
>> 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.

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

Re: [PATCH 1/5] pps: add pulse-width calculation in nsec

[Farber, Eliav]

On 6/27/2023 5:27 PM, Rodolfo Giometti wrote:
> On 25/06/23 16:21, Eliav Farber wrote:
>> 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
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>>   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(+)
>>
>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>> index d9d566f70ed1..deeecfc0a3ee 100644
>> --- a/drivers/pps/kapi.c
>> +++ b/drivers/pps/kapi.c
>> @@ -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.
>
>>       /* 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++;
>
> 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.

>> +     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.
>
>> +     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;
>>       }
>>
>> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
>> index 5d19baae6a38..8299a272af11 100644
>> --- a/drivers/pps/pps.c
>> +++ b/drivers/pps/pps.c
>> @@ -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);
>>
>> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
>> index 134bc33f6ad0..3e34de77dba6 100644
>> --- a/drivers/pps/sysfs.c
>> +++ b/drivers/pps/sysfs.c
>> @@ -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,
>>   };
>>
>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>> index 78c8ac4951b5..15f2338095c6 100644
>> --- a/include/linux/pps_kernel.h
>> +++ b/include/linux/pps_kernel.h
>> @@ -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 */
>>
>> diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
>> index 009ebcd8ced5..dd93dac0afc1 100644
>> --- a/include/uapi/linux/pps.h
>> +++ b/include/uapi/linux/pps.h
>> @@ -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)));
>
> 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...

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.

>>   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. */
>>   };
>
> 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.

ACK. I'll drop this part.

>>   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 */
>> +
>
> 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. 

ACK. I'll drop the new defines and enable width calculation when
PPS_CAPTUREASSERT and PPS_CAPTURECLEAR are both defined.

>>   #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

Re: [PATCH 1/5] pps: add pulse-width calculation in nsec

[Rodolfo Giometti]

On 02/07/23 14:20, Farber, Eliav wrote:
> On 6/27/2023 5:27 PM, Rodolfo Giometti wrote:
>> On 25/06/23 16:21, Eliav Farber wrote:
>>> 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
>>>
>>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>>> ---
>>>   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(+)
>>>
>>> diff --git a/drivers/pps/kapi.c b/drivers/pps/kapi.c
>>> index d9d566f70ed1..deeecfc0a3ee 100644
>>> --- a/drivers/pps/kapi.c
>>> +++ b/drivers/pps/kapi.c
>>> @@ -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.
>>
>>>       /* 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++;
>>
>> 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.

OK.

>>> +     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.
>>
>>> +     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;
>>>       }
>>>
>>> diff --git a/drivers/pps/pps.c b/drivers/pps/pps.c
>>> index 5d19baae6a38..8299a272af11 100644
>>> --- a/drivers/pps/pps.c
>>> +++ b/drivers/pps/pps.c
>>> @@ -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);
>>>
>>> diff --git a/drivers/pps/sysfs.c b/drivers/pps/sysfs.c
>>> index 134bc33f6ad0..3e34de77dba6 100644
>>> --- a/drivers/pps/sysfs.c
>>> +++ b/drivers/pps/sysfs.c
>>> @@ -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,
>>>   };
>>>
>>> diff --git a/include/linux/pps_kernel.h b/include/linux/pps_kernel.h
>>> index 78c8ac4951b5..15f2338095c6 100644
>>> --- a/include/linux/pps_kernel.h
>>> +++ b/include/linux/pps_kernel.h
>>> @@ -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 */
>>>
>>> diff --git a/include/uapi/linux/pps.h b/include/uapi/linux/pps.h
>>> index 009ebcd8ced5..dd93dac0afc1 100644
>>> --- a/include/uapi/linux/pps.h
>>> +++ b/include/uapi/linux/pps.h
>>> @@ -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)));
>>
>> 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...
> 
> 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.

>>>   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. */
>>>   };
>>
>> 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.
> 
> ACK. I'll drop this part.
> 
>>>   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 */
>>> +
>>
>> 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. 
> 
> ACK. I'll drop the new defines and enable width calculation when
> PPS_CAPTUREASSERT and PPS_CAPTURECLEAR are both defined.
> 
>>>   #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.

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

-- 
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 3/5] pps: clients: gpio: add option to set capture-clear from device-tree

[Rodolfo Giometti]

On 04/07/23 07:07, Farber, Eliav wrote:
> On 6/25/2023 5:21 PM, Eliav Farber wrote:
>> Enable capture clear events if "capture-clear" boolean property exists
>> in device-tree.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>> drivers/pps/clients/pps-gpio.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
>> index 2f4b11b4dfcd..a61dc1299ce9 100644
>> --- a/drivers/pps/clients/pps-gpio.c
>> +++ b/drivers/pps/clients/pps-gpio.c
>> @@ -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");
> 
> Rodolfo, currently data->capture_clear is 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.

> Will it be OK to setting PPS_WIDTHASSERT by default?

I think they can both be asserted by default. Why should someone may prefer to 
disable one of them?

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