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

[Krzysztof Kozlowski]

On 28/06/2023 10:47, Farber, Eliav wrote:
> 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@xxxxxxxxxx>
>>> ---
>>>  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").

I know the history and question was not about it. You add DT support, so
whatever board files were doing, does not matter really.

> 
> 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.

The binding still describes actual hardware - there is a system with a
PPS signal on a GPIO.

Your reasoning for this property is because you need it:
"It is because there is no place in the pps-gpio driver that sets"
With this approach we would need to accept every weird or bogus
property, because someone needs it for the driver.

Bindings are for the hardware, even if the concept is a bit more
software-driven.

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

module params are also disliked, so usually the answer for non-hardware
properties is sysfs ABI.

Whether this is hardware property or not, I don't know. You did not
provide rationale supporting it, so I tend to look at this as candidate
for sysfs.

> 
>> 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.

Sure, poor examples like to multiply. For assert-falling-edge, it looks
like some fake property was added instead of using proper, existing
properties indicating the polarity of GPIO and/or type of interrupt
(rising/falling edge).

Best regards,
Krzysztof