| Message ID | 20250530-qcom_battmgr_update-v2-5-9e377193a656@oss.qualcomm.com |
|---|---|
| State | New |
| Headers | show |
| Series | power: supply: Add several features support in qcom-battmgr driver | expand |
On 30/05/2025 08:35, Fenglin Wu via B4 Relay wrote: > From: Fenglin Wu <fenglin.wu@oss.qualcomm.com> > > Add charge control support for SM8550 and X1E80100. It's supported > with below two power supply properties: > > charge_control_end_threshold: SOC threshold at which the charging > should be terminated. > > charge_control_start_threshold: SOC threshold at which the charging > should be resumed. Maybe this is very obvious to battery charger experts but what does SOC mean here ? Reading your patch you pass a "int soc" and compare it to a threshold value, without 'soc' having an obvious meaning. Its a threshold right ? Why not just call it threshold ? > > Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com> > --- > drivers/power/supply/qcom_battmgr.c | 256 ++++++++++++++++++++++++++++++++++-- > 1 file changed, 248 insertions(+), 8 deletions(-) > > diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c > index d5d0200b92bdc3d9a22f44159ad45b152efe8be0..39009415f4bfcd76b010305179d3c8fb847254a6 100644 > --- a/drivers/power/supply/qcom_battmgr.c > +++ b/drivers/power/supply/qcom_battmgr.c > @@ -21,6 +21,8 @@ > enum qcom_battmgr_variant { > QCOM_BATTMGR_SM8350, > QCOM_BATTMGR_SC8280XP, > + QCOM_BATTMGR_X1E80100, > + QCOM_BATTMGR_SM8550, You're breaking ordering here. Well the ordering is already broken but, please sort this list alphanumerically. > }; > > #define BATTMGR_BAT_STATUS 0x1 > @@ -66,6 +68,9 @@ enum qcom_battmgr_variant { > #define BATT_RESISTANCE 21 > #define BATT_POWER_NOW 22 > #define BATT_POWER_AVG 23 > +#define BATT_CHG_CTRL_EN 24 > +#define BATT_CHG_CTRL_START_THR 25 > +#define BATT_CHG_CTRL_END_THR 26 > > #define BATTMGR_USB_PROPERTY_GET 0x32 > #define BATTMGR_USB_PROPERTY_SET 0x33 > @@ -90,6 +95,13 @@ enum qcom_battmgr_variant { > #define WLS_TYPE 5 > #define WLS_BOOST_EN 6 > > +#define BATTMGR_CHG_CTRL_LIMIT_EN 0x48 > +#define CHARGE_CTRL_START_THR_MIN 50 > +#define CHARGE_CTRL_START_THR_MAX 95 > +#define CHARGE_CTRL_END_THR_MIN 55 > +#define CHARGE_CTRL_END_THR_MAX 100 > +#define CHARGE_CTRL_DELTA_SOC 5 > + > struct qcom_battmgr_enable_request { > struct pmic_glink_hdr hdr; > __le32 battery_id; > @@ -124,6 +136,13 @@ struct qcom_battmgr_discharge_time_request { > __le32 reserved; > }; > > +struct qcom_battmgr_charge_ctrl_request { > + struct pmic_glink_hdr hdr; > + __le32 enable; > + __le32 target_soc; > + __le32 delta_soc; > +}; > + > struct qcom_battmgr_message { > struct pmic_glink_hdr hdr; > union { > @@ -236,6 +255,8 @@ struct qcom_battmgr_info { > unsigned int capacity_warning; > unsigned int cycle_count; > unsigned int charge_count; > + unsigned int charge_ctrl_start; > + unsigned int charge_ctrl_end; > char model_number[BATTMGR_STRING_LEN]; > char serial_number[BATTMGR_STRING_LEN]; > char oem_info[BATTMGR_STRING_LEN]; > @@ -424,6 +445,8 @@ static const u8 sm8350_bat_prop_map[] = { > [POWER_SUPPLY_PROP_RESISTANCE] = BATT_RESISTANCE, > [POWER_SUPPLY_PROP_STATE_OF_HEALTH] = BATT_SOH, > [POWER_SUPPLY_PROP_POWER_NOW] = BATT_POWER_NOW, > + [POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD] = BATT_CHG_CTRL_START_THR, > + [POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD] = BATT_CHG_CTRL_END_THR, > }; > > static int qcom_battmgr_bat_sm8350_update(struct qcom_battmgr *battmgr, > @@ -494,7 +517,8 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, > if (!battmgr->service_up) > return -EAGAIN; > > - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) > + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || > + battmgr->variant == QCOM_BATTMGR_X1E80100) Please run your series through checkpatch 0004-power-supply-qcom_battmgr-Add-state_of_health-proper.patch has no obvious style problems and is ready for submission. CHECK: Alignment should match open parenthesis #95: FILE: drivers/power/supply/qcom_battmgr.c:521: + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) CHECK: Alignment should match open parenthesis #137: FILE: drivers/power/supply/qcom_battmgr.c:678: + if (soc < CHARGE_CTRL_START_THR_MIN || + soc > CHARGE_CTRL_START_THR_MAX) { CHECK: Alignment should match open parenthesis #139: FILE: drivers/power/supply/qcom_battmgr.c:680: + dev_err(battmgr->dev, "charge control start threshold exceed range: [%u - %u]\n", + CHARGE_CTRL_START_THR_MIN, CHARGE_CTRL_START_THR_MAX); CHECK: Alignment should match open parenthesis #175: FILE: drivers/power/supply/qcom_battmgr.c:716: + if (soc < CHARGE_CTRL_END_THR_MIN || + soc > CHARGE_CTRL_END_THR_MAX) { CHECK: Alignment should match open parenthesis #177: FILE: drivers/power/supply/qcom_battmgr.c:718: + dev_err(battmgr->dev, "charge control end threshold exceed range: [%u - %u]\n", + CHARGE_CTRL_END_THR_MIN, CHARGE_CTRL_END_THR_MAX); CHECK: Alignment should match open parenthesis #196: FILE: drivers/power/supply/qcom_battmgr.c:737: +static int qcom_battmgr_bat_is_writeable(struct power_supply *psy, + enum power_supply_property psp) CHECK: Alignment should match open parenthesis #210: FILE: drivers/power/supply/qcom_battmgr.c:751: +static int qcom_battmgr_bat_set_property(struct power_supply *psy, + enum power_supply_property psp, CHECK: Alignment should match open parenthesis #326: FILE: drivers/power/supply/qcom_battmgr.c:985: + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) CHECK: Alignment should match open parenthesis #336: FILE: drivers/power/supply/qcom_battmgr.c:1108: + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) CHECK: Alignment should match open parenthesis #367: FILE: drivers/power/supply/qcom_battmgr.c:1527: + else if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) CHECK: Alignment should match open parenthesis #396: FILE: drivers/power/supply/qcom_battmgr.c:1606: + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) { > ret = qcom_battmgr_bat_sc8280xp_update(battmgr, psp); > else > ret = qcom_battmgr_bat_sm8350_update(battmgr, psp); > @@ -599,6 +623,12 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, > case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG: > val->intval = battmgr->status.charge_time; > break; > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD: > + val->intval = battmgr->info.charge_ctrl_start; > + break; > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > + val->intval = battmgr->info.charge_ctrl_end; > + break; > case POWER_SUPPLY_PROP_MANUFACTURE_YEAR: > val->intval = battmgr->info.year; > break; > @@ -624,6 +654,120 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, > return 0; > } > > +static int qcom_battmgr_set_charge_control(struct qcom_battmgr *battmgr, > + u32 target_soc, u32 delta_soc) > +{ > + struct qcom_battmgr_charge_ctrl_request request = { > + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_BATTMGR), > + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP), > + .hdr.opcode = cpu_to_le32(BATTMGR_CHG_CTRL_LIMIT_EN), > + .enable = cpu_to_le32(1), > + .target_soc = cpu_to_le32(target_soc), > + .delta_soc = cpu_to_le32(delta_soc), > + }; > + > + return qcom_battmgr_request(battmgr, &request, sizeof(request)); > +} > + > +static int qcom_battmgr_set_charge_start_threshold(struct qcom_battmgr *battmgr, int soc) > +{ > + u32 target_soc, delta_soc; > + int ret; > + > + if (soc < CHARGE_CTRL_START_THR_MIN || > + soc > CHARGE_CTRL_START_THR_MAX) { > + dev_err(battmgr->dev, "charge control start threshold exceed range: [%u - %u]\n", > + CHARGE_CTRL_START_THR_MIN, CHARGE_CTRL_START_THR_MAX); > + return -EINVAL; > + } 'soc' is what - a threshold as far as I can tell. > + > + /* > + * If the new start threshold is larger than the old end threshold, > + * move the end threshold one step (DELTA_SOC) after the new start > + * threshold. > + */ > + if (soc > battmgr->info.charge_ctrl_end) { > + target_soc = soc + CHARGE_CTRL_DELTA_SOC; > + target_soc = min_t(u32, target_soc, CHARGE_CTRL_END_THR_MAX); > + delta_soc = target_soc - soc; > + delta_soc = min_t(u32, delta_soc, CHARGE_CTRL_DELTA_SOC); > + } else { > + target_soc = battmgr->info.charge_ctrl_end; > + delta_soc = battmgr->info.charge_ctrl_end - soc; > + } > + > + mutex_lock(&battmgr->lock); > + ret = qcom_battmgr_set_charge_control(battmgr, target_soc, delta_soc); > + mutex_unlock(&battmgr->lock); > + if (!ret) { > + battmgr->info.charge_ctrl_start = soc; > + battmgr->info.charge_ctrl_end = target_soc; > + } > + > + return 0; > +} > + > +static int qcom_battmgr_set_charge_end_threshold(struct qcom_battmgr *battmgr, int soc) > +{ > + u32 delta_soc = CHARGE_CTRL_DELTA_SOC; > + int ret; > + > + if (soc < CHARGE_CTRL_END_THR_MIN || > + soc > CHARGE_CTRL_END_THR_MAX) { > + dev_err(battmgr->dev, "charge control end threshold exceed range: [%u - %u]\n", > + CHARGE_CTRL_END_THR_MIN, CHARGE_CTRL_END_THR_MAX); > + return -EINVAL; > + } > + > + if (battmgr->info.charge_ctrl_start && soc > battmgr->info.charge_ctrl_start) > + delta_soc = soc - battmgr->info.charge_ctrl_start; > + > + mutex_lock(&battmgr->lock); > + ret = qcom_battmgr_set_charge_control(battmgr, soc, delta_soc); > + mutex_unlock(&battmgr->lock); > + if (!ret) { > + battmgr->info.charge_ctrl_start = soc - delta_soc; > + battmgr->info.charge_ctrl_end = soc; > + } > + > + return 0; > +} > + > +static int qcom_battmgr_bat_is_writeable(struct power_supply *psy, > + enum power_supply_property psp) > +{ > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD: > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > + return 1; > + default: > + return 0; > + } > + > + return 0; > +} > + > +static int qcom_battmgr_bat_set_property(struct power_supply *psy, > + enum power_supply_property psp, > + const union power_supply_propval *pval) > +{ > + struct qcom_battmgr *battmgr = power_supply_get_drvdata(psy); > + > + if (!battmgr->service_up) > + return -EAGAIN; > + > + switch (psp) { > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD: > + return qcom_battmgr_set_charge_start_threshold(battmgr, pval->intval); > + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: > + return qcom_battmgr_set_charge_end_threshold(battmgr, pval->intval); > + default: > + return -EINVAL; > + } > + > + return 0; > +} > + > static const enum power_supply_property sc8280xp_bat_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_PRESENT, > @@ -657,6 +801,43 @@ static const struct power_supply_desc sc8280xp_bat_psy_desc = { > .get_property = qcom_battmgr_bat_get_property, > }; > > +static const enum power_supply_property x1e80100_bat_props[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_CYCLE_COUNT, > + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_POWER_NOW, > + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_FULL, > + POWER_SUPPLY_PROP_CHARGE_EMPTY, > + POWER_SUPPLY_PROP_CHARGE_NOW, > + POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN, > + POWER_SUPPLY_PROP_ENERGY_FULL, > + POWER_SUPPLY_PROP_ENERGY_EMPTY, > + POWER_SUPPLY_PROP_ENERGY_NOW, > + POWER_SUPPLY_PROP_TEMP, > + POWER_SUPPLY_PROP_MANUFACTURE_YEAR, > + POWER_SUPPLY_PROP_MANUFACTURE_MONTH, > + POWER_SUPPLY_PROP_MANUFACTURE_DAY, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_MANUFACTURER, > + POWER_SUPPLY_PROP_SERIAL_NUMBER, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, > +}; > + > +static const struct power_supply_desc x1e80100_bat_psy_desc = { > + .name = "qcom-battmgr-bat", > + .type = POWER_SUPPLY_TYPE_BATTERY, > + .properties = x1e80100_bat_props, > + .num_properties = ARRAY_SIZE(x1e80100_bat_props), > + .get_property = qcom_battmgr_bat_get_property, > + .set_property = qcom_battmgr_bat_set_property, > + .property_is_writeable = qcom_battmgr_bat_is_writeable, > +}; > + > static const enum power_supply_property sm8350_bat_props[] = { > POWER_SUPPLY_PROP_STATUS, > POWER_SUPPLY_PROP_HEALTH, > @@ -689,6 +870,42 @@ static const struct power_supply_desc sm8350_bat_psy_desc = { > .get_property = qcom_battmgr_bat_get_property, > }; > > +static const enum power_supply_property sm8550_bat_props[] = { > + POWER_SUPPLY_PROP_STATUS, > + POWER_SUPPLY_PROP_HEALTH, > + POWER_SUPPLY_PROP_PRESENT, > + POWER_SUPPLY_PROP_CHARGE_TYPE, > + POWER_SUPPLY_PROP_CAPACITY, > + POWER_SUPPLY_PROP_VOLTAGE_OCV, > + POWER_SUPPLY_PROP_VOLTAGE_NOW, > + POWER_SUPPLY_PROP_VOLTAGE_MAX, > + POWER_SUPPLY_PROP_CURRENT_NOW, > + POWER_SUPPLY_PROP_TEMP, > + POWER_SUPPLY_PROP_TECHNOLOGY, > + POWER_SUPPLY_PROP_CHARGE_COUNTER, > + POWER_SUPPLY_PROP_CYCLE_COUNT, > + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, > + POWER_SUPPLY_PROP_CHARGE_FULL, > + POWER_SUPPLY_PROP_MODEL_NAME, > + POWER_SUPPLY_PROP_TIME_TO_FULL_AVG, > + POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG, > + POWER_SUPPLY_PROP_RESISTANCE, > + POWER_SUPPLY_PROP_STATE_OF_HEALTH, > + POWER_SUPPLY_PROP_POWER_NOW, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, > + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, > +}; > + > +static const struct power_supply_desc sm8550_bat_psy_desc = { > + .name = "qcom-battmgr-bat", > + .type = POWER_SUPPLY_TYPE_BATTERY, > + .properties = sm8550_bat_props, > + .num_properties = ARRAY_SIZE(sm8550_bat_props), > + .get_property = qcom_battmgr_bat_get_property, > + .set_property = qcom_battmgr_bat_set_property, > + .property_is_writeable = qcom_battmgr_bat_is_writeable, > +}; > + > static int qcom_battmgr_ac_get_property(struct power_supply *psy, > enum power_supply_property psp, > union power_supply_propval *val) > @@ -764,7 +981,8 @@ static int qcom_battmgr_usb_get_property(struct power_supply *psy, > if (!battmgr->service_up) > return -EAGAIN; > > - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) > + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || > + battmgr->variant == QCOM_BATTMGR_X1E80100) > ret = qcom_battmgr_bat_sc8280xp_update(battmgr, psp); > else > ret = qcom_battmgr_usb_sm8350_update(battmgr, psp); > @@ -886,7 +1104,8 @@ static int qcom_battmgr_wls_get_property(struct power_supply *psy, > if (!battmgr->service_up) > return -EAGAIN; > > - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) > + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || > + battmgr->variant == QCOM_BATTMGR_X1E80100) > ret = qcom_battmgr_bat_sc8280xp_update(battmgr, psp); > else > ret = qcom_battmgr_wls_sm8350_update(battmgr, psp); > @@ -1196,6 +1415,12 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr, > case BATT_POWER_NOW: > battmgr->status.power_now = le32_to_cpu(resp->intval.value); > break; > + case BATT_CHG_CTRL_START_THR: > + battmgr->info.charge_ctrl_start = le32_to_cpu(resp->intval.value); > + break; > + case BATT_CHG_CTRL_END_THR: > + battmgr->info.charge_ctrl_end = le32_to_cpu(resp->intval.value); > + break; > default: > dev_warn(battmgr->dev, "unknown property %#x\n", property); > break; > @@ -1278,6 +1503,7 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr, > } > break; > case BATTMGR_REQUEST_NOTIFICATION: > + case BATTMGR_CHG_CTRL_LIMIT_EN: > battmgr->error = 0; > break; > default: > @@ -1297,7 +1523,8 @@ static void qcom_battmgr_callback(const void *data, size_t len, void *priv) > > if (opcode == BATTMGR_NOTIFICATION) > qcom_battmgr_notification(battmgr, data, len); > - else if (battmgr->variant == QCOM_BATTMGR_SC8280XP) > + else if (battmgr->variant == QCOM_BATTMGR_SC8280XP || > + battmgr->variant == QCOM_BATTMGR_X1E80100) > qcom_battmgr_sc8280xp_callback(battmgr, data, len); > else > qcom_battmgr_sm8350_callback(battmgr, data, len); > @@ -1333,7 +1560,8 @@ static void qcom_battmgr_pdr_notify(void *priv, int state) > static const struct of_device_id qcom_battmgr_of_variants[] = { > { .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)QCOM_BATTMGR_SC8280XP }, > { .compatible = "qcom,sc8280xp-pmic-glink", .data = (void *)QCOM_BATTMGR_SC8280XP }, > - { .compatible = "qcom,x1e80100-pmic-glink", .data = (void *)QCOM_BATTMGR_SC8280XP }, > + { .compatible = "qcom,x1e80100-pmic-glink", .data = (void *)QCOM_BATTMGR_X1E80100 }, > + { .compatible = "qcom,sm8550-pmic-glink", .data = (void *)QCOM_BATTMGR_SM8550 }, Please separate compat string addition from functional changes. > /* Unmatched devices falls back to QCOM_BATTMGR_SM8350 */ > {} > }; > @@ -1343,6 +1571,7 @@ static char *qcom_battmgr_battery[] = { "battery" }; > static int qcom_battmgr_probe(struct auxiliary_device *adev, > const struct auxiliary_device_id *id) > { > + const struct power_supply_desc *psy_desc; > struct power_supply_config psy_cfg_supply = {}; > struct power_supply_config psy_cfg = {}; > const struct of_device_id *match; > @@ -1373,8 +1602,14 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev, > else > battmgr->variant = QCOM_BATTMGR_SM8350; > > - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) { > - battmgr->bat_psy = devm_power_supply_register(dev, &sc8280xp_bat_psy_desc, &psy_cfg); > + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || > + battmgr->variant == QCOM_BATTMGR_X1E80100) { > + if (battmgr->variant == QCOM_BATTMGR_X1E80100) > + psy_desc = &x1e80100_bat_psy_desc; > + else > + psy_desc = &sc8280xp_bat_psy_desc; > + > + battmgr->bat_psy = devm_power_supply_register(dev, psy_desc, &psy_cfg); > if (IS_ERR(battmgr->bat_psy)) > return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy), > "failed to register battery power supply\n"); > @@ -1394,7 +1629,12 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev, > return dev_err_probe(dev, PTR_ERR(battmgr->wls_psy), > "failed to register wireless charing power supply\n"); > } else { > - battmgr->bat_psy = devm_power_supply_register(dev, &sm8350_bat_psy_desc, &psy_cfg); > + if (battmgr->variant == QCOM_BATTMGR_SM8550) > + psy_desc = &sm8550_bat_psy_desc; > + else > + psy_desc = &sm8350_bat_psy_desc; > + > + battmgr->bat_psy = devm_power_supply_register(dev, psy_desc, &psy_cfg); > if (IS_ERR(battmgr->bat_psy)) > return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy), > "failed to register battery power supply\n"); > > -- > 2.34.1 > > >
Thanks for reviewing the change! On 5/30/2025 4:48 PM, Bryan O'Donoghue wrote: > On 30/05/2025 08:35, Fenglin Wu via B4 Relay wrote: >> From: Fenglin Wu <fenglin.wu@oss.qualcomm.com> >> >> Add charge control support for SM8550 and X1E80100. It's supported >> with below two power supply properties: >> >> charge_control_end_threshold: SOC threshold at which the charging >> should be terminated. >> >> charge_control_start_threshold: SOC threshold at which the charging >> should be resumed. > > Maybe this is very obvious to battery charger experts but what does > SOC mean here ? > > Reading your patch you pass a "int soc" and compare it to a threshold > value, without 'soc' having an obvious meaning. > > Its a threshold right ? Why not just call it threshold ? > "SOC" stands for battery State of Charge, I will rephrase the commit text for better explanation. >> >> Signed-off-by: Fenglin Wu <fenglin.wu@oss.qualcomm.com> >> --- >> drivers/power/supply/qcom_battmgr.c | 256 >> ++++++++++++++++++++++++++++++++++-- >> 1 file changed, 248 insertions(+), 8 deletions(-) >> >> - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) >> + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || >> + battmgr->variant == QCOM_BATTMGR_X1E80100) > > Please run your series through checkpatch > I actually did that before sending the patches out. I run checkpatch with below two commands and I saw no issues: git format -1 xxxx --stdtout | ./script/checkpatch.pl - b4 prep --check Can you let me know what specific command that you ran with it? > 0004-power-supply-qcom_battmgr-Add-state_of_health-proper.patch has no > obvious style problems and is ready for submission. > CHECK: Alignment should match open parenthesis > #95: FILE: drivers/power/supply/qcom_battmgr.c:521: > + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || > + battmgr->variant == QCOM_BATTMGR_X1E80100) > >> >> +static int qcom_battmgr_set_charge_start_threshold(struct >> qcom_battmgr *battmgr, int soc) >> +{ >> + u32 target_soc, delta_soc; >> + int ret; >> + >> + if (soc < CHARGE_CTRL_START_THR_MIN || >> + soc > CHARGE_CTRL_START_THR_MAX) { >> + dev_err(battmgr->dev, "charge control start threshold exceed >> range: [%u - %u]\n", >> + CHARGE_CTRL_START_THR_MIN, CHARGE_CTRL_START_THR_MAX); >> + return -EINVAL; >> + } > > 'soc' is what - a threshold as far as I can tell. I will update it with a more meaningful name >> >> if (opcode == BATTMGR_NOTIFICATION) >> qcom_battmgr_notification(battmgr, data, len); >> - else if (battmgr->variant == QCOM_BATTMGR_SC8280XP) >> + else if (battmgr->variant == QCOM_BATTMGR_SC8280XP || >> + battmgr->variant == QCOM_BATTMGR_X1E80100) >> qcom_battmgr_sc8280xp_callback(battmgr, data, len); >> else >> qcom_battmgr_sm8350_callback(battmgr, data, len); >> @@ -1333,7 +1560,8 @@ static void qcom_battmgr_pdr_notify(void *priv, >> int state) >> static const struct of_device_id qcom_battmgr_of_variants[] = { >> { .compatible = "qcom,sc8180x-pmic-glink", .data = (void >> *)QCOM_BATTMGR_SC8280XP }, >> { .compatible = "qcom,sc8280xp-pmic-glink", .data = (void >> *)QCOM_BATTMGR_SC8280XP }, >> - { .compatible = "qcom,x1e80100-pmic-glink", .data = (void >> *)QCOM_BATTMGR_SC8280XP }, >> + { .compatible = "qcom,x1e80100-pmic-glink", .data = (void >> *)QCOM_BATTMGR_X1E80100 }, >> + { .compatible = "qcom,sm8550-pmic-glink", .data = (void >> *)QCOM_BATTMGR_SM8550 }, > > Please separate compat string addition from functional changes. > The compatible string "qcom,sm8550-pmic-glink" has been present in the binding for a while and it was added as a fallback of "qcom,pmic-glink". The battmgr function has been also supported well on SM8550 for a while. The change here is only specifying a different match data for SM8550 so the driver can handle some new features differently. Does it also need to add it in a separate change? If so, this change would be split into following 3 patches I think: 1) add QCOM_BATTMGR_SM8550/X1E80100 variants definition in qcom_battmgr_variant. 2) add compatible string with corresponding match data for SM8550. 3) add the charge control function support. >> /* Unmatched devices falls back to QCOM_BATTMGR_SM8350 */ >> {} >> }; >> >> >> -- >> 2.34.1 >> >> >> >
> Add charge control support for SM8550 and X1E80100.
Thank you for this, tested on my Lenovo Yoga Slim 7x, the limiting works
well, I finally don't have to worry about leaving my laptop plugged in
for too long.
One small thing I noticed is that after setting the sysfs values and
rebooting, they report 0 again. The limiting appears to stay in effect
though, so it seems that the firmware does keep the values, but Linux
does not read them back. Indeed, looking at the code, it seems that
actually reading back the values is only implemented for the SM8550.
Anyway, this is just a small nitpick, this does not really affect the
functionality, and I would support merging this series regardless of
whether the read back values are always correct.
György
diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c index d5d0200b92bdc3d9a22f44159ad45b152efe8be0..39009415f4bfcd76b010305179d3c8fb847254a6 100644 --- a/drivers/power/supply/qcom_battmgr.c +++ b/drivers/power/supply/qcom_battmgr.c @@ -21,6 +21,8 @@ enum qcom_battmgr_variant { QCOM_BATTMGR_SM8350, QCOM_BATTMGR_SC8280XP, + QCOM_BATTMGR_X1E80100, + QCOM_BATTMGR_SM8550, }; #define BATTMGR_BAT_STATUS 0x1 @@ -66,6 +68,9 @@ enum qcom_battmgr_variant { #define BATT_RESISTANCE 21 #define BATT_POWER_NOW 22 #define BATT_POWER_AVG 23 +#define BATT_CHG_CTRL_EN 24 +#define BATT_CHG_CTRL_START_THR 25 +#define BATT_CHG_CTRL_END_THR 26 #define BATTMGR_USB_PROPERTY_GET 0x32 #define BATTMGR_USB_PROPERTY_SET 0x33 @@ -90,6 +95,13 @@ enum qcom_battmgr_variant { #define WLS_TYPE 5 #define WLS_BOOST_EN 6 +#define BATTMGR_CHG_CTRL_LIMIT_EN 0x48 +#define CHARGE_CTRL_START_THR_MIN 50 +#define CHARGE_CTRL_START_THR_MAX 95 +#define CHARGE_CTRL_END_THR_MIN 55 +#define CHARGE_CTRL_END_THR_MAX 100 +#define CHARGE_CTRL_DELTA_SOC 5 + struct qcom_battmgr_enable_request { struct pmic_glink_hdr hdr; __le32 battery_id; @@ -124,6 +136,13 @@ struct qcom_battmgr_discharge_time_request { __le32 reserved; }; +struct qcom_battmgr_charge_ctrl_request { + struct pmic_glink_hdr hdr; + __le32 enable; + __le32 target_soc; + __le32 delta_soc; +}; + struct qcom_battmgr_message { struct pmic_glink_hdr hdr; union { @@ -236,6 +255,8 @@ struct qcom_battmgr_info { unsigned int capacity_warning; unsigned int cycle_count; unsigned int charge_count; + unsigned int charge_ctrl_start; + unsigned int charge_ctrl_end; char model_number[BATTMGR_STRING_LEN]; char serial_number[BATTMGR_STRING_LEN]; char oem_info[BATTMGR_STRING_LEN]; @@ -424,6 +445,8 @@ static const u8 sm8350_bat_prop_map[] = { [POWER_SUPPLY_PROP_RESISTANCE] = BATT_RESISTANCE, [POWER_SUPPLY_PROP_STATE_OF_HEALTH] = BATT_SOH, [POWER_SUPPLY_PROP_POWER_NOW] = BATT_POWER_NOW, + [POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD] = BATT_CHG_CTRL_START_THR, + [POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD] = BATT_CHG_CTRL_END_THR, }; static int qcom_battmgr_bat_sm8350_update(struct qcom_battmgr *battmgr, @@ -494,7 +517,8 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, if (!battmgr->service_up) return -EAGAIN; - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) ret = qcom_battmgr_bat_sc8280xp_update(battmgr, psp); else ret = qcom_battmgr_bat_sm8350_update(battmgr, psp); @@ -599,6 +623,12 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, case POWER_SUPPLY_PROP_TIME_TO_FULL_AVG: val->intval = battmgr->status.charge_time; break; + case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD: + val->intval = battmgr->info.charge_ctrl_start; + break; + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: + val->intval = battmgr->info.charge_ctrl_end; + break; case POWER_SUPPLY_PROP_MANUFACTURE_YEAR: val->intval = battmgr->info.year; break; @@ -624,6 +654,120 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, return 0; } +static int qcom_battmgr_set_charge_control(struct qcom_battmgr *battmgr, + u32 target_soc, u32 delta_soc) +{ + struct qcom_battmgr_charge_ctrl_request request = { + .hdr.owner = cpu_to_le32(PMIC_GLINK_OWNER_BATTMGR), + .hdr.type = cpu_to_le32(PMIC_GLINK_REQ_RESP), + .hdr.opcode = cpu_to_le32(BATTMGR_CHG_CTRL_LIMIT_EN), + .enable = cpu_to_le32(1), + .target_soc = cpu_to_le32(target_soc), + .delta_soc = cpu_to_le32(delta_soc), + }; + + return qcom_battmgr_request(battmgr, &request, sizeof(request)); +} + +static int qcom_battmgr_set_charge_start_threshold(struct qcom_battmgr *battmgr, int soc) +{ + u32 target_soc, delta_soc; + int ret; + + if (soc < CHARGE_CTRL_START_THR_MIN || + soc > CHARGE_CTRL_START_THR_MAX) { + dev_err(battmgr->dev, "charge control start threshold exceed range: [%u - %u]\n", + CHARGE_CTRL_START_THR_MIN, CHARGE_CTRL_START_THR_MAX); + return -EINVAL; + } + + /* + * If the new start threshold is larger than the old end threshold, + * move the end threshold one step (DELTA_SOC) after the new start + * threshold. + */ + if (soc > battmgr->info.charge_ctrl_end) { + target_soc = soc + CHARGE_CTRL_DELTA_SOC; + target_soc = min_t(u32, target_soc, CHARGE_CTRL_END_THR_MAX); + delta_soc = target_soc - soc; + delta_soc = min_t(u32, delta_soc, CHARGE_CTRL_DELTA_SOC); + } else { + target_soc = battmgr->info.charge_ctrl_end; + delta_soc = battmgr->info.charge_ctrl_end - soc; + } + + mutex_lock(&battmgr->lock); + ret = qcom_battmgr_set_charge_control(battmgr, target_soc, delta_soc); + mutex_unlock(&battmgr->lock); + if (!ret) { + battmgr->info.charge_ctrl_start = soc; + battmgr->info.charge_ctrl_end = target_soc; + } + + return 0; +} + +static int qcom_battmgr_set_charge_end_threshold(struct qcom_battmgr *battmgr, int soc) +{ + u32 delta_soc = CHARGE_CTRL_DELTA_SOC; + int ret; + + if (soc < CHARGE_CTRL_END_THR_MIN || + soc > CHARGE_CTRL_END_THR_MAX) { + dev_err(battmgr->dev, "charge control end threshold exceed range: [%u - %u]\n", + CHARGE_CTRL_END_THR_MIN, CHARGE_CTRL_END_THR_MAX); + return -EINVAL; + } + + if (battmgr->info.charge_ctrl_start && soc > battmgr->info.charge_ctrl_start) + delta_soc = soc - battmgr->info.charge_ctrl_start; + + mutex_lock(&battmgr->lock); + ret = qcom_battmgr_set_charge_control(battmgr, soc, delta_soc); + mutex_unlock(&battmgr->lock); + if (!ret) { + battmgr->info.charge_ctrl_start = soc - delta_soc; + battmgr->info.charge_ctrl_end = soc; + } + + return 0; +} + +static int qcom_battmgr_bat_is_writeable(struct power_supply *psy, + enum power_supply_property psp) +{ + switch (psp) { + case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD: + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: + return 1; + default: + return 0; + } + + return 0; +} + +static int qcom_battmgr_bat_set_property(struct power_supply *psy, + enum power_supply_property psp, + const union power_supply_propval *pval) +{ + struct qcom_battmgr *battmgr = power_supply_get_drvdata(psy); + + if (!battmgr->service_up) + return -EAGAIN; + + switch (psp) { + case POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD: + return qcom_battmgr_set_charge_start_threshold(battmgr, pval->intval); + case POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD: + return qcom_battmgr_set_charge_end_threshold(battmgr, pval->intval); + default: + return -EINVAL; + } + + return 0; +} + static const enum power_supply_property sc8280xp_bat_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_PRESENT, @@ -657,6 +801,43 @@ static const struct power_supply_desc sc8280xp_bat_psy_desc = { .get_property = qcom_battmgr_bat_get_property, }; +static const enum power_supply_property x1e80100_bat_props[] = { + POWER_SUPPLY_PROP_STATUS, + POWER_SUPPLY_PROP_PRESENT, + POWER_SUPPLY_PROP_TECHNOLOGY, + POWER_SUPPLY_PROP_CYCLE_COUNT, + POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN, + POWER_SUPPLY_PROP_VOLTAGE_NOW, + POWER_SUPPLY_PROP_POWER_NOW, + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, + POWER_SUPPLY_PROP_CHARGE_FULL, + POWER_SUPPLY_PROP_CHARGE_EMPTY, + POWER_SUPPLY_PROP_CHARGE_NOW, + POWER_SUPPLY_PROP_ENERGY_FULL_DESIGN, + POWER_SUPPLY_PROP_ENERGY_FULL, + POWER_SUPPLY_PROP_ENERGY_EMPTY, + POWER_SUPPLY_PROP_ENERGY_NOW, + POWER_SUPPLY_PROP_TEMP, + POWER_SUPPLY_PROP_MANUFACTURE_YEAR, + POWER_SUPPLY_PROP_MANUFACTURE_MONTH, + POWER_SUPPLY_PROP_MANUFACTURE_DAY, + POWER_SUPPLY_PROP_MODEL_NAME, + POWER_SUPPLY_PROP_MANUFACTURER, + POWER_SUPPLY_PROP_SERIAL_NUMBER, + POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, +}; + +static const struct power_supply_desc x1e80100_bat_psy_desc = { + .name = "qcom-battmgr-bat", + .type = POWER_SUPPLY_TYPE_BATTERY, + .properties = x1e80100_bat_props, + .num_properties = ARRAY_SIZE(x1e80100_bat_props), + .get_property = qcom_battmgr_bat_get_property, + .set_property = qcom_battmgr_bat_set_property, + .property_is_writeable = qcom_battmgr_bat_is_writeable, +}; + static const enum power_supply_property sm8350_bat_props[] = { POWER_SUPPLY_PROP_STATUS, POWER_SUPPLY_PROP_HEALTH, @@ -689,6 +870,42 @@ static const struct power_supply_desc sm8350_bat_psy_desc = { .get_property = qcom_battmgr_bat_get_property, }; +static const enum power_supply_property sm8550_bat_props[] = { + POWER_SUPPLY_PROP_STATUS, + POWER_SUPPLY_PROP_HEALTH, + POWER_SUPPLY_PROP_PRESENT, + POWER_SUPPLY_PROP_CHARGE_TYPE, + POWER_SUPPLY_PROP_CAPACITY, + POWER_SUPPLY_PROP_VOLTAGE_OCV, + POWER_SUPPLY_PROP_VOLTAGE_NOW, + POWER_SUPPLY_PROP_VOLTAGE_MAX, + POWER_SUPPLY_PROP_CURRENT_NOW, + POWER_SUPPLY_PROP_TEMP, + POWER_SUPPLY_PROP_TECHNOLOGY, + POWER_SUPPLY_PROP_CHARGE_COUNTER, + POWER_SUPPLY_PROP_CYCLE_COUNT, + POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN, + POWER_SUPPLY_PROP_CHARGE_FULL, + POWER_SUPPLY_PROP_MODEL_NAME, + POWER_SUPPLY_PROP_TIME_TO_FULL_AVG, + POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG, + POWER_SUPPLY_PROP_RESISTANCE, + POWER_SUPPLY_PROP_STATE_OF_HEALTH, + POWER_SUPPLY_PROP_POWER_NOW, + POWER_SUPPLY_PROP_CHARGE_CONTROL_START_THRESHOLD, + POWER_SUPPLY_PROP_CHARGE_CONTROL_END_THRESHOLD, +}; + +static const struct power_supply_desc sm8550_bat_psy_desc = { + .name = "qcom-battmgr-bat", + .type = POWER_SUPPLY_TYPE_BATTERY, + .properties = sm8550_bat_props, + .num_properties = ARRAY_SIZE(sm8550_bat_props), + .get_property = qcom_battmgr_bat_get_property, + .set_property = qcom_battmgr_bat_set_property, + .property_is_writeable = qcom_battmgr_bat_is_writeable, +}; + static int qcom_battmgr_ac_get_property(struct power_supply *psy, enum power_supply_property psp, union power_supply_propval *val) @@ -764,7 +981,8 @@ static int qcom_battmgr_usb_get_property(struct power_supply *psy, if (!battmgr->service_up) return -EAGAIN; - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) ret = qcom_battmgr_bat_sc8280xp_update(battmgr, psp); else ret = qcom_battmgr_usb_sm8350_update(battmgr, psp); @@ -886,7 +1104,8 @@ static int qcom_battmgr_wls_get_property(struct power_supply *psy, if (!battmgr->service_up) return -EAGAIN; - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) ret = qcom_battmgr_bat_sc8280xp_update(battmgr, psp); else ret = qcom_battmgr_wls_sm8350_update(battmgr, psp); @@ -1196,6 +1415,12 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr, case BATT_POWER_NOW: battmgr->status.power_now = le32_to_cpu(resp->intval.value); break; + case BATT_CHG_CTRL_START_THR: + battmgr->info.charge_ctrl_start = le32_to_cpu(resp->intval.value); + break; + case BATT_CHG_CTRL_END_THR: + battmgr->info.charge_ctrl_end = le32_to_cpu(resp->intval.value); + break; default: dev_warn(battmgr->dev, "unknown property %#x\n", property); break; @@ -1278,6 +1503,7 @@ static void qcom_battmgr_sm8350_callback(struct qcom_battmgr *battmgr, } break; case BATTMGR_REQUEST_NOTIFICATION: + case BATTMGR_CHG_CTRL_LIMIT_EN: battmgr->error = 0; break; default: @@ -1297,7 +1523,8 @@ static void qcom_battmgr_callback(const void *data, size_t len, void *priv) if (opcode == BATTMGR_NOTIFICATION) qcom_battmgr_notification(battmgr, data, len); - else if (battmgr->variant == QCOM_BATTMGR_SC8280XP) + else if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) qcom_battmgr_sc8280xp_callback(battmgr, data, len); else qcom_battmgr_sm8350_callback(battmgr, data, len); @@ -1333,7 +1560,8 @@ static void qcom_battmgr_pdr_notify(void *priv, int state) static const struct of_device_id qcom_battmgr_of_variants[] = { { .compatible = "qcom,sc8180x-pmic-glink", .data = (void *)QCOM_BATTMGR_SC8280XP }, { .compatible = "qcom,sc8280xp-pmic-glink", .data = (void *)QCOM_BATTMGR_SC8280XP }, - { .compatible = "qcom,x1e80100-pmic-glink", .data = (void *)QCOM_BATTMGR_SC8280XP }, + { .compatible = "qcom,x1e80100-pmic-glink", .data = (void *)QCOM_BATTMGR_X1E80100 }, + { .compatible = "qcom,sm8550-pmic-glink", .data = (void *)QCOM_BATTMGR_SM8550 }, /* Unmatched devices falls back to QCOM_BATTMGR_SM8350 */ {} }; @@ -1343,6 +1571,7 @@ static char *qcom_battmgr_battery[] = { "battery" }; static int qcom_battmgr_probe(struct auxiliary_device *adev, const struct auxiliary_device_id *id) { + const struct power_supply_desc *psy_desc; struct power_supply_config psy_cfg_supply = {}; struct power_supply_config psy_cfg = {}; const struct of_device_id *match; @@ -1373,8 +1602,14 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev, else battmgr->variant = QCOM_BATTMGR_SM8350; - if (battmgr->variant == QCOM_BATTMGR_SC8280XP) { - battmgr->bat_psy = devm_power_supply_register(dev, &sc8280xp_bat_psy_desc, &psy_cfg); + if (battmgr->variant == QCOM_BATTMGR_SC8280XP || + battmgr->variant == QCOM_BATTMGR_X1E80100) { + if (battmgr->variant == QCOM_BATTMGR_X1E80100) + psy_desc = &x1e80100_bat_psy_desc; + else + psy_desc = &sc8280xp_bat_psy_desc; + + battmgr->bat_psy = devm_power_supply_register(dev, psy_desc, &psy_cfg); if (IS_ERR(battmgr->bat_psy)) return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy), "failed to register battery power supply\n"); @@ -1394,7 +1629,12 @@ static int qcom_battmgr_probe(struct auxiliary_device *adev, return dev_err_probe(dev, PTR_ERR(battmgr->wls_psy), "failed to register wireless charing power supply\n"); } else { - battmgr->bat_psy = devm_power_supply_register(dev, &sm8350_bat_psy_desc, &psy_cfg); + if (battmgr->variant == QCOM_BATTMGR_SM8550) + psy_desc = &sm8550_bat_psy_desc; + else + psy_desc = &sm8350_bat_psy_desc; + + battmgr->bat_psy = devm_power_supply_register(dev, psy_desc, &psy_cfg); if (IS_ERR(battmgr->bat_psy)) return dev_err_probe(dev, PTR_ERR(battmgr->bat_psy), "failed to register battery power supply\n");