| Message ID | 20231117125919.1696980-6-jbrunet@baylibre.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | pwm: meson: dt-bindings fixup | expand |
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
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
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 --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 {
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(-)