AW: [PATCH v3 1/2] dt-bindings: phy: add realtek,rtl8380m-serdes

[markus.stockhausen at gmx.de]

> -----Ursprüngliche Nachricht-----
> Von: Krzysztof Kozlowski <krzk at kernel.org> 
> Gesendet: Montag, 14. Oktober 2024 09:18
> An: Markus Stockhausen <markus.stockhausen at gmx.de>
> Cc: vkoul at kernel.org; kishon at kernel.org; robh at kernel.org; krzk+dt at kernel.org;
> conor+dt at kernel.org; linux-phy at lists.infradead.org; devicetree at vger.kernel.org; 
> chris.packham at alliedtelesis.co.nz
> Betreff: Re: [PATCH v3 1/2] dt-bindings: phy: add realtek,rtl8380m-serdes
>
> On Sat, Oct 12, 2024 at 09:48:33AM -0400, Markus Stockhausen wrote:
> > Add bindings for the SerDes of the Realtek Otto platform. These are 
> > MIPS based network Switch SoCs with up to 52 ports divided into four 
> > different model lines.
> > ...
> > +  reg:
> > +    items:
> > +      description:
> > +        The primary register memory location. On RTL83xx devices this is the
> > +        address to the I/O register range, on RTL93xx devices this is the
> > +        address of the MDIO style command/data registers.
>
> Not much improved, still missing constraints.
>
> I don't understand why you introduce changes like this.

Hm, two stupid changes in the last two versions. This was only to get some
more meaningfull description there. Not proud of it and somehow lost what 
will be right. Looking at other places a simple 

reg:
  maxItems: 1

should be sufficient. Ok with that?

> > +
> > +  "#phy-cells":
> > +    const: 4
> > +    description:
> > +      The first number defines the SerDes to use. The second number a linked
> > +      SerDes. E.g. if a octa 1G PHY is attached to two QSGMII SerDes. The third
> > +      number is the first switch port this SerDes is working for, the fourth
> > +      number is the last switch port the SerDes is working for.
> > +
> > +  firmware-name:
> > +    maxItems: 1
> > +    description:
> > +      An alternative name of the SerDes firmware image file located in the
> > +      firmware search path. Set to "" to disable firmware loading.
>
> Missing property, not empty string, should rather indicate unneeded firmware.

Indeed more intuitive. Will drop the hardcoded firmware names in the driver
and only load if firmware-name is set.

> > +
> > +examples:
> > +  - |
> > +    serdes: phy at 1b00e780 {
> > +      compatible = "realtek,rtl9302b-serdes";
> > +      reg = <0x1b0003b0 0x8>;
>
> This does notmatch unit address... and again: this was not an issue in previous version. 
> Your new versions of patchset introduce errors and bugs. This is not how the process
> should look like. Review should improve the patch, not reduce its quality.

Agree will fix. This was wrongly mixed when I removed 3 of the
samples as requested by Rob and cleaned the rest afterwards to
have the firmware example.

Markus