| Message ID | 20240711234614.3104839-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | hwmon: (adt7475) duty cycle configuration | expand |
On 7/11/24 16:46, Chris Packham wrote: > By default the PWM duty cycle in hardware is 100%. On some systems this > can cause unwanted fan noise. Add the ability to specify the fan > connections and initial state of the PWMs via device properties. > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- ... > +static int adt7475_pwm_properties_parse_reference_args(struct fwnode_handle *fwnode, > + struct adt7475_pwm_config *cfg) > +{ > + int ret; > + struct fwnode_reference_args args = {}; > + int freq_hz; > + int duty; > + > + ret = fwnode_property_get_reference_args(fwnode, "pwms", "#pwm-cells", 0, 0, &args); > + if (ret) > + return ret; > + > + if (args.nargs != 4) { > + fwnode_handle_put(args.fwnode); > + return -EINVAL; > + } > + > + freq_hz = 1000000000UL / args.args[1]; > + duty = 255 / (args.args[1] / args.args[3]); > + You'll need to validate args.args[1] and args.args[3] to ensure that there are no divide by 0 errors. On a side note, a = b / (c / d) == b / c * d (at least for d != 0) Since the result is defined for d == 0, you'd only have to make sure that args.args[1] > 0 and that the result for the duty cycle is <= 255. > + cfg->index = args.args[0]; > + cfg->freq = find_closest(freq_hz, pwmfreq_table, ARRAY_SIZE(pwmfreq_table)); > + cfg->flags = args.args[2]; > + cfg->duty = clamp_val(duty, 0, 0xFF); > + > + fwnode_handle_put(args.fwnode); > + > + return 0; > +} > + > +static int adt7475_pwm_properties_parse_args(struct fwnode_handle *fwnode, > + struct adt7475_pwm_config *cfg) > +{ > + int ret; > + u32 args[4] = {}; > + int freq_hz; > + int duty; > + > + ret = fwnode_property_read_u32_array(fwnode, "pwms", args, ARRAY_SIZE(args)); > + if (ret) > + return ret; > + > + freq_hz = 1000000000UL / args[1]; > + duty = 255 / (args[1] / args[3]); > + > + cfg->index = args[0]; > + cfg->freq = find_closest(freq_hz, pwmfreq_table, ARRAY_SIZE(pwmfreq_table)); > + cfg->flags = args[2]; > + cfg->duty = clamp_val(duty, 0, 0xFF); > + The code above is duplicate; please combine into a single function (I don't mind if it has four parameters). Thanks, Guenter
On Thu, Jul 11, 2024 at 09:37:29PM -0700, Guenter Roeck wrote: > On 7/11/24 16:46, Chris Packham wrote: > > By default the PWM duty cycle in hardware is 100%. On some systems this > > can cause unwanted fan noise. Add the ability to specify the fan > > connections and initial state of the PWMs via device properties. > > > > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > > --- > ... > > +static int adt7475_pwm_properties_parse_reference_args(struct fwnode_handle *fwnode, > > + struct adt7475_pwm_config *cfg) > > +{ > > + int ret; > > + struct fwnode_reference_args args = {}; > > + int freq_hz; > > + int duty; > > + > > + ret = fwnode_property_get_reference_args(fwnode, "pwms", "#pwm-cells", 0, 0, &args); > > + if (ret) > > + return ret; > > + > > + if (args.nargs != 4) { > > + fwnode_handle_put(args.fwnode); > > + return -EINVAL; > > + } > > + > > + freq_hz = 1000000000UL / args.args[1]; > > + duty = 255 / (args.args[1] / args.args[3]); > > + > You'll need to validate args.args[1] and args.args[3] to ensure that there are no > divide by 0 errors. > > On a side note, > a = b / (c / d) == b / c * d (at least for d != 0) > Since the result is defined for d == 0, you'd only have to make sure > that args.args[1] > 0 and that the result for the duty cycle is <= 255. On a side side note: Depending on the actual values it might be beneficial to use b * d / c instead. b * d might overflow, but in other cases (e.g. b = 7, c = 8, d = 8) the resulting precision is much better. Best regards Uwe
I have a system that has very over spec'd fans so the amount of noise when they run at 100% duty cycle is considerable. We have userspace monitoring tools that will configure appropriate fan control parameters but there is a bit of a delay between the kernel loading the driver and the userland tools catching up to configure the fan control. This series adds device properties that allow the PWM duty cycle to be specified via device properties so the PWM duty cycle can be reduced as soon as possible. This series attempts to setup the adt7475 as a pwm provider so that we can specify these properties. The devicetree support was reasonably straight forward (example usage is in the binding patch). I struggled to get the ACPI version working well and in the end the code had to distinguish between the of_node and other case. The ASL I've ended up with is Device (ADT0) { Name (_HID, "PRP0001") Name (_CRS, ResourceTemplate () { I2cSerialBusV2 (0x2E, ControllerInitiated, 100000, AddressingMode7Bit, "^^CH00", 0x00, ResourceConsumer, , Exclusive, ) }) Name (_DSD, Package () { ToUUID (UUID_DEVICE_PROPERTIES), Package () { Package () { "compatible", "adi,adt7476" }, Package () { "#pwm-cells", 4 }, }, }) Device (FAN0) { Name (_ADR, 0) Name (_DSD, Package () { ToUUID (UUID_DEVICE_PROPERTIES), Package () { Package () { "pwms", Package () { 0, 44444, 1, 22222 } }, } }) } Device (FAN1) { Name (_ADR, 0) Name (_DSD, Package () { ToUUID (UUID_DEVICE_PROPERTIES), Package () { Package () { "pwms", Package () { 2, 44444, 1, 22222 } }, } }) } } If had to introduce a code path that parses that because try as I might I could not convince fwnode_property_get_reference_args() to fetch the information out of the ACPI data. If I've missed something obvious please let me know. Chris Packham (3): dt-bindings: hwmon: Add adt7475 fan/pwm properties dt-bindings: hwmon: adt7475: Deprecate adi,pwm-active-state hwmon: (adt7475) Add support for configuring initial PWM state .../devicetree/bindings/hwmon/adt7475.yaml | 37 +++++- drivers/hwmon/adt7475.c | 122 ++++++++++++++++++ 2 files changed, 157 insertions(+), 2 deletions(-)