So far configuring the gauge was only possible using platform data,
with no way to provide the configuration on device tree-based platforms.
Change that by looking up the configuration values from monitored-battery
property. This is especially useful on models implementing ModelGauge m5 EZ
algorithm, such as MAX17055, as all the required configuration can be
derived from a "simple-battery" DT node there.
In order to be able to access power supply framework in get_of_pdata,
move devm_power_supply_register earlier in max17042_probe.
Signed-off-by: Sebastian Krzyszkowiak <[email protected]>
---
drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
include/linux/power/max17042_battery.h | 1 +
2 files changed, 40 insertions(+), 11 deletions(-)
diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
index c39250349a1d..4c33565802d5 100644
--- a/drivers/power/supply/max17042_battery.c
+++ b/drivers/power/supply/max17042_battery.c
@@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
struct device *dev = &chip->client->dev;
struct device_node *np = dev->of_node;
u32 prop;
+ u64 data64;
struct max17042_platform_data *pdata;
+ struct power_supply_battery_info *info;
pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
if (!pdata)
@@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
pdata->vmax = INT_MAX;
+ if (pdata->enable_current_sense &&
+ power_supply_get_battery_info(chip->battery, &info) == 0) {
+ pdata->config_data = devm_kzalloc(dev, sizeof(*pdata->config_data), GFP_KERNEL);
+ if (!pdata->config_data)
+ return NULL;
+
+ if (info->charge_full_design_uah != -EINVAL) {
+ data64 = (u64)info->charge_full_design_uah * pdata->r_sns;
+ do_div(data64, MAX17042_CAPACITY_LSB);
+ pdata->config_data->design_cap = (u16)data64;
+ pdata->enable_por_init = true;
+ }
+ if (info->charge_term_current_ua != -EINVAL) {
+ data64 = (u64)info->charge_term_current_ua * pdata->r_sns;
+ do_div(data64, MAX17042_CURRENT_LSB);
+ pdata->config_data->ichgt_term = (u16)data64;
+ pdata->enable_por_init = true;
+ }
+ if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
+ if (info->voltage_max_design_uv > 4250000) {
+ pdata->config_data->model_cfg = MAX17055_MODELCFG_VCHG_BIT;
+ pdata->enable_por_init = true;
+ }
+ }
+ }
+
return pdata;
}
#endif
@@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client *client,
return -EINVAL;
}
+ i2c_set_clientdata(client, chip);
+ psy_cfg.drv_data = chip;
+ psy_cfg.of_node = dev->of_node;
+
+ chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
+ &psy_cfg);
+ if (IS_ERR(chip->battery)) {
+ dev_err(&client->dev, "failed: power supply register\n");
+ return PTR_ERR(chip->battery);
+ }
+
chip->pdata = max17042_get_pdata(chip);
if (!chip->pdata) {
dev_err(&client->dev, "no platform data provided\n");
return -EINVAL;
}
- i2c_set_clientdata(client, chip);
- psy_cfg.drv_data = chip;
- psy_cfg.of_node = dev->of_node;
-
/* When current is not measured,
* CURRENT_NOW and CURRENT_AVG properties should be invisible. */
if (!chip->pdata->enable_current_sense)
@@ -1122,13 +1157,6 @@ static int max17042_probe(struct i2c_client *client,
regmap_write(chip->regmap, MAX17042_LearnCFG, 0x0007);
}
- chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
- &psy_cfg);
- if (IS_ERR(chip->battery)) {
- dev_err(&client->dev, "failed: power supply register\n");
- return PTR_ERR(chip->battery);
- }
-
if (client->irq) {
unsigned int flags = IRQF_ONESHOT;
diff --git a/include/linux/power/max17042_battery.h b/include/linux/power/max17042_battery.h
index 6943921cab5e..367620800e7e 100644
--- a/include/linux/power/max17042_battery.h
+++ b/include/linux/power/max17042_battery.h
@@ -23,6 +23,7 @@
#define MAX17042_CHARACTERIZATION_DATA_SIZE 48
+#define MAX17055_MODELCFG_VCHG_BIT BIT(10)
#define MAX17055_MODELCFG_REFRESH_BIT BIT(15)
enum max17042_register {
--
2.35.1
On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> So far configuring the gauge was only possible using platform data,
> with no way to provide the configuration on device tree-based platforms.
>
> Change that by looking up the configuration values from monitored-battery
> property. This is especially useful on models implementing ModelGauge m5 EZ
> algorithm, such as MAX17055, as all the required configuration can be
> derived from a "simple-battery" DT node there.
>
> In order to be able to access power supply framework in get_of_pdata,
> move devm_power_supply_register earlier in max17042_probe.
>
> Signed-off-by: Sebastian Krzyszkowiak <[email protected]>
> ---
> drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
> include/linux/power/max17042_battery.h | 1 +
> 2 files changed, 40 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index c39250349a1d..4c33565802d5 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
> struct device *dev = &chip->client->dev;
> struct device_node *np = dev->of_node;
> u32 prop;
> + u64 data64;
> struct max17042_platform_data *pdata;
> + struct power_supply_battery_info *info;
>
> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> if (!pdata)
> @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
> if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
> pdata->vmax = INT_MAX;
>
> + if (pdata->enable_current_sense &&
> + power_supply_get_battery_info(chip->battery, &info) == 0) {
> + pdata->config_data = devm_kzalloc(dev, sizeof(*pdata->config_data), GFP_KERNEL);
> + if (!pdata->config_data)
> + return NULL;
> +
> + if (info->charge_full_design_uah != -EINVAL) {
> + data64 = (u64)info->charge_full_design_uah * pdata->r_sns;
> + do_div(data64, MAX17042_CAPACITY_LSB);
> + pdata->config_data->design_cap = (u16)data64;
> + pdata->enable_por_init = true;
> + }
> + if (info->charge_term_current_ua != -EINVAL) {
> + data64 = (u64)info->charge_term_current_ua * pdata->r_sns;
> + do_div(data64, MAX17042_CURRENT_LSB);
> + pdata->config_data->ichgt_term = (u16)data64;
> + pdata->enable_por_init = true;
> + }
> + if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> + if (info->voltage_max_design_uv > 4250000) {
> + pdata->config_data->model_cfg = MAX17055_MODELCFG_VCHG_BIT;
> + pdata->enable_por_init = true;
> + }
> + }
> + }
> +
> return pdata;
> }
> #endif
> @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client *client,
> return -EINVAL;
> }
>
> + i2c_set_clientdata(client, chip);
> + psy_cfg.drv_data = chip;
> + psy_cfg.of_node = dev->of_node;
> +
> + chip->battery = devm_power_supply_register(&client->dev, max17042_desc,
> + &psy_cfg);
> + if (IS_ERR(chip->battery)) {
> + dev_err(&client->dev, "failed: power supply register\n");
> + return PTR_ERR(chip->battery);
> + }
I don't think it is correct. You register power supply, thus making it
available for system, before configuring most of the data. For short
time the chip might report to the system bogus results and events.
Instead I think you should split it into two parts - init which happens
before registering power supply and after.
Best regards,
Krzysztof
On piątek, 18 marca 2022 09:40:36 CET Krzysztof Kozlowski wrote:
> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
> > So far configuring the gauge was only possible using platform data,
> > with no way to provide the configuration on device tree-based platforms.
> >
> > Change that by looking up the configuration values from monitored-battery
> > property. This is especially useful on models implementing ModelGauge m5
> > EZ
> > algorithm, such as MAX17055, as all the required configuration can be
> > derived from a "simple-battery" DT node there.
> >
> > In order to be able to access power supply framework in get_of_pdata,
> > move devm_power_supply_register earlier in max17042_probe.
> >
> > Signed-off-by: Sebastian Krzyszkowiak <[email protected]>
> > ---
> >
> > drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
> > include/linux/power/max17042_battery.h | 1 +
> > 2 files changed, 40 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/power/supply/max17042_battery.c
> > b/drivers/power/supply/max17042_battery.c index
> > c39250349a1d..4c33565802d5 100644
> > --- a/drivers/power/supply/max17042_battery.c
> > +++ b/drivers/power/supply/max17042_battery.c
> > @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
> >
> > struct device *dev = &chip->client->dev;
> > struct device_node *np = dev->of_node;
> > u32 prop;
> >
> > + u64 data64;
> >
> > struct max17042_platform_data *pdata;
> >
> > + struct power_supply_battery_info *info;
> >
> > pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
> > if (!pdata)
> >
> > @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
> >
> > if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
> >
> > pdata->vmax = INT_MAX;
> >
> > + if (pdata->enable_current_sense &&
> > + power_supply_get_battery_info(chip->battery, &info) == 0) {
> > + pdata->config_data = devm_kzalloc(dev, sizeof(*pdata-
>config_data),
> > GFP_KERNEL); + if (!pdata->config_data)
> > + return NULL;
> > +
> > + if (info->charge_full_design_uah != -EINVAL) {
> > + data64 = (u64)info->charge_full_design_uah *
pdata->r_sns;
> > + do_div(data64, MAX17042_CAPACITY_LSB);
> > + pdata->config_data->design_cap = (u16)data64;
> > + pdata->enable_por_init = true;
> > + }
> > + if (info->charge_term_current_ua != -EINVAL) {
> > + data64 = (u64)info->charge_term_current_ua *
pdata->r_sns;
> > + do_div(data64, MAX17042_CURRENT_LSB);
> > + pdata->config_data->ichgt_term = (u16)data64;
> > + pdata->enable_por_init = true;
> > + }
> > + if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
> > + if (info->voltage_max_design_uv > 4250000) {
> > + pdata->config_data->model_cfg =
MAX17055_MODELCFG_VCHG_BIT;
> > + pdata->enable_por_init = true;
> > + }
> > + }
> > + }
> > +
> >
> > return pdata;
> >
> > }
> > #endif
> >
> > @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client
> > *client,>
> > return -EINVAL;
> >
> > }
> >
> > + i2c_set_clientdata(client, chip);
> > + psy_cfg.drv_data = chip;
> > + psy_cfg.of_node = dev->of_node;
> > +
> > + chip->battery = devm_power_supply_register(&client->dev,
max17042_desc,
> > +
&psy_cfg);
> > + if (IS_ERR(chip->battery)) {
> > + dev_err(&client->dev, "failed: power supply
register\n");
> > + return PTR_ERR(chip->battery);
> > + }
>
> I don't think it is correct. You register power supply, thus making it
> available for system, before configuring most of the data. For short
> time the chip might report to the system bogus results and events.
>
> Instead I think you should split it into two parts - init which happens
> before registering power supply and after.
Simply splitting initialization into two parts won't really help. If you set
capacity, current, Vchg and refresh the model after registering power supply,
you will still end up having a short time window with bogus results. Looking
at other drivers, they seem to deal with it in the same way - they register
the power supply early, before the driver can fully configure the device.
To actually fix the problem with bogus data on init, it seems like we would
either need some support from the power supply framework to notify it when can
it actually start expecting correct data, or have a way to access the battery
information without having to register power supply beforehand.
Since power_supply_get_battery_info doesn't actually seem to depend on
power_supply device at all - it uses psy->dev for devm functions and psy-
>of_node to read the data from - I wonder if it could be split into a function
that only takes an of_node?
Cheers,
Sebastian
On 20/03/2022 22:24, Sebastian Krzyszkowiak wrote:
> On piątek, 18 marca 2022 09:40:36 CET Krzysztof Kozlowski wrote:
>> On 18/03/2022 01:10, Sebastian Krzyszkowiak wrote:
>>> So far configuring the gauge was only possible using platform data,
>>> with no way to provide the configuration on device tree-based platforms.
>>>
>>> Change that by looking up the configuration values from monitored-battery
>>> property. This is especially useful on models implementing ModelGauge m5
>>> EZ
>>> algorithm, such as MAX17055, as all the required configuration can be
>>> derived from a "simple-battery" DT node there.
>>>
>>> In order to be able to access power supply framework in get_of_pdata,
>>> move devm_power_supply_register earlier in max17042_probe.
>>>
>>> Signed-off-by: Sebastian Krzyszkowiak <[email protected]>
>>> ---
>>>
>>> drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------
>>> include/linux/power/max17042_battery.h | 1 +
>>> 2 files changed, 40 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/power/supply/max17042_battery.c
>>> b/drivers/power/supply/max17042_battery.c index
>>> c39250349a1d..4c33565802d5 100644
>>> --- a/drivers/power/supply/max17042_battery.c
>>> +++ b/drivers/power/supply/max17042_battery.c
>>> @@ -937,7 +937,9 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>>>
>>> struct device *dev = &chip->client->dev;
>>> struct device_node *np = dev->of_node;
>>> u32 prop;
>>>
>>> + u64 data64;
>>>
>>> struct max17042_platform_data *pdata;
>>>
>>> + struct power_supply_battery_info *info;
>>>
>>> pdata = devm_kzalloc(dev, sizeof(*pdata), GFP_KERNEL);
>>> if (!pdata)
>>>
>>> @@ -961,6 +963,32 @@ max17042_get_of_pdata(struct max17042_chip *chip)
>>>
>>> if (of_property_read_s32(np, "maxim,over-volt", &pdata->vmax))
>>>
>>> pdata->vmax = INT_MAX;
>>>
>>> + if (pdata->enable_current_sense &&
>>> + power_supply_get_battery_info(chip->battery, &info) == 0) {
>>> + pdata->config_data = devm_kzalloc(dev, sizeof(*pdata-
>> config_data),
>>> GFP_KERNEL); + if (!pdata->config_data)
>>> + return NULL;
>>> +
>>> + if (info->charge_full_design_uah != -EINVAL) {
>>> + data64 = (u64)info->charge_full_design_uah *
> pdata->r_sns;
>>> + do_div(data64, MAX17042_CAPACITY_LSB);
>>> + pdata->config_data->design_cap = (u16)data64;
>>> + pdata->enable_por_init = true;
>>> + }
>>> + if (info->charge_term_current_ua != -EINVAL) {
>>> + data64 = (u64)info->charge_term_current_ua *
> pdata->r_sns;
>>> + do_div(data64, MAX17042_CURRENT_LSB);
>>> + pdata->config_data->ichgt_term = (u16)data64;
>>> + pdata->enable_por_init = true;
>>> + }
>>> + if (chip->chip_type == MAXIM_DEVICE_TYPE_MAX17055) {
>>> + if (info->voltage_max_design_uv > 4250000) {
>>> + pdata->config_data->model_cfg =
> MAX17055_MODELCFG_VCHG_BIT;
>>> + pdata->enable_por_init = true;
>>> + }
>>> + }
>>> + }
>>> +
>>>
>>> return pdata;
>>>
>>> }
>>> #endif
>>>
>>> @@ -1092,16 +1120,23 @@ static int max17042_probe(struct i2c_client
>>> *client,>
>>> return -EINVAL;
>>>
>>> }
>>>
>>> + i2c_set_clientdata(client, chip);
>>> + psy_cfg.drv_data = chip;
>>> + psy_cfg.of_node = dev->of_node;
>>> +
>>> + chip->battery = devm_power_supply_register(&client->dev,
> max17042_desc,
>>> +
> &psy_cfg);
>>> + if (IS_ERR(chip->battery)) {
>>> + dev_err(&client->dev, "failed: power supply
> register\n");
>>> + return PTR_ERR(chip->battery);
>>> + }
>>
>> I don't think it is correct. You register power supply, thus making it
>> available for system, before configuring most of the data. For short
>> time the chip might report to the system bogus results and events.
>>
>> Instead I think you should split it into two parts - init which happens
>> before registering power supply and after.
>
> Simply splitting initialization into two parts won't really help. If you set
> capacity, current, Vchg and refresh the model after registering power supply,
> you will still end up having a short time window with bogus results. Looking
> at other drivers, they seem to deal with it in the same way - they register
> the power supply early, before the driver can fully configure the device.
>
> To actually fix the problem with bogus data on init, it seems like we would
> either need some support from the power supply framework to notify it when can
> it actually start expecting correct data, or have a way to access the battery
> information without having to register power supply beforehand.
Indeed I spotted similar pattern in other drivers, so this might be a
common issue.
>
> Since power_supply_get_battery_info doesn't actually seem to depend on
> power_supply device at all - it uses psy->dev for devm functions and psy-
> of_node to read the data from - I wonder if it could be split into a function
> that only takes an of_node?
That might be the best approach.
Best regards,
Krzysztof