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
