[PATCH] pps-gpio: use IRQ edge config when not capturing both edges

[PATCH] pps-gpio: use IRQ edge config when not capturing both edges

[Tim Harvey]

PPS signals with very short pulse-widths can be missed if their state
changes by the time the interrupt handler reads the GPIO pin state.

To avoid this in the case where we are only looking for one edge we can
use the edge configuration for the pin state but fall back to reading the
pin if both edges are being watched.

Signed-off-by: Tim Harvey <tharvey@gateworks.com>
---
 drivers/pps/clients/pps-gpio.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 333ad7d..0d2b807 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -61,7 +61,16 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
 
 	info = data;
 
-	rising_edge = gpio_get_value(info->gpio_pin);
+	/*
+	 * if not capturing both assert/clear events use the IRQ state
+	 * otherwise read the gpio state from the pin (which could miss
+	 * assertions on very small pulse-widths due to interrupt latency
+	 * and CPU performance).
+	 */
+	if (!info->capture_clear)
+		rising_edge = !info->assert_falling_edge;
+	else
+		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);
-- 
2.7.4

Re: [PATCH] pps-gpio: use IRQ edge config when not capturing both edges

[Rodolfo Giometti]

On 08/09/2017 21:53, Tim Harvey wrote:
> PPS signals with very short pulse-widths can be missed if their state
> changes by the time the interrupt handler reads the GPIO pin state.
> 
> To avoid this in the case where we are only looking for one edge we can
> use the edge configuration for the pin state but fall back to reading the
> pin if both edges are being watched.

I disagree. The "rising_edge" status should be get from the hardware and not 
derived by an empirical computation. Or, at least, it should be specifically 
activated by setting something like this:

         pps {
                 pinctrl-names = "default";
                 pinctrl-0 = <&pinctrl_pps>;

                 gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
		Yes-I-want-get-signal-status-in-an-epirical-way;

                 compatible = "pps-gpio";
                 status = "okay";
         };

This setting should also print a warning in order to be clear for the user that 
he/she should know what he/she is doing.

Then the code should check also the compatibility with property 
"assert-falling-edge"...

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti@hce-engineering.it
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@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

Re: [PATCH] pps-gpio: use IRQ edge config when not capturing both edges

[Tim Harvey]

On Sun, Sep 10, 2017 at 3:43 AM, Rodolfo Giometti <giometti@enneenne.com> wrote:
> On 08/09/2017 21:53, Tim Harvey wrote:
>>
>> PPS signals with very short pulse-widths can be missed if their state
>> changes by the time the interrupt handler reads the GPIO pin state.
>>
>> To avoid this in the case where we are only looking for one edge we can
>> use the edge configuration for the pin state but fall back to reading the
>> pin if both edges are being watched.
>
>
> I disagree. The "rising_edge" status should be get from the hardware and not
> derived by an empirical computation. Or, at least, it should be specifically
> activated by setting something like this:
>
>         pps {
>                 pinctrl-names = "default";
>                 pinctrl-0 = <&pinctrl_pps>;
>
>                 gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
>                 Yes-I-want-get-signal-status-in-an-epirical-way;
>
>                 compatible = "pps-gpio";
>                 status = "okay";
>         };
>
> This setting should also print a warning in order to be clear for the user
> that he/she should know what he/she is doing.
>
> Then the code should check also the compatibility with property
> "assert-falling-edge"...
>

Hi Rodolfo,

Do you agree with using the irq edge in general if/when it is
available to resolve the case where small pulse-widths can be caught?

I assumed because pps-gpio is the one configuring the irq based on
info->capture_clear and info->assert_falling_edge that that it made
sense to use that logic again when handling the interrupt but there is
likely a call I can make to determine the irq (edge) type based on the
irq.

Tim

Re: [PATCH] pps-gpio: use IRQ edge config when not capturing both edges

[Rodolfo Giometti]

On 12/09/2017 17:54, Tim Harvey wrote:
> On Sun, Sep 10, 2017 at 3:43 AM, Rodolfo Giometti <giometti@enneenne.com> wrote:
>> On 08/09/2017 21:53, Tim Harvey wrote:
>>>
>>> PPS signals with very short pulse-widths can be missed if their state
>>> changes by the time the interrupt handler reads the GPIO pin state.
>>>
>>> To avoid this in the case where we are only looking for one edge we can
>>> use the edge configuration for the pin state but fall back to reading the
>>> pin if both edges are being watched.
>>
>>
>> I disagree. The "rising_edge" status should be get from the hardware and not
>> derived by an empirical computation. Or, at least, it should be specifically
>> activated by setting something like this:
>>
>>          pps {
>>                  pinctrl-names = "default";
>>                  pinctrl-0 = <&pinctrl_pps>;
>>
>>                  gpios = <&gpio1 26 GPIO_ACTIVE_HIGH>;
>>                  Yes-I-want-get-signal-status-in-an-epirical-way;
>>
>>                  compatible = "pps-gpio";
>>                  status = "okay";
>>          };
>>
>> This setting should also print a warning in order to be clear for the user
>> that he/she should know what he/she is doing.
>>
>> Then the code should check also the compatibility with property
>> "assert-falling-edge"...
>>
> 
> Hi Rodolfo,
> 
> Do you agree with using the irq edge in general if/when it is
> available to resolve the case where small pulse-widths can be caught?
> 
> I assumed because pps-gpio is the one configuring the irq based on
> info->capture_clear and info->assert_falling_edge that that it made
> sense to use that logic again when handling the interrupt but there is
> likely a call I can make to determine the irq (edge) type based on the
> irq.

If you get the information from the hardware it's OK for me, otherwise you 
should enable this behavior by using proper DT property.

Ciao,

Rodolfo

-- 

HCE Engineering                      e-mail: giometti@hce-engineering.it
GNU/Linux Solutions                          giometti@enneenne.com
Linux Device Driver                          giometti@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