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: <e6d014ca-4568-20f2-0254-e8fe51f30e5d@kernel.org>
Date:   Wed, 23 Mar 2022 10:47:04 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
From:   Krzysztof Kozlowski <krzk@kernel.org>
Subject: Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh
 on max17055
Content-Language: en-US
To:     Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>,
        Hans de Goede <hdegoede@redhat.com>,
        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>
 <7080597.aeNJFYEL58@pliszka>
 <5d43031e-382d-b12c-bba2-0e630fbec1ad@kernel.org>
 <2957015.e9J7NaK4W3@pliszka>
In-Reply-To: <2957015.e9J7NaK4W3@pliszka>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 20/03/2022 21:44, Sebastian Krzyszkowiak wrote:
> On niedziela, 20 marca 2022 13:18:49 CET Krzysztof Kozlowski wrote:
>> On 18/03/2022 20:58, Sebastian Krzyszkowiak wrote:
>>> On piątek, 18 marca 2022 09:22:16 CET Krzysztof Kozlowski wrote:
>>>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>>>> Unlike other models, max17055 doesn't require cell characterization
>>>>> data and operates on smaller amount of input variables (DesignCap,
>>>>> VEmpty, IChgTerm and ModelCfg). Input data can already be filled in
>>>>> by max17042_override_por_values, however model refresh bit has to be
>>>>> set after adjusting input variables in order to make them apply.
>>>>>
>>>>> Signed-off-by: Sebastian Krzyszkowiak <sebastian.krzyszkowiak@puri.sm>
>>>>> ---
>>>>>
>>>>>  drivers/power/supply/max17042_battery.c | 73 +++++++++++++++----------
>>>>>  include/linux/power/max17042_battery.h  |  3 +
>>>>>  2 files changed, 48 insertions(+), 28 deletions(-)
>>>>>
>>>>> diff --git a/drivers/power/supply/max17042_battery.c
>>>>> b/drivers/power/supply/max17042_battery.c index
>>>>> c019d6c52363..c39250349a1d 100644
>>>>> --- a/drivers/power/supply/max17042_battery.c
>>>>> +++ b/drivers/power/supply/max17042_battery.c
>>>>> @@ -806,6 +806,13 @@ static inline void
>>>>> max17042_override_por_values(struct max17042_chip *chip)>
>>>>>
>>>>>  	    (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055)) {
>>>>>  		
>>>>>  		max17042_override_por(map, MAX17047_V_empty, config-
>>>>
>>>> vempty);
>>>>
>>>>>  	}
>>>>>
>>>>> +
>>>>> +	if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
>>>>> +		max17042_override_por(map, MAX17055_ModelCfg, config-
>>>>
>>>> model_cfg);
>>>>
>>>>> +		// VChg is 1 by default, so allow it to be set to 0
>>>>
>>>> Consistent comment, so /* */
>>>>
>>>> I actually do not understand fully the comment and the code. You write
>>>> entire model_cfg to MAX17055_ModelCfg and then immediately do it again,
>>>> but with smaller mask. Why?
>>>
>>> That's because VChg is 1 on POR, and max17042_override_por doesn't do
>>> anything when value equals 0 - which means that if the whole
>>> config->model_cfg is 0, VChg won't get unset (which is needed for 4.2V
>>> batteries).
>>>
>>> This could actually be replaced with a single regmap_write.
>>
>> I got it now. But if config->model_cfg is 0, should VChg be unset?
> 
> That's a good question.
> 
> max17042_override_por doesn't override the register value when the given value 
> equals zero in order to not override POR defaults with unset platform data. 
> This way one can set only the registers that they want to change in `config` 
> and the rest are untouched. This, however, only works if we assume that zero 
> means "don't touch", which isn't the case for ModelCfg.
> 
> On the Librem 5, we need to unset VChg bit because our battery is only being 
> charged up to 4.2V. Allowing to unset this bit only without having to touch 
> the rest of the register was the motivation behind the current version of this 
> patch, however, thinking about it now I can see that it fails to do that in 
> the opposite case - when the DT contains a simple-battery with maximum voltage 
> higher than 4.25V, VChg will be set in config->model_cfg causing the whole 
> register to be overwritten.

This is actually nice description which could be put into a comment there.

> 
> So, I see two possible solutions:
> 
> 1) move VChg handling to a separate variable in struct max17042_config_data. 
> This way model_cfg can stay zero when there's no need to touch the rest of the 
> register. This minimizes changes over current code.
> 
> 2) remove max17042_override_por_values in its current shape altogether and 
> make it only deal with the values that are actually being set by the driver 
> (and only extend it in the future as it gains more ability). Currently most of 
> this function is only usable with platform data - is there actually any user 
> of max17042 that would need to configure the gauge without DT in the mainline 
> kernel? My quick search didn't find any. Do we need or want to keep platform 
> data support at all?
> 
> I'm leaning towards option 2, as it seems to me that currently this driver is 
> being cluttered quite a lot by what's essentially a dead code. Adding new 
> parameters to read from DT for POR initialization (which is necessary for 
> other models than MAX17055) should be rather easy, but trying to fit them into 
> current platform_data-oriented code may be not.

I am in for removal of platform data.

Best regards,
Krzysztof