pps-gpio reports as having echo capability via sysfs, which is not
actually the case. This patch implements it.
The output pin is hardcoded as 17. This should probably be configurable
via the dtoverlay in the same way as the input pin.
---
drivers/pps/clients/pps-gpio.c | 55 ++++++++++++++++++++++++++++++++++++++----
1 file changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 333ad7d5..dd7624f1 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -35,6 +35,12 @@
#include <linux/list.h>
#include <linux/of_device.h>
#include <linux/of_gpio.h>
+#include <linux/delay.h>
+
+/* TODO: this should work like gpio_pin below but I don't know how to work with
+ * devicetree overlays.
+ */
+#define PPS_GPIO_ECHO_PIN 17
/* Info for each registered platform device */
struct pps_gpio_device_data {
@@ -63,16 +69,41 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
rising_edge = gpio_get_value(info->gpio_pin);
if ((rising_edge && !info->assert_falling_edge) ||
- (!rising_edge && info->assert_falling_edge))
+ (!rising_edge && info->assert_falling_edge)) {
+ if (info->pps->params.mode & PPS_ECHOASSERT) {
+ gpio_set_value(PPS_GPIO_ECHO_PIN, 1);
+ }
pps_event(info->pps, &ts, PPS_CAPTUREASSERT, NULL);
- else if (info->capture_clear &&
+ } else if (info->capture_clear &&
((rising_edge && info->assert_falling_edge) ||
- (!rising_edge && !info->assert_falling_edge)))
+ (!rising_edge && !info->assert_falling_edge))) {
+ if (info->pps->params.mode & PPS_ECHOCLEAR) {
+ gpio_set_value(PPS_GPIO_ECHO_PIN, 1);
+ }
pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL);
+ }
+ if (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR))
+ return IRQ_WAKE_THREAD;
return IRQ_HANDLED;
}
+/* If we sent an echo pulse, we set the output pin back to low. This results in
+ * an echo pulse of roughly 100ms length.
+ */
+static irqreturn_t pps_gpio_irq_threaded(int irq, void *data)
+{
+ const struct pps_gpio_device_data *info;
+
+ info = data;
+
+ msleep(100);
+ gpio_set_value(PPS_GPIO_ECHO_PIN, 0);
+
+ return IRQ_HANDLED;
+}
+
+
static unsigned long
get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
{
@@ -131,7 +162,20 @@ static int pps_gpio_probe(struct platform_device *pdev)
ret = gpio_direction_input(data->gpio_pin);
if (ret) {
- dev_err(&pdev->dev, "failed to set pin direction\n");
+ dev_err(&pdev->dev, "failed to set pin as input\n");
+ return -EINVAL;
+ }
+
+ ret = devm_gpio_request(&pdev->dev, PPS_GPIO_ECHO_PIN, gpio_label);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to request GPIO %u\n",
+ PPS_GPIO_ECHO_PIN);
+ return ret;
+ }
+
+ ret = gpio_direction_output(PPS_GPIO_ECHO_PIN, 0);
+ if (ret) {
+ dev_err(&pdev->dev, "failed to set pin as output\n");
return -EINVAL;
}
@@ -165,7 +209,8 @@ static int pps_gpio_probe(struct platform_device *pdev)
}
/* register IRQ interrupt handler */
- ret = devm_request_irq(&pdev->dev, data->irq, pps_gpio_irq_handler,
+ ret = devm_request_threaded_irq(&pdev->dev, data->irq,
+ pps_gpio_irq_handler, pps_gpio_irq_threaded,
get_irqf_trigger_flags(data), data->info.name, data);
if (ret) {
pps_unregister_source(data->pps);
--
2.16.1
On 06/02/18 16:58, Lukas Senger wrote:
> pps-gpio reports as having echo capability via sysfs, which is not
> actually the case. This patch implements it.
>
> The output pin is hardcoded as 17. This should probably be configurable
> via the dtoverlay in the same way as the input pin.
No hardcoded stuff please! :-D
However thank you for your tutorial code, maybe someone will find it very useful.
Ciao,
Rodolfo
--
HCE Engineering e-mail: [email protected]
GNU/Linux Solutions [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Cosino Project - the quick prototyping embedded system - http://www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
---
drivers/pps/clients/pps-gpio.c | 1 +
include/linux/pps-gpio.h | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index dd7624f1d23f..35c3b14fc9b9 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -196,6 +196,7 @@ 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);
+ data->info.echo = pps_gpio_echo;
/* register PPS source */
pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
index 0035abe41b9a..67f50e8dcd11 100644
--- a/include/linux/pps-gpio.h
+++ b/include/linux/pps-gpio.h
@@ -29,4 +29,6 @@ struct pps_gpio_platform_data {
const char *gpio_label;
};
+static void pps_gpio_echo(struct pps_device *pps, int event, void *data){}
+
#endif
--
2.16.1
> No hardcoded stuff please! :-D
I expected as much :) So I looked into it a bit more and came up with
the patch below. It works for me but I'm unsure about two things in
particular:
In the __overrides__ section of the *.dts, why does gpiopin set reg and
should echopin set this as well?
In pps-gpio.c:135 how is pdata in the first part of this if-statement
supposed to know about these values?
Thanks,
Lukas
---
arch/arm/boot/dts/overlays/pps-gpio-overlay.dts | 13 ++++++++-----
drivers/pps/clients/pps-gpio.c | 26 ++++++++++++++-----------
include/linux/pps-gpio.h | 1 +
3 files changed, 24 insertions(+), 16 deletions(-)
diff --git a/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts b/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
index 9ee4bdfa6167..06e6cf5fc6ea 100644
--- a/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
+++ b/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
@@ -10,7 +10,8 @@
compatible = "pps-gpio";
pinctrl-names = "default";
pinctrl-0 = <&pps_pins>;
- gpios = <&gpio 18 0>;
+ in-gpios = <&gpio 18 0>;
+ out-gpios = <&gpio 17 0>;
status = "okay";
};
};
@@ -20,18 +21,20 @@
target = <&gpio>;
__overlay__ {
pps_pins: pps_pins@12 {
- brcm,pins = <18>;
- brcm,function = <0>; // in
- brcm,pull = <0>; // off
+ brcm,pins = <18 17>;
+ brcm,function = <0 1>; // in out
+ brcm,pull = <0 0>; // off off
};
};
};
__overrides__ {
- gpiopin = <&pps>,"gpios:4",
+ gpiopin = <&pps>,"in-gpios:4",
<&pps>,"reg:0",
<&pps_pins>,"brcm,pins:0",
<&pps_pins>,"reg:0";
+ echopin = <&pps>,"out-gpios:4",
+ <&pps_pins>,"brcm,pins:4";
assert_falling_edge = <&pps>,"assert-falling-edge?";
};
};
diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 35c3b14fc9b9..ce3065889a7e 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -37,10 +37,6 @@
#include <linux/of_gpio.h>
#include <linux/delay.h>
-/* TODO: this should work like gpio_pin below but I don't know how to work with
- * devicetree overlays.
- */
-#define PPS_GPIO_ECHO_PIN 17
/* Info for each registered platform device */
struct pps_gpio_device_data {
@@ -50,6 +46,7 @@ struct pps_gpio_device_data {
bool assert_falling_edge;
bool capture_clear;
unsigned int gpio_pin;
+ unsigned int echo_pin;
};
/*
@@ -71,14 +68,14 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
if ((rising_edge && !info->assert_falling_edge) ||
(!rising_edge && info->assert_falling_edge)) {
if (info->pps->params.mode & PPS_ECHOASSERT) {
- gpio_set_value(PPS_GPIO_ECHO_PIN, 1);
+ gpio_set_value(info->echo_pin, 1);
}
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))) {
if (info->pps->params.mode & PPS_ECHOCLEAR) {
- gpio_set_value(PPS_GPIO_ECHO_PIN, 1);
+ gpio_set_value(info->echo_pin, 1);
}
pps_event(info->pps, &ts, PPS_CAPTURECLEAR, NULL);
}
@@ -98,7 +95,7 @@ static irqreturn_t pps_gpio_irq_threaded(int irq, void *data)
info = data;
msleep(100);
- gpio_set_value(PPS_GPIO_ECHO_PIN, 0);
+ gpio_set_value(info->echo_pin, 0);
return IRQ_HANDLED;
}
@@ -135,17 +132,24 @@ static int pps_gpio_probe(struct platform_device *pdev)
if (pdata) {
data->gpio_pin = pdata->gpio_pin;
+ data->echo_pin = pdata->echo_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);
+ ret = of_get_named_gpio(np, "in-gpios", 0);
if (ret < 0) {
dev_err(&pdev->dev, "failed to get GPIO from device tree\n");
return ret;
}
data->gpio_pin = ret;
+ ret = of_get_named_gpio(np, "out-gpios", 0);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "failed to get second GPIO from device tree\n");
+ return ret;
+ }
+ data->echo_pin = ret;
gpio_label = PPS_GPIO_NAME;
if (of_get_property(np, "assert-falling-edge", NULL))
@@ -166,14 +170,14 @@ static int pps_gpio_probe(struct platform_device *pdev)
return -EINVAL;
}
- ret = devm_gpio_request(&pdev->dev, PPS_GPIO_ECHO_PIN, gpio_label);
+ ret = devm_gpio_request(&pdev->dev, data->echo_pin, gpio_label);
if (ret) {
dev_err(&pdev->dev, "failed to request GPIO %u\n",
- PPS_GPIO_ECHO_PIN);
+ data->echo_pin);
return ret;
}
- ret = gpio_direction_output(PPS_GPIO_ECHO_PIN, 0);
+ ret = gpio_direction_output(data->echo_pin, 0);
if (ret) {
dev_err(&pdev->dev, "failed to set pin as output\n");
return -EINVAL;
diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
index 67f50e8dcd11..de1701ae1c6a 100644
--- a/include/linux/pps-gpio.h
+++ b/include/linux/pps-gpio.h
@@ -26,6 +26,7 @@ struct pps_gpio_platform_data {
bool assert_falling_edge;
bool capture_clear;
unsigned int gpio_pin;
+ unsigned int echo_pin;
const char *gpio_label;
};
--
2.16.1
On 15/02/18 13:59, Lukas Senger wrote:
^^^^^^^^^^^^^^^^^^^
Missing description and signatures...
> ---
> drivers/pps/clients/pps-gpio.c | 1 +
> include/linux/pps-gpio.h | 2 ++
> 2 files changed, 3 insertions(+)
>
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index dd7624f1d23f..35c3b14fc9b9 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -196,6 +196,7 @@ 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);
> + data->info.echo = pps_gpio_echo;
>
> /* register PPS source */
> pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
> diff --git a/include/linux/pps-gpio.h b/include/linux/pps-gpio.h
> index 0035abe41b9a..67f50e8dcd11 100644
> --- a/include/linux/pps-gpio.h
> +++ b/include/linux/pps-gpio.h
> @@ -29,4 +29,6 @@ struct pps_gpio_platform_data {
> const char *gpio_label;
> };
>
> +static void pps_gpio_echo(struct pps_device *pps, int event, void *data){}
> +
> #endif
Why a void function? You should use it to toggle echoing GPIO... =8-o
Ciao,
Rodolfo
--
HCE Engineering e-mail: [email protected]
GNU/Linux Solutions [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Cosino Project - the quick prototyping embedded system - http://www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On 15/02/18 13:59, Lukas Senger wrote:
> ---
> arch/arm/boot/dts/overlays/pps-gpio-overlay.dts | 13 ++++++++-----
> drivers/pps/clients/pps-gpio.c | 26 ++++++++++++++-----------
> include/linux/pps-gpio.h | 1 +
> 3 files changed, 24 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts b/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
> index 9ee4bdfa6167..06e6cf5fc6ea 100644
> --- a/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
> +++ b/arch/arm/boot/dts/overlays/pps-gpio-overlay.dts
> @@ -10,7 +10,8 @@
> compatible = "pps-gpio";
> pinctrl-names = "default";
> pinctrl-0 = <&pps_pins>;
> - gpios = <&gpio 18 0>;
> + in-gpios = <&gpio 18 0>;
Please, don't break backward compatibility! You can leave "gpios" as is and
using, for instance, "echo-gpios" for echoing purposes. -+
|
+--------------------+
|
vvvvvvvvv
> + out-gpios = <&gpio 17 0>;
> status = "okay";
> };
> };
> @@ -20,18 +21,20 @@
> target = <&gpio>;
> __overlay__ {
> pps_pins: pps_pins@12 {
> - brcm,pins = <18>;
> - brcm,function = <0>; // in
> - brcm,pull = <0>; // off
> + brcm,pins = <18 17>;
> + brcm,function = <0 1>; // in out
> + brcm,pull = <0 0>; // off off
These modifications are not PPS related.
> };
> };
> };
>
> __overrides__ {
> - gpiopin = <&pps>,"gpios:4",
> + gpiopin = <&pps>,"in-gpios:4",
> <&pps>,"reg:0",
> <&pps_pins>,"brcm,pins:0",
> <&pps_pins>,"reg:0";
> + echopin = <&pps>,"out-gpios:4",
> + <&pps_pins>,"brcm,pins:4";
Ditto.
> assert_falling_edge = <&pps>,"assert-falling-edge?";
> };
> };
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 35c3b14fc9b9..ce3065889a7e 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -37,10 +37,6 @@
> #include <linux/of_gpio.h>
> #include <linux/delay.h>
>
> -/* TODO: this should work like gpio_pin below but I don't know how to work with
> - * devicetree overlays.
> - */
> -#define PPS_GPIO_ECHO_PIN 17
Please provide patches against current kernel code and not against your old patches.
I stop reviewing here since following modifications are similar to just reviewed
and not acceptable. I'm sorry.
Ciao,
Rodolfo
--
HCE Engineering e-mail: [email protected]
GNU/Linux Solutions [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Cosino Project - the quick prototyping embedded system - http://www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
> Why a void function? You should use it to toggle echoing GPIO... =8-o
RFC 2783 says to generate the echo pulse "as rapidly as possible" which
is why I do the toggling in the irq handler. However this leaves the
default echo function installed which just floods syslog.
Another problem with this patch is that it also is against my old one
and not against current kernel code.
Cheers,
Lukas
On 15/02/18 16:12, Lukas Senger wrote:
>
>> Why a void function? You should use it to toggle echoing GPIO... =8-o
>
> RFC 2783 says to generate the echo pulse "as rapidly as possible" which
> is why I do the toggling in the irq handler. However this leaves the
> default echo function installed which just floods syslog.
The pps_event() function does it for you in a beauty-and-clear way by using the
info.echo() function. :-D
> Another problem with this patch is that it also is against my old one
> and not against current kernel code.
Yes, correct it please.
Ciao,
Rodolfo
--
HCE Engineering e-mail: [email protected]
GNU/Linux Solutions [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Cosino Project - the quick prototyping embedded system - http://www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
On 15/02/18 16:08, Lukas Senger wrote:
>>> @@ -20,18 +21,20 @@
>>> target = <&gpio>;
>>> __overlay__ {
>>> pps_pins: pps_pins@12 {
>>> - brcm,pins = <18>;
>>> - brcm,function = <0>; // in
>>> - brcm,pull = <0>; // off
>>> + brcm,pins = <18 17>;
>>> + brcm,function = <0 1>; // in
>>> out
>>> + brcm,pull = <0 0>; // off
>>> off
>>
>> These modifications are not PPS related.
>>
>>> };
>>> };
>>> };
>>>
>>> __overrides__ {
>>> - gpiopin = <&pps>,"gpios:4",
>>> + gpiopin = <&pps>,"in-gpios:4",
>>> <&pps>,"reg:0",
>>> <&pps_pins>,"brcm,pins:0",
>>> <&pps_pins>,"reg:0";
>>> + echopin = <&pps>,"out-gpios:4",
>>> + <&pps_pins>,"brcm,pins:4";
>>
>> Ditto.
>
> I don't understand why these modifications are unrelated. Especially
> the echopin-option should exist, shouldn't it?
These modifications are needed to define a custom instance of a PPS device which
is not part of the PPS subtree, that's why they should be put into another patch.
Ciao,
Rodolfo
--
HCE Engineering e-mail: [email protected]
GNU/Linux Solutions [email protected]
Linux Device Driver [email protected]
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Cosino Project - the quick prototyping embedded system - http://www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - http://www.consulenti-ict.it
> > @@ -20,18 +21,20 @@
> > target = <&gpio>;
> > __overlay__ {
> > pps_pins: pps_pins@12 {
> > - brcm,pins = <18>;
> > - brcm,function = <0>; // in
> > - brcm,pull = <0>; // off
> > + brcm,pins = <18 17>;
> > + brcm,function = <0 1>; // in
> > out
> > + brcm,pull = <0 0>; // off
> > off
>
> These modifications are not PPS related.
>
> > };
> > };
> > };
> >
> > __overrides__ {
> > - gpiopin = <&pps>,"gpios:4",
> > + gpiopin = <&pps>,"in-gpios:4",
> > <&pps>,"reg:0",
> > <&pps_pins>,"brcm,pins:0",
> > <&pps_pins>,"reg:0";
> > + echopin = <&pps>,"out-gpios:4",
> > + <&pps_pins>,"brcm,pins:4";
>
> Ditto.
I don't understand why these modifications are unrelated. Especially
the echopin-option should exist, shouldn't it?
Cheers,
Lukas
> The pps_event() function does it for you in a beauty-and-clear way by
> using the info.echo() function. :-D
I thought it would increase latency to use info.echo() but I just
measured it and there is no significant difference. I guess it was a
case of premature optimization :)
Cheers,
Lukas