| Message ID | 20230509200032.308934-1-liviu@dudau.co.uk (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | mips: dts: ralink: Clarify usage of MT7621 ethernet phy arguments | expand |
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ç
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!
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!
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
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; };
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(-)