| Message ID | 20220318001048.20922-5-sebastian.krzyszkowiak@puri.sm (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | MAX17055 model configuration via DT | expand |
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 <sebastian.krzyszkowiak@puri.sm> > --- > 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 <sebastian.krzyszkowiak@puri.sm> > > --- > > > > 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 <sebastian.krzyszkowiak@puri.sm> >>> --- >>> >>> 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
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 {
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 <sebastian.krzyszkowiak@puri.sm> --- drivers/power/supply/max17042_battery.c | 50 +++++++++++++++++++------ include/linux/power/max17042_battery.h | 1 + 2 files changed, 40 insertions(+), 11 deletions(-)