| Message ID | 20240306201045.1475-2-justin.swartz@risingedge.co.za (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] mips: dts: ralink: mt7621: associate uart1_pins with serial0 | expand |
Hi Justin, On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz <justin.swartz@risingedge.co.za> wrote: > > Add serial1 and serial2 nodes to define the existence of > UART1 and UART2. > > Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> > --- > arch/mips/boot/dts/ralink/mt7621.dtsi | 38 +++++++++++++++++++++++++++ > 1 file changed, 38 insertions(+) > > diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi > index dca415fdd..2069249c8 100644 > --- a/arch/mips/boot/dts/ralink/mt7621.dtsi > +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi > @@ -128,6 +128,44 @@ serial0: serial@c00 { > pinctrl-0 = <&uart1_pins>; > }; > > + serial1: serial@d00 { > + status = "disabled"; > + > + compatible = "ns16550a"; > + reg = <0xd00 0x100>; > + > + clocks = <&sysc MT7621_CLK_UART2>; > + > + interrupt-parent = <&gic>; > + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>; > + > + reg-shift = <2>; > + reg-io-width = <4>; > + no-loopback-test; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&uart2_pins>; > + }; > + > + serial2: serial@e00 { > + status = "disabled"; > + > + compatible = "ns16550a"; > + reg = <0xe00 0x100>; > + > + clocks = <&sysc MT7621_CLK_UART3>; > + > + interrupt-parent = <&gic>; > + interrupts = <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>; > + > + reg-shift = <2>; > + reg-io-width = <4>; > + no-loopback-test; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&uart3_pins>; > + }; > + Please follow the preferred order for properties described in dts coding style [0]. I know that there is some mess around the properties order in some nodes with the current dtsi file but we did not have coding style before and now we have it, so I think we should follow it at least for new additions. Best regards, Sergio Paracuellos [0]: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
Hi Sergio On 2024-03-07 12:04, Sergio Paracuellos wrote: > Hi Justin, > > On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz > <justin.swartz@risingedge.co.za> wrote: >> >> Add serial1 and serial2 nodes to define the existence of >> UART1 and UART2. >> >> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> >> --- >> arch/mips/boot/dts/ralink/mt7621.dtsi | 38 >> +++++++++++++++++++++++++++ >> 1 file changed, 38 insertions(+) >> >> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi >> b/arch/mips/boot/dts/ralink/mt7621.dtsi >> index dca415fdd..2069249c8 100644 >> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi >> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi >> @@ -128,6 +128,44 @@ serial0: serial@c00 { >> pinctrl-0 = <&uart1_pins>; >> }; >> >> + serial1: serial@d00 { >> + status = "disabled"; >> + >> + compatible = "ns16550a"; >> + reg = <0xd00 0x100>; >> + >> + clocks = <&sysc MT7621_CLK_UART2>; >> + >> + interrupt-parent = <&gic>; >> + interrupts = <GIC_SHARED 27 >> IRQ_TYPE_LEVEL_HIGH>; >> + >> + reg-shift = <2>; >> + reg-io-width = <4>; >> + no-loopback-test; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&uart2_pins>; >> + }; >> + >> + serial2: serial@e00 { >> + status = "disabled"; >> + >> + compatible = "ns16550a"; >> + reg = <0xe00 0x100>; >> + >> + clocks = <&sysc MT7621_CLK_UART3>; >> + >> + interrupt-parent = <&gic>; >> + interrupts = <GIC_SHARED 28 >> IRQ_TYPE_LEVEL_HIGH>; >> + >> + reg-shift = <2>; >> + reg-io-width = <4>; >> + no-loopback-test; >> + >> + pinctrl-names = "default"; >> + pinctrl-0 = <&uart3_pins>; >> + }; >> + > > Please follow the preferred order for properties described in dts > coding style [0]. I know that there is some mess around the properties > order in some nodes with the current dtsi file but we did not have > coding style before and now we have it, so I think we should follow it > at least for new additions. No problem. I see you've already "Acked-by" patch 1 (adding pinctrl properties to serial0) of this set, so would it be a better move to submit a new patch set that would look something like: 1. add pinctrl-name and pinctrl-0 to serial0 [no changes from what I sent] 2. reorder serial0 properties according to the DTS style guidelines 3. add serial1 and serial2 with the correct property order Or instead, submit one more patch that will reorder the properties in serial0, serial1 and serial2 - which would depend on the current set? > Best regards, > Sergio Paracuellos Regards Justin > [0]: https://docs.kernel.org/devicetree/bindings/dts-coding-style.html
Hi Justin, On Thu, Mar 7, 2024 at 4:15 PM Justin Swartz <justin.swartz@risingedge.co.za> wrote: > > Hi Sergio > > On 2024-03-07 12:04, Sergio Paracuellos wrote: > > Hi Justin, > > > > On Wed, Mar 6, 2024 at 9:11 PM Justin Swartz > > <justin.swartz@risingedge.co.za> wrote: > >> > >> Add serial1 and serial2 nodes to define the existence of > >> UART1 and UART2. > >> > >> Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> > >> --- > >> arch/mips/boot/dts/ralink/mt7621.dtsi | 38 > >> +++++++++++++++++++++++++++ > >> 1 file changed, 38 insertions(+) > >> > >> diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi > >> b/arch/mips/boot/dts/ralink/mt7621.dtsi > >> index dca415fdd..2069249c8 100644 > >> --- a/arch/mips/boot/dts/ralink/mt7621.dtsi > >> +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi > >> @@ -128,6 +128,44 @@ serial0: serial@c00 { > >> pinctrl-0 = <&uart1_pins>; > >> }; > >> > >> + serial1: serial@d00 { > >> + status = "disabled"; > >> + > >> + compatible = "ns16550a"; > >> + reg = <0xd00 0x100>; > >> + > >> + clocks = <&sysc MT7621_CLK_UART2>; > >> + > >> + interrupt-parent = <&gic>; > >> + interrupts = <GIC_SHARED 27 > >> IRQ_TYPE_LEVEL_HIGH>; > >> + > >> + reg-shift = <2>; > >> + reg-io-width = <4>; > >> + no-loopback-test; > >> + > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&uart2_pins>; > >> + }; > >> + > >> + serial2: serial@e00 { > >> + status = "disabled"; > >> + > >> + compatible = "ns16550a"; > >> + reg = <0xe00 0x100>; > >> + > >> + clocks = <&sysc MT7621_CLK_UART3>; > >> + > >> + interrupt-parent = <&gic>; > >> + interrupts = <GIC_SHARED 28 > >> IRQ_TYPE_LEVEL_HIGH>; > >> + > >> + reg-shift = <2>; > >> + reg-io-width = <4>; > >> + no-loopback-test; > >> + > >> + pinctrl-names = "default"; > >> + pinctrl-0 = <&uart3_pins>; > >> + }; > >> + > > > > Please follow the preferred order for properties described in dts > > coding style [0]. I know that there is some mess around the properties > > order in some nodes with the current dtsi file but we did not have > > coding style before and now we have it, so I think we should follow it > > at least for new additions. > > No problem. I see you've already "Acked-by" patch 1 (adding pinctrl > properties to serial0) of this set, so would it be a better move to > submit a new patch set that would look something like: > > 1. add pinctrl-name and pinctrl-0 to serial0 [no changes from what I > sent] > 2. reorder serial0 properties according to the DTS style guidelines > 3. add serial1 and serial2 with the correct property order This would be ok, thank you. Best regards, Sergio Paracuellos
diff --git a/arch/mips/boot/dts/ralink/mt7621.dtsi b/arch/mips/boot/dts/ralink/mt7621.dtsi index dca415fdd..2069249c8 100644 --- a/arch/mips/boot/dts/ralink/mt7621.dtsi +++ b/arch/mips/boot/dts/ralink/mt7621.dtsi @@ -128,6 +128,44 @@ serial0: serial@c00 { pinctrl-0 = <&uart1_pins>; }; + serial1: serial@d00 { + status = "disabled"; + + compatible = "ns16550a"; + reg = <0xd00 0x100>; + + clocks = <&sysc MT7621_CLK_UART2>; + + interrupt-parent = <&gic>; + interrupts = <GIC_SHARED 27 IRQ_TYPE_LEVEL_HIGH>; + + reg-shift = <2>; + reg-io-width = <4>; + no-loopback-test; + + pinctrl-names = "default"; + pinctrl-0 = <&uart2_pins>; + }; + + serial2: serial@e00 { + status = "disabled"; + + compatible = "ns16550a"; + reg = <0xe00 0x100>; + + clocks = <&sysc MT7621_CLK_UART3>; + + interrupt-parent = <&gic>; + interrupts = <GIC_SHARED 28 IRQ_TYPE_LEVEL_HIGH>; + + reg-shift = <2>; + reg-io-width = <4>; + no-loopback-test; + + pinctrl-names = "default"; + pinctrl-0 = <&uart3_pins>; + }; + spi0: spi@b00 { status = "disabled";
Add serial1 and serial2 nodes to define the existence of UART1 and UART2. Signed-off-by: Justin Swartz <justin.swartz@risingedge.co.za> --- arch/mips/boot/dts/ralink/mt7621.dtsi | 38 +++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) --