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

[Andrew Jeffery]

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 at 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 at 0 {
> > +		reg = <0x00>;
> > +		cooling-levels = /bits/ 8 <125 151 177 203 229 255>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x00>;
> > +	};
> +
> > +	fan at 1 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x01>;
> > +	};
> +
> > +	fan at 2 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x02>;
> > +	};
> +
> > +	fan at 3 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x03>;
> > +	};
> +
> > +	fan at 4 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x04>;
> > +	};
> +
> > +	fan at 5 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x05>;
> > +	};
> +
> > +	fan at 6 {
> > +		reg = <0x00>;
> > +		aspeed,fan-tach-ch = /bits/ 8 <0x06>;
> > +	};
> +
> > +	fan at 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 at 1e786000 {
> > +				compatible = "aspeed,ast2500-pwm-tacho";
> > +				#address-cells = <1>;
> > +				#size-cells = <0>;
> > +				reg = <0x1e786000 0x1000>;
> > +				clocks = <&pwm_tacho_fixed_clk>;
> > +				status = "disabled";
> > +			};
> >  		};
> >  	};
>  };
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 801 bytes
Desc: This is a digitally signed message part
URL: <http://lists.ozlabs.org/pipermail/openbmc/attachments/20170830/1ff4d6aa/attachment.sig>