| Message ID | 20240528225638.1211676-2-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (adt7475) duty cycle configuration | expand |
On Wed, May 29, 2024 at 10:56:36AM +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: > I realise there is still some discussion about how to express the > frequency and duty cycle. I have a personal preference for using hertz > for the frequency and 0-255 for the duty cycle but if the consensus is > to express these things some other way I'm fine with doing some math. Probably worth carrying a link to it here: https://lore.kernel.org/all/4de798f3-069e-4028-a5b5-5e6a639277e3@alliedtelesis.co.nz/ I asked Uwe to take a look & it's on his todo list. Thanks, Conor. > > 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 | 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..bfef4c803bf7 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 - 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>; > + }; > }; > }; > -- > 2.45.1 >
Uwe, On Wed, May 29, 2024 at 08:58:41AM +0100, Conor Dooley wrote: > On Wed, May 29, 2024 at 10:56:36AM +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: > > I realise there is still some discussion about how to express the > > frequency and duty cycle. I have a personal preference for using hertz > > for the frequency and 0-255 for the duty cycle but if the consensus is > > to express these things some other way I'm fine with doing some math. > > Probably worth carrying a link to it here: > https://lore.kernel.org/all/4de798f3-069e-4028-a5b5-5e6a639277e3@alliedtelesis.co.nz/ > > I asked Uwe to take a look & it's on his todo list. This is an attempt to bump this up on your todo list a bit! Thanks, Conor. > > > > 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 | 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..bfef4c803bf7 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 - 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>; > > + }; > > }; > > }; > > -- > > 2.45.1 > >
Hello Chris, sorry for taking so long to respond. Don't take it personal, I'm way behind my maintainer dutys in general. On Wed, May 29, 2024 at 10:56:36AM +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: > I realise there is still some discussion about how to express the > frequency and duty cycle. I have a personal preference for using hertz > for the frequency and 0-255 for the duty cycle but if the consensus is > to express these things some other way I'm fine with doing some math. > > 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 | 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..bfef4c803bf7 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 - 11, 14, 22, 29, 35, 44, 58, 88, 22500 Nack, don't deviate from how PWMs are usually referenced. So specify the period in nanoseconds, not Hertz. > + - 2: PWM flags 0 or PWM_POLARITY_INVERTED > + - 3: The default pwm duty cycle - 0-255 I'd be ok with that, however please add support for that in the pwm core and then just use that. You wrote that you find it irritating that the duty is specified in nanoseconds and not a percentage. The reason for that is historic. Also it gives a more precise specification (at least compared to the naive representation of the integer percentage as an integer type). For the rework of how PWM waveforms are represented I picked "duty_length" as variable name for this value, in the hope this is less confusing. untested prototype for a 4th member in pwm specifiers: diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c index 5c1d20985148..f732235df12d 100644 --- a/drivers/pwm/core.c +++ b/drivers/pwm/core.c @@ -251,7 +251,7 @@ int pwm_adjust_config(struct pwm_device *pwm) * duty cycle of 0. */ if (!state.period) { - state.duty_cycle = 0; + state.duty_cycle = pargs.duty_length; state.period = pargs.period; state.polarity = pargs.polarity; @@ -437,6 +437,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *arg if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; + pwm->args.duty_length = 0; + if (args->args_count > 3) + pwm->args.duty_length = args->args[3]; + return pwm; } EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags); @@ -457,6 +461,10 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args) if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; + pwm->args.duty_length = 0; + if (args->args_count > 2) + pwm->args.duty_length = args->args[2]; + return pwm; } EXPORT_SYMBOL_GPL(of_pwm_single_xlate); @@ -1353,6 +1361,9 @@ static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode) if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED) pwm->args.polarity = PWM_POLARITY_INVERSED; + /* Maybe extend this to apply args.args[3] if args.nargs > 3? */ + pwm->args.duty_cycle = 0; + return pwm; } @@ -1514,6 +1525,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) pwm->args.period = chosen->period; pwm->args.polarity = chosen->polarity; + pwm->args.duty_length = 0; return pwm; } diff --git a/include/linux/pwm.h b/include/linux/pwm.h index f8c2dc12dbd3..678a97706eac 100644 --- a/include/linux/pwm.h +++ b/include/linux/pwm.h @@ -41,6 +41,7 @@ enum pwm_polarity { */ struct pwm_args { u64 period; + u32 duty_length; enum pwm_polarity polarity; }; (I think it doesn't make sense to use a u64 here. At least the oftree values are only 32 bit wide. I didn't check the ACPI part, if that is only 32 bit wide, too, it would make sense to make period use a 32 bit type, too.) Best regards Uwe
Hi Uwe, On 5/07/24 21:09, Uwe Kleine-König wrote: > Hello Chris, > > sorry for taking so long to respond. Don't take it personal, I'm way > behind my maintainer dutys in general. > > On Wed, May 29, 2024 at 10:56:36AM +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: >> I realise there is still some discussion about how to express the >> frequency and duty cycle. I have a personal preference for using hertz >> for the frequency and 0-255 for the duty cycle but if the consensus is >> to express these things some other way I'm fine with doing some math. >> >> 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 | 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..bfef4c803bf7 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 - 11, 14, 22, 29, 35, 44, 58, 88, 22500 > Nack, don't deviate from how PWMs are usually referenced. So specify the > period in nanoseconds, not Hertz. OK no problem. The math is easy enough. I might end up over documenting things for the acceptable PWM period so someone can weigh in on that. >> + - 2: PWM flags 0 or PWM_POLARITY_INVERTED >> + - 3: The default pwm duty cycle - 0-255 > I'd be ok with that, however please add support for that in the pwm core > and then just use that. > > You wrote that you find it irritating that the duty is specified in > nanoseconds and not a percentage. The reason for that is historic. Also > it gives a more precise specification (at least compared to the naive > representation of the integer percentage as an integer type). > For the rework of how PWM waveforms are represented I picked > "duty_length" as variable name for this value, in the hope this is less > confusing. > > untested prototype for a 4th member in pwm specifiers: Right now the adt7475 is not a pwm_chip and I'm not really planning on making it one. My goal with this was just to make it possible for the kernel to keep the fans running quietly before userland can take over and start doing proper configuration/monitoring. The best I can do is make sure that the devicetree binding is done in such a way that it could grow pwm_chip capabilities in the future. > > diff --git a/drivers/pwm/core.c b/drivers/pwm/core.c > index 5c1d20985148..f732235df12d 100644 > --- a/drivers/pwm/core.c > +++ b/drivers/pwm/core.c > @@ -251,7 +251,7 @@ int pwm_adjust_config(struct pwm_device *pwm) > * duty cycle of 0. > */ > if (!state.period) { > - state.duty_cycle = 0; > + state.duty_cycle = pargs.duty_length; > state.period = pargs.period; > state.polarity = pargs.polarity; > > @@ -437,6 +437,10 @@ of_pwm_xlate_with_flags(struct pwm_chip *chip, const struct of_phandle_args *arg > if (args->args_count > 2 && args->args[2] & PWM_POLARITY_INVERTED) > pwm->args.polarity = PWM_POLARITY_INVERSED; > > + pwm->args.duty_length = 0; > + if (args->args_count > 3) > + pwm->args.duty_length = args->args[3]; > + > return pwm; > } > EXPORT_SYMBOL_GPL(of_pwm_xlate_with_flags); > @@ -457,6 +461,10 @@ of_pwm_single_xlate(struct pwm_chip *chip, const struct of_phandle_args *args) > if (args->args_count > 1 && args->args[1] & PWM_POLARITY_INVERTED) > pwm->args.polarity = PWM_POLARITY_INVERSED; > > + pwm->args.duty_length = 0; > + if (args->args_count > 2) > + pwm->args.duty_length = args->args[2]; > + > return pwm; > } > EXPORT_SYMBOL_GPL(of_pwm_single_xlate); > @@ -1353,6 +1361,9 @@ static struct pwm_device *acpi_pwm_get(const struct fwnode_handle *fwnode) > if (args.nargs > 2 && args.args[2] & PWM_POLARITY_INVERTED) > pwm->args.polarity = PWM_POLARITY_INVERSED; > > + /* Maybe extend this to apply args.args[3] if args.nargs > 3? */ > + pwm->args.duty_cycle = 0; > + > return pwm; > } > > @@ -1514,6 +1525,7 @@ struct pwm_device *pwm_get(struct device *dev, const char *con_id) > > pwm->args.period = chosen->period; > pwm->args.polarity = chosen->polarity; > + pwm->args.duty_length = 0; > > return pwm; > } > diff --git a/include/linux/pwm.h b/include/linux/pwm.h > index f8c2dc12dbd3..678a97706eac 100644 > --- a/include/linux/pwm.h > +++ b/include/linux/pwm.h > @@ -41,6 +41,7 @@ enum pwm_polarity { > */ > struct pwm_args { > u64 period; > + u32 duty_length; > enum pwm_polarity polarity; > }; > > (I think it doesn't make sense to use a u64 here. At least the oftree > values are only 32 bit wide. I didn't check the ACPI part, if that is > only 32 bit wide, too, it would make sense to make period use a 32 bit > type, too.) > > Best regards > Uwe
diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml index 051c976ab711..bfef4c803bf7 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 - 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: I realise there is still some discussion about how to express the frequency and duty cycle. I have a personal preference for using hertz for the frequency and 0-255 for the duty cycle but if the consensus is to express these things some other way I'm fine with doing some math. 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 | 25 ++++++++++++++++++- 1 file changed, 24 insertions(+), 1 deletion(-)