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

[Daniel Machon]

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@xxxxxxxxxxxxx>
> 
> 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@xxxxxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxxxxx>
> > +  - Daniel Machon <daniel.machon@xxxxxxxxxxxxx>
> >
> >  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