[PATCH linux dev-4.10 v2] dts: add aspeed-pwm-tacho to msn dts.

[PATCH linux dev-4.10 v2] dts: add aspeed-pwm-tacho to msn dts.

[Mykola Kostenok]

Add basic pwm-tacho-controller node to ast-g5 dtsi.

Add pwm-tacho to aspeed-bmc-mellanox-msn.dts.

Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com>
--
v1 -> v2:
 Pointed out by Andrew Jeffery:
 - rebased.
 - remove exist #size-cells and #adress-cells in dtsi from dts.
---
 arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 47 +++++++++++++++++++++++++++
 arch/arm/boot/dts/aspeed-g5.dtsi              | 17 ++++++++++
 2 files changed, 64 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
index 0774959b1222..b7854e36f052 100644
--- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
@@ -139,3 +139,50 @@
 	status = "okay";
 };
 
+&pwm_tacho {
+	status = "okay";
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_pwm0_default>;
+	#cooling-cells = <2>;
+
+	cooling: fan@0 {
+		reg = <0x00>;
+		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
+	};
+
+	fan@1 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x01>;
+	};
+
+	fan@2 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x02>;
+	};
+
+	fan@3 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x03>;
+	};
+
+	fan@4 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x04>;
+	};
+
+	fan@5 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x05>;
+	};
+
+	fan@6 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x06>;
+	};
+
+	fan@7 {
+		reg = <0x00>;
+		aspeed,fan-tach-ch = /bits/ 8 <0x07>;
+	};
+};
diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
index cdea4f4eb77c..bb7652a61bec 100644
--- a/arch/arm/boot/dts/aspeed-g5.dtsi
+++ b/arch/arm/boot/dts/aspeed-g5.dtsi
@@ -41,6 +41,14 @@
 		serial5 = &vuart;
 	};
 
+	clocks {
+		pwm_tacho_fixed_clk: fixedclk {
+			compatible = "fixed-clock";
+			#clock-cells = <0>;
+			clock-frequency = <24000000>;
+		};
+	};
+
 	ahb {
 		compatible = "simple-bus";
 		#address-cells = <1>;
@@ -373,6 +381,15 @@
 				#size-cells = <1>;
 				ranges = <0 0x1e78a000 0x1000>;
 			};
+
+			pwm_tacho: pwm-tacho-controller@1e786000 {
+				compatible = "aspeed,ast2500-pwm-tacho";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0x1e786000 0x1000>;
+				clocks = <&pwm_tacho_fixed_clk>;
+				status = "disabled";
+			};
 		};
 	};
 };
-- 
2.11.0

Re: [PATCH linux dev-4.10 v2] dts: add aspeed-pwm-tacho to msn dts.

[Andrew Jeffery]

[-- Attachment #1: Type: text/plain, Size: 4536 bytes --]

Hi Mykola

On Tue, 2017-08-29 at 18:22 +0300, Mykola Kostenok wrote:
> Add basic pwm-tacho-controller node to ast-g5 dtsi.
> 
> Add pwm-tacho to aspeed-bmc-mellanox-msn.dts.

A couple of points I should have addressed in my previous reply (but
didn't so I won't burden you with them now):

* This should be split into two patches, one adding the necessary nodes
to aspeed-g5.dtsi (a general capability), and the second adding the
nodes to aspeed-bmc-mellanox-msn.dts (machine-specific configuration).

* Describe the tricky bits of the patch in the commit message. For
instance, the non-obvious backwards nature of the bindings means weird
semantics for the reg property in the fan nodes. Also, a bit of
information on your fan topology (driving the 8 fans from one PWM)
would be good.

Generally in commit messages I look for descriptions of the motivation
for the patch (why), not the content of the change (how/what): The diff
itself describes how/what. Adding the answer to 'why' in the commit
message helps others to evaluate whether the implementation is the
right way to go. Otherwise we're stuck reverse-engineering your intent
from your implementation, which is a bit of a hindrance to identifying
bugs or better approaches.

Talking about implementations you tried and found not to work is also very
useful as it will put a stop to reviewers exploring and suggesting these
dead-ends themselves. This point is not applicable to your patch here as you
are modifying a devicetree (which is declarative, not imperative), but is
useful to keep in mind.

Anyway, thanks for the change, I've applied it to dev-4.10 with some
modifications to the commit message to address the points above. I'll take the
conversations about the bindings upstream.

Andrew

> 
> > Signed-off-by: Mykola Kostenok <c_mykolak@mellanox.com>
> --
> v1 -> v2:
>  Pointed out by Andrew Jeffery:
>  - rebased.
>  - remove exist #size-cells and #adress-cells in dtsi from dts.
> ---
>  arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 47 +++++++++++++++++++++++++++
>  arch/arm/boot/dts/aspeed-g5.dtsi              | 17 ++++++++++
>  2 files changed, 64 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> index 0774959b1222..b7854e36f052 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts
> @@ -139,3 +139,50 @@
> >  	status = "okay";
>  };
>  
> +&pwm_tacho {
> > +	status = "okay";
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&pinctrl_pwm0_default>;
> > +	#cooling-cells = <2>;
> +
> > > +	cooling: fan@0 {
> > +		reg = <0x00>;
> > +		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> > +	};
> +
> > +	fan@1 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x01>;
> > +	};
> +
> > +	fan@2 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> > +	};
> +
> > +	fan@3 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> > +	};
> +
> > +	fan@4 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> > +	};
> +
> > +	fan@5 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> > +	};
> +
> > +	fan@6 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x06>;
> > +	};
> +
> > +	fan@7 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x07>;
> > +	};
> +};
> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi
> index cdea4f4eb77c..bb7652a61bec 100644
> --- a/arch/arm/boot/dts/aspeed-g5.dtsi
> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi
> @@ -41,6 +41,14 @@
> >  		serial5 = &vuart;
> >  	};
>  
> > +	clocks {
> > +		pwm_tacho_fixed_clk: fixedclk {
> > +			compatible = "fixed-clock";
> > +			#clock-cells = <0>;
> > +			clock-frequency = <24000000>;
> > +		};
> > +	};
> +
> >  	ahb {
> >  		compatible = "simple-bus";
> >  		#address-cells = <1>;
> @@ -373,6 +381,15 @@
> >  				#size-cells = <1>;
> >  				ranges = <0 0x1e78a000 0x1000>;
> >  			};
> +
> > > +			pwm_tacho: pwm-tacho-controller@1e786000 {
> > +				compatible = "aspeed,ast2500-pwm-tacho";
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <0x1e786000 0x1000>;
> > +				clocks = <&pwm_tacho_fixed_clk>;
> > +				status = "disabled";
> > +			};
> >  		};
> >  	};
>  };

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 801 bytes --]