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

[Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>]

[-- Attachment #1: Type: text/plain, Size: 3509 bytes --]

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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]