diff mbox series

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

Message ID 20230625142134.33690-3-farbere@amazon.com
State New
Headers
Series Add PPS pulse-width support |

Commit Message

Farber, Eliav June 25, 2023, 2:21 p.m. UTC 
  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 ++++
 1 file changed, 4 insertions(+)

Comments

Krzysztof Kozlowski June 25, 2023, 3:46 p.m. UTC | #1 
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.

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

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.

Best regards,
Krzysztof
Farber, Eliav June 28, 2023, 8:47 a.m. UTC | #2 
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
Krzysztof Kozlowski July 1, 2023, 8:34 a.m. UTC | #3 
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@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").

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
diff mbox series

Patch

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).
 
 Example:
 	pps {