[LinuxPPS] gpio-pps with echopin
Rodolfo Giometti
giometti at enneenne.com
Thu Apr 9 11:50:04 CEST 2015
On Thu, Apr 09, 2015 at 11:12:41AM +0200, f.hiesinger wrote:
> Hi Folks,
> I'm currently working on the pps-gpio module to echo the reception
> on another GPIO. I'm very new at module programming so I thought you
> can give any feedback or do some reviewing. Attached you can find my
> first outline which is running on my RaspberryPi (Raspbian 3.18.9+)
> for several weeks now.
Have you an idea about the echo delay?
> Best Regards,
> Frank
> diff --git a/clients/pps-gpio.c b/clients/pps-gpio.c
> index f41bacf..a92aee8 100644
> --- a/clients/pps-gpio.c
> +++ b/clients/pps-gpio.c
> @@ -22,6 +22,8 @@
>
> #define PPS_GPIO_NAME "pps-gpio"
> #define pr_fmt(fmt) PPS_GPIO_NAME ": " fmt
> +#define GPIO_ECHO
Why?
> +//#define DEBUG_PPS
Use dev_dbg()
> #include <linux/init.h>
> #include <linux/kernel.h>
> @@ -44,6 +46,7 @@ struct pps_gpio_device_data {
> bool assert_falling_edge;
> bool capture_clear;
> unsigned int gpio_pin;
> + unsigned int gpio_echo_pin;
> };
>
> /*
> @@ -64,14 +67,31 @@ 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))
> - 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;
> }
> +/* This echo function will be called within pps_event().
> + * it is assumed, that userdef pointer (data) will point
> + * to pps_gpio_device_data
> + */
> +static void pps_gpio_echo(struct pps_device *pps, int event,
> + void *data)
> +{
> + const struct pps_gpio_device_data *info;
> + info = data;
Why not const struct pps_gpio_device_data *info = data; ?
> +
> + if (event & PPS_CAPTUREASSERT){
> + gpio_set_value(info->gpio_echo_pin, 1); // TODO: make polarity configurable (invertable)
> + }
> + else {
> + gpio_set_value(info->gpio_echo_pin, 0);
> + }
You can drop parenthesis...
Use /* */ for comments
Don't use more than 80 chars width
> +}
> static unsigned long
> get_irqf_trigger_flags(const struct pps_gpio_device_data *data)
> @@ -91,6 +111,7 @@ static int pps_gpio_probe(struct platform_device *pdev)
> {
> struct pps_gpio_device_data *data;
> const char *gpio_label;
> + const char *gpio_echo_label;
> int ret;
> int pps_default_params;
> const struct pps_gpio_platform_data *pdata = pdev->dev.platform_data;
> @@ -104,10 +125,15 @@ static int pps_gpio_probe(struct platform_device *pdev)
>
> if (pdata) {
> data->gpio_pin = pdata->gpio_pin;
> + data->gpio_echo_pin = pdata->gpio_echo_pin;
> gpio_label = pdata->gpio_label;
> + gpio_echo_label = pdata->gpio_echo_label;
>
> data->assert_falling_edge = pdata->assert_falling_edge;
> data->capture_clear = pdata->capture_clear;
> +#ifdef DEBUG_PPS
> + dev_err(&pdev->dev, "1) data->assert_falling_edge = %d , data->capture_clear = %d\n ", data->assert_falling_edge, data->capture_clear);
> +#endif
Use dev_dbg()
> } else {
> ret = of_get_gpio(np, 0);
> if (ret < 0) {
> @@ -117,15 +143,30 @@ static int pps_gpio_probe(struct platform_device *pdev)
> data->gpio_pin = ret;
> gpio_label = PPS_GPIO_NAME;
>
> +#ifdef GPIO_ECHO
> + ret = of_get_gpio(np, 1);
> + if (ret < 0) {
> + dev_err(&pdev->dev, "failed to get Echo-GPIO from device tree\n");
> + return ret;
> + }
> + data->gpio_echo_pin = ret;
> + gpio_echo_label = "pps-gpio-echo";
> +#endif
> +
Drop #ifdef, you should verify the kernel settings from the DTS file.
> 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;
> +#ifdef DEBUG_PPS
> + dev_err(&pdev->dev, "2) data->assert_falling_edge = %d , data->capture_clear = %d\n ", data->assert_falling_edge, data->capture_clear);
> +#endif
Ditto.
> }
>
> /* 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);
> + data->gpio_pin);
Code is unchanged.
> return ret;
> }
>
> @@ -135,6 +176,19 @@ static int pps_gpio_probe(struct platform_device *pdev)
> return -EINVAL;
> }
>
> +#ifdef GPIO_ECHO
> + ret = devm_gpio_request(&pdev->dev, data->gpio_echo_pin, gpio_echo_label);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to request Echo-GPIO %u\n", data->gpio_echo_pin);
> + return ret;
> + }
> + ret = gpio_direction_output(data->gpio_echo_pin, 0);
> + if (ret) {
> + dev_err(&pdev->dev, "failed to set (echo)pin direction\n");
> + return -EINVAL;
> + }
> +#endif
Ditto.
> +
> /* IRQ setup */
> ret = gpio_to_irq(data->gpio_pin);
> if (ret < 0) {
> @@ -150,13 +204,31 @@ static int pps_gpio_probe(struct platform_device *pdev)
> data->info.mode |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR |
> PPS_ECHOCLEAR;
> data->info.owner = THIS_MODULE;
> +#ifdef DEBUG_PPS
> + dev_err(&pdev->dev, "1)data->info.mode = %x\n", data->info.mode);
> + dev_err(&pdev->dev, "1)data->info.echo = %p\n", (void *)data->info.echo);
> +#endif
> +
> +#ifdef GPIO_ECHO
> + data->info.echo = pps_gpio_echo;// register echo function
> +#endif
> +#ifdef DEBUG_PPS
> + dev_err(&pdev->dev, "2)data->info.echo = %p \n", (void *) data->info.echo);
> +#endif
Ditto.
> +
> snprintf(data->info.name, PPS_MAX_NAME_LEN - 1, "%s.%d",
> pdev->name, pdev->id);
>
> +
Why an empty line here?
> /* register PPS source */
> - pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
> + //pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT;
> + pps_default_params = PPS_CAPTUREASSERT | PPS_OFFSETASSERT | PPS_ECHOASSERT;
Drop not needed code.
> if (data->capture_clear)
> - pps_default_params |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR;
> + //pps_default_params |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR;
> + pps_default_params |= PPS_CAPTURECLEAR | PPS_OFFSETCLEAR | PPS_ECHOCLEAR;
> +#ifdef DEBUG_PPS
> + dev_err(&pdev->dev, "1)pps_default_params = %x\n", pps_default_params);
> +#endif
Ditto.
> data->pps = pps_register_source(&data->info, pps_default_params);
> if (data->pps == NULL) {
> dev_err(&pdev->dev, "failed to register IRQ %d as PPS source\n",
> @@ -172,6 +244,9 @@ static int pps_gpio_probe(struct platform_device *pdev)
> dev_err(&pdev->dev, "failed to acquire IRQ %d\n", data->irq);
> return -EINVAL;
> }
> +#ifdef GPIO_ECHO
> + // Todo: Register Interrupt to reset echo-pin
> +#endif
???
>
> platform_set_drvdata(pdev, data);
> dev_info(data->pps->dev, "Registered IRQ %d as PPS source\n",
> @@ -208,6 +283,6 @@ static struct platform_driver pps_gpio_driver = {
> module_platform_driver(pps_gpio_driver);
> MODULE_AUTHOR("Ricardo Martins <rasm at fe.up.pt>");
> MODULE_AUTHOR("James Nuss <jamesnuss at nanometrics.ca>");
> -MODULE_DESCRIPTION("Use GPIO pin as PPS source");
> +MODULE_DESCRIPTION("Use GPIO pin as PPS source and echo on other GPIO pin");
Maybe is better using "Use GPIO pins as PPS source & echo"
> MODULE_LICENSE("GPL");
> -MODULE_VERSION("1.0.0");
> +MODULE_VERSION("1.0.1");
> diff --git a/linux/arch/arm/boot/dts/pps-gpio-overlay.dts b/linux/arch/arm/boot/dts/pps-gpio-overlay.dts
This file is raspberry specific?
> index 40bf0e1..c8d24f3 100644
> --- a/linux/arch/arm/boot/dts/pps-gpio-overlay.dts
> +++ b/linux/arch/arm/boot/dts/pps-gpio-overlay.dts
> @@ -10,8 +10,10 @@
> compatible = "pps-gpio";
> pinctrl-names = "default";
> pinctrl-0 = <&pps_pins>;
> - gpios = <&gpio 18 0>;
> + gpios = <&gpio 18 0>, <&gpio 21 1>;
Mmm... I'd like the driver will support more that one PPS source so we
should use a configuration settings whose allow us to do so.
To be backward compatibility we can still use something like:
* gpios = <&gpio 18 0>;
to define one PPS source and no echo
* gpios = <&gpio 18 0
&gpio 21 1>;
to define one PPS source and a connected echo
Then, in order to support more PPS sources, we can have something
like:
gpios = <&gpio 18 0
&gpio 21 1
&gpio 1 0
0>;
to define two PPS sources, the first with an echo and the second
without the echo.
> status = "okay";
> + assert-falling-edge;
> + capture-clear;
> };
> };
> };
> @@ -20,15 +22,17 @@
> target = <&gpio>;
> __overlay__ {
> pps_pins: pps_pins {
> - brcm,pins = <18>;
> - brcm,function = <0>; // in
> - brcm,pull = <0>; // off
> + brcm,pins = <18 21>;
> + brcm,function = <0 1>; // in out
> + brcm,pull = <0 0>; // off off
> };
> };
> };
>
> __overrides__ {
> gpiopin = <&pps>,"gpios:4",
> - <&pps_pins>,"brcm,pins:0";
> + <&pps_pins>,"brcm,pins:0";
> + gpioechopin = <&pps>,"gpios:16 ",
> + <&pps_pins>,"brcm,pins:4";
> };
> };
> diff --git a/linux/include/pps-gpio.h b/linux/include/pps-gpio.h
> index 0035abe..7ba609f 100644
> --- a/linux/include/pps-gpio.h
> +++ b/linux/include/pps-gpio.h
> @@ -27,6 +27,8 @@ struct pps_gpio_platform_data {
> bool capture_clear;
> unsigned int gpio_pin;
> const char *gpio_label;
> + unsigned int gpio_echo_pin;
> + const char *gpio_echo_label;
> };
>
> #endif
> _______________________________________________
> discussions mailing list
> discussions at linuxpps.org
> http://www.linuxpps.org/cgi-bin/mailman/listinfo/discussions
> Wiki: http://wiki.enneenne.com/index.php/LinuxPPS_support
--
HCE Engineering e-mail: giometti at hce-engineering.com
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.io
Freelance ICT Italia - Consulente ICT Italia - www.consulenti-ict.it
More information about the discussions
mailing list