[PATCH 2/3] renesas: change to using clock-indices

[PATCH 2/3] renesas: change to using clock-indices

[Ben Dooks]

With the addition of clock-indices, we need to change the renesas
clock implementation to use these instead of the local definition.

Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
---
 .../bindings/clock/renesas,cpg-mstp-clocks.txt         |  2 +-
 arch/arm/boot/dts/r8a7790.dtsi                         | 16 ++++++++--------
 arch/arm/boot/dts/r8a7791.dtsi                         | 18 +++++++++---------
 drivers/clk/shmobile/clk-mstp.c                        |  2 +-
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
index a6a352c..f5b4171 100644
--- a/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
+++ b/Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
@@ -43,7 +43,7 @@ Example
 		clock-output-names  			"tpu0", "mmcif1", "sdhi3", "sdhi2",
 			 "sdhi1", "sdhi0", "mmcif0";
-		renesas,clock-indices = <
+		clock-indices = <
 			R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
 			R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
 			R8A7790_CLK_MMCIF0
diff --git a/arch/arm/boot/dts/r8a7790.dtsi b/arch/arm/boot/dts/r8a7790.dtsi
index 54ab318..7959323 100644
--- a/arch/arm/boot/dts/r8a7790.dtsi
+++ b/arch/arm/boot/dts/r8a7790.dtsi
@@ -671,7 +671,7 @@
 			reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>;
 			clocks = <&mp_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <R8A7790_CLK_MSIOF0>;
+			clock-indices = <R8A7790_CLK_MSIOF0>;
 			clock-output-names = "msiof0";
 		};
 		mstp1_clks: mstp1_clks@e6150134 {
@@ -681,7 +681,7 @@
 				 <&cp_clk>, <&zs_clk>, <&zs_clk>, <&zs_clk>,
 				 <&zs_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7790_CLK_TMU1 R8A7790_CLK_TMU3 R8A7790_CLK_TMU2
 				R8A7790_CLK_CMT0 R8A7790_CLK_TMU0 R8A7790_CLK_VSP1_DU1
 				R8A7790_CLK_VSP1_DU0 R8A7790_CLK_VSP1_RT R8A7790_CLK_VSP1_SY
@@ -696,7 +696,7 @@
 			clocks = <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>,
 				 <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7790_CLK_SCIFA2 R8A7790_CLK_SCIFA1 R8A7790_CLK_SCIFA0
 				R8A7790_CLK_MSIOF2 R8A7790_CLK_SCIFB0 R8A7790_CLK_SCIFB1
 				R8A7790_CLK_MSIOF1 R8A7790_CLK_MSIOF3 R8A7790_CLK_SCIFB2
@@ -712,7 +712,7 @@
 				 <&cpg_clocks R8A7790_CLK_SD1>, <&cpg_clocks R8A7790_CLK_SD0>,
 				 <&mmc0_clk>, <&rclk_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			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
@@ -726,7 +726,7 @@
 			reg = <0 0xe6150144 0 4>, <0 0xe615003c 0 4>;
 			clocks = <&extal_clk>, <&p_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <R8A7790_CLK_THERMAL R8A7790_CLK_PWM>;
+			clock-indices = <R8A7790_CLK_THERMAL R8A7790_CLK_PWM>;
 			clock-output-names = "thermal", "pwm";
 		};
 		mstp7_clks: mstp7_clks@e615014c {
@@ -736,7 +736,7 @@
 				 <&p_clk>, <&zx_clk>, <&zx_clk>, <&zx_clk>, <&zx_clk>,
 				 <&zx_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7790_CLK_EHCI R8A7790_CLK_HSUSB R8A7790_CLK_HSCIF1
 				R8A7790_CLK_HSCIF0 R8A7790_CLK_SCIF1 R8A7790_CLK_SCIF0
 				R8A7790_CLK_DU2 R8A7790_CLK_DU1 R8A7790_CLK_DU0
@@ -752,7 +752,7 @@
 			clocks = <&zg_clk>, <&zg_clk>, <&zg_clk>, <&zg_clk>, <&p_clk>,
 				 <&zs_clk>, <&zs_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7790_CLK_VIN3 R8A7790_CLK_VIN2 R8A7790_CLK_VIN1
 				R8A7790_CLK_VIN0 R8A7790_CLK_ETHER R8A7790_CLK_SATA1
 				R8A7790_CLK_SATA0
@@ -766,7 +766,7 @@
 			clocks = <&p_clk>, <&p_clk>, <&cpg_clocks R8A7790_CLK_QSPI>,
 				 <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7790_CLK_RCAN1 R8A7790_CLK_RCAN0 R8A7790_CLK_QSPI_MOD
 				R8A7790_CLK_I2C3 R8A7790_CLK_I2C2 R8A7790_CLK_I2C1
 				R8A7790_CLK_I2C0
diff --git a/arch/arm/boot/dts/r8a7791.dtsi b/arch/arm/boot/dts/r8a7791.dtsi
index 240c4ec..166b986 100644
--- a/arch/arm/boot/dts/r8a7791.dtsi
+++ b/arch/arm/boot/dts/r8a7791.dtsi
@@ -598,7 +598,7 @@
 			reg = <0 0xe6150130 0 4>, <0 0xe6150030 0 4>;
 			clocks = <&mp_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <R8A7791_CLK_MSIOF0>;
+			clock-indices = <R8A7791_CLK_MSIOF0>;
 			clock-output-names = "msiof0";
 		};
 		mstp1_clks: mstp1_clks@e6150134 {
@@ -607,7 +607,7 @@
 			clocks = <&p_clk>, <&p_clk>, <&p_clk>, <&rclk_clk>,
 				 <&cp_clk>, <&zs_clk>, <&zs_clk>, <&zs_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_TMU1 R8A7791_CLK_TMU3 R8A7791_CLK_TMU2
 				R8A7791_CLK_CMT0 R8A7791_CLK_TMU0 R8A7791_CLK_VSP1_DU1
 				R8A7791_CLK_VSP1_DU0 R8A7791_CLK_VSP1_SY
@@ -622,7 +622,7 @@
 			clocks = <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>, <&mp_clk>,
 				 <&mp_clk>, <&mp_clk>, <&mp_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_SCIFA2 R8A7791_CLK_SCIFA1 R8A7791_CLK_SCIFA0
 				R8A7791_CLK_MSIOF2 R8A7791_CLK_SCIFB0 R8A7791_CLK_SCIFB1
 				R8A7791_CLK_MSIOF1 R8A7791_CLK_SCIFB2
@@ -637,7 +637,7 @@
 			clocks = <&cp_clk>, <&sd2_clk>, <&sd1_clk>,
 				<&cpg_clocks R8A7791_CLK_SD0>, <&mmc0_clk>, <&rclk_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_TPU0 R8A7791_CLK_SDHI2 R8A7791_CLK_SDHI1
 				R8A7791_CLK_SDHI0 R8A7791_CLK_MMCIF0 R8A7791_CLK_CMT1
 			>;
@@ -649,7 +649,7 @@
 			reg = <0 0xe6150144 0 4>, <0 0xe615003c 0 4>;
 			clocks = <&extal_clk>, <&p_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <R8A7791_CLK_THERMAL R8A7791_CLK_PWM>;
+			clock-indices = <R8A7791_CLK_THERMAL R8A7791_CLK_PWM>;
 			clock-output-names = "thermal", "pwm";
 		};
 		mstp7_clks: mstp7_clks@e615014c {
@@ -659,7 +659,7 @@
 				 <&zs_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>,
 				 <&zx_clk>, <&zx_clk>, <&zx_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_HSUSB R8A7791_CLK_HSCIF2 R8A7791_CLK_SCIF5
 				R8A7791_CLK_SCIF4 R8A7791_CLK_HSCIF1 R8A7791_CLK_HSCIF0
 				R8A7791_CLK_SCIF3 R8A7791_CLK_SCIF2 R8A7791_CLK_SCIF1
@@ -676,7 +676,7 @@
 			clocks = <&zg_clk>, <&zg_clk>, <&zg_clk>, <&p_clk>, <&zs_clk>,
 				 <&zs_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_VIN2 R8A7791_CLK_VIN1 R8A7791_CLK_VIN0
 				R8A7791_CLK_ETHER R8A7791_CLK_SATA1 R8A7791_CLK_SATA0
 			>;
@@ -690,7 +690,7 @@
 				 <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>, <&p_clk>,
 				 <&p_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_RCAN1 R8A7791_CLK_RCAN0 R8A7791_CLK_QSPI_MOD
 				R8A7791_CLK_I2C4 R8A7791_CLK_I2C4 R8A7791_CLK_I2C3
 				R8A7791_CLK_I2C2 R8A7791_CLK_I2C1 R8A7791_CLK_I2C0
@@ -704,7 +704,7 @@
 			reg = <0 0xe615099c 0 4>, <0 0xe61509ac 0 4>;
 			clocks = <&mp_clk>, <&mp_clk>, <&mp_clk>;
 			#clock-cells = <1>;
-			renesas,clock-indices = <
+			clock-indices = <
 				R8A7791_CLK_SCIFA3 R8A7791_CLK_SCIFA4 R8A7791_CLK_SCIFA5
 			>;
 			clock-output-names = "scifa3", "scifa4", "scifa5";
diff --git a/drivers/clk/shmobile/clk-mstp.c b/drivers/clk/shmobile/clk-mstp.c
index 42d5912..95a2aa7 100644
--- a/drivers/clk/shmobile/clk-mstp.c
+++ b/drivers/clk/shmobile/clk-mstp.c
@@ -197,7 +197,7 @@ static void __init cpg_mstp_clocks_init(struct device_node *np)
 			continue;
 
 		parent_name = of_clk_get_parent_name(np, i);
-		ret = of_property_read_u32_index(np, "renesas,clock-indices", i,
+		ret = of_property_read_u32_index(np, "clock-indices", i,
 						 &clkidx);
 		if (parent_name = NULL || ret < 0)
 			break;
-- 
1.8.5.3

Re: [PATCH 2/3] renesas: change to using clock-indices

[Magnus Damm]

On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks <ben.dooks@codethink.co.uk> wrote:
> With the addition of clock-indices, we need to change the renesas
> clock implementation to use these instead of the local definition.
>
> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> ---
>  .../bindings/clock/renesas,cpg-mstp-clocks.txt         |  2 +-
>  arch/arm/boot/dts/r8a7790.dtsi                         | 16 ++++++++--------
>  arch/arm/boot/dts/r8a7791.dtsi                         | 18 +++++++++---------
>  drivers/clk/shmobile/clk-mstp.c                        |  2 +-
>  4 files changed, 19 insertions(+), 19 deletions(-)

Thanks for your patches! Good to see more common bindings.

Regarding how to roll it into the SoC code, I feel that this patch
[2/3] and next patch [3/3] potentially causes breakage with the
existing binding that was part of the v3.13 upstream kernel. I'd like
to maintain backwards compatibility for our users. Also, this patch
crosses subsystems which may not be entirely ideal from a merge
perspective.

If other people in the community agree with [1/3] then may I propose
that you rework the series into:

[1/3] as-is is fine for me
[2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
code from [2/3] and merge with [3/3], stay backwards compatible.
[3/3] adjust the SoC DTS

That would maintain backwards compatibility and keep existing users happy.

Thanks,

/ magnus

Re: [PATCH 2/3] renesas: change to using clock-indices

[Laurent Pinchart]

Hi Magnus,

On Friday 14 February 2014 11:32:08 Magnus Damm wrote:
> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote:
> > With the addition of clock-indices, we need to change the renesas
> > clock implementation to use these instead of the local definition.
> > 
> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> > ---
> > 
> >  .../bindings/clock/renesas,cpg-mstp-clocks.txt   |  2 +-
> >   arch/arm/boot/dts/r8a7790.dtsi                  | 16 ++++++++--------
> >  arch/arm/boot/dts/r8a7791.dtsi                   | 18 +++++++++---------
> >  drivers/clk/shmobile/clk-mstp.c                  |  2 +-
> >  4 files changed, 19 insertions(+), 19 deletions(-)
> 
> Thanks for your patches! Good to see more common bindings.
> 
> Regarding how to roll it into the SoC code, I feel that this patch [2/3] and
> next patch [3/3] potentially causes breakage with the existing binding that
> was part of the v3.13 upstream kernel.

If I'm not mistaken that's v3.14, not v3.13.

> I'd like to maintain backwards compatibility for our users. Also, this patch
> crosses subsystems which may not be entirely ideal from a merge perspective.
> 
> If other people in the community agree with [1/3] then may I propose that
> you rework the series into:
> 
> [1/3] as-is is fine for me
> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
> code from [2/3] and merge with [3/3], stay backwards compatible.
> [3/3] adjust the SoC DTS
> 
> That would maintain backwards compatibility and keep existing users happy.

We'll have to address this DT backward compatibility nonsense at some point. 
We can't consider all bindings as stable as soon as they hit mainline, that 
just won't fly. In this particular case, given that the driver got merged in 
v3.14-rc1, and that our mainline multiplatform boards in mainline are 
virtually useless in real products given their early stage of development, I 
would rather consider the clocks DT bindings as unstable for at least a couple 
more kernel versions.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] renesas: change to using clock-indices

[Ben Dooks]

On 14/02/14 13:21, Laurent Pinchart wrote:
> Hi Magnus,
>
> On Friday 14 February 2014 11:32:08 Magnus Damm wrote:
>> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote:
>>> With the addition of clock-indices, we need to change the renesas
>>> clock implementation to use these instead of the local definition.
>>>
>>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>>> ---
>>>
>>>   .../bindings/clock/renesas,cpg-mstp-clocks.txt   |  2 +-
>>>    arch/arm/boot/dts/r8a7790.dtsi                  | 16 ++++++++--------
>>>   arch/arm/boot/dts/r8a7791.dtsi                   | 18 +++++++++---------
>>>   drivers/clk/shmobile/clk-mstp.c                  |  2 +-
>>>   4 files changed, 19 insertions(+), 19 deletions(-)
>>
>> Thanks for your patches! Good to see more common bindings.
>>
>> Regarding how to roll it into the SoC code, I feel that this patch [2/3] and
>> next patch [3/3] potentially causes breakage with the existing binding that
>> was part of the v3.13 upstream kernel.
>
> If I'm not mistaken that's v3.14, not v3.13.
>
>> I'd like to maintain backwards compatibility for our users. Also, this patch
>> crosses subsystems which may not be entirely ideal from a merge perspective.
>>
>> If other people in the community agree with [1/3] then may I propose that
>> you rework the series into:
>>
>> [1/3] as-is is fine for me
>> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
>> code from [2/3] and merge with [3/3], stay backwards compatible.
>> [3/3] adjust the SoC DTS
>>
>> That would maintain backwards compatibility and keep existing users happy.
>
> We'll have to address this DT backward compatibility nonsense at some point.
> We can't consider all bindings as stable as soon as they hit mainline, that
> just won't fly. In this particular case, given that the driver got merged in
> v3.14-rc1, and that our mainline multiplatform boards in mainline are
> virtually useless in real products given their early stage of development, I
> would rather consider the clocks DT bindings as unstable for at least a couple
> more kernel versions.

:( as someone who is being paid to do this for a real product

In this case at-least the backwards compatibility is only a couple
of lines to check for the older name.


-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

Re: [PATCH 2/3] renesas: change to using clock-indices

[Magnus Damm]

Hi Laurent,

On Fri, Feb 14, 2014 at 10:21 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Friday 14 February 2014 11:32:08 Magnus Damm wrote:
>> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote:
>> > With the addition of clock-indices, we need to change the renesas
>> > clock implementation to use these instead of the local definition.
>> >
>> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> > ---
>> >
>> >  .../bindings/clock/renesas,cpg-mstp-clocks.txt   |  2 +-
>> >   arch/arm/boot/dts/r8a7790.dtsi                  | 16 ++++++++--------
>> >  arch/arm/boot/dts/r8a7791.dtsi                   | 18 +++++++++---------
>> >  drivers/clk/shmobile/clk-mstp.c                  |  2 +-
>> >  4 files changed, 19 insertions(+), 19 deletions(-)
>>
>> Thanks for your patches! Good to see more common bindings.
>>
>> Regarding how to roll it into the SoC code, I feel that this patch [2/3] and
>> next patch [3/3] potentially causes breakage with the existing binding that
>> was part of the v3.13 upstream kernel.
>
> If I'm not mistaken that's v3.14, not v3.13.

Nope, it's merged in v3.13. Looked it up before sending the email. =)

>> I'd like to maintain backwards compatibility for our users. Also, this patch
>> crosses subsystems which may not be entirely ideal from a merge perspective.
>>
>> If other people in the community agree with [1/3] then may I propose that
>> you rework the series into:
>>
>> [1/3] as-is is fine for me
>> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
>> code from [2/3] and merge with [3/3], stay backwards compatible.
>> [3/3] adjust the SoC DTS
>>
>> That would maintain backwards compatibility and keep existing users happy.
>
> We'll have to address this DT backward compatibility nonsense at some point.
> We can't consider all bindings as stable as soon as they hit mainline, that
> just won't fly. In this particular case, given that the driver got merged in
> v3.14-rc1, and that our mainline multiplatform boards in mainline are
> virtually useless in real products given their early stage of development, I
> would rather consider the clocks DT bindings as unstable for at least a couple
> more kernel versions.

We need to chat more frequently my friend.

Cheers,

/ magnus

Re: [PATCH 2/3] renesas: change to using clock-indices

[Laurent Pinchart]

Hi Magnus,

On Friday 14 February 2014 23:28:42 Magnus Damm wrote:
> On Fri, Feb 14, 2014 at 10:21 PM, Laurent Pinchart wrote:
> > On Friday 14 February 2014 11:32:08 Magnus Damm wrote:
> >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote:
> >> > With the addition of clock-indices, we need to change the renesas
> >> > clock implementation to use these instead of the local definition.
> >> > 
> >> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >> > ---
> >> > 
> >> >  .../bindings/clock/renesas,cpg-mstp-clocks.txt   |  2 +-
> >> >   arch/arm/boot/dts/r8a7790.dtsi                  | 16 ++++++++--------
> >> >  arch/arm/boot/dts/r8a7791.dtsi                   | 18 +++++++---------
> >> >  drivers/clk/shmobile/clk-mstp.c                  |  2 +-
> >> >  4 files changed, 19 insertions(+), 19 deletions(-)
> >> 
> >> Thanks for your patches! Good to see more common bindings.
> >> 
> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3]
> >> and next patch [3/3] potentially causes breakage with the existing
> >> binding that was part of the v3.13 upstream kernel.
> > 
> > If I'm not mistaken that's v3.14, not v3.13.
> 
> Nope, it's merged in v3.13. Looked it up before sending the email. =)

I might be missing something, but

$ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
      0       0       0
$ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
      6      48     309

> >> I'd like to maintain backwards compatibility for our users. Also, this
> >> patch crosses subsystems which may not be entirely ideal from a merge
> >> perspective.
> >> 
> >> If other people in the community agree with [1/3] then may I propose that
> >> you rework the series into:
> >> 
> >> [1/3] as-is is fine for me
> >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
> >> code from [2/3] and merge with [3/3], stay backwards compatible.
> >> [3/3] adjust the SoC DTS
> >> 
> >> That would maintain backwards compatibility and keep existing users
> >> happy.
> > 
> > We'll have to address this DT backward compatibility nonsense at some
> > point. We can't consider all bindings as stable as soon as they hit
> > mainline, that just won't fly. In this particular case, given that the
> > driver got merged in v3.14-rc1, and that our mainline multiplatform
> > boards in mainline are virtually useless in real products given their
> > early stage of development, I would rather consider the clocks DT
> > bindings as unstable for at least a couple more kernel versions.
> 
> We need to chat more frequently my friend.

Sure :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] renesas: change to using clock-indices

[Laurent Pinchart]

Hi Ben,

On Friday 14 February 2014 14:03:34 Ben Dooks wrote:
> On 14/02/14 13:21, Laurent Pinchart wrote:
> > On Friday 14 February 2014 11:32:08 Magnus Damm wrote:
> >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote:
> >>> With the addition of clock-indices, we need to change the renesas
> >>> clock implementation to use these instead of the local definition.
> >>> 
> >>> Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
> >>> ---
> >>> 
> >>>  .../bindings/clock/renesas,cpg-mstp-clocks.txt  |  2 +-
> >>>  arch/arm/boot/dts/r8a7790.dtsi                  | 16 ++++++++--------
> >>>  arch/arm/boot/dts/r8a7791.dtsi                  | 18 +++++++++---------
> >>>  drivers/clk/shmobile/clk-mstp.c                 |  2 +-
> >>>  4 files changed, 19 insertions(+), 19 deletions(-)
> >> 
> >> Thanks for your patches! Good to see more common bindings.
> >> 
> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3]
> >> and next patch [3/3] potentially causes breakage with the existing
> >> binding that was part of the v3.13 upstream kernel.
> > 
> > If I'm not mistaken that's v3.14, not v3.13.
> > 
> >> I'd like to maintain backwards compatibility for our users. Also, this
> >> patch crosses subsystems which may not be entirely ideal from a merge
> >> perspective.
> >> 
> >> If other people in the community agree with [1/3] then may I propose that
> >> you rework the series into:
> >> 
> >> [1/3] as-is is fine for me
> >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
> >> code from [2/3] and merge with [3/3], stay backwards compatible.
> >> [3/3] adjust the SoC DTS
> >> 
> >> That would maintain backwards compatibility and keep existing users
> >> happy.
> > 
> > We'll have to address this DT backward compatibility nonsense at some
> > point. We can't consider all bindings as stable as soon as they hit
> > mainline, that just won't fly. In this particular case, given that the
> > driver got merged in v3.14-rc1, and that our mainline multiplatform
> > boards in mainline are virtually useless in real products given their
> > early stage of development, I would rather consider the clocks DT
> > bindings as unstable for at least a couple more kernel versions.
>
> :( as someone who is being paid to do this for a real product

I understand that, and I won't nack the patch. The problem at hand is broader 
than this though, I don't think we should consider DT bindings as stable as 
soon as they hit mainline. A couple of kernel versions are needed to test the 
bindings. This has been discussed previously, we need a way to mark bindings 
as staging and later move them to a stable state.

> In this case at-least the backwards compatibility is only a couple
> of lines to check for the older name.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] renesas: change to using clock-indices

[Geert Uytterhoeven]

Hi Laurent, Magnus,

On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3]
>> >> and next patch [3/3] potentially causes breakage with the existing
>> >> binding that was part of the v3.13 upstream kernel.
>> >
>> > If I'm not mistaken that's v3.14, not v3.13.
>>
>> Nope, it's merged in v3.13. Looked it up before sending the email. =)
>
> I might be missing something, but
>
> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
>       0       0       0
> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
>       6      48     309

It's the published bindings that count, right?

Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
was added in commit f94859c215b6d977794108a1a9a101239e393c09
("clk: shmobile: Add MSTP clock support").

$ git tag --contains f94859c215b6d977794108a1a9a101239e393c09
v3.14-rc1
v3.14-rc2
$

Now, the bindings must be in flux, as the documentation doesn't match
the code, nor the example:

Documentation says:

+  - renesas,indices: Indices of the gate clocks into the group (0 to 31)

Example and implementation use "renesas,clock-indices":

                renesas,clock-indices = <
                        R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
                        R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
                        R8A7790_CLK_MMCIF0
                >;

Woops...

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/3] renesas: change to using clock-indices

[Laurent Pinchart]

Hi Geert,

On Saturday 15 February 2014 14:20:16 Geert Uytterhoeven wrote:
> Hi Laurent, Magnus,
> 
> On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote:
> >> >> Regarding how to roll it into the SoC code, I feel that this patch
> >> >> [2/3] and next patch [3/3] potentially causes breakage with the
> >> >> existing binding that was part of the v3.13 upstream kernel.
> >> > 
> >> > If I'm not mistaken that's v3.14, not v3.13.
> >> 
> >> Nope, it's merged in v3.13. Looked it up before sending the email. =)
> > 
> > I might be missing something, but
> > 
> > $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
> >       0       0       0
> > 
> > $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
> >       6      48     309
> 
> It's the published bindings that count, right?

I suppose both count, but they've been mainlined at the same time anyway.

> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> was added in commit f94859c215b6d977794108a1a9a101239e393c09
> ("clk: shmobile: Add MSTP clock support").
> 
> $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09
> v3.14-rc1
> v3.14-rc2
> $
> 
> Now, the bindings must be in flux, as the documentation doesn't match
> the code, nor the example:
> 
> Documentation says:
> 
> +  - renesas,indices: Indices of the gate clocks into the group (0 to 31)
> 
> Example and implementation use "renesas,clock-indices":
> 
>                 renesas,clock-indices = <
>                         R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1
> R8A7790_CLK_SDHI3 R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
> R8A7790_CLK_MMCIF0
> 
>                 >;
> 
> Woops...

You don't need to convince me that the DT bindings need time to stabilize :-)

I'll fix this after hearing from Mike on Ben's patches.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] renesas: change to using clock-indices

[Magnus Damm]

Hi Laurent,

On Fri, Feb 14, 2014 at 11:34 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Friday 14 February 2014 23:28:42 Magnus Damm wrote:
>> On Fri, Feb 14, 2014 at 10:21 PM, Laurent Pinchart wrote:
>> > On Friday 14 February 2014 11:32:08 Magnus Damm wrote:
>> >> On Fri, Feb 14, 2014 at 3:02 AM, Ben Dooks wrote:
>> >> > With the addition of clock-indices, we need to change the renesas
>> >> > clock implementation to use these instead of the local definition.
>> >> >
>> >> > Signed-off-by: Ben Dooks <ben.dooks@codethink.co.uk>
>> >> > ---
>> >> >
>> >> >  .../bindings/clock/renesas,cpg-mstp-clocks.txt   |  2 +-
>> >> >   arch/arm/boot/dts/r8a7790.dtsi                  | 16 ++++++++--------
>> >> >  arch/arm/boot/dts/r8a7791.dtsi                   | 18 +++++++---------
>> >> >  drivers/clk/shmobile/clk-mstp.c                  |  2 +-
>> >> >  4 files changed, 19 insertions(+), 19 deletions(-)
>> >>
>> >> Thanks for your patches! Good to see more common bindings.
>> >>
>> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3]
>> >> and next patch [3/3] potentially causes breakage with the existing
>> >> binding that was part of the v3.13 upstream kernel.
>> >
>> > If I'm not mistaken that's v3.14, not v3.13.
>>
>> Nope, it's merged in v3.13. Looked it up before sending the email. =)
>
> I might be missing something, but
>
> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
>       0       0       0
> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
>       6      48     309

You are right, sorry about that! I'm not sure how I managed to get to
that wrong conclusion.

>> >> I'd like to maintain backwards compatibility for our users. Also, this
>> >> patch crosses subsystems which may not be entirely ideal from a merge
>> >> perspective.
>> >>
>> >> If other people in the community agree with [1/3] then may I propose that
>> >> you rework the series into:
>> >>
>> >> [1/3] as-is is fine for me
>> >> [2/3] clk-mstp and documentation changes only - Take the clk-mstp.c
>> >> code from [2/3] and merge with [3/3], stay backwards compatible.
>> >> [3/3] adjust the SoC DTS
>> >>
>> >> That would maintain backwards compatibility and keep existing users
>> >> happy.
>> >
>> > We'll have to address this DT backward compatibility nonsense at some
>> > point. We can't consider all bindings as stable as soon as they hit
>> > mainline, that just won't fly. In this particular case, given that the
>> > driver got merged in v3.14-rc1, and that our mainline multiplatform
>> > boards in mainline are virtually useless in real products given their
>> > early stage of development, I would rather consider the clocks DT
>> > bindings as unstable for at least a couple more kernel versions.
>>
>> We need to chat more frequently my friend.
>
> Sure :-)

Good!

Thanks,

/ magnus

Re: [PATCH 2/3] renesas: change to using clock-indices

[Magnus Damm]

Hi Geert,

On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Laurent, Magnus,
>
> On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>> >> Regarding how to roll it into the SoC code, I feel that this patch [2/3]
>>> >> and next patch [3/3] potentially causes breakage with the existing
>>> >> binding that was part of the v3.13 upstream kernel.
>>> >
>>> > If I'm not mistaken that's v3.14, not v3.13.
>>>
>>> Nope, it's merged in v3.13. Looked it up before sending the email. =)
>>
>> I might be missing something, but
>>
>> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
>>       0       0       0
>> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
>>       6      48     309
>
> It's the published bindings that count, right?

Good question. I don't know actually.

> Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> was added in commit f94859c215b6d977794108a1a9a101239e393c09
> ("clk: shmobile: Add MSTP clock support").
>
> $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09
> v3.14-rc1
> v3.14-rc2
> $
>
> Now, the bindings must be in flux, as the documentation doesn't match
> the code, nor the example:
>
> Documentation says:
>
> +  - renesas,indices: Indices of the gate clocks into the group (0 to 31)
>
> Example and implementation use "renesas,clock-indices":
>
>                 renesas,clock-indices = <
>                         R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1 R8A7790_CLK_SDHI3
>                         R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1 R8A7790_CLK_SDHI0
>                         R8A7790_CLK_MMCIF0
>                 >;
>
> Woops...

Thanks for spotting this. I believe Laurent will fix this up in the
not so distant future.

Cheers,

/ magnus

Re: [PATCH 2/3] renesas: change to using clock-indices

[Laurent Pinchart]

Hi Magnus,

On Tuesday 18 February 2014 10:50:07 Magnus Damm wrote:
> On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven wrote:
> > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote:
> >>> >> Regarding how to roll it into the SoC code, I feel that this patch
> >>> >> [2/3] and next patch [3/3] potentially causes breakage with the
> >>> >> existing binding that was part of the v3.13 upstream kernel.
> >>> > 
> >>> > If I'm not mistaken that's v3.14, not v3.13.
> >>> 
> >>> Nope, it's merged in v3.13. Looked it up before sending the email. =)
> >> 
> >> I might be missing something, but
> >> 
> >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
> >>       0       0       0
> >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
> >>       6      48     309
> > 
> > It's the published bindings that count, right?
> 
> Good question. I don't know actually.
> 
> > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> > was added in commit f94859c215b6d977794108a1a9a101239e393c09
> > ("clk: shmobile: Add MSTP clock support").
> > 
> > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09
> > v3.14-rc1
> > v3.14-rc2
> > $
> > 
> > Now, the bindings must be in flux, as the documentation doesn't match
> > the code, nor the example:
> > 
> > Documentation says:
> > 
> > +  - renesas,indices: Indices of the gate clocks into the group (0 to 31)
> > 
> > Example and implementation use "renesas,clock-indices":
> >                 renesas,clock-indices = <
> >                         R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1
> >                         R8A7790_CLK_SDHI3
> >                         R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1
> >                         R8A7790_CLK_SDHI0
> >                         R8A7790_CLK_MMCIF0
> >                 >;
> > 
> > Woops...
> 
> Thanks for spotting this. I believe Laurent will fix this up in the not so
> distant future.

I plan to wait for Mike to comment on Ben's patches before addressing this.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 2/3] renesas: change to using clock-indices

[Magnus Damm]

Hi Laurent,

On Tue, Feb 18, 2014 at 9:14 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Magnus,
>
> On Tuesday 18 February 2014 10:50:07 Magnus Damm wrote:
>> On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven wrote:
>> > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote:
>> >>> >> Regarding how to roll it into the SoC code, I feel that this patch
>> >>> >> [2/3] and next patch [3/3] potentially causes breakage with the
>> >>> >> existing binding that was part of the v3.13 upstream kernel.
>> >>> >
>> >>> > If I'm not mistaken that's v3.14, not v3.13.
>> >>>
>> >>> Nope, it's merged in v3.13. Looked it up before sending the email. =)
>> >>
>> >> I might be missing something, but
>> >>
>> >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
>> >>       0       0       0
>> >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
>> >>       6      48     309
>> >
>> > It's the published bindings that count, right?
>>
>> Good question. I don't know actually.
>>
>> > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
>> > was added in commit f94859c215b6d977794108a1a9a101239e393c09
>> > ("clk: shmobile: Add MSTP clock support").
>> >
>> > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09
>> > v3.14-rc1
>> > v3.14-rc2
>> > $
>> >
>> > Now, the bindings must be in flux, as the documentation doesn't match
>> > the code, nor the example:
>> >
>> > Documentation says:
>> >
>> > +  - renesas,indices: Indices of the gate clocks into the group (0 to 31)
>> >
>> > Example and implementation use "renesas,clock-indices":
>> >                 renesas,clock-indices = <
>> >                         R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1
>> >                         R8A7790_CLK_SDHI3
>> >                         R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1
>> >                         R8A7790_CLK_SDHI0
>> >                         R8A7790_CLK_MMCIF0
>> >                 >;
>> >
>> > Woops...
>>
>> Thanks for spotting this. I believe Laurent will fix this up in the not so
>> distant future.
>
> I plan to wait for Mike to comment on Ben's patches before addressing this.

Can you please explain about the reason behind this? I may not have
enough detailed understanding about this matter, but to me it looks
like two separate issues that somehow are combined together:

A) patch series from ben - development targeted at future merge windows
B) patch to fix inconsistency between documentation and actual code -
fix for targeted for -rc?

So regardless what the community decision is about A) we probably want
to fix B) anyway, no?

Thanks,

/ magnus

Re: [PATCH 2/3] renesas: change to using clock-indices

[Laurent Pinchart]

Hi Magnus,

On Wednesday 19 February 2014 15:34:39 Magnus Damm wrote:
> On Tue, Feb 18, 2014 at 9:14 PM, Laurent Pinchart wrote:
> > On Tuesday 18 February 2014 10:50:07 Magnus Damm wrote:
> >> On Sat, Feb 15, 2014 at 10:20 PM, Geert Uytterhoeven wrote:
> >> > On Fri, Feb 14, 2014 at 3:34 PM, Laurent Pinchart wrote:
> >> >>> >> Regarding how to roll it into the SoC code, I feel that this patch
> >> >>> >> [2/3] and next patch [3/3] potentially causes breakage with the
> >> >>> >> existing binding that was part of the v3.13 upstream kernel.
> >> >>> > 
> >> >>> > If I'm not mistaken that's v3.14, not v3.13.
> >> >>> 
> >> >>> Nope, it's merged in v3.13. Looked it up before sending the email. =)
> >> >> 
> >> >> I might be missing something, but
> >> >> 
> >> >> $ git log v3.13 --oneline -- drivers/clk/shmobile/ | wc
> >> >>       0       0       0
> >> >> $ git log v3.14-rc1 --oneline -- drivers/clk/shmobile/ | wc
> >> >>       6      48     309
> >> > 
> >> > It's the published bindings that count, right?
> >> 
> >> Good question. I don't know actually.
> >> 
> >> > Documentation/devicetree/bindings/clock/renesas,cpg-mstp-clocks.txt
> >> > was added in commit f94859c215b6d977794108a1a9a101239e393c09
> >> > ("clk: shmobile: Add MSTP clock support").
> >> > 
> >> > $ git tag --contains f94859c215b6d977794108a1a9a101239e393c09
> >> > v3.14-rc1
> >> > v3.14-rc2
> >> > $
> >> > 
> >> > Now, the bindings must be in flux, as the documentation doesn't match
> >> > the code, nor the example:
> >> > 
> >> > Documentation says:
> >> > 
> >> > +  - renesas,indices: Indices of the gate clocks into the group (0 to
> >> > 31)
> >> > 
> >> > Example and implementation use "renesas,clock-indices":
> >> >                 renesas,clock-indices = <
> >> >                         R8A7790_CLK_TPU0 R8A7790_CLK_MMCIF1
> >> >                         R8A7790_CLK_SDHI3
> >> >                         R8A7790_CLK_SDHI2 R8A7790_CLK_SDHI1
> >> >                         R8A7790_CLK_SDHI0
> >> >                         R8A7790_CLK_MMCIF0
> >> >                 >;
> >> > 
> >> > Woops...
> >> 
> >> Thanks for spotting this. I believe Laurent will fix this up in the not
> >> so distant future.
> > 
> > I plan to wait for Mike to comment on Ben's patches before addressing
> > this.
> 
> Can you please explain about the reason behind this? I may not have
> enough detailed understanding about this matter, but to me it looks
> like two separate issues that somehow are combined together:
> 
> A) patch series from ben - development targeted at future merge windows
> B) patch to fix inconsistency between documentation and actual code -
> fix for targeted for -rc?
> 
> So regardless what the community decision is about A) we probably want
> to fix B) anyway, no?

My bad, I had v3.15 in mind, for which I thought there would be no need to fix 
this if Ben's patches were accepted. I'll submit a patch for v3.14-rc.

-- 
Regards,

Laurent Pinchart