| Message ID | 20240520030321.3756604-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (adt7475) duty cycle configuration | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/patch-applied | success | |
| robh/dtbs-check | warning | build log |
| robh/dt-meta-schema | success |
On Mon, May 20, 2024 at 03:03:19PM +1200, Chris Packham wrote: > Add fan child nodes that allow describing the connections for the > ADT7475 to the fans it controls. This also allows setting some > initial values for the pwm duty cycle and frequency. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > > Notes: > Changes in v3: > - Use the pwm provider/consumer bindings > Changes in v2: > - Document 0 as a valid value (leaves hardware as-is) > > .../devicetree/bindings/hwmon/adt7475.yaml | 25 ++++++++++++++++++- > 1 file changed, 24 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > index 051c976ab711..99bd689ae0cd 100644 > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > @@ -51,6 +51,15 @@ properties: > enum: [0, 1] > default: 1 > > + "#pwm-cells": > + const: 4 > + description: | > + Number of cells in a PWM specifier. > + - 0: The pwm channel > + - 1: The pwm frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500 The standard binding is period in nanoseconds, not frequency in Hz. What's gained from deviating from that? > + - 2: PWM flags 0 or PWM_POLARITY_INVERTED > + - 3: The default pwm duty cycle - 0-255 Same here I guess, why not match the units used for the period for the duty cycle? Cheers, Conor.
On 5/20/24 09:49, Conor Dooley wrote: > On Mon, May 20, 2024 at 03:03:19PM +1200, Chris Packham wrote: >> Add fan child nodes that allow describing the connections for the >> ADT7475 to the fans it controls. This also allows setting some >> initial values for the pwm duty cycle and frequency. >> >> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >> --- >> >> Notes: >> Changes in v3: >> - Use the pwm provider/consumer bindings >> Changes in v2: >> - Document 0 as a valid value (leaves hardware as-is) >> >> .../devicetree/bindings/hwmon/adt7475.yaml | 25 ++++++++++++++++++- >> 1 file changed, 24 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >> index 051c976ab711..99bd689ae0cd 100644 >> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >> @@ -51,6 +51,15 @@ properties: >> enum: [0, 1] >> default: 1 >> >> + "#pwm-cells": >> + const: 4 >> + description: | >> + Number of cells in a PWM specifier. >> + - 0: The pwm channel >> + - 1: The pwm frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500 > > The standard binding is period in nanoseconds, not frequency in Hz. > What's gained from deviating from that? > I'd strongly suspect that Chris didn't know and didn't expect it, just like me. >> + - 2: PWM flags 0 or PWM_POLARITY_INVERTED >> + - 3: The default pwm duty cycle - 0-255 > > Same here I guess, why not match the units used for the period for the > duty cycle? > Same here. I am used to pwm frequency in Hz and duty cycle as percentage. Using the period instead is somewhat unusual, and I must admit that I have never encountered it while dealing with a variety of fan controllers. Just to make sure I understand this correctly - duty cycles would be (rounded): Hz ns 11 90,909,091 14 71,428,571 22 45,454,545 29: 34,482,759 35: 28,571,429 44: 22,727,273 58: 17,241,379 88: 11,363,636 22500 44,444 Examples for duty cycles would be 20%: 0,2s or 200,000,000 50%: 0.5s or 500,000,000 80%: 0.8s or 800,000,000 Is that correct ? Thanks, Guenter
On Mon, May 20, 2024 at 12:03:42PM -0700, Guenter Roeck wrote: > On 5/20/24 09:49, Conor Dooley wrote: > > On Mon, May 20, 2024 at 03:03:19PM +1200, Chris Packham wrote: > > > Add fan child nodes that allow describing the connections for the > > > ADT7475 to the fans it controls. This also allows setting some > > > initial values for the pwm duty cycle and frequency. > > > > > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > > --- > > > > > > Notes: > > > Changes in v3: > > > - Use the pwm provider/consumer bindings > > > Changes in v2: > > > - Document 0 as a valid value (leaves hardware as-is) > > > > > > .../devicetree/bindings/hwmon/adt7475.yaml | 25 ++++++++++++++++++- > > > 1 file changed, 24 insertions(+), 1 deletion(-) > > > > > > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > > > index 051c976ab711..99bd689ae0cd 100644 > > > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml > > > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > > > @@ -51,6 +51,15 @@ properties: > > > enum: [0, 1] > > > default: 1 > > > + "#pwm-cells": > > > + const: 4 > > > + description: | > > > + Number of cells in a PWM specifier. > > > + - 0: The pwm channel > > > + - 1: The pwm frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500 > > > > The standard binding is period in nanoseconds, not frequency in Hz. > > What's gained from deviating from that? > > > > I'd strongly suspect that Chris didn't know and didn't expect it, > just like me. I did point out on v2 that the information on the standard binding was in pwm.txt, but I also said "order it has <index freq flags duty>" which likely didn't help. I did however CC you both on a patch the other day where I fixed pwm.yaml so that it actually contained the information on what the cells represented. > > > + - 2: PWM flags 0 or PWM_POLARITY_INVERTED > > > + - 3: The default pwm duty cycle - 0-255 > > > > Same here I guess, why not match the units used for the period for the > > duty cycle? > > > > Same here. I am used to pwm frequency in Hz and duty cycle as percentage. > Using the period instead is somewhat unusual, and I must admit that I > have never encountered it while dealing with a variety of fan controllers. If it is what makes sense to use, because it's what everyone and their mother documents, then sure. My rationale I suppose was threefold: - Consistency with the period - Time would be more portable between providers, if 8 bits of precision is deemed insufficient for some providers. - Last & least, the PWM APIs in the kernel use time for the dutycycle If you're going to use percentages rather than time, would it not make more sense to either use percent itself with a 0-100 range or use basis points if percent doesn't provide sufficient granularity? I think it'd be good of Uwe chimed in, given we're discussing a PWM provider binding after all. > Just to make sure I understand this correctly - duty cycles would s/duty cycles/periods/ > be (rounded): > > Hz ns > 11 90,909,091 > 14 71,428,571 > 22 45,454,545 > 29: 34,482,759 > 35: 28,571,429 > 44: 22,727,273 > 58: 17,241,379 > 88: 11,363,636 > 22500 44,444 > > Examples for duty cycles would be > > 20%: 0,2s or 200,000,000 > 50%: 0.5s or 500,000,000 > 80%: 0.8s or 800,000,000 > > Is that correct ? Assuming a 1 second period, those look as expected. Cheers, Conor.
On 21/05/24 07:41, Conor Dooley wrote: > On Mon, May 20, 2024 at 12:03:42PM -0700, Guenter Roeck wrote: >> On 5/20/24 09:49, Conor Dooley wrote: >>> On Mon, May 20, 2024 at 03:03:19PM +1200, Chris Packham wrote: >>>> Add fan child nodes that allow describing the connections for the >>>> ADT7475 to the fans it controls. This also allows setting some >>>> initial values for the pwm duty cycle and frequency. >>>> >>>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> >>>> --- >>>> >>>> Notes: >>>> Changes in v3: >>>> - Use the pwm provider/consumer bindings >>>> Changes in v2: >>>> - Document 0 as a valid value (leaves hardware as-is) >>>> >>>> .../devicetree/bindings/hwmon/adt7475.yaml | 25 ++++++++++++++++++- >>>> 1 file changed, 24 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >>>> index 051c976ab711..99bd689ae0cd 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >>>> @@ -51,6 +51,15 @@ properties: >>>> enum: [0, 1] >>>> default: 1 >>>> + "#pwm-cells": >>>> + const: 4 >>>> + description: | >>>> + Number of cells in a PWM specifier. >>>> + - 0: The pwm channel >>>> + - 1: The pwm frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500 >>> The standard binding is period in nanoseconds, not frequency in Hz. >>> What's gained from deviating from that? >>> >> I'd strongly suspect that Chris didn't know and didn't expect it, >> just like me. > I did point out on v2 that the information on the standard binding was > in pwm.txt, but I also said "order it has <index freq flags duty>" which > likely didn't help. I did however CC you both on a patch the other day > where I fixed pwm.yaml so that it actually contained the information on > what the cells represented. I did see that patch thanks. And I did adjust the order of the parameters I had based on it. I personally found expressing the frequency in seconds confusing which the is main reason why I stuck with hertz. I also couldn't grok how to express the duty cycle if I did express the frequency in nanoseconds. I think maybe I'm a bit confused as to how to express a generic PWM period when the controller I'm dealing with has a frequency and a duty cycle. >>>> + - 2: PWM flags 0 or PWM_POLARITY_INVERTED >>>> + - 3: The default pwm duty cycle - 0-255 >>> Same here I guess, why not match the units used for the period for the >>> duty cycle? >>> >> Same here. I am used to pwm frequency in Hz and duty cycle as percentage. >> Using the period instead is somewhat unusual, and I must admit that I >> have never encountered it while dealing with a variety of fan controllers. My expectation is that a duty cycle is a percentage. The only reason I'm using 0-255 instead of 0-100 is because that matches the sysfs ABI. But I'd happily change to a true percentage (if I can figure out the integer math), we'd lose a little bit of precision but nothing anyone would really notice. > If it is what makes sense to use, because it's what everyone and their > mother documents, then sure. My rationale I suppose was threefold: > - Consistency with the period > - Time would be more portable between providers, if 8 bits of precision > is deemed insufficient for some providers. > - Last & least, the PWM APIs in the kernel use time for the dutycycle > > If you're going to use percentages rather than time, would it not > make more sense to either use percent itself with a 0-100 range or use > basis points if percent doesn't provide sufficient granularity? > > I think it'd be good of Uwe chimed in, given we're discussing a PWM > provider binding after all. I'd also point out I'm more than happy to go back to my v2 approach of adding specific properties for the frequency and duty cycle. >> Just to make sure I understand this correctly - duty cycles would > s/duty cycles/periods/ > >> be (rounded): >> >> Hz ns >> 11 90,909,091 >> 14 71,428,571 >> 22 45,454,545 >> 29: 34,482,759 >> 35: 28,571,429 >> 44: 22,727,273 >> 58: 17,241,379 >> 88: 11,363,636 >> 22500 44,444 >> >> Examples for duty cycles would be >> >> 20%: 0,2s or 200,000,000 >> 50%: 0.5s or 500,000,000 >> 80%: 0.8s or 800,000,000 >> >> Is that correct ? > Assuming a 1 second period, those look as expected. To me that just makes things harder to understand. If you were reading the ADT7475 datasheet you'd see things like the supported frequencies of 11, 14, ... , 88, 22500. Having to express that in nanoseconds requires making a bit of a leap. The duty cycle percentage to nanoseconds is more straight forward but seems unnecessarily obscure to me. > > Cheers, > Conor. >
diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml index 051c976ab711..99bd689ae0cd 100644 --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml @@ -51,6 +51,15 @@ properties: enum: [0, 1] default: 1 + "#pwm-cells": + const: 4 + description: | + Number of cells in a PWM specifier. + - 0: The pwm channel + - 1: The pwm frequency in hertz - 0, 11, 14, 22, 29, 35, 44, 58, 88, 22500 + - 2: PWM flags 0 or PWM_POLARITY_INVERTED + - 3: The default pwm duty cycle - 0-255 + patternProperties: "^adi,bypass-attenuator-in[0-4]$": description: | @@ -81,6 +90,10 @@ patternProperties: - smbalert# - gpio + "^fan-[0-9]+$": + $ref: fan-common.yaml# + unevaluatedProperties: false + required: - compatible - reg @@ -89,11 +102,12 @@ additionalProperties: false examples: - | + #include <dt-bindings/pwm/pwm.h> i2c { #address-cells = <1>; #size-cells = <0>; - hwmon@2e { + pwm: hwmon@2e { compatible = "adi,adt7476"; reg = <0x2e>; adi,bypass-attenuator-in0 = <1>; @@ -101,5 +115,14 @@ examples: adi,pwm-active-state = <1 0 1>; adi,pin10-function = "smbalert#"; adi,pin14-function = "tach4"; + #pwm-cells = <4>; + + fan-0 { + pwms = <&pwm 0 22500 PWM_POLARITY_INVERTED 255>; + }; + + fan-1 { + pwms = <&pwm 2 22500 PWM_POLARITY_INVERTED 255>; + }; }; };
Add fan child nodes that allow describing the connections for the ADT7475 to the fans it controls. This also allows setting some initial values for the pwm duty cycle and frequency. Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> --- Notes: Changes in v3: - Use the pwm provider/consumer bindings Changes in v2: - Document 0 as a valid value (leaves hardware as-is) .../devicetree/bindings/hwmon/adt7475.yaml | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)