Messages in this thread | ![/](/images/icornerl.gif) | | Date | Thu, 11 Apr 2024 09:20:44 +0200 | Subject | Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check when not needed | From | 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
| ![\](/images/icornerr.gif) |