[LinuxPPS] PPS echo implementation
Rodolfo Giometti
giometti at enneenne.com
Fri Sep 14 10:41:59 CEST 2018
On 14/09/18 07:21, tom burkart wrote:
> Hi all,
> attached is the latest patch for the PPS echo implementation originally authored
> by Lukas Senger.
>
> The patch applies cleanly to the current Raspberry Pi kernel (4.14.68), but has
> some extra files (devicetree items) when patched against the latest kernel
> (4.16.7) and an offset in pps-gpio.c as the mainline kernel now supports
> capturing on clear via the devicetree.
Patches must always be against __latest__ __vanilla__ kernel. Also they must be
created by using "git format-patch" command.
> Please test and comment. Ideally, this should be ready for inclusion in the
> mainline kernel.
>
> Kind regards,
>
> Tom Burkart
> Consultant
>
> AUSSEC Mob: 04 1768 2202 Fax: 02 9526 1230
> 30 Waterside Crs, Carramar NSW 2163
>
> pps-echo.patch
>
>
> diff -uNr linux.orig/arch/arm/boot/dts/overlays/Makefile linux/arch/arm/boot/dts/overlays/Makefile
> --- linux.orig/arch/arm/boot/dts/overlays/Makefile 2018-09-09 14:08:57.259678466 +1000
> +++ linux/arch/arm/boot/dts/overlays/Makefile 2018-09-09 15:44:33.479504815 +1000
> @@ -94,6 +94,7 @@
> pitft28-capacitive.dtbo \
> pitft28-resistive.dtbo \
> pitft35-resistive.dtbo \
> + pps-echo.dtbo \
> pps-gpio.dtbo \
If I well understand this is a modification of pps-gpio client, isn't it? So why
not patching pps-gpio.* directly?
> pwm.dtbo \
> pwm-2chan.dtbo \
> diff -uNr linux.orig/arch/arm/boot/dts/overlays/README linux/arch/arm/boot/dts/overlays/README
> --- linux.orig/arch/arm/boot/dts/overlays/README 2018-09-09 14:08:57.259678466 +1000
> +++ linux/arch/arm/boot/dts/overlays/README 2018-09-13 10:54:26.376019585 +1000
> @@ -1451,6 +1451,23 @@
> debug Debug output level {0-7}
>
>
> +Name: pps-echo
> +Info: Configures the pps-gpio (pulse-per-second time signal via
> + GPIO with PPS echo functionality).
> +Load: dtoverlay=pps-echo,<param>=<val>
> +Params: gpiopin Input GPIO (default "18")
> + echopin Output GPIO (default "17")
> + assert_falling_edge When present, assert is indicated by a falling
> + edge, rather than by a rising edge (default
> + off)
> + capture_clear Generate clear events on the trailing edge
> + (default off)
> + enable_pps_echo When present, enable echo functionality
> + (default on)
> + invert_pps_echo When present, invert the PPS echo pulse
> + (default off)
> +
> +
> Name: pps-gpio
> Info: Configures the pps-gpio (pulse-per-second time signal via GPIO).
> Load: dtoverlay=pps-gpio,<param>=<val>
> diff -uNr linux.orig/arch/arm/boot/dts/overlays/pps-echo-overlay.dts linux/arch/arm/boot/dts/overlays/pps-echo-overlay.dts
> --- linux.orig/arch/arm/boot/dts/overlays/pps-echo-overlay.dts 1970-01-01 10:00:00.000000000 +1000
> +++ linux/arch/arm/boot/dts/overlays/pps-echo-overlay.dts 2018-09-13 10:55:10.252020518 +1000
> @@ -0,0 +1,44 @@
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> + compatible = "brcm,bcm2708";
> + fragment at 0 {
> + target-path = "/";
> + __overlay__ {
> + pps: pps at 12 {
> + compatible = "pps-gpio";
> + pinctrl-names = "default";
> + pinctrl-0 = <&pps_pins>;
> + gpios = <&gpio 18 0>;
> + echo-gpios = <&gpio 17 0>;
> + enable-pps-echo;
> + status = "okay";
> + };
> + };
> + };
> +
> + fragment at 1 {
> + target = <&gpio>;
> + __overlay__ {
> + pps_pins: pps_pins at 12 {
> + brcm,pins = <18 17>;
> + brcm,function = <0 1>; // in out
> + brcm,pull = <0 0>; // off off
> + };
> + };
> + };
> +
> + __overrides__ {
> + gpiopin = <&pps>,"gpios:4",
> + <&pps>,"reg:0",
> + <&pps_pins>,"brcm,pins:0",
> + <&pps_pins>,"reg:0";
> + echopin = <&pps>,"echo-gpios:4",
> + <&pps_pins>,"brcm,pins:4";
> + assert_falling_edge = <&pps>,"assert-falling-edge?";
> + capture_clear = <&pps>,"capture-clear?";
> + enable_pps_echo = <&pps>,"enable-pps-echo?";
> + invert_pps_echo = <&pps>,"invert-pps-echo?";
> + };
> +};
> diff -uNr linux.orig/drivers/pps/clients/pps-gpio.c linux/drivers/pps/clients/pps-gpio.c
> --- linux.orig/drivers/pps/clients/pps-gpio.c 2018-09-09 14:09:10.383678068 +1000
> +++ linux/drivers/pps/clients/pps-gpio.c 2018-09-13 13:19:55.761547862 +1000
> @@ -35,6 +35,7 @@
> #include <linux/list.h>
> #include <linux/of_device.h>
> #include <linux/of_gpio.h>
> +#include <linux/delay.h>
>
> /* Info for each registered platform device */
> struct pps_gpio_device_data {
> @@ -43,7 +44,10 @@
> struct pps_source_info info; /* PPS source information */
> bool assert_falling_edge;
> bool capture_clear;
> + bool enable_pps_echo;
> + bool invert_pps_echo;
> unsigned int gpio_pin;
> + unsigned int echo_pin;
> };
>
> /*
> @@ -64,15 +68,46 @@
> rising_edge = gpio_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);
>
> + if (info->enable_pps_echo && (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)))
> + return IRQ_WAKE_THREAD;
I suppose this line is too long...
> return IRQ_HANDLED;
> }
>
> +static void pps_gpio_echo(struct pps_device *pps, int event, void *data)
> +{
> + const struct pps_gpio_device_data *info;
> +
> + info = data;
> +
> + if (event == PPS_CAPTUREASSERT && (pps->params.mode & PPS_ECHOASSERT))
> + gpio_set_value(info->echo_pin, (info->invert_pps_echo?0:1));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
|
Missing spaces & not needed brackets -----------------------+
> + else if (event == PPS_CAPTURECLEAR && (pps->params.mode & PPS_ECHOCLEAR))
> + gpio_set_value(info->echo_pin, (info->invert_pps_echo?0:1));
Maybe here you can use a more readable switch() like that:
switch(event) {
case PPS_CAPTUREASSERT:
if (pps->params.mode & PPS_ECHOASSERT)
gpio_set_value(info->echo_pin, info->invert_pps_echo ? 0 : 1);
break;
case PPS_CAPTURECLEAR:
if (pps->params.mode & PPS_ECHOCLEAR)
gpio_set_value(info->echo_pin, info->invert_pps_echo ? 0 : 1);
break;
}
> +}
> +
> +
> +/* 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);
Brrr... a msleep() into an IRQ handler? I know it's threaded but it's still an
IRQ handler!
Maybe is better using a kernel timer to defer this job. Also you should consider
to allow users to choose this delay by a specific DTS entry.
> + gpio_set_value(info->echo_pin, (info->invert_pps_echo?1:0));
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> static unsigned long
> get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
> {
> @@ -104,17 +139,33 @@
>
> 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;
> + data->enable_pps_echo = pdata->enable_pps_echo;
> + data->invert_pps_echo = pdata->invert_pps_echo;
> } else {
> - ret = of_get_gpio(np, 0);
> + ret = of_get_named_gpio(np, "gpios", 0);
> if (ret < 0) {
> dev_err(&pdev->dev, "failed to get GPIO from device tree\n");
> return ret;
> }
> data->gpio_pin = ret;
> + if (of_get_property(np, "enable-pps-echo", NULL)) {
I think we can safely drop property "enable-pps-echo" and considering the EHO
functionality activated if property "echo-gpios" is present.
> + data->enable_pps_echo = true;
> + ret = of_get_named_gpio(np, "echo-gpios", 0);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to get second GPIO from device tree\n");
> + data->enable_pps_echo = false;
I think here is better return ERROR instead of disabling ECHO functionality...
since a kernel developer should know what he/she's doing and a wrong parameter
should not be set to a default value which is, most probably, what the developer
doesn't want at all!
> + } else {
> + data->echo_pin = ret;
> + }
> +
> + if (of_get_property(np, "invert-pps-echo", NULL))
> + data->invert_pps_echo = true;
> + }
> gpio_label = PPS_GPIO_NAME;
>
> if (of_get_property(np, "assert-falling-edge", NULL))
> @@ -134,10 +185,25 @@
>
> 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");
It seems to me that this is not part of this patch but it's just a typo error of
current driver! Please, provide a separated patch.
> return -EINVAL;
> }
>
> + if (data->enable_pps_echo) {
> + ret = devm_gpio_request(&pdev->dev, data->echo_pin, gpio_label);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request GPIO %u\n",
> + data->echo_pin);
> + return ret;
> + }
> +
> + ret = gpio_direction_output(data->echo_pin, 0);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to set pin as output\n");
> + return -EINVAL;
> + }
> + }
> +
> /* IRQ setup */
> ret = gpio_to_irq(data->gpio_pin);
> if (ret < 0) {
> @@ -155,6 +221,8 @@
> 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;
>
> /* register PPS source */
> pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
> @@ -168,8 +236,15 @@
> }
>
> /* register IRQ interrupt handler */
> - ret = devm_request_irq(&pdev->dev, data->irq, pps_gpio_irq_handler,
> - get_irqf_trigger_flags(data), data->info.name, data);
> + if (data->enable_pps_echo) {
> + 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);
> + }
> + else {
> + ret = devm_request_irq(&pdev->dev, data->irq, pps_gpio_irq_handler,
> + get_irqf_trigger_flags(data), data->info.name, data);
> + }
> if (ret) {
> pps_unregister_source(data->pps);
> dev_err(&pdev->dev, "failed to acquire IRQ %d\n", data->irq);
> diff -uNr linux.orig/include/linux/pps-gpio.h linux/include/linux/pps-gpio.h
> --- linux.orig/include/linux/pps-gpio.h 2018-09-09 14:09:19.567677790 +1000
> +++ linux/include/linux/pps-gpio.h 2018-09-13 10:55:45.964021277 +1000
> @@ -25,7 +25,10 @@
> struct pps_gpio_platform_data {
> bool assert_falling_edge;
> bool capture_clear;
> + bool enable_pps_echo;
> + bool invert_pps_echo;
> unsigned int gpio_pin;
> + unsigned int echo_pin;
> const char *gpio_label;
> };
Ciao,
Rodolfo
--
HCE Engineering e-mail: giometti at hce-engineering.it
GNU/Linux Solutions giometti at enneenne.com
Linux Device Driver giometti at linux.it
Embedded Systems phone: +39 349 2432127
UNIX programming skype: rodolfo.giometti
Cosino Project - the quick prototyping embedded system - www.cosino.it
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
More information about the discussions
mailing list