Messages in this thread | | | Date | Wed, 10 Apr 2024 18:05:40 +0200 | Subject | Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed | From | 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
| |