| Message ID | 20240722005825.1800403-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| Headers | show |
| Series | hwmon: (adt7475) duty cycle configuration | expand |
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@alliedtelesis.co.nz> > --- > > 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. Thanks, Guenter
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@alliedtelesis.co.nz> >> --- >> >> 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. > > Thanks, > Guenter >
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@alliedtelesis.co.nz> >>> --- >>> >>> 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. Guenter >> >> Thanks, >> Guenter >>
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@alliedtelesis.co.nz> >>>> --- >>>> >>>> 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]; duty = 255 * args[3] / args[1]; ... > > Guenter > >>> >>> Thanks, >>> Guenter >>> >
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@alliedtelesis.co.nz> >>>>> --- >>>>> >>>>> 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
On Mon, Jul 22, 2024 at 04:09:46PM +1200, 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@alliedtelesis.co.nz> > > > --- > > > > > > 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. My comment was only about the build bot finding a division where the gcc stub was missing with is an indication that do_div should be used. Usually for PWMs perdiod and duty_cycle are u64, but here it's only about values from the dtb, so they are u32 and a plain / should be fine. > can probably get away with plain division, although 255 * args[3] / args[1] > might overflow in theory but shouldn't in practice. I don't like possible overflows, but I don't care enough for hwmon drivers to object. Still a check for args[3] <= 0x1010101 would be easy enough. 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 | 130 ++++++++++++++++++ 2 files changed, 165 insertions(+), 2 deletions(-)