Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion macros

[Hans de Goede <hdegoede@redhat.com>]

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