[PATCH] mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments

[PATCH] mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments

[Liviu Dudau]

The device tree uses numbers as arguments to the phys property that are
confusing for newcomers. Define names for the values and use them in the
device tree.

Signed-off-by: Liviu Dudau <liviu@dudau.co.uk>
---
 arch/mips/boot/dts/ralink/mt7621.dtsi | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi
index 7caed0d14f11a..1c584b6d0e1fa 100644
--- a/arch/mips/boot/dts/ralink/mt7621.dtsi
+++ b/arch/mips/boot/dts/ralink/mt7621.dtsi
@@ -4,6 +4,9 @@
 #include <dt-bindings/clock/mt7621-clk.h>
 #include <dt-bindings/reset/mt7621-reset.h>
 
+#define DUAL_PORT   1
+#define SINGLE_PORT 0
+
 / {
 	#address-cells = <1>;
 	#size-cells = <1>;
@@ -455,7 +458,7 @@ pcie@0,0 {
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 4 IRQ_TYPE_LEVEL_HIGH>;
 			resets = <&sysc MT7621_RST_PCIE0>;
 			clocks = <&sysc MT7621_CLK_PCIE0>;
-			phys = <&pcie0_phy 1>;
+			phys = <&pcie0_phy DUAL_PORT>;
 			phy-names = "pcie-phy0";
 			ranges;
 		};
@@ -470,7 +473,7 @@ pcie@1,0 {
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 24 IRQ_TYPE_LEVEL_HIGH>;
 			resets = <&sysc MT7621_RST_PCIE1>;
 			clocks = <&sysc MT7621_CLK_PCIE1>;
-			phys = <&pcie0_phy 1>;
+			phys = <&pcie0_phy DUAL_PORT>;
 			phy-names = "pcie-phy1";
 			ranges;
 		};
@@ -485,7 +488,7 @@ pcie@2,0 {
 			interrupt-map = <0 0 0 0 &gic GIC_SHARED 25 IRQ_TYPE_LEVEL_HIGH>;
 			resets = <&sysc MT7621_RST_PCIE2>;
 			clocks = <&sysc MT7621_CLK_PCIE2>;
-			phys = <&pcie2_phy 0>;
+			phys = <&pcie2_phy SINGLE_PORT>;
 			phy-names = "pcie-phy2";
 			ranges;
 		};
-- 
2.40.0

Re: [PATCH] mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments

[Arınç ÜNAL]

On 9.05.2023 22:00, Liviu Dudau wrote:
> The device tree uses numbers as arguments to the phys property that are
> confusing for newcomers. Define names for the values and use them in the
> device tree.
> 
> Signed-off-by: Liviu Dudau <liviu@dudau.co.uk>

You should document this on 
Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml instead 
of doing this. Under the phys property, add 'description:' and explain this.

Arınç

Re: [PATCH] mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments

[Liviu Dudau]

Hi Arınç,

On Wed, May 10, 2023 at 02:59:44PM +0200, Arınç ÜNAL wrote:
> On 9.05.2023 22:00, Liviu Dudau wrote:
> > The device tree uses numbers as arguments to the phys property that are
> > confusing for newcomers. Define names for the values and use them in the
> > device tree.
> > 
> > Signed-off-by: Liviu Dudau <liviu@dudau.co.uk>
> 
> You should document this on
> Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml instead of
> doing this. Under the phys property, add 'description:' and explain this.

There is already some sort of explanation under
Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml, so I'm
not sure what I'm improving by adding new text in the /pci/ section.

Maybe I haven't explained properly in the commit message, this is meant to
give a name to the 1 and 0 values used in the device tree, not to clarify
any perceived missing documentation.

> 
> Arınç

Best regards,
Liviu

--
Everyone who uses computers frequently has had, from time to time,
a mad desire to attack the precocious abacus with an axe.
       	   	      	     	  -- John D. Clark, Ignition!

Re: [PATCH] mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments

[Sergio Paracuellos]

Hi Liviu,

On Wed, May 10, 2023 at 7:56 PM Liviu Dudau <liviu@dudau.co.uk> wrote:
>
> Hi Arınç,
>
> On Wed, May 10, 2023 at 02:59:44PM +0200, Arınç ÜNAL wrote:
> > On 9.05.2023 22:00, Liviu Dudau wrote:
> > > The device tree uses numbers as arguments to the phys property that are
> > > confusing for newcomers. Define names for the values and use them in the
> > > device tree.
> > >
> > > Signed-off-by: Liviu Dudau <liviu@dudau.co.uk>
> >
> > You should document this on
> > instead of
> > doing this. Under the phys property, add 'description:' and explain this.
>
> There is already some sort of explanation under
> Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml, so I'm
> not sure what I'm improving by adding new text in the /pci/ section.
>
> Maybe I haven't explained properly in the commit message, this is meant to
> give a name to the 1 and 0 values used in the device tree, not to clarify
> any perceived missing documentation.

What is that useful for if the bindings are well documented? The
description in the
'Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml' file
for the '#phy-cells'
property is already telling you what that cell is used for. It is
obvious that zero means not dual ported and one means dual ported.
So for me there is no need to add anything extra but in case you want
to clarify anything you should modify binding documentation not the
device tree file at all.

Thanks,
    Sergio Paracuellos
>
> >
> > Arınç
>
> Best regards,
> Liviu
>
> --
> Everyone who uses computers frequently has had, from time to time,
> a mad desire to attack the precocious abacus with an axe.
>                                   -- John D. Clark, Ignition!

Re: [PATCH] mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments

[Liviu Dudau]

On Thu, May 11, 2023 at 06:13:41AM +0200, Sergio Paracuellos wrote:
> Hi Liviu,

Hi Sergio,

> 
> On Wed, May 10, 2023 at 7:56 PM Liviu Dudau <liviu@dudau.co.uk> wrote:
> >
> > Hi Arınç,
> >
> > On Wed, May 10, 2023 at 02:59:44PM +0200, Arınç ÜNAL wrote:
> > > On 9.05.2023 22:00, Liviu Dudau wrote:
> > > > The device tree uses numbers as arguments to the phys property that are
> > > > confusing for newcomers. Define names for the values and use them in the
> > > > device tree.
> > > >
> > > > Signed-off-by: Liviu Dudau <liviu@dudau.co.uk>
> > >
> > > You should document this on
> > > instead of
> > > doing this. Under the phys property, add 'description:' and explain this.
> >
> > There is already some sort of explanation under
> > Documentation/devicetree/bindings/phy/mediatek,mt7621-pci-phy.yaml, so I'm
> > not sure what I'm improving by adding new text in the /pci/ section.
> >
> > Maybe I haven't explained properly in the commit message, this is meant to
> > give a name to the 1 and 0 values used in the device tree, not to clarify
> > any perceived missing documentation.
> 
> What is that useful for if the bindings are well documented? The
> description in the
> 'Documentation/devicetree/bindings/pci/mediatek,mt7621-pcie.yaml' file
> for the '#phy-cells'
> property is already telling you what that cell is used for. It is
> obvious that zero means not dual ported and one means dual ported.
> So for me there is no need to add anything extra but in case you want
> to clarify anything you should modify binding documentation not the
> device tree file at all.

Understood. Then please feel free to ignore this patch.

Best regards,
Liviu

> 
> Thanks,
>     Sergio Paracuellos
> >
> > >
> > > Arınç
> >
> > Best regards,
> > Liviu

-- 
Everyone who uses computers frequently has had, from time to time,
a mad desire to attack the precocious abacus with an axe.
       	   	      	     	  -- John D. Clark, Ignition!