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 <[email protected]>
---
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 */
+#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 */
+
struct max17042_chip {
struct i2c_client *client;
struct regmap *regmap;
@@ -103,8 +112,7 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
*temp = sign_extend32(data, 15);
/* The value is converted into deci-centigrade scale */
- /* Units of LSB = 1 / 256 degree Celsius */
- *temp = *temp * 10 / 256;
+ *temp = MAX17042_TEMPERATURE(*temp * 10);
return 0;
}
@@ -161,7 +169,7 @@ static int max17042_get_status(struct max17042_chip *chip, int *status)
return ret;
avg_current = sign_extend32(data, 15);
- avg_current *= 1562500 / chip->pdata->r_sns;
+ avg_current *= MAX17042_CURRENT_LSB / chip->pdata->r_sns;
if (avg_current > 0)
*status = POWER_SUPPLY_STATUS_CHARGING;
@@ -181,7 +189,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
goto health_error;
/* bits [0-3] unused */
- vavg = val * 625 / 8;
+ vavg = MAX17042_VOLTAGE(val);
/* Convert to millivolts */
vavg /= 1000;
@@ -190,7 +198,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
goto health_error;
/* bits [0-3] unused */
- vbatt = val * 625 / 8;
+ vbatt = MAX17042_VOLTAGE(val);
/* Convert to millivolts */
vbatt /= 1000;
@@ -297,21 +305,21 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- val->intval = data * 625 / 8;
+ val->intval = MAX17042_VOLTAGE(data);
break;
case POWER_SUPPLY_PROP_VOLTAGE_AVG:
ret = regmap_read(map, MAX17042_AvgVCELL, &data);
if (ret < 0)
return ret;
- val->intval = data * 625 / 8;
+ val->intval = MAX17042_VOLTAGE(data);
break;
case POWER_SUPPLY_PROP_VOLTAGE_OCV:
ret = regmap_read(map, MAX17042_OCVInternal, &data);
if (ret < 0)
return ret;
- val->intval = data * 625 / 8;
+ val->intval = MAX17042_VOLTAGE(data);
break;
case POWER_SUPPLY_PROP_CAPACITY:
if (chip->pdata->enable_current_sense)
@@ -328,7 +336,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = data * 5000000ll;
+ data64 = MAX17042_CAPACITY_RSENSE(data);
do_div(data64, chip->pdata->r_sns);
val->intval = data64;
break;
@@ -337,7 +345,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = data * 5000000ll;
+ data64 = MAX17042_CAPACITY_RSENSE(data);
do_div(data64, chip->pdata->r_sns);
val->intval = data64;
break;
@@ -346,7 +354,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = data * 5000000ll;
+ data64 = MAX17042_CAPACITY_RSENSE(data);
do_div(data64, chip->pdata->r_sns);
val->intval = data64;
break;
@@ -355,7 +363,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = sign_extend64(data, 15) * 5000000ll;
+ data64 = MAX17042_CAPACITY_RSENSE(sign_extend64(data, 15));
val->intval = div_s64(data64, chip->pdata->r_sns);
break;
case POWER_SUPPLY_PROP_TEMP:
@@ -397,7 +405,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = sign_extend64(data, 15) * 1562500ll;
+ data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
val->intval = div_s64(data64, chip->pdata->r_sns);
} else {
return -EINVAL;
@@ -409,7 +417,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = sign_extend64(data, 15) * 1562500ll;
+ data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
val->intval = div_s64(data64, chip->pdata->r_sns);
} else {
return -EINVAL;
@@ -420,7 +428,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- data64 = data * 1562500ll;
+ data64 = MAX17042_CURRENT_RSENSE(data);
val->intval = div_s64(data64, chip->pdata->r_sns);
break;
case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
@@ -428,7 +436,7 @@ static int max17042_get_property(struct power_supply *psy,
if (ret < 0)
return ret;
- val->intval = data * 5625 / 1000;
+ val->intval = MAX17042_TIME(data);
break;
default:
return -EINVAL;
--
2.35.1
On 18/03/2022 10:00, Hans de Goede wrote:
> 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 <[email protected]>
>>> ---
>>> 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.
Not in max17042_get_status(). The usage there is without ll. Three other
places use it in 64-bit context (result is 64-bit), so there indeed. But
in max17042_get_status() this is now different.
Best regards,
Krzysztof
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 <[email protected]>
> ---
> 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.
> +#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
Best regards,
Krzysztof
Hi Krzysztof, hi Hans,
thanks for the review!
On piątek, 18 marca 2022 10:51:26 CET Hans de Goede wrote:
> Hi,
>
> On 3/18/22 10:06, Krzysztof Kozlowski wrote:
> > On 18/03/2022 10:00, Hans de Goede wrote:
> >> 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 <[email protected]>
> >>>> ---
> >>>>
> >>>> 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.
> >
> > Not in max17042_get_status(). The usage there is without ll. Three other
> > places use it in 64-bit context (result is 64-bit), so there indeed. But
> > in max17042_get_status() this is now different.
>
> Ah, good catch and there is a reason why it is not a ll there, a divide
> is done on it, which is now a 64 bit divide which will break on 32 bit
> builds...
>
> Note that e.g. this existing block:
>
> case POWER_SUPPLY_PROP_CURRENT_NOW:
> if (chip->pdata->enable_current_sense) {
> ret = regmap_read(map, MAX17042_Current, &data);
> if (ret < 0)
> return ret;
>
> data64 = sign_extend64(data, 15) * 1562500ll;
> val->intval = div_s64(data64, chip->pdata->r_sns);
> } else {
> return -EINVAL;
> }
> break;
>
> Solves this by using the div_s64 helper. So the code in
> max17042_get_status() needs to be fixed to do the same.
>
> The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
> fit in a 32 bit integer.
>
> So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
> a potential bug there and as such that really should be done in
> a separate preparation patch with a Cc stable.
>
> Regards,
>
> Hans
Yes, I've already noticed that max17042_get_status was broken, but it managed
to slip out of my mind before sending the series - although I haven't caught
that I'm introducing a yet another breakage there :) I've actually thought
about removing the unit conversion from this place whatsoever, because this
function only ever cares about the sign of what's in MAX17042_Current, so it
doesn't really need to do any division at all.
Best regards,
Sebastian
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 <[email protected]>
>> ---
>> 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
Hi,
On 3/18/22 21:06, Sebastian Krzyszkowiak wrote:
> Hi Krzysztof, hi Hans,
>
> thanks for the review!
>
> On piątek, 18 marca 2022 10:51:26 CET Hans de Goede wrote:
>> Hi,
>>
>> On 3/18/22 10:06, Krzysztof Kozlowski wrote:
>>> On 18/03/2022 10:00, Hans de Goede wrote:
>>>> 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 <[email protected]>
>>>>>> ---
>>>>>>
>>>>>> 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.
>>>
>>> Not in max17042_get_status(). The usage there is without ll. Three other
>>> places use it in 64-bit context (result is 64-bit), so there indeed. But
>>> in max17042_get_status() this is now different.
>>
>> Ah, good catch and there is a reason why it is not a ll there, a divide
>> is done on it, which is now a 64 bit divide which will break on 32 bit
>> builds...
>>
>> Note that e.g. this existing block:
>>
>> case POWER_SUPPLY_PROP_CURRENT_NOW:
>> if (chip->pdata->enable_current_sense) {
>> ret = regmap_read(map, MAX17042_Current, &data);
>> if (ret < 0)
>> return ret;
>>
>> data64 = sign_extend64(data, 15) * 1562500ll;
>> val->intval = div_s64(data64, chip->pdata->r_sns);
>> } else {
>> return -EINVAL;
>> }
>> break;
>>
>> Solves this by using the div_s64 helper. So the code in
>> max17042_get_status() needs to be fixed to do the same.
>>
>> The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
>> fit in a 32 bit integer.
>>
>> So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
>> a potential bug there and as such that really should be done in
>> a separate preparation patch with a Cc stable.
>>
>> Regards,
>>
>> Hans
>
> Yes, I've already noticed that max17042_get_status was broken, but it managed
> to slip out of my mind before sending the series - although I haven't caught
> that I'm introducing a yet another breakage there :) I've actually thought
> about removing the unit conversion from this place whatsoever, because this
> function only ever cares about the sign of what's in MAX17042_Current, so it
> doesn't really need to do any division at all.
Removing the value conversion (in a separate patch) would be a good
solution too.
Regards,
Hans
Hi,
On 3/18/22 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 <[email protected]>
> ---
> 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 */
> +#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 */
> +
> struct max17042_chip {
> struct i2c_client *client;
> struct regmap *regmap;
> @@ -103,8 +112,7 @@ static int max17042_get_temperature(struct max17042_chip *chip, int *temp)
>
> *temp = sign_extend32(data, 15);
> /* The value is converted into deci-centigrade scale */
> - /* Units of LSB = 1 / 256 degree Celsius */
> - *temp = *temp * 10 / 256;
> + *temp = MAX17042_TEMPERATURE(*temp * 10);
Shouldn't the "* 10" be part of the macro?
Otherwise this looks good to me:
Reviewed-by: Hans de Goede <[email protected]>
Regards,
Hans
> return 0;
> }
>
> @@ -161,7 +169,7 @@ static int max17042_get_status(struct max17042_chip *chip, int *status)
> return ret;
>
> avg_current = sign_extend32(data, 15);
> - avg_current *= 1562500 / chip->pdata->r_sns;
> + avg_current *= MAX17042_CURRENT_LSB / chip->pdata->r_sns;
>
> if (avg_current > 0)
> *status = POWER_SUPPLY_STATUS_CHARGING;
> @@ -181,7 +189,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
> goto health_error;
>
> /* bits [0-3] unused */
> - vavg = val * 625 / 8;
> + vavg = MAX17042_VOLTAGE(val);
> /* Convert to millivolts */
> vavg /= 1000;
>
> @@ -190,7 +198,7 @@ static int max17042_get_battery_health(struct max17042_chip *chip, int *health)
> goto health_error;
>
> /* bits [0-3] unused */
> - vbatt = val * 625 / 8;
> + vbatt = MAX17042_VOLTAGE(val);
> /* Convert to millivolts */
> vbatt /= 1000;
>
> @@ -297,21 +305,21 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - val->intval = data * 625 / 8;
> + val->intval = MAX17042_VOLTAGE(data);
> break;
> case POWER_SUPPLY_PROP_VOLTAGE_AVG:
> ret = regmap_read(map, MAX17042_AvgVCELL, &data);
> if (ret < 0)
> return ret;
>
> - val->intval = data * 625 / 8;
> + val->intval = MAX17042_VOLTAGE(data);
> break;
> case POWER_SUPPLY_PROP_VOLTAGE_OCV:
> ret = regmap_read(map, MAX17042_OCVInternal, &data);
> if (ret < 0)
> return ret;
>
> - val->intval = data * 625 / 8;
> + val->intval = MAX17042_VOLTAGE(data);
> break;
> case POWER_SUPPLY_PROP_CAPACITY:
> if (chip->pdata->enable_current_sense)
> @@ -328,7 +336,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = data * 5000000ll;
> + data64 = MAX17042_CAPACITY_RSENSE(data);
> do_div(data64, chip->pdata->r_sns);
> val->intval = data64;
> break;
> @@ -337,7 +345,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = data * 5000000ll;
> + data64 = MAX17042_CAPACITY_RSENSE(data);
> do_div(data64, chip->pdata->r_sns);
> val->intval = data64;
> break;
> @@ -346,7 +354,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = data * 5000000ll;
> + data64 = MAX17042_CAPACITY_RSENSE(data);
> do_div(data64, chip->pdata->r_sns);
> val->intval = data64;
> break;
> @@ -355,7 +363,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = sign_extend64(data, 15) * 5000000ll;
> + data64 = MAX17042_CAPACITY_RSENSE(sign_extend64(data, 15));
> val->intval = div_s64(data64, chip->pdata->r_sns);
> break;
> case POWER_SUPPLY_PROP_TEMP:
> @@ -397,7 +405,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = sign_extend64(data, 15) * 1562500ll;
> + data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
> val->intval = div_s64(data64, chip->pdata->r_sns);
> } else {
> return -EINVAL;
> @@ -409,7 +417,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = sign_extend64(data, 15) * 1562500ll;
> + data64 = MAX17042_CURRENT_RSENSE(sign_extend64(data, 15));
> val->intval = div_s64(data64, chip->pdata->r_sns);
> } else {
> return -EINVAL;
> @@ -420,7 +428,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - data64 = data * 1562500ll;
> + data64 = MAX17042_CURRENT_RSENSE(data);
> val->intval = div_s64(data64, chip->pdata->r_sns);
> break;
> case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
> @@ -428,7 +436,7 @@ static int max17042_get_property(struct power_supply *psy,
> if (ret < 0)
> return ret;
>
> - val->intval = data * 5625 / 1000;
> + val->intval = MAX17042_TIME(data);
> break;
> default:
> return -EINVAL;
Hi,
On 3/18/22 10:06, Krzysztof Kozlowski wrote:
> On 18/03/2022 10:00, Hans de Goede wrote:
>> 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 <[email protected]>
>>>> ---
>>>> 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.
>
> Not in max17042_get_status(). The usage there is without ll. Three other
> places use it in 64-bit context (result is 64-bit), so there indeed. But
> in max17042_get_status() this is now different.
Ah, good catch and there is a reason why it is not a ll there, a divide
is done on it, which is now a 64 bit divide which will break on 32 bit
builds...
Note that e.g. this existing block:
case POWER_SUPPLY_PROP_CURRENT_NOW:
if (chip->pdata->enable_current_sense) {
ret = regmap_read(map, MAX17042_Current, &data);
if (ret < 0)
return ret;
data64 = sign_extend64(data, 15) * 1562500ll;
val->intval = div_s64(data64, chip->pdata->r_sns);
} else {
return -EINVAL;
}
break;
Solves this by using the div_s64 helper. So the code in max17042_get_status()
needs to be fixed to do the same.
The "ll" is necessary because 32768 * 1562500 = 51200000000 which does not
fit in a 32 bit integer.
So fixing max17042_get_status() to use sign_extend64 + div_s64 fixes
a potential bug there and as such that really should be done in
a separate preparation patch with a Cc stable.
Regards,
Hans