On 7/21/24 21:36, Chris Packham wrote:
On 22/07/24 16:27, Guenter Roeck wrote:
On 7/21/24 21:09, Chris Packham wrote:
On 22/07/24 15:53, Guenter Roeck wrote:
On 7/21/24 17:58, 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@xxxxxxxxxxxxxxxxxxx>
---
Notes:
Changes in v6:
- Use do_div() instead of plain /
- Use a helper function to avoid repetition between the of and non-of
code paths.
Changes in v5:
- Deal with PWM frequency and duty cycle being specified in nanoseconds
Changes in v4:
- Support DT and ACPI fwnodes
- Put PWM into manual mode
Changes in v3:
- Use the pwm provider/consumer bindings
Changes in v2:
- Use correct device property string for frequency
- Allow -EINVAL and only warn on error
- Use a frequency of 0 to indicate that the hardware should be left as-is
drivers/hwmon/adt7475.c | 130 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 130 insertions(+)
diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 4224ffb30483..fc5605d34f36 100644
--- a/drivers/hwmon/adt7475.c
+++ b/drivers/hwmon/adt7475.c
@@ -21,6 +21,8 @@
#include <linux/of.h>
#include <linux/util_macros.h>
+#include <dt-bindings/pwm/pwm.h>
+
/* Indexes for the sysfs hooks */
#define INPUT 0
@@ -1662,6 +1664,130 @@ static int adt7475_set_pwm_polarity(struct i2c_client *client)
return 0;
}
+struct adt7475_pwm_config {
+ int index;
+ int freq;
+ int flags;
+ int duty;
+};
+
+static int _adt7475_pwm_properties_parse_args(u32 args[4], struct adt7475_pwm_config *cfg)
+{
+ unsigned long freq_hz;
+ unsigned long duty;
+
+ if (args[1] == 0)
+ return -EINVAL;
+
+ freq_hz = 1000000000UL;
+ do_div(freq_hz, args[1]);
+ duty = 255 * args[3];
+ do_div(duty, args[1]);
+
Gues I am a bit at loss here, just as 0-day. Why use do_div ? It is only needed
for 64-bit divide operations.
Mainly because of Uwe's comment on v5. I think I've avoided the original u64 issue now that I'm converting fwnode_reference_args::args to a u32 array. I can probably get away with plain division, although 255 * args[3] / args[1] might overflow in theory but shouldn't in practice.
I'll let the earth turn and send out a v7 that uses plain division unless someone has a strong opinion that I should sprinkle some more u64s around.
You lost me, sorry. Neither duty nor freq_hz are u64. What u64 variables
are you talking about ? Using so_div doesn't make those variables u64.
One way of fixing the 0-day complaint (I think) is to declare freq_hz and duty as u64 which would avoid all the theoretical overflow issues.
But plain division is probably easier to understand for everyone so I'll send out something like this in v7
(unsigned?) int freq_hz;
(unsigned?) int duty;
...
freq_hz = 1000000000UL / args[1];
This can not overflow.
duty = 255 * args[3] / args[1];
This will overflow if args[3] is larger than 16843009. What is its expected range ?
But even then you'd want something like
duty = div_u64(255ULL * args[3], args[1]);
or
if (args[3] >= args[1])
duty = 255;
else
duty = div_u64(255ULL * args[3], args[1]);
to be able to drop the subsequent clamp_val() on duty.
Guenter
