Re: [PATCH v14 3/3] pps: pps-gpio pps-echo implementation

2019-01-04 Thread Rodolfo Giometti

On 30/12/2018 09:33, Tom Burkart wrote:

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.


[snip]


+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;


Maybe you can write as below and saving two lines and having better readability:

struct pps_gpio_device_data *info = data;


+   switch (event) {
+   case PPS_CAPTUREASSERT:
+   if (pps->params.mode & PPS_ECHOASSERT)
+   gpiod_set_value(info->echo_pin, 1);
+   break;
+
+   case PPS_CAPTURECLEAR:
+   if (pps->params.mode & PPS_ECHOCLEAR)
+   gpiod_set_value(info->echo_pin, 1);
+   break;
+   }
+
+   /* fire the timer */
+   if (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) {
+   info->echo_timer.expires = jiffies + info->echo_timeout;
+   add_timer(>echo_timer);
+   }
+}


I think is better firing the timer if and only if we set the echo GPIO otherwise 
it's useless...


Ciao,

Rodolfo

--
GNU/Linux Solutions  e-mail: giome...@enneenne.com
Linux Device Driver  giome...@linux.it
Embedded Systems phone:  +39 349 2432127
UNIX programming skype:  rodolfo.giometti


[PATCH v14 3/3] pps: pps-gpio pps-echo implementation

2018-12-30 Thread 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 
Signed-off-by: Tom Burkart 
---
 drivers/pps/clients/pps-gpio.c | 89 --
 include/linux/pps-gpio.h   |  2 +
 2 files changed, 88 insertions(+), 3 deletions(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 4e5e9229814b..f00f5ca9f9ad 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -35,6 +35,8 @@
 #include 
 #include 
 #include 
+#include 
+#include 
 
 /* Info for each registered platform device */
 struct pps_gpio_device_data {
@@ -42,8 +44,12 @@ 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;
+   unsigned int echo_active_ms;/* PPS echo active duration */
+   unsigned long echo_timeout; /* timer timeout value in jiffies */
 };
 
 /*
@@ -64,19 +70,57 @@ 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, , PPS_CAPTUREASSERT, NULL);
+   pps_event(info->pps, , PPS_CAPTUREASSERT, data);
else if (info->capture_clear &&
((rising_edge && info->assert_falling_edge) ||
(!rising_edge && !info->assert_falling_edge)))
-   pps_event(info->pps, , PPS_CAPTURECLEAR, NULL);
+   pps_event(info->pps, , 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, 1);
+   break;
+
+   case PPS_CAPTURECLEAR:
+   if (pps->params.mode & PPS_ECHOCLEAR)
+   gpiod_set_value(info->echo_pin, 1);
+   break;
+   }
+
+   /* fire the timer */
+   if (info->pps->params.mode & (PPS_ECHOASSERT | PPS_ECHOCLEAR)) {
+   info->echo_timer.expires = jiffies + info->echo_timeout;
+   add_timer(>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, 0);
+}
+
 static int pps_gpio_setup(struct platform_device *pdev)
 {
struct pps_gpio_device_data *data = platform_get_drvdata(pdev);
struct device_node *np = pdev->dev.of_node;
+   int ret;
+   u32 value;
 
data->gpio_pin = devm_gpiod_get(>dev,
NULL,   /* request "gpios" */
@@ -87,6 +131,33 @@ static int pps_gpio_setup(struct platform_device *pdev)
return PTR_ERR(data->gpio_pin);
}
 
+   data->echo_pin = devm_gpiod_get_optional(>dev,
+   "echo",
+   GPIOD_OUT_LOW);
+   if (data->echo_pin) {
+   if (IS_ERR(data->echo_pin)) {
+   dev_err(>dev, "failed to request ECHO GPIO\n");
+   return PTR_ERR(data->echo_pin);
+   }
+
+   ret = of_property_read_u32(np,
+   "echo-active-ms",
+   );
+   if (ret) {
+   dev_err(>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(>dev,
+   "echo-active-ms: %u - bad value from OF\n",
+   data->echo_active_ms);
+   return -EINVAL;
+   }
+   }
+
if (of_property_read_bool(np, "assert-falling-edge"))
data->assert_falling_edge = true;
return 0;
@@ -122,9 +193,11 @@ static int