Update eQEP binding for TI K3 devices.
Signed-off-by: Judith Mendez <[email protected]>
---
Documentation/devicetree/bindings/counter/ti-eqep.yaml | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.yaml b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
index 85f1ff83afe72..11755074c8a91 100644
--- a/Documentation/devicetree/bindings/counter/ti-eqep.yaml
+++ b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
@@ -14,19 +14,23 @@ properties:
const: ti,am3352-eqep
reg:
- maxItems: 1
+ minItems: 1
+ maxItems: 2
interrupts:
description: The eQEP event interrupt
maxItems: 1
clocks:
- description: The clock that determines the SYSCLKOUT rate for the eQEP
+ description: The clock that determines the clock rate for the eQEP
peripheral.
maxItems: 1
clock-names:
- const: sysclkout
+ maxItems: 1
+
+ power-domains:
+ maxItems: 1
required:
- compatible
--
2.43.2
On 19/04/2024 00:14, Judith Mendez wrote:
> Update eQEP binding for TI K3 devices.
Here and in subject: everything is an update. Be specific.
A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
prefix is already stating that these are bindings.
See also:
https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
>
> Signed-off-by: Judith Mendez <[email protected]>
> ---
> Documentation/devicetree/bindings/counter/ti-eqep.yaml | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.yaml b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
> index 85f1ff83afe72..11755074c8a91 100644
> --- a/Documentation/devicetree/bindings/counter/ti-eqep.yaml
> +++ b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
> @@ -14,19 +14,23 @@ properties:
> const: ti,am3352-eqep
>
> reg:
> - maxItems: 1
> + minItems: 1
> + maxItems: 2
Why? This must be constrained. Devices either have 1 or 2, no both at
the same time.
>
> interrupts:
> description: The eQEP event interrupt
> maxItems: 1
>
> clocks:
> - description: The clock that determines the SYSCLKOUT rate for the eQEP
> + description: The clock that determines the clock rate for the eQEP
> peripheral.
> maxItems: 1
>
> clock-names:
> - const: sysclkout
> + maxItems: 1
NAK. That's just wrong, not explained at all either.
Best regards,
Krzysztof
On 4/18/24 5:14 PM, Judith Mendez wrote:
> Update eQEP binding for TI K3 devices.
It would make more sense to have this patch first in the series
before the dts changes.
>
> Signed-off-by: Judith Mendez <[email protected]>
> ---
> Documentation/devicetree/bindings/counter/ti-eqep.yaml | 10 +++++++---
> 1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.yaml b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
> index 85f1ff83afe72..11755074c8a91 100644
> --- a/Documentation/devicetree/bindings/counter/ti-eqep.yaml
> +++ b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
> @@ -14,19 +14,23 @@ properties:
> const: ti,am3352-eqep
>
As Krzysztof hinted, it sounds like we need to add new compatibles
here and have some -if: statements to account for the differences
in SoCs rather than making the bindings less strict.
> reg:
> - maxItems: 1
> + minItems: 1
> + maxItems: 2
>
> interrupts:
> description: The eQEP event interrupt
> maxItems: 1
>
> clocks:
> - description: The clock that determines the SYSCLKOUT rate for the eQEP
> + description: The clock that determines the clock rate for the eQEP
> peripheral.
> maxItems: 1
>
> clock-names:
> - const: sysclkout
> + maxItems: 1
In hindsight, this is not the best name. Since we only have one clock
we don't really need the name anyway, so for the new compatibles, we
could set clock-names: false.
> +
> + power-domains:
> + maxItems: 1
>
> required:
> - compatible
I see that the CFG0 syscon register on AM62x has some control knobs for
the EQEP so it would be good to add this to the bindings now too to try
to make the bindings as complete as possible. (I didn't look at other
chips so the same may apply to others as well.)
On 4/19/24 8:55 AM, Krzysztof Kozlowski wrote:
> On 19/04/2024 00:14, Judith Mendez wrote:
>> Update eQEP binding for TI K3 devices.
>
> Here and in subject: everything is an update. Be specific.
>
> A nit, subject: drop second/last, redundant "binding". The "dt-bindings"
> prefix is already stating that these are bindings.
> See also:
> https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18
Will fix, thanks.
>
>>
>> Signed-off-by: Judith Mendez <[email protected]>
>> ---
>> Documentation/devicetree/bindings/counter/ti-eqep.yaml | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.yaml b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
>> index 85f1ff83afe72..11755074c8a91 100644
>> --- a/Documentation/devicetree/bindings/counter/ti-eqep.yaml
>> +++ b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
>> @@ -14,19 +14,23 @@ properties:
>> const: ti,am3352-eqep
>>
>> reg:
>> - maxItems: 1
>> + minItems: 1
>> + maxItems: 2
>
> Why? This must be constrained. Devices either have 1 or 2, no both at
> the same time.
Will fix for v2.
>
>>
>> interrupts:
>> description: The eQEP event interrupt
>> maxItems: 1
>>
>> clocks:
>> - description: The clock that determines the SYSCLKOUT rate for the eQEP
>> + description: The clock that determines the clock rate for the eQEP
>> peripheral.
>> maxItems: 1
>>
>> clock-names:
>> - const: sysclkout
>> + maxItems: 1
>
> NAK. That's just wrong, not explained at all either.
>
The intention was to make the binding a bit more generic, but I see
that is not the correct direction to go, thanks for the feedback.
~ Judith
>
> Best regards,
> Krzysztof
>
>
On 4/22/24 1:25 PM, David Lechner wrote:
> On 4/18/24 5:14 PM, Judith Mendez wrote:
>> Update eQEP binding for TI K3 devices.
>
>
> It would make more sense to have this patch first in the series
> before the dts changes.
Got it.
>
>>
>> Signed-off-by: Judith Mendez <[email protected]>
>> ---
>> Documentation/devicetree/bindings/counter/ti-eqep.yaml | 10 +++++++---
>> 1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/counter/ti-eqep.yaml b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
>> index 85f1ff83afe72..11755074c8a91 100644
>> --- a/Documentation/devicetree/bindings/counter/ti-eqep.yaml
>> +++ b/Documentation/devicetree/bindings/counter/ti-eqep.yaml
>> @@ -14,19 +14,23 @@ properties:
>> const: ti,am3352-eqep
>>
>
> As Krzysztof hinted, it sounds like we need to add new compatibles
> here and have some -if: statements to account for the differences
> in SoCs rather than making the bindings less strict.
Yes, I see this is the correct action. Thanks.
>
>> reg:
>> - maxItems: 1
>> + minItems: 1
>> + maxItems: 2
>>
>> interrupts:
>> description: The eQEP event interrupt
>> maxItems: 1
>>
>> clocks:
>> - description: The clock that determines the SYSCLKOUT rate for the eQEP
>> + description: The clock that determines the clock rate for the eQEP
>> peripheral.
>> maxItems: 1
>>
>> clock-names:
>> - const: sysclkout
>> + maxItems: 1
>
> In hindsight, this is not the best name. Since we only have one clock
> we don't really need the name anyway, so for the new compatibles, we
> could set clock-names: false.
Ok, will add this to new compatible.
>
>> +
>> + power-domains:
>> + maxItems: 1
>>
>> required:
>> - compatible
>
>
> I see that the CFG0 syscon register on AM62x has some control knobs for
> the EQEP so it would be good to add this to the bindings now too to try
> to make the bindings as complete as possible. (I didn't look at other
> chips so the same may apply to others as well.)
Got it (:
~ Judith
>
On 23/04/2024 00:11, Judith Mendez wrote:
>>>
>>> interrupts:
>>> description: The eQEP event interrupt
>>> maxItems: 1
>>>
>>> clocks:
>>> - description: The clock that determines the SYSCLKOUT rate for the eQEP
>>> + description: The clock that determines the clock rate for the eQEP
>>> peripheral.
>>> maxItems: 1
>>>
>>> clock-names:
>>> - const: sysclkout
>>> + maxItems: 1
>>
>> NAK. That's just wrong, not explained at all either.
>>
>
> The intention was to make the binding a bit more generic, but I see
> that is not the correct direction to go, thanks for the feedback.
Bindings must be specific, because they describe the hardware. Your
hardware has exactly one clock input pin, not some superposition of pins
which change depending on photon stream.
Best regards,
Krzysztof