| Message ID | 20240722221737.3407958-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | hwmon: (adt7475) duty cycle configuration | expand |
On Tue, Jul 23, 2024 at 10:17:35AM +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> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> Applied. Thanks, Guenter
Hello, On Tue, Jul 23, 2024 at 10:17:35AM +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> > Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > --- > > Notes: > Changes in v7: > - None > Changes in v6: > - Collect r-by from Rob > Changes in v5: > - Use nanoseconds for PWM frequency and duty cycle as per existing > conventions for PWMs > - Set flags to 0 in example to match adi,pwm-active-state setting > Changes in v4: > - 0 is not a valid frequency value > 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 | 35 ++++++++++++++++++- > 1 file changed, 34 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > index 051c976ab711..df2b5b889e4d 100644 > --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > @@ -51,6 +51,24 @@ properties: > enum: [0, 1] > default: 1 > > + "#pwm-cells": > + const: 4 I asked to add support for #pwm-cells = <4> to the pwm core in reply to v4 (see https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/). I'm unhappy to see this merged anyhow in combination with ad-hoc parsing of the pwm properties in the driver :-\ > + description: | > + Number of cells in a PWM specifier. > + - 0: The PWM channel > + - 1: The PWM period in nanoseconds > + - 90909091 (11 Hz) > + - 71428571 (14 Hz) > + - 45454545 (22 Hz) > + - 34482759 (29 Hz) > + - 28571429 (35 Hz) > + - 22727273 (44 Hz) > + - 17241379 (58 Hz) > + - 11363636 (88 Hz) > + - 44444 (22 kHz) > + - 2: PWM flags 0 or PWM_POLARITY_INVERTED > + - 3: The default PWM duty cycle in nanoseconds > + Best regards Uwe
Hi Uwe, On 28/05/2025 04:12, Uwe Kleine-König wrote: > Hello, > > On Tue, Jul 23, 2024 at 10:17:35AM +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> >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org> >> --- >> >> Notes: >> Changes in v7: >> - None >> Changes in v6: >> - Collect r-by from Rob >> Changes in v5: >> - Use nanoseconds for PWM frequency and duty cycle as per existing >> conventions for PWMs >> - Set flags to 0 in example to match adi,pwm-active-state setting >> Changes in v4: >> - 0 is not a valid frequency value >> 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 | 35 ++++++++++++++++++- >> 1 file changed, 34 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >> index 051c976ab711..df2b5b889e4d 100644 >> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml >> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >> @@ -51,6 +51,24 @@ properties: >> enum: [0, 1] >> default: 1 >> >> + "#pwm-cells": >> + const: 4 > I asked to add support for #pwm-cells = <4> to the pwm core in reply to > v4 (see > https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/). > > I'm unhappy to see this merged anyhow in combination with ad-hoc parsing > of the pwm properties in the driver :-\ As I mentioned at the time the adt7475 is not currently pwm_chip so I need the ad-hoc parsing in that driver. I'd be happy to take you prototype patch for pwm/core.c and polish it although I don't really have a good way of testing it. >> + description: | >> + Number of cells in a PWM specifier. >> + - 0: The PWM channel >> + - 1: The PWM period in nanoseconds >> + - 90909091 (11 Hz) >> + - 71428571 (14 Hz) >> + - 45454545 (22 Hz) >> + - 34482759 (29 Hz) >> + - 28571429 (35 Hz) >> + - 22727273 (44 Hz) >> + - 17241379 (58 Hz) >> + - 11363636 (88 Hz) >> + - 44444 (22 kHz) >> + - 2: PWM flags 0 or PWM_POLARITY_INVERTED >> + - 3: The default PWM duty cycle in nanoseconds >> + > Best regards > Uwe
Hello Chris, On Tue, May 27, 2025 at 08:24:56PM +0000, Chris Packham wrote: > On 28/05/2025 04:12, Uwe Kleine-König wrote: > > Hello, > > > > On Tue, Jul 23, 2024 at 10:17:35AM +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> > >> Reviewed-by: Rob Herring (Arm) <robh@kernel.org> > >> --- > >> > >> Notes: > >> Changes in v7: > >> - None > >> Changes in v6: > >> - Collect r-by from Rob > >> Changes in v5: > >> - Use nanoseconds for PWM frequency and duty cycle as per existing > >> conventions for PWMs > >> - Set flags to 0 in example to match adi,pwm-active-state setting > >> Changes in v4: > >> - 0 is not a valid frequency value > >> 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 | 35 ++++++++++++++++++- > >> 1 file changed, 34 insertions(+), 1 deletion(-) > >> > >> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > >> index 051c976ab711..df2b5b889e4d 100644 > >> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml > >> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml > >> @@ -51,6 +51,24 @@ properties: > >> enum: [0, 1] > >> default: 1 > >> > >> + "#pwm-cells": > >> + const: 4 > > I asked to add support for #pwm-cells = <4> to the pwm core in reply to > > v4 (see > > https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/). > > > > I'm unhappy to see this merged anyhow in combination with ad-hoc parsing > > of the pwm properties in the driver :-\ > > As I mentioned at the time the adt7475 is not currently pwm_chip so I > need the ad-hoc parsing in that driver. I'd be happy to take you > prototype patch for pwm/core.c and polish it although I don't really > have a good way of testing it. It's more the deviation of the default binding for PWMs that I don't like than the ad-hoc parsing. Ideally the adt7475 would provide a pwmchip (as the binding suggests) and the fan would be formalized as a pwm-fan. With the binding that was chosen here that option becomes more ugly than necessary to implement. If I understand correctly you need the default value for duty to statically setup (or only initialize?) a fan, right? I'm not sure I like extending #pwm-cells for a default duty value. Thinking about that a while I'd prefer a binding that looks more like the clock configuration stuff because actually having the period and flags as part of the reference to the PWM to be used is also a bit strange. So I imagine something like: mypwm: pwm { compatible = "...." #pwm-cells = <1>; }; fan { compatible = "pwm-fan"; pwms = <&mypwm 1>; assigned-pwms = <&mypwm>; assigned-pwm-default-period-lengths-ns = <40000>; assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>; }; Then specifying a period (or later a duty cycle length) would be optional and could be provided iff the device needs that for operation. My mail was just me being frustrated about another special case that I'd have to handle if I go into that direction. I should have been more attentive to that development before it entered the mainline. Best regards Uwe
Hi Uwe, On 28/05/2025 18:10, Uwe Kleine-König wrote: > Hello Chris, > > On Tue, May 27, 2025 at 08:24:56PM +0000, Chris Packham wrote: >> On 28/05/2025 04:12, Uwe Kleine-König wrote: >>> Hello, >>> >>> On Tue, Jul 23, 2024 at 10:17:35AM +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> >>>> Reviewed-by: Rob Herring (Arm) <robh@kernel.org> >>>> --- >>>> >>>> Notes: >>>> Changes in v7: >>>> - None >>>> Changes in v6: >>>> - Collect r-by from Rob >>>> Changes in v5: >>>> - Use nanoseconds for PWM frequency and duty cycle as per existing >>>> conventions for PWMs >>>> - Set flags to 0 in example to match adi,pwm-active-state setting >>>> Changes in v4: >>>> - 0 is not a valid frequency value >>>> 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 | 35 ++++++++++++++++++- >>>> 1 file changed, 34 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >>>> index 051c976ab711..df2b5b889e4d 100644 >>>> --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml >>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml >>>> @@ -51,6 +51,24 @@ properties: >>>> enum: [0, 1] >>>> default: 1 >>>> >>>> + "#pwm-cells": >>>> + const: 4 >>> I asked to add support for #pwm-cells = <4> to the pwm core in reply to >>> v4 (see >>> https://lore.kernel.org/linux-pwm/drqvaon5lb2ei3jqofutbr6demibyfdhbmr24sva27gzpqdnon@fxa7rpl33iih/). >>> >>> I'm unhappy to see this merged anyhow in combination with ad-hoc parsing >>> of the pwm properties in the driver :-\ >> As I mentioned at the time the adt7475 is not currently pwm_chip so I >> need the ad-hoc parsing in that driver. I'd be happy to take you >> prototype patch for pwm/core.c and polish it although I don't really >> have a good way of testing it. > It's more the deviation of the default binding for PWMs that I don't > like than the ad-hoc parsing. Ideally the adt7475 would provide a > pwmchip (as the binding suggests) and the fan would be formalized as a > pwm-fan. With the binding that was chosen here that option becomes more > ugly than necessary to implement. > > If I understand correctly you need the default value for duty to > statically setup (or only initialize?) a fan, right? Correct. > I'm not sure I like > extending #pwm-cells for a default duty value. Thinking about that a > while I'd prefer a binding that looks more like the clock configuration > stuff because actually having the period and flags as part of the > reference to the PWM to be used is also a bit strange. So I imagine > something like: > > mypwm: pwm { > compatible = "...." > #pwm-cells = <1>; > }; > > fan { > compatible = "pwm-fan"; > pwms = <&mypwm 1>; > assigned-pwms = <&mypwm>; > assigned-pwm-default-period-lengths-ns = <40000>; > assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>; > }; > > Then specifying a period (or later a duty cycle length) would be > optional and could be provided iff the device needs that for operation. The frequency and flags were already part of the standard #pwm-cells which I think is why I was encouraged to use them. I was also trying to get something that would work as an ACPI overlay which turned out to be really hard. > My mail was just me being frustrated about another special case that I'd > have to handle if I go into that direction. I should have been more > attentive to that development before it entered the mainline. I'd be happy to deprecate the 4 cell thing and replace it with 3 cell + vendor property for the default period if that helps. > > Best regards > Uwe
Hello Chris, On Wed, May 28, 2025 at 09:18:37PM +0000, Chris Packham wrote: > On 28/05/2025 18:10, Uwe Kleine-König wrote: > > If I understand correctly you need the default value for duty to > > statically setup (or only initialize?) a fan, right? > > Correct. > > > I'm not sure I like > > extending #pwm-cells for a default duty value. Thinking about that a > > while I'd prefer a binding that looks more like the clock configuration > > stuff because actually having the period and flags as part of the > > reference to the PWM to be used is also a bit strange. So I imagine > > something like: > > > > mypwm: pwm { > > compatible = "...." > > #pwm-cells = <1>; > > }; > > > > fan { > > compatible = "pwm-fan"; > > pwms = <&mypwm 1>; > > assigned-pwms = <&mypwm>; > > assigned-pwm-default-period-lengths-ns = <40000>; > > assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>; > > }; > > > > Then specifying a period (or later a duty cycle length) would be > > optional and could be provided iff the device needs that for operation. > > The frequency and flags were already part of the standard #pwm-cells > which I think is why I was encouraged to use them. Yeah, that part is fine. This might not be the long-term future, but today that's the norm. > I was also trying to get something that would work as an ACPI overlay > which turned out to be really hard. I don't know enough about ACPI to be helpful with this quest. > > My mail was just me being frustrated about another special case that I'd > > have to handle if I go into that direction. I should have been more > > attentive to that development before it entered the mainline. > > I'd be happy to deprecate the 4 cell thing and replace it with 3 cell + > vendor property for the default period if that helps. I wonder how other similar devices determine the default duty cycle. Isn't the norm to make the fan rotate at max speed and then when userspace takes over it's speeded down? Best regards Uwe
On Wed, May 28, 2025 at 09:18:37PM +0000, Chris Packham wrote: > >> As I mentioned at the time the adt7475 is not currently pwm_chip so I > >> need the ad-hoc parsing in that driver. I'd be happy to take you > >> prototype patch for pwm/core.c and polish it although I don't really > >> have a good way of testing it. > > It's more the deviation of the default binding for PWMs that I don't > > like than the ad-hoc parsing. Ideally the adt7475 would provide a > > pwmchip (as the binding suggests) and the fan would be formalized as a We are not going to force each fan controller driver to register as pwm chip just because it provides a pwm value to control the fans - even more so since this gets really ugly if the chip can be programmed to either provide a voltage output or a pwm value to control fan speed. Maybe the next requirement is that fan controllers supporting voltage output to control fan speeds are supposed to register themselves as regulators. I really don't want to go there. Those are _not_ pwm controllers. They are special-purpose fan controllers. Forcing them into the pwm framework from devicetree perspective is bad enough, but forcing them to register as pwm controllers is a step too far. Guenter
On 30/05/2025 21:38, Uwe Kleine-König wrote: > Hello Chris, > > On Wed, May 28, 2025 at 09:18:37PM +0000, Chris Packham wrote: >> On 28/05/2025 18:10, Uwe Kleine-König wrote: >>> If I understand correctly you need the default value for duty to >>> statically setup (or only initialize?) a fan, right? >> Correct. >> >>> I'm not sure I like >>> extending #pwm-cells for a default duty value. Thinking about that a >>> while I'd prefer a binding that looks more like the clock configuration >>> stuff because actually having the period and flags as part of the >>> reference to the PWM to be used is also a bit strange. So I imagine >>> something like: >>> >>> mypwm: pwm { >>> compatible = "...." >>> #pwm-cells = <1>; >>> }; >>> >>> fan { >>> compatible = "pwm-fan"; >>> pwms = <&mypwm 1>; >>> assigned-pwms = <&mypwm>; >>> assigned-pwm-default-period-lengths-ns = <40000>; >>> assigned-pwm-default-flags = <PWM_POLARITY_INVERTED>; >>> }; >>> >>> Then specifying a period (or later a duty cycle length) would be >>> optional and could be provided iff the device needs that for operation. >> The frequency and flags were already part of the standard #pwm-cells >> which I think is why I was encouraged to use them. > Yeah, that part is fine. This might not be the long-term future, but > today that's the norm. > >> I was also trying to get something that would work as an ACPI overlay >> which turned out to be really hard. > I don't know enough about ACPI to be helpful with this quest. > >>> My mail was just me being frustrated about another special case that I'd >>> have to handle if I go into that direction. I should have been more >>> attentive to that development before it entered the mainline. >> I'd be happy to deprecate the 4 cell thing and replace it with 3 cell + >> vendor property for the default period if that helps. > I wonder how other similar devices determine the default duty cycle. > Isn't the norm to make the fan rotate at max speed and then when > userspace takes over it's speeded down? Yes that is the normal (and sensible thing do to). But occasionally hardware designers like to use incredibly over spec'd fans that are just ridiculously noisy. On some products I've worked on we added basic fan control to u-boot so we could silence the fans early in the boot. I also gather that in the PC world the fan control is often done externally to the OS. In the specific case were I needed this functionality it was an embedded x86_64 so I had neither U-Boot nor a BMC.
Hello Chris, On Mon, Jun 02, 2025 at 08:52:56PM +0000, Chris Packham wrote: > On 30/05/2025 21:38, Uwe Kleine-König wrote: > > I wonder how other similar devices determine the default duty cycle. > > Isn't the norm to make the fan rotate at max speed and then when > > userspace takes over it's speeded down? > > Yes that is the normal (and sensible thing do to). But occasionally > hardware designers like to use incredibly over spec'd fans that are > just ridiculously noisy. On some products I've worked on we added basic > fan control to u-boot so we could silence the fans early in the boot. I > also gather that in the PC world the fan control is often done > externally to the OS. In the specific case were I needed this > functionality it was an embedded x86_64 so I had neither U-Boot nor a BMC. So you're saying that Linux is the first instance that is able to setup the fan -- no BIOS, right? I think that's quite normal and the fan is only noisy until userspace takes over. So I still think that is what should happen here. In case this is really too noisy, I'd prefer the driver to know itself how to setup the fan and not put that policy into the device tree. Best regards Uwe
diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml index 051c976ab711..df2b5b889e4d 100644 --- a/Documentation/devicetree/bindings/hwmon/adt7475.yaml +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml @@ -51,6 +51,24 @@ 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 period in nanoseconds + - 90909091 (11 Hz) + - 71428571 (14 Hz) + - 45454545 (22 Hz) + - 34482759 (29 Hz) + - 28571429 (35 Hz) + - 22727273 (44 Hz) + - 17241379 (58 Hz) + - 11363636 (88 Hz) + - 44444 (22 kHz) + - 2: PWM flags 0 or PWM_POLARITY_INVERTED + - 3: The default PWM duty cycle in nanoseconds + patternProperties: "^adi,bypass-attenuator-in[0-4]$": description: | @@ -81,6 +99,10 @@ patternProperties: - smbalert# - gpio + "^fan-[0-9]+$": + $ref: fan-common.yaml# + unevaluatedProperties: false + required: - compatible - reg @@ -89,11 +111,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 +124,15 @@ examples: adi,pwm-active-state = <1 0 1>; adi,pin10-function = "smbalert#"; adi,pin14-function = "tach4"; + #pwm-cells = <4>; + + /* PWMs at 22.5 kHz frequency, 50% duty*/ + fan-0 { + pwms = <&pwm 0 44444 0 22222>; + }; + + fan-1 { + pwms = <&pwm 2 44444 0 22222>; + }; }; };