Received-SPF: pass (google.com: domain of linux-kernel+bounces-138947-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <27196117-32bc-4892-b545-d9cf43a89f0a@bootlin.com>
Date: Wed, 10 Apr 2024 18:05:40 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 1/1] pps: clients: gpio: Bypass edge's direction check
 when not needed
To: Rodolfo Giometti <giometti@enneenne.com>
Cc: linux-kernel@vger.kernel.org,
 Thomas Petazzoni <thomas.petazzoni@bootlin.com>, herve.codina@bootlin.com,
 christophercordahi@nanometrics.ca
References: <20240410113502.73038-1-bastien.curutchet@bootlin.com>
 <1f7f5b2f-54d4-4dc1-90ff-b896c930faed@enneenne.com>
 <5bda0980-2373-4284-bda4-89f0c6944e76@bootlin.com>
 <eb64ec08-2ae3-48bb-9f84-3cec362280b2@enneenne.com>
Content-Language: en-US
From: Bastien Curutchet <bastien.curutchet@bootlin.com>
In-Reply-To: <eb64ec08-2ae3-48bb-9f84-3cec362280b2@enneenne.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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