AW: [PATCH v2 1/3] dt-bindings: phy: add realtek,otto-serdes PHY binding

From: <markus.stockhausen@gmx.de>
To: "'Krzysztof Kozlowski'" <krzk@kernel.org>,
	<linux-phy@lists.infradead.org>,
	<chris.packham@alliedtelesis.co.nz>, <devicetree@vger.kernel.org>
Subject: AW: [PATCH v2 1/3] dt-bindings: phy: add realtek,otto-serdes PHY binding
Date: Wed, 16 Oct 2024 17:30:11 +0200	[thread overview]
Message-ID: <002201db1fe0$4b398dc0$e1aca940$@gmx.de> (raw)
In-Reply-To: <e0355f2b-9d77-4792-9405-14b0bf79ac32@kernel.org>

Hi Krzysztof,

with your feedback on the latest version I will take up the issues from 
v2 once again. To be sure that I do not miss anything in upcoming v5 
I will comment on all your feedback. 

> > ....
> > Changes in v2:
> > - new subject
> > - removed patch command sequences
> > - renamed parameter controlled-ports to realtek,controlled-ports
>
> Changelog goes under ---.

After reading this another 4 times now I think I understand. You mean
"put changelog below signed-off-by". Will do with next patch.

> > ....
> > diff --git 
> > a/Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml 
> > b/Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml
> > new file mode 100644
> > index 000000000000..a72ac206b35f
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/phy/realtek,otto-serdes.yaml
>
> Nothing improved.

In between renamed to compatible "realtek,rtl8380m-serdes.yaml". I hope
that fits the requested naming convention.

> > +  The driver exposes the SerDes registers different from the hardware 
> > + but instead gives a  consistent view and programming interface. So 
> > + the RTL838x series has 6 ports and 4 pages, the  RTL839x has 14 
> > + ports and 12 pages, the RTL930x has 12 ports and 64 pages and the 
> > + RTL931x has
> > +  14 ports and 192 pages.
>
> Totally messed wrapping. Please wrap your code as Linux coding style.

Was restyled in between. If this is still an issue in latest version, please advise.

> > +  reg:
> > +    items:
> > +      description:
> > +        The primary SerDes paged register memory location. Other SerDes control and management
> > +        registers are distributed all over the I/O memory space and are identified by the driver.
>
> What happened here? I asked only about |. Why are you adding unrelated changes?
>
> Anyway, still not tested and still does not look any other binding.

Has been tested in between with "make dt_binding_check".

> > +  realtek,controlled-ports:
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    description:
> > +      A bit mask defining the ports that are actively controlled by 
> > + the driver. In case a bit is
>
> Driver? Bindings are not about drivers. Drop.
>
> I don't think you implemented my feedback.

All these have been removed.

> > +additionalProperties:
> > +  false
>
> Please open any existing binding and do it like there. Or start from scratch from example-schema.

Was converted to one line.

> > +
> > +examples:
> > +  - |
> > +    serdes: serdes@1b00e780 {
> > +      compatible = "realtek,rtl8380-serdes", "realtek,otto-serdes";
> > +      reg = <0x1b00e780 0x1200>;
> > +      controlled-ports = <0x003f>;
> > +      #phy-cells = <4>;
> > +    };
>
> One example is enough.

Only one example left.

Best regards.

Markus

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy