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

[Bastien Curutchet <bastien.curutchet@bootlin.com>]

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