| Message ID | 20240722005825.1800403-4-chris.packham@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (adt7475) duty cycle configuration | expand |
Hi Chris,
kernel test robot noticed the following build errors:
[auto build test ERROR on groeck-staging/hwmon-next]
[also build test ERROR on linus/master v6.10 next-20240719]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Chris-Packham/dt-bindings-hwmon-Add-adt7475-fan-pwm-properties/20240722-091004
base: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git hwmon-next
patch link: https://lore.kernel.org/r/20240722005825.1800403-4-chris.packham%40alliedtelesis.co.nz
patch subject: [PATCH v6 3/3] hwmon: (adt7475) Add support for configuring initial PWM state
config: arm-randconfig-002-20240722 (https://download.01.org/0day-ci/archive/20240722/202407221153.LJjYD5i7-lkp@intel.com/config)
compiler: clang version 19.0.0git (https://github.com/llvm/llvm-project ad154281230d83ee551e12d5be48bb956ef47ed3)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240722/202407221153.LJjYD5i7-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202407221153.LJjYD5i7-lkp@intel.com/
All error/warnings (new ones prefixed by >>):
In file included from drivers/hwmon/adt7475.c:15:
In file included from include/linux/i2c.h:19:
In file included from include/linux/regulator/consumer.h:35:
In file included from include/linux/suspend.h:5:
In file included from include/linux/swap.h:9:
In file included from include/linux/memcontrol.h:21:
In file included from include/linux/mm.h:2253:
include/linux/vmstat.h:514:36: warning: arithmetic between different enumeration types ('enum node_stat_item' and 'enum lru_list') [-Wenum-enum-conversion]
514 | return node_stat_name(NR_LRU_BASE + lru) + 3; // skip "nr_"
| ~~~~~~~~~~~ ^ ~~~
>> drivers/hwmon/adt7475.c:1683:2: warning: comparison of distinct pointer types ('typeof ((freq_hz)) *' (aka 'unsigned long *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
1683 | do_div(freq_hz, args[1]);
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
>> drivers/hwmon/adt7475.c:1683:2: error: incompatible pointer types passing 'unsigned long *' to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
1683 | do_div(freq_hz, args[1]);
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
arch/arm/include/asm/div64.h:24:45: note: passing argument to parameter 'n' here
24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
| ^
>> drivers/hwmon/adt7475.c:1685:2: warning: comparison of distinct pointer types ('typeof ((duty)) *' (aka 'unsigned long *') and 'uint64_t *' (aka 'unsigned long long *')) [-Wcompare-distinct-pointer-types]
1685 | do_div(duty, args[1]);
| ^~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:222:28: note: expanded from macro 'do_div'
222 | (void)(((typeof((n)) *)0) == ((uint64_t *)0)); \
| ~~~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~~~~
drivers/hwmon/adt7475.c:1685:2: error: incompatible pointer types passing 'unsigned long *' to parameter of type 'uint64_t *' (aka 'unsigned long long *') [-Werror,-Wincompatible-pointer-types]
1685 | do_div(duty, args[1]);
| ^~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:238:22: note: expanded from macro 'do_div'
238 | __rem = __div64_32(&(n), __base); \
| ^~~~
arch/arm/include/asm/div64.h:24:45: note: passing argument to parameter 'n' here
24 | static inline uint32_t __div64_32(uint64_t *n, uint32_t base)
| ^
>> drivers/hwmon/adt7475.c:1683:2: warning: shift count >= width of type [-Wshift-count-overflow]
1683 | do_div(freq_hz, args[1]);
| ^~~~~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
drivers/hwmon/adt7475.c:1685:2: warning: shift count >= width of type [-Wshift-count-overflow]
1685 | do_div(duty, args[1]);
| ^~~~~~~~~~~~~~~~~~~~~
include/asm-generic/div64.h:234:25: note: expanded from macro 'do_div'
234 | } else if (likely(((n) >> 32) == 0)) { \
| ^ ~~
include/linux/compiler.h:76:40: note: expanded from macro 'likely'
76 | # define likely(x) __builtin_expect(!!(x), 1)
| ^
5 warnings and 2 errors generated.
vim +1683 drivers/hwmon/adt7475.c
1673
1674 static int _adt7475_pwm_properties_parse_args(u32 args[4], struct adt7475_pwm_config *cfg)
1675 {
1676 unsigned long freq_hz;
1677 unsigned long duty;
1678
1679 if (args[1] == 0)
1680 return -EINVAL;
1681
1682 freq_hz = 1000000000UL;
> 1683 do_div(freq_hz, args[1]);
1684 duty = 255 * args[3];
> 1685 do_div(duty, args[1]);
1686
1687 cfg->index = args[0];
1688 cfg->freq = find_closest(freq_hz, pwmfreq_table, ARRAY_SIZE(pwmfreq_table));
1689 cfg->flags = args[2];
1690 cfg->duty = clamp_val(duty, 0, 0xFF);
1691
1692 return 0;
1693 }
1694
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
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]); + + 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); + + return 0; +} + +static int adt7475_pwm_properties_parse_reference_args(struct fwnode_handle *fwnode, + struct adt7475_pwm_config *cfg) +{ + int ret, i; + struct fwnode_reference_args rargs = {}; + u32 args[4] = {}; + + ret = fwnode_property_get_reference_args(fwnode, "pwms", "#pwm-cells", 0, 0, &rargs); + if (ret) + return ret; + + if (rargs.nargs != 4) { + fwnode_handle_put(rargs.fwnode); + return -EINVAL; + } + + for (i = 0; i < 4; i++) + args[i] = rargs.args[i]; + + ret = _adt7475_pwm_properties_parse_args(args, cfg); + + fwnode_handle_put(rargs.fwnode); + + return ret; +} + +static int adt7475_pwm_properties_parse_args(struct fwnode_handle *fwnode, + struct adt7475_pwm_config *cfg) +{ + int ret; + u32 args[4] = {}; + + ret = fwnode_property_read_u32_array(fwnode, "pwms", args, ARRAY_SIZE(args)); + if (ret) + return ret; + + return _adt7475_pwm_properties_parse_args(args, cfg); + +} + +static int adt7475_fan_pwm_config(struct i2c_client *client) +{ + struct adt7475_data *data = i2c_get_clientdata(client); + struct fwnode_handle *child; + struct adt7475_pwm_config cfg = {}; + int ret; + + device_for_each_child_node(&client->dev, child) { + if (!fwnode_property_present(child, "pwms")) + continue; + + if (is_of_node(child)) + ret = adt7475_pwm_properties_parse_reference_args(child, &cfg); + else + ret = adt7475_pwm_properties_parse_args(child, &cfg); + + if (cfg.index >= ADT7475_PWM_COUNT) + return -EINVAL; + + ret = adt7475_read(PWM_CONFIG_REG(cfg.index)); + if (ret < 0) + return ret; + data->pwm[CONTROL][cfg.index] = ret; + if (cfg.flags & PWM_POLARITY_INVERTED) + data->pwm[CONTROL][cfg.index] |= BIT(4); + else + data->pwm[CONTROL][cfg.index] &= ~BIT(4); + + /* Force to manual mode so PWM values take effect */ + data->pwm[CONTROL][cfg.index] &= ~0xE0; + data->pwm[CONTROL][cfg.index] |= 0x07 << 5; + + ret = i2c_smbus_write_byte_data(client, PWM_CONFIG_REG(cfg.index), + data->pwm[CONTROL][cfg.index]); + if (ret) + return ret; + + data->pwm[INPUT][cfg.index] = cfg.duty; + ret = i2c_smbus_write_byte_data(client, PWM_REG(cfg.index), + data->pwm[INPUT][cfg.index]); + if (ret) + return ret; + + data->range[cfg.index] = adt7475_read(TEMP_TRANGE_REG(cfg.index)); + data->range[cfg.index] &= ~0xf; + data->range[cfg.index] |= cfg.freq; + + ret = i2c_smbus_write_byte_data(client, TEMP_TRANGE_REG(cfg.index), + data->range[cfg.index]); + if (ret) + return ret; + } + + return 0; +} + static int adt7475_probe(struct i2c_client *client) { enum chips chip; @@ -1778,6 +1904,10 @@ static int adt7475_probe(struct i2c_client *client) if (ret && ret != -EINVAL) dev_warn(&client->dev, "Error configuring pwm polarity\n"); + ret = adt7475_fan_pwm_config(client); + if (ret) + dev_warn(&client->dev, "Error %d configuring fan/pwm\n", ret); + /* Start monitoring */ switch (chip) { case adt7475:
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(+)