Date:   Thu, 14 Sep 2023 19:55:30 +0500
From:   Nikita Travkin <nikita@...n.ru>
To:     Sebastian Reichel <sebastian.reichel@...labora.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v2 4/4] power: supply: Add driver for pm8916 lbc

Sebastian Reichel писал(а) 14.09.2023 19:25:
> Hi,
> 
> On Mon, Jul 31, 2023 at 10:06:27PM +0500, Nikita Travkin wrote:
>> pm8916 LBC is a Linear Battery Charger hardware block in pm8916 PMIC.
>>
>> This block implements simple CC/CV charging for Li-Po batteries.
>> The hardware has internal state machine to switch between modes and
>> works mostly autonomously, only needing the limits and targets to be
>> set to operate.
>>
>> This driver allows setting limits and enabling the LBC block, monitoring
>> it's state.
>>
>> Signed-off-by: Nikita Travkin <nikita@...n.ru>
>> ---
>> v2: Fix missed warnings, get irq by name
>> ---
> 
> Looks mostly good, but I have a few small requests.
> 
>>  drivers/power/supply/Kconfig      |  11 ++
>>  drivers/power/supply/Makefile     |   1 +
>>  drivers/power/supply/pm8916_lbc.c | 383 ++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 395 insertions(+)
>>
>> diff --git a/drivers/power/supply/Kconfig b/drivers/power/supply/Kconfig
>> index e93a5a4d03e2..a2ea249a57c6 100644
>> --- a/drivers/power/supply/Kconfig
>> +++ b/drivers/power/supply/Kconfig
>> @@ -640,6 +640,17 @@ config BATTERY_PM8916_BMS_VM
>>  	  To compile this driver as module, choose M here: the
>>  	  module will be called pm8916_bms_vm.
>>
>> +config CHARGER_PM8916_LBC
>> +	tristate "Qualcomm PM8916 Linear Battery Charger support"
>> +	depends on MFD_SPMI_PMIC || COMPILE_TEST
>> +	help
>> +	  Say Y here to add support for Linear Battery Charger block
>> +	  found in some Qualcomm PMICs such as PM8916. This hardware
>> +	  blokc provides simple CC/CV battery charger.
>> +
>> +	  To compile this driver as module, choose M here: the
>> +	  module will be called pm8916_lbc.
>> +
>>  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 fdf7916f80ed..e4bd9eb1261b 100644
>> --- a/drivers/power/supply/Makefile
>> +++ b/drivers/power/supply/Makefile
>> @@ -85,6 +85,7 @@ 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_PM8916_LBC)	+= pm8916_lbc.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_lbc.c b/drivers/power/supply/pm8916_lbc.c
>> new file mode 100644
>> index 000000000000..490cb7064dbf
>> --- /dev/null
>> +++ b/drivers/power/supply/pm8916_lbc.c
>> @@ -0,0 +1,383 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Nikita Travkin <nikita@...n.ru>
>> + */
>> +
>> +#include <linux/errno.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
> 
> It should be fine to remove the of headers after my proposed
> changes.
> 

Will switch to the device_* and drop.

>> +#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>
>> +#include <linux/extcon-provider.h>
>> +
>> +/* Two bytes: type + subtype */
>> +#define PM8916_PERPH_TYPE 0x04
>> +#define PM8916_LBC_CHGR_TYPE 0x1502
>> +#define PM8916_LBC_BAT_IF_TYPE 0x1602
>> +#define PM8916_LBC_USB_TYPE 0x1702
>> +#define PM8916_LBC_MISC_TYPE 0x1802
>> +
>> +#define PM8916_LBC_CHGR_CHG_OPTION 0x08
>> +#define PM8916_LBC_CHGR_PMIC_CHARGER BIT(7)
>> +
>> +#define PM8916_LBC_CHGR_CHG_STATUS 0x09
>> +
>> +#define PM8916_INT_RT_STS 0x10
>> +
>> +#define PM8916_LBC_USB_USBIN_VALID BIT(1)
>> +
>> +#define PM8916_LBC_CHGR_VDD_MAX 0x40
>> +#define PM8916_LBC_CHGR_VDD_SAFE 0x41
>> +#define PM8916_LBC_CHGR_IBAT_MAX 0x44
>> +#define PM8916_LBC_CHGR_IBAT_SAFE 0x45
>> +
>> +#define PM8916_LBC_CHGR_TCHG_MAX_EN 0x60
>> +#define PM8916_LBC_CHGR_TCHG_MAX_ENABLED BIT(7)
>> +#define PM8916_LBC_CHGR_TCHG_MAX 0x61
>> +
>> +#define PM8916_LBC_CHGR_CHG_CTRL 0x49
>> +#define PM8916_LBC_CHGR_CHG_EN BIT(7)
>> +#define PM8916_LBC_CHGR_PSTG_EN BIT(5)
>> +
>> +#define PM8916_LBC_CHGR_MIN_CURRENT 90000
>> +#define PM8916_LBC_CHGR_MAX_CURRENT 1440000
>> +
>> +#define PM8916_LBC_CHGR_MIN_VOLTAGE 4000000
>> +#define PM8916_LBC_CHGR_MAX_VOLTAGE 4775000
>> +#define PM8916_LBC_CHGR_VOLTAGE_STEP 25000
>> +
>> +#define PM8916_LBC_CHGR_MIN_TIME 4
>> +#define PM8916_LBC_CHGR_MAX_TIME 256
>> +
>> +struct pm8916_lbc_charger {
>> +	struct device *dev;
>> +	struct extcon_dev *edev;
>> +	struct power_supply *charger;
>> +	struct power_supply_battery_info *info;
>> +	struct regmap *regmap;
>> +	unsigned int reg[4];
>> +	bool online;
>> +	unsigned int charge_voltage_max;
>> +	unsigned int charge_voltage_safe;
>> +	unsigned int charge_current_max;
>> +	unsigned int charge_current_safe;
>> +};
>> +
>> +static const unsigned int pm8916_lbc_charger_cable[] = {
>> +	EXTCON_USB,
>> +	EXTCON_NONE,
>> +};
>> +
>> +enum {
>> +	LBC_CHGR = 0,
>> +	LBC_BAT_IF,
>> +	LBC_USB,
>> +	LBC_MISC,
>> +};
>> +
>> +static int pm8916_lbc_charger_configure(struct pm8916_lbc_charger *chg)
>> +{
>> +	int ret = 0;
>> +	unsigned int tmp;
>> +
>> +	chg->charge_voltage_max = clamp_t(u32, chg->charge_voltage_max,
>> +					  PM8916_LBC_CHGR_MIN_VOLTAGE, chg->charge_voltage_safe);
>> +
>> +	tmp = chg->charge_voltage_max - PM8916_LBC_CHGR_MIN_VOLTAGE;
>> +	tmp /= PM8916_LBC_CHGR_VOLTAGE_STEP;
>> +	chg->charge_voltage_max = PM8916_LBC_CHGR_MIN_VOLTAGE + tmp * PM8916_LBC_CHGR_VOLTAGE_STEP;
>> +
>> +	ret = regmap_write(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_VDD_MAX, tmp);
>> +	if (ret)
>> +		goto error;
>> +
>> +	chg->charge_current_max = min(chg->charge_current_max, chg->charge_current_safe);
>> +
>> +	tmp = clamp_t(u32, chg->charge_current_max,
>> +		      PM8916_LBC_CHGR_MIN_CURRENT, PM8916_LBC_CHGR_MAX_CURRENT);
>> +
>> +	tmp = chg->charge_current_max / PM8916_LBC_CHGR_MIN_CURRENT - 1;
>> +	chg->charge_current_max = (tmp + 1) * PM8916_LBC_CHGR_MIN_CURRENT;
>> +
>> +	ret = regmap_write(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_IBAT_MAX, tmp);
>> +	if (ret)
>> +		goto error;
>> +
>> +	ret = regmap_write(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_CHG_CTRL,
>> +			   PM8916_LBC_CHGR_CHG_EN | PM8916_LBC_CHGR_PSTG_EN);
>> +	if (ret)
>> +		goto error;
>> +
>> +	return ret;
>> +
>> +error:
>> +	dev_err(chg->dev, "Failed to configure charging: %pe\n", ERR_PTR(ret));
>> +	return ret;
>> +}
>> +
>> +static int pm8916_lbc_charger_get_property(struct power_supply *psy,
>> +					   enum power_supply_property psp,
>> +					   union power_supply_propval *val)
>> +{
>> +	struct pm8916_lbc_charger *chg = power_supply_get_drvdata(psy);
>> +
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_ONLINE:
>> +		val->intval = chg->online;
>> +		return 0;
>> +
>> +	case POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX:
>> +		val->intval = chg->charge_voltage_max;
>> +		return 0;
>> +
>> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> +		val->intval = chg->charge_current_max;
>> +		return 0;
>> +
>> +	default:
>> +		return -EINVAL;
>> +	};
>> +}
>> +
>> +static int pm8916_lbc_charger_set_property(struct power_supply *psy,
>> +					   enum power_supply_property prop,
>> +					   const union power_supply_propval *val)
>> +{
>> +	struct pm8916_lbc_charger *chg = power_supply_get_drvdata(psy);
>> +
>> +	switch (prop) {
>> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> +		chg->charge_current_max = val->intval;
>> +		return pm8916_lbc_charger_configure(chg);
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +}
>> +
>> +static int pm8916_lbc_charger_property_is_writeable(struct power_supply *psy,
>> +						    enum power_supply_property psp)
>> +{
>> +	switch (psp) {
>> +	case POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>> +static enum power_supply_property pm8916_lbc_charger_properties[] = {
>> +	POWER_SUPPLY_PROP_ONLINE,
>> +	POWER_SUPPLY_PROP_CONSTANT_CHARGE_VOLTAGE_MAX,
>> +	POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT,
>> +};
> 
> POWER_SUPPLY_PROP_INPUT_CURRENT_LIMIT is about the charger input,
> e.g. 500mA limit for a USB based charger. The variable names you are
> using suggests, that you want to expose
> POWER_SUPPLY_PROP_CONSTANT_CHARGE_CURRENT instead.
> 

Hm yes, I was not sure which one is more appropriate but CHARGE_CURRENT
seems more appropriate indeed. Will switch to that.

>> +static irqreturn_t pm8916_lbc_charger_state_changed_irq(int irq, void *data)
>> +{
>> +	struct pm8916_lbc_charger *chg = data;
>> +	unsigned int tmp;
>> +	int ret;
>> +
>> +	ret = regmap_read(chg->regmap, chg->reg[LBC_USB] + PM8916_INT_RT_STS, &tmp);
>> +	if (ret)
>> +		return IRQ_HANDLED;
>> +
>> +	chg->online = !!(tmp & PM8916_LBC_USB_USBIN_VALID);
>> +	extcon_set_state_sync(chg->edev, EXTCON_USB, chg->online);
>> +
>> +	power_supply_changed(chg->charger);
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int pm8916_lbc_charger_probe_dt(struct pm8916_lbc_charger *chg)
>> +{
>> +	struct device *dev = chg->dev;
>> +	struct device_node *np = dev->of_node;
>> +	int ret = 0;
>> +	unsigned int tmp;
>> +
>> +	ret = of_property_read_u32(np, "qcom,fast-charge-safe-voltage", &chg->charge_voltage_safe);
> 
> device_property_read_u32(...)
> 
>> +	if (ret)
>> +		return ret;
>> +	if (chg->charge_voltage_safe < PM8916_LBC_CHGR_MIN_VOLTAGE)
>> +		return -EINVAL;
>> +
>> +	chg->charge_voltage_safe = clamp_t(u32, chg->charge_voltage_safe,
>> +					PM8916_LBC_CHGR_MIN_VOLTAGE, PM8916_LBC_CHGR_MAX_VOLTAGE);
>> +
>> +	tmp = chg->charge_voltage_safe - PM8916_LBC_CHGR_MIN_VOLTAGE;
>> +	tmp /= PM8916_LBC_CHGR_VOLTAGE_STEP;
>> +	ret = regmap_write(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_VDD_SAFE, tmp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32(np, "qcom,fast-charge-safe-current", &chg->charge_current_safe);
> 
> device_property_read_u32(...)
> 
>> +	if (ret)
>> +		return ret;
>> +	if (chg->charge_current_safe < PM8916_LBC_CHGR_MIN_CURRENT)
>> +		return -EINVAL;
>> +
>> +	chg->charge_current_safe = clamp_t(u32, chg->charge_current_safe,
>> +					PM8916_LBC_CHGR_MIN_CURRENT, PM8916_LBC_CHGR_MAX_CURRENT);
>> +
>> +	chg->charge_current_max = chg->charge_current_safe;
>> +
>> +	tmp = chg->charge_current_safe / PM8916_LBC_CHGR_MIN_CURRENT - 1;
>> +	ret = regmap_write(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_IBAT_SAFE, tmp);
>> +	if (ret)
>> +		return ret;
>> +
>> +	/* Disable charger timeout. */
>> +	ret = regmap_write(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_TCHG_MAX_EN, 0x00);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return ret;
>> +}
>> +
>> +static const struct power_supply_desc pm8916_lbc_charger_psy_desc = {
>> +	.name = "pm8916-lbc-chgr",
>> +	.type = POWER_SUPPLY_TYPE_USB,
>> +	.properties = pm8916_lbc_charger_properties,
>> +	.num_properties = ARRAY_SIZE(pm8916_lbc_charger_properties),
>> +	.get_property = pm8916_lbc_charger_get_property,
>> +	.set_property = pm8916_lbc_charger_set_property,
>> +	.property_is_writeable = pm8916_lbc_charger_property_is_writeable,
>> +};
>> +
>> +static int pm8916_lbc_charger_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct pm8916_lbc_charger *chg;
>> +	struct power_supply_config psy_cfg = {};
>> +	int ret, len, irq;
>> +	unsigned int tmp;
>> +
>> +	chg = devm_kzalloc(dev, sizeof(*chg), GFP_KERNEL);
>> +	if (!chg)
>> +		return -ENOMEM;
>> +
>> +	chg->dev = dev;
>> +
>> +	chg->regmap = dev_get_regmap(pdev->dev.parent, NULL);
>> +	if (!chg->regmap)
>> +		return -ENODEV;
>> +
>> +	len = of_property_count_u32_elems(dev->of_node, "reg");
> 
> device_property_count_u32(...)
> 
>> +	if (len < 0)
>> +		return len;
>> +	if (len != 4)
>> +		return dev_err_probe(dev, -EINVAL,
>> +				     "Wrong amount of reg values: %d (4 expected)\n", len);
>> +
>> +	irq = platform_get_irq_byname(pdev, "usb_vbus");
>> +	if (irq < 0)
>> +		return irq;
>> +
>> +	ret = devm_request_threaded_irq(dev, irq, NULL, pm8916_lbc_charger_state_changed_irq,
>> +					IRQF_ONESHOT, "pm8916_lbc", chg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = of_property_read_u32_array(dev->of_node, "reg", chg->reg, len);
> 
> device_property_read_u32_array(...)
> 
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = regmap_bulk_read(chg->regmap, chg->reg[LBC_CHGR] + PM8916_PERPH_TYPE, &tmp, 2);
>> +	if (ret)
>> +		goto comm_error;
>> +	if (tmp != PM8916_LBC_CHGR_TYPE)
>> +		goto type_error;
>> +
>> +	ret = regmap_bulk_read(chg->regmap, chg->reg[LBC_BAT_IF] + PM8916_PERPH_TYPE, &tmp, 2);
>> +	if (ret)
>> +		goto comm_error;
>> +	if (tmp != PM8916_LBC_BAT_IF_TYPE)
>> +		goto type_error;
>> +
>> +	ret = regmap_bulk_read(chg->regmap, chg->reg[LBC_USB] + PM8916_PERPH_TYPE, &tmp, 2);
>> +	if (ret)
>> +		goto comm_error;
>> +	if (tmp != PM8916_LBC_USB_TYPE)
>> +		goto type_error;
>> +
>> +	ret = regmap_bulk_read(chg->regmap, chg->reg[LBC_MISC] + PM8916_PERPH_TYPE, &tmp, 2);
>> +	if (ret)
>> +		goto comm_error;
>> +	if (tmp != PM8916_LBC_MISC_TYPE)
>> +		goto type_error;
>> +
>> +	ret = regmap_read(chg->regmap, chg->reg[LBC_CHGR] + PM8916_LBC_CHGR_CHG_OPTION, &tmp);
>> +	if (ret)
>> +		goto comm_error;
>> +	if (tmp != PM8916_LBC_CHGR_PMIC_CHARGER)
>> +		dev_err_probe(dev, -ENODEV, "The system is using an external charger\n");
>> +
>> +	ret = pm8916_lbc_charger_probe_dt(chg);
>> +	if (ret)
>> +		dev_err_probe(dev, ret, "Error while parsing device tree\n");
>> +
>> +	psy_cfg.drv_data = chg;
>> +	psy_cfg.of_node = dev->of_node;
>> +
>> +	chg->charger = devm_power_supply_register(dev, &pm8916_lbc_charger_psy_desc, &psy_cfg);
>> +	if (IS_ERR(chg->charger))
>> +		return dev_err_probe(dev, PTR_ERR(chg->charger), "Unable to register charger\n");
>> +
>> +	ret = power_supply_get_battery_info(chg->charger, &chg->info);
>> +	if (ret)
>> +		return dev_err_probe(dev, ret, "Unable to get battery info\n");
>> +
>> +	chg->edev = devm_extcon_dev_allocate(dev, pm8916_lbc_charger_cable);
>> +	if (IS_ERR(chg->edev))
>> +		return PTR_ERR(chg->edev);
>> +
>> +	ret = devm_extcon_dev_register(dev, chg->edev);
>> +	if (ret < 0)
>> +		return dev_err_probe(dev, ret, "failed to register extcon device\n");
>> +
>> +	ret = regmap_read(chg->regmap, chg->reg[LBC_USB] + PM8916_INT_RT_STS, &tmp);
>> +	if (ret)
>> +		goto comm_error;
>> +
>> +	chg->online = !!(tmp & PM8916_LBC_USB_USBIN_VALID);
>> +	extcon_set_state_sync(chg->edev, EXTCON_USB, chg->online);
>> +
>> +	chg->charge_voltage_max = chg->info->voltage_max_design_uv;
>> +	ret = pm8916_lbc_charger_configure(chg);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return 0;
>> +
>> +comm_error:
>> +	return dev_err_probe(dev, ret, "Unable to communicate with device\n");
>> +
>> +type_error:
>> +	return dev_err_probe(dev, -ENODEV, "Device reported wrong type: 0x%X\n", tmp);
>> +}
>> +
>> +static const struct of_device_id pm8916_lbc_charger_of_match[] = {
>> +	{ .compatible = "qcom,pm8916-lbc", },
>> +	{ },
> 
> {},
> 
> (i.e. remove space and trailing, for the terminator entry)
> 

Ack, will remove inner space and trailing comma.

>> +};
>> +MODULE_DEVICE_TABLE(of, pm8916_lbc_charger_of_match);
>> +
>> +static struct platform_driver pm8916_lbc_charger_driver = {
>> +	.driver = {
>> +		.name = "pm8916-lbc",
>> +		.of_match_table = of_match_ptr(pm8916_lbc_charger_of_match),
> 
> .of_match_table = pm8916_lbc_charger_of_match,
> 

Ack

Thanks for the review!

Nikita

>> +	},
>> +	.probe = pm8916_lbc_charger_probe,
>> +};
>> +module_platform_driver(pm8916_lbc_charger_driver);
>> +
>> +MODULE_DESCRIPTION("pm8916 LBC driver");
>> +MODULE_AUTHOR("Nikita Travkin <nikita@...n.ru>");
>> +MODULE_LICENSE("GPL");
> 
> Thanks and sorry for the slow review.
> 
> -- Sebastian

Powered by blists - more mailing lists