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

From: Rodolfo Giometti
Date: Fri Apr 12 2024 - 02:47:29 EST


On 11/04/24 14:44, Bastien Curutchet wrote:
Hi Rodolfo

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!


My understanding is that clear events are to be captured only when this
capture_clear boolean is set. If it is not set, the PPS_CAPTURECLEAR
flag is not added to pps_source_info->mode and get_irqf_trigger_flags()
will return only one edge flag (rising or falling depending on
assert-falling-edge DT property).

Yes. You are right.

By the way, I see that the capture_clear is never set since the legacy
platform data support has been dropped (commit ee89646619ba).

I see, but it can be re-enabled in the future... In this scenario, I think we should add a DT entry to enable this special behavior. Maybe we can also add a warning as below:

static irqreturn_t pps_gpio_irq_handler(int irq, void *data)
{
...
if ((rising_edge && !info->assert_falling_edge) ||
(!rising_edge && info->assert_falling_edge))
pps_event(info->pps, &ts, PPS_CAPTUREASSERT, data);
else if (info->capture_clear &&
((rising_edge && info->assert_falling_edge) ||
(!rising_edge && !info->assert_falling_edge)))
pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data);
else
dev_warn_ratelimited(dev, "no ASSERT or CAPTURE event? "
"Maybe you need support-tiny-assert-pulse?");

return IRQ_HANDLED;
}

Ciao,

Rodolfo

P.S. I'm sorry, but I'm not good at finding names... ^_^"

--
GNU/Linux Solutions e-mail: giometti@xxxxxxxxxxxx
Linux Device Driver giometti@xxxxxxxx
Embedded Systems phone: +39 349 2432127
UNIX programming