linux-kernel - Re: [PATCH v6 3/3] hwmon: (adt7475) Add support for configuring initial PWM state

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <e28e9ff2-a48e-4cf2-b763-6bf3b5d9a959@alliedtelesis.co.nz>
Date: Mon, 22 Jul 2024 16:36:38 +1200
From: Chris Packham <chris.packham@...iedtelesis.co.nz>
To: Guenter Roeck <linux@...ck-us.net>, jdelvare@...e.com, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, ukleinek@...nel.org
Cc: linux-hwmon@...r.kernel.org, devicetree@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org
Subject: Re: [PATCH v6 3/3] hwmon: (adt7475) Add support for configuring
 initial PWM state


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@...iedtelesis.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
>>>
>