Re: [PATCH 2/4] power: supply: max17042_battery: use ModelCfg refresh on max17055

[Krzysztof Kozlowski]

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@xxxxxxx>
>>>>> ---
>>>>>
>>>>>  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