| Message ID | 20240311111347.23067-4-chanh@os.amperecomputing.com |
|---|---|
| State | New |
| Headers | show |
| Series | Update the max31790 driver | expand |
On Mon, Mar 11, 2024 at 06:13:47PM +0700, Chanh Nguyen wrote: > Add pwmout-pin-as-tach-input property. > > Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> > --- > Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ > 1 file changed, 11 insertions(+) > > diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml > index 5a93e6bdebda..447cac17053a 100644 > --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml > +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml > @@ -25,6 +25,16 @@ properties: > reg: > maxItems: 1 > > + pwmout-pin-as-tach-input: > + description: | > + An array of six integers responds to six PWM channels for > + configuring the pwm to tach mode. > + When set to 0, the associated PWMOUT produces a PWM waveform for > + control of fan speed. When set to 1, PWMOUT becomes a TACH input > + $ref: /schemas/types.yaml#/definitions/uint8-array > + maxItems: 6 > + minItems: 6 Seems incomplete. For example, fan tachs have different number of pulses per revolution, don't you need to know that too? There's a common fan binding now (or pending). You should use that and this property won't be needed. Rob
On 3/11/24 10:34, Rob Herring wrote: > On Mon, Mar 11, 2024 at 06:13:47PM +0700, Chanh Nguyen wrote: >> Add pwmout-pin-as-tach-input property. >> >> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >> --- >> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> index 5a93e6bdebda..447cac17053a 100644 >> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> @@ -25,6 +25,16 @@ properties: >> reg: >> maxItems: 1 >> >> + pwmout-pin-as-tach-input: >> + description: | >> + An array of six integers responds to six PWM channels for >> + configuring the pwm to tach mode. >> + When set to 0, the associated PWMOUT produces a PWM waveform for >> + control of fan speed. When set to 1, PWMOUT becomes a TACH input >> + $ref: /schemas/types.yaml#/definitions/uint8-array >> + maxItems: 6 >> + minItems: 6 > > Seems incomplete. For example, fan tachs have different number of > pulses per revolution, don't you need to know that too? > Per Documentation/ABI/testing/sysfs-class-hwmon: What: /sys/class/hwmon/hwmonX/fanY_pulses Description: Number of tachometer pulses per fan revolution. Integer value, typically between 1 and 4. RW This value is a characteristic of the fan connected to the device's input, so it has to be set in accordance with the fan model. Should only be created if the chip has a register to configure the number of pulses. In the absence of such a register (and thus attribute) the value assumed by all devices is 2 pulses per fan revolution. We only expect the property (and attribute) to exist if the controller supports it. Guenter
On 11/03/2024 23:56, Krzysztof Kozlowski wrote: > On 11/03/2024 12:13, Chanh Nguyen wrote: >> Add pwmout-pin-as-tach-input property. > > Why is this split from original binding? This does not make much > sense... Add complete hardware description. > Ok Krzysztof, I will merg the "[PATCH 1/3] dt-bindings: hwmon: Add maxim max31790 driver bindings" commit and "[PATCH 3/3] dt-bindings: hwmon: max31790: Add pwmout-pin-as-tach-input property" commit. >> >> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >> --- >> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> index 5a93e6bdebda..447cac17053a 100644 >> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> @@ -25,6 +25,16 @@ properties: >> reg: >> maxItems: 1 >> >> + pwmout-pin-as-tach-input: >> + description: | >> + An array of six integers responds to six PWM channels for >> + configuring the pwm to tach mode. >> + When set to 0, the associated PWMOUT produces a PWM waveform for >> + control of fan speed. When set to 1, PWMOUT becomes a TACH input > > No vendor prefix, so generic property... but where is it defined? > Thank Krzysztof! It is not generic property, I'll add the vendor prefix. I'll update the "pwmout-pin-as-tach-input" to "maxim,pwmout-pin-as-tach-input" at v2. > > Best regards, > Krzysztof >
On 12/03/2024 00:34, Rob Herring wrote: > On Mon, Mar 11, 2024 at 06:13:47PM +0700, Chanh Nguyen wrote: >> Add pwmout-pin-as-tach-input property. >> >> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >> --- >> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >> 1 file changed, 11 insertions(+) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> index 5a93e6bdebda..447cac17053a 100644 >> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >> @@ -25,6 +25,16 @@ properties: >> reg: >> maxItems: 1 >> >> + pwmout-pin-as-tach-input: >> + description: | >> + An array of six integers responds to six PWM channels for >> + configuring the pwm to tach mode. >> + When set to 0, the associated PWMOUT produces a PWM waveform for >> + control of fan speed. When set to 1, PWMOUT becomes a TACH input >> + $ref: /schemas/types.yaml#/definitions/uint8-array >> + maxItems: 6 >> + minItems: 6 > > Seems incomplete. For example, fan tachs have different number of > pulses per revolution, don't you need to know that too? > > There's a common fan binding now (or pending). You should use that and > this property won't be needed. > > Rob Thank Rob, I checked in the Documentation/devicetree/bindings/hwmon/fan-common.yaml. I found the tach-ch property, but it seems define the tach channel used for fan. tach-ch: description: The tach channel used for the fan. $ref: /schemas/types.yaml#/definitions/uint8-array I would like to define a new vendor property to configure the PWM-OUT pin to become a TACH-IN pin. So I introduce the "maxim,pwmout-pin-as-tach-input" property. Please help me share your comments!
On 12/03/2024 00:52, Guenter Roeck wrote: > On 3/11/24 10:34, Rob Herring wrote: >> On Mon, Mar 11, 2024 at 06:13:47PM +0700, Chanh Nguyen wrote: >>> Add pwmout-pin-as-tach-input property. >>> >>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >>> --- >>> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> index 5a93e6bdebda..447cac17053a 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> @@ -25,6 +25,16 @@ properties: >>> reg: >>> maxItems: 1 >>> + pwmout-pin-as-tach-input: >>> + description: | >>> + An array of six integers responds to six PWM channels for >>> + configuring the pwm to tach mode. >>> + When set to 0, the associated PWMOUT produces a PWM waveform for >>> + control of fan speed. When set to 1, PWMOUT becomes a TACH input >>> + $ref: /schemas/types.yaml#/definitions/uint8-array >>> + maxItems: 6 >>> + minItems: 6 >> >> Seems incomplete. For example, fan tachs have different number of >> pulses per revolution, don't you need to know that too? >> > > Per Documentation/ABI/testing/sysfs-class-hwmon: > > What: /sys/class/hwmon/hwmonX/fanY_pulses > Description: > Number of tachometer pulses per fan revolution. > > Integer value, typically between 1 and 4. > > RW > > This value is a characteristic of the fan connected to the > device's input, so it has to be set in accordance with > the fan > model. > > Should only be created if the chip has a register to > configure > the number of pulses. In the absence of such a register > (and > thus attribute) the value assumed by all devices is 2 > pulses > per fan revolution. > > We only expect the property (and attribute) to exist if the controller > supports it. > > Guenter > Hi Guenter and Rob, Most general-purpose brushless DC fans produce two tachometer pulses per revolution. My fan devices also produce two tachometer pulses per revolution. In max31790.c, I saw the formula that is used to calculate TACH Count Registers, which defines the pulses per revolution as 2 #define RPM_TO_REG(rpm, sr) ((60 * (sr) * 8192) / ((rpm) * 2)) Do you think we should support the pulses-per-revolution property in this case?
On 18/03/2024 17:02, Krzysztof Kozlowski wrote: > On 18/03/2024 10:48, Chanh Nguyen wrote: >> >> >> On 11/03/2024 23:56, Krzysztof Kozlowski wrote: >>> On 11/03/2024 12:13, Chanh Nguyen wrote: >>>> Add pwmout-pin-as-tach-input property. >>> >>> Why is this split from original binding? This does not make much >>> sense... Add complete hardware description. >>> >> >> Ok Krzysztof, I will merg the "[PATCH 1/3] dt-bindings: hwmon: Add maxim >> max31790 driver bindings" commit and "[PATCH 3/3] dt-bindings: hwmon: >> max31790: Add pwmout-pin-as-tach-input property" commit. > > Later I checked your driver code and this explains a bit. However first > patch should explain that instead. The split is fine, but just lack of > rationale is confusing. > Thank Krzysztof. I'll try to explain in detail each patch in v2. > >> >>>> >>>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >>>> --- >>>> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >>>> 1 file changed, 11 insertions(+) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> index 5a93e6bdebda..447cac17053a 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>>> @@ -25,6 +25,16 @@ properties: >>>> reg: >>>> maxItems: 1 >>>> >>>> + pwmout-pin-as-tach-input: >>>> + description: | >>>> + An array of six integers responds to six PWM channels for >>>> + configuring the pwm to tach mode. >>>> + When set to 0, the associated PWMOUT produces a PWM waveform for >>>> + control of fan speed. When set to 1, PWMOUT becomes a TACH input >>> >>> No vendor prefix, so generic property... but where is it defined? >>> >> >> Thank Krzysztof! It is not generic property, I'll add the vendor prefix. >> >> I'll update the "pwmout-pin-as-tach-input" to >> "maxim,pwmout-pin-as-tach-input" at v2. > > Except that you should really look into common properties and use them. > Or explain why do you need new property? > > Best regards, > Krzysztof > I'm also discussing with Rob Herring that on patch 3/3, I checked in the Documentation/devicetree/bindings/hwmon/fan-common.yaml. I found the tach-ch property, but it seems to define the tach channel used for the fan. It does not match my purpose. I want to configure the PWM-OUT pin to become a TACH-IN pin. I wonder if I can use the tach-ch property for my purpose.
On 18/03/2024 16:53, Chanh Nguyen wrote: > > > On 12/03/2024 00:34, Rob Herring wrote: >> On Mon, Mar 11, 2024 at 06:13:47PM +0700, Chanh Nguyen wrote: >>> Add pwmout-pin-as-tach-input property. >>> >>> Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> >>> --- >>> Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ >>> 1 file changed, 11 insertions(+) >>> >>> diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> index 5a93e6bdebda..447cac17053a 100644 >>> --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml >>> @@ -25,6 +25,16 @@ properties: >>> reg: >>> maxItems: 1 >>> + pwmout-pin-as-tach-input: >>> + description: | >>> + An array of six integers responds to six PWM channels for >>> + configuring the pwm to tach mode. >>> + When set to 0, the associated PWMOUT produces a PWM waveform for >>> + control of fan speed. When set to 1, PWMOUT becomes a TACH input >>> + $ref: /schemas/types.yaml#/definitions/uint8-array >>> + maxItems: 6 >>> + minItems: 6 >> >> Seems incomplete. For example, fan tachs have different number of >> pulses per revolution, don't you need to know that too? >> >> There's a common fan binding now (or pending). You should use that and >> this property won't be needed. >> >> Rob > > Thank Rob, > > I checked in the > Documentation/devicetree/bindings/hwmon/fan-common.yaml. I found the > tach-ch property, but it seems define the tach channel used for fan. > > tach-ch: > description: > The tach channel used for the fan. > $ref: /schemas/types.yaml#/definitions/uint8-array > > I would like to define a new vendor property to configure the PWM-OUT > pin to become a TACH-IN pin. So I introduce the > "maxim,pwmout-pin-as-tach-input" property. Please help me share your > comments! Hi Guenter and Rob, I'm preparing for patch v2. I'm looking forward to hear your advice. Should I use the "tach-ch" property (a common fan property) or define new vendor property ("maxim,pwmout-pin-as-tach-input") for my purpose? Thank you very much! Chanh
diff --git a/Documentation/devicetree/bindings/hwmon/max31790.yaml b/Documentation/devicetree/bindings/hwmon/max31790.yaml index 5a93e6bdebda..447cac17053a 100644 --- a/Documentation/devicetree/bindings/hwmon/max31790.yaml +++ b/Documentation/devicetree/bindings/hwmon/max31790.yaml @@ -25,6 +25,16 @@ properties: reg: maxItems: 1 + pwmout-pin-as-tach-input: + description: | + An array of six integers responds to six PWM channels for + configuring the pwm to tach mode. + When set to 0, the associated PWMOUT produces a PWM waveform for + control of fan speed. When set to 1, PWMOUT becomes a TACH input + $ref: /schemas/types.yaml#/definitions/uint8-array + maxItems: 6 + minItems: 6 + required: - compatible - reg @@ -40,5 +50,6 @@ examples: max31790@20 { compatible = "maxim,max31790"; reg = <0x20>; + pwmout-pin-as-tach-input = /bits/ 8 <0 0 0 0 1 1>; }; };
Add pwmout-pin-as-tach-input property. Signed-off-by: Chanh Nguyen <chanh@os.amperecomputing.com> --- Documentation/devicetree/bindings/hwmon/max31790.yaml | 11 +++++++++++ 1 file changed, 11 insertions(+)