Re: [PATCH 2/5] dt-bindings: pps: pps-gpio: introduce capture-clear property

["Farber, Eliav" <farbere@amazon.com>]

On 6/25/2023 6:46 PM, Krzysztof Kozlowski wrote:
> On 25/06/2023 16:21, Eliav Farber wrote:
>> Add a new optional "capture-clear" boolean property.
>> When present, PPS clear events shall also be reported.
>>
>> Signed-off-by: Eliav Farber <farbere@amazon.com>
>> ---
>>  Documentation/devicetree/bindings/pps/pps-gpio.txt | 4 ++++
>
> Please convert the bindings to DT schema first.
I will convert to DT schema first, if I indeed end up modifying this file.

>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/pps/pps-gpio.txt 
>> b/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> index 9012a2a02e14..8d588e38c44e 100644
>> --- a/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> +++ b/Documentation/devicetree/bindings/pps/pps-gpio.txt
>> @@ -14,6 +14,10 @@ Additional required properties for the PPS ECHO 
>> functionality:
>>  Optional properties:
>>  - assert-falling-edge: when present, assert is indicated by a 
>> falling edge
>>                         (instead of by a rising edge)
>> +- capture-clear: when present, report also PPS clear events, which 
>> is the
>> +                 opposite of the assert edge (if assert is 
>> rising-edge then
>> +                 clear is falling-edge and if assert is falling-edge 
>> then
>> +                 clear is rising-edge).
>
> Why this is board-dependant? Falling edge is happening anyway, so it is
> in the hardware all the time. DT describes the hardware, not Linux
> driver behavior.
Falling edge of the pulse is happening all the time, but the falling
edge event is currently never reported by the pps-gpio driver.

It is because there is no place in the pps-gpio driver that sets
info->capture_clear, so
   pps_event(info->pps, &ts, PPS_CAPTURECLEAR, data);
will never be called.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pps/clients/pps-gpio.c?h=v6.4#n59

There was an option in the past to set info->capture_clear, but that
option was removed in commit ee89646619ba ("pps: clients: gpio: Get rid
of legacy platform data").

This node in the DT isn't a pure HW device, but rather a SW driver.
The patch I shared allows to set capture-clear from device-tree same as
setting of assert-falling-edge is done.

Do you suggest I enable capture_clear in a different way?
Perhaps module-param?

> What's more, your property name sounds a lot like a driver property, so
> even if this is suitable for DT, you would need to name it properly to
> match hardware feature/characteristic.
I chose capture-clear as a name since it is aligned with the driver's
terminology. It sets the capture_clear parameter, just like
assert-falling-edge in DT sets assert_falling_edge parameter.

---
Regards, Eliav