[PATCH v6 0/4] PPS: pps-gpio PPS ECHO implementation

[PATCH v6 0/4] PPS: pps-gpio PPS ECHO implementation

[Tom Burkart]

Hi all,
please find attached the PPS-GPIO PPS ECHO implementation patch. The
driver claims to have echo functionality in the sysfs interface but this
functionality is not present.  This patch provides this functionality.

Parts 1 and 2 of the patch change the original driver from the number
based GPIO API to the descriptor based API.  This does break the backward
compatibility of all devicetree descriptions for this driver as the
descriptor based API appends either "-gpios" or "-gpio" to all names
passed to it and then tries to find a match in the devicetree.

Parts 3 and 4 then add the PPS ECHO functionality.  This is enabled if a
"echo-gpios" entry is found in the devicetree.

Changes in v4:
There is a bugfix in part 2 that was returning an uninitialised variable
and juggling some code that speeds up the probe by not running code when
not using PPS ECHO.

Changes in v5:
Meet all requirements of checkpatch

Changes in v6:
Cosmetic changes to code layout

On the linuxpps mailing list it was suggested to use a hrtimer for
resetting the GPIO ECHO active state to the inactive state.
Please also comment on whether a hrtimer is necessary/desirable for the
purpose of resetting the echo pin active state.  I am happy to implement
it if there is a need.

Please install, test and comment as it is now a quite major change to
the driver.
Please do send suggestions for improvement.

Tom Burkart

Tom Burkart (4):
  dt-bindings: pps: descriptor-based gpio, capture-clear addition
  pps: descriptor-based gpio, capture-clear addition
  dt-bindings: pps: pps-gpio PPS ECHO implementation
  pps: pps-gpio pps-echo implementation

 Documentation/devicetree/bindings/pps/pps-gpio.txt |  15 ++-
 drivers/pps/clients/pps-gpio.c                     | 133 +++++++++++++++++----
 include/linux/pps-gpio.h                           |   5 +-
 3 files changed, 126 insertions(+), 27 deletions(-)

-- 
2.12.3

[PATCH v6 1/4] dt-bindings: pps: descriptor-based gpio, capture-clear addition

[Tom Burkart]

This patch changes the devicetree bindings for the pps-gpio driver
from the integer based API to the descriptor based API.  It also adds
documentation for the device tree capture-clear option.

The change from integer based GPIO API to the descriptor based API
breaks backward compatibility for the devicetree.  This is due to
the descriptor based API appending "-gpio" or "-gpios" (see
Documentation/gpio/base.txt.)

Signed-off-by: Tom Burkart <tom@aussec.com>
---
 Documentation/devicetree/bindings/pps/pps-gpio.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
index 3683874832ae..f7789e48a022 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
@@ -5,19 +5,21 @@ a GPIO pin.
 
 Required properties:
 - compatible: should be "pps-gpio"
-- gpios: one PPS GPIO in the format described by ../gpio/gpio.txt
+- pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt
 
 Optional properties:
 - assert-falling-edge: when present, assert is indicated by a falling edge
                        (instead of by a rising edge)
+- capture-clear: when present, also capture the PPS clear event
 
 Example:
 	pps {
 		pinctrl-names = "default";
 		pinctrl-0 = <&pinctrl_pps>;
 
-		gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
+		pps-gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
 		assert-falling-edge;
+		capture-clear;
 
 		compatible = "pps-gpio";
 	};
-- 
2.12.3

[PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition

[Tom Burkart]

This patch changes the GPIO access for the pps-gpio driver from the
integer based API to the descriptor based API.  It also adds
documentation for the device tree capture-clear option and
device tree capture-clear extraction.

The change from integer based GPIO API to the descriptor based API
breaks backward compatibility for the devicetree.  This is due to
the descriptor based API appending "-gpio" or "-gpios" (see
Documentation/gpio/base.txt.)

Signed-off-by: Tom Burkart <tom@aussec.com>
---
 drivers/pps/clients/pps-gpio.c | 33 +++++++++++----------------------
 include/linux/pps-gpio.h       |  2 +-
 2 files changed, 12 insertions(+), 23 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 333ad7d5b45b..d25710883794 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -31,7 +31,7 @@
 #include <linux/slab.h>
 #include <linux/pps_kernel.h>
 #include <linux/pps-gpio.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/list.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
@@ -41,9 +41,9 @@ struct pps_gpio_device_data {
 	int irq;			/* IRQ used as PPS source */
 	struct pps_device *pps;		/* PPS source device */
 	struct pps_source_info info;	/* PPS source information */
+	struct gpio_desc *gpio_pin;	/* GPIO port descriptors */
 	bool assert_falling_edge;
 	bool capture_clear;
-	unsigned int gpio_pin;
 };
 
 /*
@@ -61,7 +61,7 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
 
 	info = data;
 
-	rising_edge = gpio_get_value(info->gpio_pin);
+	rising_edge = gpiod_get_value(info->gpio_pin);
 	if ((rising_edge && !info->assert_falling_edge) ||
 			(!rising_edge && info->assert_falling_edge))
 		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
@@ -109,34 +109,23 @@ static int pps_gpio_probe(struct platform_device *pdev)
 		data->assert_falling_edge = pdata->assert_falling_edge;
 		data->capture_clear = pdata->capture_clear;
 	} else {
-		ret = of_get_gpio(np, 0);
-		if (ret < 0) {
-			dev_err(&pdev->dev, "failed to get GPIO from device tree\n");
-			return ret;
+		/* GPIO setup */
+		data->gpio_pin = devm_gpiod_get(&pdev->dev, "pps", GPIOD_IN);
+		if (IS_ERR(data->gpio_pin)) {
+			dev_err(&pdev->dev, "failed to request PPS GPIO\n");
+			return PTR_ERR(data->gpio_pin);
 		}
-		data->gpio_pin = ret;
 		gpio_label = PPS_GPIO_NAME;
 
 		if (of_get_property(np, "assert-falling-edge", NULL))
 			data->assert_falling_edge = true;
-	}
-
-	/* GPIO setup */
-	ret = devm_gpio_request(&pdev->dev, data->gpio_pin, gpio_label);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to request GPIO %u\n",
-			data->gpio_pin);
-		return ret;
-	}
 
-	ret = gpio_direction_input(data->gpio_pin);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to set pin direction\n");
-		return -EINVAL;
+		if (of_get_property(np, "capture-clear", NULL))
+			data->capture_clear = true;
 	}
 
 	/* IRQ setup */
-	ret = gpio_to_irq(data->gpio_pin);
+	ret = gpiod_to_irq(data->gpio_pin);
 	if (ret < 0) {
 		dev_err(&pdev->dev, "failed to map GPIO to IRQ: %d\n", ret);
 		return -EINVAL;
diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
index 56f35dd3d01d..671a029fc80b 100644
--- a/include/linux/pps-gpio.h
+++ b/include/linux/pps-gpio.h
@@ -23,9 +23,9 @@
 #define _PPS_GPIO_H
 
 struct pps_gpio_platform_data {
+	struct gpio_desc *gpio_pin;
 	bool assert_falling_edge;
 	bool capture_clear;
-	unsigned int gpio_pin;
 	const char *gpio_label;
 };
 
-- 
2.12.3

[PATCH v6 3/4] dt-bindings: pps: pps-gpio PPS ECHO implementation

[Tom Burkart]

This patch implements the device tree changes required for the pps
echo functionality for pps-gpio, that sysfs claims is available
already.

This patch was originally written by Lukas Senger as part of a masters
thesis project and modified for inclusion into the linux kernel by Tom
Burkart.

Signed-off-by: Lukas Senger <lukas@fridolin.com>
Signed-off-by: Tom Burkart <tom@aussec.com>
---
 Documentation/devicetree/bindings/pps/pps-gpio.txt | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
index f7789e48a022..dabf885e568a 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
@@ -7,10 +7,15 @@ Required properties:
 - compatible: should be "pps-gpio"
 - pps-gpios: one PPS GPIO in the format described by ../gpio/gpio.txt
 
+Additional required properties for the PPS ECHO functionality:
+- echo-gpios: one PPS ECHO GPIO in the format described by ../gpio/gpio.txt
+- echo-active-ms: duration in ms of the active portion of the echo pulse
+
 Optional properties:
 - assert-falling-edge: when present, assert is indicated by a falling edge
                        (instead of by a rising edge)
 - capture-clear: when present, also capture the PPS clear event
+- invert-pps-echo: when present, invert the PPS ECHO pulse
 
 Example:
 	pps {
@@ -21,5 +26,9 @@ Example:
 		assert-falling-edge;
 		capture-clear;
 
+		echo-gpios = <&gpio1 27 GPIO_ACTIVE_HIGH>;
+		echo-active-ms = <100>;
+		invert-pps-echo;
+
 		compatible = "pps-gpio";
 	};
-- 
2.12.3

[PATCH v6 4/4] pps: pps-gpio pps-echo implementation

[Tom Burkart]

This patch implements the pps echo functionality for pps-gpio, that
sysfs claims is available already.

Configuration is done via device tree bindings.

This patch was originally written by Lukas Senger as part of a masters
thesis project and modified for inclusion into the linux kernel by Tom
Burkart.

Signed-off-by: Lukas Senger <lukas@fridolin.com>
Signed-off-by: Tom Burkart <tom@aussec.com>
---
 drivers/pps/clients/pps-gpio.c | 104 +++++++++++++++++++++++++++++++++++++++--
 include/linux/pps-gpio.h       |   3 ++
 2 files changed, 103 insertions(+), 4 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index d25710883794..f5ffa2dc67b5 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -35,6 +35,8 @@
 #include <linux/list.h>
 #include <linux/of_device.h>
 #include <linux/of_gpio.h>
+#include <linux/timer.h>
+#include <linux/jiffies.h>
 
 /* Info for each registered platform device */
 struct pps_gpio_device_data {
@@ -42,8 +44,14 @@ struct pps_gpio_device_data {
 	struct pps_device *pps;		/* PPS source device */
 	struct pps_source_info info;	/* PPS source information */
 	struct gpio_desc *gpio_pin;	/* GPIO port descriptors */
+	struct gpio_desc *echo_pin;
+	struct timer_list echo_timer;	/* timer to reset echo active state */
 	bool assert_falling_edge;
 	bool capture_clear;
+	bool enable_pps_echo;
+	bool invert_pps_echo;
+	unsigned int echo_active_ms;	/* PPS echo active duration */
+	unsigned long echo_timeout;	/* timer timeout value in jiffies */
 };
 
 /*
@@ -64,15 +72,54 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
 	rising_edge = gpiod_get_value(info->gpio_pin);
 	if ((rising_edge && !info->assert_falling_edge) ||
 			(!rising_edge && info->assert_falling_edge))
-		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
+		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
 	else if (info->capture_clear &&
 			((rising_edge && info->assert_falling_edge) ||
-			 (!rising_edge && !info->assert_falling_edge)))
-		pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL);
+			(!rising_edge && !info->assert_falling_edge)))
+		pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data);
 
 	return IRQ_HANDLED;
 }
 
+static void pps_gpio_echo(struct pps_device *pps, int event, void *data)
+{
+	/* add_timer() needs to write into info->echo_timer */
+	struct pps_gpio_device_data *info;
+
+	info = data;
+
+	switch (event) {
+	case PPS_CAPTUREASSERT:
+		if (pps->params.mode & PPS_ECHOASSERT)
+			gpiod_set_value(info->echo_pin,
+				info->invert_pps_echo ? 0 : 1);
+		break;
+
+	case PPS_CAPTURECLEAR:
+		if (pps->params.mode & PPS_ECHOCLEAR)
+			gpiod_set_value(info->echo_pin,
+				info->invert_pps_echo ? 0 : 1);
+		break;
+	}
+
+	/* fire the timer */
+	if (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) {
+		info->echo_timer.expires = jiffies + info->echo_timeout;
+		add_timer(&info->echo_timer);
+	}
+}
+
+/* Timer callback to reset the echo pin to the inactive state */
+static void pps_gpio_echo_timer_callback(struct timer_list *t)
+{
+	const struct pps_gpio_device_data *info;
+
+	info = from_timer(info, t, echo_timer);
+
+	gpiod_set_value(info->echo_pin,
+		info->invert_pps_echo ? 1 : 0);
+}
+
 static unsigned long
 get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
 {
@@ -95,6 +142,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
 	int pps_default_params;
 	const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
 	struct device_node *np = pdev->dev.of_node;
+	u32 value;
 
 	/* allocate space for device info */
 	data = devm_kzalloc(&pdev->dev, sizeof(struct pps_gpio_device_data),
@@ -104,10 +152,15 @@ static int pps_gpio_probe(struct platform_device *pdev)
 
 	if (pdata) {
 		data->gpio_pin = pdata->gpio_pin;
+		data->echo_pin = pdata->echo_pin;
+		if (pdata->echo_pin != NULL)
+			data->enable_pps_echo = true;
 		gpio_label = pdata->gpio_label;
 
 		data->assert_falling_edge = pdata->assert_falling_edge;
 		data->capture_clear = pdata->capture_clear;
+		data->invert_pps_echo = pdata->invert_pps_echo;
+		data->echo_active_ms = pdata->echo_active_ms;
 	} else {
 		/* GPIO setup */
 		data->gpio_pin = devm_gpiod_get(&pdev->dev, "pps", GPIOD_IN);
@@ -117,11 +170,44 @@ static int pps_gpio_probe(struct platform_device *pdev)
 		}
 		gpio_label = PPS_GPIO_NAME;
 
+		if (of_get_property(np, "echo-gpios", NULL)) {
+			data->enable_pps_echo = true;
+
+			data->echo_pin = devm_gpiod_get(&pdev->dev,
+				"echo",
+				GPIOD_OUT_LOW);
+			if (IS_ERR(data->echo_pin)) {
+				dev_err(&pdev->dev, "failed to request ECHO GPIO\n");
+				return PTR_ERR(data->echo_pin);
+			}
+
+			ret = of_property_read_u32(np,
+				"echo-active-ms",
+				&value);
+			if (ret) {
+				dev_err(&pdev->dev,
+					"failed to get echo-active-ms from OF\n");
+				return ret;
+			}
+			data->echo_active_ms = value;
+		}
+
 		if (of_get_property(np, "assert-falling-edge", NULL))
 			data->assert_falling_edge = true;
 
 		if (of_get_property(np, "capture-clear", NULL))
 			data->capture_clear = true;
+
+		if (of_get_property(np, "invert-pps-echo", NULL))
+			data->invert_pps_echo = true;
+	}
+	/* sanity check on echo_active_ms */
+	if (data->enable_pps_echo
+			&& (!data->echo_active_ms || data->echo_active_ms > 999)) {
+		dev_err(&pdev->dev,
+			"echo-active-ms: %u - bad value from OF\n",
+			data->echo_active_ms);
+		return -EINVAL;
 	}
 
 	/* IRQ setup */
@@ -141,6 +227,11 @@ static int pps_gpio_probe(struct platform_device *pdev)
 	data->info.owner = THIS_MODULE;
 	snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
 		 pdev->name, pdev->id);
+	if (data->enable_pps_echo) {
+		data->info.echo = pps_gpio_echo;
+		data->echo_timeout = msecs_to_jiffies(data->echo_active_ms);
+		timer_setup(&data->echo_timer, pps_gpio_echo_timer_callback, 0);
+	}
 
 	/* register PPS source */
 	pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
@@ -174,6 +265,11 @@ static int pps_gpio_remove(struct platform_device *pdev)
 	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
 
 	pps_unregister_source(data->pps);
+	if (data->enable_pps_echo) {
+		del_timer_sync(&data->echo_timer);
+		/* reset echo pin in any case */
+		gpiod_set_value(data->echo_pin, data->invert_pps_echo ? 1 : 0);
+	}
 	dev_info(&pdev->dev, "removed IRQ %d as PPS source\n", data->irq);
 	return 0;
 }
@@ -198,4 +294,4 @@ MODULE_AUTHOR("Ricardo Martins <rasm@fe.up.pt>");
 MODULE_AUTHOR("James Nuss <jamesnuss@nanometrics.ca>");
 MODULE_DESCRIPTION("Use GPIO pin as PPS source");
 MODULE_LICENSE("GPL");
-MODULE_VERSION("1.0.0");
+MODULE_VERSION("1.1.0");
diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
index 671a029fc80b..81c13d32ad0d 100644
--- a/include/linux/pps-gpio.h
+++ b/include/linux/pps-gpio.h
@@ -24,8 +24,11 @@
 
 struct pps_gpio_platform_data {
 	struct gpio_desc *gpio_pin;
+	struct gpio_desc *echo_pin;
 	bool assert_falling_edge;
 	bool capture_clear;
+	bool invert_pps_echo;
+	unsigned int echo_active_ms;
 	const char *gpio_label;
 };
 
-- 
2.12.3

Re: [PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition

[Richard Cochran]

On Tue, Nov 13, 2018 at 02:40:05PM +1100, Tom Burkart wrote:
> The change from integer based GPIO API to the descriptor based API
> breaks backward compatibility for the devicetree.  This is due to
> the descriptor based API appending "-gpio" or "-gpios" (see
> Documentation/gpio/base.txt.)

Device tree is a user space facing ABI, and so you must continue to
support the legacy binding as well.

Thanks,
Richard

Re: [PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition

[tom burkart]

Quoting Richard Cochran <richardcochran@gmail.com>:

> Device tree is a user space facing ABI, and so you must continue to
> support the legacy binding as well.
Hi Richard,
thanks for the pointer.  I will work on generating a new patch to
address this and therefore not break backward compatibility.

Tom

Re: [PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition

[Philipp Zabel]

Hi Tom,

On Tue, Nov 13, 2018 at 5:08 AM Tom Burkart <tom@aussec.com> wrote:
>
> This patch changes the GPIO access for the pps-gpio driver from the
> integer based API to the descriptor based API.  It also adds
> documentation for the device tree capture-clear option and
> device tree capture-clear extraction.
>
> The change from integer based GPIO API to the descriptor based API
> breaks backward compatibility for the devicetree.  This is due to
> the descriptor based API appending "-gpio" or "-gpios" (see
> Documentation/gpio/base.txt.)

Documentation/devicetree/bindings/gpio/gpio.txt says:
 "While a non-existent <name> is considered valid
  for compatibility reasons (resolving to the "gpios" property),
  it is not allowed for new bindings."

This is not a new binding, so there should be no reason to change it.

gpiod_get() and friends support this by using the "gpios" property if
they are passed a NULL con_id.
of_find_gpio() in drivers/gpio/gpiolib-of.c contains the relevant code.

regards
Philipp

Re: [PATCH v6 2/4] pps: descriptor-based gpio, capture-clear addition

[tom burkart]

Quoting Philipp Zabel <philipp.zabel@gmail.com>:

> On Tue, Nov 13, 2018 at 5:08 AM Tom Burkart <tom@aussec.com> wrote:
>>
>> This patch changes the GPIO access for the pps-gpio driver from the
>> integer based API to the descriptor based API.  It also adds
>> documentation for the device tree capture-clear option and
>> device tree capture-clear extraction.
>>
>> The change from integer based GPIO API to the descriptor based API
>> breaks backward compatibility for the devicetree.  This is due to
>> the descriptor based API appending "-gpio" or "-gpios" (see
>> Documentation/gpio/base.txt.)
>
> Documentation/devicetree/bindings/gpio/gpio.txt says:
>  "While a non-existent <name> is considered valid
>   for compatibility reasons (resolving to the "gpios" property),
>   it is not allowed for new bindings."
>
> This is not a new binding, so there should be no reason to change it.
>
> gpiod_get() and friends support this by using the "gpios" property if
> they are passed a NULL con_id.
> of_find_gpio() in drivers/gpio/gpiolib-of.c contains the relevant code.

Hi Phillip,

Oops, sounds like I didn't read the documentation properly.  Thanks  
for the clarification.
A new patch will be on its way shortly...

Tom