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@xxxxxxxxxxx>
> > > ---
> > >   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@xxxxxxxxxxxx
Linux Device Driver                          giometti@xxxxxxxx
Embedded Systems                     phone:  +39 349 2432127
UNIX programming