[{"id":1759870,"web_url":"http://patchwork.ozlabs.org/comment/1759870/","msgid":"<1504069726.5933.23.camel@aj.id.au>","list_archive_url":null,"date":"2017-08-30T05:08:46","subject":"Re: [PATCH linux dev-4.10 v2] dts: add aspeed-pwm-tacho to msn dts.","submitter":{"id":68332,"url":"http://patchwork.ozlabs.org/api/people/68332/","name":"Andrew Jeffery","email":"andrew@aj.id.au"},"content":"Hi Mykola\n\nOn Tue, 2017-08-29 at 18:22 +0300, Mykola Kostenok wrote:\n> Add basic pwm-tacho-controller node to ast-g5 dtsi.\n> \n> Add pwm-tacho to aspeed-bmc-mellanox-msn.dts.\n\nA couple of points I should have addressed in my previous reply (but\ndidn't so I won't burden you with them now):\n\n* This should be split into two patches, one adding the necessary nodes\nto aspeed-g5.dtsi (a general capability), and the second adding the\nnodes to aspeed-bmc-mellanox-msn.dts (machine-specific configuration).\n\n* Describe the tricky bits of the patch in the commit message. For\ninstance, the non-obvious backwards nature of the bindings means weird\nsemantics for the reg property in the fan nodes. Also, a bit of\ninformation on your fan topology (driving the 8 fans from one PWM)\nwould be good.\n\nGenerally in commit messages I look for descriptions of the motivation\nfor the patch (why), not the content of the change (how/what): The diff\nitself describes how/what. Adding the answer to 'why' in the commit\nmessage helps others to evaluate whether the implementation is the\nright way to go. Otherwise we're stuck reverse-engineering your intent\nfrom your implementation, which is a bit of a hindrance to identifying\nbugs or better approaches.\n\nTalking about implementations you tried and found not to work is also very\nuseful as it will put a stop to reviewers exploring and suggesting these\ndead-ends themselves. This point is not applicable to your patch here as you\nare modifying a devicetree (which is declarative, not imperative), but is\nuseful to keep in mind.\n\nAnyway, thanks for the change, I've applied it to dev-4.10 with some\nmodifications to the commit message to address the points above. I'll take the\nconversations about the bindings upstream.\n\nAndrew\n\n> \n> > Signed-off-by: Mykola Kostenok \n> --\n> v1 -> v2:\n>  Pointed out by Andrew Jeffery:\n>  - rebased.\n>  - remove exist #size-cells and #adress-cells in dtsi from dts.\n> ---\n>  arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts | 47 +++++++++++++++++++++++++++\n>  arch/arm/boot/dts/aspeed-g5.dtsi              | 17 ++++++++++\n>  2 files changed, 64 insertions(+)\n> \n> diff --git a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts\n> index 0774959b1222..b7854e36f052 100644\n> --- a/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts\n> +++ b/arch/arm/boot/dts/aspeed-bmc-mellanox-msn.dts\n> @@ -139,3 +139,50 @@\n> >  \tstatus = \"okay\";\n>  };\n>  \n> +&pwm_tacho {\n> > +\tstatus = \"okay\";\n> > +\tpinctrl-names = \"default\";\n> > +\tpinctrl-0 = <&pinctrl_pwm0_default>;\n> > +\t#cooling-cells = <2>;\n> +\n> > > +\tcooling: fan@0 {\n> > +\t\treg = <0x00>;\n> > +\t\tcooling-levels = /bits/ 8 <125 151 177 203 229 255>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x00>;\n> > +\t};\n> +\n> > +\tfan@1 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x01>;\n> > +\t};\n> +\n> > +\tfan@2 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x02>;\n> > +\t};\n> +\n> > +\tfan@3 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x03>;\n> > +\t};\n> +\n> > +\tfan@4 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x04>;\n> > +\t};\n> +\n> > +\tfan@5 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x05>;\n> > +\t};\n> +\n> > +\tfan@6 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x06>;\n> > +\t};\n> +\n> > +\tfan@7 {\n> > +\t\treg = <0x00>;\n> > +\t\taspeed,fan-tach-ch = /bits/ 8 <0x07>;\n> > +\t};\n> +};\n> diff --git a/arch/arm/boot/dts/aspeed-g5.dtsi b/arch/arm/boot/dts/aspeed-g5.dtsi\n> index cdea4f4eb77c..bb7652a61bec 100644\n> --- a/arch/arm/boot/dts/aspeed-g5.dtsi\n> +++ b/arch/arm/boot/dts/aspeed-g5.dtsi\n> @@ -41,6 +41,14 @@\n> >  \t\tserial5 = &vuart;\n> >  \t};\n>  \n> > +\tclocks {\n> > +\t\tpwm_tacho_fixed_clk: fixedclk {\n> > +\t\t\tcompatible = \"fixed-clock\";\n> > +\t\t\t#clock-cells = <0>;\n> > +\t\t\tclock-frequency = <24000000>;\n> > +\t\t};\n> > +\t};\n> +\n> >  \tahb {\n> >  \t\tcompatible = \"simple-bus\";\n> >  \t\t#address-cells = <1>;\n> @@ -373,6 +381,15 @@\n> >  \t\t\t\t#size-cells = <1>;\n> >  \t\t\t\tranges = <0 0x1e78a000 0x1000>;\n> >  \t\t\t};\n> +\n> > > +\t\t\tpwm_tacho: pwm-tacho-controller@1e786000 {\n> > +\t\t\t\tcompatible = \"aspeed,ast2500-pwm-tacho\";\n> > +\t\t\t\t#address-cells = <1>;\n> > +\t\t\t\t#size-cells = <0>;\n> > +\t\t\t\treg = <0x1e786000 0x1000>;\n> > +\t\t\t\tclocks = <&pwm_tacho_fixed_clk>;\n> > +\t\t\t\tstatus = \"disabled\";\n> > +\t\t\t};\n> >  \t\t};\n> >  \t};\n>  };","headers":{"Return-Path":"","X-Original-To":["incoming@patchwork.ozlabs.org","openbmc@lists.ozlabs.org"],"Delivered-To":["patchwork-incoming@bilbo.ozlabs.org","openbmc@lists.ozlabs.org"],"Received":["from lists.ozlabs.org (lists.ozlabs.org [103.22.144.68])\n\t(using TLSv1.2 with cipher ADH-AES256-GCM-SHA384 (256/256 bits))\n\t(No client certificate requested)\n\tby ozlabs.org (Postfix) with ESMTPS id 3xhtp45zCFz9sP5\n\tfor ;\n\tWed, 30 Aug 2017 15:09:00 +1000 (AEST)","from lists.ozlabs.org (lists.ozlabs.org [IPv6:2401:3900:2:1::3])\n\tby lists.ozlabs.org (Postfix) with ESMTP id 3xhtp44m1FzDqJ8\n\tfor ;\n\tWed, 30 Aug 2017 15:09:00 +1000 (AEST)","from out2-smtp.messagingengine.com (out2-smtp.messagingengine.com\n\t[66.111.4.26])\n\t(using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits))\n\t(No client certificate requested)\n\tby lists.ozlabs.org (Postfix) with ESMTPS id 3xhtny4FT3zDq5k\n\tfor ; Wed, 30 Aug 2017 15:08:54 +1000 (AEST)","from compute4.internal (compute4.nyi.internal [10.202.2.44])\n\tby mailout.nyi.internal (Postfix) with ESMTP id 3C34020F5F;\n\tWed, 30 Aug 2017 01:08:52 -0400 (EDT)","from frontend2 ([10.202.2.161])\n\tby compute4.internal (MEProxy); Wed, 30 Aug 2017 01:08:52 -0400","from keelia (unknown [203.0.153.9])\n\tby mail.messagingengine.com (Postfix) with ESMTPA id D5B1724A82;\n\tWed, 30 Aug 2017 01:08:49 -0400 (EDT)"],"Authentication-Results":["ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"RydFJ7+M\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"OE3fw0rk\"; \n\tdkim-atps=neutral","lists.ozlabs.org;\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"RydFJ7+M\";\n\tdkim=fail reason=\"signature verification failed\" (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com header.b=\"OE3fw0rk\"; \n\tdkim-atps=neutral","lists.ozlabs.org; dkim=pass (2048-bit key;\n\tunprotected) header.d=aj.id.au header.i=@aj.id.au header.b=\"RydFJ7+M\";\n\tdkim=pass (2048-bit key;\n\tunprotected) header.d=messagingengine.com\n\theader.i=@messagingengine.com\n\theader.b=\"OE3fw0rk\"; dkim-atps=neutral"],"DKIM-Signature":["v=1; a=rsa-sha256; c=relaxed/relaxed; d=aj.id.au; h=cc\n\t:content-type:date:from:in-reply-to:message-id:mime-version\n\t:references:subject:to:x-me-sender:x-me-sender:x-sasl-enc\n\t:x-sasl-enc; s=fm1; bh=n51n5GFNV341g+f4ud5S/jUvdT5dVmWnZV/yAkEnl\n\tt8=; b=RydFJ7+M7Zm6nxlySj17XsyVtfUNVfA6kj9owrCmeNgkjSsreKhRvR6qd\n\ti60nFBBtzdQFo4yiFVKGu/682cg/MS5aq850h1sWZw/pafrAlsXYda9icvC48WqV\n\tDqLLHy67JMIHqVSnasBIOUEvRfbttK6VLsBJRWvsXNy2F1WgDYrTm0uk10gFcbcv\n\tQoCOl+A/gwj6d0BfaS1l2oxfQh6tpoywlHVB93Hpo1X71+tT08sSM7E9SNsyZjx1\n\tiCQWrKGfFyUWnScLQdg3NdM9SRLb7VTqDgnd0vDU570lifqVxYWWSzub27FKan7O\n\tuERtRKDy2OCYL/oipgM0ihk1KxAVA==","v=1; a=rsa-sha256; c=relaxed/relaxed; d=\n\tmessagingengine.com; h=cc:content-type:date:from:in-reply-to\n\t:message-id:mime-version:references:subject:to:x-me-sender\n\t:x-me-sender:x-sasl-enc:x-sasl-enc; s=fm1; bh=n51n5GFNV341g+f4ud\n\t5S/jUvdT5dVmWnZV/yAkEnlt8=; b=OE3fw0rkeZLmlqDWBDqpWEIxsGS5K08VBF\n\tgCLv9P6kwtEuXcCkRmW4wbVu7HM79gWHniUcWtUJX4ybr/4z0pkwe65gVV8GxAID\n\trzSd2YcQdlrdhi42SFCBZtlEjB/sPAJSnfUOYu0iyoxFuDgpetc6jMuab4gEoD1W\n\tWGPC0r/uSwQzJtOX4ci1Ri9nb7V/kVCQFwPd717KwiHTjRElgU/EigeQPdokiNzt\n\tJNyPHW1ewSnQuH8nO1zrwa6j0aoSR2T/lcKuJov7CfL1n5l2SfOTd8DrJzrpuQfL\n\t0VQDRxr4nxZtL1yQyB7kYJ06aYzjoJuM08lJFgx2UYHC4KjBCafg=="],"X-ME-Sender":"","X-Sasl-enc":"AeVbo7Fhsm7AfBOZnjVpx3JS4zDHdXp9aocC0jWzrwoe 1504069731","Message-ID":"<1504069726.5933.23.camel@aj.id.au>","Subject":"Re: [PATCH linux dev-4.10 v2] dts: add aspeed-pwm-tacho to msn dts.","From":"Andrew Jeffery ","To":"Mykola Kostenok , Joel Stanley , \n\topenbmc@lists.ozlabs.org","Date":"Wed, 30 Aug 2017 14:38:46 +0930","In-Reply-To":"<20170829152202.122188-1-c_mykolak@mellanox.com>","References":"<20170829152202.122188-1-c_mykolak@mellanox.com>","Content-Type":"multipart/signed; micalg=\"pgp-sha512\";\n\tprotocol=\"application/pgp-signature\";\n\tboundary=\"=-WYu+D83W3nNw1326QtFU\"","X-Mailer":"Evolution 3.22.6-1ubuntu1 ","Mime-Version":"1.0","X-BeenThere":"openbmc@lists.ozlabs.org","X-Mailman-Version":"2.1.23","Precedence":"list","List-Id":"Development list for OpenBMC ","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"Ohad Oz , Vadim Pasternak ","Errors-To":"openbmc-bounces+incoming=patchwork.ozlabs.org@lists.ozlabs.org","Sender":"\"openbmc\"\n\t"}}]