<div dir="ltr"><font face="arial, helvetica, sans-serif"><br></font><div class="gmail_extra"><font face="arial, helvetica, sans-serif"><br></font><div class="gmail_quote"><font face="arial, helvetica, sans-serif">On Fri, Jan 13, 2017 at 8:21 AM, Rob Herring <span dir="ltr"><<a href="mailto:robh@kernel.org" target="_blank">robh@kernel.org</a>></span> wrote:<br></font><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><font face="arial, helvetica, sans-serif"><span class="gmail-">On Mon, Jan 09, 2017 at 01:59:34PM -0800, Jaghathiswari Rankappagounder Natarajan wrote:<br>
> This binding provides interface for adding values related to ASPEED<br>
> AST2400/2500 PWM and Fan tach controller support.<br>
> The PWM controller can support upto 8 PWM output ports.<br>
> The Fan tach controller can support upto 16 tachometer inputs.<br>
> PWM clock types M, N and 0 are three types just to have three independent<br>
> PWM sources.<br>
><br>
> Signed-off-by: Jaghathiswari Rankappagounder Natarajan <<a href="mailto:jaghu@google.com">jaghu@google.com</a>><br>
> ---<br>
> .../devicetree/bindings/hwmon/<wbr>aspeed-pwm-tacho.txt | 153 +++++++++++++++++++++<br>
<br>
</span>Perhaps bindings/pwm/... even though this is more than just PWM.<br></font></blockquote><span id="gmail-docs-internal-guid-925fcd73-df36-67e8-06c5-2d11df420ef6"><font face="arial, helvetica, sans-serif"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">The corresponding hwmon driver is present in /drivers/hwmon/. So would it make more</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">sense to include the devicetree document for that driver in /devicetree/bindings/hwmon/.</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">In future if there any more hwmon related functionalities are added to this driver then </span><span style="white-space:pre-wrap">/devicetree/bindings/hwmon/ would be more relevant.</span></p></font></span><div><font face="arial, helvetica, sans-serif"> </font></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="gmail-h5"><font face="arial, helvetica, sans-serif"><br>
> 1 file changed, 153 insertions(+)<br>
> create mode 100644 Documentation/devicetree/<wbr>bindings/hwmon/aspeed-pwm-<wbr>tacho.txt<br>
><br>
> diff --git a/Documentation/devicetree/<wbr>bindings/hwmon/aspeed-pwm-<wbr>tacho.txt b/Documentation/devicetree/<wbr>bindings/hwmon/aspeed-pwm-<wbr>tacho.txt<br>
> new file mode 100644<br>
> index 000000000000..8f346409ee8c<br>
> --- /dev/null<br>
> +++ b/Documentation/devicetree/<wbr>bindings/hwmon/aspeed-pwm-<wbr>tacho.txt<br>
> @@ -0,0 +1,153 @@<br>
> +ASPEED AST2400/AST2500 PWM and Fan Tacho controller device driver<br>
> +<br>
> +The ASPEED PWM controller can support upto 8 PWM outputs. The ASPEED Fan Tacho<br>
> +controller can support upto 16 tachometer inputs. The PWM controller supports<br>
> +3 types of frequency mode PWM for fan speed control. PWM clock types M, N and 0<br>
> +are 3 types of frequency mode PWM just to have 3 independent PWM sources.<br>
> +<br>
> +Required properties for pwm_tacho node:<br>
> +- #address-cells : should be 1.<br>
> +<br>
> +- #size-cells : should be 1.<br>
> +<br>
> +- reg : address and length of the register set for the device.<br>
> +<br>
> +- pinctrl-names : a pinctrl state named "default" must be defined.<br>
> +<br>
> +- pinctrl-0 : phandle referencing pin configuration of the AST2400/AST2500 PWM<br>
> + ports.<br>
> +<br>
> +- compatible : should be "aspeed,aspeed2400-pwm-tacho" for AST2400 or<br>
> + "aspeed,aspeed2500-pwm-tacho" for AST2500.<br>
> +<br>
> +- clocks : a fixed clock providing input clock frequency(PWM<br>
> + and Fan Tach clock)<br>
> +<br>
> +type_values subnode format:<br>
<br>
</font></div></div><font face="arial, helvetica, sans-serif">Don't use '_' in node or property names.<br></font></blockquote><div><font face="arial, helvetica, sans-serif">Done. </font></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<font face="arial, helvetica, sans-serif"><span class="gmail-"><br>
> +===========================<br>
> +Under type_values subnode there can be upto 3 child nodes indicating type M/N/O<br>
> +values. Atleast one child node is required.<br>
> +<br>
> +Required properties for the child node(type M/N/O):<br>
> +- pwm_period : indicates type M/N/O PWM period, as per the AST2400/AST2500<br>
> + datasheet. integer value in the range 0 to 255.<br>
<br>
<br>
> +<br>
> +- pwm_clock_division_l : indicates type M/N/O PWM clock division L value,<br>
> + as per the AST2400/AST2500 datasheet.<br>
> + integer value in the range 0 to 15.<br>
> + 0 here indicates divide 1, 1 indicates divide 2,<br>
> + 2 indicates divide 4, 3 indicates divide 6, and so on<br>
> + till 15 indicates divide 30.<br>
> +<br>
> +- pwm_clock_division_h : indicates type M/N/O PWM clock division H value,<br>
> + as per the AST2400/AST2500 datasheet.<br>
> + integer value in the range 0 to 15.<br>
> + 0 here indicates divide 1, 1 indicates divide 2,<br>
> + 2 indicates divide 4, 3 indicates divide 8, and so on<br>
> + till 15 indicates divide 32768.<br>
<br>
</span>Can't you have a single divider value and driver convert to register<br>
values? Really, you should specify the PWM period in ns/us and calculate<br>
the divider based on the input clock freq. (i.e. use the clock binding).<br>
There's already PWM binding to specify the period.<br>
<br>
I think you should have a node for the fan using the PWM binding and<br>
perhaps moving some of these properties to the fan node.<br></font></blockquote><div><font face="arial, helvetica, sans-serif"><br></font></div><span id="gmail-docs-internal-guid-925fcd73-df37-2aff-2c4c-3847b2a5c22c"><font face="arial, helvetica, sans-serif"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">There are three different types M, N, O. These three types are just to have </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">different PWM/Fan Tach settings. Each type can have the following settings:</span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">PWM related: </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 1) PWM period </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 2) PWM clock division high </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 3) PWM clock division low </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> Fan Tach related: </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 1) Fan Tach type enable </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 2) Fan Tach clock division </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 3) Fan Tach mode selection </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> 4) Fan Tach period </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">Each PWM port should be assigned a type which can be type M, N or O. </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"> </span></p><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap">Each Fan Tach channel should be assigned a PWM source, which can be PWM port A </span></p></font></span><div><font face="arial, helvetica, sans-serif"><span style="white-space:pre-wrap">through H. The Fan Tach channel implicitly gets the type (M, N or O), from the type assigned to its PWM source.</span> </font></div><div><pre style="color:rgb(0,0,0)"><font face="arial, helvetica, sans-serif">If the clock type is type M then :
The PWM frequency = 24MHz / (type M clock division L value * type M clock division H value * </font></pre><pre style="color:rgb(0,0,0)"><font face="arial, helvetica, sans-serif">(type M PWM period value + 1))</font></pre><pre style="color:rgb(0,0,0)"><font face="arial, helvetica, sans-serif">Because of this formula, I thought I could use properties to specify type clock division High and Low and</font></pre><pre style="color:rgb(0,0,0)"><font face="arial, helvetica, sans-serif"> type period values so that the desired PWM frequency be achieved.</font></pre></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div><div class="gmail-h5"><font face="arial, helvetica, sans-serif"><br>
> +<br>
> +- fan_tach_enable : indicates fan tach enable of type M/N/O as per the<br>
> + AST2400/AST2500 datasheet. boolean value.<br>
> +<br>
> +- fan_tach_clock_division : indicates fan tach clock division as per the<br>
> + AST2400/AST2500 datasheet.<br>
> + integer value in the range 0 to 7.<br>
> + 0 indicates divide 4, 1 indicates divide 16,<br>
> + 2 indicates divide 64, 3 indicates divide 256<br>
> + and so on till 7 indicates divide 65536.<br>
> +<br>
> +- fan_tach_mode_selection : indicates fan tach mode mode selection as per the<br>
> + AST2400/AST2500 datasheet. integer value in the<br>
> + range 0 to 2. 0 indicates falling edge, 1 indicates<br>
> + rising edge and 2 indicates both edges.<br>
> +<br>
> +- fan_tach_period : indicates fan tach period as per the AST2400/AST2500<br>
> + datasheet. integer value (can be upto 16 bits long).<br>
> +<br>
> +pwm_port subnode format:<br>
> +========================<br>
> +Under pwm_port subnode there can upto 8 child nodes each indicating values<br>
> +for one of the 8 PWM output ports.<br>
> +<br>
> +Required properties for each child node(starting from PWM A through PWM H):<br>
> +- enable : enable PWM #X port, X ranges from A through H. boolean value.<br>
<br>
</font></div></div><font face="arial, helvetica, sans-serif">A connection in the PWM binding would imply this.<br></font></blockquote><span id="gmail-docs-internal-guid-925fcd73-df37-f22f-d84a-e6ea2b376107"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Had to change this property to indicate PWM port number.</font></span></p></span><div><font face="arial, helvetica, sans-serif"> </font></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<font face="arial, helvetica, sans-serif"><span class="gmail-"><br>
> +<br>
> +- type : indicates type selection value of PWM #X port, X ranges from A<br>
> + through H. integer value in the range 0 to 2;<br>
> + 0 indicates type M, 1 indicates type N, 2 indicates type O.<br>
<br>
</span>This is meaningless to me.<br></font></blockquote><div><font face="arial, helvetica, sans-serif"><br></font></div><span id="gmail-docs-internal-guid-925fcd73-df37-b1ad-757a-5b688520a46f"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Each PWM port should be assigned a type which can be type M, N or O. </font></span></p></span><div><font face="arial, helvetica, sans-serif"> </font></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<font face="arial, helvetica, sans-serif"><span class="gmail-"><br>
> +- fan_ctrl : set the PWM fan control initial value. integer value between<br>
> + 0(off) and 255(full speed).<br>
> +<br>
> +fan_tach_channel subnode format:<br>
> +=============================<wbr>===<br>
> +Under fan_tach_channel subnode there can be upto 16 child nodes each indicating<br>
> +values for one of the 16 fan tach channels.<br>
> +<br>
> +Required properties for each child node(starting from fan tach #0 through<br>
> +fan tach #16):<br>
> +- fan-ctrl-gpios : should specify the tachometer input pin on the hardware.<br>
<br>
</span>I don't understand the connection here. You have a tach block with 16<br>
inputs on the SoC, so why the GPIOs? If the GPIOs go to the fan, then a<br>
fan node should have the GPIO properties.<br></font></blockquote><pre style="color:rgb(0,0,0)"><font face="arial, helvetica, sans-serif">This property specifies the tachometer input GPIO pin on the hardware. Fan Tachometer function</font></pre><pre style="color:rgb(0,0,0)"><font face="arial, helvetica, sans-serif"> can only work when GPIO is in ”input mode”<span style="color:rgb(34,34,34)"> .</span></font></pre><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<font face="arial, helvetica, sans-serif"><span class="gmail-"><br>
> +<br>
> +- enable : enable fan tach #X, X ranges from 0 through 16. boolean value.<br>
<br>
</span>'status' property is the standard way to enable a node or not. Not sure<br>
if that makes sense here though.<br></font></blockquote><span id="gmail-docs-internal-guid-925fcd73-df39-d239-17ce-36025c23f350"><p dir="ltr" style="line-height:1.38;margin-top:0pt;margin-bottom:0pt"><span style="vertical-align:baseline;white-space:pre-wrap"><font face="arial, helvetica, sans-serif">Had to change this property to indicate Fan tach channel number. </font></span></p></span><div><font face="arial, helvetica, sans-serif"> </font></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div class="gmail-HOEnZb"><div class="gmail-h5"><font face="arial, helvetica, sans-serif"><br>
> +<br>
> +- pwm_source : indicates PWM source of fan tach #X, X ranges from 0 through 16.<br>
> + integer value in the range 0 to 7. 0 indicates PWM port A,<br>
> + 1 indicates PWM port B and so on till 7 indicates PWM port H.<br>
</font></div></div></blockquote></div><br></div></div>