lkml.org 
[lkml]   [2024]   [Apr]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed
From


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

\
 
 \ /
  Last update: 2024-05-27 16:33    [W:0.061 / U:2.716 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site