| Message ID | 20240418221417.1592787-6-jm@ti.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Enable eQEP DT support for Sitara K3 platforms | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
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 <jm@ti.com> > --- > 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 <jm@ti.com> > --- > 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 <jm@ti.com> >> --- >> 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 <jm@ti.com> >> --- >> 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
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
Update eQEP binding for TI K3 devices. Signed-off-by: Judith Mendez <jm@ti.com> --- Documentation/devicetree/bindings/counter/ti-eqep.yaml | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-)