Message-ID: <e0511d39-7915-3ce1-60c7-9d7739f1b253@linaro.org>
Date:   Sun, 10 Apr 2022 21:01:12 +0200
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Josua Mayer <josua@...id-run.com>, netdev@...r.kernel.org
Cc:     alvaro.karsz@...id-run.com,
        Michael Hennerich <michael.hennerich@...log.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Alexandru Ardelean <alexandru.ardelean@...log.com>
Subject: Re: [PATCH 1/3] dt: adin: document clk-out property

On 10/04/2022 20:41, Josua Mayer wrote:
>>
>> Adjust subject prefix to the subsystem (dt-bindings, not dt, missing net).
> Ack. So something like
> dt-bindings: net: adin: document clk-out property

Yes.


>>> Signed-off-by: Josua Mayer <josua@...id-run.com>
>>> ---
>>>   Documentation/devicetree/bindings/net/adi,adin.yaml | 5 +++++
>>>   1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/adi,adin.yaml b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>> index 1129f2b58e98..4e421bf5193d 100644
>>> --- a/Documentation/devicetree/bindings/net/adi,adin.yaml
>>> +++ b/Documentation/devicetree/bindings/net/adi,adin.yaml
>>> @@ -36,6 +36,11 @@ properties:
>>>       enum: [ 4, 8, 12, 16, 20, 24 ]
>>>       default: 8
>>>   
>>> +  adi,clk-out-frequency:
>> Use types defined by the dtschema, so "adi,clk-out-hz". Then no need for
>> type/ref.
> That sounds useful, I was not aware. The only inspiration I used was the 
> at803x driver ...
> It seemed natural to share the property name as it serves the same 
> purpose here.

Indeed ar803x uses such property. In general reusing properties is a
good idea, but not all properties are good enough to copy. I don't know
why adi,clk-out-frequency got accepted because we really stick to common
units when possible.

https://github.com/devicetree-org/dt-schema/blob/main/dtschema/schemas/property-units.yaml

>>> +    description: Clock output frequency in Hertz.
>>> +    $ref: /schemas/types.yaml#/definitions/uint32
>>> +    enum: [125000000]
>> If only one value, then "const: 125000000", but why do you need such
>> value in DT if it is always the same?
> Yes yes yes.
>  From my understanding of the adin1300 data-sheet, it can provide either 
> a 25MHz clock or a 125MHz clock on its GP_CLK pin. So for the context of 
> this feature we would have two options. However because we found the 
> documentation very confusing we skipped the 25MHz option.
> 
> Actually my statement above omits some of the options.
> - There are actually two 125MHz clocks, the first called "recovered" and 
> the second "free running".
> - One can let the phy choose the rate based on its internal state.
> This is indicated on page 73 of the datasheet
> (https://www.analog.com/media/en/technical-documentation/data-sheets/adin1300.pdf)
> 
> Because of this confusion we wanted to for now only allow selecting the 
> free-running 125MHz clock.

Hm, so you do not insist on actual frequency but rather type of the
clock (freerunning instead of recovered and 25 MHz). Then the frequency
does not look enough because it does not offer you the choice of clock
(freerunning or recovered) and instead you could have enum like:
  adi,phy-output-clock:
  $ref: /schemas/types.yaml#/definitions/string
  enum: [125mhz-freerunning, 125mhz-recovered, 25mhz-freeruning....]

Judging by page 24 you have 5 or more options... This could be also
numeric ID (enum [0, 1, 2, 3 ...]) with some explanation, but strings
seem easier to understand.

The binding should describe the hardware, not implementation in Linux,
therefore you should actually list all possible choices. The driver then
can just return EINVAL on unsupported choices (or map them back to only
one supported).


Best regards,
Krzysztof

Powered by blists - more mailing lists