[PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Bastien Curutchet]

In the IRQ handler, the GPIO's state is read to verify the direction of
the edge that triggered the interruption before generating the PPS event.
If a pulse is too short, the GPIO line can reach back its original state
before this verification and the PPS event is lost.

This check is needed when info->capture_clear is set because it needs
interruptions on both rising and falling edges. When info->capture_clear
is not set, interruption is triggered by one edge only so this check can
be omitted.

Bypass the edge's direction verification when info->capture_clear is not
set.

Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
---
 drivers/pps/clients/pps-gpio.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 2f4b11b4dfcd..c2a96e3e3836 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -52,6 +52,15 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
 
 	info = data;
 
+	if (!info->capture_clear) {
+		/*
+		 * If capture_clear is unset, IRQ is triggered by one edge only.
+		 * So the check on edge direction is not needed here
+		 */
+		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
+		return IRQ_HANDLED;
+	}
+
 	rising_edge = gpiod_get_value(info->gpio_pin);
 	if ((rising_edge && !info->assert_falling_edge) ||
 			(!rising_edge && info->assert_falling_edge))
-- 
2.44.0

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Rodolfo Giometti]

On 10/04/24 13:35, Bastien Curutchet wrote:
> In the IRQ handler, the GPIO's state is read to verify the direction of
> the edge that triggered the interruption before generating the PPS event.
> If a pulse is too short, the GPIO line can reach back its original state
> before this verification and the PPS event is lost.
> 
> This check is needed when info->capture_clear is set because it needs
> interruptions on both rising and falling edges. When info->capture_clear
> is not set, interruption is triggered by one edge only so this check can
> be omitted.
> 
> Bypass the edge's direction verification when info->capture_clear is not
> set.
> 
> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
> ---
>   drivers/pps/clients/pps-gpio.c | 9 +++++++++
>   1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
> index 2f4b11b4dfcd..c2a96e3e3836 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -52,6 +52,15 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
>   
>   	info = data;
>   
> +	if (!info->capture_clear) {
> +		/*
> +		 * If capture_clear is unset, IRQ is triggered by one edge only.
> +		 * So the check on edge direction is not needed here
> +		 */
> +		pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
> +		return IRQ_HANDLED;
> +	}
> +
>   	rising_edge = gpiod_get_value(info->gpio_pin);
>   	if ((rising_edge && !info->assert_falling_edge) ||
>   			(!rising_edge && info->assert_falling_edge))

Apart the code duplication, which are the real benefits of doing so?

Rodolfo

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

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Bastien Curutchet]

Hi Rodolfo,

On 4/10/24 16:23, Rodolfo Giometti wrote:
> On 10/04/24 13:35, Bastien Curutchet wrote:
>> In the IRQ handler, the GPIO's state is read to verify the direction of
>> the edge that triggered the interruption before generating the PPS event.
>> If a pulse is too short, the GPIO line can reach back its original state
>> before this verification and the PPS event is lost.
>>
>> This check is needed when info->capture_clear is set because it needs
>> interruptions on both rising and falling edges. When info->capture_clear
>> is not set, interruption is triggered by one edge only so this check can
>> be omitted.
>>
>> Bypass the edge's direction verification when info->capture_clear is not
>> set.
>>
>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>> ---
>>   drivers/pps/clients/pps-gpio.c | 9 +++++++++
>>   1 file changed, 9 insertions(+)
>>
>> diff --git a/drivers/pps/clients/pps-gpio.c 
>> b/drivers/pps/clients/pps-gpio.c
>> index 2f4b11b4dfcd..c2a96e3e3836 100644
>> --- a/drivers/pps/clients/pps-gpio.c
>> +++ b/drivers/pps/clients/pps-gpio.c
>> @@ -52,6 +52,15 @@ static irqreturn_t pps_gpio_irq_handler(int irq, 
>> void *data)
>>       info = data;
>> +    if (!info->capture_clear) {
>> +        /*
>> +         * If capture_clear is unset, IRQ is triggered by one edge only.
>> +         * So the check on edge direction is not needed here
>> +         */
>> +        pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>> +        return IRQ_HANDLED;
>> +    }
>> +
>>       rising_edge = gpiod_get_value(info->gpio_pin);
>>       if ((rising_edge && !info->assert_falling_edge) ||
>>               (!rising_edge && info->assert_falling_edge))
> 
> Apart the code duplication, which are the real benefits of doing so?
> 

It prevents from losing a PPS event when the pulse is so short (or the
kernel so busy) that the trailing edge of the pulse occurs before the
interrupt handler can read the state of the GPIO pin.


Best regards,
Bastien

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Rodolfo Giometti]

On 10/04/24 16:46, Bastien Curutchet wrote:
> Hi Rodolfo,
> 
> On 4/10/24 16:23, Rodolfo Giometti wrote:
>> On 10/04/24 13:35, Bastien Curutchet wrote:
>>> In the IRQ handler, the GPIO's state is read to verify the direction of
>>> the edge that triggered the interruption before generating the PPS event.
>>> If a pulse is too short, the GPIO line can reach back its original state
>>> before this verification and the PPS event is lost.
>>>
>>> This check is needed when info->capture_clear is set because it needs
>>> interruptions on both rising and falling edges. When info->capture_clear
>>> is not set, interruption is triggered by one edge only so this check can
>>> be omitted.
>>>
>>> Bypass the edge's direction verification when info->capture_clear is not
>>> set.
>>>
>>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>>> ---
>>>   drivers/pps/clients/pps-gpio.c | 9 +++++++++
>>>   1 file changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
>>> index 2f4b11b4dfcd..c2a96e3e3836 100644
>>> --- a/drivers/pps/clients/pps-gpio.c
>>> +++ b/drivers/pps/clients/pps-gpio.c
>>> @@ -52,6 +52,15 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
>>>       info = data;
>>> +    if (!info->capture_clear) {
>>> +        /*
>>> +         * If capture_clear is unset, IRQ is triggered by one edge only.
>>> +         * So the check on edge direction is not needed here
>>> +         */
>>> +        pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>>> +        return IRQ_HANDLED;
>>> +    }
>>> +
>>>       rising_edge = gpiod_get_value(info->gpio_pin);
>>>       if ((rising_edge && !info->assert_falling_edge) ||
>>>               (!rising_edge && info->assert_falling_edge))
>>
>> Apart the code duplication, which are the real benefits of doing so?
>>
> 
> It prevents from losing a PPS event when the pulse is so short (or the
> kernel so busy) that the trailing edge of the pulse occurs before the
> interrupt handler can read the state of the GPIO pin.

Have you a real case when this happens?

In any cases we should avoid code duplication... so I think we should do 
something as below:

diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
index 2f4b11b4dfcd..f05fb15ed7f4 100644
--- a/drivers/pps/clients/pps-gpio.c
+++ b/drivers/pps/clients/pps-gpio.c
@@ -52,7 +52,9 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)

         info = data;

-       rising_edge = gpiod_get_value(info->gpio_pin);
+       rising_edge = info->capture_clear ? \
+                       gpiod_get_value(info->gpio_pin) : \
+                       !info->assert_falling_edge;
         if ((rising_edge && !info->assert_falling_edge) ||
                         (!rising_edge && info->assert_falling_edge))
                 pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);

Please, review and test it before resubmitting. :)

Ciao,

Rodolfo

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

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Bastien Curutchet]

On 4/10/24 17:24, Rodolfo Giometti wrote:
> On 10/04/24 16:46, Bastien Curutchet wrote:
>> Hi Rodolfo,
>>
>> On 4/10/24 16:23, Rodolfo Giometti wrote:
>>> On 10/04/24 13:35, Bastien Curutchet wrote:
>>>> In the IRQ handler, the GPIO's state is read to verify the direction of
>>>> the edge that triggered the interruption before generating the PPS 
>>>> event.
>>>> If a pulse is too short, the GPIO line can reach back its original 
>>>> state
>>>> before this verification and the PPS event is lost.
>>>>
>>>> This check is needed when info->capture_clear is set because it needs
>>>> interruptions on both rising and falling edges. When 
>>>> info->capture_clear
>>>> is not set, interruption is triggered by one edge only so this check 
>>>> can
>>>> be omitted.
>>>>
>>>> Bypass the edge's direction verification when info->capture_clear is 
>>>> not
>>>> set.
>>>>
>>>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>>>> ---
>>>>   drivers/pps/clients/pps-gpio.c | 9 +++++++++
>>>>   1 file changed, 9 insertions(+)
>>>>
>>>> diff --git a/drivers/pps/clients/pps-gpio.c 
>>>> b/drivers/pps/clients/pps-gpio.c
>>>> index 2f4b11b4dfcd..c2a96e3e3836 100644
>>>> --- a/drivers/pps/clients/pps-gpio.c
>>>> +++ b/drivers/pps/clients/pps-gpio.c
>>>> @@ -52,6 +52,15 @@ static irqreturn_t pps_gpio_irq_handler(int irq, 
>>>> void *data)
>>>>       info = data;
>>>> +    if (!info->capture_clear) {
>>>> +        /*
>>>> +         * If capture_clear is unset, IRQ is triggered by one edge 
>>>> only.
>>>> +         * So the check on edge direction is not needed here
>>>> +         */
>>>> +        pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>>>> +        return IRQ_HANDLED;
>>>> +    }
>>>> +
>>>>       rising_edge = gpiod_get_value(info->gpio_pin);
>>>>       if ((rising_edge && !info->assert_falling_edge) ||
>>>>               (!rising_edge && info->assert_falling_edge))
>>>
>>> Apart the code duplication, which are the real benefits of doing so?
>>>
>>
>> It prevents from losing a PPS event when the pulse is so short (or the
>> kernel so busy) that the trailing edge of the pulse occurs before the
>> interrupt handler can read the state of the GPIO pin.
> 
> Have you a real case when this happens?
> 

Yes, on my use case, a GPS provides a tiny pulse (~10 us) that is
sometimes missed when CPU is very busy.

> In any cases we should avoid code duplication... so I think we should do 
> something as below:
> 
> diff --git a/drivers/pps/clients/pps-gpio.c 
> b/drivers/pps/clients/pps-gpio.c
> index 2f4b11b4dfcd..f05fb15ed7f4 100644
> --- a/drivers/pps/clients/pps-gpio.c
> +++ b/drivers/pps/clients/pps-gpio.c
> @@ -52,7 +52,9 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void 
> *data)
> 
>          info = data;
> 
> -       rising_edge = gpiod_get_value(info->gpio_pin);
> +       rising_edge = info->capture_clear ? \
> +                       gpiod_get_value(info->gpio_pin) : \
> +                       !info->assert_falling_edge;
>          if ((rising_edge && !info->assert_falling_edge) ||
>                          (!rising_edge && info->assert_falling_edge))
>                  pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
> 
> Please, review and test it before resubmitting. :)
> 

I'll try this and send a V2 after my tests, thank you.

Best regards,
Bastien

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Rodolfo Giometti]

On 10/04/24 18:05, Bastien Curutchet wrote:
> 
> 
> On 4/10/24 17:24, Rodolfo Giometti wrote:
>> On 10/04/24 16:46, Bastien Curutchet wrote:
>>> Hi Rodolfo,
>>>
>>> On 4/10/24 16:23, Rodolfo Giometti wrote:
>>>> On 10/04/24 13:35, Bastien Curutchet wrote:
>>>>> In the IRQ handler, the GPIO's state is read to verify the direction of
>>>>> the edge that triggered the interruption before generating the PPS event.
>>>>> If a pulse is too short, the GPIO line can reach back its original state
>>>>> before this verification and the PPS event is lost.
>>>>>
>>>>> This check is needed when info->capture_clear is set because it needs
>>>>> interruptions on both rising and falling edges. When info->capture_clear
>>>>> is not set, interruption is triggered by one edge only so this check can
>>>>> be omitted.
>>>>>
>>>>> Bypass the edge's direction verification when info->capture_clear is not
>>>>> set.
>>>>>
>>>>> Signed-off-by: Bastien Curutchet <bastien.curutchet@bootlin.com>
>>>>> ---
>>>>>   drivers/pps/clients/pps-gpio.c | 9 +++++++++
>>>>>   1 file changed, 9 insertions(+)
>>>>>
>>>>> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
>>>>> index 2f4b11b4dfcd..c2a96e3e3836 100644
>>>>> --- a/drivers/pps/clients/pps-gpio.c
>>>>> +++ b/drivers/pps/clients/pps-gpio.c
>>>>> @@ -52,6 +52,15 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void 
>>>>> *data)
>>>>>       info = data;
>>>>> +    if (!info->capture_clear) {
>>>>> +        /*
>>>>> +         * If capture_clear is unset, IRQ is triggered by one edge only.
>>>>> +         * So the check on edge direction is not needed here
>>>>> +         */
>>>>> +        pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>>>>> +        return IRQ_HANDLED;
>>>>> +    }
>>>>> +
>>>>>       rising_edge = gpiod_get_value(info->gpio_pin);
>>>>>       if ((rising_edge && !info->assert_falling_edge) ||
>>>>>               (!rising_edge && info->assert_falling_edge))
>>>>
>>>> Apart the code duplication, which are the real benefits of doing so?
>>>>
>>>
>>> It prevents from losing a PPS event when the pulse is so short (or the
>>> kernel so busy) that the trailing edge of the pulse occurs before the
>>> interrupt handler can read the state of the GPIO pin.
>>
>> Have you a real case when this happens?
>>
> 
> Yes, on my use case, a GPS provides a tiny pulse (~10 us) that is
> sometimes missed when CPU is very busy.

I see...

>> In any cases we should avoid code duplication... so I think we should do 
>> something as below:
>>
>> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
>> index 2f4b11b4dfcd..f05fb15ed7f4 100644
>> --- a/drivers/pps/clients/pps-gpio.c
>> +++ b/drivers/pps/clients/pps-gpio.c
>> @@ -52,7 +52,9 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
>>
>>          info = data;
>>
>> -       rising_edge = gpiod_get_value(info->gpio_pin);
>> +       rising_edge = info->capture_clear ? \
>> +                       gpiod_get_value(info->gpio_pin) : \
>> +                       !info->assert_falling_edge;
>>          if ((rising_edge && !info->assert_falling_edge) ||
>>                          (!rising_edge && info->assert_falling_edge))
>>                  pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>>
>> Please, review and test it before resubmitting. :)
>>
> 
> I'll try this and send a V2 after my tests, thank you.

OK, thanks.

However we should think very well about this modification since it could be the 
case where we have a device sending both assert and clear events but we wish to 
catch just the asserts... in this case we will get doubled asserts!

Maybe, can we add a special flag within the DTS (something as 
"support-tiny-pulses" or something like that) to specify that we are in this 
special condition and then checking this setting against capture_clear flag?

Ciao,

Rodolfo

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

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Bastien Curutchet]

Hi Rodolfo

>>> diff --git a/drivers/pps/clients/pps-gpio.c 
>>> b/drivers/pps/clients/pps-gpio.c
>>> index 2f4b11b4dfcd..f05fb15ed7f4 100644
>>> --- a/drivers/pps/clients/pps-gpio.c
>>> +++ b/drivers/pps/clients/pps-gpio.c
>>> @@ -52,7 +52,9 @@ static irqreturn_t pps_gpio_irq_handler(int irq, 
>>> void *data)
>>>
>>>          info = data;
>>>
>>> -       rising_edge = gpiod_get_value(info->gpio_pin);
>>> +       rising_edge = info->capture_clear ? \
>>> +                       gpiod_get_value(info->gpio_pin) : \
>>> +                       !info->assert_falling_edge;
>>>          if ((rising_edge && !info->assert_falling_edge) ||
>>>                          (!rising_edge && info->assert_falling_edge))
>>>                  pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>>>
>>> Please, review and test it before resubmitting. :)
>>>
>>
>> I'll try this and send a V2 after my tests, thank you.
> 
> OK, thanks.
> 
> However we should think very well about this modification since it could 
> be the case where we have a device sending both assert and clear events 
> but we wish to catch just the asserts... in this case we will get 
> doubled asserts!
> 

My understanding is that clear events are to be captured only when this
capture_clear boolean is set. If it is not set, the PPS_CAPTURECLEAR
flag is not added to pps_source_info->mode and get_irqf_trigger_flags()
will return only one edge flag (rising or falling depending on
assert-falling-edge DT property).

By the way, I see that the capture_clear is never set since the legacy
platform data support has been dropped (commit ee89646619ba).


Best regards,
Bastien

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Rodolfo Giometti]

On 11/04/24 14:44, Bastien Curutchet wrote:
> Hi Rodolfo
> 
>>>> diff --git a/drivers/pps/clients/pps-gpio.c b/drivers/pps/clients/pps-gpio.c
>>>> index 2f4b11b4dfcd..f05fb15ed7f4 100644
>>>> --- a/drivers/pps/clients/pps-gpio.c
>>>> +++ b/drivers/pps/clients/pps-gpio.c
>>>> @@ -52,7 +52,9 @@ static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
>>>>
>>>>          info = data;
>>>>
>>>> -       rising_edge = gpiod_get_value(info->gpio_pin);
>>>> +       rising_edge = info->capture_clear ? \
>>>> +                       gpiod_get_value(info->gpio_pin) : \
>>>> +                       !info->assert_falling_edge;
>>>>          if ((rising_edge && !info->assert_falling_edge) ||
>>>>                          (!rising_edge && info->assert_falling_edge))
>>>>                  pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
>>>>
>>>> Please, review and test it before resubmitting. :)
>>>>
>>>
>>> I'll try this and send a V2 after my tests, thank you.
>>
>> OK, thanks.
>>
>> However we should think very well about this modification since it could be 
>> the case where we have a device sending both assert and clear events but we 
>> wish to catch just the asserts... in this case we will get doubled asserts!
>>
> 
> My understanding is that clear events are to be captured only when this
> capture_clear boolean is set. If it is not set, the PPS_CAPTURECLEAR
> flag is not added to pps_source_info->mode and get_irqf_trigger_flags()
> will return only one edge flag (rising or falling depending on
> assert-falling-edge DT property).

Yes. You are right.

> By the way, I see that the capture_clear is never set since the legacy
> platform data support has been dropped (commit ee89646619ba).

I see, but it can be re-enabled in the future... In this scenario, I think we 
should add a DT entry to enable this special behavior. Maybe we can also add a 
warning as below:

static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
{
         ...
         if ((rising_edge && !info->assert_falling_edge) ||
                         (!rising_edge && info->assert_falling_edge))
                 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, data);
	else
		dev_warn_ratelimited(dev, "no ASSERT or CAPTURE event? "
			"Maybe you need support-tiny-assert-pulse?");

         return IRQ_HANDLED;
}

Ciao,

Rodolfo

P.S. I'm sorry, but I'm not good at finding names... ^_^"

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

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Bastien Curutchet]

Hi Rodolfo,

On 4/12/24 08:44, Rodolfo Giometti wrote:
> On 11/04/24 14:44, Bastien Curutchet wrote:
>>>
>>> However we should think very well about this modification since it 
>>> could be the case where we have a device sending both assert and 
>>> clear events but we wish to catch just the asserts... in this case we 
>>> will get doubled asserts!
>>>
>>
>> My understanding is that clear events are to be captured only when this
>> capture_clear boolean is set. If it is not set, the PPS_CAPTURECLEAR
>> flag is not added to pps_source_info->mode and get_irqf_trigger_flags()
>> will return only one edge flag (rising or falling depending on
>> assert-falling-edge DT property).
> 
> Yes. You are right.
> 
>> By the way, I see that the capture_clear is never set since the legacy
>> platform data support has been dropped (commit ee89646619ba).
> 
> I see, but it can be re-enabled in the future... In this scenario, I 
> think we should add a DT entry to enable this special behavior. Maybe we 
> can also add a warning as below: >
> static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
> {
>          ...
>          if ((rising_edge && !info->assert_falling_edge) ||
>                          (!rising_edge && info->assert_falling_edge))
>                  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, data);
>      else
>          dev_warn_ratelimited(dev, "no ASSERT or CAPTURE event? "
>              "Maybe you need support-tiny-assert-pulse?");
> 
>          return IRQ_HANDLED;
> }
> 

I'm not sure a DT entry is needed. IMO there are two cases:
  1) capture_clear is unset. We need to capture only assert events,
     interrupt will be triggered by assert edge only so there is no need
     to check GPIO state: we can use the bypass.
  2) capture_clear is set. We need to capture assert and/or clear
     events, interrupt will be triggered by both assert and clear edges
     so we can't avoid the GPIO state checking to distinguish clear
     events from assert events: we can't use the bypass.

So if we bypass the GPIO's state check when capture_clear is unset and
leave current behavior when capture_clear is set:
  - case 1) will be more efficient and we won't lose tiny pulses anymore
  - case 2) is unchanged: we still might lose tiny pulses but as bypass
can't be done here, I think that we can't do better.

I agree that adding warning when the handler is left without triggering
a pps event can be useful, I can add it in a V3 version.


Best regards,
Bastien

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Bastien Curutchet]

Hi Rodolfo,


On 4/12/24 14:20, Bastien Curutchet wrote:
> Hi Rodolfo,
> 
> On 4/12/24 08:44, Rodolfo Giometti wrote:
>> On 11/04/24 14:44, Bastien Curutchet wrote:
>>>>
>>>> However we should think very well about this modification since it 
>>>> could be the case where we have a device sending both assert and 
>>>> clear events but we wish to catch just the asserts... in this case 
>>>> we will get doubled asserts!
>>>>
>>>
>>> My understanding is that clear events are to be captured only when this
>>> capture_clear boolean is set. If it is not set, the PPS_CAPTURECLEAR
>>> flag is not added to pps_source_info->mode and get_irqf_trigger_flags()
>>> will return only one edge flag (rising or falling depending on
>>> assert-falling-edge DT property).
>>
>> Yes. You are right.
>>
>>> By the way, I see that the capture_clear is never set since the legacy
>>> platform data support has been dropped (commit ee89646619ba).
>>
>> I see, but it can be re-enabled in the future... In this scenario, I 
>> think we should add a DT entry to enable this special behavior. Maybe 
>> we can also add a warning as below: >
>> static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
>> {
>>          ...
>>          if ((rising_edge && !info->assert_falling_edge) ||
>>                          (!rising_edge && info->assert_falling_edge))
>>                  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, data);
>>      else
>>          dev_warn_ratelimited(dev, "no ASSERT or CAPTURE event? "
>>              "Maybe you need support-tiny-assert-pulse?");
>>
>>          return IRQ_HANDLED;
>> }
>>
> 
> I'm not sure a DT entry is needed. IMO there are two cases:
>   1) capture_clear is unset. We need to capture only assert events,
>      interrupt will be triggered by assert edge only so there is no need
>      to check GPIO state: we can use the bypass.
>   2) capture_clear is set. We need to capture assert and/or clear
>      events, interrupt will be triggered by both assert and clear edges
>      so we can't avoid the GPIO state checking to distinguish clear
>      events from assert events: we can't use the bypass.
> 
> So if we bypass the GPIO's state check when capture_clear is unset and
> leave current behavior when capture_clear is set:
>   - case 1) will be more efficient and we won't lose tiny pulses anymore
>   - case 2) is unchanged: we still might lose tiny pulses but as bypass
> can't be done here, I think that we can't do better.
> 
> I agree that adding warning when the handler is left without triggering
> a pps event can be useful, I can add it in a V3 version.
> 

Would this be OK for you ? If yes, I'll send a V3 version without DT
entry but with an additional warning.


Best regards,
Bastien

Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed

[Rodolfo Giometti]

On 25/04/24 08:11, Bastien Curutchet wrote:
> Hi Rodolfo,
> 
> 
> On 4/12/24 14:20, Bastien Curutchet wrote:
>> Hi Rodolfo,
>>
>> On 4/12/24 08:44, Rodolfo Giometti wrote:
>>> On 11/04/24 14:44, Bastien Curutchet wrote:
>>>>>
>>>>> However we should think very well about this modification since it could be 
>>>>> the case where we have a device sending both assert and clear events but we 
>>>>> wish to catch just the asserts... in this case we will get doubled asserts!
>>>>>
>>>>
>>>> My understanding is that clear events are to be captured only when this
>>>> capture_clear boolean is set. If it is not set, the PPS_CAPTURECLEAR
>>>> flag is not added to pps_source_info->mode and get_irqf_trigger_flags()
>>>> will return only one edge flag (rising or falling depending on
>>>> assert-falling-edge DT property).
>>>
>>> Yes. You are right.
>>>
>>>> By the way, I see that the capture_clear is never set since the legacy
>>>> platform data support has been dropped (commit ee89646619ba).
>>>
>>> I see, but it can be re-enabled in the future... In this scenario, I think we 
>>> should add a DT entry to enable this special behavior. Maybe we can also add 
>>> a warning as below: >
>>> static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
>>> {
>>>          ...
>>>          if ((rising_edge && !info->assert_falling_edge) ||
>>>                          (!rising_edge && info->assert_falling_edge))
>>>                  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, data);
>>>      else
>>>          dev_warn_ratelimited(dev, "no ASSERT or CAPTURE event? "
>>>              "Maybe you need support-tiny-assert-pulse?");
>>>
>>>          return IRQ_HANDLED;
>>> }
>>>
>>
>> I'm not sure a DT entry is needed. IMO there are two cases:
>>   1) capture_clear is unset. We need to capture only assert events,
>>      interrupt will be triggered by assert edge only so there is no need
>>      to check GPIO state: we can use the bypass.
>>   2) capture_clear is set. We need to capture assert and/or clear
>>      events, interrupt will be triggered by both assert and clear edges
>>      so we can't avoid the GPIO state checking to distinguish clear
>>      events from assert events: we can't use the bypass.
>>
>> So if we bypass the GPIO's state check when capture_clear is unset and
>> leave current behavior when capture_clear is set:
>>   - case 1) will be more efficient and we won't lose tiny pulses anymore
>>   - case 2) is unchanged: we still might lose tiny pulses but as bypass
>> can't be done here, I think that we can't do better.
>>
>> I agree that adding warning when the handler is left without triggering
>> a pps event can be useful, I can add it in a V3 version.
>>
> 
> Would this be OK for you ? If yes, I'll send a V3 version without DT
> entry but with an additional warning.

Sorry, I completely missed this e-mail! :(

OK, I agree.

Ciao,

Rodolfo

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