Sebastian Reichel писал(а) 14.09.2023 19:35: > Hi, > > On Mon, Jul 31, 2023 at 10:06:26PM +0500, Nikita Travkin wrote: >> This driver adds basic support for VM-BMS found in pm8916. >> >> VM-BMS is a very basic fuel-gauge hardware block that is, sadly, >> incapable of any gauging. The hardware supports measuring OCV in >> sleep mode, where the battery is not in use, or measuring average >> voltage over time when the device is active. >> >> This driver implements basic value readout from this block. >> >> Signed-off-by: Nikita Travkin <nikita@xxxxxxx> >> --- >> v2: Get irq by name >> --- > > Thanks for the patch. I have a few small change requests. > >> drivers/power/supply/Kconfig | 11 ++ >> drivers/power/supply/Makefile | 1 + >> drivers/power/supply/pm8916_bms_vm.c | 296 +++++++++++++++++++++++++++++++++++ >> 3 files changed, 308 insertions(+) >> >> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig >> index 663a1c423806..e93a5a4d03e2 100644 >> --- a/drivers/power/supply/Kconfig >> +++ b/drivers/power/supply/Kconfig >> @@ -629,6 +629,17 @@ config CHARGER_QCOM_SMBB >> documentation for more detail. The base name for this driver is >> 'pm8941_charger'. >> >> +config BATTERY_PM8916_BMS_VM >> + tristate "Qualcomm PM8916 BMS-VM support" >> + depends on MFD_SPMI_PMIC || COMPILE_TEST >> + help >> + Say Y to add support for Voltage Mode BMS block found in some >> + Qualcomm PMICs such as PM8916. This hardware block provides >> + battery voltage monitoring for the system. >> + >> + To compile this driver as module, choose M here: the >> + module will be called pm8916_bms_vm. >> + >> config CHARGER_BQ2415X >> tristate "TI BQ2415x battery charger driver" >> depends on I2C >> diff --git a/drivers/power/supply/Makefile b/drivers/power/supply/Makefile >> index a8a9fa6de1e9..fdf7916f80ed 100644 >> --- a/drivers/power/supply/Makefile >> +++ b/drivers/power/supply/Makefile >> @@ -84,6 +84,7 @@ obj-$(CONFIG_CHARGER_MP2629) += mp2629_charger.o >> obj-$(CONFIG_CHARGER_MT6360) += mt6360_charger.o >> obj-$(CONFIG_CHARGER_MT6370) += mt6370-charger.o >> obj-$(CONFIG_CHARGER_QCOM_SMBB) += qcom_smbb.o >> +obj-$(CONFIG_BATTERY_PM8916_BMS_VM) += pm8916_bms_vm.o >> obj-$(CONFIG_CHARGER_BQ2415X) += bq2415x_charger.o >> obj-$(CONFIG_CHARGER_BQ24190) += bq24190_charger.o >> obj-$(CONFIG_CHARGER_BQ24257) += bq24257_charger.o >> diff --git a/drivers/power/supply/pm8916_bms_vm.c b/drivers/power/supply/pm8916_bms_vm.c >> new file mode 100644 >> index 000000000000..6cf00bf1c466 >> --- /dev/null >> +++ b/drivers/power/supply/pm8916_bms_vm.c >> @@ -0,0 +1,296 @@ >> +// SPDX-License-Identifier: GPL-2.0-only >> +/* >> + * Copyright (c) 2023, Nikita Travkin <nikita@xxxxxxx> >> + */ >> + >> +#include <linux/errno.h> >> +#include <linux/module.h> >> +#include <linux/of.h> >> +#include <linux/of_device.h> > > You should be able to remove the of headers after my proposed > changes. > Will switch and drop this. >> +#include <linux/platform_device.h> >> +#include <linux/power_supply.h> >> +#include <linux/property.h> >> +#include <linux/regmap.h> >> +#include <linux/slab.h> >> +#include <linux/delay.h> >> +#include <linux/interrupt.h> >> + >> +#define PM8916_PERPH_TYPE 0x04 >> +#define PM8916_BMS_VM_TYPE 0x020D >> + >> +#define PM8916_SEC_ACCESS 0xD0 >> +#define PM8916_SEC_MAGIC 0xA5 >> + >> +#define PM8916_BMS_VM_STATUS1 0x08 >> +#define PM8916_BMS_VM_FSM_STATE(x) (((x) & 0b00111000) >> 3) >> +#define PM8916_BMS_VM_FSM_STATE_S2 0x2 >> + >> +#define PM8916_BMS_VM_MODE_CTL 0x40 >> +#define PM8916_BMS_VM_MODE_FORCE_S3 (BIT(0) | BIT(1)) >> +#define PM8916_BMS_VM_MODE_NORMAL (BIT(1) | BIT(3)) >> + >> +#define PM8916_BMS_VM_EN_CTL 0x46 >> +#define PM8916_BMS_ENABLED BIT(7) >> + >> +#define PM8916_BMS_VM_FIFO_LENGTH_CTL 0x47 >> +#define PM8916_BMS_VM_S1_SAMPLE_INTERVAL_CTL 0x55 >> +#define PM8916_BMS_VM_S2_SAMPLE_INTERVAL_CTL 0x56 >> +#define PM8916_BMS_VM_S3_S7_OCV_DATA0 0x6A >> +#define PM8916_BMS_VM_BMS_FIFO_REG_0_LSB 0xC0 >> + >> +/* Using only 1 fifo is broken in hardware */ >> +#define PM8916_BMS_VM_FIFO_COUNT 2 /* 2 .. 8 */ >> + >> +#define PM8916_BMS_VM_S1_SAMPLE_INTERVAL 10 >> +#define PM8916_BMS_VM_S2_SAMPLE_INTERVAL 10 >> + >> +struct pm8916_bms_vm_battery { >> + struct device *dev; >> + struct power_supply *battery; >> + struct power_supply_battery_info *info; >> + struct regmap *regmap; >> + unsigned int reg; >> + unsigned int last_ocv; >> + unsigned int vbat_now; >> +}; >> + >> +static int pm8916_bms_vm_battery_get_property(struct power_supply *psy, >> + enum power_supply_property psp, >> + union power_supply_propval *val) >> +{ >> + struct pm8916_bms_vm_battery *bat = power_supply_get_drvdata(psy); >> + struct power_supply_battery_info *info = bat->info; >> + int supplied; >> + >> + switch (psp) { >> + case POWER_SUPPLY_PROP_STATUS: >> + supplied = power_supply_am_i_supplied(psy); >> + >> + if (supplied < 0 && supplied != -ENODEV) >> + return supplied; >> + else if (supplied && supplied != -ENODEV) >> + val->intval = POWER_SUPPLY_STATUS_CHARGING; >> + else >> + val->intval = POWER_SUPPLY_STATUS_DISCHARGING; >> + return 0; >> + >> + case POWER_SUPPLY_PROP_HEALTH: >> + if (bat->vbat_now < info->voltage_min_design_uv) >> + val->intval = POWER_SUPPLY_HEALTH_DEAD; >> + else if (bat->vbat_now > info->voltage_max_design_uv) >> + val->intval = POWER_SUPPLY_HEALTH_OVERVOLTAGE; >> + else >> + val->intval = POWER_SUPPLY_HEALTH_GOOD; >> + return 0; >> + >> + case POWER_SUPPLY_PROP_VOLTAGE_NOW: >> + val->intval = bat->vbat_now; >> + return 0; >> + >> + case POWER_SUPPLY_PROP_VOLTAGE_BOOT: >> + /* Returning last known ocv value here - it changes after suspend. */ >> + val->intval = bat->last_ocv; >> + return 0; > > Returning OCV from last suspend is not the same as VOLTAGE_BOOT. How > about exposing POWER_SUPPLY_PROP_VOLTAGE_OCV and returning -ENODATA > if the value is older than 180 seconds? > Hm, indeed, I didn't think of this as an option... Will implement that instead. Thanks, Nikita >> + >> + default: >> + return -EINVAL; >> + } >> +} >> + >> +static enum power_supply_property pm8916_bms_vm_battery_properties[] = { >> + POWER_SUPPLY_PROP_STATUS, >> + POWER_SUPPLY_PROP_VOLTAGE_NOW, >> + POWER_SUPPLY_PROP_VOLTAGE_BOOT, >> + POWER_SUPPLY_PROP_HEALTH, >> +}; >> + >> +static irqreturn_t pm8916_bms_vm_fifo_update_done_irq(int irq, void *data) >> +{ >> + struct pm8916_bms_vm_battery *bat = data; >> + u16 vbat_data[PM8916_BMS_VM_FIFO_COUNT]; >> + int ret; >> + >> + ret = regmap_bulk_read(bat->regmap, bat->reg + PM8916_BMS_VM_BMS_FIFO_REG_0_LSB, >> + &vbat_data, PM8916_BMS_VM_FIFO_COUNT * 2); >> + if (ret) >> + return IRQ_HANDLED; >> + >> + /* >> + * The VM-BMS hardware only collects voltage data and the software >> + * has to process it to calculate the OCV and SoC. Hardware provides >> + * up to 8 averaged measurements for software to take in account. >> + * >> + * Just use the last measured value for now to report the current >> + * battery voltage. >> + */ >> + bat->vbat_now = vbat_data[PM8916_BMS_VM_FIFO_COUNT - 1] * 300; >> + >> + power_supply_changed(bat->battery); >> + >> + return IRQ_HANDLED; >> +} >> + >> +static const struct power_supply_desc pm8916_bms_vm_battery_psy_desc = { >> + .name = "pm8916-bms-vm", >> + .type = POWER_SUPPLY_TYPE_BATTERY, >> + .properties = pm8916_bms_vm_battery_properties, >> + .num_properties = ARRAY_SIZE(pm8916_bms_vm_battery_properties), >> + .get_property = pm8916_bms_vm_battery_get_property, >> +}; >> + >> +static int pm8916_bms_vm_battery_probe(struct platform_device *pdev) >> +{ >> + struct device *dev = &pdev->dev; >> + struct pm8916_bms_vm_battery *bat; >> + struct power_supply_config psy_cfg = {}; >> + int ret, irq; >> + unsigned int tmp; >> + >> + bat = devm_kzalloc(dev, sizeof(*bat), GFP_KERNEL); >> + if (!bat) >> + return -ENOMEM; >> + >> + bat->dev = dev; >> + >> + bat->regmap = dev_get_regmap(pdev->dev.parent, NULL); >> + if (!bat->regmap) >> + return -ENODEV; >> + >> + of_property_read_u32(dev->of_node, "reg", &bat->reg); > > device_property_read_u32(...) > >> + if (bat->reg < 0) >> + return -EINVAL; >> + >> + irq = platform_get_irq_byname(pdev, "fifo"); >> + if (irq < 0) >> + return irq; >> + >> + ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_bms_vm_fifo_update_done_irq, >> + IRQF_ONESHOT, "pm8916_vm_bms", bat); >> + if (ret) >> + return ret; >> + >> + ret = regmap_bulk_read(bat->regmap, bat->reg + PM8916_PERPH_TYPE, &tmp, 2); >> + if (ret) >> + goto comm_error; >> + >> + if (tmp != PM8916_BMS_VM_TYPE) >> + return dev_err_probe(dev, -ENODEV, "Device reported wrong type: 0x%X\n", tmp); >> + >> + ret = regmap_write(bat->regmap, bat->reg + PM8916_BMS_VM_S1_SAMPLE_INTERVAL_CTL, >> + PM8916_BMS_VM_S1_SAMPLE_INTERVAL); >> + if (ret) >> + goto comm_error; >> + ret = regmap_write(bat->regmap, bat->reg + PM8916_BMS_VM_S2_SAMPLE_INTERVAL_CTL, >> + PM8916_BMS_VM_S2_SAMPLE_INTERVAL); >> + if (ret) >> + goto comm_error; >> + ret = regmap_write(bat->regmap, bat->reg + PM8916_BMS_VM_FIFO_LENGTH_CTL, >> + PM8916_BMS_VM_FIFO_COUNT << 4 | PM8916_BMS_VM_FIFO_COUNT); >> + if (ret) >> + goto comm_error; >> + ret = regmap_write(bat->regmap, >> + bat->reg + PM8916_BMS_VM_EN_CTL, PM8916_BMS_ENABLED); >> + if (ret) >> + goto comm_error; >> + >> + ret = regmap_bulk_read(bat->regmap, >> + bat->reg + PM8916_BMS_VM_S3_S7_OCV_DATA0, &tmp, 2); >> + if (ret) >> + goto comm_error; >> + >> + bat->last_ocv = tmp * 300; >> + bat->vbat_now = bat->last_ocv; >> + >> + psy_cfg.drv_data = bat; >> + psy_cfg.of_node = dev->of_node; >> + >> + bat->battery = devm_power_supply_register(dev, &pm8916_bms_vm_battery_psy_desc, &psy_cfg); >> + if (IS_ERR(bat->battery)) >> + return dev_err_probe(dev, PTR_ERR(bat->battery), "Unable to register battery\n"); >> + >> + ret = power_supply_get_battery_info(bat->battery, &bat->info); >> + if (ret) >> + return dev_err_probe(dev, ret, "Unable to get battery info\n"); >> + >> + platform_set_drvdata(pdev, bat); >> + >> + return 0; >> + >> +comm_error: >> + return dev_err_probe(dev, ret, "Unable to communicate with device\n"); >> +} >> + >> +static int pm8916_bms_vm_battery_suspend(struct platform_device *pdev, pm_message_t state) >> +{ >> + struct pm8916_bms_vm_battery *bat = platform_get_drvdata(pdev); >> + int ret; >> + >> + /* >> + * Due to a hardware quirk the FSM doesn't switch states normally. >> + * Instead we unlock the debug registers and force S3 (Measure OCV/Sleep) >> + * mode every time we suspend. >> + */ >> + >> + ret = regmap_write(bat->regmap, >> + bat->reg + PM8916_SEC_ACCESS, PM8916_SEC_MAGIC); >> + if (ret) >> + goto error; >> + ret = regmap_write(bat->regmap, >> + bat->reg + PM8916_BMS_VM_MODE_CTL, PM8916_BMS_VM_MODE_FORCE_S3); >> + if (ret) >> + goto error; >> + >> + return 0; >> + >> +error: >> + dev_err(bat->dev, "Failed to force S3 mode: %pe\n", ERR_PTR(ret)); >> + return ret; >> +} >> + >> +static int pm8916_bms_vm_battery_resume(struct platform_device *pdev) >> +{ >> + struct pm8916_bms_vm_battery *bat = platform_get_drvdata(pdev); >> + int ret; >> + unsigned int tmp; >> + >> + ret = regmap_bulk_read(bat->regmap, >> + bat->reg + PM8916_BMS_VM_S3_S7_OCV_DATA0, &tmp, 2); >> + >> + bat->last_ocv = tmp * 300; >> + >> + ret = regmap_write(bat->regmap, >> + bat->reg + PM8916_SEC_ACCESS, PM8916_SEC_MAGIC); >> + if (ret) >> + goto error; >> + ret = regmap_write(bat->regmap, >> + bat->reg + PM8916_BMS_VM_MODE_CTL, PM8916_BMS_VM_MODE_NORMAL); >> + if (ret) >> + goto error; >> + >> + return 0; >> + >> +error: >> + dev_err(bat->dev, "Failed to return normal mode: %pe\n", ERR_PTR(ret)); >> + return ret; >> +} >> + >> +static const struct of_device_id pm8916_bms_vm_battery_of_match[] = { >> + { .compatible = "qcom,pm8916-bms-vm", }, >> + { }, > > {} > > (i.e. remove space and trailing , for terminator entry) > >> +}; >> +MODULE_DEVICE_TABLE(of, pm8916_bms_vm_battery_of_match); >> + >> +static struct platform_driver pm8916_bms_vm_battery_driver = { >> + .driver = { >> + .name = "pm8916-bms-vm", >> + .of_match_table = of_match_ptr(pm8916_bms_vm_battery_of_match), > > remove of_match_ptr(). > >> + }, >> + .probe = pm8916_bms_vm_battery_probe, >> + .suspend = pm8916_bms_vm_battery_suspend, >> + .resume = pm8916_bms_vm_battery_resume, >> +}; >> +module_platform_driver(pm8916_bms_vm_battery_driver); >> + >> +MODULE_DESCRIPTION("pm8916 BMS-VM driver"); >> +MODULE_AUTHOR("Nikita Travkin <nikita@xxxxxxx>"); >> +MODULE_LICENSE("GPL"); > > Otherwise LGTM, > > -- Sebastian