| Message ID | 1394573078-20767-5-git-send-email-wsa@the-dreams.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Wolfram, Thank you for the patch. On Tuesday 11 March 2014 22:24:37 Wolfram Sang wrote: > From: Wolfram Sang <wsa@sang-engineering.com> > > Signed-off-by: Wolfram Sang <wsa@sang-engineering.com> > --- > Note: Adding clocks whilst keeping the current sorting is very likely to > break a previously working clock IMO. Imagine adding PCIEC clock inbetween > IIC0 and IIC1 here. Adding chronologically and grouped by similar function > blocks is easier to track. An example addition could then look like: > > R8A7790_CLK_TPU0 > R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > R8A7790_CLK_MMCIF1 R8A7790_CLK_MMCIF0 > R8A7790_CLK_CMT1 > R8A7790_CLK_IIC2 R8A7790_CLK_IIC1 R8A7790_CLK_IIC0 > + R8A7790_CLK_PCIEC > > arch/arm/boot/dts/r8a7790.dtsi | 27 ++++++++++++++------------- > 1 file changed, 14 insertions(+), 13 deletions(-) > > diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi > index da69afc9e5cb..9e15fb9858e7 100644 > --- a/arch/arm/boot/dts/r8a7790.dtsi > +++ b/arch/arm/boot/dts/r8a7790.dtsi > @@ -702,18 +702,19 @@ > mstp3_clks: mstp3_clks@e615013c { > compatible = "renesas,r8a7790-mstp-clocks", > "renesas,cpg-mstp-clocks"; > reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; > - clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>, > - <&cpg_clocks R8A7790_CLK_SD1>, > <&cpg_clocks R8A7790_CLK_SD0>, > - <&mmc0_clk>, <&rclk_clk>; > + clocks = <&hp_clk>, <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, > + <&sd2_clk>, <&cpg_clocks R8A7790_CLK_SD1>, > <&cpg_clocks R8A7790_CLK_SD0>, <&mmc0_clk>, > + <&hp_clk>, <&hp_clk>, <&rclk_clk>; > #clock-cells = <1>; > renesas,clock-indices = < > - R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 > - R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > - R8A7790_CLK_MMCIF0 R8A7790_CLK_CMT1 > + R8A7790_CLK_IIC2 R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 > R8A7790_CLK_SDHI3 > + R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > R8A7790_CLK_MMCIF0 > + R8A7790_CLK_IIC0 R8A7790_CLK_IIC1 R8A7790_CLK_CMT1 > > >; > > clock-output-names = > - "tpu0", "mmcif1", "sdhi3", "sdhi2", > - "sdhi1", "sdhi0", "mmcif0", "cmt1"; > + "i2c6", "tpu0", "mmcif1", "sdhi3", > + "sdhi2", "sdhi1", "sdhi0", "mmcif0", > + "i2c4", "i2c5", "cmt1"; What about calling the clocks iic0-iic2 ? > }; > mstp5_clks: mstp5_clks@e6150144 { > compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp- clocks"; > @@ -757,16 +758,16 @@ > mstp9_clks: mstp9_clks@e6150994 { > compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp- clocks"; > reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>; > - clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>, > + clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>, > <&cp_clk>, > <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>; > #clock-cells = <1>; > renesas,clock-indices = < > - R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD > - R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1 > - R8A7790_CLK_I2C0 > + R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD > R8A7790_CLK_IICDVFS > + R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1 > R8A7790_CLK_I2C0 > >; > > clock-output-names = > - "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2", "i2c1", "i2c0"; > + "rcan1", "rcan0", "qspi_mod", "i2c7", And iic3 ? > + "i2c3", "i2c2", "i2c1", "i2c0"; > }; > };
Hi Wolfram, On Wed, Mar 12, 2014 at 6:24 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > From: Wolfram Sang <wsa@sang-engineering.com> > > Signed-off-by: Wolfram Sang <wsa@sang-engineering.com> > --- > Note: Adding clocks whilst keeping the current sorting is very likely to > break a previously working clock IMO. Imagine adding PCIEC clock inbetween IIC0 > and IIC1 here. Adding chronologically and grouped by similar function blocks is > easier to track. An example addition could then look like: > > R8A7790_CLK_TPU0 > R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > R8A7790_CLK_MMCIF1 R8A7790_CLK_MMCIF0 > R8A7790_CLK_CMT1 > R8A7790_CLK_IIC2 R8A7790_CLK_IIC1 R8A7790_CLK_IIC0 > + R8A7790_CLK_PCIEC Can you please care to explain a bit more about why you see a risk here? Is it a risk for typo or something else? It looks to me that this is just a matter about adding the entry at the right position in several places. Thanks, / magnus
On Wed, Mar 12, 2014 at 08:05:26PM +0900, Magnus Damm wrote: > Hi Wolfram, > > On Wed, Mar 12, 2014 at 6:24 AM, Wolfram Sang <wsa@the-dreams.de> wrote: > > From: Wolfram Sang <wsa@sang-engineering.com> > > > > Signed-off-by: Wolfram Sang <wsa@sang-engineering.com> > > --- > > Note: Adding clocks whilst keeping the current sorting is very likely to > > break a previously working clock IMO. Imagine adding PCIEC clock inbetween IIC0 > > and IIC1 here. Adding chronologically and grouped by similar function blocks is > > easier to track. An example addition could then look like: > > > > R8A7790_CLK_TPU0 > > R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 > > R8A7790_CLK_MMCIF1 R8A7790_CLK_MMCIF0 > > R8A7790_CLK_CMT1 > > R8A7790_CLK_IIC2 R8A7790_CLK_IIC1 R8A7790_CLK_IIC0 > > + R8A7790_CLK_PCIEC > > Can you please care to explain a bit more about why you see a risk > here? Is it a risk for typo or something else? The risk here is if you put something in the "middle" then there is a chance you might be off, and, for example, mix up parent clocks. Especially when you need to reformat paragraphs because of too long lines. Yeah, all this can be avoided by careful review, but this review really needs to be careful. > It looks to me that this is just a matter about adding the entry at > the right position in several places. Yes. IMO adding it with the pattern I sketched above will make it a piece of cake since the diff is a lot more simple than the patch I originally submitted. That being said, I will keep the current sorting for consistency reasons but I personally made up my mind ;)
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi index da69afc9e5cb..9e15fb9858e7 100644 --- a/arch/arm/boot/dts/r8a7790.dtsi +++ b/arch/arm/boot/dts/r8a7790.dtsi @@ -702,18 +702,19 @@ mstp3_clks: mstp3_clks@e615013c { compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks"; reg = <0 0xe615013c 0 4>, <0 0xe6150048 0 4>; - clocks = <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, <&sd2_clk>, - <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>, - <&mmc0_clk>, <&rclk_clk>; + clocks = <&hp_clk>, <&cp_clk>, <&mmc1_clk>, <&sd3_clk>, + <&sd2_clk>, <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>, <&mmc0_clk>, + <&hp_clk>, <&hp_clk>, <&rclk_clk>; #clock-cells = <1>; renesas,clock-indices = < - R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 - R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 - R8A7790_CLK_MMCIF0 R8A7790_CLK_CMT1 + R8A7790_CLK_IIC2 R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3 + R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0 R8A7790_CLK_MMCIF0 + R8A7790_CLK_IIC0 R8A7790_CLK_IIC1 R8A7790_CLK_CMT1 >; clock-output-names = - "tpu0", "mmcif1", "sdhi3", "sdhi2", - "sdhi1", "sdhi0", "mmcif0", "cmt1"; + "i2c6", "tpu0", "mmcif1", "sdhi3", + "sdhi2", "sdhi1", "sdhi0", "mmcif0", + "i2c4", "i2c5", "cmt1"; }; mstp5_clks: mstp5_clks@e6150144 { compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks"; @@ -757,16 +758,16 @@ mstp9_clks: mstp9_clks@e6150994 { compatible = "renesas,r8a7790-mstp-clocks", "renesas,cpg-mstp-clocks"; reg = <0 0xe6150994 0 4>, <0 0xe61509a4 0 4>; - clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>, + clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>, <&cp_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>; #clock-cells = <1>; renesas,clock-indices = < - R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD - R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1 - R8A7790_CLK_I2C0 + R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD R8A7790_CLK_IICDVFS + R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1 R8A7790_CLK_I2C0 >; clock-output-names = - "rcan1", "rcan0", "qspi_mod", "i2c3", "i2c2", "i2c1", "i2c0"; + "rcan1", "rcan0", "qspi_mod", "i2c7", + "i2c3", "i2c2", "i2c1", "i2c0"; }; };