diff mbox series

[v2,5/6] arm: dts: amlogic: migrate pwms to new meson8 v2 binding

Message ID 20231117125919.1696980-6-jbrunet@baylibre.com
State New
Headers
Series pwm: meson: dt-bindings fixup |

Commit Message

Jerome Brunet Nov. 17, 2023, 12:59 p.m. UTC 
  Update Amlogic based SoC PWMs to meson8-pwm-v2 compatible

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 arch/arm/boot/dts/amlogic/meson.dtsi           |  4 ++--
 arch/arm/boot/dts/amlogic/meson8.dtsi          | 16 +++++++++++++---
 arch/arm/boot/dts/amlogic/meson8b-ec100.dts    |  2 --
 arch/arm/boot/dts/amlogic/meson8b-mxq.dts      |  2 --
 arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts |  2 --
 arch/arm/boot/dts/amlogic/meson8b.dtsi         | 16 +++++++++++++---
 6 files changed, 28 insertions(+), 14 deletions(-)

Comments

Krzysztof Kozlowski Nov. 22, 2023, 8:39 a.m. UTC | #1 
On 17/11/2023 13:59, Jerome Brunet wrote:
> Update Amlogic based SoC PWMs to meson8-pwm-v2 compatible

Why? Your commit msg must explain this. You break users of this DTS on
older kernels and also this makes it impossible to apply via different
branches in the same cycle. All this needs explanation and proper
justification. Your message tells here nothing, because "what" is quite
obvious.

> 
> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
> ---
>  arch/arm/boot/dts/amlogic/meson.dtsi           |  4 ++--
>  arch/arm/boot/dts/amlogic/meson8.dtsi          | 16 +++++++++++++---
>  arch/arm/boot/dts/amlogic/meson8b-ec100.dts    |  2 --
>  arch/arm/boot/dts/amlogic/meson8b-mxq.dts      |  2 --
>  arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts |  2 --
>  arch/arm/boot/dts/amlogic/meson8b.dtsi         | 16 +++++++++++++---
>  6 files changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/amlogic/meson.dtsi b/arch/arm/boot/dts/amlogic/meson.dtsi
> index 8e3860d5d916..80cc004ad5fe 100644
> --- a/arch/arm/boot/dts/amlogic/meson.dtsi
> +++ b/arch/arm/boot/dts/amlogic/meson.dtsi
> @@ -83,14 +83,14 @@ i2c_A: i2c@8500 {
>  			};
>  
>  			pwm_ab: pwm@8550 {
> -				compatible = "amlogic,meson-pwm";
> +				compatible = "amlogic,meson8-pwm-v2";

That's breaking users of this DTS (old kernel, out of tree, other
projects) for no real reasons without explanation.

Best regards,
Krzysztof
Jerome Brunet Nov. 22, 2023, 2:52 p.m. UTC | #2 
On Wed 22 Nov 2023 at 09:39, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 17/11/2023 13:59, Jerome Brunet wrote:
>> Update Amlogic based SoC PWMs to meson8-pwm-v2 compatible
>
> Why? Your commit msg must explain this. You break users of this DTS on
> older kernels and also this makes it impossible to apply via different
> branches in the same cycle. All this needs explanation and proper
> justification. Your message tells here nothing, because "what" is quite
> obvious.
>

I provided all the explanation possible through the different commits of
this series. I can re-state here if it helps

>> 
>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>> ---
>>  arch/arm/boot/dts/amlogic/meson.dtsi           |  4 ++--
>>  arch/arm/boot/dts/amlogic/meson8.dtsi          | 16 +++++++++++++---
>>  arch/arm/boot/dts/amlogic/meson8b-ec100.dts    |  2 --
>>  arch/arm/boot/dts/amlogic/meson8b-mxq.dts      |  2 --
>>  arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts |  2 --
>>  arch/arm/boot/dts/amlogic/meson8b.dtsi         | 16 +++++++++++++---
>>  6 files changed, 28 insertions(+), 14 deletions(-)
>> 
>> diff --git a/arch/arm/boot/dts/amlogic/meson.dtsi b/arch/arm/boot/dts/amlogic/meson.dtsi
>> index 8e3860d5d916..80cc004ad5fe 100644
>> --- a/arch/arm/boot/dts/amlogic/meson.dtsi
>> +++ b/arch/arm/boot/dts/amlogic/meson.dtsi
>> @@ -83,14 +83,14 @@ i2c_A: i2c@8500 {
>>  			};
>>  
>>  			pwm_ab: pwm@8550 {
>> -				compatible = "amlogic,meson-pwm";
>> +				compatible = "amlogic,meson8-pwm-v2";
>
> That's breaking users of this DTS (old kernel, out of tree, other
> projects) for no real reasons without explanation.

"amlogic,meson-pwm" will continue to match, meaning of bindings is unchanged

How do you propose to fix badly designed bindings then ?

if we cant even introduce a new compatible to fix things up. It is supposed to
stay and broken till the end of time ?

>
> Best regards,
> Krzysztof
Krzysztof Kozlowski Nov. 22, 2023, 3:10 p.m. UTC | #3 
On 22/11/2023 15:52, Jerome Brunet wrote:
> 
> On Wed 22 Nov 2023 at 09:39, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 17/11/2023 13:59, Jerome Brunet wrote:
>>> Update Amlogic based SoC PWMs to meson8-pwm-v2 compatible
>>
>> Why? Your commit msg must explain this. You break users of this DTS on
>> older kernels and also this makes it impossible to apply via different
>> branches in the same cycle. All this needs explanation and proper
>> justification. Your message tells here nothing, because "what" is quite
>> obvious.
>>
> 
> I provided all the explanation possible through the different commits of
> this series. I can re-state here if it helps

DTS commits stand on their own and must not go via same branch as
driver, so how does driver commit msg help Git history?

> 
>>>
>>> Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
>>> ---
>>>  arch/arm/boot/dts/amlogic/meson.dtsi           |  4 ++--
>>>  arch/arm/boot/dts/amlogic/meson8.dtsi          | 16 +++++++++++++---
>>>  arch/arm/boot/dts/amlogic/meson8b-ec100.dts    |  2 --
>>>  arch/arm/boot/dts/amlogic/meson8b-mxq.dts      |  2 --
>>>  arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts |  2 --
>>>  arch/arm/boot/dts/amlogic/meson8b.dtsi         | 16 +++++++++++++---
>>>  6 files changed, 28 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/arm/boot/dts/amlogic/meson.dtsi b/arch/arm/boot/dts/amlogic/meson.dtsi
>>> index 8e3860d5d916..80cc004ad5fe 100644
>>> --- a/arch/arm/boot/dts/amlogic/meson.dtsi
>>> +++ b/arch/arm/boot/dts/amlogic/meson.dtsi
>>> @@ -83,14 +83,14 @@ i2c_A: i2c@8500 {
>>>  			};
>>>  
>>>  			pwm_ab: pwm@8550 {
>>> -				compatible = "amlogic,meson-pwm";
>>> +				compatible = "amlogic,meson8-pwm-v2";
>>
>> That's breaking users of this DTS (old kernel, out of tree, other
>> projects) for no real reasons without explanation.
> 
> "amlogic,meson-pwm" will continue to match, meaning of bindings is unchanged

No, because new DTS does not have amlogic,meson-pwm, thus all existing
users see breakage.

> 
> How do you propose to fix badly designed bindings then ?

Justify and introduce incompatible changes, breaking the ABI. Anyway
this is a requirement, because, as I said in other reply, you cannot
have compatible for software model!

> 
> if we cant even introduce a new compatible to fix things up. It is supposed to
> stay and broken till the end of time ?

No, you cannot introduce new compatible for new OS.

Fix the bindings instead with proper justification. We did it many
times, what's the problem here?

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/amlogic/meson.dtsi b/arch/arm/boot/dts/amlogic/meson.dtsi
index 8e3860d5d916..80cc004ad5fe 100644
--- a/arch/arm/boot/dts/amlogic/meson.dtsi
+++ b/arch/arm/boot/dts/amlogic/meson.dtsi
@@ -83,14 +83,14 @@  i2c_A: i2c@8500 {
 			};
 
 			pwm_ab: pwm@8550 {
-				compatible = "amlogic,meson-pwm";
+				compatible = "amlogic,meson8-pwm-v2";
 				reg = <0x8550 0x10>;
 				#pwm-cells = <3>;
 				status = "disabled";
 			};
 
 			pwm_cd: pwm@8650 {
-				compatible = "amlogic,meson-pwm";
+				compatible = "amlogic,meson8-pwm-v2";
 				reg = <0x8650 0x10>;
 				#pwm-cells = <3>;
 				status = "disabled";
diff --git a/arch/arm/boot/dts/amlogic/meson8.dtsi b/arch/arm/boot/dts/amlogic/meson8.dtsi
index 59932fbfd5d5..153b8fe9c506 100644
--- a/arch/arm/boot/dts/amlogic/meson8.dtsi
+++ b/arch/arm/boot/dts/amlogic/meson8.dtsi
@@ -450,10 +450,14 @@  analog_top: analog-top@81a8 {
 	};
 
 	pwm_ef: pwm@86c0 {
-		compatible = "amlogic,meson8-pwm", "amlogic,meson8b-pwm";
+		compatible = "amlogic,meson8-pwm-v2";
 		reg = <0x86c0 0x10>;
 		#pwm-cells = <3>;
 		status = "disabled";
+		clocks = <&xtal>,
+			 <0>,
+			 <&clkc CLKID_FCLK_DIV4>,
+			 <&clkc CLKID_FCLK_DIV3>;
 	};
 
 	clock-measure@8758 {
@@ -702,11 +706,17 @@  timer@600 {
 };
 
 &pwm_ab {
-	compatible = "amlogic,meson8-pwm", "amlogic,meson8b-pwm";
+	clocks = <&xtal>,
+		 <0>,
+		 <&clkc CLKID_FCLK_DIV4>,
+		 <&clkc CLKID_FCLK_DIV3>;
 };
 
 &pwm_cd {
-	compatible = "amlogic,meson8-pwm", "amlogic,meson8b-pwm";
+	clocks = <&xtal>,
+		 <0>,
+		 <&clkc CLKID_FCLK_DIV4>,
+		 <&clkc CLKID_FCLK_DIV3>;
 };
 
 &rtc {
diff --git a/arch/arm/boot/dts/amlogic/meson8b-ec100.dts b/arch/arm/boot/dts/amlogic/meson8b-ec100.dts
index 3da47349eaaf..cdd7d04db256 100644
--- a/arch/arm/boot/dts/amlogic/meson8b-ec100.dts
+++ b/arch/arm/boot/dts/amlogic/meson8b-ec100.dts
@@ -441,8 +441,6 @@  &pwm_cd {
 	status = "okay";
 	pinctrl-0 = <&pwm_c1_pins>, <&pwm_d_pins>;
 	pinctrl-names = "default";
-	clocks = <&xtal>, <&xtal>;
-	clock-names = "clkin0", "clkin1";
 };
 
 &rtc {
diff --git a/arch/arm/boot/dts/amlogic/meson8b-mxq.dts b/arch/arm/boot/dts/amlogic/meson8b-mxq.dts
index 7adedd3258c3..68f4f70f4f03 100644
--- a/arch/arm/boot/dts/amlogic/meson8b-mxq.dts
+++ b/arch/arm/boot/dts/amlogic/meson8b-mxq.dts
@@ -162,8 +162,6 @@  &pwm_cd {
 	status = "okay";
 	pinctrl-0 = <&pwm_c1_pins>, <&pwm_d_pins>;
 	pinctrl-names = "default";
-	clocks = <&xtal>, <&xtal>;
-	clock-names = "clkin0", "clkin1";
 };
 
 &uart_AO {
diff --git a/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts b/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
index 941682844faf..ff955b960688 100644
--- a/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
+++ b/arch/arm/boot/dts/amlogic/meson8b-odroidc1.dts
@@ -347,8 +347,6 @@  &pwm_cd {
 	status = "okay";
 	pinctrl-0 = <&pwm_c1_pins>, <&pwm_d_pins>;
 	pinctrl-names = "default";
-	clocks = <&xtal>, <&xtal>;
-	clock-names = "clkin0", "clkin1";
 };
 
 &rtc {
diff --git a/arch/arm/boot/dts/amlogic/meson8b.dtsi b/arch/arm/boot/dts/amlogic/meson8b.dtsi
index 5198f5177c2c..6c91eda92e8b 100644
--- a/arch/arm/boot/dts/amlogic/meson8b.dtsi
+++ b/arch/arm/boot/dts/amlogic/meson8b.dtsi
@@ -404,10 +404,14 @@  analog_top: analog-top@81a8 {
 	};
 
 	pwm_ef: pwm@86c0 {
-		compatible = "amlogic,meson8b-pwm";
+		compatible = "amlogic,meson8-pwm-v2";
 		reg = <0x86c0 0x10>;
 		#pwm-cells = <3>;
 		status = "disabled";
+		clocks = <&xtal>,
+			 <0>,
+			 <&clkc CLKID_FCLK_DIV4>,
+			 <&clkc CLKID_FCLK_DIV3>;
 	};
 
 	clock-measure@8758 {
@@ -677,11 +681,17 @@  timer@600 {
 };
 
 &pwm_ab {
-	compatible = "amlogic,meson8b-pwm";
+	clocks = <&xtal>,
+		 <0>,
+		 <&clkc CLKID_FCLK_DIV4>,
+		 <&clkc CLKID_FCLK_DIV3>;
 };
 
 &pwm_cd {
-	compatible = "amlogic,meson8b-pwm";
+	clocks = <&xtal>,
+		 <0>,
+		 <&clkc CLKID_FCLK_DIV4>,
+		 <&clkc CLKID_FCLK_DIV3>;
 };
 
 &rtc {