Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <fa8ed8cb-9def-dc1f-b8be-d238569510be@redhat.com>
Date:   Sat, 19 Mar 2022 14:47:06 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.4.0
Subject: Re: [PATCH 1/4] power: supply: max17042_battery: Add unit conversion
 macros
Content-Language: en-US
To:     Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>,
        Krzysztof Kozlowski <krzk@kernel.org>,
        Marek Szyprowski <m.szyprowski@samsung.com>,
        Sebastian Reichel <sre@kernel.org>, linux-pm@vger.kernel.org
Cc:     Purism Kernel Team <kernel@puri.sm>, Rob Herring <robh@kernel.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org
References: <20220318001048.20922-1-sebastian.krzyszkowiak@puri.sm>
 <f15c415a-ad08-ae4f-c79c-574964ab9cb0@kernel.org>
 <1c4a7088-bcef-ca5c-ff3e-efd1049dc402@redhat.com>
 <7342538.iIbC2pHGDl@pliszka>
From:   Hans de Goede <hdegoede@redhat.com>
In-Reply-To: <7342538.iIbC2pHGDl@pliszka>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

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

Removing the value conversion (in a separate patch) would be a good
solution too.

Regards,

Hans