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 <[email protected]>
---
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
+ regmap_update_bits(map, MAX17055_ModelCfg,
+ MAX17055_MODELCFG_VCHG_BIT, config->model_cfg);
+ }
}
static int max17042_init_chip(struct max17042_chip *chip)
@@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip *chip)
int ret;
max17042_override_por_values(chip);
+
+ if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
+ regmap_write_bits(map, MAX17055_ModelCfg,
+ MAX17055_MODELCFG_REFRESH_BIT, MAX17055_MODELCFG_REFRESH_BIT);
+ }
+
/* After Power up, the MAX17042 requires 500mS in order
* to perform signal debouncing and initial SOC reporting
*/
msleep(500);
- /* Initialize configuration */
- max17042_write_config_regs(chip);
-
- /* write cell characterization data */
- ret = max17042_init_model(chip);
- if (ret) {
- dev_err(&chip->client->dev, "%s init failed\n",
- __func__);
- return -EIO;
- }
+ if ((chip->chip_type == MAXIM_DEVICE_TYPE_MAX17042) ||
+ (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17047) ||
+ (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17050)) {
+ /* Initialize configuration */
+ max17042_write_config_regs(chip);
+
+ /* write cell characterization data */
+ ret = max17042_init_model(chip);
+ if (ret) {
+ dev_err(&chip->client->dev, "%s init failed\n",
+ __func__);
+ return -EIO;
+ }
- ret = max17042_verify_model_lock(chip);
- if (ret) {
- dev_err(&chip->client->dev, "%s lock verify failed\n",
- __func__);
- return -EIO;
- }
- /* write custom parameters */
- max17042_write_custom_regs(chip);
+ ret = max17042_verify_model_lock(chip);
+ if (ret) {
+ dev_err(&chip->client->dev, "%s lock verify failed\n",
+ __func__);
+ return -EIO;
+ }
+ /* write custom parameters */
+ max17042_write_custom_regs(chip);
- /* update capacity params */
- max17042_update_capacity_regs(chip);
+ /* update capacity params */
+ max17042_update_capacity_regs(chip);
- /* delay must be atleast 350mS to allow VFSOC
- * to be calculated from the new configuration
- */
- msleep(350);
+ /* delay must be at least 350mS to allow VFSOC
+ * to be calculated from the new configuration
+ */
+ msleep(350);
- /* reset vfsoc0 reg */
- max17042_reset_vfsoc0_reg(chip);
+ /* reset vfsoc0 reg */
+ max17042_reset_vfsoc0_reg(chip);
- /* load new capacity params */
- max17042_load_new_capacity_params(chip);
+ /* load new capacity params */
+ max17042_load_new_capacity_params(chip);
+ }
/* Init complete, Clear the POR bit */
regmap_update_bits(map, MAX17042_STATUS, STATUS_POR_BIT, 0x0);
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index c417abd2ab70..6943921cab5e 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -23,6 +23,8 @@
#define MAX17042_CHARACTERIZATION_DATA_SIZE 48
+#define MAX17055_MODELCFG_REFRESH_BIT BIT(15)
+
enum max17042_register {
MAX17042_STATUS = 0x00,
MAX17042_VALRT_Th = 0x01,
@@ -208,6 +210,7 @@ struct max17042_config_data {
u16 full_soc_thresh; /* 0x13 */
u16 design_cap; /* 0x18 */
u16 ichgt_term; /* 0x1E */
+ u16 model_cfg; /* 0xDB */
/* MG3 config */
u16 at_rate; /* 0x04 */
--
2.35.1
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 <[email protected]>
> ---
> 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?
> + regmap_update_bits(map, MAX17055_ModelCfg,
> + MAX17055_MODELCFG_VCHG_BIT, config->model_cfg);
Can you align the continued line with previous line? Same in other
places if it is not aligned.
> + }
> }
>
> static int max17042_init_chip(struct max17042_chip *chip)
> @@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip *chip)
> int ret;
>
Best regards,
Krzysztof
Hi,
On 3/18/22 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 <[email protected]>
> ---
> 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
> + regmap_update_bits(map, MAX17055_ModelCfg,
> + MAX17055_MODELCFG_VCHG_BIT, config->model_cfg);
> + }
> }
>
> static int max17042_init_chip(struct max17042_chip *chip)
> @@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip *chip)
> int ret;
>
> max17042_override_por_values(chip);
> +
> + if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> + regmap_write_bits(map, MAX17055_ModelCfg,
> + MAX17055_MODELCFG_REFRESH_BIT, MAX17055_MODELCFG_REFRESH_BIT);
> + }
> +
This can be folded into the if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {}
which you add to max17042_override_por_values() above.
> /* After Power up, the MAX17042 requires 500mS in order
> * to perform signal debouncing and initial SOC reporting
> */
> msleep(500);
>
> - /* Initialize configuration */
> - max17042_write_config_regs(chip);
> -
> - /* write cell characterization data */
> - ret = max17042_init_model(chip);
> - if (ret) {
> - dev_err(&chip->client->dev, "%s init failed\n",
> - __func__);
> - return -EIO;
> - }
> + if ((chip->chip_type == MAXIM_DEVICE_TYPE_MAX17042) ||
> + (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17047) ||
> + (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17050)) {
> + /* Initialize configuration */
> + max17042_write_config_regs(chip);
> +
> + /* write cell characterization data */
> + ret = max17042_init_model(chip);
> + if (ret) {
> + dev_err(&chip->client->dev, "%s init failed\n",
> + __func__);
> + return -EIO;
> + }
>
> - ret = max17042_verify_model_lock(chip);
> - if (ret) {
> - dev_err(&chip->client->dev, "%s lock verify failed\n",
> - __func__);
> - return -EIO;
> - }
> - /* write custom parameters */
> - max17042_write_custom_regs(chip);
> + ret = max17042_verify_model_lock(chip);
> + if (ret) {
> + dev_err(&chip->client->dev, "%s lock verify failed\n",
> + __func__);
> + return -EIO;
> + }
> + /* write custom parameters */
> + max17042_write_custom_regs(chip);
>
> - /* update capacity params */
> - max17042_update_capacity_regs(chip);
> + /* update capacity params */
> + max17042_update_capacity_regs(chip);
>
> - /* delay must be atleast 350mS to allow VFSOC
> - * to be calculated from the new configuration
> - */
> - msleep(350);
> + /* delay must be at least 350mS to allow VFSOC
> + * to be calculated from the new configuration
> + */
> + msleep(350);
>
> - /* reset vfsoc0 reg */
> - max17042_reset_vfsoc0_reg(chip);
> + /* reset vfsoc0 reg */
> + max17042_reset_vfsoc0_reg(chip);
>
> - /* load new capacity params */
> - max17042_load_new_capacity_params(chip);
> + /* load new capacity params */
> + max17042_load_new_capacity_params(chip);
> + }
>
> /* Init complete, Clear the POR bit */
> regmap_update_bits(map, MAX17042_STATUS, STATUS_POR_BIT, 0x0);
> diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
> index c417abd2ab70..6943921cab5e 100644
> --- a/include/linux/power/max17042_battery.h
> +++ b/include/linux/power/max17042_battery.h
> @@ -23,6 +23,8 @@
>
> #define MAX17042_CHARACTERIZATION_DATA_SIZE 48
>
> +#define MAX17055_MODELCFG_REFRESH_BIT BIT(15)
> +
> enum max17042_register {
> MAX17042_STATUS = 0x00,
> MAX17042_VALRT_Th = 0x01,
> @@ -208,6 +210,7 @@ struct max17042_config_data {
> u16 full_soc_thresh; /* 0x13 */
> u16 design_cap; /* 0x18 */
> u16 ichgt_term; /* 0x1E */
> + u16 model_cfg; /* 0xDB */
>
> /* MG3 config */
> u16 at_rate; /* 0x04 */
Regards,
Hans
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 <[email protected]>
>>> ---
>>>
>>> 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?
Best regards,
Krzysztof
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 <[email protected]>
> > ---
> >
> > 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.
> > + regmap_update_bits(map, MAX17055_ModelCfg,
> > + MAX17055_MODELCFG_VCHG_BIT,
config->model_cfg);
>
> Can you align the continued line with previous line? Same in other
> places if it is not aligned.
>
> > + }
> >
> > }
> >
> > static int max17042_init_chip(struct max17042_chip *chip)
> >
> > @@ -814,44 +821,54 @@ static int max17042_init_chip(struct max17042_chip
> > *chip)>
> > int ret;
>
> Best regards,
> Krzysztof
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 <[email protected]>
> >>> ---
> >>>
> >>> 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.
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.
Regards,
Sebastian
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 <[email protected]>
>>>>> ---
>>>>>
>>>>> 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