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

[PATCH v9 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 ABI to the descriptor based ABI.

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 v8:
Changes requested by Rob Herring and Philipp Zabel:
DT explanation and don't change the DT entry for the PPS gpio.

Changes in v9:
Simplify "if" expression by doing echo_active_ms validation earlier.

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 this is useful/desirable.

Please install, test and comment as it is now a quite major change to
the driver.
Suggestions for improvement are welcome.

Tom Burkart

Tom Burkart (4):
  dt-bindings: pps: 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 |  11 ++
 drivers/pps/clients/pps-gpio.c                     | 179 ++++++++++++++++-----
 include/linux/pps-gpio.h                           |   6 +-
 3 files changed, 152 insertions(+), 44 deletions(-)

-- 
2.12.3

[PATCH v9 1/4] dt-bindings: pps: capture-clear addition

[Tom Burkart]

It adds documentation for the device tree capture-clear option.

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

diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt b/Documentation/devicetree/bindings/pps/pps-gpio.txt
index 3683874832ae..1155d49c2699 100644
--- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
+++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
@@ -10,6 +10,7 @@ Required properties:
 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 {
@@ -18,6 +19,7 @@ Example:
 
 		gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
 		assert-falling-edge;
+		capture-clear;
 
 		compatible = "pps-gpio";
 	};
-- 
2.12.3

[PATCH v9 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 ABI to the descriptor based ABI.  It also adds the
extraction of the device tree capture-clear option.

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

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 333ad7d5b45b..d2fbc91dc8fc 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,18 +61,49 @@ 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);
 	else if (info->capture_clear &&
 			((rising_edge && info->assert_falling_edge) ||
-			 (!rising_edge && !info->assert_falling_edge)))
+			(!rising_edge && !info->assert_falling_edge)))
 		pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL);
 
 	return IRQ_HANDLED;
 }
 
+static int pps_gpio_setup(struct platform_device *pdev)
+{
+	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
+	const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
+	struct device_node *np = pdev->dev.of_node;
+	int ret;
+
+	if (pdata) {
+		data->gpio_pin = pdata->gpio_pin;
+
+		data->assert_falling_edge = pdata->assert_falling_edge;
+		data->capture_clear = pdata->capture_clear;
+	} else {
+		data->gpio_pin = devm_gpiod_get(&pdev->dev,
+			NULL,	/* request "gpios" */
+			GPIOD_IN);
+		if (IS_ERR(data->gpio_pin)) {
+			dev_err(&pdev->dev,
+				"failed to request PPS GPIO\n");
+			return PTR_ERR(data->gpio_pin);
+		}
+
+		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;
+	}
+	return 0;
+}
+
 static unsigned long
 get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
 {
@@ -90,53 +121,23 @@ get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
 static int pps_gpio_probe(struct platform_device *pdev)
 {
 	struct pps_gpio_device_data *data;
-	const char *gpio_label;
 	int ret;
 	int pps_default_params;
-	const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
-	struct device_node *np = pdev->dev.of_node;
 
 	/* allocate space for device info */
 	data = devm_kzalloc(&pdev->dev, sizeof(struct pps_gpio_device_data),
 			GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
-
-	if (pdata) {
-		data->gpio_pin = pdata->gpio_pin;
-		gpio_label = pdata->gpio_label;
-
-		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;
-		}
-		data->gpio_pin = ret;
-		gpio_label = PPS_GPIO_NAME;
-
-		if (of_get_property(np, "assert-falling-edge", NULL))
-			data->assert_falling_edge = true;
-	}
+	platform_set_drvdata(pdev, data);
 
 	/* 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");
+	ret = pps_gpio_setup(pdev);
+	if (ret)
 		return -EINVAL;
-	}
 
 	/* 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;
@@ -173,7 +174,6 @@ static int pps_gpio_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	platform_set_drvdata(pdev, data);
 	dev_info(data->pps->dev, "Registered IRQ %d as PPS source\n",
 		 data->irq);
 
@@ -209,4 +209,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 56f35dd3d01d..f028d2cda6f5 100644
--- a/include/linux/pps-gpio.h
+++ b/include/linux/pps-gpio.h
@@ -23,10 +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;
 };
 
 #endif /* _PPS_GPIO_H */
-- 
2.12.3

[PATCH v9 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 1155d49c2699..e09f6f2405c5 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"
 - 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 v9 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 | 101 +++++++++++++++++++++++++++++++++++++++--
 include/linux/pps-gpio.h       |   3 ++
 2 files changed, 101 insertions(+), 3 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index d2fbc91dc8fc..14293eb2ff5a 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,27 +72,72 @@ 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);
+		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 int pps_gpio_setup(struct platform_device *pdev)
 {
 	struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
 	const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
 	struct device_node *np = pdev->dev.of_node;
 	int ret;
+	u32 value;
 
 	if (pdata) {
 		data->gpio_pin = pdata->gpio_pin;
+		data->echo_pin = pdata->echo_pin;
+		if (pdata->echo_pin != NULL)
+			data->enable_pps_echo = true;
 
 		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 {
 		data->gpio_pin = devm_gpiod_get(&pdev->dev,
 			NULL,	/* request "gpios" */
@@ -95,11 +148,43 @@ static int pps_gpio_setup(struct platform_device *pdev)
 			return PTR_ERR(data->gpio_pin);
 		}
 
+		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;
+			/* sanity check on echo_active_ms */
+			if (!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;
+			}
+		}
+
 		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;
 	}
 	return 0;
 }
@@ -153,6 +238,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;
@@ -185,6 +275,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;
 }
@@ -209,4 +304,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.1.0");
+MODULE_VERSION("1.2.0");
diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
index f028d2cda6f5..5390f18c73a5 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;
 };
 
 #endif /* _PPS_GPIO_H */
-- 
2.12.3

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

[Rob Herring]

On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
>
> 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 1155d49c2699..e09f6f2405c5 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"
>  - 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

Why do you need this? Can't you just make the echo gpio GPIO_ACTIVE_LOW?

BTW, using the flag probably should have been done for
'assert-falling-edge' as well.

Rob

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

[tom burkart]

Hi Rob,

Quoting Rob Herring <robh@kernel.org>:

> On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
>>
>> 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 1155d49c2699..e09f6f2405c5 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"
>>  - 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
>
> Why do you need this? Can't you just make the echo gpio GPIO_ACTIVE_LOW?
>
> BTW, using the flag probably should have been done for
> 'assert-falling-edge' as well.

The hardware I use expects a positive-going echo pulse, however, it  
was really easy to give users the option to have it inverted in case  
they use different hardware that expects a negative-going edge.

I may well be needing this option myself if I find that the GPS I am  
using produces better results for negative-going edges than  
positive-going edges.

Tom

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

[Rob Herring]

On Mon, Nov 26, 2018 at 10:06 PM tom burkart <tom@aussec.com> wrote:
>
> Hi Rob,
>
> Quoting Rob Herring <robh@kernel.org>:
>
> > On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
> >>
> >> 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 1155d49c2699..e09f6f2405c5 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"
> >>  - 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
> >
> > Why do you need this? Can't you just make the echo gpio GPIO_ACTIVE_LOW?
> >
> > BTW, using the flag probably should have been done for
> > 'assert-falling-edge' as well.
>
> The hardware I use expects a positive-going echo pulse, however, it
> was really easy to give users the option to have it inverted in case
> they use different hardware that expects a negative-going edge.

It will be even easier to implement if you use GPIO_ACTIVE_LOW or
GPIO_ACTIVE_HIGH as appropriate. If the flag is set appropriately,
then gpiod_set_value(gpio, 1) asserts the pulse and
gpiod_set_value(gpio, 0) deasserts it no matter which way the h/w is
wired. You can then get rid of invert_pps_echo in the driver.

Rob

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

[tom burkart]

Quoting Rob Herring <robh@kernel.org>:

> On Mon, Nov 26, 2018 at 10:06 PM tom burkart <tom@aussec.com> wrote:
>>
>> Hi Rob,
>>
>> Quoting Rob Herring <robh@kernel.org>:
>>
>> > On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
>> >>
>> >> 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 1155d49c2699..e09f6f2405c5 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"
>> >>  - 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
>> >
>> > Why do you need this? Can't you just make the echo gpio GPIO_ACTIVE_LOW?
>> >
>> > BTW, using the flag probably should have been done for
>> > 'assert-falling-edge' as well.
>>
>> The hardware I use expects a positive-going echo pulse, however, it
>> was really easy to give users the option to have it inverted in case
>> they use different hardware that expects a negative-going edge.
>
> It will be even easier to implement if you use GPIO_ACTIVE_LOW or
> GPIO_ACTIVE_HIGH as appropriate. If the flag is set appropriately,
> then gpiod_set_value(gpio, 1) asserts the pulse and
> gpiod_set_value(gpio, 0) deasserts it no matter which way the h/w is
> wired. You can then get rid of invert_pps_echo in the driver.

Hi Rob,
sure we can do that, however, a hardware change will need a recompile  
of the kernel, rather than setting a flag.
You still need something to help decide whether to use GPIO_ACTIVE_LOW  
or GPIO_ACTIVE_HIGH depending on what hardware is currently connected  
to the system.

I am happy to delete that code as well, if it makes you happy.

Tom

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

[tom burkart]

Quoting Rob Herring <robh@kernel.org>:

> On Mon, Nov 26, 2018 at 10:06 PM tom burkart <tom@aussec.com> wrote:
>>
>> Quoting Rob Herring <robh@kernel.org>:
>>
>> > On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
>> >>
>> >> 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 1155d49c2699..e09f6f2405c5 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"
>> >>  - 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
>> >
>> > Why do you need this? Can't you just make the echo gpio GPIO_ACTIVE_LOW?
>> >
>> > BTW, using the flag probably should have been done for
>> > 'assert-falling-edge' as well.
>>
>> The hardware I use expects a positive-going echo pulse, however, it
>> was really easy to give users the option to have it inverted in case
>> they use different hardware that expects a negative-going edge.
>
> It will be even easier to implement if you use GPIO_ACTIVE_LOW or
> GPIO_ACTIVE_HIGH as appropriate. If the flag is set appropriately,
> then gpiod_set_value(gpio, 1) asserts the pulse and
> gpiod_set_value(gpio, 0) deasserts it no matter which way the h/w is
> wired. You can then get rid of invert_pps_echo in the driver.

Hi Rob,
I have looked at the appropriate changes to my code to implement the  
above.  However, there is no GPIO_ACTIVE_* as part of the gpiod_flags  
enum (include/linux/gpio/consumer.h).

What am I missing?

Tom

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

[Rob Herring]

On Mon, Dec 10, 2018 at 8:17 PM tom burkart <tom@aussec.com> wrote:
>
> Quoting Rob Herring <robh@kernel.org>:
>
> > On Mon, Nov 26, 2018 at 10:06 PM tom burkart <tom@aussec.com> wrote:
> >>
> >> Quoting Rob Herring <robh@kernel.org>:
> >>
> >> > On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
> >> >>
> >> >> 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 1155d49c2699..e09f6f2405c5 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"
> >> >>  - 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
> >> >
> >> > Why do you need this? Can't you just make the echo gpio GPIO_ACTIVE_LOW?
> >> >
> >> > BTW, using the flag probably should have been done for
> >> > 'assert-falling-edge' as well.
> >>
> >> The hardware I use expects a positive-going echo pulse, however, it
> >> was really easy to give users the option to have it inverted in case
> >> they use different hardware that expects a negative-going edge.
> >
> > It will be even easier to implement if you use GPIO_ACTIVE_LOW or
> > GPIO_ACTIVE_HIGH as appropriate. If the flag is set appropriately,
> > then gpiod_set_value(gpio, 1) asserts the pulse and
> > gpiod_set_value(gpio, 0) deasserts it no matter which way the h/w is
> > wired. You can then get rid of invert_pps_echo in the driver.
>
> Hi Rob,
> I have looked at the appropriate changes to my code to implement the
> above.  However, there is no GPIO_ACTIVE_* as part of the gpiod_flags
> enum (include/linux/gpio/consumer.h).
>
> What am I missing?

You don't need to know in the driver. You set the gpio state to 1 for
active, 0 for inactive and that does the right thing based on the flag
in the DT. IOW, if the DT defines the GPIO as active low, then setting
the gpio state to 1 will result in the GPIO being 0V. It's a bit
annoying and confusing at first until you realize the driver can just
handle either polarity transparently.

Rob

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

[tom burkart]

Quoting Rob Herring <robh@kernel.org>:

> On Mon, Dec 10, 2018 at 8:17 PM tom burkart <tom@aussec.com> wrote:
>>
>> Quoting Rob Herring <robh@kernel.org>:
>>
>> > On Mon, Nov 26, 2018 at 10:06 PM tom burkart <tom@aussec.com> wrote:
>> >>
>> >> Quoting Rob Herring <robh@kernel.org>:
>> >>
>> >> > On Thu, Nov 22, 2018 at 3:49 AM Tom Burkart <tom@aussec.com> wrote:
>> >> >>
>> >> >> 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 1155d49c2699..e09f6f2405c5 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"
>> >> >>  - 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
>> >> >
>> >> > Why do you need this? Can't you just make the echo gpio  
>> GPIO_ACTIVE_LOW?
>> >> >
>> >> > BTW, using the flag probably should have been done for
>> >> > 'assert-falling-edge' as well.
>> >>
>> >> The hardware I use expects a positive-going echo pulse, however, it
>> >> was really easy to give users the option to have it inverted in case
>> >> they use different hardware that expects a negative-going edge.
>> >
>> > It will be even easier to implement if you use GPIO_ACTIVE_LOW or
>> > GPIO_ACTIVE_HIGH as appropriate. If the flag is set appropriately,
>> > then gpiod_set_value(gpio, 1) asserts the pulse and
>> > gpiod_set_value(gpio, 0) deasserts it no matter which way the h/w is
>> > wired. You can then get rid of invert_pps_echo in the driver.
>>
>> Hi Rob,
>> I have looked at the appropriate changes to my code to implement the
>> above.  However, there is no GPIO_ACTIVE_* as part of the gpiod_flags
>> enum (include/linux/gpio/consumer.h).
>>
>> What am I missing?
>
> You don't need to know in the driver. You set the gpio state to 1 for
> active, 0 for inactive and that does the right thing based on the flag
> in the DT. IOW, if the DT defines the GPIO as active low, then setting
> the gpio state to 1 will result in the GPIO being 0V. It's a bit
> annoying and confusing at first until you realize the driver can just
> handle either polarity transparently.

Hi Rob,
thanks a lot!  The penny finally dropped.

My patch v12 will have the changes implemented (no invert-pps-echo).

Tom