linux-kernel - Re: [PATCH 1/4] power: supply: max17042

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: <3c890d11-75c6-8370-5659-a78b1a9baea2@redhat.com>
Date:   Fri, 18 Mar 2022 10:00:58 +0100
From:   Hans de Goede <hdegoede@...hat.com>
To:     Krzysztof Kozlowski <krzk@...nel.org>,
        Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>,
        Marek Szyprowski <m.szyprowski@...sung.com>,
        Sebastian Reichel <sre@...nel.org>, linux-pm@...r.kernel.org
Cc:     Purism Kernel Team <kernel@...i.sm>, Rob Herring <robh@...nel.org>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion
 macros

Hi,

On 3/18/22 09:16, Krzysztof Kozlowski wrote:
> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>> Instead of sprinkling the code with magic numbers, put the unit
>> definitions used by the gauge into a set of macros. Macros are
>> used instead of simple defines in order to not require floating
>> point operations for divisions.
>>
>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@...i.sm>
>> ---
>>  drivers/power/supply/max17042_battery.c | 40 +++++++++++++++----------
>>  1 file changed, 24 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
>> index ab031bbfbe78..c019d6c52363 100644
>> --- a/drivers/power/supply/max17042_battery.c
>> +++ b/drivers/power/supply/max17042_battery.c
>> @@ -51,6 +51,15 @@
>>  
>>  #define MAX17042_VMAX_TOLERANCE		50 /* 50 mV */
>>  
>> +#define MAX17042_CURRENT_LSB		1562500ll /* µV */
> 
> Is this really long long? The usage in max17042_get_status() is with int
> operand and result.

The "ll" is part of the original code which these macros replace,
dropping the "ll" is IMHO out of scope for this patch, it would
clearly break the only change 1 thing per patch/commit rule.

>> +#define MAX17042_CURRENT_RSENSE(x)	(x * MAX17042_CURRENT_LSB) /* µV */
>> +#define MAX17042_CAPACITY_LSB		5000000ll /* µVh */
>> +#define MAX17042_CAPACITY_RSENSE(x)	(x * MAX17042_CAPACITY_LSB) /* µVh */
>> +#define MAX17042_TIME(x)		(x * 5625 / 1000) /* s */
>> +#define MAX17042_VOLTAGE(x)		(x * 625 / 8) /* µV */
>> +#define MAX17042_RESISTANCE(x)		(x / 4096) /* Ω */
>> +#define MAX17042_TEMPERATURE(x)		(x / 256) /* °C */
> 
> Please enclose the "x" in (), in each macro

Ack, right I should have spotted that in my own review.

Regards,

Hans