diff mbox series

[8/9] dt-bindings: phy: sparx5: document lan969x in sparx5 dt-bindings

Message ID 20240906-sparx5-lan969x-serdes-driver-v1-8-8d630614c58a@microchip.com
State Superseded
Headers show
Series phy: sparx5-serdes: add support for lan969x serdes driver | expand

Commit Message

Daniel Machon Sept. 6, 2024, 12:52 p.m. UTC 
Document lan969x in the existing Sparx5 dt-bindings.

Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
---
 .../bindings/phy/microchip,sparx5-serdes.yaml          | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Sept. 6, 2024, 12:59 p.m. UTC | #1 
On 06/09/2024 14:52, Daniel Machon wrote:
> Document lan969x in the existing Sparx5 dt-bindings.
> 

Say something useful, not copy of subject.

> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>

A nit, subject: drop second/last, redundant "dt-bindings". 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


> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> ---
>  .../bindings/phy/microchip,sparx5-serdes.yaml          | 18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
> index bdbdb3bbddbe..1e07a311e8a5 100644
> --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
> +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
> @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller
>  
>  maintainers:
>    - Steen Hegelund <steen.hegelund@microchip.com>
> +  - Daniel Machon <daniel.machon@microchip.com>
>  
>  description: |
>    The Sparx5 SERDES interfaces share the same basic functionality, but
> @@ -62,12 +63,17 @@ description: |
>    * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII)
>    * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER)
>  
> +  lan969x has ten SERDES10G interfaces that share the same features, operating
> +  modes and data rates as the equivalent Sparx5 SERDES10G interfaces.
> +
>  properties:
>    $nodename:
>      pattern: "^serdes@[0-9a-f]+$"
>  
>    compatible:
> -    const: microchip,sparx5-serdes
> +    enum:
> +      - microchip,sparx5-serdes
> +      - microchip,lan969x-serdes

It seems there is no lan969x SoC/chip. Are you sure you are using
correct naming, matching what kernel is using? Maybe you just sent
whatever you had in downstream (hint: that's never a good idea).

>  
>    reg:
>      minItems: 1
> @@ -90,11 +96,19 @@ additionalProperties: false
>  
>  examples:
>    - |
> -    serdes: serdes@10808000 {
> +    serdes@10808000 {
>        compatible = "microchip,sparx5-serdes";
>        #phy-cells = <1>;
>        clocks = <&sys_clk>;
>        reg = <0x10808000 0x5d0000>;
>      };
>  
> +  - |
> +    serdes@e3410000 {
> +      compatible = "microchip,lan969x-serdes";
> +      #phy-cells = <1>;
> +      clocks = <&fabric_clk>;

No differences so no need for new example. Also please follow DTS coding
style in case of any DTS code.

Best regards,
Krzysztof
Daniel Machon Sept. 9, 2024, 8:22 a.m. UTC | #2 
Hi Krzysztof,

Thanks your comments.

> On 06/09/2024 14:52, Daniel Machon wrote:
> > Document lan969x in the existing Sparx5 dt-bindings.
> >
> 
> Say something useful, not copy of subject.
> 
> > Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
> 
> A nit, subject: drop second/last, redundant "dt-bindings". 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

Duly noted.

> 
> 
> > Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
> > ---
> >  .../bindings/phy/microchip,sparx5-serdes.yaml          | 18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
> > index bdbdb3bbddbe..1e07a311e8a5 100644
> > --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
> > +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
> > @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller
> >
> >  maintainers:
> >    - Steen Hegelund <steen.hegelund@microchip.com>
> > +  - Daniel Machon <daniel.machon@microchip.com>
> >
> >  description: |
> >    The Sparx5 SERDES interfaces share the same basic functionality, but
> > @@ -62,12 +63,17 @@ description: |
> >    * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII)
> >    * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER)
> >
> > +  lan969x has ten SERDES10G interfaces that share the same features, operating
> > +  modes and data rates as the equivalent Sparx5 SERDES10G interfaces.
> > +
> >  properties:
> >    $nodename:
> >      pattern: "^serdes@[0-9a-f]+$"
> >
> >    compatible:
> > -    const: microchip,sparx5-serdes
> > +    enum:
> > +      - microchip,sparx5-serdes
> > +      - microchip,lan969x-serdes
> 
> It seems there is no lan969x SoC/chip. Are you sure you are using
> correct naming, matching what kernel is using? Maybe you just sent
> whatever you had in downstream (hint: that's never a good idea).

You are right. There is no upstream support for lan969x SoC yet. The
upstreaming of the lan969x SoC has just begun, and this series is part
of that upstreaming effort. The lan969x switch driver (not submitted
yet) will depend on this SERDES driver, however, their functionality is
really independent of each other. That is why I am also upstreaming the
SERDES- and switch driver series independent of each other.

If these series needs to somehow be connected, by link or whatever, then
fine.  If there is some preferred way to do this, then please let me
know or point me in some direction. Thanks.

> 
> >
> >    reg:
> >      minItems: 1
> > @@ -90,11 +96,19 @@ additionalProperties: false
> >
> >  examples:
> >    - |
> > -    serdes: serdes@10808000 {
> > +    serdes@10808000 {
> >        compatible = "microchip,sparx5-serdes";
> >        #phy-cells = <1>;
> >        clocks = <&sys_clk>;
> >        reg = <0x10808000 0x5d0000>;
> >      };
> >
> > +  - |
> > +    serdes@e3410000 {
> > +      compatible = "microchip,lan969x-serdes";
> > +      #phy-cells = <1>;
> > +      clocks = <&fabric_clk>;
> 
> No differences so no need for new example. Also please follow DTS coding
> style in case of any DTS code.
> 

Ack.

> Best regards,
> Krzysztof
> 

/Daniel
Krzysztof Kozlowski Sept. 9, 2024, 8:35 a.m. UTC | #3 
On 09/09/2024 10:22, Daniel Machon wrote:
> Hi Krzysztof,
> 
> Thanks your comments.
> 
>> On 06/09/2024 14:52, Daniel Machon wrote:
>>> Document lan969x in the existing Sparx5 dt-bindings.
>>>
>>
>> Say something useful, not copy of subject.
>>
>>> Signed-off-by: Daniel Machon <daniel.machon@microchip.com>
>>
>> A nit, subject: drop second/last, redundant "dt-bindings". 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
> 
> Duly noted.
> 
>>
>>
>>> Reviewed-by: Steen Hegelund <Steen.Hegelund@microchip.com>
>>> ---
>>>  .../bindings/phy/microchip,sparx5-serdes.yaml          | 18 ++++++++++++++++--
>>>  1 file changed, 16 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
>>> index bdbdb3bbddbe..1e07a311e8a5 100644
>>> --- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
>>> +++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
>>> @@ -8,6 +8,7 @@ title: Microchip Sparx5 Serdes controller
>>>
>>>  maintainers:
>>>    - Steen Hegelund <steen.hegelund@microchip.com>
>>> +  - Daniel Machon <daniel.machon@microchip.com>
>>>
>>>  description: |
>>>    The Sparx5 SERDES interfaces share the same basic functionality, but
>>> @@ -62,12 +63,17 @@ description: |
>>>    * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII)
>>>    * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER)
>>>
>>> +  lan969x has ten SERDES10G interfaces that share the same features, operating
>>> +  modes and data rates as the equivalent Sparx5 SERDES10G interfaces.
>>> +
>>>  properties:
>>>    $nodename:
>>>      pattern: "^serdes@[0-9a-f]+$"
>>>
>>>    compatible:
>>> -    const: microchip,sparx5-serdes
>>> +    enum:
>>> +      - microchip,sparx5-serdes
>>> +      - microchip,lan969x-serdes
>>
>> It seems there is no lan969x SoC/chip. Are you sure you are using
>> correct naming, matching what kernel is using? Maybe you just sent
>> whatever you had in downstream (hint: that's never a good idea).
> 
> You are right. There is no upstream support for lan969x SoC yet. The
> upstreaming of the lan969x SoC has just begun, and this series is part
> of that upstreaming effort. The lan969x switch driver (not submitted
> yet) will depend on this SERDES driver, however, their functionality is
> really independent of each other. That is why I am also upstreaming the
> SERDES- and switch driver series independent of each other.

That's not exactly my point. Becayse lan969x appears. I claim you use
incorrect name, so are you sure you do not use wildcards?
Best regards,
Krzysztof
Daniel Machon Sept. 9, 2024, 9:43 a.m. UTC | #4 
> >>>    compatible:
> >>> -    const: microchip,sparx5-serdes
> >>> +    enum:
> >>> +      - microchip,sparx5-serdes
> >>> +      - microchip,lan969x-serdes
> >>
> >> It seems there is no lan969x SoC/chip. Are you sure you are using
> >> correct naming, matching what kernel is using? Maybe you just sent
> >> whatever you had in downstream (hint: that's never a good idea).
> >
> > You are right. There is no upstream support for lan969x SoC yet. The
> > upstreaming of the lan969x SoC has just begun, and this series is part
> > of that upstreaming effort. The lan969x switch driver (not submitted
> > yet) will depend on this SERDES driver, however, their functionality is
> > really independent of each other. That is why I am also upstreaming the
> > SERDES- and switch driver series independent of each other.
> 
> That's not exactly my point. Becayse lan969x appears. I claim you use
> incorrect name, so are you sure you do not use wildcards?
> Best regards,
> Krzysztof

Ahh.

So the problem is the 'x' in lan969x, right? I think we have a habbit of
documenting compatible strings like this in bindings. Anyway, what I can
do is document the different part numbers in the bindings: lan9691,
lan9692, lan9693, lan9694, lan9696 and lan9698.

/Daniel
Krzysztof Kozlowski Sept. 9, 2024, 10:42 a.m. UTC | #5 
On 09/09/2024 11:43, Daniel Machon wrote:
>>>>>    compatible:
>>>>> -    const: microchip,sparx5-serdes
>>>>> +    enum:
>>>>> +      - microchip,sparx5-serdes
>>>>> +      - microchip,lan969x-serdes
>>>>
>>>> It seems there is no lan969x SoC/chip. Are you sure you are using
>>>> correct naming, matching what kernel is using? Maybe you just sent
>>>> whatever you had in downstream (hint: that's never a good idea).
>>>
>>> You are right. There is no upstream support for lan969x SoC yet. The
>>> upstreaming of the lan969x SoC has just begun, and this series is part
>>> of that upstreaming effort. The lan969x switch driver (not submitted
>>> yet) will depend on this SERDES driver, however, their functionality is
>>> really independent of each other. That is why I am also upstreaming the
>>> SERDES- and switch driver series independent of each other.
>>
>> That's not exactly my point. Becayse lan969x appears. I claim you use
>> incorrect name, so are you sure you do not use wildcards?
>> Best regards,
>> Krzysztof
> 
> Ahh.
> 
> So the problem is the 'x' in lan969x, right? I think we have a habbit of
> documenting compatible strings like this in bindings. Anyway, what I can
> do is document the different part numbers in the bindings: lan9691,
> lan9692, lan9693, lan9694, lan9696 and lan9698.

Depends. I remember I was confused about such wildcards before and for
some cases wildcard was ok. For some not. I don't remember, I don't know
which case is here.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
index bdbdb3bbddbe..1e07a311e8a5 100644
--- a/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
+++ b/Documentation/devicetree/bindings/phy/microchip,sparx5-serdes.yaml
@@ -8,6 +8,7 @@  title: Microchip Sparx5 Serdes controller
 
 maintainers:
   - Steen Hegelund <steen.hegelund@microchip.com>
+  - Daniel Machon <daniel.machon@microchip.com>
 
 description: |
   The Sparx5 SERDES interfaces share the same basic functionality, but
@@ -62,12 +63,17 @@  description: |
   * 10.3125 Gbps (10GBASE-R/10GBASE-KR/USXGMII)
   * 25.78125 Gbps (25GBASE-KR/25GBASE-CR/25GBASE-SR/25GBASE-LR/25GBASE-ER)
 
+  lan969x has ten SERDES10G interfaces that share the same features, operating
+  modes and data rates as the equivalent Sparx5 SERDES10G interfaces.
+
 properties:
   $nodename:
     pattern: "^serdes@[0-9a-f]+$"
 
   compatible:
-    const: microchip,sparx5-serdes
+    enum:
+      - microchip,sparx5-serdes
+      - microchip,lan969x-serdes
 
   reg:
     minItems: 1
@@ -90,11 +96,19 @@  additionalProperties: false
 
 examples:
   - |
-    serdes: serdes@10808000 {
+    serdes@10808000 {
       compatible = "microchip,sparx5-serdes";
       #phy-cells = <1>;
       clocks = <&sys_clk>;
       reg = <0x10808000 0x5d0000>;
     };
 
+  - |
+    serdes@e3410000 {
+      compatible = "microchip,lan969x-serdes";
+      #phy-cells = <1>;
+      clocks = <&fabric_clk>;
+      reg = <0xe3410000 0x150000>;
+    };
+
 ...